Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a review with claude has pointed out:

The new checkCertSecretsChanged uses exists && deployedHash != currentHash — a new cert secret that wasn't present at deployment time is not treated as a change. This
means if a new TLS service is added to the NodeSet after the last deployment, cert secrets for it won't trigger redeployment.

This may be intentional (new certs are picked up on the next deployment anyway), but the inconsistency is worth a comment or explicit design choice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new TLS service to the NodeSet means changing instance.Spec.Services. That changes instance.Spec, which changes ConfigHash which hashes the entire instance.Spec. Since ConfigHash != DeployedConfigHash, the deployment is already skipped before we reach here at

if deployment.Status.NodeSetHashes[instance.Name] != instance.Status.ConfigHash {
      continue
 }

So the existing spec-hash check already covers this case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also the case of changing a service (that is already in the NodeSet spec) to add a TLS cert, but that seems pretty unlikely.

helper.GetLogger().Info("Cert secret content changed", "secret", name)
return true, nil
}
}

return false, nil
}
33 changes: 33 additions & 0 deletions internal/dataplane/hashes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have all secret names in the deployedSecretHashes? We can avoid this lus api call which is heavy. Also, we can reuses the label selector logic already in GetDeploymentHashesForService().

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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -1806,6 +1807,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")
Expand Down