Address review comments on host path exclusion

Detect the host path volumes by their source instead of their name, so the customized ones are excluded as well. Drop the container capability changes since the data mover needs them to access the data, and fix the copyright headers.

Signed-off-by: chlins <chlins.zhang@gmail.com>

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.
@@ -686,7 +686,7 @@ func (e *csiSnapshotExposer) createBackupPod(
 
 	// 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, hostPathVolumesOfNodeAgent...)
+	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")
 	}
@@ -752,7 +752,6 @@ func (e *csiSnapshotExposer) createBackupPod(
 	}
 
 	var securityCtx *corev1api.PodSecurityContext
-	var containerSecurityCtx *corev1api.SecurityContext
 	nodeSelector := map[string]string{}
 	podOS := corev1api.PodOS{}
 	if nodeOS == kube.NodeOSWindows {
@@ -791,18 +790,6 @@ func (e *csiSnapshotExposer) createBackupPod(
 			RunAsUser: &userID,
 		}
 
-		// The backup pod runs as root so that it can read the backup data regardless of the
-		// ownership, but it doesn't need any capability beyond that.
-		containerSecurityCtx = &corev1api.SecurityContext{
-			AllowPrivilegeEscalation: boolptr.False(),
-			Capabilities: &corev1api.Capabilities{
-				Drop: []corev1api.Capability{"ALL"},
-			},
-			SeccompProfile: &corev1api.SeccompProfile{
-				Type: corev1api.SeccompProfileTypeRuntimeDefault,
-			},
-		}
-
 		if spcNoRelabeling {
 			securityCtx.SELinuxOptions = &corev1api.SELinuxOptions{
 				Type: "spc_t",
@@ -874,13 +861,12 @@ func (e *csiSnapshotExposer) createBackupPod(
 						"data-mover",
 						"backup",
 					},
-					Args:            args,
-					VolumeMounts:    volumeMounts,
-					VolumeDevices:   volumeDevices,
-					Env:             podInfo.env,
-					EnvFrom:         podInfo.envFrom,
-					Resources:       resources,
-					SecurityContext: containerSecurityCtx,
+					Args:          args,
+					VolumeMounts:  volumeMounts,
+					VolumeDevices: volumeDevices,
+					Env:           podInfo.env,
+					EnvFrom:       podInfo.envFrom,
+					Resources:     resources,
 				},
 			},
 			PriorityClassName:             priorityClassName,

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.
@@ -630,7 +630,7 @@ func (e *genericRestoreExposer) createRestorePod(
 
 	// 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, hostPathVolumesOfNodeAgent...)
+	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")
 	}
@@ -694,7 +694,6 @@ func (e *genericRestoreExposer) createRestorePod(
 	args = append(args, podInfo.logLevelArgs...)
 
 	var securityCtx *corev1api.PodSecurityContext
-	var containerSecurityCtx *corev1api.SecurityContext
 	podOS := corev1api.PodOS{}
 	if nodeOS == kube.NodeOSWindows {
 		userID := "ContainerAdministrator"
@@ -732,18 +731,6 @@ func (e *genericRestoreExposer) createRestorePod(
 			RunAsUser: &userID,
 		}
 
-		// The restore pod runs as root so that it can restore the data with the original
-		// ownership, but it doesn't need any capability beyond that.
-		containerSecurityCtx = &corev1api.SecurityContext{
-			AllowPrivilegeEscalation: boolptr.False(),
-			Capabilities: &corev1api.Capabilities{
-				Drop: []corev1api.Capability{"ALL"},
-			},
-			SeccompProfile: &corev1api.SeccompProfile{
-				Type: corev1api.SeccompProfileTypeRuntimeDefault,
-			},
-		}
-
 		podOS.Name = kube.NodeOSLinux
 
 		affinity.NodeSelector.MatchExpressions = append(affinity.NodeSelector.MatchExpressions, metav1.LabelSelectorRequirement{
@@ -796,13 +783,12 @@ func (e *genericRestoreExposer) createRestorePod(
 						"data-mover",
 						"restore",
 					},
-					Args:            args,
-					VolumeMounts:    volumeMounts,
-					VolumeDevices:   volumeDevices,
-					Env:             podInfo.env,
-					EnvFrom:         podInfo.envFrom,
-					Resources:       resources,
-					SecurityContext: containerSecurityCtx,
+					Args:          args,
+					VolumeMounts:  volumeMounts,
+					VolumeDevices: volumeDevices,
+					Env:           podInfo.env,
+					EnvFrom:       podInfo.envFrom,
+					Resources:     resources,
 				},
 			},
 			PriorityClassName:             priorityClassName,

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.
@@ -28,17 +28,16 @@ import (
 )
 
 const (
-	// hostPluginsVolumeName is the name of the node-agent volume that mounts the kubelet
-	// plugins directory from the host.
-	hostPluginsVolumeName = "host-plugins"
-)
+	// 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
 
-// hostPathVolumesOfNodeAgent lists the node-agent volumes that expose the kubelet root
-// directory of the host. They are only required by fs-backup, which resolves and accesses
-// pod volume data through the kubelet pod directory. Other exposers access data through
-// PVCs only, so they must exclude these volumes from the inherited pod info to avoid
-// granting data mover pods unnecessary access to the host file system.
-var hostPathVolumesOfNodeAgent = []string{nodeagent.HostPodVolumeMount, hostPluginsVolumeName}
+	// 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
@@ -55,10 +54,11 @@ type inheritedPodInfo struct {
 }
 
 // getInheritedPodInfo collects the pod info to be inherited by the hosting pods from the
-// node-agent pod template. Volumes whose name is listed in excludedVolumes, together with
-// their volume mounts, are dropped from the result. Names that are not found in the
-// node-agent pod template are ignored.
-func getInheritedPodInfo(ctx context.Context, client kubernetes.Interface, veleroNamespace string, osType string, excludedVolumes ...string) (inheritedPodInfo, error) {
+// 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)
@@ -75,7 +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, podInfo.volumes = excludeVolumes(podSpec.Containers[0].VolumeMounts, podSpec.Volumes, excludedVolumes)
+	podInfo.volumeMounts, podInfo.volumes = filterVolumes(podSpec.Containers[0].VolumeMounts, podSpec.Volumes, excludeHostPath)
 
 	podInfo.dnsPolicy = podSpec.DNSPolicy
 	podInfo.dnsConfig = podSpec.DNSConfig
@@ -98,22 +98,27 @@ func getInheritedPodInfo(ctx context.Context, client kubernetes.Interface, veler
 	return podInfo, nil
 }
 
-// excludeVolumes removes the volumes matching the given names, as well as the volume mounts
-// referring to them, from the given volumes and volume mounts. An excluded name that doesn't
-// match any volume is a no-op, so callers don't need to know how the node-agent daemonset is
-// configured. Volumes that are not excluded, including the ones customized by users, are kept
-// as is.
-func excludeVolumes(volumeMounts []corev1api.VolumeMount, volumes []corev1api.Volume, excludedVolumes []string) ([]corev1api.VolumeMount, []corev1api.Volume) {
-	if len(excludedVolumes) == 0 {
+// 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{}, len(excludedVolumes))
-	for _, name := range excludedVolumes {
-		excluded[name] = struct{}{}
+	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)
 	}
 
-	var retainedMounts []corev1api.VolumeMount
+	retainedMounts := make([]corev1api.VolumeMount, 0, len(volumeMounts))
 	for _, volumeMount := range volumeMounts {
 		if _, found := excluded[volumeMount.Name]; found {
 			continue
@@ -122,14 +127,5 @@ func excludeVolumes(volumeMounts []corev1api.VolumeMount, volumes []corev1api.Vo
 		retainedMounts = append(retainedMounts, volumeMount)
 	}
 
-	var retainedVolumes []corev1api.Volume
-	for _, volume := range volumes {
-		if _, found := excluded[volume.Name]; found {
-			continue
-		}
-
-		retainedVolumes = append(retainedVolumes, volume)
-	}
-
 	return retainedMounts, retainedVolumes
 }

pkg/exposer/image_daemonset_test.go

@@ -1,3 +1,19 @@
+/*
+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 (
@@ -37,7 +53,7 @@ func TestInheritedPodInfoAgainstRealDaemonSet(t *testing.T) {
 	}
 
 	// fs-backup resolves pod volume data through the kubelet pod directory, so it keeps them.
-	fsBackupInfo, err := getInheritedPodInfo(context.Background(), client, "velero", "linux")
+	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)
 	}
@@ -47,7 +63,7 @@ func TestInheritedPodInfoAgainstRealDaemonSet(t *testing.T) {
 	}
 
 	// The data mover pods access data through PVCs, so they must not get any host path.
-	dataMoverInfo, err := getInheritedPodInfo(context.Background(), client, "velero", "linux", hostPathVolumesOfNodeAgent...)
+	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)
 	}

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.
@@ -209,9 +209,13 @@ func TestGetInheritedPodInfo(t *testing.T) {
 									MountPath: "/host_pods",
 								},
 								{
-									Name:      hostPluginsVolumeName,
+									Name:      "host-plugins",
 									MountPath: "/var/lib/kubelet/plugins",
 								},
+								{
+									Name:      "customized-host-path",
+									MountPath: "/customized",
+								},
 								{
 									Name:      "scratch",
 									MountPath: "/scratch",
@@ -233,13 +237,23 @@ func TestGetInheritedPodInfo(t *testing.T) {
 							},
 						},
 						{
-							Name: hostPluginsVolumeName,
+							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{
@@ -297,7 +311,7 @@ func TestGetInheritedPodInfo(t *testing.T) {
 		namespace       string
 		client          kubernetes.Interface
 		kubeClientObj   []runtime.Object
-		excludedVolumes []string
+		excludeHostPath bool
 		result          inheritedPodInfo
 		expectErr       string
 	}{
@@ -433,7 +447,7 @@ func TestGetInheritedPodInfo(t *testing.T) {
 			},
 		},
 		{
-			name:      "no excluded volume, host path volumes are inherited",
+			name:      "host path volumes are inherited by default",
 			namespace: "fake-ns",
 			kubeClientObj: []runtime.Object{
 				daemonSetWithHostPath,
@@ -446,12 +460,12 @@ func TestGetInheritedPodInfo(t *testing.T) {
 			},
 		},
 		{
-			name:      "host path volumes and their mounts are excluded",
+			name:      "host path volumes and their mounts are excluded, no matter how they are named",
 			namespace: "fake-ns",
 			kubeClientObj: []runtime.Object{
 				daemonSetWithHostPath,
 			},
-			excludedVolumes: hostPathVolumesOfNodeAgent,
+			excludeHostPath: true,
 			result: inheritedPodInfo{
 				image:          "image-1",
 				serviceAccount: "sa-1",
@@ -460,12 +474,12 @@ func TestGetInheritedPodInfo(t *testing.T) {
 			},
 		},
 		{
-			name:      "excluding a volume that doesn't exist doesn't affect the others",
+			name:      "excluding host path volumes keeps the others when there is none",
 			namespace: "fake-ns",
 			kubeClientObj: []runtime.Object{
 				daemonSetWithNoLog,
 			},
-			excludedVolumes: hostPathVolumesOfNodeAgent,
+			excludeHostPath: true,
 			result: inheritedPodInfo{
 				image:          "image-1",
 				serviceAccount: "sa-1",
@@ -513,50 +527,12 @@ func TestGetInheritedPodInfo(t *testing.T) {
 				},
 			},
 		},
-		{
-			name:      "excluding all volumes results in empty volumes and mounts",
-			namespace: "fake-ns",
-			kubeClientObj: []runtime.Object{
-				daemonSetWithNoLog,
-			},
-			excludedVolumes: []string{"volume-1", "volume-2"},
-			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",
-							},
-						},
-					},
-				},
-			},
-		},
 	}
 
 	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, test.excludedVolumes...)
+			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")
 	}