From 83f80691c7d362064512b5dcb90fd2c6996b0d6c Mon Sep 17 00:00:00 2001 From: Arsh Sharma Date: Thu, 22 Jul 2021 17:18:22 +0530 Subject: [PATCH 1/5] changes from pair programming session 22nd July Signed-off-by: Arsh Sharma --- .../framework/helper/featureset/featureset.go | 28 ++++++++++-- .../conformance/certificates/acme/acme.go | 45 +++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/test/e2e/framework/helper/featureset/featureset.go b/test/e2e/framework/helper/featureset/featureset.go index 31c2868c5..00ca1d631 100644 --- a/test/e2e/framework/helper/featureset/featureset.go +++ b/test/e2e/framework/helper/featureset/featureset.go @@ -32,9 +32,12 @@ func NewFeatureSet(feats ...Feature) FeatureSet { // just defines a grouping of features (i.e. a 'set'). type FeatureSet map[Feature]struct{} -// Add adds a feature to the set -func (fs FeatureSet) Add(f Feature) { - fs[f] = struct{}{} +// Add adds features to the set +func (fs FeatureSet) Add(f ...Feature) FeatureSet { + for _, feat := range f { + fs[feat] = struct{}{} + } + return fs } // Delete removes a feature from the set @@ -48,6 +51,25 @@ func (fs FeatureSet) Contains(f Feature) bool { return ok } +// Copy returns a new copy of an existing Feature Set. +// It is not safe to be called by multiple goroutines. +func (fs FeatureSet) Copy() FeatureSet { + new := make(FeatureSet) + for k, v := range fs { + new[k] = v + } + return new +} + +// List returns a slice of all features in the set. +func (fs FeatureSet) List() []Feature { + var ret []Feature + for k := range fs { + ret = append(ret, k) + } + return ret +} + // String returns this FeatureSet as a comma separated string func (fs FeatureSet) String() string { featsSlice := make([]string, len(fs)) diff --git a/test/e2e/suite/conformance/certificates/acme/acme.go b/test/e2e/suite/conformance/certificates/acme/acme.go index acfe2aa34..024471608 100644 --- a/test/e2e/suite/conformance/certificates/acme/acme.go +++ b/test/e2e/suite/conformance/certificates/acme/acme.go @@ -34,10 +34,15 @@ import ( "github.com/jetstack/cert-manager/test/e2e/suite/conformance/certificates" ) +// TODO @Arsh: should this be a flag? Maybe instead of mentioning Let's Encrypt +// directly make it Public ACME server? +const LetsEncryptStagingURL = "https://acme-staging-v02.api.letsencrypt.org/directory" + var _ = framework.ConformanceDescribe("Certificates", func() { runACMEIssuerTests(nil) }) var _ = framework.ConformanceDescribe("Certificates with External Account Binding", func() { + // TODO: make this skippable? Not all ACME issuers support EAB. runACMEIssuerTests(&cmacme.ACMEExternalAccountBinding{ KeyID: "kid-1", }) @@ -70,6 +75,13 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { featureset.IssueCAFeature, ) + // UnsupportedLetsEncryptFeatures are additional ACME features not supported by + // Let's Encrypt + var unsupportedLetsEncryptFeatures = featureset.NewFeatureSet( + featureset.IPAddressFeature, + featureset.Ed25519FeatureSet, + ) + provisionerHTTP01 := &acmeIssuerProvisioner{ eab: eab, } @@ -78,6 +90,11 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { eab: eab, } + // Let's Encrypt does not support EAB + provisionerLEHTTP01 := &acmeIssuerProvisioner{ + eab: nil, + } + (&certificates.Suite{ Name: "ACME HTTP01 Issuer", UseIngressIPAddress: true, @@ -109,6 +126,14 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { DeleteIssuerFunc: provisionerDNS01.delete, UnsupportedFeatures: unsupportedDNS01Features, }).Define() + + (&certificates.Suite{ + Name: "Let's Encrypt HTTP01 Issuer", + UseIngressIPAddress: true, + CreateIssuerFunc: provisionerLEHTTP01.createLetsEncryptStagingHTTP01Issuer, + DeleteIssuerFunc: provisionerLEHTTP01.delete, + UnsupportedFeatures: unsupportedHTTP01Features.Copy().Add(unsupportedLetsEncryptFeatures.List()...), + }).Define() } type acmeIssuerProvisioner struct { @@ -155,6 +180,26 @@ func (a *acmeIssuerProvisioner) createHTTP01Issuer(f *framework.Framework) cmmet } } +func (a *acmeIssuerProvisioner) createLetsEncryptStagingHTTP01Issuer(f *framework.Framework) cmmeta.ObjectReference { + By("Creating a Let's Encrypt Staging HTTP01 Issuer") + + issuer := &cmapi.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "le-issuer-http01-", + }, + Spec: a.createHTTP01IssuerSpec(LetsEncryptStagingURL), + } + + issuer, err := f.CertManagerClientSet.CertmanagerV1().Issuers(f.Namespace.Name).Create(context.TODO(), issuer, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred(), "failed to create Let's Encrypt Staging HTTP01 issuer") + + return cmmeta.ObjectReference{ + Group: cmapi.SchemeGroupVersion.Group, + Kind: cmapi.IssuerKind, + Name: issuer.Name, + } +} + func (a *acmeIssuerProvisioner) createHTTP01ClusterIssuer(f *framework.Framework) cmmeta.ObjectReference { a.ensureEABSecret(f, f.Config.Addons.CertManager.ClusterResourceNamespace) From 2baaea339f83b0be018a05d9cea166d9c80e3afc Mon Sep 17 00:00:00 2001 From: Arsh Sharma Date: Tue, 27 Jul 2021 19:13:19 +0530 Subject: [PATCH 2/5] created a fs for long domain Signed-off-by: Arsh Sharma --- .../framework/helper/featureset/featureset.go | 4 ++ .../conformance/certificates/acme/acme.go | 41 ++++++++++--------- .../suite/conformance/certificates/tests.go | 2 +- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/test/e2e/framework/helper/featureset/featureset.go b/test/e2e/framework/helper/featureset/featureset.go index 00ca1d631..095eef464 100644 --- a/test/e2e/framework/helper/featureset/featureset.go +++ b/test/e2e/framework/helper/featureset/featureset.go @@ -153,4 +153,8 @@ const ( // IssueCAFeature denotes whether the target issuer is able to issue CA // certificates (i.e., certificates for which the CA basicConstraint is true) IssueCAFeature Feature = "IssueCA" + + // LongDomainFeatureSet denotes whether the target issuer is able to sign + // a certificate that defines a long domain + LongDomainFeatureSet Feature = "LongDomain" ) diff --git a/test/e2e/suite/conformance/certificates/acme/acme.go b/test/e2e/suite/conformance/certificates/acme/acme.go index 024471608..ed9a6c328 100644 --- a/test/e2e/suite/conformance/certificates/acme/acme.go +++ b/test/e2e/suite/conformance/certificates/acme/acme.go @@ -19,6 +19,7 @@ package acme import ( "context" "encoding/base64" + "strings" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -34,15 +35,10 @@ import ( "github.com/jetstack/cert-manager/test/e2e/suite/conformance/certificates" ) -// TODO @Arsh: should this be a flag? Maybe instead of mentioning Let's Encrypt -// directly make it Public ACME server? -const LetsEncryptStagingURL = "https://acme-staging-v02.api.letsencrypt.org/directory" - var _ = framework.ConformanceDescribe("Certificates", func() { runACMEIssuerTests(nil) }) var _ = framework.ConformanceDescribe("Certificates with External Account Binding", func() { - // TODO: make this skippable? Not all ACME issuers support EAB. runACMEIssuerTests(&cmacme.ACMEExternalAccountBinding{ KeyID: "kid-1", }) @@ -75,11 +71,12 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { featureset.IssueCAFeature, ) - // UnsupportedLetsEncryptFeatures are additional ACME features not supported by - // Let's Encrypt - var unsupportedLetsEncryptFeatures = featureset.NewFeatureSet( + // UnsupportedPublicACMEServerFeatures are additional ACME features not supported by + // public ACME servers + var unsupportedPublicACMEServerFeatures = featureset.NewFeatureSet( featureset.IPAddressFeature, featureset.Ed25519FeatureSet, + featureset.LongDomainFeatureSet, ) provisionerHTTP01 := &acmeIssuerProvisioner{ @@ -90,8 +87,7 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { eab: eab, } - // Let's Encrypt does not support EAB - provisionerLEHTTP01 := &acmeIssuerProvisioner{ + provisionerPACMEHTTP01 := &acmeIssuerProvisioner{ eab: nil, } @@ -128,11 +124,11 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { }).Define() (&certificates.Suite{ - Name: "Let's Encrypt HTTP01 Issuer", + Name: "Public ACME Server HTTP01 Issuer", UseIngressIPAddress: true, - CreateIssuerFunc: provisionerLEHTTP01.createLetsEncryptStagingHTTP01Issuer, - DeleteIssuerFunc: provisionerLEHTTP01.delete, - UnsupportedFeatures: unsupportedHTTP01Features.Copy().Add(unsupportedLetsEncryptFeatures.List()...), + CreateIssuerFunc: provisionerPACMEHTTP01.createPublicACMEServerStagingHTTP01Issuer, + DeleteIssuerFunc: provisionerPACMEHTTP01.delete, + UnsupportedFeatures: unsupportedHTTP01Features.Copy().Add(unsupportedPublicACMEServerFeatures.List()...), }).Define() } @@ -180,18 +176,25 @@ func (a *acmeIssuerProvisioner) createHTTP01Issuer(f *framework.Framework) cmmet } } -func (a *acmeIssuerProvisioner) createLetsEncryptStagingHTTP01Issuer(f *framework.Framework) cmmeta.ObjectReference { - By("Creating a Let's Encrypt Staging HTTP01 Issuer") +func (a *acmeIssuerProvisioner) createPublicACMEServerStagingHTTP01Issuer(f *framework.Framework) cmmeta.ObjectReference { + By("Creating a Public ACME Server Staging HTTP01 Issuer") + + var PublicACMEServerStagingURL string + if strings.Contains(f.Config.Addons.ACMEServer.URL, "pebble") { + PublicACMEServerStagingURL = "https://acme-staging-v02.api.letsencrypt.org/directory" + } else { + PublicACMEServerStagingURL = f.Config.Addons.ACMEServer.URL + } issuer := &cmapi.Issuer{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: "le-issuer-http01-", + GenerateName: "pacme-issuer-http01-", }, - Spec: a.createHTTP01IssuerSpec(LetsEncryptStagingURL), + Spec: a.createHTTP01IssuerSpec(PublicACMEServerStagingURL), } issuer, err := f.CertManagerClientSet.CertmanagerV1().Issuers(f.Namespace.Name).Create(context.TODO(), issuer, metav1.CreateOptions{}) - Expect(err).NotTo(HaveOccurred(), "failed to create Let's Encrypt Staging HTTP01 issuer") + Expect(err).NotTo(HaveOccurred(), "failed to create Public ACME Server Staging HTTP01 issuer") return cmmeta.ObjectReference{ Group: cmapi.SchemeGroupVersion.Group, diff --git a/test/e2e/suite/conformance/certificates/tests.go b/test/e2e/suite/conformance/certificates/tests.go index c7250818f..dd5f415bf 100644 --- a/test/e2e/suite/conformance/certificates/tests.go +++ b/test/e2e/suite/conformance/certificates/tests.go @@ -795,7 +795,7 @@ func (s *Suite) Define() { By("Sanity-check the issued Certificate") err = f.Helper().ValidateCertificate(f.Namespace.Name, "testcert", validations...) Expect(err).NotTo(HaveOccurred()) - }, featureset.OnlySAN) + }, featureset.OnlySAN, featureset.LongDomainFeatureSet) s.it(f, "should allow updating an existing certificate with a new DNS Name", func(issuerRef cmmeta.ObjectReference) { testCertificate := &cmapi.Certificate{ From 8ce7ca8d63c297e8d65752d5520b34bd27f5a9cc Mon Sep 17 00:00:00 2001 From: Arsh Sharma Date: Fri, 30 Jul 2021 17:00:56 +0530 Subject: [PATCH 3/5] testing: seeing if skip option fixes the failing tests Signed-off-by: Arsh Sharma --- test/e2e/suite/conformance/certificates/acme/acme.go | 1 + test/e2e/suite/conformance/certificates/suite.go | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/test/e2e/suite/conformance/certificates/acme/acme.go b/test/e2e/suite/conformance/certificates/acme/acme.go index ed9a6c328..d6f50d293 100644 --- a/test/e2e/suite/conformance/certificates/acme/acme.go +++ b/test/e2e/suite/conformance/certificates/acme/acme.go @@ -129,6 +129,7 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { CreateIssuerFunc: provisionerPACMEHTTP01.createPublicACMEServerStagingHTTP01Issuer, DeleteIssuerFunc: provisionerPACMEHTTP01.delete, UnsupportedFeatures: unsupportedHTTP01Features.Copy().Add(unsupportedPublicACMEServerFeatures.List()...), + Skip: true, }).Define() } diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index c4e51a745..629f224de 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -68,6 +68,8 @@ type Suite struct { // completed is used internally to track whether Complete() has been called completed bool + + Skip bool } // complete will validate configuration and set default values. @@ -93,6 +95,10 @@ func (s *Suite) complete(f *framework.Framework) { // it is called by the tests to in Define() to setup and run the test func (s *Suite) it(f *framework.Framework, name string, fn func(cmmeta.ObjectReference), requiredFeatures ...featureset.Feature) { + if s.Skip { + fmt.Fprintln(GinkgoWriter, "skipping cause skip set to true") + return + } if !s.checkFeatures(requiredFeatures...) { fmt.Fprintln(GinkgoWriter, "skipping case due to unsupported features") return From 58410f5deb48d42e00d07ca2c41435d85c003c1a Mon Sep 17 00:00:00 2001 From: Arsh Sharma Date: Fri, 30 Jul 2021 18:21:58 +0530 Subject: [PATCH 4/5] added comment for skip Signed-off-by: Arsh Sharma --- test/e2e/suite/conformance/certificates/suite.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index 629f224de..0e93d42a7 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -69,6 +69,7 @@ type Suite struct { // completed is used internally to track whether Complete() has been called completed bool + // Skip if set to true allows skipping the suite. Default value is false. Skip bool } @@ -96,7 +97,7 @@ func (s *Suite) complete(f *framework.Framework) { // it is called by the tests to in Define() to setup and run the test func (s *Suite) it(f *framework.Framework, name string, fn func(cmmeta.ObjectReference), requiredFeatures ...featureset.Feature) { if s.Skip { - fmt.Fprintln(GinkgoWriter, "skipping cause skip set to true") + fmt.Fprintln(GinkgoWriter, "skipping cause suite.Skip was set to true") return } if !s.checkFeatures(requiredFeatures...) { From 89bf0022d6ea890678af48e98b31b95278963a29 Mon Sep 17 00:00:00 2001 From: Arsh Sharma Date: Mon, 2 Aug 2021 16:22:51 +0530 Subject: [PATCH 5/5] changes from pair programming sesh Signed-off-by: Arsh Sharma --- test/e2e/suite/conformance/certificates/acme/acme.go | 1 - test/e2e/suite/conformance/certificates/suite.go | 7 ------- test/e2e/suite/conformance/certificates/tests.go | 8 +++++++- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/test/e2e/suite/conformance/certificates/acme/acme.go b/test/e2e/suite/conformance/certificates/acme/acme.go index d6f50d293..ed9a6c328 100644 --- a/test/e2e/suite/conformance/certificates/acme/acme.go +++ b/test/e2e/suite/conformance/certificates/acme/acme.go @@ -129,7 +129,6 @@ func runACMEIssuerTests(eab *cmacme.ACMEExternalAccountBinding) { CreateIssuerFunc: provisionerPACMEHTTP01.createPublicACMEServerStagingHTTP01Issuer, DeleteIssuerFunc: provisionerPACMEHTTP01.delete, UnsupportedFeatures: unsupportedHTTP01Features.Copy().Add(unsupportedPublicACMEServerFeatures.List()...), - Skip: true, }).Define() } diff --git a/test/e2e/suite/conformance/certificates/suite.go b/test/e2e/suite/conformance/certificates/suite.go index 0e93d42a7..c4e51a745 100644 --- a/test/e2e/suite/conformance/certificates/suite.go +++ b/test/e2e/suite/conformance/certificates/suite.go @@ -68,9 +68,6 @@ type Suite struct { // completed is used internally to track whether Complete() has been called completed bool - - // Skip if set to true allows skipping the suite. Default value is false. - Skip bool } // complete will validate configuration and set default values. @@ -96,10 +93,6 @@ func (s *Suite) complete(f *framework.Framework) { // it is called by the tests to in Define() to setup and run the test func (s *Suite) it(f *framework.Framework, name string, fn func(cmmeta.ObjectReference), requiredFeatures ...featureset.Feature) { - if s.Skip { - fmt.Fprintln(GinkgoWriter, "skipping cause suite.Skip was set to true") - return - } if !s.checkFeatures(requiredFeatures...) { fmt.Fprintln(GinkgoWriter, "skipping case due to unsupported features") return diff --git a/test/e2e/suite/conformance/certificates/tests.go b/test/e2e/suite/conformance/certificates/tests.go index dd5f415bf..6ca4e4ac2 100644 --- a/test/e2e/suite/conformance/certificates/tests.go +++ b/test/e2e/suite/conformance/certificates/tests.go @@ -18,6 +18,7 @@ package certificates import ( "context" + "strings" "time" . "github.com/onsi/ginkgo" @@ -52,10 +53,15 @@ func (s *Suite) Define() { // Wrap this in a BeforeEach else flags will not have been parsed and // f.Config will not be populated at the time that this code is run. BeforeEach(func() { + // Special case Public ACME Servers against being run in the standard + // e2e tests. + if strings.Contains(s.Name, "Public ACME Server") && strings.Contains(f.Config.Addons.ACMEServer.URL, "pebble") { + Skip("Not running public ACME tests against local cluster.") + return + } if s.completed { return } - s.complete(f) if s.UseIngressIPAddress {