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..255513132 --- /dev/null +++ b/adr/2026-08-26-sanitize-automount-volume-names.md @@ -0,0 +1,95 @@ +# 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. 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 + +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 +- `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` (extended to detect automount-vs-automount name collisions) diff --git a/pkg/common/naming.go b/pkg/common/naming.go index 779497bef..e8f4e183c 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,34 @@ 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 { + sanitized := strings.ToLower(name) + sanitized = NonAlphaNumRegexp.ReplaceAllString(sanitized, "-") + sanitized = strings.Trim(sanitized, "-") + 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..d0c493497 --- /dev/null +++ b/pkg/common/naming_test.go @@ -0,0 +1,80 @@ +// +// 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", + }, + { + // 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", + input: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-----bb", + expected: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-b", + }, + { + name: "truncates characters without a trailing hyphen, keeping a valid ending", + input: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-----bb", + 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/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'" 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"]