diff --git a/pkg/apis/certmanager/v1alpha2/types.go b/pkg/apis/certmanager/v1alpha2/types.go index 027109e3a..5174fff9a 100644 --- a/pkg/apis/certmanager/v1alpha2/types.go +++ b/pkg/apis/certmanager/v1alpha2/types.go @@ -28,10 +28,10 @@ const ( CertificateNameKey = "cert-manager.io/certificate-name" ) -// Legacy annotation names for Secrets +// Deprecated annotation names for Secrets const ( - LegacyIssuerNameAnnotationKey = "certmanager.k8s.io/issuer-name" - LegacyIssuerKindAnnotationKey = "certmanager.k8s.io/issuer-kind" + DeprecatedIssuerNameAnnotationKey = "certmanager.k8s.io/issuer-name" + DeprecatedIssuerKindAnnotationKey = "certmanager.k8s.io/issuer-kind" ) const ( diff --git a/pkg/controller/certificates/sync.go b/pkg/controller/certificates/sync.go index cd868b8e1..86f683aac 100644 --- a/pkg/controller/certificates/sync.go +++ b/pkg/controller/certificates/sync.go @@ -487,6 +487,7 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt // Otherwise, the existing resource will be updated. // The first return argument will be true if the resource was updated/created // without error. +// updateSecretData will also update deprecated annotations if they exist. func (c *certificateRequestManager) updateSecretData(ctx context.Context, crt *cmapi.Certificate, existingSecret *corev1.Secret, data secretData) (bool, error) { s := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -774,6 +775,7 @@ type secretData struct { // setSecretValues will NOT actually update the resource in the apiserver. // If updating an existing Secret resource returned by an api client 'lister', // make sure to DeepCopy the object first to avoid modifying data in-cache. +// It will also update depreciated issuer name and kind annotations if they exist. func setSecretValues(ctx context.Context, crt *cmapi.Certificate, s *corev1.Secret, data secretData) error { // initialize the `Data` field if it is nil if s.Data == nil { @@ -792,6 +794,15 @@ func setSecretValues(ctx context.Context, crt *cmapi.Certificate, s *corev1.Secr s.Annotations[cmapi.IssuerNameAnnotationKey] = crt.Spec.IssuerRef.Name s.Annotations[cmapi.IssuerKindAnnotationKey] = apiutil.IssuerKind(crt.Spec.IssuerRef) + // If deprecated annotations exist with any value, then they too shall be + // updated + if _, ok := s.Annotations[cmapi.DeprecatedIssuerNameAnnotationKey]; ok { + s.Annotations[cmapi.DeprecatedIssuerNameAnnotationKey] = crt.Spec.IssuerRef.Name + } + if _, ok := s.Annotations[cmapi.DeprecatedIssuerKindAnnotationKey]; ok { + s.Annotations[cmapi.DeprecatedIssuerKindAnnotationKey] = apiutil.IssuerKind(crt.Spec.IssuerRef) + } + // if the certificate data is empty, clear the subject related annotations if len(data.cert) == 0 { delete(s.Annotations, cmapi.CommonNameAnnotationKey) diff --git a/pkg/controller/certificates/util.go b/pkg/controller/certificates/util.go index 2a2469db6..37e616a4c 100644 --- a/pkg/controller/certificates/util.go +++ b/pkg/controller/certificates/util.go @@ -121,16 +121,30 @@ func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert * secret.Annotations = make(map[string]string) } - // validate that the issuer is correct - if crt.Spec.IssuerRef.Name != secret.Annotations[v1alpha2.IssuerNameAnnotationKey] && - crt.Spec.IssuerRef.Name != secret.Annotations[v1alpha2.LegacyIssuerNameAnnotationKey] { - errs = append(errs, fmt.Sprintf("Issuer of the certificate is not up to date: %q", secret.Annotations[v1alpha2.IssuerNameAnnotationKey])) + // Validate that the issuer name and kind is correct + // If the new annotation exists and doesn't match then error + // If the new annotation doesn't exist and the old annotation doesn't match then error + + annotationError := func(k, v string) { + errs = append(errs, fmt.Sprintf("Issuer %q of the certificate is not up to date: %q", k, v)) } - // validate that the issuer kind is correct - if apiutil.IssuerKind(crt.Spec.IssuerRef) != secret.Annotations[v1alpha2.IssuerKindAnnotationKey] && - apiutil.IssuerKind(crt.Spec.IssuerRef) != secret.Annotations[v1alpha2.LegacyIssuerKindAnnotationKey] { - errs = append(errs, fmt.Sprintf("Issuer kind of the certificate is not up to date: %q", secret.Annotations[v1alpha2.IssuerKindAnnotationKey])) + name, ok := secret.Annotations[v1alpha2.IssuerNameAnnotationKey] + if !ok { + if secret.Annotations[v1alpha2.DeprecatedIssuerNameAnnotationKey] != crt.Spec.IssuerRef.Name { + annotationError(v1alpha2.DeprecatedIssuerNameAnnotationKey, secret.Annotations[v1alpha2.DeprecatedIssuerNameAnnotationKey]) + } + } else if name != crt.Spec.IssuerRef.Name { + annotationError(v1alpha2.IssuerNameAnnotationKey, secret.Annotations[v1alpha2.IssuerNameAnnotationKey]) + } + + kind, ok := secret.Annotations[v1alpha2.IssuerKindAnnotationKey] + if !ok { + if secret.Annotations[v1alpha2.DeprecatedIssuerKindAnnotationKey] != apiutil.IssuerKind(crt.Spec.IssuerRef) { + annotationError(v1alpha2.DeprecatedIssuerKindAnnotationKey, secret.Annotations[v1alpha2.DeprecatedIssuerKindAnnotationKey]) + } + } else if kind != apiutil.IssuerKind(crt.Spec.IssuerRef) { + annotationError(v1alpha2.IssuerKindAnnotationKey, secret.Annotations[v1alpha2.IssuerKindAnnotationKey]) } return len(errs) == 0, errs diff --git a/pkg/controller/certificates/util_test.go b/pkg/controller/certificates/util_test.go index 220058899..afeba1666 100644 --- a/pkg/controller/certificates/util_test.go +++ b/pkg/controller/certificates/util_test.go @@ -116,21 +116,155 @@ func TestCertificateMatchesSpec(t *testing.T) { }, }, - "if the issuer name and kind uses the lagacy annotation then it should still match the spec": { - cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate, - gen.SetCertificateCommonName(""), - )), - certificate: gen.CertificateFrom(exampleBundle.certificate, - gen.SetCertificateCommonName(""), - ), + "if the issuer name and kind uses v1alpha2 annotation then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), secret: gen.SecretFrom(secret, gen.SetSecretAnnotations(map[string]string{ - cmapi.LegacyIssuerNameAnnotationKey: "ca-issuer", - cmapi.LegacyIssuerKindAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", })), expMatch: true, expErrors: nil, }, + + "if the issuer name uses v1alpha2 annotation but kind uses depreicated then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.DeprecatedIssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name uses deprecated annotation but kind uses v1alpha2 then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name and kind uses the deprecated annotation then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "ca-issuer", + cmapi.DeprecatedIssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name uses v1alpha2 and kind uses both the deprecated and v1alpha2 annotation then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerKindAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name both the deprecated and v1alpha2 annotation and kind uses deprecated then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name and kind uses both the deprecated and v1alpha2 annotation then it should still match the spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "ca-issuer", + cmapi.DeprecatedIssuerKindAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name and kind uses both the deprecated and v1alpha2 annotation but no values in deprecated annotations then should match spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "foo", + cmapi.DeprecatedIssuerKindAnnotationKey: "bar", + cmapi.IssuerNameAnnotationKey: "ca-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + })), + expMatch: true, + expErrors: nil, + }, + + "if the issuer name and kind deprecated annotations are correct but v1alpha2 values are wrong then should not match spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "ca-issuer", + cmapi.DeprecatedIssuerKindAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "foo", + cmapi.IssuerKindAnnotationKey: "bar", + })), + expMatch: false, + expErrors: []string{ + `Issuer "cert-manager.io/issuer-name" of the certificate is not up to date: "foo"`, + `Issuer "cert-manager.io/issuer-kind" of the certificate is not up to date: "bar"`, + }, + }, + + "if the issuer name and kind deprecated annotations are correct but v1alpha2 values are empty but exist then should not match spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "ca-issuer", + cmapi.DeprecatedIssuerKindAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "", + cmapi.IssuerKindAnnotationKey: "", + })), + expMatch: false, + expErrors: []string{ + `Issuer "cert-manager.io/issuer-name" of the certificate is not up to date: ""`, + `Issuer "cert-manager.io/issuer-kind" of the certificate is not up to date: ""`, + }, + }, + "if the issuer name and kind deprecated annotations are wrong and no v1alpha2 values then should not match spec": { + cb: mustCreateCryptoBundle(t, gen.CertificateFrom(exampleBundle.certificate)), + certificate: gen.CertificateFrom(exampleBundle.certificate), + secret: gen.SecretFrom(secret, + gen.SetSecretAnnotations(map[string]string{ + cmapi.DeprecatedIssuerNameAnnotationKey: "foo", + cmapi.DeprecatedIssuerKindAnnotationKey: "bar", + })), + expMatch: false, + expErrors: []string{ + `Issuer "certmanager.k8s.io/issuer-name" of the certificate is not up to date: "foo"`, + `Issuer "certmanager.k8s.io/issuer-kind" of the certificate is not up to date: "bar"`, + }, + }, } { t.Run(name, func(t *testing.T) { match, errs := certificateMatchesSpec(