From 64d78c6e10c19c022982a10bc8726ea4525f9b54 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Thu, 13 Jan 2022 17:43:30 +0000 Subject: [PATCH] Update certificates controller with new secret manager signatures and tests Signed-off-by: joshvanl --- .../certificates/issuing/BUILD.bazel | 2 + .../issuing/issuing_controller.go | 18 +- .../issuing/issuing_controller_test.go | 213 ++++-------------- .../certificates/issuing/temporary.go | 2 +- .../certificates/secrettemplate/BUILD.bazel | 1 - .../secret_template_controller.go | 6 +- .../secret_template_controller_test.go | 12 - 7 files changed, 60 insertions(+), 194 deletions(-) diff --git a/pkg/controller/certificates/issuing/BUILD.bazel b/pkg/controller/certificates/issuing/BUILD.bazel index 3b7d4f4ce..105a5b8f8 100644 --- a/pkg/controller/certificates/issuing/BUILD.bazel +++ b/pkg/controller/certificates/issuing/BUILD.bazel @@ -60,9 +60,11 @@ go_test( deps = [ "//pkg/apis/certmanager/v1:go_default_library", "//pkg/apis/meta/v1:go_default_library", + "//pkg/controller/certificates/internal/secretsmanager:go_default_library", "//pkg/controller/certificates/internal/test:go_default_library", "//pkg/controller/test:go_default_library", "//test/unit/gen:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index 87e1003ab..7c78e1531 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -69,8 +69,11 @@ type controller struct { client cmclient.Interface - // secretManager is used to create and update Secrets with certificate and key data - secretsManager *secretsmanager.SecretsManager + // secretsUpdateData is used by the SecretTemplate controller for + // re-reconciling Secrets where the SecretTemplate is not up to date with a + // Certificate's secret. + secretsUpdateData func(context.Context, *cmapi.Certificate, secretsmanager.SecretData) error + // localTemporarySigner signs a certificate that is stored temporarily localTemporarySigner localTemporarySignerFn } @@ -120,10 +123,8 @@ func NewController( } secretsManager := secretsmanager.New( - kubeClient, - secretsInformer.Lister(), - restConfig, - certificateControllerOptions.EnableOwnerRef, + kubeClient.CoreV1(), secretsInformer.Lister(), + restConfig, certificateControllerOptions.EnableOwnerRef, ) return &controller{ @@ -133,7 +134,7 @@ func NewController( client: client, recorder: recorder, clock: clock, - secretsManager: secretsManager, + secretsUpdateData: secretsManager.UpdateData, localTemporarySigner: certificates.GenerateLocallySignedTemporaryCertificate, }, queue, mustSync } @@ -360,8 +361,7 @@ func (c *controller) issueCertificate(ctx context.Context, nextRevision int, crt CA: req.Status.CA, } - err = c.secretsManager.UpdateData(ctx, crt, secretData) - if err != nil { + if err := c.secretsUpdateData(ctx, crt, secretData); err != nil { return err } diff --git a/pkg/controller/certificates/issuing/issuing_controller_test.go b/pkg/controller/certificates/issuing/issuing_controller_test.go index fd47f8523..812d367d4 100644 --- a/pkg/controller/certificates/issuing/issuing_controller_test.go +++ b/pkg/controller/certificates/issuing/issuing_controller_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,6 +33,7 @@ import ( cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" + "github.com/jetstack/cert-manager/pkg/controller/certificates/internal/secretsmanager" internaltest "github.com/jetstack/cert-manager/pkg/controller/certificates/internal/test" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" "github.com/jetstack/cert-manager/test/unit/gen" @@ -52,7 +54,8 @@ func TestIssuingController(t *testing.T) { type testT struct { builder *testpkg.Builder - certificate *cmapi.Certificate + certificate *cmapi.Certificate + expSecretUpdateDataCall *secretsmanager.SecretData expectedErr bool } @@ -444,37 +447,16 @@ func TestIssuingController(t *testing.T) { gen.SetCertificateRevision(2), ), )), - testpkg.NewAction(coretesting.NewCreateAction( - corev1.SchemeGroupVersion.WithResource("secrets"), - exampleBundle.Certificate.Namespace, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: exampleBundle.Certificate.Namespace, - Name: "output", - Annotations: map[string]string{ - cmapi.CertificateNameKey: "test", - cmapi.IssuerKindAnnotationKey: "Issuer", - cmapi.IssuerNameAnnotationKey: "ca-issuer", - cmapi.IssuerGroupAnnotationKey: "foo.io", - cmapi.CommonNameAnnotationKey: "", - cmapi.AltNamesAnnotationKey: "example.com", - cmapi.IPSANAnnotationKey: "", - cmapi.URISANAnnotationKey: "", - }, - Labels: map[string]string{}, - }, - Data: map[string][]byte{ - corev1.TLSCertKey: exampleBundle.CertificateRequestReady.Status.Certificate, - corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, - }, - Type: corev1.SecretTypeTLS, - }, - )), }, ExpectedEvents: []string{ "Normal Issuing The certificate has been successfully issued", }, }, + expSecretUpdateDataCall: &secretsmanager.SecretData{ + Certificate: exampleBundle.CertificateRequestReady.Status.Certificate, + PrivateKey: exampleBundle.PrivateKeyBytes, + CA: nil, + }, expectedErr: false, }, @@ -519,38 +501,16 @@ func TestIssuingController(t *testing.T) { gen.SetCertificateRevision(2), ), )), - testpkg.NewAction(coretesting.NewUpdateAction( - corev1.SchemeGroupVersion.WithResource("secrets"), - exampleBundle.Certificate.Namespace, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: exampleBundle.Certificate.Namespace, - Name: "output", - Annotations: map[string]string{ - "my-custom": "annotation", - cmapi.CertificateNameKey: "test", - cmapi.IssuerGroupAnnotationKey: "foo.io", - cmapi.IssuerKindAnnotationKey: "Issuer", - cmapi.IssuerNameAnnotationKey: "ca-issuer", - cmapi.CommonNameAnnotationKey: "", - cmapi.AltNamesAnnotationKey: "example.com", - cmapi.IPSANAnnotationKey: "", - cmapi.URISANAnnotationKey: "", - }, - Labels: map[string]string{}, - }, - Data: map[string][]byte{ - corev1.TLSCertKey: exampleBundle.CertificateRequestReady.Status.Certificate, - corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, - }, - Type: corev1.SecretTypeTLS, - }, - )), }, ExpectedEvents: []string{ "Normal Issuing The certificate has been successfully issued", }, }, + expSecretUpdateDataCall: &secretsmanager.SecretData{ + Certificate: exampleBundle.CertificateRequestReady.Status.Certificate, + PrivateKey: exampleBundle.PrivateKeyBytes, + CA: nil, + }, expectedErr: false, }, @@ -579,38 +539,15 @@ func TestIssuingController(t *testing.T) { }, }, }, - ExpectedActions: []testpkg.Action{ - testpkg.NewAction(coretesting.NewCreateAction( - corev1.SchemeGroupVersion.WithResource("secrets"), - exampleBundle.Certificate.Namespace, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: exampleBundle.Certificate.Namespace, - Name: "output", - Annotations: map[string]string{ - cmapi.CertificateNameKey: "test", - cmapi.IssuerGroupAnnotationKey: "foo.io", - cmapi.IssuerKindAnnotationKey: "Issuer", - cmapi.IssuerNameAnnotationKey: "ca-issuer", - cmapi.CommonNameAnnotationKey: "", - cmapi.AltNamesAnnotationKey: "example.com", - cmapi.IPSANAnnotationKey: "", - cmapi.URISANAnnotationKey: "", - }, - Labels: map[string]string{}, - }, - Data: map[string][]byte{ - corev1.TLSCertKey: exampleBundle.LocalTemporaryCertificateBytes, - corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, - }, - Type: corev1.SecretTypeTLS, - }, - )), - }, ExpectedEvents: []string{ "Normal Issuing Issued temporary certificate", }, }, + expSecretUpdateDataCall: &secretsmanager.SecretData{ + Certificate: exampleBundle.LocalTemporaryCertificateBytes, + PrivateKey: exampleBundle.PrivateKeyBytes, + CA: nil, + }, expectedErr: false, }, @@ -650,39 +587,15 @@ func TestIssuingController(t *testing.T) { Type: corev1.SecretTypeTLS, }, }, - ExpectedActions: []testpkg.Action{ - testpkg.NewAction(coretesting.NewUpdateAction( - corev1.SchemeGroupVersion.WithResource("secrets"), - exampleBundle.Certificate.Namespace, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: exampleBundle.Certificate.Namespace, - Name: "output", - Annotations: map[string]string{ - "my-custom": "annotation", - cmapi.CertificateNameKey: "test", - cmapi.IssuerGroupAnnotationKey: "foo.io", - cmapi.IssuerKindAnnotationKey: "Issuer", - cmapi.IssuerNameAnnotationKey: "ca-issuer", - cmapi.CommonNameAnnotationKey: "", - cmapi.AltNamesAnnotationKey: "example.com", - cmapi.IPSANAnnotationKey: "", - cmapi.URISANAnnotationKey: "", - }, - Labels: map[string]string{}, - }, - Data: map[string][]byte{ - corev1.TLSCertKey: exampleBundle.LocalTemporaryCertificateBytes, - corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, - }, - Type: corev1.SecretTypeTLS, - }, - )), - }, ExpectedEvents: []string{ "Normal Issuing Issued temporary certificate", }, }, + expSecretUpdateDataCall: &secretsmanager.SecretData{ + Certificate: exampleBundle.LocalTemporaryCertificateBytes, + PrivateKey: exampleBundle.PrivateKeyBytes, + CA: nil, + }, expectedErr: false, }, @@ -771,39 +684,15 @@ func TestIssuingController(t *testing.T) { Type: corev1.SecretTypeTLS, }, }, - ExpectedActions: []testpkg.Action{ - testpkg.NewAction(coretesting.NewUpdateAction( - corev1.SchemeGroupVersion.WithResource("secrets"), - exampleBundle.Certificate.Namespace, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: exampleBundle.Certificate.Namespace, - Name: "output", - Annotations: map[string]string{ - "my-custom": "annotation", - cmapi.CertificateNameKey: "test", - cmapi.IssuerGroupAnnotationKey: "foo.io", - cmapi.IssuerKindAnnotationKey: "Issuer", - cmapi.IssuerNameAnnotationKey: "ca-issuer", - cmapi.CommonNameAnnotationKey: "", - cmapi.AltNamesAnnotationKey: "example.com", - cmapi.IPSANAnnotationKey: "", - cmapi.URISANAnnotationKey: "", - }, - Labels: map[string]string{}, - }, - Data: map[string][]byte{ - corev1.TLSCertKey: exampleBundle.LocalTemporaryCertificateBytes, - corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, - }, - Type: corev1.SecretTypeTLS, - }, - )), - }, ExpectedEvents: []string{ "Normal Issuing Issued temporary certificate", }, }, + expSecretUpdateDataCall: &secretsmanager.SecretData{ + Certificate: exampleBundle.LocalTemporaryCertificateBytes, + PrivateKey: exampleBundle.PrivateKeyBytes, + CA: nil, + }, expectedErr: false, }, @@ -889,37 +778,16 @@ func TestIssuingController(t *testing.T) { gen.SetCertificateRevision(2), ), )), - testpkg.NewAction(coretesting.NewCreateAction( - corev1.SchemeGroupVersion.WithResource("secrets"), - exampleBundle.Certificate.Namespace, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: exampleBundle.Certificate.Namespace, - Name: "output", - Annotations: map[string]string{ - cmapi.CertificateNameKey: "test", - cmapi.IssuerGroupAnnotationKey: "foo.io", - cmapi.IssuerKindAnnotationKey: "Issuer", - cmapi.IssuerNameAnnotationKey: "ca-issuer", - cmapi.CommonNameAnnotationKey: "", - cmapi.AltNamesAnnotationKey: "example.com", - cmapi.IPSANAnnotationKey: "", - cmapi.URISANAnnotationKey: "", - }, - Labels: map[string]string{}, - }, - Data: map[string][]byte{ - corev1.TLSCertKey: exampleBundle.CertificateRequestReady.Status.Certificate, - corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, - }, - Type: corev1.SecretTypeTLS, - }, - )), }, ExpectedEvents: []string{ "Normal Issuing The certificate has been successfully issued", }, }, + expSecretUpdateDataCall: &secretsmanager.SecretData{ + Certificate: exampleBundle.CertificateRequestReady.Status.Certificate, + PrivateKey: exampleBundle.PrivateKeyBytes, + CA: nil, + }, expectedErr: false, }, "if certificate is in Issuing state with temp annotation, one CertificateRequest Failed, a target Secret does not exist, mark the Certificate as failed": { @@ -1038,13 +906,24 @@ func TestIssuingController(t *testing.T) { fixedClock.SetTime(fixedClockStart) test.builder.Clock = fixedClock test.builder.T = t - test.builder.Init() + test.builder.InitWithRESTConfig() defer test.builder.Stop() w := controllerWrapper{} _, _, err := w.Register(test.builder.Context) require.NoError(t, err) w.controller.localTemporarySigner = testLocalTemporarySignerFn(exampleBundle.LocalTemporaryCertificateBytes) + + var secretsUpdateDataCalled bool + w.controller.secretsUpdateData = func(_ context.Context, _ *cmapi.Certificate, secretData secretsmanager.SecretData) error { + secretsUpdateDataCalled = true + assert.Equal(t, *test.expSecretUpdateDataCall, secretData) + return nil + } + t.Cleanup(func() { + assert.Equal(t, test.expSecretUpdateDataCall != nil, secretsUpdateDataCalled) + }) + test.builder.Start() key, err := cache.MetaNamespaceKeyFunc(test.certificate) diff --git a/pkg/controller/certificates/issuing/temporary.go b/pkg/controller/certificates/issuing/temporary.go index c47b69726..3173039ca 100644 --- a/pkg/controller/certificates/issuing/temporary.go +++ b/pkg/controller/certificates/issuing/temporary.go @@ -77,7 +77,7 @@ func (c *controller) ensureTemporaryCertificate(ctx context.Context, crt *cmapi. Certificate: certData, PrivateKey: pkData, } - if err := c.secretsManager.UpdateData(ctx, crt, secretData); err != nil { + if err := c.secretsUpdateData(ctx, crt, secretData); err != nil { return false, err } diff --git a/pkg/controller/certificates/secrettemplate/BUILD.bazel b/pkg/controller/certificates/secrettemplate/BUILD.bazel index 41bd20117..dd55fc56b 100644 --- a/pkg/controller/certificates/secrettemplate/BUILD.bazel +++ b/pkg/controller/certificates/secrettemplate/BUILD.bazel @@ -57,7 +57,6 @@ go_test( "//pkg/apis/meta/v1:go_default_library", "//pkg/controller/certificates/internal/secretsmanager:go_default_library", "//pkg/controller/test:go_default_library", - "//pkg/util/feature:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/certificates/secrettemplate/secret_template_controller.go b/pkg/controller/certificates/secrettemplate/secret_template_controller.go index d1251d58f..b45367ed8 100644 --- a/pkg/controller/certificates/secrettemplate/secret_template_controller.go +++ b/pkg/controller/certificates/secrettemplate/secret_template_controller.go @@ -104,10 +104,8 @@ func NewController( } secretsManager := secretsmanager.New( - kubeClient, - secretsInformer.Lister(), - restConfig, - certificateControllerOptions.EnableOwnerRef, + kubeClient.CoreV1(), secretsInformer.Lister(), + restConfig, certificateControllerOptions.EnableOwnerRef, ) return &controller{ diff --git a/pkg/controller/certificates/secrettemplate/secret_template_controller_test.go b/pkg/controller/certificates/secrettemplate/secret_template_controller_test.go index 3e39e670b..7d33d6987 100644 --- a/pkg/controller/certificates/secrettemplate/secret_template_controller_test.go +++ b/pkg/controller/certificates/secrettemplate/secret_template_controller_test.go @@ -28,23 +28,11 @@ import ( cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" "github.com/jetstack/cert-manager/pkg/controller/certificates/internal/secretsmanager" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" - utilfeature "github.com/jetstack/cert-manager/pkg/util/feature" ) func Test_ProcessItem(t *testing.T) { const fieldManager = "cert-manager-unit-tests" - // Enable SecretTemplate feature gate. This controller will only be - // registered if this feature gate is enabled. - assert.NoError(t, - utilfeature.DefaultMutableFeatureGate.Set("ExperimentalSecretApplySecretTemplateControllerMinKubernetesVTODO=true"), - ) - t.Cleanup(func() { - assert.NoError(t, - utilfeature.DefaultMutableFeatureGate.Set("ExperimentalSecretApplySecretTemplateControllerMinKubernetesVTODO=false"), - ) - }) - tests := map[string]struct { // key that should be passed to ProcessItem. // if not set, the 'namespace/name' of the 'Certificate' field will be used.