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

Addressed code review feedback and simplified the unit-tests

Browse Source 
Signed-off-by: Richard Wall <richard.wall@jetstack.io>
This commit is contained in:
Richard Wall 2022-05-14 14:24:13 +01:00
parent 557d14a0cd
commit 1ade01f819
5 changed files with 136 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

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

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

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

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

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

45
pkg/controller/acmechallenges/update.go
View File

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

84
pkg/controller/acmechallenges/update_test.go
View File

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