weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Refactor the update and updateStatus to a single deferred function

Browse Source 
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
This commit is contained in:
Richard Wall 2022-05-11 16:39:42 +01:00
parent 1481ce24a5
commit 557d14a0cd
10 changed files with 346 additions and 139 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
pkg/controller/acmechallenges/BUILD.bazel
View File

@ -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",
],
)

10
pkg/controller/acmechallenges/controller.go
View File

@ -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")
}

19
pkg/controller/acmechallenges/finalizer.go
View File

@ -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
}

60
pkg/controller/acmechallenges/finalizer_test.go
View File

@ -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")
}
})
}
}

56
pkg/controller/acmechallenges/sync.go
View File

@ -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{})
}
}

8
pkg/controller/acmechallenges/sync_test.go
View File

@ -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,

132
pkg/controller/acmechallenges/update.go Normal file
View File

@ -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)
}

184
pkg/controller/acmechallenges/update_test.go Normal file
View File

@ -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")
}
}
}
})
}
}

2
pkg/controller/test/context_builder.go
View File

@ -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

6
test/unit/gen/challenge.go
View File

@ -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{}
}
}