From 374db0b458bc0a3d388573b88b8301ec3dbf865f Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 21:30:16 +0100 Subject: [PATCH 01/10] Refactor ACME issuer cleanup orders code Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 100 +++++++++++++++++++++++++-------------- 1 file changed, 65 insertions(+), 35 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index d94dc0869..f5c14d47f 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -27,10 +27,12 @@ import ( "time" "github.com/golang/glog" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "github.com/jetstack/cert-manager/pkg/acme" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" @@ -49,50 +51,34 @@ var ( ) func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.IssueResponse, error) { - // initially, we do not set the csr on the order resource we build. - // this is to save having the overhead of generating a new CSR in the case - // where the Order resource is up to date already + // Initially, we do not set the csr on the order resource we build. + // This is to save having the overhead of generating a new CSR in the case + // where the Order resource is up to date already, and also because we have + // not actually read the existing certificate private key yet to ensure it + // exists. expectedOrder, err := buildOrder(crt, nil) if err != nil { return issuer.IssueResponse{}, err } - // attempt to retrieve the existing order resource for this Certificate - var existingOrder *v1alpha1.Order - - glog.V(4).Infof("Attempting to retrieve existing orders for %q/%q", crt.Namespace, crt.Name) - labelMap := certLabels(crt.Name) - selector := labels.NewSelector() - for k, v := range labelMap { - req, err := labels.NewRequirement(k, selection.Equals, []string{v}) - if err != nil { - return issuer.IssueResponse{}, err - } - selector.Add(*req) - } - existingOrders, err := a.orderLister.Orders(crt.Namespace).List(selector) + // Cleanup Order resources that are owned by this Certificate but are not + // up to date (i.e. do not match the requirements on the Certificate). + // Because the order name returned by buildOrder is a hash of its spec, we + // can simply delete all order resources that are owned by us that do not + // have the same name. + err = a.cleanupOwnedOrders(crt, expectedOrder.Name) if err != nil { + glog.Errorf("Error cleaning up old orders: %v", err) return issuer.IssueResponse{}, err } - for _, o := range existingOrders { - // Don't touch any objects that don't have this certificate set as the - // owner reference. - if !metav1.IsControlledBy(o, crt) { - continue - } - if o.Name == expectedOrder.Name { - existingOrder = o - glog.V(4).Infof("Found existing order %q for certificate %s/%s", existingOrder.Name, crt.Namespace, crt.Name) - continue - } - - // delete any old order resources - glog.Infof("Deleting Order resource %s/%s", o.Namespace, o.Name) - err := a.CMClient.CertmanagerV1alpha1().Orders(o.Namespace).Delete(o.Name, nil) - if err != nil { - return issuer.IssueResponse{}, err - } + // Obtain the existing Order for this Certificate from the API server. + // If it does not exist, we continue on as it will be created from + // the generated expectedOrder. + existingOrder, err := a.orderLister.Orders(expectedOrder.Namespace).Get(expectedOrder.Name) + if err != nil && !apierrors.IsNotFound(err) { + glog.Errorf("Error getting existing Order resource: %v", err) + return issuer.IssueResponse{}, err } // get existing certificate private key @@ -273,6 +259,50 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss }, nil } +func (a *Acme) cleanupOwnedOrders(crt *v1alpha1.Certificate, retain string) error { + labelMap := certLabels(crt.Name) + selector := labels.NewSelector() + for k, v := range labelMap { + req, err := labels.NewRequirement(k, selection.Equals, []string{v}) + if err != nil { + return err + } + selector.Add(*req) + } + existingOrders, err := a.orderLister.Orders(crt.Namespace).List(selector) + if err != nil { + return err + } + + var errs []error + for _, o := range existingOrders { + // Don't touch any objects that don't have this certificate set as the + // owner reference. + if !metav1.IsControlledBy(o, crt) { + continue + } + + if o.Name == retain { + glog.V(4).Infof("Skipping cleanup for active order resource %q", retain) + continue + } + + // delete any old order resources + glog.Infof("Deleting Order resource %s/%s", o.Namespace, o.Name) + a.Recorder.Eventf(crt, corev1.EventTypeNormal, "Cleanup", + fmt.Sprintf("Deleting old Order resource %q", o.Name)) + + err := a.CMClient.CertmanagerV1alpha1().Orders(o.Namespace).Delete(o.Name, nil) + if err != nil && !apierrors.IsNotFound(err) { + glog.Errorf("Error deleting Order resource %s/%s: %v", o.Namespace, o.Name, err) + errs = append(errs, err) + continue + } + } + + return utilerrors.NewAggregate(errs) +} + // retryOrder will delete the existing order with the foreground // deletion policy. // If delete successfully (i.e. cleaned up), the order name will be From 536b6fd76ffab4531f5bf33b0881882c18260262 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 21:31:47 +0100 Subject: [PATCH 02/10] Refactor ACME issuer generate private key code Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 77 +++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 29 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index f5c14d47f..05409a427 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -19,6 +19,7 @@ package acme import ( "bytes" "context" + "crypto" "crypto/x509" "encoding/json" "encoding/pem" @@ -81,38 +82,25 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return issuer.IssueResponse{}, err } - // get existing certificate private key - glog.V(4).Infof("Attempting to fetch existing certificate private key") - key, err := kube.SecretTLSKey(a.secretsLister, crt.Namespace, crt.Spec.SecretName) + key, generated, err := a.getCertificatePrivateKey(crt) if err != nil { - // if the private key is not found, or is formatted incorrectly, we - // will generate a new one and create/overwrite the secret. - if !apierrors.IsNotFound(err) && !errors.IsInvalidData(err) { - return issuer.IssueResponse{}, err - } - - // TODO: perhaps we shouldn't overwrite the secret if it is invalid, - // and instead bail out here? - - glog.V(4).Infof("Generating new private key for %s/%s", crt.Namespace, crt.Name) - // generate a new private key. - key, err := pki.GenerateRSAPrivateKey(2048) - if err != nil { - return issuer.IssueResponse{}, err - } - - keyBytes, err := pki.EncodePrivateKey(key) - if err != nil { - return issuer.IssueResponse{}, err - } + glog.Errorf("Error getting certificate private key: %v", err) + return issuer.IssueResponse{}, err + } + if generated { + // If we have generated a new private key, we return here to ensure we + // successfully persist the key before creating any CSRs with it. glog.V(4).Infof("Storing new certificate private key for %s/%s", crt.Namespace, crt.Name) - // We return the private key here early, and trigger an immediate requeue. - // This is because we have just generated a new one, and to keep our - // later logic simple we store it immediately. - // TODO: remove the 'requeue: true' as it could cause a race condition - // where our lister has not observed the new private key generated above - return issuer.IssueResponse{PrivateKey: keyBytes}, nil + + keyPem, err := pki.EncodePrivateKey(key) + if err != nil { + return issuer.IssueResponse{}, err + } + + return issuer.IssueResponse{ + PrivateKey: keyPem, + }, nil } // if there is an existing order, we check to make sure it is up to date @@ -303,6 +291,37 @@ func (a *Acme) cleanupOwnedOrders(crt *v1alpha1.Certificate, retain string) erro return utilerrors.NewAggregate(errs) } +func (a *Acme) getCertificatePrivateKey(crt *v1alpha1.Certificate) (crypto.Signer, bool, error) { + glog.V(4).Infof("Attempting to fetch existing certificate private key") + + // If a private key already exists, reuse it. + // TODO: if we have not observed the update to the Secret resource with the + // private key yet, we may in some cases loop and re-generate the private key + // over and over. We could attempt to use the live clientset to read the + // private key too to avoid this case. + key, err := kube.SecretTLSKey(a.secretsLister, crt.Namespace, crt.Spec.SecretName) + if err == nil { + return key, false, nil + } + + // We only generate a new private key if the existing one is not found or + // contains invalid data. + // TODO: should we re-generate on InvalidData? + if !apierrors.IsNotFound(err) && !errors.IsInvalidData(err) { + return nil, false, err + } + + glog.V(4).Infof("Generating new private key for %s/%s", crt.Namespace, crt.Name) + + // generate a new private key. + rsaKey, err := pki.GenerateRSAPrivateKey(2048) + if err != nil { + return nil, false, err + } + + return rsaKey, true, nil +} + // retryOrder will delete the existing order with the foreground // deletion policy. // If delete successfully (i.e. cleaned up), the order name will be From e4399e87c5d69ab34e016ac7314a6fd67ef198d8 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 21:34:56 +0100 Subject: [PATCH 03/10] Move private key generation to start of Issue Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 41 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 05409a427..907d127de 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -52,6 +52,26 @@ var ( ) func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.IssueResponse, error) { + key, generated, err := a.getCertificatePrivateKey(crt) + if err != nil { + glog.Errorf("Error getting certificate private key: %v", err) + return issuer.IssueResponse{}, err + } + if generated { + // If we have generated a new private key, we return here to ensure we + // successfully persist the key before creating any CSRs with it. + glog.V(4).Infof("Storing new certificate private key for %s/%s", crt.Namespace, crt.Name) + + keyPem, err := pki.EncodePrivateKey(key) + if err != nil { + return issuer.IssueResponse{}, err + } + + return issuer.IssueResponse{ + PrivateKey: keyPem, + }, nil + } + // Initially, we do not set the csr on the order resource we build. // This is to save having the overhead of generating a new CSR in the case // where the Order resource is up to date already, and also because we have @@ -82,27 +102,6 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return issuer.IssueResponse{}, err } - key, generated, err := a.getCertificatePrivateKey(crt) - if err != nil { - glog.Errorf("Error getting certificate private key: %v", err) - return issuer.IssueResponse{}, err - } - if generated { - // If we have generated a new private key, we return here to ensure we - // successfully persist the key before creating any CSRs with it. - - glog.V(4).Infof("Storing new certificate private key for %s/%s", crt.Namespace, crt.Name) - - keyPem, err := pki.EncodePrivateKey(key) - if err != nil { - return issuer.IssueResponse{}, err - } - - return issuer.IssueResponse{ - PrivateKey: keyPem, - }, nil - } - // if there is an existing order, we check to make sure it is up to date // with the current certificate & issuer configuration. // if it is not, we will abandon the old order and create a new one. From f2551d383258ba1a835e6678d90f203ec34cb1a6 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 21:37:31 +0100 Subject: [PATCH 04/10] Reorder checking for existingOrder Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 133 ++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 66 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 907d127de..49d1c34fd 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -101,6 +101,9 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss glog.Errorf("Error getting existing Order resource: %v", err) return issuer.IssueResponse{}, err } + if existingOrder == nil { + return issuer.IssueResponse{}, a.createNewOrder(crt, expectedOrder, key) + } // if there is an existing order, we check to make sure it is up to date // with the current certificate & issuer configuration. @@ -109,82 +112,52 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss // well as the back-off applied to failing ACME Orders. // They should therefore *only* match on changes to the actual Certificate // resource, or underlying Order (i.e. user interaction). - if existingOrder != nil { - glog.V(4).Infof("Validating existing order CSR for Certificate %s/%s", crt.Namespace, crt.Name) + glog.V(4).Infof("Validating existing order CSR for Certificate %s/%s", crt.Namespace, crt.Name) - // if the existing order has expired, we should create a new one - // TODO: implement this order state in the acmeorders controller - if existingOrder.Status.State == v1alpha1.Expired { - return a.retryOrder(existingOrder) - } - - // check if the existing order has failed. - // If it has, we check to see when it last failed and 'back-off' if it - // failed in the recent past. - if acme.IsFailureState(existingOrder.Status.State) { - if crt.Status.LastFailureTime == nil { - nowTime := metav1.NewTime(a.clock.Now()) - crt.Status.LastFailureTime = &nowTime - } - - if time.Now().Sub(crt.Status.LastFailureTime.Time) < createOrderWaitDuration { - return issuer.IssueResponse{}, fmt.Errorf("applying acme order back-off for certificate %s/%s because it has failed within the last %s", crt.Namespace, crt.Name, createOrderWaitDuration) - } - - // otherwise, we clear the lastFailureTime and create a new order - // as the back-off time has passed. - crt.Status.LastFailureTime = nil - - return a.retryOrder(existingOrder) - } - - // check the CSR is created by the private key that we hold - csrBytes := existingOrder.Spec.CSR - if len(csrBytes) == 0 { - return a.retryOrder(existingOrder) - } - existingCSR, err := x509.ParseCertificateRequest(csrBytes) - if err != nil { - return a.retryOrder(existingOrder) - } - - matches, err := pki.PublicKeyMatchesCSR(key.Public(), existingCSR) - if err != nil { - return issuer.IssueResponse{}, err - } - - if !matches { - glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key. Creating new order.", crt.Namespace, crt.Name) - return a.retryOrder(existingOrder) - } + // if the existing order has expired, we should create a new one + // TODO: implement this order state in the acmeorders controller + if existingOrder.Status.State == v1alpha1.Expired { + return a.retryOrder(existingOrder) } - if existingOrder == nil { - glog.V(4).Infof("Creating new Order resource for Certificate %s/%s", crt.Namespace, crt.Name) - - csr, err := pki.GenerateCSR(a.issuer, crt) - if err != nil { - // TODO: what errors can be produced here? some error types might - // be permenant, and we should handle that properly. - return issuer.IssueResponse{}, err + // check if the existing order has failed. + // If it has, we check to see when it last failed and 'back-off' if it + // failed in the recent past. + if acme.IsFailureState(existingOrder.Status.State) { + if crt.Status.LastFailureTime == nil { + nowTime := metav1.NewTime(a.clock.Now()) + crt.Status.LastFailureTime = &nowTime } - csrBytes, err := pki.EncodeCSR(csr, key) - if err != nil { - return issuer.IssueResponse{}, err + if time.Now().Sub(crt.Status.LastFailureTime.Time) < createOrderWaitDuration { + return issuer.IssueResponse{}, fmt.Errorf("applying acme order back-off for certificate %s/%s because it has failed within the last %s", crt.Namespace, crt.Name, createOrderWaitDuration) } - // set the CSR field on the order to be created - expectedOrder.Spec.CSR = csrBytes + // otherwise, we clear the lastFailureTime and create a new order + // as the back-off time has passed. + crt.Status.LastFailureTime = nil - existingOrder, err = a.CMClient.CertmanagerV1alpha1().Orders(crt.Namespace).Create(expectedOrder) - if err != nil { - return issuer.IssueResponse{}, err - } + return a.retryOrder(existingOrder) + } - glog.V(4).Infof("Created new Order resource named %q for Certificate %s/%s", expectedOrder.Name, crt.Namespace, crt.Name) + // check the CSR is created by the private key that we hold + csrBytes := existingOrder.Spec.CSR + if len(csrBytes) == 0 { + return a.retryOrder(existingOrder) + } + existingCSR, err := x509.ParseCertificateRequest(csrBytes) + if err != nil { + return a.retryOrder(existingOrder) + } - return issuer.IssueResponse{}, nil + matches, err := pki.PublicKeyMatchesCSR(key.Public(), existingCSR) + if err != nil { + return issuer.IssueResponse{}, err + } + + if !matches { + glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key. Creating new order.", crt.Namespace, crt.Name) + return a.retryOrder(existingOrder) } if existingOrder.Status.State != v1alpha1.Valid { @@ -321,6 +294,34 @@ func (a *Acme) getCertificatePrivateKey(crt *v1alpha1.Certificate) (crypto.Signe return rsaKey, true, nil } +func (a *Acme) createNewOrder(crt *v1alpha1.Certificate, template *v1alpha1.Order, key crypto.Signer) error { + glog.V(4).Infof("Creating new Order resource for Certificate %s/%s", crt.Namespace, crt.Name) + + csr, err := pki.GenerateCSR(a.issuer, crt) + if err != nil { + // TODO: what errors can be produced here? some error types might + // be permenant, and we should handle that properly. + return err + } + + csrBytes, err := pki.EncodeCSR(csr, key) + if err != nil { + return err + } + + // set the CSR field on the order to be created + template.Spec.CSR = csrBytes + + _, err = a.CMClient.CertmanagerV1alpha1().Orders(template.Namespace).Create(template) + if err != nil { + return err + } + + glog.V(4).Infof("Created new Order resource named %q for Certificate %s/%s", template.Name, crt.Namespace, crt.Name) + + return nil +} + // retryOrder will delete the existing order with the foreground // deletion policy. // If delete successfully (i.e. cleaned up), the order name will be From f553f8e8a43d7ed9e88b688eeb62645377bea25b Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 22:29:39 +0100 Subject: [PATCH 05/10] Move existing order checking into own function Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 54 +++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 49d1c34fd..5ebe78d36 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -114,6 +114,15 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss // resource, or underlying Order (i.e. user interaction). glog.V(4).Infof("Validating existing order CSR for Certificate %s/%s", crt.Namespace, crt.Name) + validForKey, err := existingOrderIsValidForKey(existingOrder, key) + if err != nil { + return issuer.IssueResponse{}, err + } + if !validForKey { + glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key. Creating new order.", crt.Namespace, crt.Name) + + } + // if the existing order has expired, we should create a new one // TODO: implement this order state in the acmeorders controller if existingOrder.Status.State == v1alpha1.Expired { @@ -140,26 +149,6 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return a.retryOrder(existingOrder) } - // check the CSR is created by the private key that we hold - csrBytes := existingOrder.Spec.CSR - if len(csrBytes) == 0 { - return a.retryOrder(existingOrder) - } - existingCSR, err := x509.ParseCertificateRequest(csrBytes) - if err != nil { - return a.retryOrder(existingOrder) - } - - matches, err := pki.PublicKeyMatchesCSR(key.Public(), existingCSR) - if err != nil { - return issuer.IssueResponse{}, err - } - - if !matches { - glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key. Creating new order.", crt.Namespace, crt.Name) - return a.retryOrder(existingOrder) - } - if existingOrder.Status.State != v1alpha1.Valid { glog.Infof("Order %s/%s is not in 'valid' state. Waiting for Order to transition before attempting to issue Certificate.", existingOrder.Namespace, existingOrder.Name) @@ -342,6 +331,31 @@ func (a *Acme) retryOrder(existingOrder *v1alpha1.Order) (issuer.IssueResponse, return issuer.IssueResponse{}, nil } +func existingOrderIsValidForKey(o *v1alpha1.Order, key crypto.Signer) (bool, error) { + // check the CSR is created by the private key that we hold + csrBytes := o.Spec.CSR + if len(csrBytes) == 0 { + // Handles a weird case where an Order exists *without* a CSR set + return false, nil + } + existingCSR, err := x509.ParseCertificateRequest(csrBytes) + if err != nil { + // Absorb invalid CSR datas as 'not valid' + return false, nil + } + + matches, err := pki.PublicKeyMatchesCSR(key.Public(), existingCSR) + if err != nil { + // If this returns an error, something bad happened parsing somewhere + return false, err + } + if !matches { + return false, nil + } + + return true, nil +} + func buildOrder(crt *v1alpha1.Certificate, csr []byte) (*v1alpha1.Order, error) { spec := v1alpha1.OrderSpec{ CSR: csr, From 87a479e6cb0c8a734c280d62d474548bb26ebe26 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 22:44:14 +0100 Subject: [PATCH 06/10] Add extra comments in ACME Issuer function Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 5ebe78d36..46931a85a 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -120,18 +120,20 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss } if !validForKey { glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key. Creating new order.", crt.Namespace, crt.Name) - } - // if the existing order has expired, we should create a new one - // TODO: implement this order state in the acmeorders controller + // If the existing order has expired, we should create a new one + // TODO: implement setting this order state in the acmeorders controller if existingOrder.Status.State == v1alpha1.Expired { return a.retryOrder(existingOrder) } - // check if the existing order has failed. - // If it has, we check to see when it last failed and 'back-off' if it - // failed in the recent past. + // If the existing order has failed, we should check if the Certificate + // already has a LastFailureTime + // - If it does not, then this is a new failure and we record the LastFailureTime + // as Now() and return + // - If it does, and it is more than the 'back-off' period ago, we retry the order + // - Otherwise we return an error to attempt re-processing at a later time if acme.IsFailureState(existingOrder.Status.State) { if crt.Status.LastFailureTime == nil { nowTime := metav1.NewTime(a.clock.Now()) @@ -164,6 +166,8 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return issuer.IssueResponse{}, err } + // We check the current Order's Certificate resource to see if it's nearing expiry. + // If it is, this implies that it is an old order that is now out of date. certSlice, err := cl.GetCertificate(ctx, existingOrder.Status.CertificateURL) if err != nil { // TODO: parse returned ACME error and potentially re-create order. From 403a746bfa06acb9abedf66d01b69c4a5d78ba05 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 22:46:06 +0100 Subject: [PATCH 07/10] Always reset LastFailureTime in retryOrder Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 46931a85a..b4ac5089d 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -125,7 +125,7 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss // If the existing order has expired, we should create a new one // TODO: implement setting this order state in the acmeorders controller if existingOrder.Status.State == v1alpha1.Expired { - return a.retryOrder(existingOrder) + return a.retryOrder(crt, existingOrder) } // If the existing order has failed, we should check if the Certificate @@ -144,11 +144,7 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return issuer.IssueResponse{}, fmt.Errorf("applying acme order back-off for certificate %s/%s because it has failed within the last %s", crt.Namespace, crt.Name, createOrderWaitDuration) } - // otherwise, we clear the lastFailureTime and create a new order - // as the back-off time has passed. - crt.Status.LastFailureTime = nil - - return a.retryOrder(existingOrder) + return a.retryOrder(crt, existingOrder) } if existingOrder.Status.State != v1alpha1.Valid { @@ -187,7 +183,7 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss if a.Context.IssuerOptions.CertificateNeedsRenew(x509Cert) { // existing order's certificate is near expiry - return a.retryOrder(existingOrder) + return a.retryOrder(crt, existingOrder) } // encode the retrieved certificates (including the chain) @@ -319,7 +315,7 @@ func (a *Acme) createNewOrder(crt *v1alpha1.Certificate, template *v1alpha1.Orde // deletion policy. // If delete successfully (i.e. cleaned up), the order name will be // reset to empty and a resync of the resource will begin. -func (a *Acme) retryOrder(existingOrder *v1alpha1.Order) (issuer.IssueResponse, error) { +func (a *Acme) retryOrder(crt *v1alpha1.Certificate, existingOrder *v1alpha1.Order) (issuer.IssueResponse, error) { foregroundDeletion := metav1.DeletePropagationForeground err := a.CMClient.CertmanagerV1alpha1().Orders(existingOrder.Namespace).Delete(existingOrder.Name, &metav1.DeleteOptions{ PropagationPolicy: &foregroundDeletion, @@ -328,6 +324,8 @@ func (a *Acme) retryOrder(existingOrder *v1alpha1.Order) (issuer.IssueResponse, return issuer.IssueResponse{}, err } + crt.Status.LastFailureTime = nil + // Updating the certificate status will trigger a requeue once the change // has been observed by the informer. // If we set Requeue: true here, we may cause a race where the lister has From 965757cce0bed7e70ec37c43d2c55be30b2e8def Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 22:46:26 +0100 Subject: [PATCH 08/10] Retry order if existing Order certificate is invalid Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index b4ac5089d..41f07994b 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -177,8 +177,8 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss x509Cert, err := x509.ParseCertificate(certSlice[0]) if err != nil { - // TODO: parse returned ACME error and potentially re-create order. - return issuer.IssueResponse{}, fmt.Errorf("failed to parse returned x509 certificate: %v", err.Error()) + // if parsing the certificate fails, recreate the order + return a.retryOrder(crt, existingOrder) } if a.Context.IssuerOptions.CertificateNeedsRenew(x509Cert) { From c1bd9c4a2e3e03c8b32e4bf03640ff3240a986e4 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 22:54:02 +0100 Subject: [PATCH 09/10] Add missing call to retryOrder in existing order value for private key check Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 41f07994b..dd2c25a6f 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -120,6 +120,7 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss } if !validForKey { glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key. Creating new order.", crt.Namespace, crt.Name) + return a.retryOrder(crt, existingOrder) } // If the existing order has expired, we should create a new one From 36ac13bb1473da0ff2fded62be682a86a6f22ee2 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Mon, 15 Oct 2018 23:12:46 +0100 Subject: [PATCH 10/10] Run //hack:update-bazel Signed-off-by: James Munnelly --- pkg/issuer/acme/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/issuer/acme/BUILD.bazel b/pkg/issuer/acme/BUILD.bazel index ea25d0600..266bc012d 100644 --- a/pkg/issuer/acme/BUILD.bazel +++ b/pkg/issuer/acme/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", "//vendor/k8s.io/apimachinery/pkg/selection:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//vendor/k8s.io/client-go/listers/core/v1:go_default_library", "//vendor/k8s.io/utils/clock:go_default_library", ],