From 1ade01f8191c97e90d301679409825ba003a7002 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Sat, 14 May 2022 14:24:13 +0100 Subject: [PATCH] Addressed code review feedback and simplified the unit-tests Signed-off-by: Richard Wall --- pkg/controller/acmechallenges/controller.go | 18 ++--- pkg/controller/acmechallenges/sync.go | 11 ++- pkg/controller/acmechallenges/sync_test.go | 50 ++++++++++++ pkg/controller/acmechallenges/update.go | 45 +++++++---- pkg/controller/acmechallenges/update_test.go | 84 ++++++++++---------- 5 files changed, 136 insertions(+), 72 deletions(-) diff --git a/pkg/controller/acmechallenges/controller.go b/pkg/controller/acmechallenges/controller.go index 7e12942c0..91dc22291 100644 --- a/pkg/controller/acmechallenges/controller.go +++ b/pkg/controller/acmechallenges/controller.go @@ -30,7 +30,6 @@ import ( "github.com/cert-manager/cert-manager/internal/ingress" "github.com/cert-manager/cert-manager/pkg/acme/accounts" - cmclient "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned" cmacmelisters "github.com/cert-manager/cert-manager/pkg/client/listers/acme/v1" cmlisters "github.com/cert-manager/cert-manager/pkg/client/listers/certmanager/v1" controllerpkg "github.com/cert-manager/cert-manager/pkg/controller" @@ -54,9 +53,6 @@ type controller struct { clusterIssuerLister cmlisters.ClusterIssuerLister secretLister corelisters.SecretLister - // fieldManager is the manager name used for the Apply operations. - fieldManager string - // ACME challenge solvers are instantiated once at the time of controller // construction. // This also allows for easy mocking of the different challenge mechanisms. @@ -69,8 +65,6 @@ type controller struct { // used to record Events about resources to the API recorder record.EventRecorder - // clientset used to update cert-manager API resources - cmClient cmclient.Interface // maintain a reference to the workqueue for this controller // so the handleOwnedResource method can enqueue resources @@ -82,6 +76,10 @@ type controller struct { dns01Nameservers []string DNS01CheckRetryPeriod time.Duration + + // objectUpdater implements the updateObject function which is used to save + // changes to the Challenge.Status and Challenge.Finalizers + objectUpdater } func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitingInterface, []cache.InformerSynced, error) { @@ -140,8 +138,6 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin c.helper = issuer.NewHelper(c.issuerLister, c.clusterIssuerLister) c.scheduler = scheduler.New(logf.NewContext(ctx.RootContext, c.log), c.challengeLister, ctx.SchedulerOptions.MaxConcurrentChallenges) c.recorder = ctx.Recorder - c.cmClient = ctx.CMClient - c.fieldManager = ctx.FieldManager c.accountRegistry = ctx.ACMEOptions.AccountRegistry c.httpSolver, err = http.NewSolver(ctx) @@ -157,6 +153,10 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin c.dns01Nameservers = ctx.ACMEOptions.DNS01Nameservers c.DNS01CheckRetryPeriod = ctx.ACMEOptions.DNS01CheckRetryPeriod + // Construct an objectUpdater which is used to save changes to the Challenge + // object, either using Update or using Patch + Server Side Apply. + c.objectUpdater = newObjectUpdater(ctx.CMClient, ctx.FieldManager) + return c.queue, mustSync, nil } @@ -185,7 +185,7 @@ func (c *controller) runScheduler(ctx context.Context) { log := logf.WithResource(log, chOriginal) ch := chOriginal.DeepCopy() ch.Status.Processing = true - if err := newObjectUpdater(c.cmClient, c.fieldManager).updateObject(ctx, chOriginal, ch); err != nil { + if err := c.updateObject(ctx, chOriginal, ch); err != nil { log.Error(err, "error scheduling challenge for processing") return } diff --git a/pkg/controller/acmechallenges/sync.go b/pkg/controller/acmechallenges/sync.go index 1157097ad..695ade4f0 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -65,10 +65,13 @@ func (c *controller) Sync(ctx context.Context, chOriginal *cmacme.Challenge) (er ch := chOriginal.DeepCopy() defer func() { - err = utilerrors.NewAggregate([]error{ - err, - newObjectUpdater(c.cmClient, c.fieldManager).updateObject(ctx, chOriginal, ch), - }) + if updateError := c.updateObject(ctx, chOriginal, ch); updateError != nil { + if errors.Is(updateError, argumentError) { + log.Error(updateError, "If this error occurs there is a bug in cert-manager. Please report it. Not retrying.") + return + } + err = utilerrors.NewAggregate([]error{err, updateError}) + } }() if !ch.DeletionTimestamp.IsZero() { diff --git a/pkg/controller/acmechallenges/sync_test.go b/pkg/controller/acmechallenges/sync_test.go index 6f064b0a0..1a84feccd 100644 --- a/pkg/controller/acmechallenges/sync_test.go +++ b/pkg/controller/acmechallenges/sync_test.go @@ -18,6 +18,7 @@ package acmechallenges import ( "context" + "errors" "fmt" "testing" @@ -88,6 +89,8 @@ func TestSyncHappyPath(t *testing.T) { ) deletedChallenge := gen.ChallengeFrom(baseChallenge, gen.SetChallengeDeletionTimestamp(metav1.Now())) + + simulatedCleanupError := errors.New("simulated-cleanup-error") tests := map[string]testT{ "cleanup if the challenge is deleted and remove the finalizer": { challenge: gen.ChallengeFrom(deletedChallenge, @@ -121,6 +124,53 @@ func TestSyncHappyPath(t *testing.T) { }, }, }, + "if the challenge is deleted and the cleanup fails, set the reason (and remove the finalizer, which is a bug)": { + challenge: gen.ChallengeFrom(deletedChallenge, + gen.SetChallengeProcessing(true), + gen.SetChallengeURL("testurl"), + gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), + ), + httpSolver: &fakeSolver{ + fakeCleanUp: func(ctx context.Context, issuer v1.GenericIssuer, ch *cmacme.Challenge) error { + return simulatedCleanupError + }, + }, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.ChallengeFrom(deletedChallenge, + gen.SetChallengeProcessing(true), + gen.SetChallengeURL("testurl"), + gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), + ), + testIssuerHTTP01Enabled, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateAction(cmacme.SchemeGroupVersion.WithResource("challenges"), + gen.DefaultTestNamespace, + gen.ChallengeFrom(deletedChallenge, + gen.SetChallengeProcessing(true), + gen.SetChallengeURL("testurl"), + gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), + gen.SetChallengeFinalizers([]string{}), + gen.SetChallengeReason(simulatedCleanupError.Error()), + ))), + testpkg.NewAction( + coretesting.NewUpdateSubresourceAction(cmacme.SchemeGroupVersion.WithResource("challenges"), + "status", + gen.DefaultTestNamespace, + gen.ChallengeFrom(deletedChallenge, + gen.SetChallengeProcessing(true), + gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), + gen.SetChallengeURL("testurl"), + gen.SetChallengeFinalizers([]string{}), + gen.SetChallengeReason(simulatedCleanupError.Error()), + ))), + }, + ExpectedEvents: []string{ + fmt.Sprintf("Warning CleanUpError Error cleaning up challenge: %s", simulatedCleanupError), + }, + }, + }, "if finalizer is missing, add it": { challenge: gen.ChallengeFrom(baseChallenge, gen.SetChallengeProcessing(true), diff --git a/pkg/controller/acmechallenges/update.go b/pkg/controller/acmechallenges/update.go index 006f50685..a8ce45194 100644 --- a/pkg/controller/acmechallenges/update.go +++ b/pkg/controller/acmechallenges/update.go @@ -18,6 +18,8 @@ package acmechallenges import ( "context" + stderrors "errors" + "fmt" "github.com/pkg/errors" apiequality "k8s.io/apimachinery/pkg/api/equality" @@ -33,21 +35,27 @@ import ( "github.com/cert-manager/cert-manager/test/unit/gen" ) -type updater interface { +var argumentError = stderrors.New("invalid arguments") + +type objectUpdateClient interface { update(context.Context, *cmacme.Challenge) (*cmacme.Challenge, error) updateStatus(context.Context, *cmacme.Challenge) (*cmacme.Challenge, error) } -type objectUpdater struct { - updater +type objectUpdater interface { + updateObject(context.Context, *cmacme.Challenge, *cmacme.Challenge) error } -func newObjectUpdater(cl versioned.Interface, fieldManager string) *objectUpdater { - o := &objectUpdater{ - updater: &objectUpdaterDefault{cl: cl}, +type defaultObjectUpdater struct { + objectUpdateClient +} + +func newObjectUpdater(cl versioned.Interface, fieldManager string) objectUpdater { + o := &defaultObjectUpdater{ + objectUpdateClient: &objectUpdateClientDefault{cl: cl}, } if utilfeature.DefaultFeatureGate.Enabled(feature.ServerSideApply) { - o.updater = &objectUpdaterSSA{ + o.objectUpdateClient = &objectUpdateClientSSA{ fieldManager: fieldManager, cl: cl, } @@ -64,13 +72,18 @@ func newObjectUpdater(cl versioned.Interface, fieldManager string) *objectUpdate // will exit without error and the remaining operations will be skipped. // Only the Finalizers and Status fields may be modified. If there are any // modifications to new object, outside of the Finalizers and Status fields, -// this function will panic. -func (o *objectUpdater) updateObject(ctx context.Context, old, new *cmacme.Challenge) error { +// this function return an error. +func (o *defaultObjectUpdater) updateObject(ctx context.Context, old, new *cmacme.Challenge) error { if !apiequality.Semantic.DeepEqual( gen.ChallengeFrom(old, gen.SetChallengeFinalizers(nil), gen.ResetChallengeStatus()), gen.ChallengeFrom(new, gen.SetChallengeFinalizers(nil), gen.ResetChallengeStatus()), ) { - panic("only the finalizers and status fields may be modified") + return errors.WithStack( + fmt.Errorf( + "%w: in updateObject: unexpected differences between old and new: only the finalizers and status fields may be modified", + argumentError, + ), + ) } var updateFunctions []func() (*cmacme.Challenge, error) @@ -106,27 +119,27 @@ func (o *objectUpdater) updateObject(ctx context.Context, old, new *cmacme.Chall return utilerrors.NewAggregate(errors) } -type objectUpdaterDefault struct { +type objectUpdateClientDefault struct { cl versioned.Interface } -func (o *objectUpdaterDefault) update(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { +func (o *objectUpdateClientDefault) update(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { return o.cl.AcmeV1().Challenges(new.Namespace).Update(ctx, new, metav1.UpdateOptions{}) } -func (o *objectUpdaterDefault) updateStatus(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { +func (o *objectUpdateClientDefault) updateStatus(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { return o.cl.AcmeV1().Challenges(new.Namespace).UpdateStatus(ctx, new, metav1.UpdateOptions{}) } -type objectUpdaterSSA struct { +type objectUpdateClientSSA struct { cl versioned.Interface fieldManager string } -func (o *objectUpdaterSSA) update(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { +func (o *objectUpdateClientSSA) update(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { return internalchallenges.Apply(ctx, o.cl, o.fieldManager, new) } -func (o *objectUpdaterSSA) updateStatus(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { +func (o *objectUpdateClientSSA) updateStatus(ctx context.Context, new *cmacme.Challenge) (*cmacme.Challenge, error) { return internalchallenges.ApplyStatus(ctx, o.cl, o.fieldManager, new) } diff --git a/pkg/controller/acmechallenges/update_test.go b/pkg/controller/acmechallenges/update_test.go index 8311ee271..a41f640c2 100644 --- a/pkg/controller/acmechallenges/update_test.go +++ b/pkg/controller/acmechallenges/update_test.go @@ -19,6 +19,7 @@ package acmechallenges import ( "context" "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -39,7 +40,11 @@ func TestUpdateObjectStandard(t *testing.T) { } func TestUpdateObjectSSA(t *testing.T) { - t.Skip("Server Side Apply cannot be tested because PatchType is not supported by the fake versioned client") + t.Skip( + "Server Side Apply cannot be tested because PatchType is not supported by the fake versioned client. See:", + "https://github.com/kubernetes/client-go/issues/970", + "https://github.com/kubernetes/client-go/issues/992", + ) defer featuretesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, feature.ServerSideApply, true)() runUpdateObjectTests(t) } @@ -49,12 +54,12 @@ func runUpdateObjectTests(t *testing.T) { simulatedUpdateStatusError := errors.New("simulated-update-status-error") tests := []struct { - name string - mods []gen.ChallengeModifier - notFound bool - apiResponse clienttesting.ReactionFunc - panicMessage string - errorMessage string + name string + mods []gen.ChallengeModifier + notFound bool + updateError error + updateStatusError error + errorMessage string }{ // Modifying the finalizers and any status fields results in both // finalizers and status being updated. @@ -76,13 +81,16 @@ func runUpdateObjectTests(t *testing.T) { notFound: true, }, // Only the Finalizers and Status fields can be updated. Updates to any - // other fields suggests a programming error and results in a panic. + // other fields suggests a programming error an argument error. { - name: "panic-on-non-finalizer-non-status-modifications", + name: "error-on-non-finalizer-non-status-modifications", mods: []gen.ChallengeModifier{ gen.SetChallengeDNSName("new-dns-name"), }, - panicMessage: "only the finalizers and status fields may be modified", + errorMessage: fmt.Sprintf( + "%s: in updateObject: unexpected differences between old and new: only the finalizers and status fields may be modified", + argumentError, + ), }, // If the Update API call fails, that error is returned. { @@ -90,13 +98,8 @@ func runUpdateObjectTests(t *testing.T) { mods: []gen.ChallengeModifier{ gen.SetChallengeFinalizers([]string{"example.com/another-finalizer"}), }, - apiResponse: func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { - if action.GetSubresource() == "" { - return true, nil, simulatedUpdateError - } - return false, nil, nil - }, - errorMessage: "when updating the finalizers: simulated-update-error", + updateError: simulatedUpdateError, + errorMessage: fmt.Sprintf("when updating the finalizers: %s", simulatedUpdateError), }, // If the UpdateStatus API call fails, that error is returned. { @@ -104,13 +107,8 @@ func runUpdateObjectTests(t *testing.T) { mods: []gen.ChallengeModifier{ gen.SetChallengePresented(true), }, - apiResponse: func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { - if action.GetSubresource() == "status" { - return true, nil, simulatedUpdateStatusError - } - return false, nil, nil - }, - errorMessage: "when updating the status: simulated-update-status-error", + updateStatusError: simulatedUpdateStatusError, + errorMessage: fmt.Sprintf("when updating the status: %s", simulatedUpdateStatusError), }, // If both Update and UpdateStatus API calls fail, both errors are returned. { @@ -119,16 +117,13 @@ func runUpdateObjectTests(t *testing.T) { gen.SetChallengeFinalizers([]string{"example.com/another-finalizer"}), gen.SetChallengePresented(true), }, - apiResponse: func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { - if action.GetSubresource() == "" { - return true, nil, simulatedUpdateError - } - if action.GetSubresource() == "status" { - return true, nil, simulatedUpdateStatusError - } - return false, nil, nil - }, - errorMessage: "[when updating the status: simulated-update-status-error, when updating the finalizers: simulated-update-error]", + updateError: simulatedUpdateError, + updateStatusError: simulatedUpdateStatusError, + errorMessage: fmt.Sprintf( + "[when updating the status: %s, when updating the finalizers: %s]", + simulatedUpdateStatusError, + simulatedUpdateError, + ), }, } for _, tt := range tests { @@ -142,17 +137,20 @@ func runUpdateObjectTests(t *testing.T) { objects = nil } cl := fake.NewSimpleClientset(objects...) - if tt.apiResponse != nil { - t.Log("Simulating an API server 'update' error") - cl.PrependReactor("update", "*", tt.apiResponse) + if tt.updateError != nil { + cl.PrependReactor("update", "challenges", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + t.Log("Simulating a challenge update error") + return true, nil, tt.updateError + }) + } + if tt.updateStatusError != nil { + cl.PrependReactor("update", "challenges/status", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + t.Log("Simulating a challenge/status update error") + return true, nil, tt.updateStatusError + }) } updater := newObjectUpdater(cl, "test-fieldmanager") - if tt.panicMessage != "" { - assert.PanicsWithValue(t, tt.panicMessage, func() { _ = updater.updateObject(ctx, old, new) }, - "updateObject should panic when changes are made to fields other than Finalizers and Status") - return - } - t.Log("Executing the function") + t.Log("Calling updateObject") updateObjectErr := updater.updateObject(ctx, old, new) if tt.errorMessage == "" { assert.NoError(t, updateObjectErr)