Merge pull request #1116 from munnerz/cleanup-challenges

CleanUp ACME challenges after issuing and on delete using finalizer

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"
+)

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:
+// It is the core control function for ACME challenges.
-// - TODO
 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
+	if ch.DeletionTimestamp != nil {
-	// left for us to do here.
+		return c.handleFinalizer(ctx, ch)
-	if acme.IsFinalState(ch.Status.State) {
+	}
-		// we set processing to false now, as this item has finished being processed.
+
-		ch.Status.Processing = false
+	// 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

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

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