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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
