diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned.go b/pkg/controller/certificaterequests/selfsigned/selfsigned.go index 72a9a1dff..d27277376 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned.go @@ -22,9 +22,11 @@ import ( "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" + "github.com/go-logr/logr" apiutil "github.com/jetstack/cert-manager/pkg/api/util" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" controllerpkg "github.com/jetstack/cert-manager/pkg/controller" @@ -78,10 +80,19 @@ func (s *SelfSigned) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) skRef, ok := cr.ObjectMeta.Annotations[v1alpha1.CRPrivateKeyAnnotationKey] if !ok || skRef == "" { - return nil, errorNoAnnotation + s.reportFaliedStatus(log, cr, errorNoAnnotation, "ErrorAnnotation", + fmt.Sprintf("Referenced secret %s/%s not found", cr.Namespace, skRef)) + + return nil, nil } sk, err := kube.SecretTLSKey(ctx, s.secretsLister, cr.Namespace, skRef) + if k8sErrors.IsNotFound(err) { + s.reportFaliedStatus(log, cr, err, "ErrorSecret", + fmt.Sprintf("Referenced secret %s/%s not found", cr.Namespace, skRef)) + + return nil, nil + } if err != nil { return nil, fmt.Errorf("failed to get private key %s referenced in the annotation '%s': %s", skRef, v1alpha1.CRPrivateKeyAnnotationKey, err) @@ -89,32 +100,33 @@ func (s *SelfSigned) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) template, err := pki.GenerateTemplateFromCertificateRequest(cr) if err != nil { - log.Error(err, "error generating certificate template") - s.recorder.Eventf(cr, corev1.EventTypeWarning, "ErrorSigning", "Error generating certificate template: %v", err) - return nil, err + s.reportFaliedStatus(log, cr, err, "ErrorGenerating", "Failed to generate certificate template") + return nil, nil } // extract the public component of the key pk, err := pki.PublicKeyForPrivateKey(sk) if err != nil { - log.Error(err, "failed to get public key from private key") - return nil, err + s.reportFaliedStatus(log, cr, err, "ErrorPublicKey", "Failed to get public key from private key") + return nil, nil } ok, err = pki.PublicKeyMatchesPublicKey(pk, template.PublicKey) - if err != nil { - log.Error(err, "failed to verify CSR is signed by referenced private key") - return nil, err - } + if err != nil || !ok { - if !ok { - return nil, errors.New("CSR not signed by referenced private key") + if err == nil { + err = errors.New("CSR not signed by referenced private key") + } + + s.reportFaliedStatus(log, cr, err, "ErrorKeyMatch", "Error generating certificate template") + + return nil, nil } // sign and encode the certificate certPem, _, err := pki.SignCertificate(template, template, pk, sk) if err != nil { - s.recorder.Eventf(cr, corev1.EventTypeWarning, "ErrorSigning", "Error signing certificate: %v", err) + s.reportFaliedStatus(log, cr, err, "ErrorSigning", "Error signing certificate") return nil, err } @@ -125,3 +137,14 @@ func (s *SelfSigned) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) CA: certPem, }, nil } + +func (s *SelfSigned) reportFaliedStatus(log logr.Logger, cr *v1alpha1.CertificateRequest, err error, + reason, message string) { + // TODO: add mechanism here to handle invalid input errors which should result in a permanent failure + apiutil.SetCertificateRequestCondition(cr, v1alpha1.CertificateRequestConditionReady, + v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonFailed, + message) + + log.Error(err, message) + s.recorder.Event(cr, corev1.EventTypeWarning, reason, fmt.Sprintf("%s: %v", message, err)) +} diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go index 54d13277c..2a48d9575 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go @@ -24,7 +24,6 @@ import ( "crypto/x509/pkix" "encoding/asn1" "encoding/pem" - "fmt" "testing" corev1 "k8s.io/api/core/v1" @@ -58,6 +57,13 @@ func generateCSR(t *testing.T, secretKey crypto.Signer, alg x509.SignatureAlgori return csr } +func mustNoResponse(t *testing.T, args []interface{}) { + resp := args[1].(*issuer.IssueResponse) + if resp != nil { + t.Errorf("unexpected response, exp='nil' got='%+v'", resp) + } +} + func TestSign(t *testing.T) { skRSA, err := pki.GenerateRSAPrivateKey(2048) if err != nil { @@ -126,7 +132,7 @@ func TestSign(t *testing.T) { } tests := map[string]selfsignedFixture{ - "a CertificateRequest with no certmanager.k8s.io/selfsigned-private-key annotation errors": { + "a CertificateRequest with no certmanager.k8s.io/selfsigned-private-key annotation should record error": { Issuer: gen.Issuer("selfsigned-issuer", gen.SetIssuerSelfSigned(v1alpha1.SelfSignedIssuer{}), ), @@ -135,16 +141,16 @@ func TestSign(t *testing.T) { Builder: &testpkg.Builder{ KubeObjects: []runtime.Object{}, CertManagerObjects: []runtime.Object{}, + ExpectedEvents: []string{ + "Warning ErrorAnnotation Referenced secret default-unit-test-ns/ not found: self signed issuer requires 'certmanager.k8s.io/private-key-secret-name' annotation set to secret name holding the private key", + }, }, - Err: true, + Err: false, CheckFn: func(t *testing.T, s *selfsignedFixture, args ...interface{}) { - err := args[2].(error) - if err == nil || err != errorNoAnnotation { - t.Errorf("unexpected error, exp='%s' got='%+v'", errorNoAnnotation, err) - } + mustNoResponse(t, args) }, }, - "a CertificateRequest with a certmanager.k8s.io/private-key-secret-name annotation but empty string should error": { + "a CertificateRequest with a certmanager.k8s.io/private-key-secret-name annotation but empty string should record error": { Issuer: gen.Issuer("selfsigned-issuer", gen.SetIssuerSelfSigned(v1alpha1.SelfSignedIssuer{}), ), @@ -157,16 +163,15 @@ func TestSign(t *testing.T) { Builder: &testpkg.Builder{ KubeObjects: []runtime.Object{}, CertManagerObjects: []runtime.Object{}, + ExpectedEvents: []string{ + "Warning ErrorAnnotation Referenced secret default-unit-test-ns/ not found: self signed issuer requires 'certmanager.k8s.io/private-key-secret-name' annotation set to secret name holding the private key", + }, }, - Err: true, + Err: false, CheckFn: func(t *testing.T, s *selfsignedFixture, args ...interface{}) { - err := args[2].(error) - if err == nil || err != errorNoAnnotation { - t.Errorf("unexpected error, exp='%s' got='%+v'", errorNoAnnotation, err) - } }, }, - "a CertificateRequest with a certmanager.k8s.io/private-key-secret-name annotation but the referenced secret doesn't exist should error": { + "a CertificateRequest with a certmanager.k8s.io/private-key-secret-name annotation but the referenced secret doesn't exist should record error": { Issuer: gen.Issuer("selfsigned-issuer", gen.SetIssuerSelfSigned(v1alpha1.SelfSignedIssuer{}), ), @@ -178,14 +183,12 @@ func TestSign(t *testing.T) { Builder: &testpkg.Builder{ KubeObjects: []runtime.Object{}, CertManagerObjects: []runtime.Object{}, + ExpectedEvents: []string{ + `Warning ErrorSecret Referenced secret default-unit-test-ns/a-non-existent-secret not found: secret "a-non-existent-secret" not found`, + }, }, - Err: true, + Err: false, CheckFn: func(t *testing.T, s *selfsignedFixture, args ...interface{}) { - notFoundError := `failed to get private key a-non-existent-secret referenced in the annotation 'certmanager.k8s.io/private-key-secret-name': secret "a-non-existent-secret" not found` - err := args[2].(error) - if err == nil || err.Error() != notFoundError { - t.Errorf("unexpected error, exp='%s' got='%+v'", notFoundError, err) - } }, }, "a CertificateRequest with a bad CSR should error": { @@ -201,17 +204,15 @@ func TestSign(t *testing.T) { Builder: &testpkg.Builder{ KubeObjects: []runtime.Object{rsaKeySecret}, CertManagerObjects: []runtime.Object{}, + ExpectedEvents: []string{ + "Warning ErrorGenerating Failed to generate certificate template: failed to decode csr from certificate request resource default-unit-test-ns/test-cr", + }, }, - Err: true, + Err: false, CheckFn: func(t *testing.T, s *selfsignedFixture, args ...interface{}) { - parseCSRError := fmt.Sprintf("failed to decode csr from certificate request resource %s/test-cr", gen.DefaultTestNamespace) - err := args[2].(error) - if err == nil || err.Error() != parseCSRError { - t.Errorf("unexpected error, exp='%s' got='%+v'", parseCSRError, err) - } }, }, - "a CertificateRequest referencing a bad key should error": { + "a CertificateRequest referencing a bad key should record an error": { Issuer: gen.Issuer("selfsigned-issuer", gen.SetIssuerSelfSigned(v1alpha1.SelfSignedIssuer{}), ), @@ -234,7 +235,7 @@ func TestSign(t *testing.T) { } }, }, - "a CertificateRequest referencing a key which did not sign the CSR should error": { + "a CertificateRequest referencing a key which did not sign the CSR should record an error": { Issuer: gen.Issuer("selfsigned-issuer", gen.SetIssuerSelfSigned(v1alpha1.SelfSignedIssuer{}), ), @@ -247,14 +248,12 @@ func TestSign(t *testing.T) { Builder: &testpkg.Builder{ KubeObjects: []runtime.Object{anotherRSAKeySecret}, CertManagerObjects: []runtime.Object{}, + ExpectedEvents: []string{ + "Warning ErrorKeyMatch Error generating certificate template: CSR not signed by referenced private key", + }, }, - Err: true, + Err: false, CheckFn: func(t *testing.T, s *selfsignedFixture, args ...interface{}) { - badKeyError := "CSR not signed by referenced private key" - err := args[2].(error) - if err == nil || err.Error() != badKeyError { - t.Errorf("unexpected error, exp='%s' got='%+v'", badKeyError, err) - } }, }, "a valid RSA key should sign a self signed certificate": { @@ -342,14 +341,6 @@ func TestSign(t *testing.T) { t.Errorf("Expected function to get an error, but got: %v", err) } - if test.Err && resp != nil { - t.Errorf("unexpected response, exp=nil got='%+v'", resp) - } - - if !test.Err && resp == nil { - t.Errorf("expected response but got nil") - } - test.Finish(t, crCopy, resp, err) }) } diff --git a/pkg/controller/certificaterequests/selfsigned/util_test.go b/pkg/controller/certificaterequests/selfsigned/util_test.go index 8b92cb3eb..8457543ff 100644 --- a/pkg/controller/certificaterequests/selfsigned/util_test.go +++ b/pkg/controller/certificaterequests/selfsigned/util_test.go @@ -78,6 +78,9 @@ func (s *selfsignedFixture) Finish(t *testing.T, args ...interface{}) { if err := s.Builder.AllActionsExecuted(); err != nil { t.Errorf(err.Error()) } + if err := s.Builder.AllEventsCalled(); err != nil { + t.Errorf(err.Error()) + } // resync listers before running checks s.Builder.Sync()