Merge pull request #2516 from meyskens/computed-name-non-char

Fix issue in ComputeCertificateRequestName when 52nd is a dot

pkg/api/util/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -33,3 +33,14 @@ filegroup(
     tags = ["automanaged"],
     visibility = ["//visibility:public"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["names_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//pkg/apis/certmanager/v1alpha2:go_default_library",
+        "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library",
+        "@io_k8s_apimachinery//pkg/util/validation:go_default_library",
+    ],
+)

pkg/api/util/names.go

@@ -21,11 +21,12 @@ import (
 	"fmt"
 	"hash/fnv"
 
+	"regexp"
+
 	cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2"
 )
 
 func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) {
-	crt = crt.DeepCopy()
 	specBytes, err := json.Marshal(crt.Spec)
 	if err != nil {
 		return "", err
@@ -37,7 +38,14 @@ func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) {
 		return "", err
 	}
 
-	// shorten the cert name to 52 chars to ensure the total length of the name
-	// is less than or equal to 64 characters
-	return fmt.Sprintf("%.52s-%d", crt.Name, hashF.Sum32()), nil
+	crtName := crt.Name
+	if len(crtName) >= 52 {
+		// shorten the cert name to 52 chars to ensure the total length of the name
+		// also shorten the 52 char string to the last non-symbol character
+		// is less than or equal to 64 characters
+		validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crtName), -1)
+		crtName = crtName[:validCharIndexes[len(validCharIndexes)-1][1]]
+	}
+
+	return fmt.Sprintf("%s-%d", crtName, hashF.Sum32()), nil
 }

pkg/api/util/names_test.go

@@ -0,0 +1,113 @@
+/*
+Copyright 2019 The Jetstack cert-manager contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"testing"
+
+	cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
+)
+
+func TestComputeCertificateRequestName(t *testing.T) {
+	type args struct {
+		crt *cmapi.Certificate
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    string
+		wantErr bool
+	}{
+		{
+			name: "Name generation short domains",
+			args: args{
+				crt: &cmapi.Certificate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "unit.test.jetstack.io",
+					},
+					Spec: cmapi.CertificateSpec{
+						CommonName: "unit.test.jetstack.io",
+					},
+				},
+			},
+			wantErr: false,
+			want:    "unit.test.jetstack.io-1683025094",
+		},
+		{
+			name: "Name generation too long domains",
+			args: args{
+				crt: &cmapi.Certificate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab.jetstack.io",
+					},
+					Spec: cmapi.CertificateSpec{
+						CommonName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab.jetstack.io",
+					},
+				},
+			},
+			wantErr: false,
+			want:    "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-108802726",
+		},
+		{
+			name: "Name generation for dot as 52nd char",
+			args: args{
+				crt: &cmapi.Certificate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io",
+					},
+					Spec: cmapi.CertificateSpec{
+						CommonName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io",
+					},
+				},
+			},
+			wantErr: false,
+			want:    "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-225297437",
+		},
+		{
+			name: "Name generation for dot as 54td char",
+			args: args{
+				crt: &cmapi.Certificate{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io",
+					},
+					Spec: cmapi.CertificateSpec{
+						CommonName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io",
+					},
+				},
+			},
+			wantErr: false,
+			want:    "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-1448584771",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ComputeCertificateRequestName(tt.args.crt)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ComputeCertificateRequestName() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("ComputeCertificateRequestName() = %v, want %v", got, tt.want)
+			}
+			if len(validation.IsQualifiedName(got)) != 0 {
+				t.Errorf("ComputeCertificateRequestName() = %v is not DNS-1123 valid", got)
+			}
+		})
+	}
+}