Merge pull request #1990 from oliashish/fix/cert_change_detection_nodeset

Detect TLS cert secret changes in NodeSet reconciler

Author: openshift-merge-bot[bot]

internal/controller/dataplane/openstackdataplanenodeset_controller.go

@@ -596,6 +596,16 @@ func checkDeployment(ctx context.Context, helper *helper.Helper,
 				continue
 			}
 
+			hasCertSecretsChanged, err := checkCertSecretsChanged(ctx, helper, instance, deployment.Status.SecretHashes)
+
+			if err != nil {
+				return isNodeSetDeploymentReady, isNodeSetDeploymentRunning, isNodeSetDeploymentFailed, failedDeploymentName, err
+			}
+
+			if hasCertSecretsChanged {
+				continue
+			}
+
 			isNodeSetDeploymentReady = true
 			for k, v := range deployment.Status.ConfigMapHashes {
 				instance.Status.ConfigMapHashes[k] = v
@@ -853,6 +863,26 @@ func (r *OpenStackDataPlaneNodeSetReconciler) secretWatcherFn(
 	ctx context.Context, obj client.Object,
 ) []reconcile.Request {
 	Log := r.GetLogger(ctx)
+
+	// Check if this is a cert secret (has NodeSet, Service, and ServiceKey labels).
+	// Cert secrets created by EnsureTLSCerts carry these labels but are not
+	// listed in AnsibleVarsFrom, so the field-index lookup below would miss them.
+	labels := obj.GetLabels()
+	if nodeSetName, ok := labels[deployment.NodeSetLabel]; ok {
+		_, hasSvcLabel := labels[deployment.ServiceLabel]
+		_, hasSvcKeyLabel := labels[deployment.ServiceKeyLabel]
+		if hasSvcLabel && hasSvcKeyLabel {
+			Log.Info(fmt.Sprintf("reconcile loop for openstackdataplanenodeset %s triggered by cert secret %s",
+				nodeSetName, obj.GetName()))
+			return []reconcile.Request{{
+				NamespacedName: types.NamespacedName{
+					Namespace: obj.GetNamespace(),
+					Name:      nodeSetName,
+				},
+			}}
+		}
+	}
+
 	nodeSets := &dataplanev1.OpenStackDataPlaneNodeSetList{}
 	kind := strings.ToLower(obj.GetObjectKind().GroupVersionKind().Kind)
 	selector := "spec.ansibleVarsFrom.ansible.configMaps"
@@ -986,3 +1016,31 @@ func checkAnsibleVarsFromChanged(
 
 	return false, nil
 }
+
+// checkCertSecretsChanged computes current hashes for TLS cert secrets
+// belonging to the NodeSet and compares them with deployed hashes.
+// Returns true if any cert secret content has changed, false otherwise.
+func checkCertSecretsChanged(
+	ctx context.Context,
+	helper *helper.Helper,
+	instance *dataplanev1.OpenStackDataPlaneNodeSet,
+	deployedSecretHashes map[string]string,
+) (bool, error) {
+	currentCertHashes, err := deployment.GetCertSecretHashes(ctx, helper, instance.Namespace, instance.Name)
+	if err != nil {
+		return false, err
+	}
+
+	// Only flag secrets that were previously deployed and have changed content.
+	// New cert secrets (not yet in deployedSecretHashes) are not flagged here
+	// because adding a new TLS service changes instance.Spec, which is already
+	// detected by the ConfigHash comparison earlier in checkDeployment.
+	for name, currentHash := range currentCertHashes {
+		if deployedHash, exists := deployedSecretHashes[name]; exists && deployedHash != currentHash {
+			helper.GetLogger().Info("Cert secret content changed", "secret", name)
+			return true, nil
+		}
+	}
+
+	return false, nil
+}

internal/dataplane/hashes.go

@@ -139,3 +139,36 @@ func ProcessAnsibleVarsFrom(
 	}
 	return nil
 }
+
+// GetCertSecretHashes returns a map of secret-name to hash for all TLS cert
+// secrets belonging to the given NodeSet. Cert secrets are identified by having
+// both the NodeSetLabel and ServiceLabel labels set by EnsureTLSCerts.
+func GetCertSecretHashes(
+	ctx context.Context,
+	helper *helper.Helper,
+	namespace string,
+	nodeSetName string,
+) (map[string]string, error) {
+	labelSelector := map[string]string{
+		NodeSetLabel: nodeSetName,
+	}
+	secrets, err := secret.GetSecrets(ctx, helper, namespace, labelSelector)
+	if err != nil {
+		return nil, err
+	}
+
+	certHashes := make(map[string]string)
+	for i := range secrets.Items {
+		sec := &secrets.Items[i]
+		if _, hasSvcLabel := sec.Labels[ServiceLabel]; !hasSvcLabel {
+			continue
+		}
+		hash, err := secret.Hash(sec)
+		if err != nil {
+			helper.GetLogger().Error(err, "Unable to hash cert Secret", "secret", sec.Name)
+			return nil, err
+		}
+		certHashes[sec.Name] = hash
+	}
+	return certHashes, nil
+}

test/functional/dataplane/openstackdataplanenodeset_controller_test.go

@@ -31,6 +31,7 @@ import (
 	. "github.com/openstack-k8s-operators/lib-common/modules/common/test/helpers"
 	"gopkg.in/yaml.v3"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
 )
@@ -1805,6 +1806,104 @@ var _ = Describe("Dataplane NodeSet Test", func() {
 			})
 		})
 
