-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
THREESCALE-11020 Redis TLS certs and keys for porta and backend #1035
base: master
Are you sure you want to change the base?
Conversation
5414a05
to
2eea781
Compare
1347947
to
d661921
Compare
pkg/3scale/amp/component/system.go
Outdated
|
||
SystemSecretSystemRedisSslCa = "SSL_CA" | ||
SystemSecretSystemRedisSslCert = "SSL_CERT" | ||
SystemSecretSystemRedisSslKey = "SSL_KEY" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valerymo think we need to agree on a naming scheme here as it may be possible that a customer has different certs for Redis and the System-db. Propose that we prefix with CACHE_
as may not be using redis going forward
SystemSecretSystemRedisSslKey = "SSL_KEY" | |
SystemSecretSystemRedisSslCa = "CACHE_SSL_CA" | |
SystemSecretSystemRedisSslCert = "CACHE_SSL_CERT" | |
SystemSecretSystemRedisSslKey = "CACHE_SSL_KEY" |
and I use the prefix SYSTEMDB_
SystemSecretSystemRedisSslKey = "SSL_KEY" | |
SystemSecretSslCa = "SYSTEMDB_SSL_CA" | |
SystemSecretSslCert = "SYSTEMDB_SSL_CERT" | |
SystemSecretSslKey = "SYSTEMDB_SSL_KEY" |
What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austincunningham thanks for comment.
It's Done,
- "REDIS_" prefix was added to "SSL" names that appear in backend-redis and system-redis secrtes
- done in separate commit
- Tested locally
- Validation notes updated
Thanks
921879a
to
0ea718f
Compare
@@ -1535,3 +1537,26 @@ type APIManagerList struct { | |||
func init() { | |||
SchemeBuilder.Register(&APIManager{}, &APIManagerList{}) | |||
} | |||
|
|||
func (apimanager *APIManager) IsSystemRedisTLSEnabled() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the RedisTLSEnabled is a global flag, why do we make a differentiation between system and backend redis here?
Can you have system redis TLS enabled but not worker TLS enabled? Doesn't system app connect to system redis and backend redis - which would to me mean, you can't have TLS on system and not on worker? Not sure about the other way around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for comment @MStokluska . We can't have TLS on System if no TLS on Backend. But it can be vice-a-versa - TLS on Backend but not on System. This is what I see from Jira - System is depended to Backend (system has env vars from backend-redis secret), but Backend is not related to system-redis secret. Hope it's ok. But please tell me if I need check it from Joan. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So based on what you are saying:
Backend TLS - ON and System TLS - OFF is supported.
Backend TLS - OFF and System TLS - ON is not supported.
Based on the Jira description, it looks to me like they actually can be enabled independent of each other since backend is not reliant on system (meaning from backend pov, system SSL can be on or off). System is reliant on backend redis, but there seems to be individual flags for system/backend in system envs:
BACKEND_REDIS_SSL - REDIS_SSL.
Do you agree? Is this how it is currently working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is how it's currently working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks, do we have any indication (apim error or so) to let users know that tls on backend must be enabled if system tls is enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have it in documentation. I reviewed docs to make this is more clear, separate commit.
Thanks
pkg/3scale/amp/component/backend.go
Outdated
@@ -398,6 +437,9 @@ func (backend *Backend) buildBackendWorkerEnv() []v1.EnvVar { | |||
) | |||
} | |||
|
|||
if backend.Options.RedisTLSEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have these in buildBackendCommonEnv - common in my understanding applies to both, worker and listener - is this really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thank you
pkg/3scale/amp/component/backend.go
Outdated
@@ -422,6 +464,9 @@ func (backend *Backend) buildBackendListenerEnv() []v1.EnvVar { | |||
v1.EnvVar{Name: "CONFIG_LISTENER_PROMETHEUS_METRICS_ENABLED", Value: "true"}, | |||
) | |||
} | |||
if backend.Options.RedisTLSEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have these in buildBackendCommonEnv - common in my understanding applies to both, worker and listener - is this really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thank you
pkg/3scale/amp/component/backend.go
Outdated
func (backend *Backend) backendVolumes() []v1.Volume { | ||
res := []v1.Volume{} | ||
|
||
if backend.Options.RedisTLSEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just side note - so are we saying here that Redis TLS can be enabled on worker or listener IF system fails to have TLS enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System TLS certs are coming from both system-redis and backend-redis secrets. System TLS envs BACKEND_REDIS_CA_FILE, BACKEND_REDIS_PRIVATE_KEY - they are populated from Backend-redis secret. So System is depends on Backend TLS definitions.
But Backend TLS is not related to system - all certs are coming from backend-redis.
Hope I understand the quesion. Thank you Michal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I wrote before, as I understand - it can be TLS on backend, but not on system. Backend TLS is not dependent to system, but system TLS is dependent to backend. System is populating BACKEND_REDIS_CLIENT_CERT and BACKEND_REDIS_PRIVATE_KEY from backend-redis secret.
@@ -69,12 +70,14 @@ func (r *RedisOptionsProvider) GetRedisOptions() (*component.RedisOptions, error | |||
r.setPersistentVolumeClaimOptions() | |||
r.setPriorityClassNames() | |||
r.setTopologySpreadConstraints() | |||
//r.setTLSEnabled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned. Thank you
doc/apimanager-reference.md
Outdated
| REDIS_SSL_KEY | The private key for the Redis client certificate | Required to set TLS Redis connection. Only for TLS | | ||
| REDIS_SSL_QUEUES_CA | Redis Queues Certificate Authority (CA) certificate | Required to set TLS Redis connection. Only for TLS | | ||
| REDIS_SSL_QUEUES_CERT | Redis Queues client certificate | Required to set TLS Redis connection. Only for TLS | | ||
| REDIS_SSL_QUEUES_KEY | The private key for the Redis Queues client certificate | Required to set TLS Redis connection. Only for TLS | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you missing SSL_MODE ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @austincunningham ,
I'm not using SSL_MODE
but using REDIS_SSL and BACKEND_REDIS_SSL (for system), CONFIG_REDIS_SSL and CONFIG_QUEUES_SSL (for backend),
as it defined in Task.
These env vars are set in Pods, but not in secret.
They set to true if Any of related env vars defined in secret. This is how it's defined in Task/Requirements,
I rechecked it with Joan - thread https://app.slack.com/client/E030G10V24F/search).
These are also links to porta/backend:
System/porta:
- BACKEND_REDIS_SSL: https://github.com/3scale/porta/blob/master/config/examples/backend_redis.yml#L14
- REDIS_SS: https://github.com/3scale/porta/blob/master/config/examples/redis.yml#L13
Backend/apisonator:
- CONFIG_REDIS_SSL: https://github.com/3scale/apisonator/blob/master/openshift/3scale_backend.conf#L44
- CONFIG_QUEUES_SSL: https://github.com/3scale/apisonator/blob/master/openshift/3scale_backend.conf#L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that is the same as ssl mode.
Although not sure if the same tls values are available for REDIS as are available for MySQL and Postgres
🤔
DATABASE_SSL_MODE. Values: Mysql ref., Postgres ref.
Don't see similar documented in redis docs so guess its ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austincunningham , I'm not using SSL_MODE
but using REDIS_SSL and BACKEND_REDIS_SSL (for system), CONFIG_REDIS_SSL and CONFIG_QUEUES_SSL (for backend),
as it defined in Task.
These env vars are set in pods, but not in secret. And these env vars are used in Porta and apisonator (links below)
These env vars are set to true if Any of related env vars defined in secret.
I rechecked it with Joan - thread https://app.slack.com/client/E030G10V24F/search).
These are also links to porta/backend:
System/porta:
- BACKEND_REDIS_SSL: https://github.com/3scale/porta/blob/master/config/examples/backend_redis.yml#L14
- REDIS_SS: https://github.com/3scale/porta/blob/master/config/examples/redis.yml#L13
Backend/apisonator:
- CONFIG_REDIS_SSL: https://github.com/3scale/apisonator/blob/master/openshift/3scale_backend.conf#L44
- CONFIG_QUEUES_SSL: https://github.com/3scale/apisonator/blob/master/openshift/3scale_backend.conf#L27
Thank you
doc/apimanager-reference.md
Outdated
| AppLabel | `appLabel` | string | No | `3scale-api-management` | The value of the `app` label that will be applied to the API management solution | ||
| TenantName | `tenantName` | string | No | `3scale` | Tenant name under the root that Admin UI will be available with -admin suffix. | ||
| **Field** | **json/yaml field**| **Type** | **Required** | **Default value** | **Description** | | ||
| --- | --- | --- | --- | --- |---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goland does this sort of auto edit, I usually edit md files in another ide to stop this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austincunningham , Thanks a lot! I updated sections related to Redis TLS . I used now VSCodium.
Looks like there are more places where extra spaces added, but didn't touch them. Table of contents updated, as VSCodium required this, I checked, seems it's working fine.
doc/operator-user-guide.md
Outdated
- Client Private Key | ||
- TLS certificate files are populated from the **backend-redis** and **system-redis** secrets. | ||
|
||
- The tables below show the mapping between TLS certificate environment variables in the pods, their corresponding definitions in the related Redis backend and system secrets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we should only be documenting the ENV VAR the user has to set and not the ones set my the operator to avoid confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austincunningham thank you for comment. It's User Guide. I thought it could help to User to understand how it's working, and investigate/debug if required. If you strongly disagree I will remove this, but seems to me it can be useful. Thanks
0ea718f
to
a686c2f
Compare
048f9bd
to
3aea74e
Compare
31bfbdd
to
b695c71
Compare
b695c71
to
4a9847c
Compare
@@ -84,6 +84,13 @@ type APIManagerSpec struct { | |||
PodDisruptionBudget *PodDisruptionBudgetSpec `json:"podDisruptionBudget,omitempty"` | |||
// +optional | |||
Monitoring *MonitoringSpec `json:"monitoring,omitempty"` | |||
|
|||
// +optional | |||
SystemRedisTLSEnabled *bool `json:"systemRedisTLSEnabled,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move this to system spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks
// +optional | ||
SystemRedisTLSEnabled *bool `json:"systemRedisTLSEnabled,omitempty"` | ||
// +optional | ||
BackendRedisTLSEnabled *bool `json:"backendRedisTLSEnabled,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move both backend redis tls flags to backend spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks
tlsEnabled := false | ||
if apimanager.Spec.SystemRedisTLSEnabled != nil && | ||
*apimanager.Spec.SystemRedisTLSEnabled && | ||
apimanager.Spec.ExternalComponents != nil && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
external components check should not be here. All installations of 3scale on 2.16 + must use external.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks
tlsEnabled := false | ||
if apimanager.Spec.BackendRedisTLSEnabled != nil && | ||
*apimanager.Spec.BackendRedisTLSEnabled && | ||
apimanager.Spec.ExternalComponents != nil && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
external components check should not be here. All installations of 3scale on 2.16 + must use external.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks
tlsEnabled := false | ||
if apimanager.Spec.QueuesRedisTLSEnabled != nil && | ||
*apimanager.Spec.QueuesRedisTLSEnabled && | ||
apimanager.Spec.ExternalComponents != nil && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
external components check should not be here. All installations of 3scale on 2.16 + must use external.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks
} | ||
|
||
func (a *APIManager) backendRedisTLSValidationBackendSecret() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed . Thanks
func (r *APIManagerReconciler) validateRedisTLSSystemRedisSecret(cr *appsv1alpha1.APIManager) field.ErrorList { | ||
fieldErrors := field.ErrorList{} | ||
fieldErrors = append(fieldErrors, r.validateRedisTLSRedisSecret(cr, "system-redis", | ||
"REDIS_SSL_CA", "REDIS_SSL_CERT", "REDIS_SSL_KEY", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to me we are doubling up the validation.
The validation could be synced with what we have so far when it comes to validation these secrets, which is here:
pkg/3scale/amp/operator/highavailability_options_provider.go
The validateCR function, which uses this function, is used for validation of the apim CR. Not secrets.
Please move the logic of validating the secrets to https://github.com/3scale/3scale-operator/blob/master/pkg/3scale/amp/operator/highavailability_options_provider.go
You can check on what @austincunningham did here: #1026
By doing this, we are validating our secrets early (the first thing that happens after preflights and before reconciling all 3scale components) and in the correct place.
@@ -1,43 +1,46 @@ | |||
# User Guide | |||
|
|||
<!--ts--> | |||
* [User Guide](#user-guide) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what triggered refactor of this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't know, but the doc is updated now, as TLS enabled flags moved to system and backend specs.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to figure out why this has happened?
Are you using the toc generator?
@@ -48,8 +48,6 @@ github.com/Azure/go-autorest/tracing v0.5.0/go.mod h1:r/s2XiOKccPW3HrqB+W0TQzfbt | |||
github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU= | |||
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= | |||
github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= | |||
github.com/MStokluska/3scale-porta-go-client v0.0.0-20250124150520-23225e662421 h1:P/vWCWycdFARt5APkZ6yJs/1DWvkeESSRZZHMl1wkFo= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🤦
} | ||
|
||
// check sentinelHostsFieldName | ||
data, exists := secret.Data[sentinelHostsFieldName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sentinel field can exists as empty - does it mean we are expecting sentinel to be setup correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, enduser can deside not to use Sentinel, same as it was without TLS
if err != nil { | ||
return fmt.Errorf("URL has an invalid structure: %s", host) | ||
} | ||
if parsedURL.Scheme == "rediss" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know for sure that if using sentinels with tls, every host will have rediss?
are there any other scenarios where this can be set to a different value?
In general, I think I would just check if:
- TLS required KEYS are present in secret
- TLS required KEYS have non-empty VALUES
The rest of validation will be performed via preflight + components directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding "every host will have rediss" - Yes, as for example. We want use TLS for client communication with Redis Server. Redis Server - secured. We have 3 Sentinel hosts for Backend (for HA), first - using TLS (rediss), - other - not. If Sentinel is available - Client will not communicate with Redis Server directrly, it will use Sentinel. Secure Sentinel found, and communication established. Secure sentinel failed. Sentinel will do switch to "backup" Sentinel, but it's not secure. Communication - failed. We need prevent this case, and check that If Sentinel hosts defined and We want use TLS - then All Sentine hosts should be secure (rediss://)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validation moved to HA, and only basic validation done now. Thank you
91ff191
to
00923cd
Compare
76ac677
to
f95667d
Compare
f95667d
to
6cdf551
Compare
return false | ||
} | ||
|
||
func ValidateRedisURL(url string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used, I think we can drop this?
return nil | ||
} | ||
|
||
func ValidateRedisSentinelHostsForTLS(sentinelHosts string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used, I think we can drop this?
@@ -927,9 +1059,10 @@ func (system *System) SidekiqDeployment(containerImage string) *k8sappsv1.Deploy | |||
Command: []string{ | |||
"bash", | |||
"-c", | |||
"bundle exec sh -c \"until rake boot:redis && curl --output /dev/null --silent --fail --head http://system-master:3000/status; do sleep $SLEEP_SECONDS; done\"", | |||
"bundle exec sh -c \"until rake boot:redis && curl --insecure --output /dev/null --silent --fail --head http://system-master:3000/status; do sleep $SLEEP_SECONDS; done\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we bypassing ssl cert validation here?
Jira: https://issues.redhat.com/browse/THREESCALE-11020
Add a way for the user to provide Redis TLS certs and keys for porta and backend
This PR enables Porta and Apisonator to load TLS configuration details for connecting to Redis. It introduces new environment variables that specify the locations of certificate files and indicate whether TLS mode is enabled.
The PR includes validation for Redis certificate-related fields and Redis URLs within the system-redis and backend-redis secrets, ensuring proper TLS communication.
Additionally, this PR validates the Sentinel Hosts to ensure they match the master Redis configuration defined in the APIManager CRs for system, backend, and queues.
Documentation has been updated to reflect these changes.
Validation
1. Install Redis Server for Test
2. Certificates preparing
3. Update Redis Server with new server certificate
Update
redis-tls-secret
secret, using new created:Restart redis pod
4. Install 3scale
You may prepare the secrets in whichever way is most convenient for you.
Update Client Certificates in
system-redis
andbackedn-redis
secrets via UI. The following tables are for matching data field names with the certificate files created before:Secret: system-redis
Please note:
rediss
indicates a secure connection and port6380
is used for secure Redis connections. For example:4.2. Create s3-credentials secret
4.3. Create APIManager CR and Run Operator
5 Check results
5.1 Check Environment Variables and Certificates in Pods
System pods: system-sidekiq and system-app
and the certificate files are populated for the System:
6 TLS - Test Cases
In the previous sections, we provided details for testing of following scenario:
Test 1 - negative
Test 2 - negative
Test 3 - negative
rediss
Test4
1. APIManagerCR: Redis TLS is Enabled for system, backend and queues.
2. backend-redis secret - TLS fields are populated correctly
3. system-redis secrets - TLS fields are empty
Installation completed successfully, All pods are running no errors.