-
Notifications
You must be signed in to change notification settings - Fork 113
Detect TLS cert secret changes in NodeSet reconciler #1990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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().
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deployedSecretHashes contains DataSource, AnsibleVarsFrom, and cert secrets in one map, there is no way
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| 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 | ||
| } | ||
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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
So the existing spec-hash check already covers this case.
There was a problem hiding this comment.
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.