diff --git a/changelogs/unreleased/10150-chlins b/changelogs/unreleased/10150-chlins new file mode 100644 index 000000000..158a3f43c --- /dev/null +++ b/changelogs/unreleased/10150-chlins @@ -0,0 +1 @@ +Drop node-agent host path mounts from data mover pods diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index ed510c798..2e8e08889 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/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") } diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index 0f4b9c5b4..16a114e64 100644 --- a/pkg/exposer/generic_restore.go +++ b/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") } diff --git a/pkg/exposer/image.go b/pkg/exposer/image.go index 2157d8175..396303d4f 100644 --- a/pkg/exposer/image.go +++ b/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 +} diff --git a/pkg/exposer/image_daemonset_test.go b/pkg/exposer/image_daemonset_test.go new file mode 100644 index 000000000..21ae104df --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/exposer/image_test.go b/pkg/exposer/image_test.go index 5c47f5c04..a7672b344 100644 --- a/pkg/exposer/image_test.go +++ b/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) diff --git a/pkg/exposer/pod_volume.go b/pkg/exposer/pod_volume.go index 0526b2c5e..5d6de1831 100644 --- a/pkg/exposer/pod_volume.go +++ b/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") }