Creates CR reporter util and updates ca_test.go

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

pkg/controller/certificaterequests/BUILD.bazel

@@ -46,6 +46,7 @@ filegroup(
         ":package-srcs",
         "//pkg/controller/certificaterequests/ca:all-srcs",
         "//pkg/controller/certificaterequests/fake:all-srcs",
+        "//pkg/controller/certificaterequests/util:all-srcs",
     ],
     tags = ["automanaged"],
     visibility = ["//visibility:public"],

pkg/controller/certificaterequests/ca/BUILD.bazel

@@ -10,11 +10,12 @@ go_library(
         "//pkg/apis/certmanager/v1alpha1:go_default_library",
         "//pkg/controller:go_default_library",
         "//pkg/controller/certificaterequests:go_default_library",
+        "//pkg/controller/certificaterequests/util:go_default_library",
         "//pkg/issuer:go_default_library",
         "//pkg/logs:go_default_library",
+        "//pkg/util/errors:go_default_library",
         "//pkg/util/kube:go_default_library",
         "//pkg/util/pki: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/client-go/listers/core/v1:go_default_library",
         "//vendor/k8s.io/client-go/tools/record:go_default_library",
@@ -43,11 +44,13 @@ go_test(
         "//pkg/apis/certmanager:go_default_library",
         "//pkg/apis/certmanager/v1alpha1:go_default_library",
         "//pkg/controller/test:go_default_library",
+        "//pkg/controller/test/fake:go_default_library",
         "//pkg/issuer:go_default_library",
         "//pkg/util/pki:go_default_library",
         "//test/unit/gen:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/client-go/listers/core/v1:go_default_library",
     ],
 )

pkg/controller/certificaterequests/ca/ca.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 
-	corev1 "k8s.io/api/core/v1"
 	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/record"
@@ -29,8 +28,10 @@ import (
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
 	controllerpkg "github.com/jetstack/cert-manager/pkg/controller"
 	"github.com/jetstack/cert-manager/pkg/controller/certificaterequests"
+	crutil "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util"
 	issuerpkg "github.com/jetstack/cert-manager/pkg/issuer"
 	logf "github.com/jetstack/cert-manager/pkg/logs"
+	cmerrors "github.com/jetstack/cert-manager/pkg/util/errors"
 	"github.com/jetstack/cert-manager/pkg/util/kube"
 	"github.com/jetstack/cert-manager/pkg/util/pki"
 )
@@ -76,60 +77,47 @@ func NewCA(ctx *controllerpkg.Context) *CA {
 	}
 }
 
