-
Notifications
You must be signed in to change notification settings - Fork 17
CLOUDP-290847: Remove cert hash annotations #383
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
Conversation
e6b05c9
to
6736153
Compare
6736153
to
66935c4
Compare
if mrs.Spec.Security.IsTLSEnabled() { | ||
certSecretName := mrs.Spec.GetSecurity().MemberCertificateSecretName(mrs.Name) | ||
internalClusterCertSecretName := mrs.Spec.GetSecurity().InternalClusterAuthSecretName(mrs.Name) | ||
tlsCertHash := enterprisepem.ReadHashFromSecret(ctx, r.SecretClient, mrs.Namespace, certSecretName, "", log) |
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.
I see that tlsCertHash
and tlsCertPath
are calculated here and also in the other controllers/operator/mongodbreplicaset_controller.go
. Can we move this logic to common controller?
I thought about method like:
func (r *ReconcileCommonController) tlsCertHashAndPath(ctx, ...) (string, string) {
tlsCertHash := enterprisepem.ReadHashFromSecret(ctx, r.SecretClient, mrs.Namespace, certSecretName, "", log)
if tlsCertHash == "" {
return "", ""
}
return tlsCertHash, fmt.Sprintf("%s/%s", util.TLSCertMountPath, tlsCertHash)
}
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.
I would like to leave this out of the scope of this PR. Here I want to make a minimal change so that I can progress with my main goal (agent cert rotation).
There are more places where we read the secret and calculate the hash and I have a feeling that after adding agent cert rotation it will be a bit more clear if there is a pattern which we can abstract somehow. I'll look into this if I have time left after implementing agent cert rotation.
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.
Understand. In that case can you add TODO item for improving this later?
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.
I've added a to do in another branch (I'll eventually push the to do or a change to #389). Don't want to push to this branch and wait for CI again.
|
Summary
This refactors the way we do cert rotation: we no longer write certificate hashes into annotations of STS. This simplifies the flow and will make it easier to implement agent cert rotation (CLOUDP-290847) in a similar way.
Proof of Work
Existing tests must be green.
Checklist
skip-changelog
label if not needed