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..15a2f7eca 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -53,21 +53,14 @@ 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() - // 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) { + if reflect.DeepEqual(oldChal.Status, ch.Status) && len(oldChal.Finalizers) == len(ch.Finalizers) { return } _, updateErr := c.CMClient.CertmanagerV1alpha1().Challenges(ch.Namespace).Update(ch) @@ -76,11 +69,13 @@ 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 + if ch.DeletionTimestamp != nil { + 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 } @@ -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,38 @@ 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 { + 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:] + + 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) + 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/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), ))), }, }, diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 5bfe2b0ab..85b7be98e 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -104,6 +104,19 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) { // TODO: if the certificate bytes are nil, we should attempt to retrieve // the certificate for the order using GetCertificate 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 } @@ -187,14 +200,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 } @@ -280,6 +287,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" ) @@ -421,6 +439,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, }