From 2529d7d5a65e2b5d1a5f4a6de5ad12eeb9ad4ee8 Mon Sep 17 00:00:00 2001 From: carlory Date: Tue, 5 Nov 2024 15:08:52 +0800 Subject: [PATCH 1/3] TestMakeMounts: add new cases for the image volume feature --- pkg/kubelet/kubelet_pods_linux_test.go | 272 ++++++++++++++++++++++--- pkg/kubelet/kubelet_volumes_test.go | 5 +- 2 files changed, 243 insertions(+), 34 deletions(-) diff --git a/pkg/kubelet/kubelet_pods_linux_test.go b/pkg/kubelet/kubelet_pods_linux_test.go index 798d8e167b2..7b76d0a1932 100644 --- a/pkg/kubelet/kubelet_pods_linux_test.go +++ b/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) + }) + } } } diff --git a/pkg/kubelet/kubelet_volumes_test.go b/pkg/kubelet/kubelet_volumes_test.go index 30509115028..bc322f6302b 100644 --- a/pkg/kubelet/kubelet_volumes_test.go +++ b/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 { From b6c9c2d6fa6cd812131d19567a86ae7126dcd2dd Mon Sep 17 00:00:00 2001 From: carlory Date: Tue, 5 Nov 2024 16:35:59 +0800 Subject: [PATCH 2/3] 1. When the kubelet constructs the cri mounts for the container which references an `image` volume source type, It passes the missing mount attributes to the CRI implementation, including `readOnly`, `propagation`, and `recursiveReadOnly`. When the readOnly field of the containerMount is explicitly set to false, the kubelet will take the `readOnly`as true to the CRI implementation because the image volume plugin requires the mount to be read-only. 2. Fix a bug where the pod is unexpectedly running when the `image` volume source type is used and mounted to `/etc/hosts` in the container. --- pkg/kubelet/kubelet_pods.go | 124 ++++++++++++++++++------------------ pkg/volume/image/image.go | 4 +- 2 files changed, 65 insertions(+), 63 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 6072a48027c..9e016b2f77c 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/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`: %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 { + // 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, diff --git a/pkg/volume/image/image.go b/pkg/volume/image/image.go index fcadcb903cf..ac742e7cafc 100644 --- a/pkg/volume/image/image.go +++ b/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, } } From c6e6fadc487f2b99ff14c4c743dbce2e15e4531b Mon Sep 17 00:00:00 2001 From: carlory Date: Tue, 5 Nov 2024 21:26:00 +0800 Subject: [PATCH 3/3] fix pull-kubernetes-linter-hints --- pkg/kubelet/kubelet_pods.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 9e016b2f77c..a549e2b585b 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -327,7 +327,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h err = volumevalidation.ValidatePathNoBacksteps(subPath) if err != nil { - return nil, cleanupAction, fmt.Errorf("unable to provision SubPath `%s`: %v", subPath, err) + return nil, cleanupAction, fmt.Errorf("unable to provision SubPath `%s`: %w", subPath, err) } volumePath := hostPath