diff --git a/pkg/acme/BUILD.bazel b/pkg/acme/BUILD.bazel index 1d8b77397..7d5c6f829 100644 --- a/pkg/acme/BUILD.bazel +++ b/pkg/acme/BUILD.bazel @@ -12,6 +12,7 @@ go_library( deps = [ "//pkg/acme/client:go_default_library", "//pkg/acme/client/middleware:go_default_library", + "//pkg/api/util:go_default_library", "//pkg/apis/acme/v1alpha2:go_default_library", "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/apis/meta/v1:go_default_library", diff --git a/pkg/acme/helper.go b/pkg/acme/helper.go index 965b53aaf..a9faeef29 100644 --- a/pkg/acme/helper.go +++ b/pkg/acme/helper.go @@ -22,6 +22,7 @@ import ( corelisters "k8s.io/client-go/listers/core/v1" acme "github.com/jetstack/cert-manager/pkg/acme/client" + apiutil "github.com/jetstack/cert-manager/pkg/api/util" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" cmerrors "github.com/jetstack/cert-manager/pkg/util/errors" @@ -95,6 +96,12 @@ func (h *helperImpl) ClientForIssuer(iss cmapi.GenericIssuer) (acme.Interface, e if acmeSpec == nil { return nil, fmt.Errorf("issuer %q is not an ACME issuer. Ensure the 'acme' stanza is correctly specified on your Issuer resource", iss.GetObjectMeta().Name) } + if !apiutil.IssuerHasCondition(iss, cmapi.IssuerCondition{ + Type: cmapi.IssuerConditionReady, + Status: cmmeta.ConditionTrue, + }) { + return nil, fmt.Errorf("issuer %q is not in a 'Ready' state. not constructing client until issuer is ready", iss.GetObjectMeta().Name) + } ns := iss.GetObjectMeta().Namespace if ns == "" { diff --git a/pkg/controller/acmechallenges/sync.go b/pkg/controller/acmechallenges/sync.go index 9add076ff..ba8f49a92 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -229,6 +229,11 @@ func handleError(ch *cmacme.Challenge, err error) error { // absorb the error as updating the challenge's status will trigger a sync return nil } + if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { + ch.Status.State = cmacme.Errored + ch.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) + return nil + } return err } diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 4bb0b70a1..b566c8a6b 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -79,6 +79,14 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { case o.Status.FinalizeURL == "": log.Info("Updating Order status as status.finalizeURL is not set") _, err := c.updateOrderStatus(ctx, cl, o) + 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") + c.setOrderState(&o.Status, string(cmacme.Errored)) + o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) + return nil + } + } return err case anyAuthorizationsMissingMetadata(o): log.Info("Fetching Authorizations from ACME server as status.authorizations contains unpopulated authorizations") @@ -143,12 +151,28 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { // no way that we will attempt and continue the order anyway. log.Info("Update Order status as at least one Challenge has failed") _, err := c.updateOrderStatus(ctx, cl, o) + 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") + c.setOrderState(&o.Status, string(cmacme.Errored)) + o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) + return nil + } + } return err // anyChallengesFailed(challenges) == false is already implied by the above // case, but explicitly check it here in case anything changes in future. case !anyChallengesFailed(challenges) && allChallengesFinal(challenges): log.Info("All challenges are in a final state, updating order state") _, err := c.updateOrderStatus(ctx, cl, o) + 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") + c.setOrderState(&o.Status, string(cmacme.Errored)) + o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) + return nil + } + } return err } @@ -176,8 +200,15 @@ func (c *controller) createOrder(ctx context.Context, cl acmecl.Interface, o *cm orderTemplate := acmeapi.NewOrder(identifierSet.List()...) dbg.Info("constructed order template", "template", orderTemplate) acmeOrder, err := cl.CreateOrder(ctx, orderTemplate) + if acmeErr, ok := err.(*acmeapi.Error); ok { + if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { + log.Error(err, "failed to create Order resource due to bad request, marking Order as failed") + c.setOrderState(&o.Status, string(cmacme.Errored)) + o.Status.Reason = fmt.Sprintf("Failed to create Order: %v", err) + return nil + } + } if err != nil { - // TODO: handle transient vs permanent failures here return fmt.Errorf("error creating new order: %v", err) } log.Info("submitted Order to ACME server") @@ -198,16 +229,7 @@ func (c *controller) updateOrderStatus(ctx context.Context, cl acmecl.Interface, log.Info("Fetching Order metadata from ACME server") acmeOrder, err := cl.GetOrder(ctx, o.Status.URL) - 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") - c.setOrderState(&o.Status, string(cmacme.Errored)) - o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) - return nil, nil - } - } if err != nil { - // TODO: handle different ACME error types return nil, err } @@ -416,15 +438,33 @@ func (c *controller) finalizeOrder(ctx context.Context, cl acmecl.Interface, o * } certSlice, err := cl.FinalizeOrder(ctx, o.Status.FinalizeURL, derBytes) - // always update the order status after calling Finalize - this allows - // us to record the current orders status on this order resource - // despite it not being returned directly by the acme client. + // if an ACME error is returned and it's a 4xx error, mark this Order as + // failed and do not retry it until after applying the global backoff. + if acmeErr, ok := err.(*acmeapi.Error); ok { + if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { + log.Error(err, "failed to finalize Order resource due to bad request, marking Order as failed") + c.setOrderState(&o.Status, string(cmacme.Errored)) + o.Status.Reason = fmt.Sprintf("Failed to finalize Order: %v", err) + return nil + } + } + // even if any other kind of error occurred, we always update the order + // status after calling Finalize - this allows us to record the current + // order's status on this order resource despite it not being returned + // directly by the acme client. // This will catch cases where the Order cannot be finalized because it // 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.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) + return nil + } + } if errUpdate != nil { - // TODO: mark permanent failure? return fmt.Errorf("error syncing order status: %v", errUpdate) } // check for errors from FinalizeOrder @@ -458,6 +498,14 @@ func (c *controller) storeCertificateOnStatus(ctx context.Context, o *cmacme.Ord func (c *controller) fetchCertificateData(ctx context.Context, cl acmecl.Interface, o *cmacme.Order) error { log := logf.FromContext(ctx) acmeOrder, err := c.updateOrderStatus(ctx, cl, o) + 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") + c.setOrderState(&o.Status, string(cmacme.Errored)) + o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err) + return nil + } + } if err != nil { return err }