-func (c *CA) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) (*issuerpkg.IssueResponse, error) {
+func (c *CA) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest, issuerObj v1alpha1.GenericIssuer) (*issuerpkg.IssueResponse, error) {
 	log := logf.FromContext(ctx, "sign")
+	reporter := crutil.NewReporter(log, cr, c.recorder)
 
-	issuer, err := c.helper.GetGenericIssuer(cr.Spec.IssuerRef, cr.Namespace)
-	if k8sErrors.IsNotFound(err) {
-		apiutil.SetCertificateRequestCondition(cr, v1alpha1.CertificateRequestConditionReady,
-			v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonPending,
-			fmt.Sprintf("Referenced %s not found", apiutil.IssuerKind(cr.Spec.IssuerRef)))
+	secretName := issuerObj.GetSpec().CA.SecretName
+	resourceNamespace := c.issuerOptions.ResourceNamespace(issuerObj)
 
-		c.recorder.Event(cr, corev1.EventTypeWarning, v1alpha1.CertificateRequestReasonPending, err.Error())
-
-		log.WithValues(
-			logf.RelatedResourceNameKey, cr.Spec.IssuerRef.Name,
-			logf.RelatedResourceKindKey, cr.Spec.IssuerRef.Kind,
-		).Error(err, "failed to find referenced issuer")
-
-		return nil, nil
-	}
+	// get a copy of the CA certificate named on the Issuer
+	caCerts, caKey, err := kube.SecretTLSKeyPair(ctx, c.secretsLister, resourceNamespace, issuerObj.GetSpec().CA.SecretName)
 	if err != nil {
-		return nil, err
-	}
+		log := logf.WithRelatedResourceName(log, issuerObj.GetSpec().CA.SecretName, resourceNamespace, "Secret")
+		reporter = reporter.WithLog(log)
 
-	resourceNamespace := c.issuerOptions.ResourceNamespace(issuer)
+		if k8sErrors.IsNotFound(err) {
+			reporter.Pending(err, "MissingSecret",
+				fmt.Sprintf("Referenced secret %s/%s not found", resourceNamespace, secretName))
 
-	// get a copy of the CA certificate named on the Issuer
-	caCerts, caKey, err := kube.SecretTLSKeyPair(ctx, c.secretsLister, resourceNamespace, issuer.GetSpec().CA.SecretName)
-	if k8sErrors.IsNotFound(err) {
-		log := logf.WithRelatedResourceName(log, issuer.GetSpec().CA.SecretName, resourceNamespace, "Secret")
-		log.Info("error getting signing CA for Issuer")
+			return nil, nil
+		}
 
-		c.recorder.Event(cr, corev1.EventTypeWarning, v1alpha1.CertificateRequestReasonPending, err.Error())
+		if cmerrors.IsInvalidData(err) {
+			reporter.Pending(err, "ErrorParsingSecret",
+				fmt.Sprintf("Failed to parse key cert pair from secret %s/%s", resourceNamespace, secretName))
+			return nil, nil
+		}
 
-		return nil, nil
-	}
-	if err != nil {
+		// We are probably in a network error here so we should backoff and retry
+		reporter.Pending(err, "ErrorGettingSecret",
+			fmt.Sprintf("Failed to get key cert pair from secret %s/%s", resourceNamespace, secretName))
 		return nil, err
 	}
 
 	template, err := pki.GenerateTemplateFromCertificateRequest(cr)
 	if err != nil {
-		apiutil.SetCertificateRequestCondition(cr, v1alpha1.CertificateRequestConditionReady,
-			v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonFailed,
-			fmt.Sprintf("Failed to generate certificate template: %s", err))
-
-		// TODO: add mechanism here to handle invalid input errors which should result in a permanent failure
-		log.Error(err, "error generating certificate template")
-		c.recorder.Eventf(cr, corev1.EventTypeWarning, "ErrorSigning", "Error generating certificate template: %v", err)
+		reporter.Failed(err, "ErrorSigning", "Error generating certificate template")
 		return nil, nil
 	}
 
 	certPEM, caPEM, err := pki.SignCSRTemplate(caCerts, caKey, template)
 	if err != nil {
-		log.Error(err, "error signing certificate")
-		c.recorder.Eventf(cr, corev1.EventTypeWarning, "ErrorSigning", "Error signing certificate: %v", err)
+		reporter.Failed(err, "ErrorSigning", "Error signing certificate")
 		return nil, err
 	}
 

pkg/controller/certificaterequests/ca/ca_test.go

@@ -26,17 +26,20 @@ import (
 	"crypto/x509/pkix"
 	"encoding/asn1"
 	"encoding/pem"
+	"errors"
 	"reflect"
 	"testing"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	clientcorev1 "k8s.io/client-go/listers/core/v1"
 
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager"
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
 	testpkg "github.com/jetstack/cert-manager/pkg/controller/test"
+	testfake "github.com/jetstack/cert-manager/pkg/controller/test/fake"
 	"github.com/jetstack/cert-manager/pkg/issuer"
 	"github.com/jetstack/cert-manager/pkg/util/pki"
 	"github.com/jetstack/cert-manager/test/unit/gen"
@@ -166,8 +169,13 @@ func TestSign(t *testing.T) {
 	rootRSANoKeySecret := rootRSACASecret.DeepCopy()
 	rootRSANoKeySecret.Data[corev1.TLSPrivateKeyKey] = make([]byte, 0)
 
+	basicIssuer := gen.Issuer("ca-issuer",
+		gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}),
+	)
+
 	tests := map[string]testT{
 		"sign a CertificateRequest": {
+			issuer: basicIssuer,
 			certificateRequest: gen.CertificateRequest("test-cr",
 				gen.SetCertificateRequestIsCA(true),
 				gen.SetCertificateRequestCSR(caCSR),
@@ -178,17 +186,14 @@ func TestSign(t *testing.T) {
 				}),
 			),
 			builder: &testpkg.Builder{
-				KubeObjects: []runtime.Object{rootRSACASecret},
-				CertManagerObjects: []runtime.Object{
-					gen.Issuer("ca-issuer",
-						gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}),
-					),
-				},
+				KubeObjects:        []runtime.Object{rootRSACASecret},
+				CertManagerObjects: []runtime.Object{},
 				// we are not expecting key on response
 				CheckFn: noPrivateKeyFieldsSetCheck(rsaPEMCert),
 			},
 		},
 		"fail to find CA tls key pair": {
+			issuer: basicIssuer,
 			certificateRequest: gen.CertificateRequest("test-cr",
 				gen.SetCertificateRequestIsCA(true),
 				gen.SetCertificateRequestCSR(caCSR),
@@ -199,17 +204,16 @@ func TestSign(t *testing.T) {
 				}),
 			),
 			builder: &testpkg.Builder{
-				KubeObjects: []runtime.Object{},
-				CertManagerObjects: []runtime.Object{gen.Issuer("ca-issuer",
-					gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}),
-				)},
+				KubeObjects:        []runtime.Object{},
+				CertManagerObjects: []runtime.Object{},
 				ExpectedEvents: []string{
-					`Warning Pending secret "root-ca-secret" not found`,
+					`Normal MissingSecret Referenced secret default-unit-test-ns/root-ca-secret not found: secret "root-ca-secret" not found`,
 				},
 				CheckFn: mustNoResponse,
 			},
 		},
 		"given bad CSR should fail Certificate generation": {
+			issuer: basicIssuer,
 			certificateRequest: gen.CertificateRequest("test-cr",
 				gen.SetCertificateRequestIsCA(true),
 				gen.SetCertificateRequestCSR([]byte("bad-csr")),
@@ -220,17 +224,16 @@ func TestSign(t *testing.T) {
 				}),
 			),
 			builder: &testpkg.Builder{
-				KubeObjects: []runtime.Object{rootRSACASecret},
-				CertManagerObjects: []runtime.Object{gen.Issuer("ca-issuer",
-					gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}),
-				)},
+				KubeObjects:        []runtime.Object{rootRSACASecret},
+				CertManagerObjects: []runtime.Object{},
 				ExpectedEvents: []string{
 					`Warning ErrorSigning Error generating certificate template: failed to decode csr from certificate request resource default-unit-test-ns/test-cr`,
 				},
 				CheckFn: mustNoResponse,
 			},
 		},
 		"no CA certificate should fail a signing": {
+			issuer: basicIssuer,
 			certificateRequest: gen.CertificateRequest("test-cr",
 				gen.SetCertificateRequestIsCA(true),
 				gen.SetCertificateRequestCSR(caCSR),
@@ -241,22 +244,38 @@ func TestSign(t *testing.T) {
 				}),
 			),
 			builder: &testpkg.Builder{
-				KubeObjects: []runtime.Object{rootRSANoCASecret},
-				CertManagerObjects: []runtime.Object{gen.Issuer("ca-issuer",
-					gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}),
-				)},
-				CheckFn: func(builder *testpkg.Builder, args ...interface{}) {
-					err := args[1].(error)
-					badCAError := `error decoding cert PEM block`
-					if err == nil || err.Error() != badCAError {
-						t.Errorf("unexpected error, exp='%s' got='%+v'", badCAError, err)
-					}
-					mustNoResponse(builder, args...)
+				KubeObjects:        []runtime.Object{rootRSANoCASecret},
+				CertManagerObjects: []runtime.Object{},
+				ExpectedEvents: []string{
+					`Normal ErrorParsingSecret Failed to parse key cert pair from secret default-unit-test-ns/root-ca-secret: error decoding cert PEM block`,
 				},
+				CheckFn: mustNoResponse,
 			},
