[GitHub] [lucene-solr-operator] HoustonPutman commented on a change in pull request #209: Basic TLS Support

[GitBox <gi...@apache.org>]

HoustonPutman commented on a change in pull request #209:
URL: https://github.com/apache/lucene-solr-operator/pull/209#discussion_r570530780



##########
File path: controllers/solrcloud_controller.go
##########
@@ -381,7 +436,7 @@ func (r *SolrCloudReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 
 	if !reflect.DeepEqual(instance.Status, newStatus) {
 		instance.Status = newStatus
-		logger.Info("Updating SolrCloud Status")
+		r.Log.Info("Updating SolrCloud Status: ", "namespace", instance.Namespace, "name", instance.Name, "status", instance.Status)

Review comment:
       can you keep the logger.Info() and just add the "status" part? It'll keep the logging consistent.

##########
File path: controllers/solrcloud_controller.go
##########
@@ -76,6 +77,8 @@ func UseZkCRD(useCRD bool) {
 // +kubebuilder:rbac:groups=solr.apache.org,resources=solrclouds/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=zookeeper.pravega.io,resources=zookeeperclusters,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=zookeeper.pravega.io,resources=zookeeperclusters/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete

Review comment:
       For secrets, we should be able to get away with: `verbs=get;list;watch`. I think...

##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -971,3 +996,47 @@ type SolrCloudList struct {
 func init() {
 	SchemeBuilder.Register(&SolrCloud{}, &SolrCloudList{})
 }
+
+// +kubebuilder:validation:Enum=None;Want;Need
+type ClientAuthType string
+
+const (
+	None ClientAuthType = "None"
+	Want ClientAuthType = "Want"
+	Need ClientAuthType = "Need"
+)
+
+type SolrTLSOptions struct {
+	// TLS Secret containing a pkcs12 keystore
+	PKCS12Secret *corev1.SecretKeySelector `json:"pkcs12Secret"`
+
+	// Secret containing the key store password; this field is required as most JVMs do not support pkcs12 keystores without a password
+	KeyStorePasswordSecret *corev1.SecretKeySelector `json:"keyStorePasswordSecret"`
+
+	// Determines the client authentication method, either None, Want, or Need;
+	// this affects K8s ability to call liveness / readiness probes so use cautiously.
+	// +optional

Review comment:
       You can use `// +kubebuilder:default=None`, and get rid of the `withDefaults()` method.

##########
File path: controllers/solrcloud_controller.go
##########
@@ -261,12 +263,65 @@ 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 {
+		ctx := context.TODO()
+		foundTLSSecret := &corev1.Secret{}
+		lookupErr := r.Get(ctx, types.NamespacedName{Name: instance.Spec.SolrTLS.PKCS12Secret.Name, Namespace: instance.Namespace}, foundTLSSecret)
+		if lookupErr != nil {
+			return requeueOrNot, lookupErr
+		} else {
+			// Make sure the secret containing the keystore password exists as well
+			if foundTLSSecret != nil {
+				keyStorePasswordSecret := &corev1.Secret{}
+				err := r.Get(ctx, types.NamespacedName{Name: instance.Spec.SolrTLS.KeyStorePasswordSecret.Name, Namespace: foundTLSSecret.Namespace}, keyStorePasswordSecret)
+				if err != nil {
+					return requeueOrNot, lookupErr
+				}
+			}
+
+			// 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 {
+				tlsCertBytes, ok := foundTLSSecret.Data["tls.crt"]
+				if ok {
+					tlsCertMd5 = fmt.Sprintf("%x", md5.Sum(tlsCertBytes))
+				} else {
+					r.Log.Error(err, "tls.crt key not found TLS secret", "name", foundTLSSecret.Name)
+					return requeueOrNot, nil
+				}
+			}
+
+			if _, ok := foundTLSSecret.Data[instance.Spec.SolrTLS.PKCS12Secret.Key]; !ok {
+				// the keystore.p12 key is not in the TLS secret, indicating we need to create it using an initContainer
+				needsPkcs12InitContainer = true
+			}
+		}
+
+		// create a job to set the cluster property but we need a ZK connection first
+		urlSchemeJob := util.GenerateUrlSchemeJob(instance, &newStatus)

Review comment:
       Does this have to happen before the Solr nodes are started?

##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -52,6 +52,10 @@ const (
 
 	SolrTechnologyLabel      = "solr-cloud"
 	ZookeeperTechnologyLabel = "zookeeper"
+
+	DefaultKeyStorePath         = "/var/solr/tls"

Review comment:
       These can go in solr_util.go right? I don't think they are used in the API.

##########
File path: controllers/solrcloud_controller.go
##########
@@ -716,7 +768,9 @@ func (r *SolrCloudReconciler) SetupWithManagerAndReconciler(mgr ctrl.Manager, re
 		Owns(&corev1.ConfigMap{}).
 		Owns(&appsv1.StatefulSet{}).
 		Owns(&corev1.Service{}).
-		Owns(&extv1.Ingress{})
+		Owns(&extv1.Ingress{}).
+		Owns(&corev1.Secret{}).

Review comment:
       We just watch secrets, not create them I think. This will require that lovely `indexAndWatchForTlsSecrets()` method. I can handle this though!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org