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
| labelSelector := map[string]string{ | ||
| NodeSetLabel: nodeSetName, | ||
| } | ||
| secrets, err := secret.GetSecrets(ctx, helper, namespace, labelSelector) |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
deployedSecretHashes contains DataSource, AnsibleVarsFrom, and cert secrets in one map, there is no way
to distinguish without fetching each.
Reusing GetDeploymentHashesForService would need a get per service + a List per service/certKey, which is in nested loop.
There was a problem hiding this comment.
We probably need to refactor the status struct as it's currently flat map and also nodeset now contains secret hashes for other nodesets via deployment. That would make all these much simpler.
|
/cherry-pick 18-stable |
|
@oliashish: new pull request created: #2000 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 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