diff --git a/pkg/api/util/issuers.go b/pkg/api/util/issuers.go index d51aceb45..b82beda5d 100644 --- a/pkg/api/util/issuers.go +++ b/pkg/api/util/issuers.go @@ -52,3 +52,11 @@ func NameForIssuer(i cmapi.GenericIssuer) (string, error) { } return "", fmt.Errorf("no issuer specified for Issuer '%s/%s'", i.GetObjectMeta().Namespace, i.GetObjectMeta().Name) } + +// issuerKind returns the kind of issuer for a certificate +func IssuerKind(ref cmapi.ObjectReference) string { + if ref.Kind == "" { + return cmapi.IssuerKind + } + return ref.Kind +} diff --git a/pkg/controller/certificaterequests/BUILD.bazel b/pkg/controller/certificaterequests/BUILD.bazel index 0a7317430..10db1451a 100644 --- a/pkg/controller/certificaterequests/BUILD.bazel +++ b/pkg/controller/certificaterequests/BUILD.bazel @@ -20,7 +20,6 @@ go_library( "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", "//pkg/metrics:go_default_library", - "//pkg/util:go_default_library", "//pkg/util/pki:go_default_library", "//vendor/github.com/go-logr/logr:go_default_library", "//vendor/github.com/kr/pretty:go_default_library", diff --git a/pkg/controller/certificaterequests/ca/BUILD.bazel b/pkg/controller/certificaterequests/ca/BUILD.bazel index 8b7c9510b..2f111174b 100644 --- a/pkg/controller/certificaterequests/ca/BUILD.bazel +++ b/pkg/controller/certificaterequests/ca/BUILD.bazel @@ -12,7 +12,6 @@ go_library( "//pkg/controller/certificaterequests:go_default_library", "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", - "//pkg/util:go_default_library", "//pkg/util/kube:go_default_library", "//pkg/util/pki:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", diff --git a/pkg/controller/certificaterequests/ca/ca.go b/pkg/controller/certificaterequests/ca/ca.go index 514d47704..efdd5409b 100644 --- a/pkg/controller/certificaterequests/ca/ca.go +++ b/pkg/controller/certificaterequests/ca/ca.go @@ -31,7 +31,6 @@ import ( "github.com/jetstack/cert-manager/pkg/controller/certificaterequests" "github.com/jetstack/cert-manager/pkg/issuer" logf "github.com/jetstack/cert-manager/pkg/logs" - "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/kube" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -84,7 +83,7 @@ func (c *CA) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) (*issuer if k8sErrors.IsNotFound(err) { apiutil.SetCertificateRequestCondition(cr, v1alpha1.CertificateRequestConditionReady, v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonPending, - fmt.Sprintf("Referenced %s not found", util.IssuerKind(cr.Spec.IssuerRef))) + fmt.Sprintf("Referenced %s not found", apiutil.IssuerKind(cr.Spec.IssuerRef))) c.recorder.Event(cr, corev1.EventTypeWarning, v1alpha1.CertificateRequestReasonPending, err.Error()) @@ -92,6 +91,8 @@ func (c *CA) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) (*issuer logf.RelatedResourceNameKey, cr.Spec.IssuerRef.Name, logf.RelatedResourceKindKey, cr.Spec.IssuerRef.Kind, ).Error(err, "failed to find referenced issuer") + + return nil, nil } if err != nil { return nil, err @@ -106,6 +107,8 @@ func (c *CA) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) (*issuer log.Info("error getting signing CA for Issuer") c.recorder.Event(cr, corev1.EventTypeWarning, v1alpha1.CertificateRequestReasonPending, err.Error()) + + return nil, nil } if err != nil { return nil, err @@ -114,9 +117,10 @@ func (c *CA) Sign(ctx context.Context, cr *v1alpha1.CertificateRequest) (*issuer template, err := pki.GenerateTemplateFromCertificateRequest(cr) if err != nil { apiutil.SetCertificateRequestCondition(cr, v1alpha1.CertificateRequestConditionReady, - v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonPending, - fmt.Sprintf("Referenced %s not found", util.IssuerKind(cr.Spec.IssuerRef))) + 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) return nil, err diff --git a/pkg/controller/certificaterequests/ca/ca_test.go b/pkg/controller/certificaterequests/ca/ca_test.go index f6fdce72c..d9afead93 100644 --- a/pkg/controller/certificaterequests/ca/ca_test.go +++ b/pkg/controller/certificaterequests/ca/ca_test.go @@ -204,18 +204,10 @@ func TestSign(t *testing.T) { gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}), )}, }, - ExpectedEvents: []string{ - `Warning Pending secret "root-ca-secret" not found`, - }, CheckFn: func(t *testing.T, s *caFixture, args ...interface{}) { - err := args[2].(error) - noSecretError := `secret "root-ca-secret" not found` - if err == nil || err.Error() != noSecretError { - t.Errorf("unexpected error, exp='%s' got='%+v'", noSecretError, err) - } mustNoResponse(t, args) }, - Err: true, + Err: false, }, "given bad CSR should fail Certificate generation": { CertificateRequest: gen.CertificateRequest("test-cr", @@ -233,9 +225,6 @@ func TestSign(t *testing.T) { gen.SetIssuerCA(v1alpha1.CAIssuer{SecretName: "root-ca-secret"}), )}, }, - ExpectedEvents: []string{ - `Warning ErrorSigning Error generating certificate template: failed to decode csr from certificate request resource default-unit-test-ns/test-cr`, - }, CheckFn: func(t *testing.T, s *caFixture, args ...interface{}) { err := args[2].(error) badCSRError := `failed to decode csr from certificate request resource default-unit-test-ns/test-cr` diff --git a/pkg/controller/certificaterequests/sync.go b/pkg/controller/certificaterequests/sync.go index 695b9254e..644c52e9b 100644 --- a/pkg/controller/certificaterequests/sync.go +++ b/pkg/controller/certificaterequests/sync.go @@ -32,7 +32,6 @@ import ( "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" "github.com/jetstack/cert-manager/pkg/apis/certmanager/validation" logf "github.com/jetstack/cert-manager/pkg/logs" - "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -69,7 +68,7 @@ func (c *Controller) Sync(ctx context.Context, cr *v1alpha1.CertificateRequest) if k8sErrors.IsNotFound(err) { apiutil.SetCertificateRequestCondition(crCopy, v1alpha1.CertificateRequestConditionReady, v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonPending, - fmt.Sprintf("Referenced %s not found", util.IssuerKind(crCopy.Spec.IssuerRef))) + fmt.Sprintf("Referenced %s not found", apiutil.IssuerKind(crCopy.Spec.IssuerRef))) c.recorder.Eventf(crCopy, corev1.EventTypeWarning, v1alpha1.CertificateRequestReasonPending, err.Error()) @@ -93,7 +92,7 @@ func (c *Controller) Sync(ctx context.Context, cr *v1alpha1.CertificateRequest) if err != nil { apiutil.SetCertificateRequestCondition(crCopy, v1alpha1.CertificateRequestConditionReady, v1alpha1.ConditionFalse, v1alpha1.CertificateRequestReasonPending, - fmt.Sprintf("Referenced %s not found", util.IssuerKind(crCopy.Spec.IssuerRef))) + fmt.Sprintf("Referenced %s not found", apiutil.IssuerKind(crCopy.Spec.IssuerRef))) c.recorder.Eventf(crCopy, corev1.EventTypeWarning, v1alpha1.CertificateRequestReasonPending, err.Error()) log.Error(err, "failed to obtain referenced issuer type") diff --git a/pkg/controller/certificaterequests/util_test.go b/pkg/controller/certificaterequests/util_test.go index 44ad668ed..23ff5489f 100644 --- a/pkg/controller/certificaterequests/util_test.go +++ b/pkg/controller/certificaterequests/util_test.go @@ -68,6 +68,9 @@ func (f *controllerFixture) Setup(t *testing.T) { func (f *controllerFixture) Finish(t *testing.T, args ...interface{}) { defer f.Builder.Stop() + if err := f.Builder.AllReactorsCalled(); err != nil { + t.Errorf("Not all expected reactors were called: %v", err) + } if err := f.Builder.AllActionsExecuted(); err != nil { t.Errorf(err.Error()) } diff --git a/pkg/controller/certificates/certificate_request.go b/pkg/controller/certificates/certificate_request.go index 40bcf189d..8053086db 100644 --- a/pkg/controller/certificates/certificate_request.go +++ b/pkg/controller/certificates/certificate_request.go @@ -51,7 +51,6 @@ import ( logf "github.com/jetstack/cert-manager/pkg/logs" "github.com/jetstack/cert-manager/pkg/metrics" "github.com/jetstack/cert-manager/pkg/scheduler" - "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/errors" "github.com/jetstack/cert-manager/pkg/util/kube" "github.com/jetstack/cert-manager/pkg/util/pki" @@ -895,7 +894,7 @@ func setSecretValues(ctx context.Context, crt *cmapi.Certificate, s *corev1.Secr s.Annotations[cmapi.CertificateNameKey] = crt.Name s.Annotations[cmapi.IssuerNameAnnotationKey] = crt.Spec.IssuerRef.Name - s.Annotations[cmapi.IssuerKindAnnotationKey] = util.IssuerKind(crt.Spec.IssuerRef) + s.Annotations[cmapi.IssuerKindAnnotationKey] = apiutil.IssuerKind(crt.Spec.IssuerRef) // if the certificate data is empty, clear the subject related annotations if len(data.cert) == 0 { diff --git a/pkg/controller/certificates/sync.go b/pkg/controller/certificates/sync.go index 70af18449..e037513ae 100644 --- a/pkg/controller/certificates/sync.go +++ b/pkg/controller/certificates/sync.go @@ -294,7 +294,7 @@ func certificateMatchesSpec(crt *v1alpha1.Certificate, key crypto.Signer, cert * } // validate that the issuer kind is correct - if util.IssuerKind(crt.Spec.IssuerRef) != secret.Annotations[v1alpha1.IssuerKindAnnotationKey] { + if apiutil.IssuerKind(crt.Spec.IssuerRef) != secret.Annotations[v1alpha1.IssuerKindAnnotationKey] { errs = append(errs, fmt.Sprintf("Issuer kind of the certificate is not up to date: %q", secret.Annotations[v1alpha1.IssuerKindAnnotationKey])) } @@ -442,7 +442,7 @@ func (c *controller) updateSecret(ctx context.Context, crt *v1alpha1.Certificate // not just when a new certificate is issued if x509Cert != nil { secret.Annotations[v1alpha1.IssuerNameAnnotationKey] = crt.Spec.IssuerRef.Name - secret.Annotations[v1alpha1.IssuerKindAnnotationKey] = util.IssuerKind(crt.Spec.IssuerRef) + secret.Annotations[v1alpha1.IssuerKindAnnotationKey] = apiutil.IssuerKind(crt.Spec.IssuerRef) secret.Annotations[v1alpha1.CommonNameAnnotationKey] = x509Cert.Subject.CommonName secret.Annotations[v1alpha1.AltNamesAnnotationKey] = strings.Join(x509Cert.DNSNames, ",") secret.Annotations[v1alpha1.IPSANAnnotationKey] = strings.Join(pki.IPAddressesToString(x509Cert.IPAddresses), ",") diff --git a/pkg/controller/test/actions.go b/pkg/controller/test/actions.go index c8aa44ead..a977dbd8d 100644 --- a/pkg/controller/test/actions.go +++ b/pkg/controller/test/actions.go @@ -84,8 +84,5 @@ func (a *action) Matches(act coretesting.Action) error { return nil } - fmt.Printf("EXP:%+v\n", objExp.GetObject()) - fmt.Printf("ACT:%+v\n", objExp.GetObject()) - return fmt.Errorf("unexpected difference between actions: %s", pretty.Diff(objExp.GetObject(), objAct.GetObject())) } diff --git a/pkg/controller/test/context_builder.go b/pkg/controller/test/context_builder.go index eab6c9373..42bc5dd51 100644 --- a/pkg/controller/test/context_builder.go +++ b/pkg/controller/test/context_builder.go @@ -59,7 +59,6 @@ type Builder struct { stopCh chan struct{} events []string requiredReactors map[string]bool - fakeRecorder *record.FakeRecorder *controller.Context } @@ -99,14 +98,31 @@ func (b *Builder) Start() { // create a fake recorder with a buffer of 5. // this may need to be increased in future to acomodate tests that // produce more than 5 events - b.fakeRecorder = record.NewFakeRecorder(5) - b.Recorder = b.fakeRecorder + b.Recorder = record.NewFakeRecorder(5) + // read all events out of the recorder and just log for now + // TODO: validate logged events + go func() { + // Skip logging event messages due to a race/timing issue in the test + // framework that needs investigating, where log is called after the + // test has finished + + //r, ok := b.Recorder.(*record.FakeRecorder) + //if !ok { + // return + //} + + // exits when r.Events is closed in Finish + //for e := range r.Events { + // b.logf("Event logged: %v", e) + //} + }() b.FakeKubeClient().PrependReactor("create", "*", b.generateNameReactor) b.FakeCMClient().PrependReactor("create", "*", b.generateNameReactor) b.KubeSharedInformerFactory = kubeinformers.NewSharedInformerFactory(b.Client, informerResyncPeriod) b.SharedInformerFactory = informers.NewSharedInformerFactory(b.CMClient, informerResyncPeriod) b.stopCh = make(chan struct{}) + go b.readEvents() } func (b *Builder) FakeKubeClient() *kubefake.Clientset { @@ -125,6 +141,18 @@ func (b *Builder) FakeCMInformerFactory() informers.SharedInformerFactory { return b.Context.SharedInformerFactory } +func (b *Builder) EnsureReactorCalled(testName string, fn coretesting.ReactionFunc) coretesting.ReactionFunc { + b.requiredReactors[testName] = false + return func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + handled, ret, err = fn(action) + if !handled { + return + } + b.requiredReactors[testName] = true + return + } +} + func (b *Builder) AllReactorsCalled() error { var errs []error for n, reactorCalled := range b.requiredReactors { @@ -190,10 +218,6 @@ func actionToString(a coretesting.Action) string { return fmt.Sprintf("%s %q in namespace %s", a.GetVerb(), a.GetResource(), a.GetNamespace()) } -func (b *Builder) Foo() { - close(b.fakeRecorder.Events) -} - // Stop will signal the informers to stop watching changes // This method is *not* safe to be called concurrently func (b *Builder) Stop() { @@ -203,9 +227,9 @@ func (b *Builder) Stop() { close(b.stopCh) - //if r, ok := b.Recorder.(*record.FakeRecorder); ok { - // close(r.Events) - //} + if r, ok := b.Recorder.(*record.FakeRecorder); ok { + close(r.Events) + } } // WaitForResync will wait for the informer factory informer duration by @@ -232,18 +256,18 @@ func (b *Builder) FakeEventRecorder() *record.FakeRecorder { } func (b *Builder) Events() []string { - if b.fakeRecorder.Events != nil { - b.readEvents() - } return b.events } func (b *Builder) readEvents() { - close(b.fakeRecorder.Events) - for e := range b.FakeEventRecorder().Events { - b.events = append(b.events, e) + for { + select { + case e := <-b.FakeEventRecorder().Events: + b.events = append(b.events, e) + case <-b.stopCh: + return + } } - b.fakeRecorder.Events = nil } func mustAllSync(in map[reflect.Type]bool) error { diff --git a/pkg/util/BUILD.bazel b/pkg/util/BUILD.bazel index 8351980c7..732110067 100644 --- a/pkg/util/BUILD.bazel +++ b/pkg/util/BUILD.bazel @@ -5,7 +5,6 @@ go_library( srcs = [ "context.go", "ingress.go", - "issuer.go", "useragent.go", "util.go", "version.go", @@ -17,7 +16,6 @@ go_library( "AppGitCommit": "{STABLE_APP_GIT_COMMIT}", "AppGitState": "{STABLE_APP_GIT_STATE}", }, - deps = ["//pkg/apis/certmanager/v1alpha1:go_default_library"], ) go_test( diff --git a/pkg/util/issuer.go b/pkg/util/issuer.go deleted file mode 100644 index c90b0b28d..000000000 --- a/pkg/util/issuer.go +++ /dev/null @@ -1,29 +0,0 @@ -/* -Copyright 2019 The Jetstack cert-manager contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package util - -import ( - "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" -) - -// issuerKind returns the kind of issuer for a certificate -func IssuerKind(ref v1alpha1.ObjectReference) string { - if ref.Kind == "" { - return v1alpha1.IssuerKind - } - return ref.Kind -}