BUGFIX: CertificateRequest short names must be unique.

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

pkg/api/util/names.go

@@ -20,7 +20,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"hash/fnv"
-
 	"regexp"
 )
 
@@ -47,6 +46,30 @@ func ComputeName(prefix string, obj interface{}) (string, error) {
 	return fmt.Sprintf("%s-%d", prefix, hashF.Sum32()), nil
 }
 
+// ComputeUniqueDeterministicNameFromData returns a short unique name for the given object.
+func ComputeUniqueDeterministicNameFromData(fullName string) (string, error) {
+	const maxNameLength = 61 // 52 + 1 + 8
+
+	if len(fullName) <= maxNameLength {
+		return fullName, nil
+	}
+
+	hashF := fnv.New32()
+
+	_, err := hashF.Write([]byte(fullName))
+	if err != nil {
+		return "", err
+	}
+
+	prefix := DNSSafeShortenTo52Characters(fullName)
+
+	if len(prefix) == 0 {
+		return fmt.Sprintf("%08x", hashF.Sum32()), nil
+	}
+
+	return fmt.Sprintf("%s-%08x", prefix, hashF.Sum32()), nil
+}
+
 // DNSSafeShortenTo52Characters shortens the input string to 52 chars and ensures the last char is an alpha-numeric character.
 func DNSSafeShortenTo52Characters(in string) string {
 	if len(in) >= 52 {

pkg/api/util/names_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"fmt"
 	"testing"
 
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -111,3 +112,46 @@ func TestComputeName(t *testing.T) {
 		})
 	}
 }
+
+func TestComputeUniqueDeterministicNameFromData(t *testing.T) {
+	type testcase struct {
+		in     string
+		expOut string
+	}
+
+	tests := []testcase{
+		{
+			in:     "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-014e7a7f",
+		},
+		{
+			in:     "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+		},
+		{
+			in:     "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.",
+			expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.",
+		},
+		{
+			in:     "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaa",
+			expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaa",
+		},
+		{
+			in:     "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaa",
+			expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-72314ca8",
+		},
+	}
+
+	for i, test := range tests {
+		test := test
+		t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) {
+			out, err := ComputeUniqueDeterministicNameFromData(test.in)
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+			if out != test.expOut {
+				t.Errorf("expected %q, got %q", test.expOut, out)
+			}
+		})
+	}
+}

pkg/controller/certificates/requestmanager/requestmanager_controller.go

@@ -394,7 +394,16 @@ 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)
+
+		// limit the name to 54 chars to leave room for 8 characters that represent the revision
+		// number and a hyphen. This way, we stay within the 63 character limit for pod names (which is not
+		// a hard limit since we are working with CertificateRequest resources).
+		crName, err := apiutil.ComputeUniqueDeterministicNameFromData(crt.Name)
+		if err != nil {
+			return err
+		}
+
+		cr.ObjectMeta.Name = fmt.Sprintf("%s-%d", crName, nextRevision)
 	}
 
 	cr, err = c.client.CertmanagerV1().CertificateRequests(cr.Namespace).Create(ctx, cr, metav1.CreateOptions{FieldManager: c.fieldManager})

pkg/controller/certificates/requestmanager/requestmanager_controller_test.go

@@ -90,6 +90,14 @@ func TestProcessItem(t *testing.T) {
 		},
 		Spec: cmapi.CertificateSpec{CommonName: "test-bundle-3"}},
 	)
+	bundle4 := mustCreateCryptoBundle(t, &cmapi.Certificate{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "testns",
+			Name:      "long-name-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabcccccccccccc",
+			UID:       "test",
+		},
+		Spec: cmapi.CertificateSpec{CommonName: "test-bundle-4"}},
+	)
 	fixedNow := metav1.NewTime(time.Now())
 	fixedClock := fakeclock.NewFakeClock(fixedNow.Time)
 	failedCRConditionPreviousIssuance := cmapi.CertificateRequestCondition{
@@ -228,6 +236,54 @@ func TestProcessItem(t *testing.T) {
 					)), relaxedCertificateRequestMatcher),
 			},
 		},
+		"create a CertificateRequest if none exists (with long name)": {
+			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(bundle4.certificate,
+				gen.SetCertificateNextPrivateKeySecretName("exists"),
+				gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}),
+				gen.SetCertificateRevision(19),
+			),
+			expectedEvents: []string{`Normal Requested Created new CertificateRequest resource "long-name-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab-6c93132b-20"`},
+			expectedActions: []testpkg.Action{
+				testpkg.NewCustomMatch(coretesting.NewCreateAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns",
+					gen.CertificateRequestFrom(bundle4.certificateRequest,
+						gen.SetCertificateRequestName("long-name-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab-6c93132b-20"),
+						gen.SetCertificateRequestAnnotations(map[string]string{
+							cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists",
+							cmapi.CertificateRequestRevisionAnnotationKey:   "20",
+						}),
+					)), relaxedCertificateRequestMatcher),
+			},
+		},
+		"create a CertificateRequest if none exists (with long name and very large revision)": {
+			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(bundle4.certificate,
+				gen.SetCertificateNextPrivateKeySecretName("exists"),
+				gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}),
+				gen.SetCertificateRevision(999999999),
+			),
+			expectedEvents: []string{`Normal Requested Created new CertificateRequest resource "long-name-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab-6c93132b-1000000000"`},
+			expectedActions: []testpkg.Action{
+				testpkg.NewCustomMatch(coretesting.NewCreateAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns",
+					gen.CertificateRequestFrom(bundle4.certificateRequest,
+						gen.SetCertificateRequestName("long-name-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab-6c93132b-1000000000"),
+						gen.SetCertificateRequestAnnotations(map[string]string{
+							cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists",
+							cmapi.CertificateRequestRevisionAnnotationKey:   "1000000000",
+						}),
+					)), relaxedCertificateRequestMatcher),
+			},
+		},
 		"delete the owned CertificateRequest and create a new one if existing one does not have the annotation": {
 			secrets: []runtime.Object{
 				&corev1.Secret{