Merge pull request #126806 from carlory/fix-image-volume-mount

Kubelet should honour the VolumeAttributes which are reported by the volume plugin

pkg/kubelet/kubelet_pods.go

@@ -277,18 +277,6 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
 	mounts := []kubecontainer.Mount{}
 	var cleanupAction func()
 	for i, mount := range container.VolumeMounts {
-		// Check if the mount is referencing an OCI volume
-		if imageVolumes != nil && utilfeature.DefaultFeatureGate.Enabled(features.ImageVolume) {
-			if image, ok := imageVolumes[mount.Name]; ok {
-				mounts = append(mounts, kubecontainer.Mount{
-					Name:          mount.Name,
-					ContainerPath: mount.MountPath,
-					Image:         image,
-				})
-				continue
-			}
-		}
-
 		// do not mount /etc/hosts if container is already mounting on the path
 		mountEtcHostsFile = mountEtcHostsFile && (mount.MountPath != etcHostsPath)
 		vol, ok := podVolumes[mount.Name]
@@ -306,69 +294,82 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
 			vol.SELinuxLabeled = true
 			relabelVolume = true
 		}
-		hostPath, err := volumeutil.GetPath(vol.Mounter)
-		if err != nil {
-			return nil, cleanupAction, err
+
+		var (
+			hostPath string
+			image    *runtimeapi.ImageSpec
+			err      error
+		)
+
+		if imageVolumes != nil && utilfeature.DefaultFeatureGate.Enabled(features.ImageVolume) {
+			image = imageVolumes[mount.Name]
 		}
 
-		subPath := mount.SubPath
-		if mount.SubPathExpr != "" {
-			subPath, err = kubecontainer.ExpandContainerVolumeMounts(mount, expandEnvs)
-
+		if image == nil {
+			hostPath, err = volumeutil.GetPath(vol.Mounter)
 			if err != nil {
 				return nil, cleanupAction, err
 			}
-		}
 
-		if subPath != "" {
-			if utilfs.IsAbs(subPath) {
-				return nil, cleanupAction, fmt.Errorf("error SubPath `%s` must not be an absolute path", subPath)
-			}
+			subPath := mount.SubPath
+			if mount.SubPathExpr != "" {
+				subPath, err = kubecontainer.ExpandContainerVolumeMounts(mount, expandEnvs)
 
-			err = volumevalidation.ValidatePathNoBacksteps(subPath)
-			if err != nil {
-				return nil, cleanupAction, fmt.Errorf("unable to provision SubPath `%s`: %v", subPath, err)
-			}
-
-			volumePath := hostPath
-			hostPath = filepath.Join(volumePath, subPath)
-
-			if subPathExists, err := hu.PathExists(hostPath); err != nil {
-				klog.ErrorS(nil, "Could not determine if subPath exists, will not attempt to change its permissions", "path", hostPath)
-			} else if !subPathExists {
-				// Create the sub path now because if it's auto-created later when referenced, it may have an
-				// incorrect ownership and mode. For example, the sub path directory must have at least g+rwx
-				// when the pod specifies an fsGroup, and if the directory is not created here, Docker will
-				// later auto-create it with the incorrect mode 0750
-				// Make extra care not to escape the volume!
-				perm, err := hu.GetMode(volumePath)
 				if err != nil {
 					return nil, cleanupAction, err
 				}
-				if err := subpather.SafeMakeDir(subPath, volumePath, perm); err != nil {
+			}
+
+			if subPath != "" {
+				if utilfs.IsAbs(subPath) {
+					return nil, cleanupAction, fmt.Errorf("error SubPath `%s` must not be an absolute path", subPath)
+				}
+
+				err = volumevalidation.ValidatePathNoBacksteps(subPath)
+				if err != nil {
+					return nil, cleanupAction, fmt.Errorf("unable to provision SubPath `%s`: %w", subPath, err)
+				}
+
+				volumePath := hostPath
+				hostPath = filepath.Join(volumePath, subPath)
+
+				if subPathExists, err := hu.PathExists(hostPath); err != nil {
+					klog.ErrorS(nil, "Could not determine if subPath exists, will not attempt to change its permissions", "path", hostPath)
+				} else if !subPathExists {
+					// Create the sub path now because if it's auto-created later when referenced, it may have an
+					// incorrect ownership and mode. For example, the sub path directory must have at least g+rwx
+					// when the pod specifies an fsGroup, and if the directory is not created here, Docker will
+					// later auto-create it with the incorrect mode 0750
+					// Make extra care not to escape the volume!
+					perm, err := hu.GetMode(volumePath)
+					if err != nil {
+						return nil, cleanupAction, err
+					}
+					if err := subpather.SafeMakeDir(subPath, volumePath, perm); err != nil {
+						// Don't pass detailed error back to the user because it could give information about host filesystem
+						klog.ErrorS(err, "Failed to create subPath directory for volumeMount of the container", "containerName", container.Name, "volumeMountName", mount.Name)
+						return nil, cleanupAction, fmt.Errorf("failed to create subPath directory for volumeMount %q of container %q", mount.Name, container.Name)
+					}
+				}
+				hostPath, cleanupAction, err = subpather.PrepareSafeSubpath(subpath.Subpath{
+					VolumeMountIndex: i,
+					Path:             hostPath,
+					VolumeName:       vol.InnerVolumeSpecName,
+					VolumePath:       volumePath,
+					PodDir:           podDir,
+					ContainerName:    container.Name,
+				})
+				if err != nil {
 					// Don't pass detailed error back to the user because it could give information about host filesystem
-					klog.ErrorS(err, "Failed to create subPath directory for volumeMount of the container", "containerName", container.Name, "volumeMountName", mount.Name)
-					return nil, cleanupAction, fmt.Errorf("failed to create subPath directory for volumeMount %q of container %q", mount.Name, container.Name)
+					klog.ErrorS(err, "Failed to prepare subPath for volumeMount of the container", "containerName", container.Name, "volumeMountName", mount.Name)
+					return nil, cleanupAction, fmt.Errorf("failed to prepare subPath for volumeMount %q of container %q", mount.Name, container.Name)
 				}
 			}
-			hostPath, cleanupAction, err = subpather.PrepareSafeSubpath(subpath.Subpath{
-				VolumeMountIndex: i,
-				Path:             hostPath,
-				VolumeName:       vol.InnerVolumeSpecName,
-				VolumePath:       volumePath,
-				PodDir:           podDir,
-				ContainerName:    container.Name,
-			})
-			if err != nil {
-				// Don't pass detailed error back to the user because it could give information about host filesystem
-				klog.ErrorS(err, "Failed to prepare subPath for volumeMount of the container", "containerName", container.Name, "volumeMountName", mount.Name)
-				return nil, cleanupAction, fmt.Errorf("failed to prepare subPath for volumeMount %q of container %q", mount.Name, container.Name)
-			}
-		}
 
-		// Docker Volume Mounts fail on Windows if it is not of the form C:/
-		if volumeutil.IsWindowsLocalPath(runtime.GOOS, hostPath) {
-			hostPath = volumeutil.MakeAbsolutePath(runtime.GOOS, hostPath)
+			// Docker Volume Mounts fail on Windows if it is not of the form C:/
+			if hostPath != "" && volumeutil.IsWindowsLocalPath(runtime.GOOS, hostPath) {
+				hostPath = volumeutil.MakeAbsolutePath(runtime.GOOS, hostPath)
+			}
 		}
 
 		containerPath := mount.MountPath
@@ -396,6 +397,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
 			Name:              mount.Name,
 			ContainerPath:     containerPath,
 			HostPath:          hostPath,
+			Image:             image,
 			ReadOnly:          mount.ReadOnly || mustMountRO,
 			RecursiveReadOnly: rro,
 			SELinuxRelabel:    relabelVolume,

pkg/kubelet/kubelet_pods_linux_test.go

@@ -20,16 +20,21 @@ limitations under the License.
 package kubelet
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/ptr"
 
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
 	_ "k8s.io/kubernetes/pkg/apis/core/install"
+	"k8s.io/kubernetes/pkg/features"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/volume"
 	volumetest "k8s.io/kubernetes/pkg/volume/testing"
 	"k8s.io/kubernetes/pkg/volume/util/hostutil"
 	"k8s.io/kubernetes/pkg/volume/util/subpath"
@@ -40,16 +45,21 @@ func TestMakeMounts(t *testing.T) {
 	propagationHostToContainer := v1.MountPropagationHostToContainer
 	propagationBidirectional := v1.MountPropagationBidirectional
 	propagationNone := v1.MountPropagationNone
+	image1 := &runtimeapi.ImageSpec{Image: "image1"}
+	image2 := &runtimeapi.ImageSpec{Image: "image2"}
 
 	testCases := map[string]struct {
-		container      v1.Container
-		podVolumes     kubecontainer.VolumeMap
-		supportsRRO    bool
-		expectErr      bool
-		expectedErrMsg string
-		expectedMounts []kubecontainer.Mount
+		container                 v1.Container
+		podVolumes                kubecontainer.VolumeMap
+		imageVolumes              kubecontainer.ImageVolumes
+		imageVolumeFeatureEnabled []bool
+		supportsRRO               bool
+		expectErr                 bool
+		expectedErrMsg            string
+		expectedMounts            []kubecontainer.Mount
 	}{
-		"valid mounts in unprivileged container": {
+		"valid mounts in unprivileged container": { // TODO: remove it once image volume feature is GA
+			imageVolumeFeatureEnabled: []bool{true, false},
 			podVolumes: kubecontainer.VolumeMap{
 				"disk":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
 				"disk4": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/host"}},
@@ -118,7 +128,8 @@ func TestMakeMounts(t *testing.T) {
 			},
 			expectErr: false,
 		},
-		"valid mounts in privileged container": {
+		"valid mounts in privileged container": { // TODO: remove it once image volume feature is GA
+			imageVolumeFeatureEnabled: []bool{true, false},
 			podVolumes: kubecontainer.VolumeMap{
 				"disk":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
 				"disk4": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/host"}},
@@ -177,7 +188,198 @@ func TestMakeMounts(t *testing.T) {
 			},
 			expectErr: false,
 		},
+		"valid mounts in unprivileged container with image volumes": {
+			imageVolumeFeatureEnabled: []bool{true},
+			podVolumes: kubecontainer.VolumeMap{
+				"disk":   kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
+				"disk4":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/host"}},
+				"disk5":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/var/lib/kubelet/podID/volumes/empty/disk5"}},
+				"image1": kubecontainer.VolumeInfo{Mounter: &stubVolume{attributes: volume.Attributes{ReadOnly: true}}},
+				"image2": kubecontainer.VolumeInfo{Mounter: &stubVolume{attributes: volume.Attributes{ReadOnly: true}}},
+			},
+			imageVolumes: kubecontainer.ImageVolumes{
+				"image1": image1,
+				"image2": image2,
+			},
+			container: v1.Container{
+				Name: "container1",
+				VolumeMounts: []v1.VolumeMount{
+					{
+						MountPath:        "/etc/hosts",
+						Name:             "disk",
+						ReadOnly:         false,
+						MountPropagation: &propagationHostToContainer,
+					},
+					{
+						MountPath:        "/mnt/path3",
+						Name:             "disk",
+						ReadOnly:         true,
+						MountPropagation: &propagationNone,
+					},
+					{
+						MountPath: "/mnt/path4",
+						Name:      "disk4",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/path5",
+						Name:      "disk5",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/image1",
+						Name:      "image1",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/image2",
+						Name:      "image2",
+						ReadOnly:  true,
+					},
+				},
+			},
+			expectedMounts: []kubecontainer.Mount{
+				{
+					Name:           "disk",
+					ContainerPath:  "/etc/hosts",
+					HostPath:       "/mnt/disk",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_HOST_TO_CONTAINER,
+				},
+				{
+					Name:           "disk",
+					ContainerPath:  "/mnt/path3",
+					HostPath:       "/mnt/disk",
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_PRIVATE,
+				},
+				{
+					Name:           "disk4",
+					ContainerPath:  "/mnt/path4",
+					HostPath:       "/mnt/host",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_PRIVATE,
+				},
+				{
+					Name:           "disk5",
+					ContainerPath:  "/mnt/path5",
+					HostPath:       "/var/lib/kubelet/podID/volumes/empty/disk5",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_PRIVATE,
+				},
+				{
+					Name:           "image1",
+					ContainerPath:  "/mnt/image1",
+					Image:          image1,
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+				},
+				{
+					Name:           "image2",
+					ContainerPath:  "/mnt/image2",
+					Image:          image2,
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+				},
+			},
+			expectErr: false,
+		},
+		"valid mounts in privileged container with image volumes": {
+			imageVolumeFeatureEnabled: []bool{true},
+			podVolumes: kubecontainer.VolumeMap{
+				"disk":   kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
+				"disk4":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/host"}},
+				"disk5":  kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/var/lib/kubelet/podID/volumes/empty/disk5"}},
+				"image1": kubecontainer.VolumeInfo{Mounter: &stubVolume{attributes: volume.Attributes{ReadOnly: true}}},
+				"image2": kubecontainer.VolumeInfo{Mounter: &stubVolume{attributes: volume.Attributes{ReadOnly: true}}},
+			},
+			imageVolumes: kubecontainer.ImageVolumes{
+				"image1": image1,
+				"image2": image2,
+			},
+			container: v1.Container{
+				Name: "container1",
+				VolumeMounts: []v1.VolumeMount{
+					{
+						MountPath:        "/etc/hosts",
+						Name:             "disk",
+						ReadOnly:         false,
+						MountPropagation: &propagationBidirectional,
+					},
+					{
+						MountPath:        "/mnt/path3",
+						Name:             "disk",
+						ReadOnly:         true,
+						MountPropagation: &propagationHostToContainer,
+					},
+					{
+						MountPath: "/mnt/path4",
+						Name:      "disk4",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/image1",
+						Name:      "image1",
+						ReadOnly:  false,
+					},
+					{
+						MountPath: "/mnt/image2",
+						Name:      "image2",
+						ReadOnly:  true,
+					},
+				},
+				SecurityContext: &v1.SecurityContext{
+					Privileged: &bTrue,
+				},
+			},
+			expectedMounts: []kubecontainer.Mount{
+				{
+					Name:           "disk",
+					ContainerPath:  "/etc/hosts",
+					HostPath:       "/mnt/disk",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_BIDIRECTIONAL,
+				},
+				{
+					Name:           "disk",
+					ContainerPath:  "/mnt/path3",
+					HostPath:       "/mnt/disk",
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_HOST_TO_CONTAINER,
+				},
+				{
+					Name:           "disk4",
+					ContainerPath:  "/mnt/path4",
+					HostPath:       "/mnt/host",
+					ReadOnly:       false,
+					SELinuxRelabel: false,
+					Propagation:    runtimeapi.MountPropagation_PROPAGATION_PRIVATE,
+				},
+				{
+					Name:           "image1",
+					ContainerPath:  "/mnt/image1",
+					Image:          image1,
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+				},
+				{
+					Name:           "image2",
+					ContainerPath:  "/mnt/image2",
+					Image:          image2,
+					ReadOnly:       true,
+					SELinuxRelabel: false,
+				},
+			},
+			expectErr: false,
+		},
 		"invalid absolute SubPath": {
+			imageVolumeFeatureEnabled: []bool{true, false},
 			podVolumes: kubecontainer.VolumeMap{
 				"disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
 			},
@@ -195,6 +397,7 @@ func TestMakeMounts(t *testing.T) {
 			expectedErrMsg: "error SubPath `/must/not/be/absolute` must not be an absolute path",
 		},
 		"invalid SubPath with backsteps": {
+			imageVolumeFeatureEnabled: []bool{true, false},
 			podVolumes: kubecontainer.VolumeMap{
 				"disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "/mnt/disk"}},
 			},
@@ -212,7 +415,7 @@ func TestMakeMounts(t *testing.T) {
 			expectedErrMsg: "unable to provision SubPath `no/backsteps/../allowed`: must not contain '..'",
 		},
 		"volume doesn't exist": {
-			podVolumes: kubecontainer.VolumeMap{},
+			imageVolumeFeatureEnabled: []bool{true, false},
 			container: v1.Container{
 				VolumeMounts: []v1.VolumeMount{
 					{
@@ -226,6 +429,7 @@ func TestMakeMounts(t *testing.T) {
 			expectedErrMsg: "cannot find volume \"disk\" to mount into container \"\"",
 		},
 		"volume mounter is nil": {
+			imageVolumeFeatureEnabled: []bool{true, false},
 			podVolumes: kubecontainer.VolumeMap{
 				"disk": kubecontainer.VolumeInfo{},
 			},
@@ -244,32 +448,36 @@ func TestMakeMounts(t *testing.T) {
 	}
 
 	for name, tc := range testCases {
-		t.Run(name, func(t *testing.T) {
-			fhu := hostutil.NewFakeHostUtil(nil)
-			fsp := &subpath.FakeSubpath{}
-			pod := v1.Pod{
-				Spec: v1.PodSpec{
-					HostNetwork: true,
-				},
-			}
-
-			mounts, _, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", []string{""}, tc.podVolumes, fhu, fsp, nil, tc.supportsRRO, nil)
-
-			// validate only the error if we expect an error
-			if tc.expectErr {
-				if err == nil || err.Error() != tc.expectedErrMsg {
-					t.Fatalf("expected error message `%s` but got `%v`", tc.expectedErrMsg, err)
+		for _, featureEnabled := range tc.imageVolumeFeatureEnabled {
+			name := fmt.Sprintf("features.ImageVolume is %v, %s", featureEnabled, name)
+			t.Run(name, func(t *testing.T) {
+				featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ImageVolume, featureEnabled)
+				fhu := hostutil.NewFakeHostUtil(nil)
+				fsp := &subpath.FakeSubpath{}
+				pod := v1.Pod{
+					Spec: v1.PodSpec{
+						HostNetwork: true,
+					},
 				}
-				return
-			}
 
-			// otherwise validate the mounts
-			if err != nil {
-				t.Fatal(err)
-			}
+				mounts, _, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", []string{""}, tc.podVolumes, fhu, fsp, nil, tc.supportsRRO, tc.imageVolumes)
 
-			assert.Equal(t, tc.expectedMounts, mounts, "mounts of container %+v", tc.container)
-		})
+				// validate only the error if we expect an error
+				if tc.expectErr {
+					if err == nil || err.Error() != tc.expectedErrMsg {
+						t.Fatalf("expected error message `%s` but got `%v`", tc.expectedErrMsg, err)
+					}
+					return
+				}
+
+				// otherwise validate the mounts
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				assert.Equal(t, tc.expectedMounts, mounts, "mounts of container %+v", tc.container)
+			})
+		}
 	}
 }
 

pkg/kubelet/kubelet_volumes_test.go

@@ -689,7 +689,8 @@ func TestVolumeUnmountAndDetachControllerEnabled(t *testing.T) {
 }
 
 type stubVolume struct {
-	path string
+	path       string
+	attributes volume.Attributes
 	volume.MetricsNil
 }
 
@@ -698,7 +699,7 @@ func (f *stubVolume) GetPath() string {
 }
 
 func (f *stubVolume) GetAttributes() volume.Attributes {
-	return volume.Attributes{}
+	return f.attributes
 }
 
 func (f *stubVolume) SetUp(mounterArgs volume.MounterArgs) error {

pkg/volume/image/image.go

@@ -67,8 +67,8 @@ func (o *imagePlugin) ConstructVolumeSpec(volumeName, mountPath string) (volume.
 func (o *imagePlugin) GetAttributes() volume.Attributes {
 	return volume.Attributes{
 		ReadOnly:       true,
-		Managed:        true,
-		SELinuxRelabel: true,
+		Managed:        false,
+		SELinuxRelabel: false,
 	}
 }