Merge pull request #4618 from jetstack-bot/cherry-pick-4616-to-release-1.5

[release-1.5] Don't make extra ACME calls for Order CRs that have failed

pkg/controller/acmeorders/sync.go

@@ -84,6 +84,10 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) {
 	}
 
 	switch {
+	case acme.IsFailureState(o.Status.State):
+		log.V(logf.DebugLevel).Info("Doing nothing as Order is in a failed state")
+		// if the Order is failed there's nothing left for us to do, return nil
+		return nil
 	case o.Status.URL == "":
 		log.V(logf.DebugLevel).Info("Creating new ACME order as status.url is not set")
 		return c.createOrder(ctx, cl, o)
@@ -102,10 +106,6 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) {
 	case anyAuthorizationsMissingMetadata(o):
 		log.V(logf.DebugLevel).Info("Fetching Authorizations from ACME server as status.authorizations contains unpopulated authorizations")
 		return c.fetchMetadataForAuthorizations(ctx, o, cl)
-	case acme.IsFailureState(o.Status.State):
-		log.V(logf.DebugLevel).Info("Doing nothing as Order is in a failed state")
-		// if the Order is failed there's nothing left for us to do, return nil
-		return nil
 	case o.Status.State == cmacme.Valid && o.Status.Certificate == nil:
 		log.V(logf.DebugLevel).Info("Order is in a Valid state but the Certificate data is empty, fetching existing Certificate")
 		return c.fetchCertificateData(ctx, cl, o)

pkg/controller/acmeorders/sync_test.go

@@ -109,10 +109,16 @@ func TestSyncHappyPath(t *testing.T) {
 		},
 	}
 
+	erroredStatus := cmacme.OrderStatus{
+		State: cmacme.Errored,
+	}
+
 	testOrderPending := gen.OrderFrom(testOrder, gen.SetOrderStatus(pendingStatus))
 	testOrderInvalid := testOrderPending.DeepCopy()
 	testOrderInvalid.Status.State = cmacme.Invalid
 	testOrderInvalid.Status.FailureTime = &nowMetaTime
+	testOrderErrored := gen.OrderFrom(testOrder, gen.SetOrderStatus(erroredStatus))
+	testOrderErrored.Status.FailureTime = &nowMetaTime
 	testOrderValid := testOrderPending.DeepCopy()
 	testOrderValid.Status.State = cmacme.Valid
 	// pem encoded word 'test'
@@ -585,7 +591,7 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt
 			},
 			acmeClient: &acmecl.FakeACME{},
 		},
-		"do nothing if the order is failed": {
+		"do nothing if the order is invalid": {
 			order: testOrderInvalid,
 			builder: &testpkg.Builder{
 				CertManagerObjects: []runtime.Object{testIssuerHTTP01TestCom, testOrderInvalid},
@@ -593,6 +599,14 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt
 			},
 			acmeClient: &acmecl.FakeACME{},
 		},
+		"do nothing if the order is in errored state with no url or finalize url on status": {
+			order: testOrderErrored,
+			builder: &testpkg.Builder{
+				CertManagerObjects: []runtime.Object{testIssuerHTTP01TestCom, testOrderErrored},
+				ExpectedActions:    []testpkg.Action{},
+			},
+			acmeClient: &acmecl.FakeACME{},
+		},
 	}
 
 	for name, test := range tests {