change how we copy labels to importer pod (#4091)

when we are using populators, instead of copying
labels from target pvc -> prime pvc -> pod, skip
copying to prime and instead get the labels from
target pvc by using the prime pvc's owner ref.

convert functional test for this to unit test.

Signed-off-by: dsanatar <dsanatar@redhat.com>

Author: dsanatar

pkg/controller/BUILD.bazel

@@ -120,6 +120,7 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",

pkg/controller/import-controller.go

@@ -941,8 +941,22 @@ func createImporterPod(ctx context.Context, log logr.Logger, client client.Clien
 
 	util.SetRecommendedLabels(pod, installerLabels, "cdi-controller")
 
+	srcLabels := args.pvc.Labels
+	if _, isPopulator := args.pvc.Annotations[cc.AnnPopulatorKind]; isPopulator {
+		ownerRef := metav1.GetControllerOf(args.pvc)
+		if ownerRef == nil || ownerRef.Kind != "PersistentVolumeClaim" {
+			return nil, fmt.Errorf("pvc %s/%s does not have a valid owner reference", args.pvc.Namespace, args.pvc.Name)
+		}
+
+		pvc := &corev1.PersistentVolumeClaim{}
+		if err := client.Get(context.TODO(), types.NamespacedName{Namespace: args.pvc.Namespace, Name: ownerRef.Name}, pvc); err != nil {
+			return nil, err
+		}
+		srcLabels = pvc.GetLabels()
+	}
+
 	// add any labels from pvc to the importer pod
-	util.MergeLabels(args.pvc.Labels, pod.Labels)
+	util.MergeLabels(srcLabels, pod.Labels)
 
 	if err = client.Create(context.TODO(), pod); err != nil {
 		return nil, err

pkg/controller/import-controller_test.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	kvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -907,6 +908,64 @@ var _ = Describe("Create Importer Pod", func() {
 		Entry("should create pod with block volume mode and scratchspace", createBlockPvc("testBlockPvc1", "default", map[string]string{cc.AnnEndpoint: testEndPoint, cc.AnnPodPhase: string(corev1.PodPending), cc.AnnImportPod: "podName", cc.AnnPriorityClassName: "p0", cc.AnnPodServiceAccount: "my-sa"}, nil), &scratchPvcName),
 	)
 
+	DescribeTable("should copy labels from target PVC when creating pod", func(isPopulator bool) {
+		targetPvc := cc.CreatePvc("targetPvc", "default",
+			map[string]string{
+				cc.AnnEndpoint:  testEndPoint,
+				cc.AnnImportPod: "podName",
+			},
+			map[string]string{
+				"custom-label": "value",
+			},
+		)
+
+		objects := []runtime.Object{targetPvc}
+		podArgs := &importerPodArgs{
+			image:      testImage,
+			verbose:    "5",
+			pullPolicy: testPullPolicy,
+			podEnvVar:  &importPodEnvVar{},
+			pvc:        targetPvc,
+		}
+
+		if isPopulator {
+			primePvc := cc.CreatePvc("primePvc", "default",
+				map[string]string{
+					cc.AnnEndpoint:      testEndPoint,
+					cc.AnnImportPod:     "podName",
+					cc.AnnPopulatorKind: cdiv1.VolumeImportSourceRef,
+				},
+				map[string]string{
+					"prime-label": "prime-value",
+				},
+			)
+			primePvc.OwnerReferences = []metav1.OwnerReference{
+				*metav1.NewControllerRef(targetPvc, schema.GroupVersionKind{
+					Group:   "",
+					Version: "v1",
+					Kind:    "PersistentVolumeClaim",
+				}),
+			}
+			objects = append(objects, primePvc)
+			podArgs.pvc = primePvc
+		}
+
+		reconciler := createImportReconciler(objects...)
+
+		pod, err := createImporterPod(context.TODO(), reconciler.log, reconciler.client, podArgs, map[string]string{})
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Verifying pod has labels from target PVC")
+		Expect(pod.Labels).To(HaveKeyWithValue("custom-label", "value"))
+		if isPopulator {
+			By("Verifying pod does not have labels from prime PVC")
+			Expect(pod.Labels).ToNot(HaveKey("prime-label"))
+		}
+	},
+		Entry("using populator", true),
+		Entry("using non-populator", false),
+	)
+
 	DescribeTable("should append current checkpoint name to importer pod", func(pvcName, checkpointID string) {
 		pvc := cc.CreatePvc(pvcName, "default", map[string]string{cc.AnnCurrentCheckpoint: checkpointID, cc.AnnEndpoint: testEndPoint}, nil)
 		pvc.Status.Phase = v1.ClaimBound

pkg/controller/populators/import-populator_test.go

@@ -285,6 +285,25 @@ var _ = Describe("Import populator tests", func() {
 			Entry("retain pod annotation is passed", AnnPodRetainAfterCompletion, "true", "true"),
 		)
 
+		It("Should not copy target PVC labels to PVC Prime", func() {
+			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name,
+				map[string]string{}, map[string]string{"custom-label": "value"}, corev1.ClaimPending)
+			targetPvc.Spec.DataSourceRef = dataSourceRef
+			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
+
+			By("Reconcile")
+			reconciler = createImportPopulatorReconciler(targetPvc, volumeImportSource, sc)
+			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: targetPvcName, Namespace: metav1.NamespaceDefault}})
+			Expect(err).To(Not(HaveOccurred()))
+			Expect(result).To(Not(BeNil()))
+
+			By("Checking prime PVC does not have target PVC labels")
+			pvcPrime, err := reconciler.getPVCPrime(targetPvc)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pvcPrime).ToNot(BeNil())
+			Expect(pvcPrime.GetLabels()).ToNot(HaveKey("custom-label"))
+		})
+
 		It("should trigger appropriate event when using AnnPodRetainAfterCompletion", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name,
 				map[string]string{AnnPodPhase: string(corev1.PodSucceeded)}, nil, corev1.ClaimPending)

pkg/controller/populators/populator-base.go

@@ -231,7 +231,6 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc
 	}
 	cc.CopyAllowedAnnotations(pvc, pvcPrime)
 	util.SetRecommendedLabels(pvcPrime, r.installerLabels, "cdi-controller")
-	util.MergeLabels(pvc.Labels, pvcPrime.Labels)
 
 	// We use the populator-specific pvcModifierFunc to add required annotations
 	if updatePVCForPopulation != nil {

tests/import_test.go

@@ -2097,55 +2097,6 @@ var _ = Describe("Containerdisk envs to PVC labels", func() {
 	)
 })
 
-var _ = Describe("Propagate DV Labels to Importer Pod", func() {
-	f := framework.NewFramework(namespacePrefix)
-
-	const (
-		testKubevirtKey    = "test.kubevirt.io/test"
-		testKubevirtValue  = "true"
-		testNonKubevirtKey = "testLabel"
-		testNonKubevirtVal = "none"
-	)
-
-	DescribeTable("Import pod should inherit any labels from Data Volume", func(usePopulator string) {
-
-		dataVolume := utils.NewDataVolumeWithHTTPImport("label-test", "100Mi", fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs))
-		dataVolume.Annotations[controller.AnnImmediateBinding] = "true"
-		dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true"
-		dataVolume.Annotations[controller.AnnUsePopulator] = usePopulator
-
-		dataVolume.Labels = map[string]string{
-			testKubevirtKey:    testKubevirtValue,
-			testNonKubevirtKey: testNonKubevirtVal,
-		}
-
-		By(fmt.Sprintf("Create new datavolume %s", dataVolume.Name))
-		dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
-		Expect(err).ToNot(HaveOccurred())
-
-		By("Verify pvc was created")
-		_, err = utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name)
-		Expect(err).ToNot(HaveOccurred())
-
-		By("Wait for import to be completed")
-		err = utils.WaitForDataVolumePhase(f, dataVolume.Namespace, cdiv1.Succeeded, dataVolume.Name)
-		Expect(err).ToNot(HaveOccurred(), "Datavolume not in phase succeeded in time")
-
-		By("Find importer pod")
-		importer, err := utils.FindPodByPrefix(f.K8sClient, dataVolume.Namespace, common.ImporterPodName, common.CDILabelSelector)
-		Expect(err).ToNot(HaveOccurred())
-
-		By("Check labels were appended")
-		importLabels := importer.GetLabels()
-		Expect(importLabels).Should(HaveKeyWithValue(testKubevirtKey, testKubevirtValue))
-		Expect(importLabels).Should(HaveKeyWithValue(testNonKubevirtKey, testNonKubevirtVal))
-
-	},
-		Entry("With Populators", "true"),
-		Entry("Without Populators", "false"),
-	)
-})
-
 var _ = Describe("pull image failure", func() {
 	var (
 		f = framework.NewFramework(namespacePrefix)