-
Notifications
You must be signed in to change notification settings - Fork 95
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-11156 add tls env var to zync and system deployments #1026
base: master
Are you sure you want to change the base?
Conversation
5417862
to
906d087
Compare
58f5423
to
cec4cf1
Compare
694a709
to
7646db2
Compare
/test test-e2e |
52d6f47
to
de4820b
Compare
/test test-e2e |
8c46579
to
f626b13
Compare
f626b13
to
812d331
Compare
bc1e44b
to
a5fd165
Compare
a5fd165
to
f0af6cb
Compare
f0af6cb
to
d7eecd7
Compare
THREESCALE-11156 add volume mounts for secrets THREESCALE-11156 fix default internal to work without tls THREESCALE-11156 set cert permissions THREESCALE-11156 add apimanger flags for TLS THREESCALE-11156 fix e2e THREESCALE-11156 add Database TLS doc THREESCALE-11156 add watchby logic THREESCALE-11156 removed internal database changes
d7eecd7
to
247a2e0
Compare
@@ -84,6 +84,10 @@ type APIManagerSpec struct { | |||
PodDisruptionBudget *PodDisruptionBudgetSpec `json:"podDisruptionBudget,omitempty"` | |||
// +optional | |||
Monitoring *MonitoringSpec `json:"monitoring,omitempty"` | |||
// +optional | |||
SystemDatabaseTLSEnabled *bool `json:"systemDatabaseTLSEnabled,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.
if it's not a problem, please move this to system spec and zync spec respectively. Not a big deal if you prefer this is a global.
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 believe it would be a problem to move. It's preference really. And it doesn't matter to me where it lives .
helper.EnvVarFromSecretOptional("DB_SSL_CERT", SystemSecretSystemDatabaseSecretName, SystemSecretSslCert), | ||
helper.EnvVarFromSecretOptional("DB_SSL_KEY", SystemSecretSystemDatabaseSecretName, SystemSecretSslKey), | ||
helper.EnvVarFromSecretOptional("DATABASE_SSL_MODE", SystemSecretSystemDatabaseSecretName, SystemSecretDatabaseSslMode), | ||
helper.EnvVarFromValue("DATABASE_SSL_CA", helper.TlsCertPresent("DATABASE_SSL_CA", SystemSecretSystemDatabaseSecretName, system.Options.SystemDbTLSEnabled)), |
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.
as discussed, let's move the creation of empty keys + validation of keys + validation of values here: https://github.com/3scale/3scale-operator/blob/master/pkg/3scale/amp/operator/highavailability_options_provider.go#L29
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.
Agree its better if the reconciler catches 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.
Yes the secret validation should check if the secret values are populated in the highavailability reconcile and populate with an empty string if not set and return an error.
Two things here
- It doesn't look like highavailability_options_provider GetHighAvailabilityOptions() handles zync database at the moment (maybe because spec.highavaibility is deprecated). Can add the functionally but am now doubting this is the correct place to do so.
- And may still leave the helper to populate the env vars that are not available in the secret .
i.e. the env vars in the secret have actual certs where as the env var that are used have paths to certs
MountPath: "/var/lib/searchd", | ||
}, | ||
}, | ||
//Env: s.commonSearchdEnvVars(), |
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.
Think it was commented during some debugging . So I would say its 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.
having reviewed its not required.
Issue
THREESCALE-11156
What
Add database client side tls to 3scale
Verification
DATABASE_SSL_MODE=disable