From 3211d2e81dd3465cbfa922cea78bb6a34aab05ee Mon Sep 17 00:00:00 2001 From: "jesus m. rodriguez" Date: Wed, 3 Feb 2021 11:43:16 -0500 Subject: [PATCH 1/3] 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. Fixes #4470 Signed-off-by: jesus m. rodriguez --- .../fix-trim-format-dns1123-functions.yaml | 8 ++ .../registry/configmap/deployment_test.go | 4 +- internal/util/k8sutil/k8sutil.go | 12 ++- internal/util/k8sutil/k8sutil_test.go | 87 +++++++++++++++++++ 4 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 changelog/fragments/fix-trim-format-dns1123-functions.yaml diff --git a/changelog/fragments/fix-trim-format-dns1123-functions.yaml b/changelog/fragments/fix-trim-format-dns1123-functions.yaml new file mode 100644 index 00000000000..2c2896ea2a4 --- /dev/null +++ b/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 diff --git a/internal/olm/operator/registry/configmap/deployment_test.go b/internal/olm/operator/registry/configmap/deployment_test.go index 5b73cdf9760..63572a517d4 100644 --- a/internal/olm/operator/registry/configmap/deployment_test.go +++ b/internal/olm/operator/registry/configmap/deployment_test.go @@ -30,7 +30,7 @@ var _ = Describe("Deployment", func() { It("should return the formatted servername", func() { 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)) diff --git a/internal/util/k8sutil/k8sutil.go b/internal/util/k8sutil/k8sutil.go index 9fe23a6c865..cbe352e129d 100644 --- a/internal/util/k8sutil/k8sutil.go +++ b/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.Trim(n, "-") } return name } @@ -102,7 +106,11 @@ 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:] + lbl := label[len(label)-validation.DNS1123LabelMaxLength:] + if len(validation.IsDNS1123Label(lbl)) != 0 { + return FormatOperatorNameDNS1123(lbl) + } + return lbl } return label } diff --git a/internal/util/k8sutil/k8sutil_test.go b/internal/util/k8sutil/k8sutil_test.go index 9fcdcbaadc6..03441c548ab 100644 --- a/internal/util/k8sutil/k8sutil_test.go +++ b/internal/util/k8sutil/k8sutil_test.go @@ -240,3 +240,90 @@ 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", + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + result := FormatOperatorNameDNS1123(tc.label) + assert.Equal(t, tc.expected, result) + }) + } +} From 1a0bec5f2cfaf654f068be7add5156ad85fe3534 Mon Sep 17 00:00:00 2001 From: "jesus m. rodriguez" Date: Wed, 3 Feb 2021 13:54:09 -0500 Subject: [PATCH 2/3] Handle capitals and use more efficient trim mechanism Signed-off-by: jesus m. rodriguez --- internal/util/k8sutil/k8sutil.go | 8 ++------ internal/util/k8sutil/k8sutil_test.go | 5 +++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/util/k8sutil/k8sutil.go b/internal/util/k8sutil/k8sutil.go index cbe352e129d..297fffa80a2 100644 --- a/internal/util/k8sutil/k8sutil.go +++ b/internal/util/k8sutil/k8sutil.go @@ -97,7 +97,7 @@ func FormatOperatorNameDNS1123(name string) string { n := dns1123LabelRegexp.ReplaceAllString(name, "-") // Now let's remove any leading or trailing - - return strings.Trim(n, "-") + return strings.ToLower(strings.Trim(n, "-")) } return name } @@ -106,11 +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 { - lbl := label[len(label)-validation.DNS1123LabelMaxLength:] - if len(validation.IsDNS1123Label(lbl)) != 0 { - return FormatOperatorNameDNS1123(lbl) - } - return lbl + return strings.Trim(label[len(label)-validation.DNS1123LabelMaxLength:], "-") } return label } diff --git a/internal/util/k8sutil/k8sutil_test.go b/internal/util/k8sutil/k8sutil_test.go index 03441c548ab..21b6690885d 100644 --- a/internal/util/k8sutil/k8sutil_test.go +++ b/internal/util/k8sutil/k8sutil_test.go @@ -319,6 +319,11 @@ func TestFormatOperatorNameDNS1123(t *testing.T) { 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) { From 9abbec8f6ca2ecddf0026e0dca80044ea740d34f Mon Sep 17 00:00:00 2001 From: "jesus m. rodriguez" Date: Wed, 3 Feb 2021 18:39:32 -0500 Subject: [PATCH 3/3] React to new expectation. Signed-off-by: jesus m. rodriguez --- internal/olm/operator/registry/configmap/deployment_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/olm/operator/registry/configmap/deployment_test.go b/internal/olm/operator/registry/configmap/deployment_test.go index 63572a517d4..a97e1a47428 100644 --- a/internal/olm/operator/registry/configmap/deployment_test.go +++ b/internal/olm/operator/registry/configmap/deployment_test.go @@ -28,7 +28,7 @@ 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")) })