diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index e95a0fcc155..c8d77e5e927 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -286,6 +286,13 @@ const ( // // Extend the default scheduler to be aware of volume topology and handle PV provisioning DynamicProvisioningScheduling utilfeature.Feature = "DynamicProvisioningScheduling" + + // owner: @kevtaylor + // alpha: v1.11 + // + // Allow subpath environment variable substitution + // Only applicable if the VolumeSubpath feature is also enabled + VolumeSubpathEnvExpansion utilfeature.Feature = "VolumeSubpathEnvExpansion" ) func init() { @@ -335,6 +342,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS VolumeSubpath: {Default: true, PreRelease: utilfeature.GA}, BalanceAttachedNodeVolumes: {Default: false, PreRelease: utilfeature.Alpha}, DynamicProvisioningScheduling: {Default: false, PreRelease: utilfeature.Alpha}, + VolumeSubpathEnvExpansion: {Default: false, PreRelease: utilfeature.Alpha}, // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: diff --git a/pkg/kubelet/container/helpers.go b/pkg/kubelet/container/helpers.go index 399fa959f45..b1299942f98 100644 --- a/pkg/kubelet/container/helpers.go +++ b/pkg/kubelet/container/helpers.go @@ -133,6 +133,11 @@ func ExpandContainerCommandOnlyStatic(containerCommand []string, envs []v1.EnvVa return command } +func ExpandContainerVolumeMounts(mount v1.VolumeMount, envs []EnvVar) (expandedSubpath string) { + mapping := expansion.MappingFuncFor(EnvVarsToMap(envs)) + return expansion.Expand(mount.SubPath, mapping) +} + func ExpandContainerCommandAndArgs(container *v1.Container, envs []EnvVar) (command []string, args []string) { mapping := expansion.MappingFuncFor(EnvVarsToMap(envs)) diff --git a/pkg/kubelet/container/helpers_test.go b/pkg/kubelet/container/helpers_test.go index 14d9d6e6c8c..d6c2792c60b 100644 --- a/pkg/kubelet/container/helpers_test.go +++ b/pkg/kubelet/container/helpers_test.go @@ -138,6 +138,114 @@ func TestExpandCommandAndArgs(t *testing.T) { } } +func TestExpandVolumeMountsWithSubpath(t *testing.T) { + cases := []struct { + name string + container *v1.Container + envs []EnvVar + expectedSubPath string + expectedMountPath string + }{ + { + name: "subpath with no expansion", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{SubPath: "foo"}}, + }, + expectedSubPath: "foo", + expectedMountPath: "", + }, + { + name: "volumes with expanded subpath", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{SubPath: "foo/$(POD_NAME)"}}, + }, + envs: []EnvVar{ + { + Name: "POD_NAME", + Value: "bar", + }, + }, + expectedSubPath: "foo/bar", + expectedMountPath: "", + }, + { + name: "volumes expanded with empty subpath", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{SubPath: ""}}, + }, + envs: []EnvVar{ + { + Name: "POD_NAME", + Value: "bar", + }, + }, + expectedSubPath: "", + expectedMountPath: "", + }, + { + name: "volumes expanded with no envs subpath", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{SubPath: "/foo/$(POD_NAME)"}}, + }, + expectedSubPath: "/foo/$(POD_NAME)", + expectedMountPath: "", + }, + { + name: "volumes expanded with leading environment variable", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{SubPath: "$(POD_NAME)/bar"}}, + }, + envs: []EnvVar{ + { + Name: "POD_NAME", + Value: "foo", + }, + }, + expectedSubPath: "foo/bar", + expectedMountPath: "", + }, + { + name: "volumes with volume and subpath", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{MountPath: "/foo", SubPath: "$(POD_NAME)/bar"}}, + }, + envs: []EnvVar{ + { + Name: "POD_NAME", + Value: "foo", + }, + }, + expectedSubPath: "foo/bar", + expectedMountPath: "/foo", + }, + { + name: "volumes with volume and no subpath", + container: &v1.Container{ + VolumeMounts: []v1.VolumeMount{{MountPath: "/foo"}}, + }, + envs: []EnvVar{ + { + Name: "POD_NAME", + Value: "foo", + }, + }, + expectedSubPath: "", + expectedMountPath: "/foo", + }, + } + + for _, tc := range cases { + actualSubPath := ExpandContainerVolumeMounts(tc.container.VolumeMounts[0], tc.envs) + if e, a := tc.expectedSubPath, actualSubPath; !reflect.DeepEqual(e, a) { + t.Errorf("%v: unexpected subpath; expected %v, got %v", tc.name, e, a) + } + if e, a := tc.expectedMountPath, tc.container.VolumeMounts[0].MountPath; !reflect.DeepEqual(e, a) { + t.Errorf("%v: unexpected mountpath; expected %v, got %v", tc.name, e, a) + } + } + +} + func TestShouldContainerBeRestarted(t *testing.T) { pod := &v1.Pod{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 46a17a01a58..50c28c0ebec 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -129,7 +129,8 @@ func (kl *Kubelet) makeBlockVolumes(pod *v1.Pod, container *v1.Container, podVol } // makeMounts determines the mount points for the given container. -func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, hostDomain, podIP string, podVolumes kubecontainer.VolumeMap, mounter mountutil.Interface) ([]kubecontainer.Mount, func(), error) { +func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, hostDomain, podIP string, podVolumes kubecontainer.VolumeMap, mounter mountutil.Interface, expandEnvs []kubecontainer.EnvVar) ([]kubecontainer.Mount, func(), error) { + // Kubernetes only mounts on /etc/hosts if: // - container is not an infrastructure (pause) container // - container is not already mounting on /etc/hosts @@ -166,6 +167,11 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h return nil, cleanupAction, fmt.Errorf("volume subpaths are disabled") } + // Expand subpath variables + if utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpathEnvExpansion) { + mount.SubPath = kubecontainer.ExpandContainerVolumeMounts(mount, expandEnvs) + } + if filepath.IsAbs(mount.SubPath) { return nil, cleanupAction, fmt.Errorf("error SubPath `%s` must not be an absolute path", mount.SubPath) } @@ -454,18 +460,18 @@ func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Contai opts.Devices = append(opts.Devices, blkVolumes...) } - mounts, cleanupAction, err := makeMounts(pod, kl.getPodDir(pod.UID), container, hostname, hostDomainName, podIP, volumes, kl.mounter) + envs, err := kl.makeEnvironmentVariables(pod, container, podIP) + if err != nil { + return nil, nil, err + } + opts.Envs = append(opts.Envs, envs...) + + mounts, cleanupAction, err := makeMounts(pod, kl.getPodDir(pod.UID), container, hostname, hostDomainName, podIP, volumes, kl.mounter, opts.Envs) if err != nil { return nil, cleanupAction, err } opts.Mounts = append(opts.Mounts, mounts...) - envs, err := kl.makeEnvironmentVariables(pod, container, podIP) - if err != nil { - return nil, cleanupAction, err - } - opts.Envs = append(opts.Envs, envs...) - // Disabling adding TerminationMessagePath on Windows as these files would be mounted as docker volume and // Docker for Windows has a bug where only directories can be mounted if len(container.TerminationMessagePath) != 0 && runtime.GOOS != "windows" { diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index 9d2c813848b..6a26e20550e 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -273,7 +273,7 @@ func TestMakeMounts(t *testing.T) { return } - mounts, _, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", "", tc.podVolumes, fm) + mounts, _, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", "", tc.podVolumes, fm, nil) // validate only the error if we expect an error if tc.expectErr { @@ -296,7 +296,7 @@ func TestMakeMounts(t *testing.T) { t.Errorf("Failed to enable feature gate for MountPropagation: %v", err) return } - mounts, _, err = makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", "", tc.podVolumes, fm) + mounts, _, err = makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", "", tc.podVolumes, fm, nil) if !tc.expectErr { expectedPrivateMounts := []kubecontainer.Mount{} for _, mount := range tc.expectedMounts { @@ -357,7 +357,7 @@ func TestDisabledSubpath(t *testing.T) { defer utilfeature.DefaultFeatureGate.Set("VolumeSubpath=true") for name, test := range cases { - _, _, err := makeMounts(&pod, "/pod", &test.container, "fakepodname", "", "", podVolumes, fm) + _, _, err := makeMounts(&pod, "/pod", &test.container, "fakepodname", "", "", podVolumes, fm, nil) if err != nil && !test.expectError { t.Errorf("test %v failed: %v", name, err) } diff --git a/pkg/kubelet/kubelet_pods_windows_test.go b/pkg/kubelet/kubelet_pods_windows_test.go index cc16b358fb0..628c2ecdd7a 100644 --- a/pkg/kubelet/kubelet_pods_windows_test.go +++ b/pkg/kubelet/kubelet_pods_windows_test.go @@ -66,7 +66,7 @@ func TestMakeMountsWindows(t *testing.T) { } fm := &mount.FakeMounter{} - mounts, _, _ := makeMounts(&pod, "/pod", &container, "fakepodname", "", "", podVolumes, fm) + mounts, _, _ := makeMounts(&pod, "/pod", &container, "fakepodname", "", "", podVolumes, fm, nil) expectedMounts := []kubecontainer.Mount{ { diff --git a/test/e2e/common/expansion.go b/test/e2e/common/expansion.go index eadb324fecf..17123dd8c87 100644 --- a/test/e2e/common/expansion.go +++ b/test/e2e/common/expansion.go @@ -19,8 +19,13 @@ package common import ( "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/kubernetes/test/e2e/framework" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "time" ) // These tests exercise the Kubernetes expansion syntax $(VAR). @@ -144,4 +149,188 @@ var _ = framework.KubeDescribe("Variable Expansion", func() { "test-value", }) }) + + /* + Testname: var-expansion-subpath + Description: Make sure a container's subpath can be set using an + expansion of environment variables. + */ + It("should allow substituting values in a volume subpath [Feature:VolumeSubpathEnvExpansion][NodeAlphaFeature:VolumeSubpathEnvExpansion]", func() { + podName := "var-expansion-" + string(uuid.NewUUID()) + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + Labels: map[string]string{"name": podName}, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "dapi-container", + Image: busyboxImage, + Command: []string{"sh", "-c", "test -d /testcontainer/" + podName + ";echo $?"}, + Env: []v1.EnvVar{ + { + Name: "POD_NAME", + Value: podName, + }, + }, + VolumeMounts: []v1.VolumeMount{ + { + Name: "workdir1", + MountPath: "/logscontainer", + SubPath: "$(POD_NAME)", + }, + { + Name: "workdir2", + MountPath: "/testcontainer", + }, + }, + }, + }, + RestartPolicy: v1.RestartPolicyNever, + Volumes: []v1.Volume{ + { + Name: "workdir1", + VolumeSource: v1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{Path: "/tmp"}, + }, + }, + { + Name: "workdir2", + VolumeSource: v1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{Path: "/tmp"}, + }, + }, + }, + }, + } + + f.TestContainerOutput("substitution in volume subpath", pod, 0, []string{ + "0", + }) + }) + + /* + Testname: var-expansion-subpath-with-backticks + Description: Make sure a container's subpath can not be set using an + expansion of environment variables when backticks are supplied. + */ + It("should fail substituting values in a volume subpath with backticks [Feature:VolumeSubpathEnvExpansion][NodeAlphaFeature:VolumeSubpathEnvExpansion][Slow]", func() { + + podName := "var-expansion-" + string(uuid.NewUUID()) + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + Labels: map[string]string{"name": podName}, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "dapi-container", + Image: busyboxImage, + Env: []v1.EnvVar{ + { + Name: "POD_NAME", + Value: "..", + }, + }, + VolumeMounts: []v1.VolumeMount{ + { + Name: "workdir1", + MountPath: "/logscontainer", + SubPath: "$(POD_NAME)", + }, + }, + }, + }, + RestartPolicy: v1.RestartPolicyNever, + Volumes: []v1.Volume{ + { + Name: "workdir1", + VolumeSource: v1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + }, + }, + }, + } + + // Pod should fail + testPodFailSubpath(f, pod, "SubPath `..`: must not contain '..'") + }) + + /* + Testname: var-expansion-subpath-with-absolute-path + Description: Make sure a container's subpath can not be set using an + expansion of environment variables when absoluete path is supplied. + */ + It("should fail substituting values in a volume subpath with absolute path [Feature:VolumeSubpathEnvExpansion][NodeAlphaFeature:VolumeSubpathEnvExpansion][Slow]", func() { + + podName := "var-expansion-" + string(uuid.NewUUID()) + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + Labels: map[string]string{"name": podName}, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "dapi-container", + Image: busyboxImage, + Env: []v1.EnvVar{ + { + Name: "POD_NAME", + Value: "/tmp", + }, + }, + VolumeMounts: []v1.VolumeMount{ + { + Name: "workdir1", + MountPath: "/logscontainer", + SubPath: "$(POD_NAME)", + }, + }, + }, + }, + RestartPolicy: v1.RestartPolicyNever, + Volumes: []v1.Volume{ + { + Name: "workdir1", + VolumeSource: v1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + }, + }, + }, + } + + // Pod should fail + testPodFailSubpath(f, pod, "SubPath `/tmp` must not be an absolute path") + }) }) + +func testPodFailSubpath(f *framework.Framework, pod *v1.Pod, errorText string) { + + pod, err := f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(pod) + Expect(err).ToNot(HaveOccurred(), "while creating pod") + + defer func() { + framework.DeletePodWithWait(f, f.ClientSet, pod) + }() + + err = framework.WaitTimeoutForPodRunningInNamespace(f.ClientSet, pod.Name, pod.Namespace, 30*time.Second) + Expect(err).To(HaveOccurred(), "while waiting for pod to be running") + + selector := fields.Set{ + "involvedObject.kind": "Pod", + "involvedObject.name": pod.Name, + "involvedObject.namespace": f.Namespace.Name, + "reason": "Failed", + }.AsSelector().String() + + options := metav1.ListOptions{FieldSelector: selector} + events, err := f.ClientSet.CoreV1().Events(f.Namespace.Name).List(options) + Expect(err).NotTo(HaveOccurred(), "while getting pod events") + Expect(len(events.Items)).NotTo(Equal(0), "no events found") + Expect(events.Items[0].Message).To(ContainSubstring(errorText), "subpath error not found") +}