Merge pull request #5487 from sathyanarays/fix_cr_duplicates

Generate Certificate Request with predictable name

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},
 }

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,6 +409,14 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi
 	}
 
 	c.recorder.Eventf(crt, corev1.EventTypeNormal, reasonRequested, "Created new CertificateRequest resource %q", cr.Name)
+
+	// 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)
 	}

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()

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