You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/08/17 20:10:23 UTC
[GitHub] [solr-operator] thelabdude opened a new pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
thelabdude opened a new pull request #309:
URL: https://github.com/apache/solr-operator/pull/309
Fixes #308
Consolidated most of the TLS related utility code into a new file in the `util` package: `solr_tls_util.go`
All the TLS related config (env vars, annotations, initContainers, volumes/mounts) all get appended to the config for a `StatefulSet` or `Deployment` in one step at the end vs. being infused at various places in `GenerateStatefulSet` and `GenerateSolrPrometheusExporterDeployment`. I like this approach because all the TLS config is mostly additive. There are a few places where we have to be careful about overriding previous settings (such as the `entrypoint` for the exporter when using the `mountedServerTLSDir` but that's only in a few small places)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690739575
##########
File path: controllers/solrcloud_controller.go
##########
@@ -364,51 +364,57 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
blockReconciliationOfStatefulSet = true
}
- tlsCertMd5 := ""
- needsPkcs12InitContainer := false // flag if the StatefulSet needs an additional initCont to create PKCS12 keystore
// don't start reconciling TLS until we have ZK connectivity, avoids TLS code having to check for ZK
- if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil && instance.Spec.SolrTLS.PKCS12Secret != nil {
- foundTLSSecret, err := r.verifyTLSSecretConfig(instance.Spec.SolrTLS.PKCS12Secret.Name, instance.Namespace, instance.Spec.SolrTLS.KeyStorePasswordSecret)
- if err != nil {
- return requeueOrNot, err
- } else {
- // We have a watch on secrets, so will get notified when the secret changes (such as after cert renewal)
- // capture the hash of the secret and stash in an annotation so that pods get restarted if the cert changes
- if instance.Spec.SolrTLS.RestartOnTLSSecretUpdate {
- if tlsCertBytes, ok := foundTLSSecret.Data[util.TLSCertKey]; ok {
- tlsCertMd5 = fmt.Sprintf("%x", md5.Sum(tlsCertBytes))
- } else {
- return requeueOrNot, fmt.Errorf("%s key not found in TLS secret %s, cannot watch for updates to"+
- " the cert without this data but 'solrTLS.restartOnTLSSecretUpdate' is enabled!",
- util.TLSCertKey, foundTLSSecret.Name)
+ var tls *util.TLSConfig
+ if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil {
+ tls = &util.TLSConfig{}
Review comment:
`TLSConfig` is fine with me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690688351
##########
File path: controllers/controller_utils_test.go
##########
@@ -190,14 +190,6 @@ func verifyUserSuppliedTLSConfig(t *testing.T, tls *solr.SolrTLSOptions, expecte
assert.Equal(t, expectedKeystorePasswordSecretKey, tls.KeyStorePasswordSecret.Key)
assert.Equal(t, expectedTlsSecretName, tls.PKCS12Secret.Name)
assert.Equal(t, "keystore.p12", tls.PKCS12Secret.Key)
-
- // is there a separate truststore?
- expectedTrustStorePath := ""
- if tls.TrustStoreSecret != nil {
- expectedTrustStorePath = util.DefaultTrustStorePath + "/" + tls.TrustStoreSecret.Key
- }
-
- expectTLSEnvVars(t, util.TLSEnvVars(tls, needsPkcs12InitContainer), expectedKeystorePasswordSecretName, expectedKeystorePasswordSecretKey, needsPkcs12InitContainer, expectedTrustStorePath)
Review comment:
We don't need to call `expectTLSEnvVars` in this part of the test code as it already gets called after reconciliation, see `expectTLSConfigOnPodTemplate` and `expectMountedTLSDirConfigOnPodTemplate`. Removing this code allows us to hide the `TLSEnvVars` in the util package, it doesn't need to be exposed here.
##########
File path: controllers/solrcloud_controller.go
##########
@@ -215,15 +215,15 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
// if there's a user-provided config, it must have one of the expected keys
if !hasLogXml && !hasSolrXml {
// TODO: Create event for the CRD.
- return requeueOrNot, fmt.Errorf("User provided ConfigMap %s must have one of 'solr.xml' and/or 'log4j2.xml'",
+ return requeueOrNot, fmt.Errorf("user provided ConfigMap %s must have one of 'solr.xml' and/or 'log4j2.xml'",
Review comment:
Just cleaning up some IDE nits here ... doesn't need to be in this PR but shouldn't hurt either ;-)
##########
File path: controllers/solrcloud_controller_tls_test.go
##########
@@ -126,7 +126,6 @@ func TestMountedTLSDir(t *testing.T) {
mountedDir := &solr.MountedTLSDirectory{}
mountedDir.Path = "/mounted-tls-dir"
instance.Spec.SolrTLS = &solr.SolrTLSOptions{MountedServerTLSDir: mountedDir, CheckPeerName: true, ClientAuth: "Need", VerifyClientHostname: true}
- expectMountedTLSDirEnvVars(t, util.TLSEnvVars(instance.Spec.SolrTLS, false))
Review comment:
the TLS env vars will get checked in the `expectMountedTLSDirConfigOnPodTemplate` after reconciliation, so no need to do here ... same comment for change below this too ...
##########
File path: controllers/util/solr_util.go
##########
@@ -601,6 +552,11 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Enrich the StatefulSet config to enable TLS on Solr pods if needed
+ if tls != nil {
+ tls.enableTLSOnSolrCloudStatefulSet(stateful)
Review comment:
This is the crux of the design I took for this refactoring task. Basically, take a configured StatefulSet and then enrich it with all the TLS things in one go vs. spread throughout the method as it was before.
##########
File path: controllers/solrcloud_controller.go
##########
@@ -364,51 +364,57 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
blockReconciliationOfStatefulSet = true
}
- tlsCertMd5 := ""
- needsPkcs12InitContainer := false // flag if the StatefulSet needs an additional initCont to create PKCS12 keystore
// don't start reconciling TLS until we have ZK connectivity, avoids TLS code having to check for ZK
- if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil && instance.Spec.SolrTLS.PKCS12Secret != nil {
- foundTLSSecret, err := r.verifyTLSSecretConfig(instance.Spec.SolrTLS.PKCS12Secret.Name, instance.Namespace, instance.Spec.SolrTLS.KeyStorePasswordSecret)
- if err != nil {
- return requeueOrNot, err
- } else {
- // We have a watch on secrets, so will get notified when the secret changes (such as after cert renewal)
- // capture the hash of the secret and stash in an annotation so that pods get restarted if the cert changes
- if instance.Spec.SolrTLS.RestartOnTLSSecretUpdate {
- if tlsCertBytes, ok := foundTLSSecret.Data[util.TLSCertKey]; ok {
- tlsCertMd5 = fmt.Sprintf("%x", md5.Sum(tlsCertBytes))
- } else {
- return requeueOrNot, fmt.Errorf("%s key not found in TLS secret %s, cannot watch for updates to"+
- " the cert without this data but 'solrTLS.restartOnTLSSecretUpdate' is enabled!",
- util.TLSCertKey, foundTLSSecret.Name)
+ var tls *util.TLSConfig
+ if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil {
+ tls = &util.TLSConfig{}
Review comment:
The `TLSConfig` struct allows us to hold the `TLSOptions` that from from the user config as well as additional config info determined during reconciliation, such as the MD5 hash of the cert. Not married to the name of this struct ... could be `TLSOptionsAndReconciledVars`
##########
File path: controllers/solrprometheusexporter_controller.go
##########
@@ -197,28 +197,30 @@ func (r *SolrPrometheusExporterReconciler) Reconcile(req ctrl.Request) (ctrl.Res
prometheusExporter.Spec.SolrReference.SolrTLS.KeyStorePasswordSecret.Key, keyStorePasswordSecret.Name)
}
- tlsClientOptions = &util.TLSClientOptions{}
Review comment:
The `TLSClientOptions` name was misleading once I repurposed this struct to be used by the exporter and the StatefulSet code
##########
File path: controllers/util/prometheus_exporter_util.go
##########
@@ -46,17 +45,9 @@ type SolrConnectionInfo struct {
StandaloneAddress string
}
-// Used internally to capture config needed to provided Solr client apps like the exporter
-// with config needed to call TLS enabled Solr pods
-type TLSClientOptions struct {
Review comment:
Replaced by `TLSConfig` in `solr_tls_util.go`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690703569
##########
File path: controllers/solrcloud_controller.go
##########
@@ -364,51 +364,57 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
blockReconciliationOfStatefulSet = true
}
- tlsCertMd5 := ""
- needsPkcs12InitContainer := false // flag if the StatefulSet needs an additional initCont to create PKCS12 keystore
// don't start reconciling TLS until we have ZK connectivity, avoids TLS code having to check for ZK
- if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil && instance.Spec.SolrTLS.PKCS12Secret != nil {
- foundTLSSecret, err := r.verifyTLSSecretConfig(instance.Spec.SolrTLS.PKCS12Secret.Name, instance.Namespace, instance.Spec.SolrTLS.KeyStorePasswordSecret)
- if err != nil {
- return requeueOrNot, err
- } else {
- // We have a watch on secrets, so will get notified when the secret changes (such as after cert renewal)
- // capture the hash of the secret and stash in an annotation so that pods get restarted if the cert changes
- if instance.Spec.SolrTLS.RestartOnTLSSecretUpdate {
- if tlsCertBytes, ok := foundTLSSecret.Data[util.TLSCertKey]; ok {
- tlsCertMd5 = fmt.Sprintf("%x", md5.Sum(tlsCertBytes))
- } else {
- return requeueOrNot, fmt.Errorf("%s key not found in TLS secret %s, cannot watch for updates to"+
- " the cert without this data but 'solrTLS.restartOnTLSSecretUpdate' is enabled!",
- util.TLSCertKey, foundTLSSecret.Name)
+ var tls *util.TLSConfig
+ if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil {
+ tls = &util.TLSConfig{}
Review comment:
The `TLSConfig` struct allows us to hold the `TLSOptions` that from from the user config as well as additional config info determined during reconciliation, such as the MD5 hash of the cert. Not married to the name of this struct ... could be `TLSOptionsAndReconciledVars`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690697734
##########
File path: controllers/util/solr_util.go
##########
@@ -601,6 +552,11 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Enrich the StatefulSet config to enable TLS on Solr pods if needed
+ if tls != nil {
+ tls.enableTLSOnSolrCloudStatefulSet(stateful)
Review comment:
This is the crux of the design I took for this refactoring task. Basically, take a configured StatefulSet and then enrich it with all the TLS things in one go vs. spread throughout the method as it was before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690689552
##########
File path: controllers/solrcloud_controller_tls_test.go
##########
@@ -126,7 +126,6 @@ func TestMountedTLSDir(t *testing.T) {
mountedDir := &solr.MountedTLSDirectory{}
mountedDir.Path = "/mounted-tls-dir"
instance.Spec.SolrTLS = &solr.SolrTLSOptions{MountedServerTLSDir: mountedDir, CheckPeerName: true, ClientAuth: "Need", VerifyClientHostname: true}
- expectMountedTLSDirEnvVars(t, util.TLSEnvVars(instance.Spec.SolrTLS, false))
Review comment:
the TLS env vars will get checked in the `expectMountedTLSDirConfigOnPodTemplate` after reconciliation, so no need to do here ... same comment for change below this too ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude merged pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude merged pull request #309:
URL: https://github.com/apache/solr-operator/pull/309
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690705380
##########
File path: controllers/util/prometheus_exporter_util.go
##########
@@ -46,17 +45,9 @@ type SolrConnectionInfo struct {
StandaloneAddress string
}
-// Used internally to capture config needed to provided Solr client apps like the exporter
-// with config needed to call TLS enabled Solr pods
-type TLSClientOptions struct {
Review comment:
Replaced by `TLSConfig` in `solr_tls_util.go`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690739265
##########
File path: controllers/util/solr_util.go
##########
@@ -601,6 +552,11 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Enrich the StatefulSet config to enable TLS on Solr pods if needed
+ if tls != nil {
+ tls.enableTLSOnSolrCloudStatefulSet(stateful)
Review comment:
Yeah this is great. Hopefully we can abstract out other parts of this awful method and make it nice and clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690688691
##########
File path: controllers/solrcloud_controller.go
##########
@@ -215,15 +215,15 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
// if there's a user-provided config, it must have one of the expected keys
if !hasLogXml && !hasSolrXml {
// TODO: Create event for the CRD.
- return requeueOrNot, fmt.Errorf("User provided ConfigMap %s must have one of 'solr.xml' and/or 'log4j2.xml'",
+ return requeueOrNot, fmt.Errorf("user provided ConfigMap %s must have one of 'solr.xml' and/or 'log4j2.xml'",
Review comment:
Just cleaning up some IDE nits here ... doesn't need to be in this PR but shouldn't hurt either ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690704848
##########
File path: controllers/solrprometheusexporter_controller.go
##########
@@ -197,28 +197,30 @@ func (r *SolrPrometheusExporterReconciler) Reconcile(req ctrl.Request) (ctrl.Res
prometheusExporter.Spec.SolrReference.SolrTLS.KeyStorePasswordSecret.Key, keyStorePasswordSecret.Name)
}
- tlsClientOptions = &util.TLSClientOptions{}
Review comment:
The `TLSClientOptions` name was misleading once I repurposed this struct to be used by the exporter and the StatefulSet code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] thelabdude commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690688351
##########
File path: controllers/controller_utils_test.go
##########
@@ -190,14 +190,6 @@ func verifyUserSuppliedTLSConfig(t *testing.T, tls *solr.SolrTLSOptions, expecte
assert.Equal(t, expectedKeystorePasswordSecretKey, tls.KeyStorePasswordSecret.Key)
assert.Equal(t, expectedTlsSecretName, tls.PKCS12Secret.Name)
assert.Equal(t, "keystore.p12", tls.PKCS12Secret.Key)
-
- // is there a separate truststore?
- expectedTrustStorePath := ""
- if tls.TrustStoreSecret != nil {
- expectedTrustStorePath = util.DefaultTrustStorePath + "/" + tls.TrustStoreSecret.Key
- }
-
- expectTLSEnvVars(t, util.TLSEnvVars(tls, needsPkcs12InitContainer), expectedKeystorePasswordSecretName, expectedKeystorePasswordSecretKey, needsPkcs12InitContainer, expectedTrustStorePath)
Review comment:
We don't need to call `expectTLSEnvVars` in this part of the test code as it already gets called after reconciliation, see `expectTLSConfigOnPodTemplate` and `expectMountedTLSDirConfigOnPodTemplate`. Removing this code allows us to hide the `TLSEnvVars` in the util package, it doesn't need to be exposed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #309: Refactor TLS setup code into a separate file and consolidate logic between the StatefulSet and exporter Deployment
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #309:
URL: https://github.com/apache/solr-operator/pull/309#discussion_r690739265
##########
File path: controllers/util/solr_util.go
##########
@@ -601,6 +552,11 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Enrich the StatefulSet config to enable TLS on Solr pods if needed
+ if tls != nil {
+ tls.enableTLSOnSolrCloudStatefulSet(stateful)
Review comment:
Yeah this is great. Hopefully we can abstract out other parts of this awful method and make it nice and clean.
##########
File path: controllers/solrcloud_controller.go
##########
@@ -364,51 +364,57 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
blockReconciliationOfStatefulSet = true
}
- tlsCertMd5 := ""
- needsPkcs12InitContainer := false // flag if the StatefulSet needs an additional initCont to create PKCS12 keystore
// don't start reconciling TLS until we have ZK connectivity, avoids TLS code having to check for ZK
- if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil && instance.Spec.SolrTLS.PKCS12Secret != nil {
- foundTLSSecret, err := r.verifyTLSSecretConfig(instance.Spec.SolrTLS.PKCS12Secret.Name, instance.Namespace, instance.Spec.SolrTLS.KeyStorePasswordSecret)
- if err != nil {
- return requeueOrNot, err
- } else {
- // We have a watch on secrets, so will get notified when the secret changes (such as after cert renewal)
- // capture the hash of the secret and stash in an annotation so that pods get restarted if the cert changes
- if instance.Spec.SolrTLS.RestartOnTLSSecretUpdate {
- if tlsCertBytes, ok := foundTLSSecret.Data[util.TLSCertKey]; ok {
- tlsCertMd5 = fmt.Sprintf("%x", md5.Sum(tlsCertBytes))
- } else {
- return requeueOrNot, fmt.Errorf("%s key not found in TLS secret %s, cannot watch for updates to"+
- " the cert without this data but 'solrTLS.restartOnTLSSecretUpdate' is enabled!",
- util.TLSCertKey, foundTLSSecret.Name)
+ var tls *util.TLSConfig
+ if !blockReconciliationOfStatefulSet && instance.Spec.SolrTLS != nil {
+ tls = &util.TLSConfig{}
Review comment:
`TLSConfig` is fine with me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org