diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index f5683f80e..5dbc14d7b 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -152,6 +152,15 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { return err } + if o.Status.State == cmacme.Ready { + log.V(logf.DebugLevel).Info("Finalizing Order as order state is 'Ready'") + return c.finalizeOrder(ctx, cl, o, genericIssuer) + } + + // Note: each of the following code paths uses the ACME Order retrieved + // here. Be mindful when adding new code below this call to ACME server- + // if the new code does not need this ACME order, try to place it above + // this call to avoid extra calls to ACME. acmeOrder, err := getACMEOrder(ctx, cl, o) // Order probably has been deleted, we cannot recover here. if acmeErr, ok := err.(*acmeapi.Error); ok { @@ -167,16 +176,12 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { } switch { - case o.Status.State == cmacme.Ready: - log.V(logf.DebugLevel).Info("Finalizing Order as order state is 'Ready'") - return c.finalizeOrder(ctx, cl, o, genericIssuer) - case anyChallengesFailed(challenges): // TODO (@munnerz): instead of waiting for the ACME server to mark this // Order as failed, we could just mark the Order as failed as there is // no way that we will attempt and continue the order anyway. log.V(logf.DebugLevel).Info("Update Order status as at least one Challenge has failed") - _, err := c.updateOrderStatus(ctx, cl, o) + _, err := c.updateOrderStatusFromACMEOrder(ctx, cl, o, acmeOrder) if acmeErr, ok := err.(*acmeapi.Error); ok { if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { log.Error(err, "failed to update Order status due to a 4xx error, marking Order as failed") @@ -216,7 +221,7 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { case !anyChallengesFailed(challenges) && allChallengesFinal(challenges): log.V(logf.DebugLevel).Info("All challenges are in a final state, updating order state") - _, err := c.updateOrderStatus(ctx, cl, o) + _, err := c.updateOrderStatusFromACMEOrder(ctx, cl, o, acmeOrder) if acmeErr, ok := err.(*acmeapi.Error); ok { if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { log.Error(err, "failed to update Order status due to a 4xx error, marking Order as failed") @@ -285,6 +290,11 @@ func (c *controller) updateOrderStatus(ctx context.Context, cl acmecl.Interface, if err != nil { return nil, err } + + return c.updateOrderStatusFromACMEOrder(ctx, cl, o, acmeOrder) +} + +func (c *controller) updateOrderStatusFromACMEOrder(ctx context.Context, cl acmecl.Interface, o *cmacme.Order, acmeOrder *acmeapi.Order) (*acmeapi.Order, error) { // Workaround bug in golang.org/x/crypto/acme implementation whereby the // order's URI field will be empty when calling GetOrder due to the // 'Location' header not being set on the response from the ACME server. @@ -514,11 +524,11 @@ func (c *controller) finalizeOrder(ctx context.Context, cl acmecl.Interface, o * // if it is already in the 'valid' state, as upon retry we will // then retrieve the Certificate resource. _, errUpdate := c.updateOrderStatus(ctx, cl, o) - if acmeErr, ok := err.(*acmeapi.Error); ok { + if acmeErr, ok := errUpdate.(*acmeapi.Error); ok { if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { log.Error(err, "failed to update Order status due to a 4xx error, marking Order as failed") c.setOrderState(&o.Status, string(cmacme.Errored)) - o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) + o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", errUpdate) return nil } } diff --git a/pkg/controller/acmeorders/sync_test.go b/pkg/controller/acmeorders/sync_test.go index ada298b9e..f13a325ee 100644 --- a/pkg/controller/acmeorders/sync_test.go +++ b/pkg/controller/acmeorders/sync_test.go @@ -39,7 +39,7 @@ import ( "github.com/jetstack/cert-manager/test/unit/gen" ) -func TestSyncHappyPath(t *testing.T) { +func TestSync(t *testing.T) { nowTime := time.Now() nowMetaTime := metav1.NewTime(nowTime) fixedClock := fakeclock.NewFakeClock(nowTime) @@ -113,12 +113,40 @@ func TestSyncHappyPath(t *testing.T) { State: cmacme.Errored, } + erroredStatusWithDetail := cmacme.OrderStatus{ + State: cmacme.Errored, + FailureTime: &nowMetaTime, + URL: "http://testurl.com/abcde", + FinalizeURL: "http://testurl.com/abcde/finalize", + Reason: "Failed to finalize Order: 429 : some error", + Authorizations: []cmacme.ACMEAuthorization{ + { + URL: "http://authzurl", + Identifier: "test.com", + InitialState: cmacme.Valid, + Challenges: []cmacme.ACMEChallenge{ + { + URL: "http://chalurl", + Token: "token", + Type: "http-01", + }, + }, + }, + }, + } + + acmeError429 := acmeapi.Error{ + StatusCode: 429, + Detail: "some error", + } + 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 + testOrderErroredWithDetail := gen.OrderFrom(testOrderPending, gen.SetOrderStatus(erroredStatusWithDetail)) testOrderValid := testOrderPending.DeepCopy() testOrderValid.Status.State = cmacme.Valid // pem encoded word 'test' @@ -514,6 +542,53 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt }, }, }, + "call FinalizeOrder and update the order state to 'errored' if finalize fails with a 4xx ACME error": { + order: gen.OrderFrom(testOrderErroredWithDetail, gen.SetOrderState(cmacme.Ready)), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{testIssuerHTTP01TestCom, gen.OrderFrom(testOrderErroredWithDetail, gen.SetOrderState(cmacme.Ready))}, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction(cmacme.SchemeGroupVersion.WithResource("orders"), + "status", + testOrderErroredWithDetail.Namespace, testOrderErroredWithDetail)), + }, + }, + acmeClient: &acmecl.FakeACME{ + FakeGetOrder: func(_ context.Context, url string) (*acmeapi.Order, error) { + return testACMEOrderReady, nil + }, + FakeCreateOrderCert: func(_ context.Context, url string, csr []byte, bundle bool) ([][]byte, string, error) { + return nil, "", &acmeError429 + }, + FakeHTTP01ChallengeResponse: func(s string) (string, error) { + // TODO: assert s = "token" + return "key", nil + }, + }, + }, + "call FinalizeOrder, return error if finalize fails with an unspecified error": { + order: gen.OrderFrom(testOrderErroredWithDetail, gen.SetOrderState(cmacme.Ready)), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{testIssuerHTTP01TestCom, gen.OrderFrom(testOrderErroredWithDetail, gen.SetOrderState(cmacme.Ready))}, + ExpectedActions: []testpkg.Action{ + // testpkg.NewAction(coretesting.NewUpdateSubresourceAction(cmacme.SchemeGroupVersion.WithResource("orders"), + // "status", + // testOrderErroredWithDetail.Namespace, testOrderErroredWithDetail)), + }, + }, + acmeClient: &acmecl.FakeACME{ + FakeGetOrder: func(_ context.Context, url string) (*acmeapi.Order, error) { + return testACMEOrderReady, nil + }, + FakeCreateOrderCert: func(_ context.Context, url string, csr []byte, bundle bool) ([][]byte, string, error) { + return nil, "", errors.New("some error") + }, + FakeHTTP01ChallengeResponse: func(s string) (string, error) { + // TODO: assert s = "token" + return "key", nil + }, + }, + expectErr: true, + }, "call FinalizeOrder fetch alternate cert chain": { order: testOrderReady.DeepCopy(), builder: &testpkg.Builder{