Fix LoadAffinity mutation accumulating OS node selector terms (#10342)

* Fix LoadAffinity mutation accumulating OS node selector terms

The node-agent parses the loadAffinity configuration once at startup and
keeps it in memory. GetLoadAffinityByStorageClass returned a pointer to one
of the elements of that cached list rather than a copy, so the exposers,
which append a kubernetes.io/os match expression to the returned affinity,
were mutating the shared configuration. Every DataUpload or DataDownload
appended another OS term, growing the data mover pod spec until it could
eventually exceed the object size limit.

Return a deep copy from GetLoadAffinityByStorageClass so that callers can
safely modify the result. A shallow copy is not enough because the
MatchExpressions slice header would still be shared with the source.

Fixes #10341

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Add changelog

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

---------

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
(cherry picked from commit f27a4ad8c0)

Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>

changelogs/unreleased/10342-shubham-pampattiwar

@@ -0,0 +1 @@
+Fix issue #10341, avoid mutating the cached node-agent LoadAffinity so the OS node selector term is not appended repeatedly to data mover pods

pkg/util/kube/pod.go

@@ -319,9 +319,26 @@ func ExitPodWithMessage(logger logrus.FieldLogger, succeed bool, message string,
 	funcExit(exitCode)
 }
 
+// deepCopy returns a deep copy of the LoadAffinity, so that the returned value
+// can be safely modified without affecting the source.
+func (a *LoadAffinity) deepCopy() *LoadAffinity {
+	if a == nil {
+		return nil
+	}
+
+	result := &LoadAffinity{
+		StorageClass: a.StorageClass,
+	}
+	a.NodeSelector.DeepCopyInto(&result.NodeSelector)
+
+	return result
+}
+
 // GetLoadAffinityByStorageClass retrieves the LoadAffinity from the parameter affinityList.
 // The function first try to find by the scName. If there is no such LoadAffinity,
 // it will try to get the LoadAffinity whose StorageClass has no value.
+// The returned LoadAffinity is a deep copy of the matched element, so that the
+// callers can modify it without corrupting the shared node-agent configuration.
 func GetLoadAffinityByStorageClass(
 	affinityList []*LoadAffinity,
 	scName string,
@@ -332,7 +349,7 @@ func GetLoadAffinityByStorageClass(
 	for _, affinity := range affinityList {
 		if affinity.StorageClass == scName {
 			logger.WithField("StorageClass", scName).Info("Found pod's affinity setting per StorageClass.")
-			return affinity
+			return affinity.deepCopy()
 		}
 
 		if affinity.StorageClass == "" && globalAffinity == nil {
@@ -346,5 +363,5 @@ func GetLoadAffinityByStorageClass(
 		logger.Info("No Affinity is found for pod.")
 	}
 
-	return globalAffinity
+	return globalAffinity.deepCopy()
 }

pkg/util/kube/pod_test.go

@@ -1545,3 +1545,82 @@ func TestGetLoadAffinityByStorageClass(t *testing.T) {
 		})
 	}
 }
+
+func TestGetLoadAffinityByStorageClassReturnsCopy(t *testing.T) {
+	newAffinityList := func() []*LoadAffinity {
+		return []*LoadAffinity{
+			{
+				NodeSelector: metav1.LabelSelector{
+					MatchLabels: map[string]string{"pool": "backup"},
+					MatchExpressions: []metav1.LabelSelectorRequirement{
+						{
+							Key:      corev1api.LabelArchStable,
+							Operator: metav1.LabelSelectorOpIn,
+							Values:   []string{"amd64"},
+						},
+					},
+				},
+			},
+			{
+				NodeSelector: metav1.LabelSelector{
+					MatchExpressions: []metav1.LabelSelectorRequirement{
+						{
+							Key:      corev1api.LabelArchStable,
+							Operator: metav1.LabelSelectorOpIn,
+							Values:   []string{"arm64"},
+						},
+					},
+				},
+				StorageClass: "storage-class-01",
+			},
+		}
+	}
+
+	tests := []struct {
+		name   string
+		scName string
+	}{
+		{
+			name:   "global affinity",
+			scName: "no-such-storage-class",
+		},
+		{
+			name:   "affinity matched by StorageClass",
+			scName: "storage-class-01",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			affinityList := newAffinityList()
+
+			// Simulate the exposers, which append an OS related term to the returned
+			// affinity on every expose call. The source list must not be affected.
+			for range 3 {
+				result := GetLoadAffinityByStorageClass(affinityList, test.scName, velerotest.NewLogger())
+				require.NotNil(t, result)
+
+				result.NodeSelector.MatchExpressions = append(result.NodeSelector.MatchExpressions, metav1.LabelSelectorRequirement{
+					Key:      NodeOSLabel,
+					Operator: metav1.LabelSelectorOpNotIn,
+					Values:   []string{NodeOSWindows},
+				})
+
+				assert.Len(t, result.NodeSelector.MatchExpressions, 2)
+			}
+
+			assert.Equal(t, newAffinityList(), affinityList)
+
+			// The other fields must be copied as well.
+			result := GetLoadAffinityByStorageClass(affinityList, test.scName, velerotest.NewLogger())
+			require.NotNil(t, result)
+			result.StorageClass = "modified"
+			result.NodeSelector.MatchExpressions[0].Values[0] = "modified"
+			if result.NodeSelector.MatchLabels != nil {
+				result.NodeSelector.MatchLabels["pool"] = "modified"
+			}
+
+			assert.Equal(t, newAffinityList(), affinityList)
+		})
+	}
+}