From bae51b92b294622367bcffedc355a926f5250611 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 18 Nov 2020 09:50:12 +0000 Subject: [PATCH 1/4] Simplify some ingress-shim helper functions Signed-off-by: Richard Wall --- pkg/controller/ingress-shim/sync.go | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index 7f14a64e2..f93bea0a5 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -156,12 +156,8 @@ func (c *controller) buildCertificates(ctx context.Context, ing *networkingv1bet }, } - err = c.setIssuerSpecificConfig(crt, ing, tls) - if err != nil { - return nil, nil, err - } - - c.setCommonName(crt, ing) + setIssuerSpecificConfig(crt, ing) + setCommonName(crt, ing) // check if a Certificate for this TLS entry already exists, and if it // does then skip this entry @@ -188,10 +184,7 @@ func (c *controller) buildCertificates(ctx context.Context, ing *networkingv1bet updateCrt.Spec = crt.Spec updateCrt.Labels = crt.Labels - err = c.setIssuerSpecificConfig(updateCrt, ing, tls) - if err != nil { - return nil, nil, err - } + setIssuerSpecificConfig(updateCrt, ing) updateCrts = append(updateCrts, updateCrt) } else { newCrts = append(newCrts, crt) @@ -273,7 +266,7 @@ func certNeedsUpdate(a, b *cmapi.Certificate) bool { return false } -func (c *controller) setIssuerSpecificConfig(crt *cmapi.Certificate, ing *networkingv1beta1.Ingress, tls networkingv1beta1.IngressTLS) error { +func setIssuerSpecificConfig(crt *cmapi.Certificate, ing *networkingv1beta1.Ingress) { ingAnnotations := ing.Annotations if ingAnnotations == nil { ingAnnotations = map[string]string{} @@ -299,11 +292,9 @@ func (c *controller) setIssuerSpecificConfig(crt *cmapi.Certificate, ing *networ } crt.Annotations[cmacme.ACMECertificateHTTP01IngressClassOverride] = ingressClassVal } - - return nil } -func (c *controller) setCommonName(crt *cmapi.Certificate, ing *networkingv1beta1.Ingress) { +func setCommonName(crt *cmapi.Certificate, ing *networkingv1beta1.Ingress) { // if annotation is set use that as CN if ing.Annotations != nil && ing.Annotations[cmapi.CommonNameAnnotationKey] != "" { crt.Spec.CommonName = ing.Annotations[cmapi.CommonNameAnnotationKey] From 26aa0e29fafa85373ba896c494c37eeefe28ea3d Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 18 Nov 2020 14:00:28 +0000 Subject: [PATCH 2/4] Add a renew-before Ingress annotation to set the renewBefore field on the Certificate Signed-off-by: Richard Wall --- pkg/apis/certmanager/v1/types.go | 3 + pkg/controller/ingress-shim/BUILD.bazel | 7 +- pkg/controller/ingress-shim/helper.go | 49 ++++++++++ pkg/controller/ingress-shim/helper_test.go | 105 +++++++++++++++++++++ pkg/controller/ingress-shim/sync.go | 4 +- pkg/controller/ingress-shim/sync_test.go | 26 +++++ 6 files changed, 192 insertions(+), 2 deletions(-) create mode 100644 pkg/controller/ingress-shim/helper.go create mode 100644 pkg/controller/ingress-shim/helper_test.go diff --git a/pkg/apis/certmanager/v1/types.go b/pkg/apis/certmanager/v1/types.go index c69ffff9f..8aa6dcb95 100644 --- a/pkg/apis/certmanager/v1/types.go +++ b/pkg/apis/certmanager/v1/types.go @@ -30,6 +30,9 @@ const ( // Annotation key for certificate common name. CommonNameAnnotationKey = "cert-manager.io/common-name" + // Annotation key for certificate renewBefore. + RenewBeforeAnnotationKey = "cert-manager.io/renew-before" + // Annotation key the 'name' of the Issuer resource. IssuerNameAnnotationKey = "cert-manager.io/issuer-name" diff --git a/pkg/controller/ingress-shim/BUILD.bazel b/pkg/controller/ingress-shim/BUILD.bazel index 1c5b10c02..43d74dfdb 100644 --- a/pkg/controller/ingress-shim/BUILD.bazel +++ b/pkg/controller/ingress-shim/BUILD.bazel @@ -5,6 +5,7 @@ go_library( srcs = [ "checks.go", "controller.go", + "helper.go", "sync.go", ], importpath = "github.com/jetstack/cert-manager/pkg/controller/ingress-shim", @@ -36,7 +37,10 @@ go_library( go_test( name = "go_default_test", - srcs = ["sync_test.go"], + srcs = [ + "helper_test.go", + "sync_test.go", + ], embed = [":go_default_library"], deps = [ "//pkg/apis/acme/v1:go_default_library", @@ -44,6 +48,7 @@ go_test( "//pkg/apis/meta/v1:go_default_library", "//pkg/controller/test:go_default_library", "//test/unit/gen:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", "@io_k8s_api//networking/v1beta1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", diff --git a/pkg/controller/ingress-shim/helper.go b/pkg/controller/ingress-shim/helper.go new file mode 100644 index 000000000..dff0200e7 --- /dev/null +++ b/pkg/controller/ingress-shim/helper.go @@ -0,0 +1,49 @@ +/* +Copyright 2020 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "errors" + "fmt" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" +) + +var ( + errNilCertificate = errors.New("the supplied Certificate pointer was nil") + errInvalidIngressAnnotation = errors.New("invalid ingress annotation") +) + +func translateIngressAnnotations(crt *cmapi.Certificate, annotations map[string]string) error { + if crt == nil { + return errNilCertificate + } + if commonName, found := annotations[cmapi.CommonNameAnnotationKey]; found { + crt.Spec.CommonName = commonName + } + if renewBefore, found := annotations[cmapi.RenewBeforeAnnotationKey]; found { + duration, err := time.ParseDuration(renewBefore) + if err != nil { + return fmt.Errorf("%w %q: %v", errInvalidIngressAnnotation, cmapi.RenewBeforeAnnotationKey, err) + } + crt.Spec.RenewBefore = &metav1.Duration{Duration: duration} + } + return nil +} diff --git a/pkg/controller/ingress-shim/helper_test.go b/pkg/controller/ingress-shim/helper_test.go new file mode 100644 index 000000000..1d4161974 --- /dev/null +++ b/pkg/controller/ingress-shim/helper_test.go @@ -0,0 +1,105 @@ +/* +Copyright 2020 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + "github.com/jetstack/cert-manager/test/unit/gen" +) + +func TestTranslateIngressAnnotations(t *testing.T) { + type testCase struct { + crt *cmapi.Certificate + annotations map[string]string + mutate func(*testCase) + check func(*assert.Assertions, *cmapi.Certificate) + expectedError error + } + + validAnnotations := func() map[string]string { + return map[string]string{ + cmapi.CommonNameAnnotationKey: "www.example.com", + cmapi.RenewBeforeAnnotationKey: "24h", + } + } + + tests := map[string]testCase{ + "success": { + crt: gen.Certificate("example-cert"), + annotations: validAnnotations(), + check: func(a *assert.Assertions, crt *cmapi.Certificate) { + a.Equal("www.example.com", crt.Spec.CommonName) + a.Equal(&metav1.Duration{Duration: time.Hour * 24}, crt.Spec.RenewBefore) + }, + }, + "nil annotations": { + crt: gen.Certificate("example-cert"), + annotations: nil, + }, + "empty annotations": { + crt: gen.Certificate("example-cert"), + annotations: map[string]string{}, + }, + "nil certificate": { + crt: nil, + annotations: validAnnotations(), + expectedError: errNilCertificate, + }, + "bad renewBefore": { + crt: gen.Certificate("example-cert"), + annotations: validAnnotations(), + mutate: func(tc *testCase) { + tc.annotations[cmapi.RenewBeforeAnnotationKey] = "an un-parsable duration string" + }, + expectedError: errInvalidIngressAnnotation, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + if tc.mutate != nil { + tc.mutate(&tc) + } + crt := tc.crt.DeepCopy() + + err := translateIngressAnnotations(crt, tc.annotations) + + if tc.expectedError != nil { + assertErrorIs(t, err, tc.expectedError) + } else { + assert.NoError(t, err) + } + if tc.check != nil { + tc.check(assert.New(t), crt) + } + }) + } +} + +// assertErrorIs checks that the supplied error has the target error in its chain. +// TODO Upgrade to next release of testify package which has this built in. +func assertErrorIs(t *testing.T, err, target error) { + if assert.Error(t, err) { + assert.Truef(t, errors.Is(err, target), "unexpected error type. err: %v, target: %v", err, target) + } +} diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index f93bea0a5..4e6f60d4e 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -157,7 +157,9 @@ func (c *controller) buildCertificates(ctx context.Context, ing *networkingv1bet } setIssuerSpecificConfig(crt, ing) - setCommonName(crt, ing) + if err := translateIngressAnnotations(crt, ing.Annotations); err != nil { + return nil, nil, err + } // check if a Certificate for this TLS entry already exists, and if it // does then skip this entry diff --git a/pkg/controller/ingress-shim/sync_test.go b/pkg/controller/ingress-shim/sync_test.go index 87d5aaf67..791f40a98 100644 --- a/pkg/controller/ingress-shim/sync_test.go +++ b/pkg/controller/ingress-shim/sync_test.go @@ -960,6 +960,32 @@ func TestSync(t *testing.T) { }, }, }, + { + Name: "Failure to translateIngressAnnotations", + Issuer: acmeIssuer, + Ingress: &networkingv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-name", + Namespace: gen.DefaultTestNamespace, + Annotations: map[string]string{ + cmapi.IngressIssuerNameAnnotationKey: "issuer-name", + cmapi.IssuerKindAnnotationKey: "Issuer", + cmapi.IssuerGroupAnnotationKey: "cert-manager.io", + cmapi.RenewBeforeAnnotationKey: "invalid renew before value", + }, + UID: types.UID("ingress-name"), + }, + Spec: networkingv1beta1.IngressSpec{ + TLS: []networkingv1beta1.IngressTLS{ + { + Hosts: []string{"example.com"}, + SecretName: "example-com-tls", + }, + }, + }, + }, + Err: true, + }, } testFn := func(test testT) func(t *testing.T) { return func(t *testing.T) { From 9cd3eaabf753d46193ad699b5f412224b42c1c34 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 16 Dec 2020 09:40:28 +0000 Subject: [PATCH 3/4] Add a duration Ingress annotation to set the duration field on Certificate Signed-off-by: Richard Wall --- pkg/apis/certmanager/v1/types.go | 3 +++ pkg/controller/ingress-shim/helper.go | 7 +++++++ pkg/controller/ingress-shim/helper_test.go | 10 ++++++++++ 3 files changed, 20 insertions(+) diff --git a/pkg/apis/certmanager/v1/types.go b/pkg/apis/certmanager/v1/types.go index 8aa6dcb95..e48bb7af0 100644 --- a/pkg/apis/certmanager/v1/types.go +++ b/pkg/apis/certmanager/v1/types.go @@ -30,6 +30,9 @@ const ( // Annotation key for certificate common name. CommonNameAnnotationKey = "cert-manager.io/common-name" + // Duration key for certificate duration. + DurationAnnotationKey = "cert-manager.io/duration" + // Annotation key for certificate renewBefore. RenewBeforeAnnotationKey = "cert-manager.io/renew-before" diff --git a/pkg/controller/ingress-shim/helper.go b/pkg/controller/ingress-shim/helper.go index dff0200e7..b254fc948 100644 --- a/pkg/controller/ingress-shim/helper.go +++ b/pkg/controller/ingress-shim/helper.go @@ -38,6 +38,13 @@ func translateIngressAnnotations(crt *cmapi.Certificate, annotations map[string] if commonName, found := annotations[cmapi.CommonNameAnnotationKey]; found { crt.Spec.CommonName = commonName } + if duration, found := annotations[cmapi.DurationAnnotationKey]; found { + duration, err := time.ParseDuration(duration) + if err != nil { + return fmt.Errorf("%w %q: %v", errInvalidIngressAnnotation, cmapi.DurationAnnotationKey, err) + } + crt.Spec.Duration = &metav1.Duration{Duration: duration} + } if renewBefore, found := annotations[cmapi.RenewBeforeAnnotationKey]; found { duration, err := time.ParseDuration(renewBefore) if err != nil { diff --git a/pkg/controller/ingress-shim/helper_test.go b/pkg/controller/ingress-shim/helper_test.go index 1d4161974..c38e6ae15 100644 --- a/pkg/controller/ingress-shim/helper_test.go +++ b/pkg/controller/ingress-shim/helper_test.go @@ -40,6 +40,7 @@ func TestTranslateIngressAnnotations(t *testing.T) { validAnnotations := func() map[string]string { return map[string]string{ cmapi.CommonNameAnnotationKey: "www.example.com", + cmapi.DurationAnnotationKey: "168h", // 1 week cmapi.RenewBeforeAnnotationKey: "24h", } } @@ -50,6 +51,7 @@ func TestTranslateIngressAnnotations(t *testing.T) { annotations: validAnnotations(), check: func(a *assert.Assertions, crt *cmapi.Certificate) { a.Equal("www.example.com", crt.Spec.CommonName) + a.Equal(&metav1.Duration{Duration: time.Hour * 24 * 7}, crt.Spec.Duration) a.Equal(&metav1.Duration{Duration: time.Hour * 24}, crt.Spec.RenewBefore) }, }, @@ -66,6 +68,14 @@ func TestTranslateIngressAnnotations(t *testing.T) { annotations: validAnnotations(), expectedError: errNilCertificate, }, + "bad duration": { + crt: gen.Certificate("example-cert"), + annotations: validAnnotations(), + mutate: func(tc *testCase) { + tc.annotations[cmapi.DurationAnnotationKey] = "an un-parsable duration string" + }, + expectedError: errInvalidIngressAnnotation, + }, "bad renewBefore": { crt: gen.Certificate("example-cert"), annotations: validAnnotations(), From 4ba546c97b6ad3c07a259d77b09f77d0688553e1 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 16 Dec 2020 10:19:02 +0000 Subject: [PATCH 4/4] E2E test for all Certificate field related ingress annotations Signed-off-by: Richard Wall --- .../suite/conformance/certificates/tests.go | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/test/e2e/suite/conformance/certificates/tests.go b/test/e2e/suite/conformance/certificates/tests.go index a4da01557..d361b9bac 100644 --- a/test/e2e/suite/conformance/certificates/tests.go +++ b/test/e2e/suite/conformance/certificates/tests.go @@ -522,19 +522,23 @@ func (s *Suite) Define() { Expect(err).NotTo(HaveOccurred()) }, featureset.OnlySAN) - s.it(f, "should issue a basic certificate for a single commonName and distinct dnsName defined by an ingress with annotations", func(issuerRef cmmeta.ObjectReference) { + s.it(f, "should issue a basic certificate defined by an ingress with certificate field annotations", func(issuerRef cmmeta.ObjectReference) { ingClient := f.KubeClientSet.NetworkingV1beta1().Ingresses(f.Namespace.Name) name := "testcert-ingress" secretName := "testcert-ingress-tls" domain := s.newDomain() + duration := time.Hour * 999 + renewBefore := time.Hour * 111 - By("Creating an Ingress with the issuer name annotation set") + By("Creating an Ingress with annotations for issuerRef and other Certificate fields") ingress, err := ingClient.Create(context.TODO(), e2eutil.NewIngress(name, secretName, map[string]string{ "cert-manager.io/issuer": issuerRef.Name, "cert-manager.io/issuer-kind": issuerRef.Kind, "cert-manager.io/issuer-group": issuerRef.Group, "cert-manager.io/common-name": domain, + "cert-manager.io/duration": duration.String(), + "cert-manager.io/renew-before": renewBefore.String(), }, domain), metav1.CreateOptions{}) Expect(err).NotTo(HaveOccurred()) @@ -549,6 +553,22 @@ func (s *Suite) Define() { err = f.Helper().WaitCertificateIssued(f.Namespace.Name, certName, time.Minute*5) Expect(err).NotTo(HaveOccurred()) + // Verify that the ingres-shim has translated all the supplied + // annotations into equivalent Certificate field values + By("Validating the created Certificate") + err = f.Helper().ValidateCertificate( + f.Namespace.Name, certName, + func(certificate *cmapi.Certificate, _ *corev1.Secret) error { + Expect(certificate.Spec.DNSNames).To(ConsistOf(domain)) + Expect(certificate.Spec.CommonName).To(Equal(domain)) + Expect(certificate.Spec.Duration.Duration).To(Equal(duration)) + Expect(certificate.Spec.RenewBefore.Duration).To(Equal(renewBefore)) + return nil + }, + ) + + // Verify that the issuer has preserved all the Certificate values + // in the signed certificate By("Validating the issued Certificate...") err = f.Helper().ValidateCertificate(f.Namespace.Name, certName, f.Helper().ValidationSetForUnsupportedFeatureSet(s.UnsupportedFeatures)...) Expect(err).NotTo(HaveOccurred())