Fixed invalid object names generated for long package names (#4471)

* Fixed invalid object names generated for long package names

* TrimDNS1123Label would trim strings but end up creating invalid DNS1123
strings.
* FormatOperatorNameDNS1123 would return strings that were invalid if
  they begin or end with non-alphanumeric or hyphens.

* Handle capitals and use more efficient trim mechanism

* React to new expectation.

Fixes #4470

Signed-off-by: jesus m. rodriguez <[email protected]>

Author: jmrodri

changelog/fragments/fix-trim-format-dns1123-functions.yaml

@@ -0,0 +1,8 @@
+entries:
+  - description: >
+      Fixed invalid object names generated for long package names passed
+      to `run packagemanifests` & `run bundle`.
+
+    kind: "bugfix"
+
+    breaking: false

internal/olm/operator/registry/configmap/deployment_test.go

@@ -28,9 +28,9 @@ var _ = Describe("Deployment", func() {
 
 	Describe("getRegistryServerName", func() {
 		It("should return the formatted servername", func() {
-			Expect(getRegistryServerName("pkgName")).Should(Equal("pkgName-registry-server"))
+			Expect(getRegistryServerName("pkgName")).Should(Equal("pkgname-registry-server"))
 			// This checks if all the special characters are handled correctly
-			Expect(getRegistryServerName("$abc.foo$@(&#(&!*)@&#")).Should(Equal("-abc-foo--registry-server"))
+			Expect(getRegistryServerName("$abc.foo$@(&#(&!*)@&#")).Should(Equal("abc-foo-registry-server"))
 		})
 	})
 
@@ -39,7 +39,7 @@ var _ = Describe("Deployment", func() {
 			labels := map[string]string{
 				"owner":        "operator-sdk",
 				"package-name": "$abc.foo$@(&#(&!*)@&#",
-				"server-name":  "-abc-foo--registry-server",
+				"server-name":  "abc-foo-registry-server",
 			}
 
 			Expect(getRegistryDeploymentLabels("$abc.foo$@(&#(&!*)@&#")).Should(Equal(labels))

internal/util/k8sutil/k8sutil.go

@@ -93,7 +93,11 @@ var dns1123LabelRegexp = regexp.MustCompile("[^a-zA-Z0-9]+")
 // replacing all non-compliant UTF-8 characters with "-".
 func FormatOperatorNameDNS1123(name string) string {
 	if len(validation.IsDNS1123Label(name)) != 0 {
-		return dns1123LabelRegexp.ReplaceAllString(name, "-")
+		// Use - for any of the non-matching characters
+		n := dns1123LabelRegexp.ReplaceAllString(name, "-")
+
+		// Now let's remove any leading or trailing -
+		return strings.ToLower(strings.Trim(n, "-"))
 	}
 	return name
 }
@@ -102,7 +106,7 @@ func FormatOperatorNameDNS1123(name string) string {
 // by removing characters from the beginning of label such that len(label) <= 63.
 func TrimDNS1123Label(label string) string {
 	if len(label) > validation.DNS1123LabelMaxLength {
-		return label[len(label)-validation.DNS1123LabelMaxLength:]
+		return strings.Trim(label[len(label)-validation.DNS1123LabelMaxLength:], "-")
 	}
 	return label
 }

internal/util/k8sutil/k8sutil_test.go

@@ -240,3 +240,95 @@ func TestSupportsOwnerReference(t *testing.T) {
 		})
 	}
 }
+
+func TestTrimDNS1123Label(t *testing.T) {
+	type testcase struct {
+		name     string
+		label    string
+		expected string
+	}
+	testcases := []testcase{
+		{
+			name:     "return valid truncated values",
+			label:    "quay-io-raffaelespazzoli-proactive-node-scaling-operator-bundle-latest",
+			expected: "raffaelespazzoli-proactive-node-scaling-operator-bundle-latest",
+		},
+		{
+			name:     "valid labels with proper length are noops",
+			label:    "raffaelespazzoli-proactive-node-scaling-operator-bundle-latest",
+			expected: "raffaelespazzoli-proactive-node-scaling-operator-bundle-latest",
+		},
+		{
+			name:     "short invalid labels are left alone",
+			label:    "-$*@*#fixed-invalid(__$)@+==-name-#$($",
+			expected: "-$*@*#fixed-invalid(__$)@+==-name-#$($",
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := TrimDNS1123Label(tc.label)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+func TestFormatOperatorNameDNS1123(t *testing.T) {
+	type testcase struct {
+		name     string
+		label    string
+		expected string
+	}
+	testcases := []testcase{
+		{
+			name:     "should not start with -",
+			label:    "-doesnot-start-with-hyphen",
+			expected: "doesnot-start-with-hyphen",
+		},
+		{
+			name:     "should not start with non-alphanumeric",
+			label:    "$@*#(@does-notstart-garbage",
+			expected: "does-notstart-garbage",
+		},
+		{
+			name:     "should not have non-alphanumeric",
+			label:    "sample-1234$@*#(@does-notstart-garbage",
+			expected: "sample-1234-does-notstart-garbage",
+		},
+		{
+			name:     "should not end with non-alphanumeric",
+			label:    "sample-1234-does-notstart-garbage#$*@#*($_!-_@(",
+			expected: "sample-1234-does-notstart-garbage",
+		},
+		{
+			name:     "should not start or end with hyphen",
+			label:    "-does-not-start-or-end-with-hyphen---",
+			expected: "does-not-start-or-end-with-hyphen",
+		},
+		{
+			name:     "empty string is a noop",
+			label:    "",
+			expected: "",
+		},
+		{
+			name:     "string of invalid characters results in empty string",
+			label:    "@#@#)$*!!_$#*$*!@",
+			expected: "",
+		},
+		{
+			name:     "valid long names are not trimmed",
+			label:    "quay-io-raffaelespazzoli-proactive-node-scaling-operator-bundle-latest",
+			expected: "quay-io-raffaelespazzoli-proactive-node-scaling-operator-bundle-latest",
+		},
+		{
+			name:     "should not contain capital letters",
+			label:    "QUAY-IO-gobble-gobBLE",
+			expected: "quay-io-gobble-gobble",
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := FormatOperatorNameDNS1123(tc.label)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}