+		When("Cert secret changes are detected after deployment", func() {
+			var certSecretName types.NamespacedName
+			var certTestServiceName types.NamespacedName
+
+			BeforeEach(func() {
+				certSecretName = types.NamespacedName{
+					Name:      "cert-cert-test-service-default-edpm-compute-node-1",
+					Namespace: namespace,
+				}
+				certTestServiceName = types.NamespacedName{
+					Name:      "cert-test-service",
+					Namespace: namespace,
+				}
+
+				nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")
+				nodeSetSpec["preProvisioned"] = true
+				nodeSetSpec["tlsEnabled"] = false
+				nodeSetSpec["services"] = []string{"cert-test-service"}
+
+				DeferCleanup(th.DeleteInstance, CreateDataPlaneServiceFromSpec(certTestServiceName, map[string]interface{}{
+					"playbook": "test",
+					"tlsCerts": map[string]interface{}{
+						"default": map[string]interface{}{
+							"contents": []string{"dnsnames"},
+						},
+					},
+				}))
+
+				// Create a cert secret with the labels that EnsureTLSCerts sets.
+				// Labels must match the service name and certKey so that
+				// GetDeploymentHashesForService picks them up.
+				certSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      certSecretName.Name,
+						Namespace: certSecretName.Namespace,
+						Labels: map[string]string{
+							"osdpns":                dataplaneNodeSetName.Name,
+							"osdp-service":          certTestServiceName.Name,
+							"osdp-service-cert-key": "default",
+							"hostname":              "edpm-compute-node-1",
+						},
+					},
+					Data: map[string][]byte{
+						"tls.crt": []byte("original-cert-data"),
+						"tls.key": []byte("original-key-data"),
+						"ca.crt":  []byte("original-ca-data"),
+					},
+				}
+				Expect(th.K8sClient.Create(th.Ctx, certSecret)).To(Succeed())
+				DeferCleanup(th.K8sClient.Delete, th.Ctx, certSecret)
+
+				DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
+				DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
+				DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
+				DeferCleanup(th.DeleteInstance, CreateDataplaneDeployment(dataplaneDeploymentName, DefaultDataPlaneDeploymentSpec()))
+				CreateSSHSecret(dataplaneSSHSecretName)
+				CreateCABundleSecret(caBundleSecretName)
+				SimulateDNSMasqComplete(dnsMasqName)
+				SimulateIPSetComplete(dataplaneNodeName)
+				SimulateDNSDataComplete(dataplaneNodeSetName)
+			})
+
+			It("Should detect cert renewal and mark NodeSet as needing redeployment", func() {
+				// Complete the deployment
+				Eventually(func(g Gomega) {
+					ansibleeeName := types.NamespacedName{
+						Name:      "cert-test-service-" + dataplaneDeploymentName.Name + "-" + dataplaneNodeSetName.Name,
+						Namespace: namespace,
+					}
+					ansibleEE := GetAnsibleee(ansibleeeName)
+					ansibleEE.Status.Succeeded = 1
+					g.Expect(th.K8sClient.Status().Update(th.Ctx, ansibleEE)).To(Succeed())
+				}, th.Timeout, th.Interval).Should(Succeed())
+
+				// Wait for deployment to be ready and cert secret hash to be tracked
+				Eventually(func(g Gomega) {
+					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
+					g.Expect(instance.Status.Conditions.IsTrue(condition.DeploymentReadyCondition)).To(BeTrue())
+					g.Expect(instance.Status.SecretHashes).Should(HaveKey(certSecretName.Name))
+				}, th.Timeout, th.Interval).Should(Succeed())
+
+				// Simulate cert-manager renewal: update the cert secret data
+				Eventually(func(g Gomega) {
+					secret := &corev1.Secret{}
+					g.Expect(th.K8sClient.Get(th.Ctx, certSecretName, secret)).To(Succeed())
+					secret.Data["tls.crt"] = []byte("renewed-cert-data")
+					secret.Data["tls.key"] = []byte("renewed-key-data")
+					g.Expect(th.K8sClient.Update(th.Ctx, secret)).To(Succeed())
+				}, th.Timeout, th.Interval).Should(Succeed())
+
+				// Verify DeploymentReadyCondition becomes False
+				Eventually(func(g Gomega) {
+					instance := GetDataplaneNodeSet(dataplaneNodeSetName)
+					g.Expect(instance.Status.Conditions.IsFalse(condition.DeploymentReadyCondition)).To(BeTrue())
+				}, th.Timeout, th.Interval).Should(Succeed())
+			})
+		})
+
 		When("Running deployments exist with completed deployment", func() {
 			BeforeEach(func() {
 				nodeSetSpec := DefaultDataPlaneNodeSetSpec("edpm-compute")