Merge pull request #10150 from chlins/fix/data-mover-drop-host-mounts

Drop node-agent host path mounts from data mover pods

changelogs/unreleased/10150-chlins

@@ -0,0 +1 @@
+Drop node-agent host path mounts from data mover pods

pkg/exposer/csi_snapshot.go

@@ -1,5 +1,5 @@
 /*
-Copyright The Velero Contributors.
+Copyright the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -684,7 +684,9 @@ func (e *csiSnapshotExposer) createBackupPod(
 	containerName := string(ownerObject.UID)
 	volumeName := string(ownerObject.UID)
 
-	podInfo, err := getInheritedPodInfo(ctx, e.kubeClient, ownerObject.Namespace, nodeOS)
+	// The backup pod reads the data through the backup PVC only, so the node-agent's host
+	// path volumes to the kubelet root directory are not inherited.
+	podInfo, err := getInheritedPodInfo(ctx, e.kubeClient, ownerObject.Namespace, nodeOS, excludeHostPathVolumes)
 	if err != nil {
 		return nil, errors.Wrap(err, "error to get inherited pod info from node-agent")
 	}

pkg/exposer/generic_restore.go

@@ -1,5 +1,5 @@
 /*
-Copyright The Velero Contributors.
+Copyright the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -628,7 +628,9 @@ func (e *genericRestoreExposer) createRestorePod(
 		affinity = &kube.LoadAffinity{}
 	}
 
-	podInfo, err := getInheritedPodInfo(ctx, e.kubeClient, ownerObject.Namespace, nodeOS)
+	// The restore pod writes the data through the restore PVC only, so the node-agent's host
+	// path volumes to the kubelet root directory are not inherited.
+	podInfo, err := getInheritedPodInfo(ctx, e.kubeClient, ownerObject.Namespace, nodeOS, excludeHostPathVolumes)
 	if err != nil {
 		return nil, errors.Wrap(err, "error to get inherited pod info from node-agent")
 	}

pkg/exposer/image.go

@@ -1,5 +1,5 @@
 /*
-Copyright The Velero Contributors.
+Copyright the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -27,6 +27,18 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/nodeagent"
 )
 
+const (
+	// excludeHostPathVolumes indicates that the volumes backed by a host path are not
+	// inherited from the node-agent. The exposers accessing data through PVCs use it so
+	// that the hosting pods don't get unnecessary access to the host file system.
+	excludeHostPathVolumes = true
+
+	// inheritHostPathVolumes indicates that the volumes backed by a host path are
+	// inherited from the node-agent. fs-backup uses it because it resolves and accesses
+	// the pod volume data through the kubelet pod directory on the host.
+	inheritHostPathVolumes = false
+)
+
 type inheritedPodInfo struct {
 	image            string
 	serviceAccount   string
@@ -41,7 +53,12 @@ type inheritedPodInfo struct {
 	imagePullSecrets []corev1api.LocalObjectReference
 }
 
-func getInheritedPodInfo(ctx context.Context, client kubernetes.Interface, veleroNamespace string, osType string) (inheritedPodInfo, error) {
+// getInheritedPodInfo collects the pod info to be inherited by the hosting pods from the
+// node-agent pod template. When excludeHostPath is true, the volumes backed by a host path,
+// together with their volume mounts, are dropped from the result. The volumes are detected
+// by their source instead of their name, so the ones customized in the node-agent daemonset
+// are covered as well.
+func getInheritedPodInfo(ctx context.Context, client kubernetes.Interface, veleroNamespace string, osType string, excludeHostPath bool) (inheritedPodInfo, error) {
 	podInfo := inheritedPodInfo{}
 
 	podSpec, err := nodeagent.GetPodSpec(ctx, client, veleroNamespace, osType)
@@ -58,8 +75,7 @@ func getInheritedPodInfo(ctx context.Context, client kubernetes.Interface, veler
 
 	podInfo.env = podSpec.Containers[0].Env
 	podInfo.envFrom = podSpec.Containers[0].EnvFrom
-	podInfo.volumeMounts = podSpec.Containers[0].VolumeMounts
-	podInfo.volumes = podSpec.Volumes
+	podInfo.volumeMounts, podInfo.volumes = filterVolumes(podSpec.Containers[0].VolumeMounts, podSpec.Volumes, excludeHostPath)
 
 	podInfo.dnsPolicy = podSpec.DNSPolicy
 	podInfo.dnsConfig = podSpec.DNSConfig
@@ -81,3 +97,35 @@ func getInheritedPodInfo(ctx context.Context, client kubernetes.Interface, veler
 
 	return podInfo, nil
 }
+
+// filterVolumes removes the volumes backed by a host path, as well as the volume mounts
+// referring to them, when excludeHostPath is true. The volumes are recognized by their
+// source, so the host path volumes customized in the node-agent daemonset are removed as
+// well. The other volumes, including the ones customized by users, are kept as is.
+func filterVolumes(volumeMounts []corev1api.VolumeMount, volumes []corev1api.Volume, excludeHostPath bool) ([]corev1api.VolumeMount, []corev1api.Volume) {
+	if !excludeHostPath {
+		return volumeMounts, volumes
+	}
+
+	excluded := make(map[string]struct{})
+	retainedVolumes := make([]corev1api.Volume, 0, len(volumes))
+	for _, volume := range volumes {
+		if volume.HostPath != nil {
+			excluded[volume.Name] = struct{}{}
+			continue
+		}
+
+		retainedVolumes = append(retainedVolumes, volume)
+	}
+
+	retainedMounts := make([]corev1api.VolumeMount, 0, len(volumeMounts))
+	for _, volumeMount := range volumeMounts {
+		if _, found := excluded[volumeMount.Name]; found {
+			continue
+		}
+
+		retainedMounts = append(retainedMounts, volumeMount)
+	}
+
+	return retainedMounts, retainedVolumes
+}

pkg/exposer/image_daemonset_test.go

@@ -0,0 +1,91 @@
+/*
+Copyright the Velero contributors.
+
+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 exposer
+
+import (
+	"context"
+	"testing"
+
+	appsv1api "k8s.io/api/apps/v1"
+	corev1api "k8s.io/api/core/v1"
+	"k8s.io/client-go/kubernetes/fake"
+
+	"github.com/vmware-tanzu/velero/pkg/install"
+)
+
+// TestInheritedPodInfoAgainstRealDaemonSet guards the exclusion against the node-agent
+// daemonset that is actually installed, so that a host path volume added to the daemonset
+// later is not silently inherited by the data mover pods.
+func TestInheritedPodInfoAgainstRealDaemonSet(t *testing.T) {
+	nodeAgent := install.DaemonSet("velero")
+	client := fake.NewSimpleClientset(&appsv1api.DaemonSet{
+		ObjectMeta: nodeAgent.ObjectMeta,
+		Spec:       nodeAgent.Spec,
+	})
+
+	hostPathVolumes := func(volumes []corev1api.Volume) []string {
+		names := []string{}
+		for _, volume := range volumes {
+			if volume.HostPath != nil {
+				names = append(names, volume.Name)
+			}
+		}
+		return names
+	}
+
+	// The installed daemonset must carry host path volumes, otherwise this test is vacuous.
+	if len(hostPathVolumes(nodeAgent.Spec.Template.Spec.Volumes)) == 0 {
+		t.Fatal("the installed node-agent daemonset is expected to have host path volumes")
+	}
+
+	// fs-backup resolves pod volume data through the kubelet pod directory, so it keeps them.
+	fsBackupInfo, err := getInheritedPodInfo(context.Background(), client, "velero", "linux", inheritHostPathVolumes)
+	if err != nil {
+		t.Fatalf("error to get inherited pod info for fs-backup: %v", err)
+	}
+
+	if len(hostPathVolumes(fsBackupInfo.volumes)) == 0 {
+		t.Error("fs-backup is expected to inherit the host path volumes")
+	}
+
+	// The data mover pods access data through PVCs, so they must not get any host path.
+	dataMoverInfo, err := getInheritedPodInfo(context.Background(), client, "velero", "linux", excludeHostPathVolumes)
+	if err != nil {
+		t.Fatalf("error to get inherited pod info for data mover: %v", err)
+	}
+
+	if inherited := hostPathVolumes(dataMoverInfo.volumes); len(inherited) > 0 {
+		t.Errorf("data mover pods are not expected to inherit host path volumes, but got %v", inherited)
+	}
+
+	// The other volumes, e.g., the scratch volume, are still required.
+	if len(dataMoverInfo.volumes) == 0 {
+		t.Error("data mover pods are expected to inherit the volumes other than the host path ones")
+	}
+
+	// Every remaining mount must still have its backing volume.
+	volumeNames := map[string]struct{}{}
+	for _, volume := range dataMoverInfo.volumes {
+		volumeNames[volume.Name] = struct{}{}
+	}
+
+	for _, volumeMount := range dataMoverInfo.volumeMounts {
+		if _, exist := volumeNames[volumeMount.Name]; !exist {
+			t.Errorf("volume mount %q doesn't have a backing volume", volumeMount.Name)
+		}
+	}
+}

pkg/exposer/image_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright The Velero Contributors.
+Copyright the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes"
 
+	"github.com/vmware-tanzu/velero/pkg/nodeagent"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 
 	appsv1api "k8s.io/api/apps/v1"
@@ -187,16 +188,132 @@ func TestGetInheritedPodInfo(t *testing.T) {
 		},
 	}
 
+	daemonSetWithHostPath := &appsv1api.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "fake-ns",
+			Name:      "node-agent",
+		},
+		TypeMeta: metav1.TypeMeta{
+			Kind: "DaemonSet",
+		},
+		Spec: appsv1api.DaemonSetSpec{
+			Template: corev1api.PodTemplateSpec{
+				Spec: corev1api.PodSpec{
+					Containers: []corev1api.Container{
+						{
+							Name:  "container-1",
+							Image: "image-1",
+							VolumeMounts: []corev1api.VolumeMount{
+								{
+									Name:      nodeagent.HostPodVolumeMount,
+									MountPath: "/host_pods",
+								},
+								{
+									Name:      "host-plugins",
+									MountPath: "/var/lib/kubelet/plugins",
+								},
+								{
+									Name:      "customized-host-path",
+									MountPath: "/customized",
+								},
+								{
+									Name:      "scratch",
+									MountPath: "/scratch",
+								},
+								{
+									Name:      "user-credentials",
+									MountPath: "/credentials",
+								},
+							},
+						},
+					},
+					Volumes: []corev1api.Volume{
+						{
+							Name: nodeagent.HostPodVolumeMount,
+							VolumeSource: corev1api.VolumeSource{
+								HostPath: &corev1api.HostPathVolumeSource{
+									Path: "/var/lib/kubelet/pods",
+								},
+							},
+						},
+						{
+							Name: "host-plugins",
+							VolumeSource: corev1api.VolumeSource{
+								HostPath: &corev1api.HostPathVolumeSource{
+									Path: "/var/lib/kubelet/plugins",
+								},
+							},
+						},
+						{
+							// A host path volume added by users. It's not named after any
+							// well-known volume, so it can only be recognized by its source.
+							Name: "customized-host-path",
+							VolumeSource: corev1api.VolumeSource{
+								HostPath: &corev1api.HostPathVolumeSource{
+									Path: "/mnt/customized",
+								},
+							},
+						},
+						{
+							Name: "scratch",
+							VolumeSource: corev1api.VolumeSource{
+								EmptyDir: new(corev1api.EmptyDirVolumeSource),
+							},
+						},
+						{
+							Name: "user-credentials",
+							VolumeSource: corev1api.VolumeSource{
+								Secret: &corev1api.SecretVolumeSource{
+									SecretName: "user-credentials",
+								},
+							},
+						},
+					},
+					ServiceAccountName: "sa-1",
+				},
+			},
+		},
+	}
+
+	scratchAndCredentialMounts := []corev1api.VolumeMount{
+		{
+			Name:      "scratch",
+			MountPath: "/scratch",
+		},
+		{
+			Name:      "user-credentials",
+			MountPath: "/credentials",
+		},
+	}
+
+	scratchAndCredentialVolumes := []corev1api.Volume{
+		{
+			Name: "scratch",
+			VolumeSource: corev1api.VolumeSource{
+				EmptyDir: new(corev1api.EmptyDirVolumeSource),
+			},
+		},
+		{
+			Name: "user-credentials",
+			VolumeSource: corev1api.VolumeSource{
+				Secret: &corev1api.SecretVolumeSource{
+					SecretName: "user-credentials",
+				},
+			},
+		},
+	}
+
 	scheme := runtime.NewScheme()
 	appsv1api.AddToScheme(scheme)
 
 	tests := []struct {
-		name          string
-		namespace     string
-		client        kubernetes.Interface
-		kubeClientObj []runtime.Object
-		result        inheritedPodInfo
-		expectErr     string
+		name            string
+		namespace       string
+		client          kubernetes.Interface
+		kubeClientObj   []runtime.Object
+		excludeHostPath bool
+		result          inheritedPodInfo
+		expectErr       string
 	}{
 		{
 			name:      "ds is not found",
@@ -329,12 +446,93 @@ func TestGetInheritedPodInfo(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:      "host path volumes are inherited by default",
+			namespace: "fake-ns",
+			kubeClientObj: []runtime.Object{
+				daemonSetWithHostPath,
+			},
+			result: inheritedPodInfo{
+				image:          "image-1",
+				serviceAccount: "sa-1",
+				volumeMounts:   daemonSetWithHostPath.Spec.Template.Spec.Containers[0].VolumeMounts,
+				volumes:        daemonSetWithHostPath.Spec.Template.Spec.Volumes,
+			},
+		},
+		{
+			name:      "host path volumes and their mounts are excluded, no matter how they are named",
+			namespace: "fake-ns",
+			kubeClientObj: []runtime.Object{
+				daemonSetWithHostPath,
+			},
+			excludeHostPath: true,
+			result: inheritedPodInfo{
+				image:          "image-1",
+				serviceAccount: "sa-1",
+				volumeMounts:   scratchAndCredentialMounts,
+				volumes:        scratchAndCredentialVolumes,
+			},
+		},
+		{
+			name:      "excluding host path volumes keeps the others when there is none",
+			namespace: "fake-ns",
+			kubeClientObj: []runtime.Object{
+				daemonSetWithNoLog,
+			},
+			excludeHostPath: true,
+			result: inheritedPodInfo{
+				image:          "image-1",
+				serviceAccount: "sa-1",
+				env: []corev1api.EnvVar{
+					{
+						Name:  "env-1",
+						Value: "value-1",
+					},
+					{
+						Name:  "env-2",
+						Value: "value-2",
+					},
+				},
+				envFrom: []corev1api.EnvFromSource{
+					{
+						ConfigMapRef: &corev1api.ConfigMapEnvSource{
+							LocalObjectReference: corev1api.LocalObjectReference{
+								Name: "test-configmap",
+							},
+						},
+					},
+					{
+						SecretRef: &corev1api.SecretEnvSource{
+							LocalObjectReference: corev1api.LocalObjectReference{
+								Name: "test-secret",
+							},
+						},
+					},
+				},
+				volumeMounts: []corev1api.VolumeMount{
+					{
+						Name: "volume-1",
+					},
+					{
+						Name: "volume-2",
+					},
+				},
+				volumes: []corev1api.Volume{
+					{
+						Name: "volume-1",
+					},
+					{
+						Name: "volume-2",
+					},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...)
-			info, err := getInheritedPodInfo(t.Context(), fakeKubeClient, test.namespace, kube.NodeOSLinux)
+			info, err := getInheritedPodInfo(t.Context(), fakeKubeClient, test.namespace, kube.NodeOSLinux, test.excludeHostPath)
 
 			if test.expectErr == "" {
 				require.NoError(t, err)

pkg/exposer/pod_volume.go

@@ -1,5 +1,5 @@
 /*
-Copyright The Velero Contributors.
+Copyright the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -365,7 +365,7 @@ func (e *podVolumeExposer) createHostingPod(
 	clientVolumeName := string(ownerObject.UID)
 	clientVolumePath := "/" + clientVolumeName
 
-	podInfo, err := getInheritedPodInfo(ctx, e.kubeClient, ownerObject.Namespace, nodeOS)
+	podInfo, err := getInheritedPodInfo(ctx, e.kubeClient, ownerObject.Namespace, nodeOS, inheritHostPathVolumes)
 	if err != nil {
 		return nil, errors.Wrap(err, "error to get inherited pod info from node-agent")
 	}