Annotate created secrets with cert information #388
Conversation
jetstack-bot
added
release-note
|
/test e2e v1.8 |
pkg/controller/certificates/sync.go
Outdated
| altNamesAnnotation = "certmanager.k8s.io/alt-names" | ||
| commonNameAnnotation = "certmanager.k8s.io/common-name" | ||
| issuerNameAnnotation = "certmanager.k8s.io/issuer-name" | ||
| issuerKindAnnotation = "certmanager.k8s.io/issuer-kind" |
There was a problem hiding this comment.
Can you move these into pkg/apis/certmanager/v1alpha1/types.go at the top of the file as consts, export them, and suffix each with Key?
pkg/controller/certificates/sync.go
Outdated
| } | ||
|
|
||
| secret.Annotations[altNamesAnnotation] = strings.Join(spec.DNSNames, ",") | ||
| secret.Annotations[commonNameAnnotation] = spec.CommonName |
There was a problem hiding this comment.
We shouldn't directly copy these values from the Certificate resource - they default in peculiar ways (see here: https://github.com/kragniz/cert-manager/blob/f6210c12c627155a580ce21f041b81e610e3051a/pkg/controller/certificates/sync.go#L100-L107)
pkg/controller/certificates/sync.go
Outdated
| secret.Annotations[altNamesAnnotation] = strings.Join(spec.DNSNames, ",") | ||
| secret.Annotations[commonNameAnnotation] = spec.CommonName | ||
| secret.Annotations[issuerNameAnnotation] = spec.IssuerRef.Name | ||
| secret.Annotations[issuerKindAnnotation] = spec.IssuerRef.Kind |
There was a problem hiding this comment.
Again, cannot read Kind directly from the Certificate spec as it is an optional field and we don't have defaulting yet due to CRDs (and we haven't created a MutatingWebhook in cert-manager yet)
And also move annotation keys to v1alpha1
jetstack-bot
added
size/M
pkg/controller/certificates/sync.go
Outdated
| cn, err := pki.CommonNameForCertificate(crt) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Sorry to go back and forth - a part of me is worried if we set the annotations based on the certificate itself, if this function is ever called with old certificate data but a new certificate resource (for some reason), it would be incorrect.
It's not a massive issue right now, but I'm concerned it's undocumented behaviour. For now, a comment on this function would probably suffice. Otherwise we should look at either reading the cert, or passing in the vars computed in the callsite to this function.
|
A decent quality of life addition. The annotations don't tell you which Certificate resource created and manages this Secret. That back-reference would be quite useful, e.g. to find which Certificate to delete/change for a Secret you are using. Something like: Then you could roll a |
|
@whereisaaron Kubernetes has Owner References for this, and we've proposed adding owner references to secret resources in #328. |
|
/retest |
|
/test e2e v1.8 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: munnerz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jetstack-bot
added
the
approved
What this PR does / why we need it: Previously, it was hard to inspect why a secret was created. This adds additional information in the form of annotations which should help keep track of created secrets.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #262Release note: