From c090b4401cb5fe1726f2671facbb293f8a7d62b4 Mon Sep 17 00:00:00 2001 From: Ondrej Skutka Date: Wed, 26 Aug 2026 11:47:53 +0200 Subject: [PATCH 1/4] fix: sanitize automount volume names to be DNS-1123 compliant Auto-mounted Secrets, ConfigMaps, and PVCs derive their pod volume name from the object's name. Kubernetes object names and volume names have different validation rules: a name that is legal for a Secret (e.g. "test.pullsecret") can be invalid as a volume name, which must be a DNS-1123 label. Previously the object name was used verbatim, so mounting such an object produced an invalid Deployment and the workspace failed to start. Sanitize the derived volume name (lowercase, replace invalid characters with '-', trim edges, truncate to 63 chars) via a shared helper in pkg/common/naming.go, used by AutoMount{Secret,ConfigMap,PVC}VolumeName. The volume still references the underlying object by its original, unmodified name. Adds unit, fixture-based integration, and e2e coverage, plus an ADR documenting the sanitize-vs-reject decision. Assisted-by: Claude Signed-off-by: Ondrej Skutka --- ...6-08-26-sanitize-automount-volume-names.md | 92 ++++++++ pkg/common/naming.go | 33 ++- pkg/common/naming_test.go | 67 ++++++ .../testSanitizesInvalidVolumeNames.yaml | 54 +++++ .../automount_volume_sanitization_tests.go | 197 ++++++++++++++++++ .../volume-sanitization-test-workspace.yaml | 14 ++ 6 files changed, 450 insertions(+), 7 deletions(-) create mode 100644 adr/2026-08-26-sanitize-automount-volume-names.md create mode 100644 pkg/common/naming_test.go create mode 100644 pkg/provision/automount/testdata/testSanitizesInvalidVolumeNames.yaml create mode 100644 test/e2e/pkg/tests/automount_volume_sanitization_tests.go create mode 100644 test/resources/volume-sanitization-test-workspace.yaml diff --git a/adr/2026-08-26-sanitize-automount-volume-names.md b/adr/2026-08-26-sanitize-automount-volume-names.md new file mode 100644 index 000000000..bdc2f5076 --- /dev/null +++ b/adr/2026-08-26-sanitize-automount-volume-names.md @@ -0,0 +1,92 @@ +# Sanitize automount volume names to be DNS-1123 compliant + +**Status**: Proposed +**Date**: 2026-08-26 +**Deciders**: DevWorkspace Operator maintainers +**Related Issue**: CRW-9800 + +## Context + +When a Secret, ConfigMap, or PVC is auto-mounted into a workspace (via the +`controller.devfile.io/mount-to-devworkspace` label), DWO derives a pod volume +name from the object's name. Previously the object name was used verbatim +(`AutoMountSecretVolumeName`, `AutoMountConfigMapVolumeName`, +`AutoMountPVCVolumeName` all returned their input unchanged). + +Kubernetes object names and volume names have *different* validation rules. A +Secret named `test.pullsecret` is perfectly legal, but a pod volume name must be +a DNS-1123 label (lowercase alphanumeric plus `-`, must start/end alphanumeric, +≤63 chars). A dot is invalid in a volume name. As a result, auto-mounting a +secret whose name contained a dot (or other invalid character) produced an +invalid Deployment, and the workspace failed to start. + +The object name itself is valid and must be preserved — the volume's +`secretName`/`configMap.name`/`claimName` still has to reference the real +object. Only DWO's *derivation* of the volume name was wrong. + +## Decision + +Sanitize the derived volume name to a DNS-1123 label via a shared +`sanitizeVolumeName` helper in `pkg/common/naming.go`, used by all three +`AutoMount*VolumeName` functions. Sanitization lowercases the name, replaces +runs of invalid characters with `-`, trims leading/trailing `-`, and truncates +to 63 characters (trimming any trailing `-` left by truncation). + +The volume's reference to the underlying object (`secretName`, `configMap.name`, +`claimName`) continues to use the original, unmodified object name. + +## Considered Alternatives + +### Alternative 1: Reject invalid object names at admission (webhook validation) + +Add a validating webhook that denies a workspace (or the labeled object) when an +auto-mount source has a name that cannot form a valid volume name. + +**Rejected because**: +- The object name is legal Kubernetes; rejecting it pushes a DWO-internal + limitation onto the user, who did nothing wrong. +- Auto-mounted objects are matched by label and can be created independently of + (and after) the workspace, so there is no single admission point that cleanly + owns this validation. +- It is a worse user experience: the workspace fails instead of just working. + +### Alternative 2: Keep names verbatim, only truncate for length + +The pre-existing behavior already tolerated long names implicitly; only add +length handling. + +**Rejected because**: +- It does not fix the reported bug — invalid *characters* (dots, underscores, + etc.), not just length, are the failure in CRW-9800. + +## Consequences + +### Positive + +1. Auto-mounting objects with names that are legal in Kubernetes but invalid as + volume names now works transparently. +2. Length handling (≤63 chars) is now correct as a side effect, replacing the + previous reliance on never adding characters to the name. + +### Negative + +1. Sanitization is not injective: two distinct object names can map to the same + volume name (e.g. `test.pullsecret` and `test-pullsecret`). This is an + accepted trade-off — such a collision is caught by the existing + `checkAutoMountVolumesForCollision` check, which surfaces a clear error rather + than producing a silently broken spec. Previously these names were distinct; + the collision case is new but rare and fails loudly. + +### Neutral + +1. The old comment on `AutoMount*VolumeName` explaining why prefixes were not + added (to avoid exceeding 63 chars) was removed, as length is now handled + explicitly by `sanitizeVolumeName`. + +## References + +- `pkg/common/naming.go` — `sanitizeVolumeName` and the `AutoMount*VolumeName` functions +- `pkg/common/naming_test.go` — unit tests for sanitization +- `pkg/provision/automount/testdata/testSanitizesInvalidVolumeNames.yaml` — fixture-based integration test +- `test/e2e/pkg/tests/automount_volume_sanitization_tests.go` — end-to-end test +- `pkg/provision/automount/common.go` — `checkAutoMountVolumesForCollision` diff --git a/pkg/common/naming.go b/pkg/common/naming.go index 779497bef..42078c318 100644 --- a/pkg/common/naming.go +++ b/pkg/common/naming.go @@ -1,5 +1,5 @@ // -// Copyright (c) 2019-2025 Red Hat, Inc. +// Copyright (c) 2019-2026 Red Hat, Inc. // 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 @@ -116,19 +116,38 @@ func MetadataConfigMapName(workspaceId string) string { return fmt.Sprintf("%s-metadata", workspaceId) } -// We can't add prefixes to automount volume names, as adding any characters -// can potentially push the name over the 63 character limit (if the original -// object has a long name) func AutoMountConfigMapVolumeName(volumeName string) string { - return volumeName + return sanitizeVolumeName(volumeName) } func AutoMountSecretVolumeName(volumeName string) string { - return volumeName + return sanitizeVolumeName(volumeName) } func AutoMountPVCVolumeName(pvcName string) string { - return pvcName + return sanitizeVolumeName(pvcName) +} + +// sanitizeVolumeName converts a name to be DNS-1123 label compliant for use as a Kubernetes volume name. +// Volume names must: +// - contain at most 63 characters +// - contain only lowercase alphanumeric characters or '-' +// - start with an alphanumeric character +// - end with an alphanumeric character +// +// Generated by Claude +func sanitizeVolumeName(name string) string { + // Convert to lowercase + sanitized := strings.ToLower(name) + // Replace any non-alphanumeric characters (including dots) with hyphens + sanitized = NonAlphaNumRegexp.ReplaceAllString(sanitized, "-") + // Trim leading and trailing hyphens + sanitized = strings.Trim(sanitized, "-") + // Ensure length is at most 63 characters (DNS-1123 label limit) + if len(sanitized) > 63 { + sanitized = strings.TrimSuffix(sanitized[:63], "-") + } + return sanitized } func AutoMountProjectedVolumeName(mountPath string) string { diff --git a/pkg/common/naming_test.go b/pkg/common/naming_test.go new file mode 100644 index 000000000..5c1f9e148 --- /dev/null +++ b/pkg/common/naming_test.go @@ -0,0 +1,67 @@ +// +// Copyright (c) 2019-2026 Red Hat, Inc. +// 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 common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSanitizeVolumeName(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "replaces dots with hyphens", + input: "test.pullsecret", + expected: "test-pullsecret", + }, + { + name: "replaces all invalid characters and lowercases", + input: "Test.Secret_Name@example", + expected: "test-secret-name-example", + }, + { + name: "collapses consecutive invalid characters and trims edges", + input: ".test...secret.", + expected: "test-secret", + }, + { + name: "leaves already valid names unchanged", + input: "valid-secret-123", + expected: "valid-secret-123", + }, + { + name: "truncates to 63 characters without a trailing hyphen", + input: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbb", + expected: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := sanitizeVolumeName(tt.input) + assert.Equal(t, tt.expected, result, "sanitizeVolumeName(%q) should match expected value", tt.input) + + // Verify DNS-1123 label compliance + assert.LessOrEqual(t, len(result), 63, "Volume name should not exceed 63 characters") + assert.Regexp(t, "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", result, "Volume name should be a valid DNS-1123 label") + }) + } +} diff --git a/pkg/provision/automount/testdata/testSanitizesInvalidVolumeNames.yaml b/pkg/provision/automount/testdata/testSanitizesInvalidVolumeNames.yaml new file mode 100644 index 000000000..56c440f4f --- /dev/null +++ b/pkg/provision/automount/testdata/testSanitizesInvalidVolumeNames.yaml @@ -0,0 +1,54 @@ +# Volume names derived from automounted objects must be DNS-1123 label +# compliant. Object names may legally contain characters (e.g. dots) that are +# invalid in a volume name, so the derived volume name must be sanitized while +# still referencing the original object by its real name. +name: "Sanitizes invalid characters in automount volume names" + +input: + secrets: + - + apiVersion: v1 + kind: Secret + metadata: + name: test.pullsecret + labels: + controller.devfile.io/mount-to-devworkspace: "true" + controller.devfile.io/watch-secret: "true" + annotations: + controller.devfile.io/mount-as: file + controller.devfile.io/mount-path: /tmp/secret/file + type: Opaque + data: + test_data: aGVsbG8K # "hello" + configmaps: + - + apiVersion: v1 + kind: ConfigMap + metadata: + name: test.configmap + labels: + controller.devfile.io/mount-to-devworkspace: "true" + controller.devfile.io/watch-configmap: "true" + annotations: + controller.devfile.io/mount-as: file + controller.devfile.io/mount-path: /tmp/configmap/file + data: + configmap-key: "hello" + +output: + volumes: + - name: test-pullsecret + secret: + secretName: test.pullsecret + defaultMode: 0640 + - name: test-configmap + configmap: + name: test.configmap + defaultMode: 0640 + volumeMounts: + - name: test-pullsecret + readOnly: true + mountPath: /tmp/secret/file + - name: test-configmap + readOnly: true + mountPath: /tmp/configmap/file diff --git a/test/e2e/pkg/tests/automount_volume_sanitization_tests.go b/test/e2e/pkg/tests/automount_volume_sanitization_tests.go new file mode 100644 index 000000000..ee1bf6d28 --- /dev/null +++ b/test/e2e/pkg/tests/automount_volume_sanitization_tests.go @@ -0,0 +1,197 @@ +// +// Copyright (c) 2019-2026 Red Hat, Inc. +// 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 tests + +import ( + "context" + "fmt" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/test/e2e/pkg/config" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// Verifies that a secret whose name contains characters invalid in a volume name +// (e.g. dots) is auto-mounted with a sanitized, DNS-1123 compliant volume name. +var _ = ginkgo.Describe("[Automount Secret with Invalid Volume Name Characters]", ginkgo.Ordered, func() { + defer ginkgo.GinkgoRecover() + + const ( + workspaceName = "volume-sanitization-test" + secretWithDots = "test.pullsecret" // Invalid: contains dots + expectedVolumeName = "test-pullsecret" // Expected sanitized name + secretData = "test-secret-data" + ) + + ginkgo.AfterAll(func() { + // Delete the test secret + _ = config.DevK8sClient.Kube().CoreV1().Secrets(config.DevWorkspaceNamespace). + Delete(context.TODO(), secretWithDots, metav1.DeleteOptions{}) + + // Cleanup workspace and wait for PVC to be fully deleted + // This prevents PVC conflicts in subsequent tests, especially in CI environments + _ = config.DevK8sClient.DeleteDevWorkspaceAndWait(workspaceName, config.DevWorkspaceNamespace) + }) + + ginkgo.It("Create secret with dots in name and mount-to-devworkspace label", func() { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretWithDots, + Namespace: config.DevWorkspaceNamespace, + Labels: map[string]string{ + "controller.devfile.io/mount-to-devworkspace": "true", + "controller.devfile.io/watch-secret": "true", + }, + }, + StringData: map[string]string{ + "test-key": secretData, + }, + Type: corev1.SecretTypeOpaque, + } + + _, err := config.DevK8sClient.Kube().CoreV1().Secrets(config.DevWorkspaceNamespace). + Create(context.TODO(), secret, metav1.CreateOptions{}) + if err != nil { + ginkgo.Fail(fmt.Sprintf("Failed to create secret with dots in name: %s", err.Error())) + } + }) + + ginkgo.It("Create and start DevWorkspace that should auto-mount the secret", func() { + commandResult, err := config.DevK8sClient.OcApplyWorkspace( + config.DevWorkspaceNamespace, + "test/resources/volume-sanitization-test-workspace.yaml", + ) + if err != nil { + ginkgo.Fail(fmt.Sprintf("Failed to create DevWorkspace: %s %s", err.Error(), commandResult)) + } + }) + + ginkgo.It("Wait for DevWorkspace to reach Running status", func() { + deploy, err := config.DevK8sClient.WaitDevWsStatus( + workspaceName, + config.DevWorkspaceNamespace, + dw.DevWorkspaceStatusRunning, + ) + if !deploy { + ginkgo.Fail(fmt.Sprintf("DevWorkspace didn't start properly. Error: %s", err)) + } + }) + + var podName string + ginkgo.It("Verify deployment has sanitized volume name", func() { + podSelector := fmt.Sprintf("controller.devfile.io/devworkspace_name=%s", workspaceName) + var err error + podName, err = config.AdminK8sClient.GetPodNameBySelector(podSelector, config.DevWorkspaceNamespace) + if err != nil { + ginkgo.Fail(fmt.Sprintf("Cannot get workspace pod by selector. Error: %s", err)) + } + + pod, err := config.DevK8sClient.Kube().CoreV1().Pods(config.DevWorkspaceNamespace). + Get(context.TODO(), podName, metav1.GetOptions{}) + if err != nil { + ginkgo.Fail(fmt.Sprintf("Failed to get pod: %s", err.Error())) + } + + // Verify volume name is sanitized (dots replaced with hyphens) + volumeFound := false + for _, volume := range pod.Spec.Volumes { + if volume.Name == expectedVolumeName { + volumeFound = true + // Verify it's a secret volume with the correct secret name + if volume.Secret == nil { + ginkgo.Fail(fmt.Sprintf("Volume %s is not a secret volume", expectedVolumeName)) + } + if volume.Secret.SecretName != secretWithDots { + ginkgo.Fail(fmt.Sprintf("Volume %s references wrong secret: %s, expected: %s", + expectedVolumeName, volume.Secret.SecretName, secretWithDots)) + } + break + } + // Also verify the original name (with dots) is NOT used + if volume.Name == secretWithDots { + ginkgo.Fail(fmt.Sprintf("Volume name was not sanitized: found volume with name '%s' (should be '%s')", + secretWithDots, expectedVolumeName)) + } + } + + if !volumeFound { + ginkgo.Fail(fmt.Sprintf("Sanitized volume name '%s' not found in pod volumes. Available volumes: %v", + expectedVolumeName, getPodVolumeNames(pod))) + } + }) + + ginkgo.It("Verify volume mount uses sanitized volume name", func() { + pod, err := config.DevK8sClient.Kube().CoreV1().Pods(config.DevWorkspaceNamespace). + Get(context.TODO(), podName, metav1.GetOptions{}) + if err != nil { + ginkgo.Fail(fmt.Sprintf("Failed to get pod: %s", err.Error())) + } + + // Check all containers for the volume mount + volumeMountFound := false + for _, container := range pod.Spec.Containers { + for _, volumeMount := range container.VolumeMounts { + if volumeMount.Name == expectedVolumeName { + volumeMountFound = true + // Verify mount path + expectedMountPath := fmt.Sprintf("/etc/secret/%s", secretWithDots) + if volumeMount.MountPath != expectedMountPath { + ginkgo.Fail(fmt.Sprintf("Volume mount path incorrect: got %s, expected %s", + volumeMount.MountPath, expectedMountPath)) + } + break + } + } + } + + if !volumeMountFound { + ginkgo.Fail(fmt.Sprintf("Volume mount with sanitized name '%s' not found in any container", + expectedVolumeName)) + } + }) + + ginkgo.It("Verify secret data is accessible inside the container", func() { + // Execute command to verify the secret is mounted and accessible + containerName := "test-container" + secretFilePath := fmt.Sprintf("/etc/secret/%s/test-key", secretWithDots) + catCommand := fmt.Sprintf("cat %s", secretFilePath) + + resultOfExecCommand, err := config.DevK8sClient.ExecCommandInContainer( + podName, + config.DevWorkspaceNamespace, + containerName, + catCommand, + ) + if err != nil { + ginkgo.Fail(fmt.Sprintf("Cannot execute command in the devworkspace container. Error: `%s`. Exec output: `%s`", + err, resultOfExecCommand)) + } + + gomega.Expect(resultOfExecCommand).To(gomega.ContainSubstring(secretData)) + }) +}) + +// Helper function to get pod volume names for debugging +func getPodVolumeNames(pod *corev1.Pod) []string { + volumeNames := make([]string, len(pod.Spec.Volumes)) + for i, volume := range pod.Spec.Volumes { + volumeNames[i] = volume.Name + } + return volumeNames +} diff --git a/test/resources/volume-sanitization-test-workspace.yaml b/test/resources/volume-sanitization-test-workspace.yaml new file mode 100644 index 000000000..77441ff30 --- /dev/null +++ b/test/resources/volume-sanitization-test-workspace.yaml @@ -0,0 +1,14 @@ +# Test workspace: verifies that secrets with invalid volume name characters (dots) are properly sanitized +kind: DevWorkspace +apiVersion: workspace.devfile.io/v1alpha2 +metadata: + name: volume-sanitization-test +spec: + started: true + template: + components: + - name: test-container + container: + image: quay.io/devfile/universal-developer-image:latest + mountSources: false + args: ["tail", "-f", "/dev/null"] From 46dbef10fbcb49f5cfdaefa3bc0ba41f90f84229 Mon Sep 17 00:00:00 2001 From: Ondrej Skutka Date: Wed, 26 Aug 2026 15:04:59 +0200 Subject: [PATCH 2/4] Update pkg/common/naming.go Co-authored-by: Rohan Kumar Signed-off-by: Ondrej Skutka --- pkg/common/naming.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/common/naming.go b/pkg/common/naming.go index 42078c318..e8f4e183c 100644 --- a/pkg/common/naming.go +++ b/pkg/common/naming.go @@ -137,13 +137,9 @@ func AutoMountPVCVolumeName(pvcName string) string { // // Generated by Claude func sanitizeVolumeName(name string) string { - // Convert to lowercase sanitized := strings.ToLower(name) - // Replace any non-alphanumeric characters (including dots) with hyphens sanitized = NonAlphaNumRegexp.ReplaceAllString(sanitized, "-") - // Trim leading and trailing hyphens sanitized = strings.Trim(sanitized, "-") - // Ensure length is at most 63 characters (DNS-1123 label limit) if len(sanitized) > 63 { sanitized = strings.TrimSuffix(sanitized[:63], "-") } From 816e83d78a082d019ea423fe67c8aab2b9653302 Mon Sep 17 00:00:00 2001 From: Ondrej Skutka Date: Wed, 26 Aug 2026 16:24:59 +0200 Subject: [PATCH 3/4] fix(automount): detect volume name collisions after sanitization Sanitizing automount volume names to be DNS-1123 compliant is not injective: two distinct object names can resolve to the same volume name (e.g. secrets 'test.pullsecret' and 'test-pullsecret' both sanitize to 'test-pullsecret'). Previously such a collision produced a pod spec with duplicate volume names and no error, leaving the Deployment to be rejected by the API server. Extend checkAutomountVolumesForCollision to detect automount-vs-automount volume name collisions and surface a clear error naming both source objects. Assisted-by: Claude Signed-off-by: Ondrej Skutka --- ...6-08-26-sanitize-automount-volume-names.md | 13 ++++--- pkg/provision/automount/common.go | 11 +++++- pkg/provision/automount/common_test.go | 32 +++++++++++++++-- ...rDuplicateVolumeNameAfterSanitization.yaml | 36 +++++++++++++++++++ 4 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 pkg/provision/automount/testdata/errorDuplicateVolumeNameAfterSanitization.yaml diff --git a/adr/2026-08-26-sanitize-automount-volume-names.md b/adr/2026-08-26-sanitize-automount-volume-names.md index bdc2f5076..255513132 100644 --- a/adr/2026-08-26-sanitize-automount-volume-names.md +++ b/adr/2026-08-26-sanitize-automount-volume-names.md @@ -72,10 +72,12 @@ length handling. 1. Sanitization is not injective: two distinct object names can map to the same volume name (e.g. `test.pullsecret` and `test-pullsecret`). This is an - accepted trade-off — such a collision is caught by the existing - `checkAutoMountVolumesForCollision` check, which surfaces a clear error rather - than producing a silently broken spec. Previously these names were distinct; - the collision case is new but rare and fails loudly. + accepted trade-off. Because `checkAutomountVolumesForCollision` previously only + detected DevWorkspace-vs-automount name collisions and mount-path collisions — + not two *automounted* objects resolving to the same name — this change also + extends that check to catch the new case, so it surfaces a clear error rather + than producing an invalid pod spec that the API server rejects. Previously + these names were distinct; the collision case is new but rare and fails loudly. ### Neutral @@ -88,5 +90,6 @@ length handling. - `pkg/common/naming.go` — `sanitizeVolumeName` and the `AutoMount*VolumeName` functions - `pkg/common/naming_test.go` — unit tests for sanitization - `pkg/provision/automount/testdata/testSanitizesInvalidVolumeNames.yaml` — fixture-based integration test +- `pkg/provision/automount/testdata/errorDuplicateVolumeNameAfterSanitization.yaml` — fixture for the collision case - `test/e2e/pkg/tests/automount_volume_sanitization_tests.go` — end-to-end test -- `pkg/provision/automount/common.go` — `checkAutoMountVolumesForCollision` +- `pkg/provision/automount/common.go` — `checkAutomountVolumesForCollision` (extended to detect automount-vs-automount name collisions) diff --git a/pkg/provision/automount/common.go b/pkg/provision/automount/common.go index 8cae61ae3..e082f5e8a 100644 --- a/pkg/provision/automount/common.go +++ b/pkg/provision/automount/common.go @@ -126,9 +126,18 @@ func getAutomountResources( } func checkAutomountVolumesForCollision(podAdditions *v1alpha1.PodAdditions, automount *Resources) error { - // Get a map of automounted volume names to volume structs + // Get a map of automounted volume names to volume structs. Two automounted objects can resolve to the + // same (sanitized) volume name -- e.g. secrets 'test.pullsecret' and 'test-pullsecret' both sanitize to + // 'test-pullsecret' -- which would produce an invalid pod spec with duplicate volume names. Detect this + // here so it surfaces as a clear error instead of a Deployment rejected by the API server. automountVolumeNames := map[string]corev1.Volume{} for _, volume := range automount.Volumes { + if conflict, exists := automountVolumeNames[volume.Name]; exists { + return &dwerrors.FailError{ + Message: fmt.Sprintf("auto-mounted volumes from %s and %s resolve to the same volume name '%s'", + formatVolumeDescription(volume), formatVolumeDescription(conflict), volume.Name), + } + } automountVolumeNames[volume.Name] = volume } diff --git a/pkg/provision/automount/common_test.go b/pkg/provision/automount/common_test.go index c19b0c1ca..d702cd68c 100644 --- a/pkg/provision/automount/common_test.go +++ b/pkg/provision/automount/common_test.go @@ -167,7 +167,11 @@ func TestProvisionAutomountResourcesInto(t *testing.T) { func TestCheckAutoMountVolumesForCollision(t *testing.T) { type volumeDesc struct { - name string + name string + // sourceName is the name of the underlying object (secret/configmap/pvc) referenced by the volume. + // It defaults to name when empty; set it separately to model two distinct objects whose (sanitized) + // volume names collide. + sourceName string mountPath string volumeType mountedVolumeType } @@ -251,16 +255,38 @@ func TestCheckAutoMountVolumesForCollision(t *testing.T) { }, errRegexp: "auto-mounted volumes from configmap 'testVolume2' and secret 'testVolume1' have the same mount path", }, + { + name: "Detects volume name collision between automounted volumes", + automountPodAdditions: []volumeDesc{ + { + name: "test-pullsecret", + sourceName: "test.pullsecret", + mountPath: "/test/mount1", + volumeType: secretVolumeType, + }, + { + name: "test-pullsecret", + sourceName: "test-pullsecret", + mountPath: "/test/mount2", + volumeType: secretVolumeType, + }, + }, + errRegexp: "auto-mounted volumes from secret 'test-pullsecret' and secret 'test.pullsecret' resolve to the same volume name 'test-pullsecret'", + }, } convertDescToVolume := func(desc volumeDesc) (*corev1.Volume, *corev1.VolumeMount, *corev1.Container) { + sourceName := desc.sourceName + if sourceName == "" { + sourceName = desc.name + } switch desc.volumeType { case secretVolumeType: volume := &corev1.Volume{ Name: desc.name, VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ - SecretName: desc.name, + SecretName: sourceName, }, }, } @@ -275,7 +301,7 @@ func TestCheckAutoMountVolumesForCollision(t *testing.T) { VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ - Name: desc.name, + Name: sourceName, }, }, }, diff --git a/pkg/provision/automount/testdata/errorDuplicateVolumeNameAfterSanitization.yaml b/pkg/provision/automount/testdata/errorDuplicateVolumeNameAfterSanitization.yaml new file mode 100644 index 000000000..4d803df15 --- /dev/null +++ b/pkg/provision/automount/testdata/errorDuplicateVolumeNameAfterSanitization.yaml @@ -0,0 +1,36 @@ +# Two objects whose names differ only by characters that sanitization collapses +# (a dot vs a hyphen) resolve to the same volume name. This must fail with a clear +# error rather than producing an invalid pod spec with duplicate volume names. +name: "Errors when two automounted objects resolve to the same volume name" + +input: + secrets: + - + apiVersion: v1 + kind: Secret + metadata: + name: test.pullsecret + labels: + controller.devfile.io/mount-to-devworkspace: "true" + controller.devfile.io/watch-secret: "true" + annotations: + controller.devfile.io/mount-as: file + type: Opaque + data: + test_data: aGVsbG8K # "hello" + - + apiVersion: v1 + kind: Secret + metadata: + name: test-pullsecret + labels: + controller.devfile.io/mount-to-devworkspace: "true" + controller.devfile.io/watch-secret: "true" + annotations: + controller.devfile.io/mount-as: file + type: Opaque + data: + test_data: aGVsbG8K # "hello" + +output: + errRegexp: "resolve to the same volume name 'test-pullsecret'" From dfdfdda7a512c78d4d0c2cf724e9b83a269da868 Mon Sep 17 00:00:00 2001 From: Ondrej Skutka Date: Thu, 27 Aug 2026 15:52:11 +0200 Subject: [PATCH 4/4] test(naming): add coverage for hyphen collapse and truncation boundary Prove sanitizeVolumeName never leaves consecutive hyphens: the [^a-z0-9]+ regex collapses any run of invalid characters (including literal hyphens) into a single '-', so a single TrimSuffix suffices after truncation. Adds a literal-hyphen-collapse case and a truncation case where the collapsed hyphen lands exactly at position 63 and is trimmed. Assisted-by: Claude Signed-off-by: Ondrej Skutka --- pkg/common/naming_test.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/common/naming_test.go b/pkg/common/naming_test.go index 5c1f9e148..d0c493497 100644 --- a/pkg/common/naming_test.go +++ b/pkg/common/naming_test.go @@ -42,14 +42,27 @@ func TestSanitizeVolumeName(t *testing.T) { input: ".test...secret.", expected: "test-secret", }, + { + // Hyphens are non-alphanumeric, so the [^a-z0-9]+ regex matches a run of + // literal hyphens and collapses it to a single '-'. This guarantees the + // sanitized name can never contain two or more consecutive hyphens. + name: "collapses consecutive literal hyphens into a single hyphen", + input: "test--.-secret", + expected: "test-secret", + }, { name: "leaves already valid names unchanged", input: "valid-secret-123", expected: "valid-secret-123", }, { - name: "truncates to 63 characters without a trailing hyphen", - input: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbb", + name: "truncates to 63 characters", + input: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-----bb", + expected: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-b", + }, + { + name: "truncates characters without a trailing hyphen, keeping a valid ending", + input: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-----bb", expected: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", }, }