Merge pull request #61080 from liggitt/subpath-test

Automatic merge from submit-queue (batch tested with PRs 60737, 60739, 61080, 60968, 60951). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Detect backsteps correctly in base path detection

Avoids false positives with atomic writer `..<timestamp>` directories

Fixes #61076

/assign @msau42 @jsafrane

```release-note
Fix a regression that prevented using `subPath` volume mounts with secret, configMap, projected, and downwardAPI volumes
```

pkg/kubelet/util/util.go

@@ -19,8 +19,6 @@ package util
 import (
 	"fmt"
 	"net/url"
-	"path/filepath"
-	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -47,14 +45,3 @@ func parseEndpoint(endpoint string) (string, string, error) {
 		return u.Scheme, "", fmt.Errorf("protocol %q not supported", u.Scheme)
 	}
 }
-
-func pathWithinBase(fullPath, basePath string) bool {
-	rel, err := filepath.Rel(basePath, fullPath)
-	if err != nil {
-		return false
-	}
-	if strings.HasPrefix(rel, "..") {
-		return false
-	}
-	return true
-}

pkg/util/mount/mount.go

@@ -325,9 +325,15 @@ func pathWithinBase(fullPath, basePath string) bool {
 	if err != nil {
 		return false
 	}
-	if strings.HasPrefix(rel, "..") {
+	if startsWithBackstep(rel) {
 		// Needed to escape the base path
 		return false
 	}
 	return true
 }
+
+// startsWithBackstep checks if the given path starts with a backstep segment
+func startsWithBackstep(rel string) bool {
+	// normalize to / and check for ../
+	return rel == ".." || strings.HasPrefix(filepath.ToSlash(rel), "../")
+}

pkg/util/mount/mount_linux_test.go

@@ -402,6 +402,12 @@ func TestPathWithinBase(t *testing.T) {
 			basePath: "/a",
 			expected: false,
 		},
+		{
+			name:     "configmap subpath",
+			fullPath: "/var/lib/kubelet/pods/uuid/volumes/kubernetes.io~configmap/config/..timestamp/file.txt",
+			basePath: "/var/lib/kubelet/pods/uuid/volumes/kubernetes.io~configmap/config",
+			expected: true,
+		},
 	}
 	for _, test := range tests {
 		if pathWithinBase(test.fullPath, test.basePath) != test.expected {

pkg/util/mount/mount_windows.go

@@ -291,7 +291,7 @@ func lockAndCheckSubPathWithoutSymlink(volumePath, subPath string) ([]uintptr, e
 	if err != nil {
 		return []uintptr{}, fmt.Errorf("Rel(%s, %s) error: %v", volumePath, subPath, err)
 	}
-	if strings.HasPrefix(relSubPath, "..") {
+	if startsWithBackstep(relSubPath) {
 		return []uintptr{}, fmt.Errorf("SubPath %q not within volume path %q", subPath, volumePath)
 	}
 
@@ -552,7 +552,7 @@ func findExistingPrefix(base, pathname string) (string, []string, error) {
 		return base, nil, err
 	}
 
-	if strings.HasPrefix(rel, "..") {
+	if startsWithBackstep(rel) {
 		return base, nil, fmt.Errorf("pathname(%s) is not within base(%s)", pathname, base)
 	}
 

pkg/util/mount/mount_windows_test.go

@@ -538,6 +538,11 @@ func TestPathWithinBase(t *testing.T) {
 			basePath:       `c:\tmp\a\b\c`,
 			expectedResult: false,
 		},
+		{
+			fullPath:       `c:\kubelet\pods\uuid\volumes\kubernetes.io~configmap\config\..timestamp\file.txt`,
+			basePath:       `c:\kubelet\pods\uuid\volumes\kubernetes.io~configmap\config`,
+			expectedResult: true,
+		},
 	}
 
 	for _, test := range tests {

test/e2e/storage/subpath.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	"k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -78,6 +79,120 @@ var _ = utils.SIGDescribe("Subpath", func() {
 
 	f := framework.NewDefaultFramework("subpath")
 
+	Context("Atomic writer volumes", func() {
+		var err error
+
+		BeforeEach(func() {
+			By("Setting up data")
+			secret := &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "my-secret"}, Data: map[string][]byte{"secret-key": []byte("secret-value")}}
+			secret, err = f.ClientSet.CoreV1().Secrets(f.Namespace.Name).Create(secret)
+			if err != nil && !apierrors.IsAlreadyExists(err) {
+				Expect(err).ToNot(HaveOccurred())
+			}
+
+			configmap := &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "my-configmap"}, Data: map[string]string{"configmap-key": "configmap-value"}}
+			configmap, err = f.ClientSet.CoreV1().ConfigMaps(f.Namespace.Name).Create(configmap)
+			if err != nil && !apierrors.IsAlreadyExists(err) {
+				Expect(err).ToNot(HaveOccurred())
+			}
+		})
+
+		gracePeriod := int64(1)
+
+		It("should support subpaths with secret pod", func() {
+			pod := &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{Name: "pod-subpath-test-secret"},
+				Spec: v1.PodSpec{
+					Containers: []v1.Container{{
+						Name:         "test",
+						Image:        mountImage,
+						Args:         []string{"--file_content=" + volumePath},
+						VolumeMounts: []v1.VolumeMount{{Name: "secret", MountPath: volumePath, SubPath: "secret-key"}},
+					}},
+					RestartPolicy:                 v1.RestartPolicyNever,
+					TerminationGracePeriodSeconds: &gracePeriod,
+					Volumes: []v1.Volume{{Name: "secret", VolumeSource: v1.VolumeSource{Secret: &v1.SecretVolumeSource{SecretName: "my-secret"}}}},
+				},
+			}
+			By(fmt.Sprintf("Creating pod %s", pod.Name))
+			f.TestContainerOutput("secret", pod, 0, []string{"secret-value"})
+		})
+
+		It("should support subpaths with configmap pod", func() {
+			pod := &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{Name: "pod-subpath-test-configmap"},
+				Spec: v1.PodSpec{
+					Containers: []v1.Container{{
+						Name:         "test",
+						Image:        mountImage,
+						Args:         []string{"--file_content=" + volumePath},
+						VolumeMounts: []v1.VolumeMount{{Name: "configmap", MountPath: volumePath, SubPath: "configmap-key"}},
+					}},
+					RestartPolicy:                 v1.RestartPolicyNever,
+					TerminationGracePeriodSeconds: &gracePeriod,
+					Volumes: []v1.Volume{{Name: "configmap", VolumeSource: v1.VolumeSource{ConfigMap: &v1.ConfigMapVolumeSource{LocalObjectReference: v1.LocalObjectReference{Name: "my-configmap"}}}}},
+				},
+			}
+			By(fmt.Sprintf("Creating pod %s", pod.Name))
+			f.TestContainerOutput("configmap", pod, 0, []string{"configmap-value"})
+		})
+
+		It("should support subpaths with downward pod", func() {
+			pod := &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{Name: "pod-subpath-test-downward"},
+				Spec: v1.PodSpec{
+					Containers: []v1.Container{{
+						Name:         "test",
+						Image:        mountImage,
+						Args:         []string{"--file_content=" + volumePath},
+						VolumeMounts: []v1.VolumeMount{{Name: "downward", MountPath: volumePath, SubPath: "downward/podname"}},
+					}},
+					RestartPolicy:                 v1.RestartPolicyNever,
+					TerminationGracePeriodSeconds: &gracePeriod,
+					Volumes: []v1.Volume{
+						{Name: "downward", VolumeSource: v1.VolumeSource{
+							DownwardAPI: &v1.DownwardAPIVolumeSource{
+								Items: []v1.DownwardAPIVolumeFile{{Path: "downward/podname", FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}}},
+							},
+						}},
+					},
+				},
+			}
+			By(fmt.Sprintf("Creating pod %s", pod.Name))
+			f.TestContainerOutput("downward", pod, 0, []string{"content of file \"" + volumePath + "\": " + pod.Name})
+		})
+
+		It("should support subpaths with projected pod", func() {
+			pod := &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{Name: "pod-subpath-test-secret"},
+				Spec: v1.PodSpec{
+					Containers: []v1.Container{{
+						Name:         "test",
+						Image:        mountImage,
+						Args:         []string{"--file_content=" + volumePath},
+						VolumeMounts: []v1.VolumeMount{{Name: "projected", MountPath: volumePath, SubPath: "projected/configmap-key"}},
+					}},
+					RestartPolicy:                 v1.RestartPolicyNever,
+					TerminationGracePeriodSeconds: &gracePeriod,
+					Volumes: []v1.Volume{
+						{Name: "projected", VolumeSource: v1.VolumeSource{
+							Projected: &v1.ProjectedVolumeSource{
+								Sources: []v1.VolumeProjection{
+									{ConfigMap: &v1.ConfigMapProjection{
+										LocalObjectReference: v1.LocalObjectReference{Name: "my-configmap"},
+										Items:                []v1.KeyToPath{{Path: "projected/configmap-key", Key: "configmap-key"}},
+									}},
+								},
+							},
+						}},
+					},
+				},
+			}
+			By(fmt.Sprintf("Creating pod %s", pod.Name))
+			f.TestContainerOutput("projected", pod, 0, []string{"configmap-value"})
+		})
+	})
+
 	for volType, volInit := range initVolSources {
 		curVolType := volType
 		curVolInit := volInit