From adc11ebc381f6762325e9f3d7bb61e878fa81761 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Tue, 28 May 2024 10:20:21 +0200 Subject: [PATCH] [RHIDP-2246] Fix reconciliation issue when referenced ConfigMap/Secret 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 --------- Co-authored-by: Gennady Azarenkov --- integration_tests/cr-config_test.go | 13 +++++++++ pkg/model/secretfiles_test.go | 11 +++++--- pkg/utils/utils.go | 42 +++++++++++++++++++++++++++-- pkg/utils/utils_test.go | 38 ++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 pkg/utils/utils_test.go diff --git a/integration_tests/cr-config_test.go b/integration_tests/cr-config_test.go index 8dd3d1c8..0c98a691 100644 --- a/integration_tests/cr-config_test.go +++ b/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,6 +81,7 @@ var _ = When("create backstage with CR configured", func() { ConfigMaps: []bsv1alpha1.ObjectKeyRef{ {Name: appConfig1}, {Name: appConfig2, Key: "key21"}, + {Name: appConfig3}, }, }, ExtraFiles: &bsv1alpha1.ExtraFiles{ @@ -85,10 +89,12 @@ var _ = When("create backstage with CR configured", func() { 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)) diff --git a/pkg/model/secretfiles_test.go b/pkg/model/secretfiles_test.go index c463d470..4a95110f 100644 --- a/pkg/model/secretfiles_test.go +++ b/pkg/model/secretfiles_test.go @@ -16,6 +16,7 @@ package model import ( "context" + "testing" "redhat-developer/red-hat-developer-hub-operator/pkg/utils" @@ -23,8 +24,6 @@ import ( 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) } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index fa109274..788123a4 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -21,6 +21,8 @@ import ( "fmt" "os" "path/filepath" + "regexp" + "strings" "k8s.io/client-go/discovery" ctrl "sigs.k8s.io/controller-runtime" @@ -28,6 +30,8 @@ import ( "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 +} diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go new file mode 100644 index 00000000..05cec926 --- /dev/null +++ b/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) + } + }) + } +}