From 66820ed03e3230d7ab75f73814f84460a2ba9d24 Mon Sep 17 00:00:00 2001 From: Inteon <42113979+inteon@users.noreply.github.com> Date: Wed, 4 Aug 2021 18:50:29 +0200 Subject: [PATCH] resolve bugs Signed-off-by: Inteon <42113979+inteon@users.noreply.github.com> --- test/e2e/framework/helper/certificates.go | 24 ++++++++++--------- .../conformance/certificates/BUILD.bazel | 2 -- .../suite/conformance/certificates/tests.go | 20 ++++------------ .../suite/issuers/acme/certificate/http01.go | 6 ++--- test/e2e/suite/issuers/ca/certificate.go | 8 +++---- test/e2e/suite/serving/cainjector.go | 17 ++++++------- 6 files changed, 30 insertions(+), 47 deletions(-) diff --git a/test/e2e/framework/helper/certificates.go b/test/e2e/framework/helper/certificates.go index 6e86272ac..e6937bba5 100644 --- a/test/e2e/framework/helper/certificates.go +++ b/test/e2e/framework/helper/certificates.go @@ -38,7 +38,7 @@ import ( // WaitForCertificateToExist waits for the named certificate to exist and returns the certificate func (h *Helper) WaitForCertificateToExist(namespace string, name string, timeout time.Duration) (*cmapi.Certificate, error) { client := h.CMClient.CertmanagerV1().Certificates(namespace) - var certificate *v1.Certificate = nil + var certificate *v1.Certificate pollErr := wait.PollImmediate(500*time.Millisecond, timeout, func() (bool, error) { log.Logf("Waiting for Certificate %v to exist", name) var err error @@ -56,7 +56,7 @@ func (h *Helper) WaitForCertificateToExist(namespace string, name string, timeou } func (h *Helper) waitForCertificateCondition(client clientset.CertificateInterface, name string, check func(*v1.Certificate) bool, timeout time.Duration) (*cmapi.Certificate, error) { - var certificate *v1.Certificate = nil + var certificate *v1.Certificate pollErr := wait.PollImmediate(500*time.Millisecond, timeout, func() (bool, error) { var err error certificate, err = client.Get(context.TODO(), name, metav1.GetOptions{}) @@ -102,8 +102,9 @@ func (h *Helper) WaitForCertificateReadyAndDoneIssuing(cert *cmapi.Certificate, Status: cmmeta.ConditionTrue, ObservedGeneration: cert.Generation, } - issuing_condition := cmapi.CertificateCondition{ - Type: cmapi.CertificateConditionIssuing, + issuing_true_condition := cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionTrue, } return h.waitForCertificateCondition(h.CMClient.CertmanagerV1().Certificates(cert.Namespace), cert.Name, func(certificate *v1.Certificate) bool { if !apiutil.CertificateHasConditionWithObservedGeneration(certificate, ready_true_condition) { @@ -118,8 +119,8 @@ func (h *Helper) WaitForCertificateReadyAndDoneIssuing(cert *cmapi.Certificate, return false } - if apiutil.CertificateHasCondition(certificate, issuing_condition) { - log.Logf("Expected Certificate %v condition %v to be missing but it has: %v", certificate.Name, issuing_condition.Type, certificate.Status.Conditions) + if apiutil.CertificateHasCondition(certificate, issuing_true_condition) { + log.Logf("Expected Certificate %v condition %v to be missing but it has: %v", certificate.Name, issuing_true_condition.Type, certificate.Status.Conditions) return false } @@ -135,11 +136,12 @@ func (h *Helper) WaitForCertificateNotReadyAndDoneIssuing(cert *cmapi.Certificat Status: cmmeta.ConditionFalse, ObservedGeneration: cert.Generation, } - issuing_condition := cmapi.CertificateCondition{ - Type: cmapi.CertificateConditionIssuing, + issuing_true_condition := cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionTrue, } return h.waitForCertificateCondition(h.CMClient.CertmanagerV1().Certificates(cert.Namespace), cert.Name, func(certificate *v1.Certificate) bool { - if !apiutil.CertificateHasCondition(certificate, ready_false_condition) { + if !apiutil.CertificateHasConditionWithObservedGeneration(certificate, ready_false_condition) { log.Logf( "Expected Certificate %v condition %v=%v (generation >= %v) but it has: %v", certificate.Name, @@ -151,8 +153,8 @@ func (h *Helper) WaitForCertificateNotReadyAndDoneIssuing(cert *cmapi.Certificat return false } - if apiutil.CertificateHasCondition(certificate, issuing_condition) { - log.Logf("Expected Certificate %v condition %v to be missing but it has: %v", certificate.Name, issuing_condition.Type, certificate.Status.Conditions) + if apiutil.CertificateHasCondition(certificate, issuing_true_condition) { + log.Logf("Expected Certificate %v condition %v to be missing but it has: %v", certificate.Name, issuing_true_condition.Type, certificate.Status.Conditions) return false } diff --git a/test/e2e/suite/conformance/certificates/BUILD.bazel b/test/e2e/suite/conformance/certificates/BUILD.bazel index 2e5475978..1d5169f71 100644 --- a/test/e2e/suite/conformance/certificates/BUILD.bazel +++ b/test/e2e/suite/conformance/certificates/BUILD.bazel @@ -24,8 +24,6 @@ go_library( "@io_k8s_api//networking/v1:go_default_library", "@io_k8s_api//networking/v1beta1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", - "@io_k8s_apimachinery//pkg/types:go_default_library", - "@io_k8s_client_go//util/retry:go_default_library", ], ) diff --git a/test/e2e/suite/conformance/certificates/tests.go b/test/e2e/suite/conformance/certificates/tests.go index bd88e30f0..c6a73ae40 100644 --- a/test/e2e/suite/conformance/certificates/tests.go +++ b/test/e2e/suite/conformance/certificates/tests.go @@ -28,8 +28,6 @@ import ( networkingv1 "k8s.io/api/networking/v1" networkingv1beta1 "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/util/retry" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" @@ -872,26 +870,16 @@ func (s *Suite) Define() { err = f.Helper().ValidateCertificate(testCertificate, validations...) Expect(err).NotTo(HaveOccurred()) - By("Getting the latest version of the Certificate") - cert, err := f.Helper().CMClient.CertmanagerV1().Certificates(f.Namespace.Name).Get(context.TODO(), "testcert", metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred()) - By("Updating the Certificate after having added an additional dnsName") + testCertificate = testCertificate.DeepCopy() // DeepCopy before updating newDNSName := e2eutil.RandomSubdomain(s.DomainSuffix) - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - err := f.CRClient.Get(context.Background(), types.NamespacedName{Namespace: f.Namespace.Name, Name: "testcert"}, cert) - if err != nil { - return err - } + testCertificate.Spec.DNSNames = append(testCertificate.Spec.DNSNames, newDNSName) - cert.Spec.DNSNames = append(cert.Spec.DNSNames, newDNSName) - - return f.CRClient.Update(context.TODO(), cert) - }) + err = f.CRClient.Update(context.TODO(), testCertificate) Expect(err).NotTo(HaveOccurred()) By("Waiting for the Certificate Ready condition to be updated") - cert, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) + testCertificate, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(testCertificate, time.Minute*5) Expect(err).NotTo(HaveOccurred()) By("Sanity-check the issued Certificate") diff --git a/test/e2e/suite/issuers/acme/certificate/http01.go b/test/e2e/suite/issuers/acme/certificate/http01.go index 9c02411bd..2e3750cc8 100644 --- a/test/e2e/suite/issuers/acme/certificate/http01.go +++ b/test/e2e/suite/issuers/acme/certificate/http01.go @@ -48,8 +48,6 @@ import ( "github.com/jetstack/cert-manager/test/unit/gen" ) -const foreverTestTimeout = time.Second * 60 - var _ = framework.CertManagerDescribe("ACME Certificate (HTTP01)", func() { f := framework.NewDefaultFramework("create-acme-certificate-http01") @@ -259,7 +257,7 @@ var _ = framework.CertManagerDescribe("ACME Certificate (HTTP01)", func() { } By("Waiting for Certificate to exist") - cert, err := f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateSecretName, foreverTestTimeout) + cert, err := f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateSecretName, time.Second*60) Expect(err).NotTo(HaveOccurred()) By("Waiting for the Certificate to be issued...") @@ -464,7 +462,7 @@ var _ = framework.CertManagerDescribe("ACME Certificate (HTTP01)", func() { Expect(err).NotTo(HaveOccurred()) By("Waiting for Certificate to exist") - cert, err = f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateSecretName, foreverTestTimeout) + cert, err = f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateName, time.Second*60) Expect(err).NotTo(HaveOccurred()) // The pod should get remade and the certificate should be made valid. diff --git a/test/e2e/suite/issuers/ca/certificate.go b/test/e2e/suite/issuers/ca/certificate.go index f7abb756a..119af3016 100644 --- a/test/e2e/suite/issuers/ca/certificate.go +++ b/test/e2e/suite/issuers/ca/certificate.go @@ -77,7 +77,7 @@ var _ = framework.CertManagerDescribe("CA Certificate", func() { Expect(err).NotTo(HaveOccurred()) By("Verifying the Certificate is valid") By("Waiting for the Certificate to be issued...") - _, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) + cert, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) Expect(err).NotTo(HaveOccurred()) By("Validating the issued Certificate...") @@ -152,7 +152,7 @@ var _ = framework.CertManagerDescribe("CA Certificate", func() { cert, err := certClient.Create(context.TODO(), util.NewCertManagerBasicCertificate(certificateName, certificateSecretName, issuerName, v1.IssuerKind, v.inputDuration, v.inputRenewBefore), metav1.CreateOptions{}) Expect(err).NotTo(HaveOccurred()) By("Waiting for the Certificate to be issued...") - _, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) + cert, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) Expect(err).NotTo(HaveOccurred()) By("Validating the issued Certificate...") @@ -178,7 +178,7 @@ var _ = framework.CertManagerDescribe("CA Certificate", func() { cert, err := certClient.Create(context.TODO(), util.NewCertManagerBasicCertificate(certificateName, certificateSecretName, issuerName, v1.IssuerKind, nil, nil), metav1.CreateOptions{}) Expect(err).NotTo(HaveOccurred()) By("Waiting for the Certificate to be issued...") - _, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) + cert, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) Expect(err).NotTo(HaveOccurred()) By("Validating the issued Certificate...") @@ -201,7 +201,7 @@ var _ = framework.CertManagerDescribe("CA Certificate", func() { cert, err := certClient.Create(context.TODO(), gen.Certificate(certificateName, gen.SetCertificateNamespace(f.Namespace.Name), gen.SetCertificateCommonName("test.domain.com"), gen.SetCertificateSecretName(certificateSecretName), gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: issuerName, Kind: v1.IssuerKind}), gen.SetCertificateKeyUsages(v1.UsageServerAuth, v1.UsageClientAuth)), metav1.CreateOptions{}) Expect(err).NotTo(HaveOccurred()) By("Waiting for the Certificate to be issued...") - _, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) + cert, err = f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Minute*5) Expect(err).NotTo(HaveOccurred()) By("Validating the issued Certificate...") diff --git a/test/e2e/suite/serving/cainjector.go b/test/e2e/suite/serving/cainjector.go index a6688ac02..c12b47a7d 100644 --- a/test/e2e/suite/serving/cainjector.go +++ b/test/e2e/suite/serving/cainjector.go @@ -80,7 +80,7 @@ var _ = framework.CertManagerDescribe("CA Injector", func() { } Expect(f.CRClient.Delete(context.Background(), toCleanup)).To(Succeed()) }) - generalSetup := func(injectable client.Object) (runtime.Object, certmanager.Certificate, corev1.Secret) { + generalSetup := func(injectable client.Object) (runtime.Object, *certmanager.Certificate) { By("creating a " + subj + " pointing to a cert") Expect(f.CRClient.Create(context.Background(), injectable)).To(Succeed()) toCleanup = injectable @@ -113,8 +113,7 @@ var _ = framework.CertManagerDescribe("CA Injector", func() { return test.getCAs(newInjectable), nil }, "10s", "2s").Should(Equal(expectedCAs)) - return injectable, *cert, secret - + return injectable, cert } It("should inject the CA data into all CA fields", func() { @@ -150,21 +149,19 @@ var _ = framework.CertManagerDescribe("CA Injector", func() { if test.disabled != "" { Skip(test.disabled) } - injectable, cert, _ := generalSetup(test.makeInjectable("changed")) - - By("grabbing the latest copy of the cert") - Expect(f.CRClient.Get(context.Background(), types.NamespacedName{Name: cert.Name, Namespace: cert.Namespace}, &cert)).To(Succeed()) + injectable, cert := generalSetup(test.makeInjectable("changed")) By("changing the name of the corresponding secret in the cert") - secretName := types.NamespacedName{Name: cert.Spec.SecretName, Namespace: f.Namespace.Name} + cert = cert.DeepCopy() // DeepCopy before updating cert.Spec.DNSNames = append(cert.Spec.DNSNames, "something.com") - Expect(f.CRClient.Update(context.Background(), &cert)).To(Succeed()) + Expect(f.CRClient.Update(context.Background(), cert)).To(Succeed()) - _, err := f.Helper().WaitForCertificateReadyAndDoneIssuing(&cert, time.Second*30) + cert, err := f.Helper().WaitForCertificateReadyAndDoneIssuing(cert, time.Second*30) Expect(err).NotTo(HaveOccurred(), "failed to wait for Certificate to become updated") By("grabbing the new secret") var secret corev1.Secret + secretName := types.NamespacedName{Name: cert.Spec.SecretName, Namespace: f.Namespace.Name} Expect(f.CRClient.Get(context.Background(), secretName, &secret)).To(Succeed()) By("verifying that the hooks have the new data")