From 40947b0ef466c00df8113f57233c426ce52082f9 Mon Sep 17 00:00:00 2001 From: Sathyanarayanan Saravanamuthu Date: Tue, 11 Oct 2022 17:01:26 +0530 Subject: [PATCH 1/2] Generate Certificate Request with predictable name Co-authored-by: Cody W Eilar Signed-off-by: Cody W Eilar Signed-off-by: Sathyanarayanan Saravanamuthu --- internal/controller/feature/features.go | 7 +++ .../requestmanager_controller.go | 13 +++++- .../requestmanager_controller_test.go | 46 +++++++++++++++++++ test/unit/gen/certificaterequest.go | 6 +++ 4 files changed, 70 insertions(+), 2 deletions(-) diff --git a/internal/controller/feature/features.go b/internal/controller/feature/features.go index 6ceaa45d2..db4e65a68 100644 --- a/internal/controller/feature/features.go +++ b/internal/controller/feature/features.go @@ -58,6 +58,12 @@ const ( // This feature gate must be used together with LiteralCertificateSubject webhook feature gate. // See https://github.com/cert-manager/cert-manager/issues/3203 and https://github.com/cert-manager/cert-manager/issues/4424 for context. LiteralCertificateSubject featuregate.Feature = "LiteralCertificateSubject" + + // Alpha: v1.10 + // StableCertificateRequestName will enable generation of CertificateRequest resources with a fixed name. The name of the CertificateRequest will be a function of Certificate resource name and its revision + // This feature gate will disable auto-generated CertificateRequest name + // Github Issue: https://github.com/cert-manager/cert-manager/issues/4956 + StableCertificateRequestName featuregate.Feature = "StableCertificateRequestName" ) func init() { @@ -74,4 +80,5 @@ var defaultCertManagerFeatureGates = map[featuregate.Feature]featuregate.Feature AdditionalCertificateOutputFormats: {Default: false, PreRelease: featuregate.Alpha}, ServerSideApply: {Default: false, PreRelease: featuregate.Alpha}, LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha}, + StableCertificateRequestName: {Default: false, PreRelease: featuregate.Alpha}, } diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index 41770ceb4..fa25effe2 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -38,6 +38,7 @@ import ( "k8s.io/client-go/util/workqueue" "k8s.io/utils/clock" + "github.com/cert-manager/cert-manager/internal/controller/feature" apiutil "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -47,6 +48,7 @@ import ( controllerpkg "github.com/cert-manager/cert-manager/pkg/controller" "github.com/cert-manager/cert-manager/pkg/controller/certificates" logf "github.com/cert-manager/cert-manager/pkg/logs" + utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" "github.com/cert-manager/cert-manager/pkg/util/pki" "github.com/cert-manager/cert-manager/pkg/util/predicate" ) @@ -395,6 +397,11 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi }, } + if utilfeature.DefaultFeatureGate.Enabled(feature.StableCertificateRequestName) { + cr.ObjectMeta.GenerateName = "" + cr.ObjectMeta.Name = apiutil.DNSSafeShortenTo52Characters(crt.Name) + "-" + fmt.Sprintf("%d", nextRevision) + } + cr, err = c.client.CertmanagerV1().CertificateRequests(cr.Namespace).Create(ctx, cr, metav1.CreateOptions{FieldManager: c.fieldManager}) if err != nil { c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonRequestFailed, "Failed to create CertificateRequest: "+err.Error()) @@ -402,8 +409,10 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi } c.recorder.Eventf(crt, corev1.EventTypeNormal, reasonRequested, "Created new CertificateRequest resource %q", cr.Name) - if err := c.waitForCertificateRequestToExist(cr.Namespace, cr.Name); err != nil { - return fmt.Errorf("failed whilst waiting for CertificateRequest to exist - this may indicate an apiserver running slowly. Request will be retried. %w", err) + if !utilfeature.DefaultFeatureGate.Enabled(feature.StableCertificateRequestName) { + if err := c.waitForCertificateRequestToExist(cr.Namespace, cr.Name); err != nil { + return fmt.Errorf("failed whilst waiting for CertificateRequest to exist - this may indicate an apiserver running slowly. Request will be retried. %w", err) + } } return nil } diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go index 1c2e126e8..2d50b839e 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go @@ -28,12 +28,16 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" coretesting "k8s.io/client-go/testing" + "k8s.io/component-base/featuregate" + featuregatetesting "k8s.io/component-base/featuregate/testing" fakeclock "k8s.io/utils/clock/testing" + "github.com/cert-manager/cert-manager/internal/controller/feature" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" controllerpkg "github.com/cert-manager/cert-manager/pkg/controller" testpkg "github.com/cert-manager/cert-manager/pkg/controller/test" + utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" "github.com/cert-manager/cert-manager/pkg/util/pki" "github.com/cert-manager/cert-manager/test/unit/gen" ) @@ -78,6 +82,14 @@ func TestProcessItem(t *testing.T) { }, Spec: cmapi.CertificateSpec{CommonName: "test-bundle-2"}}, ) + bundle3 := mustCreateCryptoBundle(t, &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "testns", + Name: "test", + UID: "test", + }, + Spec: cmapi.CertificateSpec{CommonName: "test-bundle-3"}}, + ) fixedNow := metav1.NewTime(time.Now()) fixedClock := fakeclock.NewFakeClock(fixedNow.Time) failedCRConditionPreviousIssuance := cmapi.CertificateRequestCondition{ @@ -98,6 +110,9 @@ func TestProcessItem(t *testing.T) { // if neither is set, the key will be "" key string + // Featuregates to set for a particular test. + featuresToEnable []featuregate.Feature + // Certificate to be synced for the test. // if not set, the 'key' will be passed to ProcessItem instead. certificate *cmapi.Certificate @@ -185,6 +200,31 @@ func TestProcessItem(t *testing.T) { )), relaxedCertificateRequestMatcher), }, }, + "create a CertificateRequest if none exists and StableCertificateRequestName enabled": { + featuresToEnable: []featuregate.Feature{feature.StableCertificateRequestName}, + secrets: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: bundle3.certificate.Namespace, Name: "exists"}, + Data: map[string][]byte{corev1.TLSPrivateKeyKey: bundle3.privateKeyBytes}, + }, + }, + certificate: gen.CertificateFrom(bundle3.certificate, + gen.SetCertificateNextPrivateKeySecretName("exists"), + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}), + ), + expectedEvents: []string{`Normal Requested Created new CertificateRequest resource "test-1"`}, + expectedActions: []testpkg.Action{ + testpkg.NewCustomMatch(coretesting.NewCreateAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", + gen.CertificateRequestFrom(bundle3.certificateRequest, + gen.SetCertificateRequestName("test-1"), + gen.SetCertificateRequestGenerateName(""), + gen.SetCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", + cmapi.CertificateRequestRevisionAnnotationKey: "1", + }), + )), relaxedCertificateRequestMatcher), + }, + }, "delete the owned CertificateRequest and create a new one if existing one does not have the annotation": { secrets: []runtime.Object{ &corev1.Secret{ @@ -614,6 +654,12 @@ func TestProcessItem(t *testing.T) { if err != nil { t.Fatal(err) } + + // Enable any features for a particular test + for _, feature := range test.featuresToEnable { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, feature, true)() + } + // Start the informers and begin processing updates builder.Start() defer builder.Stop() diff --git a/test/unit/gen/certificaterequest.go b/test/unit/gen/certificaterequest.go index 5697aebc1..5d417e48c 100644 --- a/test/unit/gen/certificaterequest.go +++ b/test/unit/gen/certificaterequest.go @@ -114,6 +114,12 @@ func SetCertificateRequestName(name string) CertificateRequestModifier { } } +func SetCertificateRequestGenerateName(generateName string) CertificateRequestModifier { + return func(cr *v1.CertificateRequest) { + cr.ObjectMeta.GenerateName = generateName + } +} + func SetCertificateRequestKeyUsages(usages ...v1.KeyUsage) CertificateRequestModifier { return func(cr *v1.CertificateRequest) { cr.Spec.Usages = usages From 2969202fe2c5af78d3bb5824cf5ad18c14d559c8 Mon Sep 17 00:00:00 2001 From: Sathyanarayanan Saravanamuthu Date: Tue, 11 Oct 2022 17:22:38 +0530 Subject: [PATCH 2/2] Addressing review comments Co-authored-by: Cody W Eilar Signed-off-by: Cody W Eilar Signed-off-by: Sathyanarayanan Saravanamuthu --- .../requestmanager/requestmanager_controller.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index fa25effe2..d367caade 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -409,10 +409,16 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi } c.recorder.Eventf(crt, corev1.EventTypeNormal, reasonRequested, "Created new CertificateRequest resource %q", cr.Name) - if !utilfeature.DefaultFeatureGate.Enabled(feature.StableCertificateRequestName) { - if err := c.waitForCertificateRequestToExist(cr.Namespace, cr.Name); err != nil { - return fmt.Errorf("failed whilst waiting for CertificateRequest to exist - this may indicate an apiserver running slowly. Request will be retried. %w", err) - } + + // If the StableCertificateRequestName feature gate is enabled, skip waiting for our informer cache/lister to + // observe the creation event and instead rely on an AlreadyExists error being returned if we do attempt a + // CREATE for the same CertificateRequest name again early. + if utilfeature.DefaultFeatureGate.Enabled(feature.StableCertificateRequestName) { + return nil + } + + if err := c.waitForCertificateRequestToExist(cr.Namespace, cr.Name); err != nil { + return fmt.Errorf("failed whilst waiting for CertificateRequest to exist - this may indicate an apiserver running slowly. Request will be retried. %w", err) } return nil }