From dd8f98768b3211b924155ce15ca0088162e16155 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Wed, 28 Nov 2018 21:29:24 +0000 Subject: [PATCH 1/3] CleanUp ACME challenges after issuing and on delete using finalizer Signed-off-by: James Munnelly --- pkg/apis/certmanager/v1alpha1/const.go | 4 ++ pkg/controller/acmechallenges/sync.go | 65 +++++++++++++++++++++----- pkg/controller/acmeorders/sync.go | 33 ++++++++++--- 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/pkg/apis/certmanager/v1alpha1/const.go b/pkg/apis/certmanager/v1alpha1/const.go index ea295c345..d06631767 100644 --- a/pkg/apis/certmanager/v1alpha1/const.go +++ b/pkg/apis/certmanager/v1alpha1/const.go @@ -31,3 +31,7 @@ const ( // Default duration before certificate expiration if Issuer.spec.renewBefore is not set DefaultRenewBefore = time.Hour * 24 * 30 ) + +const ( + ACMEFinalizer = "finalizer.acme.cert-manager.io" +) diff --git a/pkg/controller/acmechallenges/sync.go b/pkg/controller/acmechallenges/sync.go index dae1c4b78..2c5a3ddbb 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -53,8 +53,7 @@ type solver interface { } // Sync will process this ACME Challenge. -// It is the core control function for ACME challenges, and handles: -// - TODO +// It is the core control function for ACME challenges. func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) { oldChal := ch ch = ch.DeepCopy() @@ -67,7 +66,7 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) defer func() { // TODO: replace with more efficient comparison - if reflect.DeepEqual(oldChal.Status, ch.Status) { + if reflect.DeepEqual(oldChal.Status, ch.Status) && len(oldChal.Finalizers) == len(ch.Finalizers) { return } _, updateErr := c.CMClient.CertmanagerV1alpha1().Challenges(ch.Namespace).Update(ch) @@ -76,12 +75,8 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) } }() - // if a challenge is in a final state, we bail out early as there is nothing - // left for us to do here. - if acme.IsFinalState(ch.Status.State) { - // we set processing to false now, as this item has finished being processed. - ch.Status.Processing = false - return nil + if ch.DeletionTimestamp != nil { + return c.handleFinalizer(ctx, ch) } genericIssuer, err := c.helper.GetGenericIssuer(ch.Spec.IssuerRef, ch.Namespace) @@ -89,6 +84,30 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) return fmt.Errorf("error reading (cluster)issuer %q: %v", ch.Spec.IssuerRef.Name, err) } + // if a challenge is in a final state, we bail out early as there is nothing + // left for us to do here. + if acme.IsFinalState(ch.Status.State) { + if ch.Status.Presented { + solver, err := c.solverFor(ch.Spec.Type) + if err != nil { + glog.Errorf("Error getting solver for challenge %q (type %q): %v", ch.Name, ch.Spec.Type, err) + return err + } + + err = solver.CleanUp(ctx, genericIssuer, ch) + if err != nil { + glog.Errorf("Error cleaning up challenge %q on deletion: %v", ch.Name, err) + return err + } + + ch.Status.Presented = false + } + + ch.Status.Processing = false + + return nil + } + cl, err := c.acmeHelper.ClientForIssuer(genericIssuer) if err != nil { return err @@ -153,10 +172,34 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) return err } - glog.Infof("Cleaning up challenge %s/%s", ch.Namespace, ch.Name) + return nil +} + +func (c *Controller) handleFinalizer(ctx context.Context, ch *cmapi.Challenge) error { + genericIssuer, err := c.helper.GetGenericIssuer(ch.Spec.IssuerRef, ch.Namespace) + if err != nil { + return fmt.Errorf("error reading (cluster)issuer %q: %v", ch.Spec.IssuerRef.Name, err) + } + + if len(ch.Finalizers) == 0 { + return nil + } + if ch.Finalizers[0] != cmapi.ACMEFinalizer { + glog.V(4).Infof("Waiting to run challenge %q finalization...", ch.Name) + return nil + } + ch.Finalizers = ch.Finalizers[1:] + + solver, err := c.solverFor(ch.Spec.Type) + if err != nil { + glog.Errorf("Error getting solver for challenge %q (type %q): %v", ch.Name, ch.Spec.Type, err) + return nil + } + err = solver.CleanUp(ctx, genericIssuer, ch) if err != nil { - return err + glog.Errorf("Error cleaning up challenge %q on deletion: %v", ch.Name, err) + return nil } return nil diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 45f14d070..dc42807bd 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -100,6 +100,19 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) { // TODO: we should find a way to periodically update the state of the resource // to reflect the current/actual state in the ACME server. if acme.IsFinalState(o.Status.State) { + existingChallenges, err := c.listChallengesForOrder(o) + if err != nil { + return err + } + + // Cleanup challenge resources once a final state has been reached + for _, ch := range existingChallenges { + err := c.CMClient.CertmanagerV1alpha1().Challenges(ch.Namespace).Delete(ch.Name, nil) + if err != nil { + return err + } + } + return nil } @@ -162,14 +175,8 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) { return fmt.Errorf("unknown order state %q", o.Status.State) } - // create a selector that we can use to find all existing Challenges for the order - sel, err := challengeSelectorForOrder(o) - if err != nil { - return err - } - // get the list of exising challenges for this order - existingChallenges, err := c.challengeLister.Challenges(o.Namespace).List(sel) + existingChallenges, err := c.listChallengesForOrder(o) if err != nil { return err } @@ -252,6 +259,17 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) { return nil } +func (c *Controller) listChallengesForOrder(o *cmapi.Order) ([]*cmapi.Challenge, error) { + // create a selector that we can use to find all existing Challenges for the order + sel, err := challengeSelectorForOrder(o) + if err != nil { + return nil, err + } + + // get the list of exising challenges for this order + return c.challengeLister.Challenges(o.Namespace).List(sel) +} + const ( orderNameLabelKey = "acme.cert-manager.io/order-name" ) @@ -393,6 +411,7 @@ func buildChallenge(i int, o *cmapi.Order, chalSpec cmapi.ChallengeSpec) *cmapi. Namespace: o.Namespace, Labels: challengeLabelsForOrder(o), OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(o, orderGvk)}, + Finalizers: []string{cmapi.ACMEFinalizer}, }, Spec: chalSpec, } From 0656d6cf775550e695e073ef2a20007f41a095fb Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Wed, 28 Nov 2018 22:06:03 +0000 Subject: [PATCH 2/3] Update acmechallenges unit tests Signed-off-by: James Munnelly --- pkg/controller/acmechallenges/sync_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/controller/acmechallenges/sync_test.go b/pkg/controller/acmechallenges/sync_test.go index 78cbbcd01..087df5fc9 100644 --- a/pkg/controller/acmechallenges/sync_test.go +++ b/pkg/controller/acmechallenges/sync_test.go @@ -256,7 +256,11 @@ func TestSyncHappyPath(t *testing.T) { gen.SetChallengeType("http-01"), gen.SetChallengePresented(true), ), - + HTTP01: &fakeSolver{ + fakeCleanUp: func(ctx context.Context, issuer v1alpha1.GenericIssuer, ch *v1alpha1.Challenge) error { + return nil + }, + }, Builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{gen.Challenge("testchal", gen.SetChallengeProcessing(true), @@ -272,7 +276,7 @@ func TestSyncHappyPath(t *testing.T) { gen.SetChallengeURL("testurl"), gen.SetChallengeState(v1alpha1.Valid), gen.SetChallengeType("http-01"), - gen.SetChallengePresented(true), + gen.SetChallengePresented(false), ))), }, }, @@ -286,6 +290,11 @@ func TestSyncHappyPath(t *testing.T) { gen.SetChallengeType("http-01"), gen.SetChallengePresented(true), ), + HTTP01: &fakeSolver{ + fakeCleanUp: func(ctx context.Context, issuer v1alpha1.GenericIssuer, ch *v1alpha1.Challenge) error { + return nil + }, + }, Builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{gen.Challenge("testchal", gen.SetChallengeProcessing(true), @@ -301,7 +310,7 @@ func TestSyncHappyPath(t *testing.T) { gen.SetChallengeURL("testurl"), gen.SetChallengeState(v1alpha1.Invalid), gen.SetChallengeType("http-01"), - gen.SetChallengePresented(true), + gen.SetChallengePresented(false), ))), }, }, From 93a7a89d4e79f9b8694a178d49a1ccc6f00a053f Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Wed, 28 Nov 2018 23:19:52 +0000 Subject: [PATCH 3/3] Ensure finalizer is always removed after one sync Signed-off-by: James Munnelly --- pkg/controller/acmechallenges/sync.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/controller/acmechallenges/sync.go b/pkg/controller/acmechallenges/sync.go index 2c5a3ddbb..15a2f7eca 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -58,12 +58,6 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) oldChal := ch ch = ch.DeepCopy() - // bail out early on if processing=false, as this challenge has not been - // scheduled yet. - if ch.Status.Processing == false { - return nil - } - defer func() { // TODO: replace with more efficient comparison if reflect.DeepEqual(oldChal.Status, ch.Status) && len(oldChal.Finalizers) == len(ch.Finalizers) { @@ -79,6 +73,12 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) return c.handleFinalizer(ctx, ch) } + // bail out early on if processing=false, as this challenge has not been + // scheduled yet. + if ch.Status.Processing == false { + return nil + } + genericIssuer, err := c.helper.GetGenericIssuer(ch.Spec.IssuerRef, ch.Namespace) if err != nil { return fmt.Errorf("error reading (cluster)issuer %q: %v", ch.Spec.IssuerRef.Name, err) @@ -176,11 +176,6 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) (err error) } func (c *Controller) handleFinalizer(ctx context.Context, ch *cmapi.Challenge) error { - genericIssuer, err := c.helper.GetGenericIssuer(ch.Spec.IssuerRef, ch.Namespace) - if err != nil { - return fmt.Errorf("error reading (cluster)issuer %q: %v", ch.Spec.IssuerRef.Name, err) - } - if len(ch.Finalizers) == 0 { return nil } @@ -190,6 +185,15 @@ func (c *Controller) handleFinalizer(ctx context.Context, ch *cmapi.Challenge) e } ch.Finalizers = ch.Finalizers[1:] + if !ch.Status.Processing { + return nil + } + + genericIssuer, err := c.helper.GetGenericIssuer(ch.Spec.IssuerRef, ch.Namespace) + if err != nil { + return fmt.Errorf("error reading (cluster)issuer %q: %v", ch.Spec.IssuerRef.Name, err) + } + solver, err := c.solverFor(ch.Spec.Type) if err != nil { glog.Errorf("Error getting solver for challenge %q (type %q): %v", ch.Name, ch.Spec.Type, err)