-			expectedErr: true,
+			expectedErr: false,
 		},
 		"no CA key should fail a signing": {
+			issuer: basicIssuer,
+			certificateRequest: gen.CertificateRequest("test-cr",
+				gen.SetCertificateRequestIsCA(true),
+				gen.SetCertificateRequestCSR(caCSR),
+				gen.SetCertificateRequestIssuer(v1alpha1.ObjectReference{
+					Name:  "ca-issuer",
+					Group: certmanager.GroupName,
+					Kind:  "Issuer",
+				}),
+			),
+			builder: &testpkg.Builder{
+				KubeObjects:        []runtime.Object{rootRSANoKeySecret},
+				CertManagerObjects: []runtime.Object{},
+				ExpectedEvents: []string{
+					`Normal ErrorParsingSecret Failed to parse key cert pair from secret default-unit-test-ns/root-ca-secret: error decoding private key PEM block`,
+				},
+				CheckFn: mustNoResponse,
+			},
+			expectedErr: false,
+		},
+		"a CertificateRequest that transiently fails a secret lookup should backoff error to retry": {
+			issuer: basicIssuer,
 			certificateRequest: gen.CertificateRequest("test-cr",
 				gen.SetCertificateRequestIsCA(true),
 				gen.SetCertificateRequestCSR(caCSR),
@@ -267,18 +286,20 @@ func TestSign(t *testing.T) {
 				}),
 			),
 			builder: &testpkg.Builder{
-				KubeObjects: []runtime.Object{rootRSANoKeySecret},
-				CertManagerObjects: []runtime.Object{gen.Issuer("ca-issuer",
-					gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}),
-				)},
-				CheckFn: func(builder *testpkg.Builder, args ...interface{}) {
-					err := args[1].(error)
-					noKeyError := "error decoding private key PEM block"
-					if err == nil || err.Error() != noKeyError {
-						builder.T.Errorf("unexpected error, exp='%s' got='%+v'", noKeyError, err)
+				KubeObjects:        []runtime.Object{rootRSACASecret},
+				CertManagerObjects: []runtime.Object{},
+				CheckFn:            mustNoResponse,
+				ExpectedEvents: []string{
+					"Normal ErrorGettingSecret Failed to get key cert pair from secret default-unit-test-ns/root-ca-secret: this is a network error",
+				},
+			},
+			FakeLister: &testfake.FakeSecretLister{
+				SecretsFn: func(namespace string) clientcorev1.SecretNamespaceLister {
+					return &testfake.FakeSecretNamespaceLister{
+						GetFn: func(name string) (ret *corev1.Secret, err error) {
+							return nil, errors.New("this is a network error")
+						},
 					}
-
-					mustNoResponse(builder, args...)
 				},
 			},
 			expectedErr: true,
@@ -295,9 +316,11 @@ func TestSign(t *testing.T) {
 type testT struct {
 	builder            *testpkg.Builder
 	certificateRequest *v1alpha1.CertificateRequest
+	issuer             v1alpha1.GenericIssuer
 
-	checkFn     func(*testpkg.Builder, ...interface{})
 	expectedErr bool
+
+	FakeLister *testfake.FakeSecretLister
 }
 
 func runTest(t *testing.T, test testT) {
@@ -306,9 +329,14 @@ func runTest(t *testing.T, test testT) {
 	defer test.builder.Stop()
 
 	c := NewCA(test.builder.Context)
+
+	if test.FakeLister != nil {
+		c.secretsLister = test.FakeLister
+	}
+
 	test.builder.Sync()
 
-	resp, err := c.Sign(context.Background(), test.certificateRequest)
+	resp, err := c.Sign(context.Background(), test.certificateRequest, test.issuer)
 	if err != nil && !test.expectedErr {
 		t.Errorf("expected to not get an error, but got: %v", err)
 	}

pkg/controller/certificaterequests/controller.go

@@ -41,7 +41,7 @@ const (
 var keyFunc = controllerpkg.KeyFunc
 
 type Issuer interface {
-	Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) (*issuer.IssueResponse, error)
+	Sign(context.Context, *v1alpha1.CertificateRequest, v1alpha1.GenericIssuer) (*issuer.IssueResponse, error)
 }
 
 type Controller struct {

pkg/controller/certificaterequests/sync.go

@@ -130,7 +130,7 @@ func (c *Controller) Sync(ctx context.Context, cr *v1alpha1.CertificateRequest)
 	dbg.Info("invoking sign function as existing certificate does not exist")
 
 	// Attempt to call the Sign function on our issuer
-	resp, err := c.issuer.Sign(ctx, crCopy)
+	resp, err := c.issuer.Sign(ctx, crCopy, issuerObj)
 	if err != nil {
 		log.Error(err, "error issuing certificate request")
 		return err

pkg/controller/certificaterequests/util/BUILD.bazel

@@ -0,0 +1,29 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["status.go"],
+    importpath = "github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/api/util:go_default_library",
+        "//pkg/apis/certmanager/v1alpha1:go_default_library",
+        "//vendor/github.com/go-logr/logr:go_default_library",
+        "//vendor/k8s.io/api/core/v1:go_default_library",
+        "//vendor/k8s.io/client-go/tools/record:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)