Merge pull request #10348 from velero-io/copilot/backport-10342

1.18: Backport #10342: Fix LoadAffinity mutation accumulating OS node selector terms

changelogs/unreleased/10348-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)
+		})
+	}
+}