[RHIDP-2246] Fix reconciliation issue when referenced ConfigMap/Secre…

…t has a dot (`.`) in its name (#363)

* Add tests highlighting the issue

* Convert volume names to valid K8s resource names

Using https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ as reference.

* Address review comments

Co-authored-by: Gennady Azarenkov <[email protected]>

---------

Co-authored-by: Gennady Azarenkov <[email protected]>

integration_tests/cr-config_test.go

@@ -58,12 +58,15 @@ var _ = When("create backstage with CR configured", func() {
 
 		appConfig1 := generateConfigMap(ctx, k8sClient, "app-config1", ns, map[string]string{"key11": "app:", "key12": "app:"}, nil, nil)
 		appConfig2 := generateConfigMap(ctx, k8sClient, "app-config2", ns, map[string]string{"key21": "app:", "key22": "app:"}, nil, nil)
+		appConfig3 := generateConfigMap(ctx, k8sClient, "app-config3.dot", ns, map[string]string{"key.31": "app31:"}, nil, nil)
 
 		cmFile1 := generateConfigMap(ctx, k8sClient, "cm-file1", ns, map[string]string{"cm11": "11", "cm12": "12"}, nil, nil)
 		cmFile2 := generateConfigMap(ctx, k8sClient, "cm-file2", ns, map[string]string{"cm21": "21", "cm22": "22"}, nil, nil)
+		cmFile3 := generateConfigMap(ctx, k8sClient, "cm-file3.dot", ns, map[string]string{"cm.31": "31"}, nil, nil)
 
 		secretFile1 := generateSecret(ctx, k8sClient, "secret-file1", ns, map[string]string{"sec11": "val11", "sec12": "val12"}, nil, nil)
 		secretFile2 := generateSecret(ctx, k8sClient, "secret-file2", ns, map[string]string{"sec21": "val21", "sec22": "val22"}, nil, nil)
+		secretFile3 := generateSecret(ctx, k8sClient, "secret-file3.dot", ns, map[string]string{"sec.31": "val31", "sec.32": "val22"}, nil, nil)
 
 		cmEnv1 := generateConfigMap(ctx, k8sClient, "cm-env1", ns, map[string]string{"cm11": "11", "cm12": "12"}, nil, nil)
 		cmEnv2 := generateConfigMap(ctx, k8sClient, "cm-env2", ns, map[string]string{"cm21": "21", "cm22": "22"}, nil, nil)
@@ -78,17 +81,20 @@ var _ = When("create backstage with CR configured", func() {
 					ConfigMaps: []bsv1alpha1.ObjectKeyRef{
 						{Name: appConfig1},
 						{Name: appConfig2, Key: "key21"},
+						{Name: appConfig3},
 					},
 				},
 				ExtraFiles: &bsv1alpha1.ExtraFiles{
 					MountPath: "/my/file/path",
 					ConfigMaps: []bsv1alpha1.ObjectKeyRef{
 						{Name: cmFile1},
 						{Name: cmFile2, Key: "cm21"},
+						{Name: cmFile3},
 					},
 					Secrets: []bsv1alpha1.ObjectKeyRef{
 						{Name: secretFile1, Key: "sec11"},
 						{Name: secretFile2, Key: "sec21"},
+						{Name: secretFile3, Key: "sec.31"},
 					},
 				},
 				ExtraEnvs: &bsv1alpha1.ExtraEnvs{
@@ -118,38 +124,45 @@ var _ = When("create backstage with CR configured", func() {
 			By("checking if app-config volumes are added to PodSpec")
 			g.Expect(utils.GenerateVolumeNameFromCmOrSecret(appConfig1)).To(BeAddedAsVolumeToPodSpec(podSpec))
 			g.Expect(utils.GenerateVolumeNameFromCmOrSecret(appConfig2)).To(BeAddedAsVolumeToPodSpec(podSpec))
+			g.Expect(utils.GenerateVolumeNameFromCmOrSecret(appConfig3)).To(BeAddedAsVolumeToPodSpec(podSpec))
 
 			By("checking if app-config volumes are mounted to the Backstage container")
 			g.Expect("/my/mount/path/key11").To(BeMountedToContainer(c))
 			g.Expect("/my/mount/path/key12").To(BeMountedToContainer(c))
 			g.Expect("/my/mount/path/key21").To(BeMountedToContainer(c))
 			g.Expect("/my/mount/path/key22").NotTo(BeMountedToContainer(c))
+			g.Expect("/my/mount/path/key.31").To(BeMountedToContainer(c))
 
 			By("checking if app-config args are added to the Backstage container")
 			g.Expect("/my/mount/path/key11").To(BeAddedAsArgToContainer(c))
 			g.Expect("/my/mount/path/key12").To(BeAddedAsArgToContainer(c))
 			g.Expect("/my/mount/path/key21").To(BeAddedAsArgToContainer(c))
 			g.Expect("/my/mount/path/key22").NotTo(BeAddedAsArgToContainer(c))
+			g.Expect("/my/mount/path/key.31").To(BeAddedAsArgToContainer(c))
 
 			By("checking if extra-cm-file volumes are added to PodSpec")
 			g.Expect(utils.GenerateVolumeNameFromCmOrSecret(cmFile1)).To(BeAddedAsVolumeToPodSpec(podSpec))
 			g.Expect(utils.GenerateVolumeNameFromCmOrSecret(cmFile2)).To(BeAddedAsVolumeToPodSpec(podSpec))
+			g.Expect(utils.GenerateVolumeNameFromCmOrSecret(cmFile3)).To(BeAddedAsVolumeToPodSpec(podSpec))
 
 			By("checking if extra-cm-file volumes are mounted to the Backstage container")
 			g.Expect("/my/file/path/cm11").To(BeMountedToContainer(c))
 			g.Expect("/my/file/path/cm12").To(BeMountedToContainer(c))
 			g.Expect("/my/file/path/cm21").To(BeMountedToContainer(c))
 			g.Expect("/my/file/path/cm22").NotTo(BeMountedToContainer(c))
+			g.Expect("/my/file/path/cm.31").To(BeMountedToContainer(c))
 
 			By("checking if extra-secret-file volumes are added to PodSpec")
 			g.Expect(utils.GenerateVolumeNameFromCmOrSecret("secret-file1")).To(BeAddedAsVolumeToPodSpec(podSpec))
 			g.Expect(utils.GenerateVolumeNameFromCmOrSecret("secret-file2")).To(BeAddedAsVolumeToPodSpec(podSpec))
+			g.Expect(utils.GenerateVolumeNameFromCmOrSecret("secret-file3.dot")).To(BeAddedAsVolumeToPodSpec(podSpec))
 
 			By("checking if extra-secret-file volumes are mounted to the Backstage container")
 			g.Expect("/my/file/path/sec11").To(BeMountedToContainer(c))
 			g.Expect("/my/file/path/sec12").NotTo(BeMountedToContainer(c))
 			g.Expect("/my/file/path/sec21").To(BeMountedToContainer(c))
 			g.Expect("/my/file/path/sec22").NotTo(BeMountedToContainer(c))
+			g.Expect("/my/file/path/sec.31").To(BeMountedToContainer(c))
 
 			By("checking if extra-envvars are injected to the Backstage container as EnvFrom")
 			g.Expect("cm-env1").To(BeEnvFromForContainer(c))

pkg/model/secretfiles_test.go

@@ -16,15 +16,14 @@ package model
 
 import (
 	"context"
+	"testing"
 
 	"redhat-developer/red-hat-developer-hub-operator/pkg/utils"
 
 	bsv1alpha1 "redhat-developer/red-hat-developer-hub-operator/api/v1alpha1"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	"testing"
-
 	"github.com/stretchr/testify/assert"
 )
 
@@ -85,6 +84,8 @@ func TestSpecifiedSecretFiles(t *testing.T) {
 	sf := &bs.Spec.Application.ExtraFiles.Secrets
 	*sf = append(*sf, bsv1alpha1.ObjectKeyRef{Name: "secret1", Key: "conf.yaml"})
 	*sf = append(*sf, bsv1alpha1.ObjectKeyRef{Name: "secret2", Key: "conf.yaml"})
+	// https://issues.redhat.com/browse/RHIDP-2246 - mounting secret/CM with dot in the name
+	*sf = append(*sf, bsv1alpha1.ObjectKeyRef{Name: "secret.dot", Key: "conf3.yaml"})
 
 	testObj := createBackstageTest(bs).withDefaultConfig(true)
 
@@ -96,11 +97,13 @@ func TestSpecifiedSecretFiles(t *testing.T) {
 	deployment := model.backstageDeployment
 	assert.NotNil(t, deployment)
 
-	assert.Equal(t, 2, len(deployment.deployment.Spec.Template.Spec.Containers[0].VolumeMounts))
+	assert.Equal(t, 3, len(deployment.deployment.Spec.Template.Spec.Containers[0].VolumeMounts))
 	assert.Equal(t, 0, len(deployment.deployment.Spec.Template.Spec.Containers[0].Args))
-	assert.Equal(t, 2, len(deployment.deployment.Spec.Template.Spec.Volumes))
+	assert.Equal(t, 3, len(deployment.deployment.Spec.Template.Spec.Volumes))
 
 	assert.Equal(t, utils.GenerateVolumeNameFromCmOrSecret("secret1"), deployment.podSpec().Volumes[0].Name)
+	assert.Equal(t, utils.GenerateVolumeNameFromCmOrSecret("secret2"), deployment.podSpec().Volumes[1].Name)
+	assert.Equal(t, utils.GenerateVolumeNameFromCmOrSecret("secret.dot"), deployment.podSpec().Volumes[2].Name)
 
 }
 

pkg/utils/utils.go

@@ -21,13 +21,17 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
+	"strings"
 
 	"k8s.io/client-go/discovery"
 	ctrl "sigs.k8s.io/controller-runtime"
 
 	"k8s.io/apimachinery/pkg/util/yaml"
 )
 
+const maxK8sResourceNameLength = 63
+
 func SetKubeLabels(labels map[string]string, backstageName string) map[string]string {
 	if labels == nil {
 		labels = map[string]string{}
@@ -51,9 +55,14 @@ func GenerateRuntimeObjectName(backstageCRName string, objectType string) string
 	return fmt.Sprintf("%s-%s", objectType, backstageCRName)
 }
 
-// GenerateVolumeNameFromCmOrSecret generates volume name for mounting ConfigMap or Secret
+// GenerateVolumeNameFromCmOrSecret generates volume name for mounting ConfigMap or Secret.
+//
+// It does so by converting the input name to an RFC 1123-compliant value, which is required by Kubernetes,
+// even if the input CM/Secret name can be a valid DNS subdomain.
+//
+// See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
 func GenerateVolumeNameFromCmOrSecret(cmOrSecretName string) string {
-	return cmOrSecretName
+	return ToRFC1123Label(cmOrSecretName)
 }
 
 func BackstageAppLabelValue(backstageName string) string {
@@ -119,3 +128,32 @@ func IsOpenshift() (bool, error) {
 
 	return false, nil
 }
+
+// ToRFC1123Label converts the given string into a valid Kubernetes label name (RFC 1123-compliant).
+// See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ for more details about the requirements.
+// It will replace any invalid characters with a dash and drop any leading or trailing dashes.
+func ToRFC1123Label(str string) string {
+	const dash = "-"
+
+	name := strings.ToLower(str)
+
+	// Replace all invalid characters with a dash
+	re := regexp.MustCompile(`[^a-z0-9-]`)
+	name = re.ReplaceAllString(name, dash)
+
+	// Replace consecutive dashes with a single dash
+	reConsecutiveDashes := regexp.MustCompile(`-+`)
+	name = reConsecutiveDashes.ReplaceAllString(name, dash)
+
+	// Truncate to maxK8sResourceNameLength characters if necessary
+	if len(name) > maxK8sResourceNameLength {
+		name = name[:maxK8sResourceNameLength]
+	}
+
+	// Continue trimming leading and trailing dashes if necessary
+	for strings.HasPrefix(name, dash) || strings.HasSuffix(name, dash) {
+		name = strings.Trim(name, dash)
+	}
+
+	return name
+}

pkg/utils/utils_test.go

@@ -0,0 +1,38 @@
+package utils
+
+import (
+	"testing"
+)
+
+func TestToRFC1123Label(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want string
+	}{
+		// The inputs below are all valid names for K8s ConfigMaps or Secrets.
+
+		{
+			name: "should replace invalid characters with a dash",
+			in:   "kube-root-ca.crt",
+			want: "kube-root-ca-crt",
+		},
+		{
+			name: "all-numeric string should remain unchanged",
+			in:   "123456789",
+			want: "123456789",
+		},
+		{
+			name: "should truncate up to the maximum length and remove leading and trailing dashes",
+			in:   "ppxkgq.df-yyatvyrgjtwivunibicne-bvyyotvonbrtfv-awylmrez.ksvqjw-z.xpgdi", /* 70 characters */
+			want: "ppxkgq-df-yyatvyrgjtwivunibicne-bvyyotvonbrtfv-awylmrez-ksvqjw",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := ToRFC1123Label(tt.in); got != tt.want {
+				t.Errorf("ToRFC1123Label() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}