From 557d14a0cd41ab586525957887e59c163bc4d1c9 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 11 May 2022 16:39:42 +0100 Subject: [PATCH 1/2] Refactor the update and updateStatus to a single deferred function Signed-off-by: Richard Wall --- pkg/controller/acmechallenges/BUILD.bazel | 8 + pkg/controller/acmechallenges/controller.go | 10 +- pkg/controller/acmechallenges/finalizer.go | 19 -- .../acmechallenges/finalizer_test.go | 60 ------ pkg/controller/acmechallenges/sync.go | 56 ++---- pkg/controller/acmechallenges/sync_test.go | 8 - pkg/controller/acmechallenges/update.go | 132 +++++++++++++ pkg/controller/acmechallenges/update_test.go | 184 ++++++++++++++++++ pkg/controller/test/context_builder.go | 2 +- test/unit/gen/challenge.go | 6 + 10 files changed, 346 insertions(+), 139 deletions(-) create mode 100644 pkg/controller/acmechallenges/update.go create mode 100644 pkg/controller/acmechallenges/update_test.go diff --git a/pkg/controller/acmechallenges/BUILD.bazel b/pkg/controller/acmechallenges/BUILD.bazel index cac4400e8..d5fffff66 100644 --- a/pkg/controller/acmechallenges/BUILD.bazel +++ b/pkg/controller/acmechallenges/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "controller.go", "finalizer.go", "sync.go", + "update.go", ], importpath = "github.com/cert-manager/cert-manager/pkg/controller/acmechallenges", visibility = ["//visibility:public"], @@ -30,7 +31,9 @@ go_library( "//pkg/issuer/acme/http:go_default_library", "//pkg/logs:go_default_library", "//pkg/util/feature:go_default_library", + "//test/unit/gen:go_default_library", "@com_github_go_logr_logr//:go_default_library", + "@com_github_pkg_errors//:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/api/equality:go_default_library", "@io_k8s_apimachinery//pkg/api/errors:go_default_library", @@ -51,22 +54,27 @@ go_test( "controller_test.go", "finalizer_test.go", "sync_test.go", + "update_test.go", ], embed = [":go_default_library"], deps = [ + "//internal/controller/feature:go_default_library", "//pkg/acme/accounts/test:go_default_library", "//pkg/acme/client:go_default_library", "//pkg/apis/acme/v1:go_default_library", "//pkg/apis/certmanager/v1:go_default_library", "//pkg/apis/meta/v1:go_default_library", + "//pkg/client/clientset/versioned/fake:go_default_library", "//pkg/controller/test:go_default_library", "//pkg/issuer:go_default_library", + "//pkg/util/feature: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_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", "@io_k8s_client_go//testing:go_default_library", + "@io_k8s_component_base//featuregate/testing:go_default_library", "@org_golang_x_crypto//acme:go_default_library", ], ) diff --git a/pkg/controller/acmechallenges/controller.go b/pkg/controller/acmechallenges/controller.go index 7694f8214..7e12942c0 100644 --- a/pkg/controller/acmechallenges/controller.go +++ b/pkg/controller/acmechallenges/controller.go @@ -181,16 +181,14 @@ func (c *controller) runScheduler(ctx context.Context) { return } - for _, ch := range toSchedule { - log := logf.WithResource(log, ch) - ch = ch.DeepCopy() + for _, chOriginal := range toSchedule { + log := logf.WithResource(log, chOriginal) + ch := chOriginal.DeepCopy() ch.Status.Processing = true - _, err := c.updateStatusOrApply(ctx, ch) - if err != nil { + if err := newObjectUpdater(c.cmClient, c.fieldManager).updateObject(ctx, chOriginal, ch); err != nil { log.Error(err, "error scheduling challenge for processing") return } - c.recorder.Event(ch, corev1.EventTypeNormal, "Started", "Challenge scheduled for processing") } diff --git a/pkg/controller/acmechallenges/finalizer.go b/pkg/controller/acmechallenges/finalizer.go index 0e3aee06b..86d80b5ff 100644 --- a/pkg/controller/acmechallenges/finalizer.go +++ b/pkg/controller/acmechallenges/finalizer.go @@ -17,8 +17,6 @@ limitations under the License. package acmechallenges import ( - "context" - "k8s.io/apimachinery/pkg/util/sets" cmacme "github.com/cert-manager/cert-manager/pkg/apis/acme/v1" @@ -31,24 +29,7 @@ import ( // deployed ("presented") resources and if successful, removes this finalizer // allowing the garbage collector to remove the challenge. -// challengeUpdater allows the spec and metadata of a Challenge to be modified. -type challengeUpdater interface { - updateOrApply(context.Context, *cmacme.Challenge) (*cmacme.Challenge, error) -} - // finalizerRequired returns true if the finalizer is not found on the challenge. func finalizerRequired(ch *cmacme.Challenge) bool { return !sets.NewString(ch.Finalizers...).Has(cmacme.ACMEFinalizer) } - -// addFinalizer adds the finalizer to the challenge and saves the change to the API -// server. -func addFinalizer(client challengeUpdater, ctx context.Context, ch *cmacme.Challenge) (*cmacme.Challenge, error) { - ch = ch.DeepCopy() - ch.Finalizers = append(ch.Finalizers, cmacme.ACMEFinalizer) - ch, err := client.updateOrApply(ctx, ch) - if err != nil { - return nil, err - } - return ch, nil -} diff --git a/pkg/controller/acmechallenges/finalizer_test.go b/pkg/controller/acmechallenges/finalizer_test.go index ce6f73c5d..c6fae054c 100644 --- a/pkg/controller/acmechallenges/finalizer_test.go +++ b/pkg/controller/acmechallenges/finalizer_test.go @@ -17,8 +17,6 @@ limitations under the License. package acmechallenges import ( - "context" - "errors" "testing" "github.com/stretchr/testify/assert" @@ -66,61 +64,3 @@ func Test_finalizerRequired(t *testing.T) { }) } } - -type fakeChallengeUpdater struct { - fakeUpdateOrApply func(context.Context, *cmacme.Challenge) (*cmacme.Challenge, error) -} - -func (o *fakeChallengeUpdater) updateOrApply(ctx context.Context, ch *cmacme.Challenge) (*cmacme.Challenge, error) { - return o.fakeUpdateOrApply(ctx, ch) -} - -func Test_addFinalizer(t *testing.T) { - simulatedError := errors.New("simulated-error") - tests := []struct { - name string - updateErr error - wantErr error - }{ - { - name: "update-success", - updateErr: nil, - wantErr: nil, - }, - { - name: "update-failure", - updateErr: simulatedError, - wantErr: simulatedError, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ch := gen.Challenge("example") - originalCh := ch.DeepCopy() - newCh, err := addFinalizer( - &fakeChallengeUpdater{ - fakeUpdateOrApply: func(_ context.Context, ch *cmacme.Challenge) (*cmacme.Challenge, error) { - if tt.updateErr != nil { - return nil, tt.updateErr - } - ch = ch.DeepCopy() - // Update the generation to simulate the sort of change - // that the API server will return. - ch.Generation += 1 - return ch, nil - }, - }, - context.TODO(), - ch, - ) - assert.Equal(t, tt.wantErr, err) - assert.Equal(t, originalCh, ch, "the supplied challenge should never be modified") - if err == nil { - assert.EqualValues(t, 1, newCh.Generation-originalCh.Generation, - "if the update succeeds the supplied challenge pointer should be updated with the updated challenge") - } else { - assert.Nil(t, newCh, "if the update fails the returned challenge should be nil") - } - }) - } -} diff --git a/pkg/controller/acmechallenges/sync.go b/pkg/controller/acmechallenges/sync.go index 4ee32dec3..1157097ad 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -23,11 +23,8 @@ import ( acmeapi "golang.org/x/crypto/acme" corev1 "k8s.io/api/core/v1" - apiequality "k8s.io/apimachinery/pkg/api/equality" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" - internalchallenges "github.com/cert-manager/cert-manager/internal/controller/challenges" "github.com/cert-manager/cert-manager/internal/controller/feature" "github.com/cert-manager/cert-manager/pkg/acme" acmecl "github.com/cert-manager/cert-manager/pkg/acme/client" @@ -62,26 +59,22 @@ type solver interface { // Sync will process this ACME Challenge. // It is the core control function for ACME challenges. -func (c *controller) Sync(ctx context.Context, ch *cmacme.Challenge) (err error) { - log := logf.FromContext(ctx).WithValues("dnsName", ch.Spec.DNSName, "type", ch.Spec.Type) +func (c *controller) Sync(ctx context.Context, chOriginal *cmacme.Challenge) (err error) { + log := logf.FromContext(ctx).WithValues("dnsName", chOriginal.Spec.DNSName, "type", chOriginal.Spec.Type) ctx = logf.NewContext(ctx, log) + ch := chOriginal.DeepCopy() - oldChal := ch - ch = ch.DeepCopy() + defer func() { + err = utilerrors.NewAggregate([]error{ + err, + newObjectUpdater(c.cmClient, c.fieldManager).updateObject(ctx, chOriginal, ch), + }) + }() if !ch.DeletionTimestamp.IsZero() { return c.handleFinalizer(ctx, ch) } - defer func() { - if apiequality.Semantic.DeepEqual(oldChal.Status, ch.Status) { - return - } - if _, updateErr := c.updateStatusOrApply(ctx, ch); updateErr != nil { - err = utilerrors.NewAggregate([]error{err, updateErr}) - } - }() - // bail out early on if processing=false, as this challenge has not been // scheduled yet. if !ch.Status.Processing { @@ -92,8 +85,8 @@ func (c *controller) Sync(ctx context.Context, ch *cmacme.Challenge) (err error) // cert-manager has a chance to clean up resources created for the // challenge. if finalizerRequired(ch) { - ch, err = addFinalizer(c, ctx, ch) - return err + ch.Finalizers = append(ch.Finalizers, cmacme.ACMEFinalizer) + return nil } genericIssuer, err := c.helper.GetGenericIssuer(ch.Spec.IssuerRef, ch.Namespace) @@ -260,19 +253,8 @@ func (c *controller) handleFinalizer(ctx context.Context, ch *cmacme.Challenge) } defer func() { - // call UpdateStatus first as we may have updated the challenge.status.reason field - ch, updateErr := c.updateStatusOrApply(ctx, ch) - if updateErr != nil { - err = utilerrors.NewAggregate([]error{err, updateErr}) - return - } // call Update to remove the metadata.finalizers entry ch.Finalizers = ch.Finalizers[1:] - _, updateErr = c.updateOrApply(ctx, ch) - if updateErr != nil { - err = utilerrors.NewAggregate([]error{err, updateErr}) - return - } }() if !ch.Status.Processing { @@ -432,19 +414,3 @@ func (c *controller) solverFor(challengeType cmacme.ACMEChallengeType) (solver, } return nil, fmt.Errorf("no solver for %q implemented", challengeType) } - -func (c *controller) updateOrApply(ctx context.Context, challenge *cmacme.Challenge) (*cmacme.Challenge, error) { - if utilfeature.DefaultFeatureGate.Enabled(feature.ServerSideApply) { - return internalchallenges.Apply(ctx, c.cmClient, c.fieldManager, challenge) - } else { - return c.cmClient.AcmeV1().Challenges(challenge.Namespace).Update(ctx, challenge, metav1.UpdateOptions{}) - } -} - -func (c *controller) updateStatusOrApply(ctx context.Context, challenge *cmacme.Challenge) (*cmacme.Challenge, error) { - if utilfeature.DefaultFeatureGate.Enabled(feature.ServerSideApply) { - return internalchallenges.ApplyStatus(ctx, c.cmClient, c.fieldManager, challenge) - } else { - return c.cmClient.AcmeV1().Challenges(challenge.Namespace).UpdateStatus(ctx, challenge, metav1.UpdateOptions{}) - } -} diff --git a/pkg/controller/acmechallenges/sync_test.go b/pkg/controller/acmechallenges/sync_test.go index 139e3b4d1..6f064b0a0 100644 --- a/pkg/controller/acmechallenges/sync_test.go +++ b/pkg/controller/acmechallenges/sync_test.go @@ -110,14 +110,6 @@ func TestSyncHappyPath(t *testing.T) { testIssuerHTTP01Enabled, }, ExpectedActions: []testpkg.Action{ - testpkg.NewAction(coretesting.NewUpdateSubresourceAction(cmacme.SchemeGroupVersion.WithResource("challenges"), - "status", - gen.DefaultTestNamespace, - gen.ChallengeFrom(deletedChallenge, - gen.SetChallengeProcessing(true), - gen.SetChallengeURL("testurl"), - gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), - ))), testpkg.NewAction(coretesting.NewUpdateAction(cmacme.SchemeGroupVersion.WithResource("challenges"), gen.DefaultTestNamespace, gen.ChallengeFrom(deletedChallenge, diff --git a/pkg/controller/acmechallenges/update.go b/pkg/controller/acmechallenges/update.go new file mode 100644 index 000000000..006f50685 --- /dev/null +++ b/pkg/controller/acmechallenges/update.go @@ -0,0 +1,132 @@ +/* +Copyright 2022 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 acmechallenges + +import ( + "context" + + "github.com/pkg/errors" + apiequality "k8s.io/apimachinery/pkg/api/equality" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + + internalchallenges "github.com/cert-manager/cert-manager/internal/controller/challenges" + "github.com/cert-manager/cert-manager/internal/controller/feature" + cmacme "github.com/cert-manager/cert-manager/pkg/apis/acme/v1" + "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned" + utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" + "github.com/cert-manager/cert-manager/test/unit/gen" +) + +type updater interface { + update(context.Context, *cmacme.Challenge) (*cmacme.Challenge, error) + updateStatus(context.Context, *cmacme.Challenge) (*cmacme.Challenge, error) +} + +type objectUpdater struct { + updater +} + +func newObjectUpdater(cl versioned.Interface, fieldManager string) *objectUpdater { + o := &objectUpdater{ + updater: &objectUpdaterDefault{cl: cl}, + } + if utilfeature.DefaultFeatureGate.Enabled(feature.ServerSideApply) { + o.updater = &objectUpdaterSSA{ + fieldManager: fieldManager, + cl: cl, + } + } + return o +} + +// updateObject updates the Finalizers if they have changed and updates the Status if it has changed. +// Finalizers are updated using the Update method while Status is updated using +// the UpdateStatus method. +// Both updates will be attempted, even if one fails, except in the case where +// one of the updates fails with a Not Found error. +// If the any of the API operations results in a Not Found error, updateObject +// 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 { + 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") + } + + var updateFunctions []func() (*cmacme.Challenge, error) + if !apiequality.Semantic.DeepEqual(old.Status, new.Status) { + updateFunctions = append( + updateFunctions, + func() (*cmacme.Challenge, error) { + obj, err := o.updateStatus(ctx, new) + return obj, errors.Wrap(err, "when updating the status") + }, + ) + } + if !apiequality.Semantic.DeepEqual(old.Finalizers, new.Finalizers) { + updateFunctions = append( + updateFunctions, + func() (*cmacme.Challenge, error) { + obj, err := o.update(ctx, new) + return obj, errors.Wrap(err, "when updating the finalizers") + }, + ) + } + var errors []error + for _, f := range updateFunctions { + if o, err := f(); err != nil { + errors = append(errors, err) + if k8sErrors.IsNotFound(err) { + return nil + } + } else { + new = o + } + } + return utilerrors.NewAggregate(errors) +} + +type objectUpdaterDefault struct { + cl versioned.Interface +} + +func (o *objectUpdaterDefault) 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) { + return o.cl.AcmeV1().Challenges(new.Namespace).UpdateStatus(ctx, new, metav1.UpdateOptions{}) +} + +type objectUpdaterSSA struct { + cl versioned.Interface + fieldManager string +} + +func (o *objectUpdaterSSA) 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) { + 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 new file mode 100644 index 000000000..8311ee271 --- /dev/null +++ b/pkg/controller/acmechallenges/update_test.go @@ -0,0 +1,184 @@ +/* +Copyright 2022 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 acmechallenges + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clienttesting "k8s.io/client-go/testing" + featuretesting "k8s.io/component-base/featuregate/testing" + + "github.com/cert-manager/cert-manager/internal/controller/feature" + "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned/fake" + utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" + "github.com/cert-manager/cert-manager/test/unit/gen" +) + +func TestUpdateObjectStandard(t *testing.T) { + runUpdateObjectTests(t) +} + +func TestUpdateObjectSSA(t *testing.T) { + t.Skip("Server Side Apply cannot be tested because PatchType is not supported by the fake versioned client") + defer featuretesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, feature.ServerSideApply, true)() + runUpdateObjectTests(t) +} + +func runUpdateObjectTests(t *testing.T) { + simulatedUpdateError := errors.New("simulated-update-error") + simulatedUpdateStatusError := errors.New("simulated-update-status-error") + + tests := []struct { + name string + mods []gen.ChallengeModifier + notFound bool + apiResponse clienttesting.ReactionFunc + panicMessage string + errorMessage string + }{ + // Modifying the finalizers and any status fields results in both + // finalizers and status being updated. + { + name: "success", + mods: []gen.ChallengeModifier{ + gen.SetChallengeFinalizers([]string{"example.com/another-finalizer"}), + gen.SetChallengePresented(true), + }, + }, + // If the API server responds with a NOT FOUND error, the error is + // ignored. Presumably the object has been deleted since the Sync + // function began executing. + { + name: "not-found", + mods: []gen.ChallengeModifier{ + gen.SetChallengePresented(true), + }, + 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. + { + name: "panic-on-non-finalizer-non-status-modifications", + mods: []gen.ChallengeModifier{ + gen.SetChallengeDNSName("new-dns-name"), + }, + panicMessage: "only the finalizers and status fields may be modified", + }, + // If the Update API call fails, that error is returned. + { + name: "update-error-only", + 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", + }, + // If the UpdateStatus API call fails, that error is returned. + { + name: "update-status-error-only", + 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", + }, + // If both Update and UpdateStatus API calls fail, both errors are returned. + { + name: "all-updates-fail", + mods: []gen.ChallengeModifier{ + 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]", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.TODO() + old := gen.Challenge("c1") + new := gen.ChallengeFrom(old, tt.mods...) + objects := []runtime.Object{old} + if tt.notFound { + t.Log("Simulating a situation where the target object has been deleted") + objects = nil + } + cl := fake.NewSimpleClientset(objects...) + if tt.apiResponse != nil { + t.Log("Simulating an API server 'update' error") + cl.PrependReactor("update", "*", tt.apiResponse) + } + 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") + updateObjectErr := updater.updateObject(ctx, old, new) + if tt.errorMessage == "" { + assert.NoError(t, updateObjectErr) + } else { + assert.EqualError(t, updateObjectErr, tt.errorMessage) + } + + if len(tt.mods) == 0 { + assert.Empty(t, cl.Actions(), "There should not be any API interactions unless the object was modified") + } + + if !tt.notFound { + t.Log("Checking whether the object was updated") + actual, err := cl.AcmeV1().Challenges(old.Namespace).Get(ctx, old.Name, metav1.GetOptions{}) + require.NoError(t, err) + if updateObjectErr == nil { + assert.Equal(t, new, actual, "updateObject did not return an error so the object in the API should have been updated") + } else { + if !errors.Is(updateObjectErr, simulatedUpdateError) { + assert.Equal(t, new.Finalizers, actual.Finalizers, "The Update did not fail so the Finalizers of the API object should have been updated") + } + if !errors.Is(updateObjectErr, simulatedUpdateStatusError) { + assert.Equal(t, new.Status, actual.Status, "The UpdateStatus did not fail so the Status of the API object should have been updated") + } + } + } + }) + } +} diff --git a/pkg/controller/test/context_builder.go b/pkg/controller/test/context_builder.go index 87e5d8ce1..2ee692ee0 100644 --- a/pkg/controller/test/context_builder.go +++ b/pkg/controller/test/context_builder.go @@ -294,7 +294,7 @@ func (b *Builder) AllActionsExecuted() error { } func actionToString(a coretesting.Action) string { - return fmt.Sprintf("%s %q in namespace %s", a.GetVerb(), a.GetResource(), a.GetNamespace()) + return fmt.Sprintf("%s %s %q in namespace %s", a.GetVerb(), a.GetSubresource(), a.GetResource(), a.GetNamespace()) } // Stop will signal the informers to stop watching changes diff --git a/test/unit/gen/challenge.go b/test/unit/gen/challenge.go index 96290035e..408c51655 100644 --- a/test/unit/gen/challenge.go +++ b/test/unit/gen/challenge.go @@ -120,3 +120,9 @@ func SetChallengeDeletionTimestamp(ts metav1.Time) ChallengeModifier { ch.DeletionTimestamp = &ts } } + +func ResetChallengeStatus() ChallengeModifier { + return func(ch *cmacme.Challenge) { + ch.Status = cmacme.ChallengeStatus{} + } +} From 1ade01f8191c97e90d301679409825ba003a7002 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Sat, 14 May 2022 14:24:13 +0100 Subject: [PATCH 2/2] 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)