From 315a14ff70bb365fda2d7cae7a74860969b31f28 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 8 Nov 2018 17:13:36 +0000 Subject: [PATCH 1/4] Add more Events to Orders and Challenges Signed-off-by: James Munnelly --- pkg/controller/acmechallenges/scheduler/BUILD.bazel | 1 + pkg/controller/acmechallenges/scheduler/sync.go | 5 ++++- pkg/controller/acmeorders/BUILD.bazel | 1 + pkg/controller/acmeorders/sync.go | 7 +++++++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/controller/acmechallenges/scheduler/BUILD.bazel b/pkg/controller/acmechallenges/scheduler/BUILD.bazel index ea4d7f5c3..300836707 100644 --- a/pkg/controller/acmechallenges/scheduler/BUILD.bazel +++ b/pkg/controller/acmechallenges/scheduler/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//pkg/controller:go_default_library", "//pkg/util:go_default_library", "//vendor/github.com/golang/glog:go_default_library", + "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", diff --git a/pkg/controller/acmechallenges/scheduler/sync.go b/pkg/controller/acmechallenges/scheduler/sync.go index 2cee03402..dbfd65362 100644 --- a/pkg/controller/acmechallenges/scheduler/sync.go +++ b/pkg/controller/acmechallenges/scheduler/sync.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/tools/cache" @@ -78,11 +79,13 @@ func (c *Controller) Sync(ctx context.Context, ch *cmapi.Challenge) error { // if there are no 'conflicts' detected above, then we can mark this challenge // as processing. ch.Status.Processing = true - _, err = c.CMClient.CertmanagerV1alpha1().Challenges(ch.Namespace).Update(ch) + ch, err = c.CMClient.CertmanagerV1alpha1().Challenges(ch.Namespace).Update(ch) if err != nil { return err } + c.Recorder.Event(ch, corev1.EventTypeNormal, "Started", "Challenge scheduled for processing") + // we ignore the return value from waitForCacheSync - if it is false, the // controller will shutdown anyway. _ = c.waitForCacheSync() diff --git a/pkg/controller/acmeorders/BUILD.bazel b/pkg/controller/acmeorders/BUILD.bazel index e0194510b..b43d3355a 100644 --- a/pkg/controller/acmeorders/BUILD.bazel +++ b/pkg/controller/acmeorders/BUILD.bazel @@ -18,6 +18,7 @@ go_library( "//pkg/util:go_default_library", "//third_party/crypto/acme:go_default_library", "//vendor/github.com/golang/glog:go_default_library", + "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index c905dc1d5..9e6783849 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -21,6 +21,7 @@ import ( "fmt" "reflect" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" @@ -186,6 +187,12 @@ func (c *Controller) Sync(ctx context.Context, o *cmapi.Order) (err error) { continue } + domainName := spec.DNSName + if spec.Wildcard { + domainName = "*." + domainName + } + c.Recorder.Eventf(ch, corev1.EventTypeNormal, "Created", "Created Challenge resource %q for domain %q", ch.Name, ch.Spec.DNSName) + existingChallenges = append(existingChallenges, ch) } From 447ad2d2ea2c58f2558a4ab515a5814b91ab399f Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 8 Nov 2018 17:21:58 +0000 Subject: [PATCH 2/4] Fire Event when creating a new Order Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 34c2cf390..49f5fff3a 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -302,11 +302,12 @@ func (a *Acme) createNewOrder(crt *v1alpha1.Certificate, template *v1alpha1.Orde // set the CSR field on the order to be created template.Spec.CSR = csrBytes - _, err = a.CMClient.CertmanagerV1alpha1().Orders(template.Namespace).Create(template) + o, err := a.CMClient.CertmanagerV1alpha1().Orders(template.Namespace).Create(template) if err != nil { return err } + a.Recorder.Eventf(crt, corev1.EventTypeNormal, "OrderCreated", "Create Order resource %q", o.Name) glog.V(4).Infof("Created new Order resource named %q for Certificate %s/%s", template.Name, crt.Namespace, crt.Name) return nil From 826441f6980b502454046f5f8353af9c8acf2e94 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 8 Nov 2018 17:41:33 +0000 Subject: [PATCH 3/4] Add more Event messages to ACME Certificate issuer Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index 49f5fff3a..ba484603a 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -61,6 +61,7 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss // 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) + a.Recorder.Eventf(crt, corev1.EventTypeNormal, "Generated", "Generated new private key") keyPem, err := pki.EncodePrivateKey(key) if err != nil { @@ -156,6 +157,8 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return issuer.IssueResponse{}, nil } + a.Recorder.Eventf(crt, corev1.EventTypeNormal, "Order %q completed successfully", existingOrder.Name) + // If the order is valid, we can attempt to retrieve the Certificate. // First obtain an ACME client to make this easier. cl, err := a.helper.ClientForIssuer(a.issuer) @@ -171,8 +174,8 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return issuer.IssueResponse{}, err } + // TODO: this is a weird error we'd not expect to see if len(certSlice) == 0 { - // TODO: parse returned ACME error and potentially re-create order. return issuer.IssueResponse{}, fmt.Errorf("invalid certificate returned from acme server") } @@ -182,6 +185,8 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss return a.retryOrder(crt, existingOrder) } + // Check if the currently available Certificate from the Order is up to date. + // This may not be the case at renewal time if the old order is still available. if a.Context.IssuerOptions.CertificateNeedsRenew(x509Cert, crt.Spec.RenewBefore) { // existing order's certificate is near expiry return a.retryOrder(crt, existingOrder) From 8f3589e59a1103bc76b1a6167a214d708f32126f Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 8 Nov 2018 17:43:45 +0000 Subject: [PATCH 4/4] Fire warning if retrieving an order's certificate fails Signed-off-by: James Munnelly --- pkg/issuer/acme/issue.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/issuer/acme/issue.go b/pkg/issuer/acme/issue.go index ba484603a..d9784901d 100644 --- a/pkg/issuer/acme/issue.go +++ b/pkg/issuer/acme/issue.go @@ -120,13 +120,14 @@ func (a *Acme) Issue(ctx context.Context, crt *v1alpha1.Certificate) (issuer.Iss 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) + glog.V(4).Infof("CSR on existing order resource does not match certificate %s/%s private key", crt.Namespace, crt.Name) return a.retryOrder(crt, existingOrder) } // 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 { + a.Recorder.Eventf(crt, corev1.EventTypeNormal, "OrderExpired", "Existing certificate for Order %q expired", existingOrder.Name) return a.retryOrder(crt, existingOrder) } @@ -166,16 +167,19 @@ 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. + // 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 so + // we retry the order. certSlice, err := cl.GetCertificate(ctx, existingOrder.Status.CertificateURL) if err != nil { - // TODO: parse returned ACME error and potentially re-create order. + a.Recorder.Eventf(crt, corev1.EventTypeWarning, "Failed to retrieve Certificate for Order %q (%s): %v", existingOrder.Name, existingOrder.Status.CertificateURL, err) return issuer.IssueResponse{}, err } - // TODO: this is a weird error we'd not expect to see + // TODO: retry the order? + // this is a weird error we'd not expect to see if len(certSlice) == 0 { + a.Recorder.Eventf(crt, corev1.EventTypeWarning, "BadCertificate", "Empty certificate data retrieved from ACME server") return issuer.IssueResponse{}, fmt.Errorf("invalid certificate returned from acme server") }