From 71e707387acdfa41d355eaf6962fcbe43e1884a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Sun, 21 Mar 2021 15:42:09 +0100 Subject: [PATCH] trigger-controller: refactor test, inject gatherer and policychain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Injecting the whole Gatherer struct was not necessary for testing since DataForCertificate is now fully unit-tested. With that, we can mock the Gatherer.Evaluate function. Since there is no reason to inject a full Gatherer object into the trigger controller, I chose to inject a simple policies.Func. I named the function "shouldReissue" since this is exactly what this function does. I also refactored the test cases to use the same gen.Certificate that we use in the rest of the codebase. Signed-off-by: Maël Valais --- .../certificates/trigger/BUILD.bazel | 2 - .../trigger/trigger_controller.go | 27 +- .../trigger/trigger_controller_test.go | 570 +++++++----------- .../certificates/trigger_controller_test.go | 7 +- 4 files changed, 221 insertions(+), 385 deletions(-) diff --git a/pkg/controller/certificates/trigger/BUILD.bazel b/pkg/controller/certificates/trigger/BUILD.bazel index 0ed7bd5b3..0b64e5ea9 100644 --- a/pkg/controller/certificates/trigger/BUILD.bazel +++ b/pkg/controller/certificates/trigger/BUILD.bazel @@ -55,14 +55,12 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/apis/certmanager/v1:go_default_library", - "//pkg/apis/meta/v1:go_default_library", "//pkg/controller:go_default_library", "//pkg/controller/certificates/trigger/policies:go_default_library", "//pkg/controller/test:go_default_library", "//test/unit/gen:go_default_library", "@com_github_go_logr_logr//testing: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", "@io_k8s_client_go//testing:go_default_library", "@io_k8s_utils//clock/testing:go_default_library", diff --git a/pkg/controller/certificates/trigger/trigger_controller.go b/pkg/controller/certificates/trigger/trigger_controller.go index 6e38f3974..b4a90e05a 100644 --- a/pkg/controller/certificates/trigger/trigger_controller.go +++ b/pkg/controller/certificates/trigger/trigger_controller.go @@ -62,16 +62,17 @@ const ( // It triggers re-issuance by adding the `Issuing` status condition when a new // certificate is required. type controller struct { - // the trigger policies to run - named here to make testing simpler - policyChain policies.Chain certificateLister cmlisters.CertificateLister certificateRequestLister cmlisters.CertificateRequestLister secretLister corelisters.SecretLister client cmclient.Interface recorder record.EventRecorder - clock clock.Clock scheduledWorkQueue scheduler.ScheduledWorkQueue - gatherer *policies.Gatherer + + // The following is used for testing purposes. + clock clock.Clock + shouldReissue policies.Func + dataForCertificate func(context.Context, *cmapi.Certificate) (policies.Input, error) } func NewController( @@ -81,7 +82,7 @@ func NewController( cmFactory cminformers.SharedInformerFactory, recorder record.EventRecorder, clock clock.Clock, - chain policies.Chain, + shouldReissue policies.Func, ) (*controller, workqueue.RateLimitingInterface, []cache.InformerSynced) { // create a queue used to queue up items to be processed queue := workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(time.Second*1, time.Second*30), ControllerName) @@ -113,18 +114,20 @@ func NewController( } return &controller{ - policyChain: chain, certificateLister: certificateInformer.Lister(), certificateRequestLister: certificateRequestInformer.Lister(), secretLister: secretsInformer.Lister(), client: client, recorder: recorder, - clock: clock, scheduledWorkQueue: scheduler.NewScheduledWorkQueue(clock, queue.Add), - gatherer: &policies.Gatherer{ + + // The following is used for testing purposes. + clock: clock, + shouldReissue: shouldReissue, + dataForCertificate: (&policies.Gatherer{ CertificateRequestLister: certificateRequestInformer.Lister(), SecretLister: secretsInformer.Lister(), - }, + }).DataForCertificate, }, queue, mustSync } @@ -153,7 +156,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return nil } - input, err := c.gatherer.DataForCertificate(ctx, crt) + input, err := c.dataForCertificate(ctx, crt) if err != nil { return err } @@ -173,7 +176,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { c.scheduleRecheckOfCertificateIfRequired(log, key, crt.Status.RenewalTime.Time.Sub(c.clock.Now())) } - reason, message, reissue := c.policyChain.Evaluate(input) + reason, message, reissue := c.shouldReissue(input) if !reissue { // no re-issuance required, return early return nil @@ -250,7 +253,7 @@ func (c *controllerWrapper) Register(ctx *controllerpkg.Context) (workqueue.Rate ctx.SharedInformerFactory, ctx.Recorder, ctx.Clock, - policies.NewTriggerPolicyChain(ctx.Clock, cmapi.DefaultRenewBefore), + policies.NewTriggerPolicyChain(ctx.Clock, cmapi.DefaultRenewBefore).Evaluate, ) c.controller = ctrl diff --git a/pkg/controller/certificates/trigger/trigger_controller_test.go b/pkg/controller/certificates/trigger/trigger_controller_test.go index 34bb47ad8..07888c491 100644 --- a/pkg/controller/certificates/trigger/trigger_controller_test.go +++ b/pkg/controller/certificates/trigger/trigger_controller_test.go @@ -18,77 +18,57 @@ package trigger import ( "context" + "fmt" "testing" "time" - corev1 "k8s.io/api/core/v1" + logtest "github.com/go-logr/logr/testing" + "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" coretesting "k8s.io/client-go/testing" fakeclock "k8s.io/utils/clock/testing" - logtest "github.com/go-logr/logr/testing" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" - cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" controllerpkg "github.com/jetstack/cert-manager/pkg/controller" "github.com/jetstack/cert-manager/pkg/controller/certificates/trigger/policies" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" "github.com/jetstack/cert-manager/test/unit/gen" - "github.com/stretchr/testify/assert" ) -// policyFuncBuilder wraps a policies.Func to allow injecting a testing.T -type policyFuncBuilder func(t *testing.T) policies.Func - func Test_controller_ProcessItem(t *testing.T) { - // now time is the current time at the start of the test (the clock is fixed) - now := time.Now() - metaNow := metav1.NewTime(now) - forceTriggeredReason := "ForceTriggered" - forceTriggeredMessage := "Re-issuance forced by unit test case" + fixedNow := metav1.NewTime(time.Now()) + tests := map[string]struct { - // key that should be passed to ProcessItem. - // if not set, the 'namespace/name' of the 'Certificate' field will be used. - // if neither is set, the key will be "" + // key that should be passed to ProcessItem. If not set, the + // 'namespace/name' of the 'Certificate' field will be used. If neither + // is set, the key will be "". key string - // Certificate to be synced for the test. - // if not set, the 'key' will be passed to ProcessItem instead. - certificate *cmapi.Certificate + // Certificate to be synced for the test. if not set, the 'key' will be + // passed to ProcessItem instead. + existingCertificate *cmapi.Certificate - // Secret, if set, will exist in the apiserver before the test is run. - secret *corev1.Secret + mockDataForCertificateReturn policies.Input + mockDataForCertificateReturnErr error + wantDataForCertificateCalled bool - // Request, if set, will exist in the apiserver before the test is run. - requests []*cmapi.CertificateRequest + mockShouldReissue func(t *testing.T) policies.Func + wantShouldReissueCalled bool - // optional chain of policy functions that should be run, wrapped with - // the policyFuncBuilder to allow injecting the sub-test's testing.T. - policyFuncs []policyFuncBuilder - - // chainShouldEvaluate will cause the test to error if the policy chain - // was not attempted to be evaluated - chainShouldEvaluate bool - // chainShouldTriggerIssuance will cause the policy chain used in the - // test to trigger issuance. - // This policyFunc will be injected at the end of the policy chain. - // If false, the policyFunc that forces an issuance will not be injected - // but user-provided policyFuncs will still behave as usual. - chainShouldTriggerIssuance bool - - // expectedEvent, if set, is an 'event string' that is expected to be fired. + // wantEvent, if set, is an 'event string' that is expected to be fired. // For example, "Normal Issuing Re-issuance forced by unit test case" // where 'Normal' is the event severity, 'Issuing' is the reason and the // remainder is the message. - expectedEvent string + wantEvent string - // expectedConditions is the expected set of conditions on the Certificate + // wantConditions is the expected set of conditions on the Certificate // resource if an Update is made. // If nil, no update is expected. // If empty, an update to the empty set/nil is expected. - expectedConditions []cmapi.CertificateCondition + wantConditions []cmapi.CertificateCondition - // err is the expected error text returned by the controller, if any. - err string + // wantErr is the expected error text returned by the controller, if any. + wantErr string }{ "do nothing if an empty 'key' is used": {}, "do nothing if an invalid 'key' is used": { @@ -97,412 +77,266 @@ func Test_controller_ProcessItem(t *testing.T) { "do nothing if a key references a Certificate that does not exist": { key: "namespace/name", }, - "do nothing if Certificate already has 'Issuing' condition": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Status: cmapi.CertificateStatus{ - Conditions: []cmapi.CertificateCondition{ - { - Type: cmapi.CertificateConditionIssuing, - Status: cmmeta.ConditionTrue, - ObservedGeneration: 3, - }, - }, - }, - }, - }, - "evaluate policy chain with only the Certificate if no Request or Secret exists": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - }, - chainShouldEvaluate: true, - policyFuncs: []policyFuncBuilder{ - // Add a policy function that ensures only the input's 'certificate' - // field is set. - func(t *testing.T) policies.Func { - return func(input policies.Input) (string, string, bool) { - if input.Certificate == nil { - t.Error("expected policy data 'Certificate' field to be set but it was not") - } - if input.Secret != nil { - t.Errorf("expected policy data 'Secret' field to be unset but it was: %+v", input.Secret) - } - if input.CurrentRevisionRequest != nil { - t.Errorf("expected policy data 'CurrentRevisionRequest' field to be unset but it was: %+v", input.CurrentRevisionRequest) - } - return "", "", false - } - }, - }, - }, - "evaluate policy chain with the Certificate and Secret if no Request exists": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Spec: cmapi.CertificateSpec{ - SecretName: "test-secret", - }, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test-secret"}, - }, - chainShouldEvaluate: true, - policyFuncs: []policyFuncBuilder{ - // Add a policy function that ensures only the input's 'certificate' - // field is set. - func(t *testing.T) policies.Func { - return func(input policies.Input) (string, string, bool) { - if input.Certificate == nil { - t.Error("expected policy data 'Certificate' field to be set but it was not") - } - if input.Secret == nil { - t.Errorf("expected policy data 'Secret' field to be set but it was not") - } - if input.CurrentRevisionRequest != nil { - t.Errorf("expected policy data 'CurrentRevisionRequest' field to be unset but it was: %+v", input.CurrentRevisionRequest) - } - return "", "", false - } - }, - }, - }, - "evaluate policy chain with the Certificate, Secret and Request if one exists": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Spec: cmapi.CertificateSpec{ - SecretName: "test-secret", - }, - Status: cmapi.CertificateStatus{ - Revision: func(i int) *int { return &i }(3), - }, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test-secret"}, - }, - requests: []*cmapi.CertificateRequest{ - { - ObjectMeta: metav1.ObjectMeta{ - Namespace: "testns", - Name: "test", - Annotations: map[string]string{ - cmapi.CertificateRequestRevisionAnnotationKey: "3", - }, - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}}, cmapi.SchemeGroupVersion.WithKind("Certificate")), - }, - }, - }, - }, - chainShouldEvaluate: true, - policyFuncs: []policyFuncBuilder{ - // Add a policy function that ensures only the input's 'certificate' - // field is set. - func(t *testing.T) policies.Func { - return func(input policies.Input) (string, string, bool) { - if input.Certificate == nil { - t.Error("expected policy data 'Certificate' field to be set but it was not") - } - if input.Secret == nil { - t.Errorf("expected policy data 'Secret' field to be set but it was not") - } - if input.CurrentRevisionRequest == nil { - t.Errorf("expected policy data 'CurrentRevisionRequest' field to be set but it was not") - } - return "", "", false - } - }, - }, - }, - "error if multiple owned CertificateRequest resources exist and have the same revision": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Spec: cmapi.CertificateSpec{ - SecretName: "test-secret", - }, - Status: cmapi.CertificateStatus{ - Revision: func(i int) *int { return &i }(3), - }, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test-secret"}, - }, - requests: []*cmapi.CertificateRequest{ - { - ObjectMeta: metav1.ObjectMeta{ - Namespace: "testns", - Name: "test", - Annotations: map[string]string{ - cmapi.CertificateRequestRevisionAnnotationKey: "3", - }, - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}}, cmapi.SchemeGroupVersion.WithKind("Certificate")), - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Namespace: "testns", - Name: "test-number-two", - Annotations: map[string]string{ - cmapi.CertificateRequestRevisionAnnotationKey: "3", - }, - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test"}}, cmapi.SchemeGroupVersion.WithKind("Certificate")), - }, - }, - }, - }, - chainShouldEvaluate: false, - err: "multiple CertificateRequest resources exist for the current revision, not triggering new issuance until requests have been cleaned up", - }, - "should evaluate policy if no certificaterequest resource exists for the current revision": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Spec: cmapi.CertificateSpec{ - SecretName: "test-secret", - }, - Status: cmapi.CertificateStatus{ - Revision: func(i int) *int { return &i }(3), - }, - }, - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test-secret"}, - }, - chainShouldEvaluate: true, - }, - "should set the 'Issuing' status condition if the chain indicates an issuance is required": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - }, - chainShouldEvaluate: true, - chainShouldTriggerIssuance: true, - expectedEvent: "Normal Issuing Re-issuance forced by unit test case", - expectedConditions: []cmapi.CertificateCondition{ - { - Type: cmapi.CertificateConditionIssuing, - Status: cmmeta.ConditionTrue, - Reason: forceTriggeredReason, - Message: forceTriggeredMessage, - LastTransitionTime: &metaNow, + "should do nothing if Certificate already has 'Issuing' condition": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateGeneration(42), + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: "Issuing", + Status: "True", ObservedGeneration: 3, - }, + }), + ), + }, + "should call shouldReissue with the correct cert, secret and current CR": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateSecretName("secret-1"), + gen.SetCertificateGeneration(42), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(2), + ), + wantDataForCertificateCalled: true, + mockDataForCertificateReturn: policies.Input{ + Secret: gen.Secret("secret-1", gen.SetSecretNamespace("testns")), + CurrentRevisionRequest: gen.CertificateRequest("cr-1", gen.SetCertificateRequestNamespace("testns"), + gen.AddCertificateRequestOwnerReferences(gen.CertificateRef("cert-1", "cert-1-uid")), + gen.SetCertificateRequestAnnotations(map[string]string{"cert-manager.io/certificate-revision": "2"}), + ), + }, + wantShouldReissueCalled: true, + mockShouldReissue: func(t *testing.T) policies.Func { + return func(gotInput policies.Input) (string, string, bool) { + expectInput := policies.Input{ + Certificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateSecretName("secret-1"), + gen.SetCertificateGeneration(42), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(2), + ), + CurrentRevisionRequest: gen.CertificateRequest("cr-1", gen.SetCertificateRequestNamespace("testns"), + gen.AddCertificateRequestOwnerReferences(gen.CertificateRef("cert-1", "cert-1-uid")), + gen.SetCertificateRequestAnnotations(map[string]string{"cert-manager.io/certificate-revision": "2"}), + ), + Secret: gen.Secret("secret-1", gen.SetSecretNamespace("testns")), + } + assert.Equal(t, expectInput, gotInput) + return "", "", false + } }, }, - "should not set the 'Issuing' status condition if the chain indicates an issuance is required if the last failure time is within the last hour": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Status: cmapi.CertificateStatus{ - LastFailureTime: func(m metav1.Time) *metav1.Time { return &m }(metav1.NewTime(now.Add(-59 * time.Minute))), - }, - }, - chainShouldEvaluate: false, - chainShouldTriggerIssuance: false, + "should log error when dataForCertificate errors": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns")), + wantDataForCertificateCalled: true, + mockDataForCertificateReturnErr: fmt.Errorf("dataForCertificate failed"), + wantErr: "dataForCertificate failed", }, - "should set the 'Issuing' status condition if the chain indicates an issuance is required if the last failure time is older than the last hour": { - certificate: &cmapi.Certificate{ - ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "test", Generation: 3}, - Status: cmapi.CertificateStatus{ - LastFailureTime: func(m metav1.Time) *metav1.Time { return &m }(metav1.NewTime(now.Add(-61 * time.Minute))), - }, + "should set Issuing=True if shouldReissue tells us to reissue": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateGeneration(42), + ), + wantDataForCertificateCalled: true, + mockDataForCertificateReturn: policies.Input{}, + wantShouldReissueCalled: true, + mockShouldReissue: func(*testing.T) policies.Func { + return func(policies.Input) (string, string, bool) { + return "ForceTriggered", "Re-issuance forced by unit test case", true + } }, - chainShouldEvaluate: true, - chainShouldTriggerIssuance: true, - expectedEvent: "Normal Issuing Re-issuance forced by unit test case", - expectedConditions: []cmapi.CertificateCondition{ - { - Type: cmapi.CertificateConditionIssuing, - Status: cmmeta.ConditionTrue, - Reason: forceTriggeredReason, - Message: forceTriggeredMessage, - LastTransitionTime: &metaNow, - ObservedGeneration: 3, - }, + wantEvent: "Normal Issuing Re-issuance forced by unit test case", + wantConditions: []cmapi.CertificateCondition{{ + Type: "Issuing", + Status: "True", + Reason: "ForceTriggered", + Message: "Re-issuance forced by unit test case", + LastTransitionTime: &fixedNow, + ObservedGeneration: 42, + }}, + }, + "should not set Issuing=True when cert has been failing for 59 minutes": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateLastFailureTime(metav1.NewTime(fixedNow.Add(-59*time.Minute))), + ), + wantDataForCertificateCalled: true, + mockDataForCertificateReturn: policies.Input{}, + wantShouldReissueCalled: false, + }, + "should set Issuing=True when cert has been failing for 61 minutes and shouldReissue returns true": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateGeneration(42), + gen.SetCertificateLastFailureTime(metav1.NewTime(fixedNow.Add(-61*time.Minute))), + ), + wantDataForCertificateCalled: true, + mockDataForCertificateReturn: policies.Input{}, + wantShouldReissueCalled: true, + mockShouldReissue: func(*testing.T) policies.Func { + return func(policies.Input) (string, string, bool) { + return "ForceTriggered", "Re-issuance forced by unit test case", true + } }, + wantEvent: "Normal Issuing Re-issuance forced by unit test case", + wantConditions: []cmapi.CertificateCondition{{ + Type: "Issuing", + Status: "True", + Reason: "ForceTriggered", + Message: "Re-issuance forced by unit test case", + LastTransitionTime: &fixedNow, + ObservedGeneration: 42, + }}, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - // Create and initialise a new unit test builder builder := &testpkg.Builder{ T: t, - Clock: fakeclock.NewFakeClock(now), + Clock: fakeclock.NewFakeClock(fixedNow.Time), } - if test.certificate != nil { - builder.CertManagerObjects = append(builder.CertManagerObjects, test.certificate) - } - if test.secret != nil { - builder.KubeObjects = append(builder.KubeObjects, test.secret) - } - for _, req := range test.requests { - builder.CertManagerObjects = append(builder.CertManagerObjects, req) + if test.existingCertificate != nil { + builder.CertManagerObjects = append(builder.CertManagerObjects, test.existingCertificate) } builder.Init() - // Register informers used by the controller using the registration wrapper w := &controllerWrapper{} _, _, err := w.Register(builder.Context) if err != nil { t.Fatal(err) } - // Fake out the default policy chain - w.policyChain = []policies.Func{} - // Record whether the policy chain was evaluated - evaluated := false - w.policyChain = append(w.policyChain, func(_ policies.Input) (string, string, bool) { - evaluated = true - return "", "", false - }) - // Add any test-specific policies to the chain - w.policyChain = append(w.policyChain, buildTestPolicyChain(t, test.policyFuncs...)...) - // If the chain should trigger an issuance, inject an 'always reissue' - // policyFunc at the end of the chain - if test.chainShouldTriggerIssuance { - w.policyChain = append(w.policyChain, func(_ policies.Input) (string, string, bool) { - return forceTriggeredReason, forceTriggeredMessage, true - }) + + gotShouldReissueCalled := false + w.shouldReissue = func(i policies.Input) (string, string, bool) { + gotShouldReissueCalled = true + if test.mockShouldReissue == nil { + t.Fatal("no mock set for shouldReissue, but shouldReissue has been called") + return "", "", false + } else { + return test.mockShouldReissue(t)(i) + } } - if test.expectedConditions != nil { - if test.certificate == nil { + + // TODO(mael): we should really remove the Certificate field from + // DataForCertificate since the input certificate is always expected + // to be the same as the output certiticate. + test.mockDataForCertificateReturn.Certificate = test.existingCertificate + + gotDataForCertificateCalled := false + w.dataForCertificate = func(context.Context, *cmapi.Certificate) (policies.Input, error) { + gotDataForCertificateCalled = true + return test.mockDataForCertificateReturn, test.mockDataForCertificateReturnErr + } + + if test.wantConditions != nil { + if test.existingCertificate == nil { t.Fatal("cannot expect an Update operation if test.certificate is nil") } - expectedCert := test.certificate.DeepCopy() - expectedCert.Status.Conditions = test.expectedConditions + expectedCert := test.existingCertificate.DeepCopy() + expectedCert.Status.Conditions = test.wantConditions builder.ExpectedActions = append(builder.ExpectedActions, testpkg.NewAction(coretesting.NewUpdateSubresourceAction( cmapi.SchemeGroupVersion.WithResource("certificates"), "status", - test.certificate.Namespace, + test.existingCertificate.Namespace, expectedCert, )), ) } - if test.expectedEvent != "" { - builder.ExpectedEvents = []string{test.expectedEvent} + if test.wantEvent != "" { + builder.ExpectedEvents = []string{test.wantEvent} } - // Start the informers and begin processing updates + builder.Start() defer builder.Stop() key := test.key - if key == "" && test.certificate != nil { - key, err = controllerpkg.KeyFunc(test.certificate) + if key == "" && test.existingCertificate != nil { + key, err = controllerpkg.KeyFunc(test.existingCertificate) if err != nil { t.Fatal(err) } } - // Call ProcessItem - err = w.controller.ProcessItem(context.Background(), key) + gotErr := w.controller.ProcessItem(context.Background(), key) switch { - case err != nil: - if test.err != err.Error() { - t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err) + case gotErr != nil: + if test.wantErr != gotErr.Error() { + t.Errorf("error text did not match, got=%s, exp=%s", gotErr.Error(), test.wantErr) } default: - if test.err != "" { - t.Errorf("got no error but expected: %s", test.err) - } - } - if evaluated != test.chainShouldEvaluate { - if test.chainShouldEvaluate { - t.Error("expected policy chain to be evaluated but it was not") - } else { - t.Error("expected policy chain to NOT be evaluated but it was") + if test.wantErr != "" { + t.Errorf("got no error but expected: %s", test.wantErr) } } - if err := builder.AllEventsCalled(); err != nil { - builder.T.Error(err) - } - if err := builder.AllActionsExecuted(); err != nil { - builder.T.Error(err) - } - if err := builder.AllReactorsCalled(); err != nil { - builder.T.Error(err) - } + assert.Equal(t, test.wantDataForCertificateCalled, gotDataForCertificateCalled, "dataForCertificate func call") + assert.Equal(t, test.wantShouldReissueCalled, gotShouldReissueCalled, "shouldReissue func call") + + builder.CheckAndFinish() }) } } -func buildTestPolicyChain(t *testing.T, funcs ...policyFuncBuilder) policies.Chain { - c := policies.Chain{} - for _, f := range funcs { - c = append(c, f(t)) - } - return c -} - func Test_shouldBackoffReissuingOnFailure(t *testing.T) { - clock := fakeclock.NewFakeClock(time.Date(2020, 11, 20, 16, 05, 00, 0000, time.UTC)) - tests := []struct { - name string + clock := fakeclock.NewFakeClock(time.Date(2020, 11, 20, 16, 05, 00, 0000, time.Local)) + + tests := map[string]struct { givenCert *cmapi.Certificate wantBackoff bool wantDelay time.Duration }{ - { - name: "no need to back off from reissuing when there is no previous failure", - givenCert: gen.Certificate("test", - gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("test-uid"), - gen.SetCertificateDNSNames("example2.com"), + "should not back off from reissuing when the input request is nil": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns")), + wantBackoff: false, + }, + "should not back off from reissuing when cert is not failing": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example2.com"), // LastFailureTime is not set here. ), wantBackoff: false, }, - { - name: "no need to back off from reissuing when the failure is more than an hour ago, reissuance can happen now", - givenCert: gen.Certificate("test", - gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("test-uid"), - gen.SetCertificateDNSNames("example2.com"), + "should not back off from reissuing when the failure happened 61 minutes ago": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example2.com"), gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-61*time.Minute))), ), wantBackoff: false, }, - { - name: "needs to back off from reissuing when the failure happened less than an hour ago", - givenCert: gen.Certificate("test", - gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("test-uid"), - gen.SetCertificateDNSNames("example2.com"), + "should not back off from reissuing when the failure happened 60 minutes ago": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-60*time.Minute))), + ), + wantBackoff: false, + }, + "should back off from reissuing when the failure happened 59 minutes ago": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-59*time.Minute))), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example2.com"), ), wantBackoff: true, wantDelay: 1 * time.Minute, }, - { - name: "no need to back off from reissuing when the failure happened exactly an hour ago", - givenCert: gen.Certificate("test", - gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("test-uid"), - gen.SetCertificateDNSNames("example2.com"), - gen.SetCertificateRevision(1), - gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-60*time.Minute))), - ), - wantBackoff: false, - wantDelay: 0, - }, - { - name: "needs to back off from reissuing for the maximum of 1 hour when failure just happened", - givenCert: gen.Certificate("test", - gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("test-uid"), - gen.SetCertificateDNSNames("example2.com"), + "should back off from reissuing when the failure happened 0 minutes ago": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now())), + gen.SetCertificateDNSNames("example2.com"), ), wantBackoff: true, wantDelay: 1 * time.Hour, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotBackoff, gotDelay := shouldBackoffReissuingOnFailure(logtest.TestLogger{T: t}, clock, tt.givenCert) - assert.Equal(t, tt.wantBackoff, gotBackoff) - assert.Equal(t, tt.wantDelay, gotDelay) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + gotBackoff, gotDelay := shouldBackoffReissuingOnFailure(logtest.TestLogger{T: t}, clock, test.givenCert) + assert.Equal(t, test.wantBackoff, gotBackoff) + assert.Equal(t, test.wantDelay, gotDelay) }) } } diff --git a/test/integration/certificates/trigger_controller_test.go b/test/integration/certificates/trigger_controller_test.go index d2e32046d..6e778c489 100644 --- a/test/integration/certificates/trigger_controller_test.go +++ b/test/integration/certificates/trigger_controller_test.go @@ -67,7 +67,8 @@ func TestTriggerController(t *testing.T) { } // default certificate renewBefore period defaultRenewBefore := time.Hour * 24 - ctrl, queue, mustSync := trigger.NewController(logf.Log, cmCl, factory, cmFactory, framework.NewEventRecorder(t), fakeClock, policies.NewTriggerPolicyChain(fakeClock, defaultRenewBefore)) + shouldReissue := policies.NewTriggerPolicyChain(fakeClock, defaultRenewBefore).Evaluate + ctrl, queue, mustSync := trigger.NewController(logf.Log, cmCl, factory, cmFactory, framework.NewEventRecorder(t), fakeClock, shouldReissue) c := controllerpkg.NewController( context.Background(), "trigger_test", @@ -127,7 +128,7 @@ func TestTriggerController_RenewNearExpiry(t *testing.T) { // Only use the 'current certificate nearing expiry' policy chain during the // test as we want to test the very specific cases of triggering/not // triggering depending on whether a renewal is required. - policyChain := policies.Chain{policies.CurrentCertificateNearingExpiry(fakeClock, defaultRenewBefore)} + shoudReissue := policies.Chain{policies.CurrentCertificateNearingExpiry(fakeClock, defaultRenewBefore)}.Evaluate // Build, instantiate and run the trigger controller. kubeClient, factory, cmCl, cmFactory := framework.NewClients(t, config) @@ -182,7 +183,7 @@ func TestTriggerController_RenewNearExpiry(t *testing.T) { } // Start the trigger controller - ctrl, queue, mustSync := trigger.NewController(logf.Log, cmCl, factory, cmFactory, framework.NewEventRecorder(t), fakeClock, policyChain) + ctrl, queue, mustSync := trigger.NewController(logf.Log, cmCl, factory, cmFactory, framework.NewEventRecorder(t), fakeClock, shoudReissue) c := controllerpkg.NewController( logf.NewContext(context.Background(), logf.Log, "trigger_controller_RenewNearExpiry"), "trigger_test",