Detect TLS cert secret changes in NodeSet reconciler#1990
Conversation
OpenStackControlPlane CRD Size Report
Threshold reference
|
|
/test openstack-operator-build-deploy-kuttl-4-20 |
| } | ||
|
|
||
| for name, currentHash := range currentCertHashes { | ||
| if deployedHash, exists := deployedSecretHashes[name]; exists && deployedHash != currentHash { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // listed in AnsibleVarsFrom, so the field-index lookup below would miss them. | ||
| labels := obj.GetLabels() | ||
| if nodeSetName, ok := labels[deployment.NodeSetLabel]; ok { | ||
| if _, hasSvcLabel := labels[deployment.ServiceLabel]; hasSvcLabel { |
There was a problem hiding this comment.
claude is recommending to also check the existence of the ServiceKeyLabel here in order to be more precise.
There was a problem hiding this comment.
Ack.
Added ServiceKeyLabel check for completeness. Technically redundant since EnsureTLSCerts always sets all three labels together, but makes the match more precise.
Cert-manager renewals were invisible because cert secrets are owned by the Certificate CR, not the NodeSet. Add label-based detection in secretWatcherFn and hash comparison in checkDeployment to surface redeployment required signal after cert rotation. Jira: OSPRH-32671 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e604e71 to
16c0fac
Compare
|
/test openstack-operator-build-deploy-kuttl-4-20 |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 05m 11s |
|
recheck |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oliashish, slagle The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
788b8c9
into
openstack-k8s-operators:main
2 participants
Cert-manager renewals were invisible because cert secrets are owned by the Certificate CR, not the NodeSet. Add label-based detection in secretWatcherFn and hash comparison in checkDeployment to surface redeployment required signal after cert rotation.
Jira: OSPRH-32671
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com