overwrite existing labels during pod Binding storage

pkg/registry/core/pod/storage/storage.go

@@ -247,12 +247,10 @@ func (r *BindingREST) setPodNodeAndMetadata(ctx context.Context, podUID types.UI
 		for k, v := range annotations {
 			pod.Annotations[k] = v
 		}
-		// Copy all labels from the Binding over to the Pod object, but do not
+		// Copy all labels from the Binding over to the Pod object, overwriting
-		// overwrite any existing labels that have been previously set, to avoid
+		// any existing labels set on the Pod.
-		// changing any existing behaviour for pods that may be defined with a
-		// template by users and created by higher-level controllers.
 		if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.PodTopologyLabelsAdmission) {
-			copyLabelsWithoutOverwriting(pod, labels)
+			copyLabelsWithOverwriting(pod, labels)
 		}
 		podutil.UpdatePodCondition(&pod.Status, &api.PodCondition{
 			Type:   api.PodScheduled,
@@ -264,7 +262,7 @@ func (r *BindingREST) setPodNodeAndMetadata(ctx context.Context, podUID types.UI
 	return finalPod, err
 }
 
-func copyLabelsWithoutOverwriting(pod *api.Pod, labels map[string]string) {
+func copyLabelsWithOverwriting(pod *api.Pod, labels map[string]string) {
 	if len(labels) == 0 {
 		// nothing to do
 		return
@@ -274,10 +272,6 @@ func copyLabelsWithoutOverwriting(pod *api.Pod, labels map[string]string) {
 	}
 	// Iterate over the binding's labels and copy them across to the Pod.
 	for k, v := range labels {
-		if _, ok := pod.Labels[k]; ok {
-			// don't overwrite labels that are already present on the Pod
-			continue
-		}
 		pod.Labels[k] = v
 	}
 }

plugin/pkg/admission/podtopologylabels/admission.go

@@ -172,7 +172,7 @@ func (p *Plugin) admitPod(ctx context.Context, a admission.Attributes, o admissi
 	if pod.Labels == nil {
 		pod.Labels = make(map[string]string)
 	}
-	// avoid overwriting any existing labels on the Pod.
+	// overwrite any existing labels on the pod
 	mergeLabels(pod.Labels, labelsToCopy)
 
 	return nil
@@ -199,12 +199,9 @@ func (p *Plugin) topologyLabelsForNodeName(nodeName string) (map[string]string,
 	return labels, nil
 }
 
-// mergeLabels merges new into existing, without overwriting existing keys.
+// mergeLabels merges new into existing, overwriting existing keys.
 func mergeLabels(existing, new map[string]string) {
 	for k, v := range new {
-		if _, exists := existing[k]; exists {
-			continue
-		}
 		existing[k] = v
 	}
 }

plugin/pkg/admission/podtopologylabels/admission_test.go

@@ -89,7 +89,7 @@ func TestPodTopology(t *testing.T) {
 			existingBindingLabels: map[string]string{},
 		},
 		{
-			name: "does not overwrite existing topology labels on Binding objects",
+			name: "overwrites existing topology labels",
 			existingBindingLabels: map[string]string{
 				"topology.k8s.io/zone": "oldValue",
 			},
@@ -97,7 +97,7 @@ func TestPodTopology(t *testing.T) {
 				"topology.k8s.io/zone": "newValue",
 			},
 			expectedBindingLabels: map[string]string{
-				"topology.k8s.io/zone": "oldValue",
+				"topology.k8s.io/zone": "newValue",
 			},
 		},
 		{

test/integration/pods/pods_test.go

@@ -79,6 +79,24 @@ func TestPodTopologyLabels(t *testing.T) {
 				"topology.k8s.io/region": "region",
 			},
 		},
+		{
+			name: "labels from Bindings overwriting existing labels on Pod",
+			existingPodLabels: map[string]string{
+				"topology.k8s.io/zone":   "bad-zone",
+				"topology.k8s.io/region": "bad-region",
+				"topology.k8s.io/abc":    "123",
+			},
+			targetNodeLabels: map[string]string{
+				"topology.k8s.io/zone":   "zone",
+				"topology.k8s.io/region": "region",
+				"topology.k8s.io/abc":    "456", // this label isn't in (zone, region) so isn't copied
+			},
+			expectedPodLabels: map[string]string{
+				"topology.k8s.io/zone":   "zone",
+				"topology.k8s.io/region": "region",
+				"topology.k8s.io/abc":    "123",
+			},
+		},
 	}
 	// Enable the feature BEFORE starting the test server, as the admission plugin only checks feature gates
 	// on start up and not on each invocation at runtime.
@@ -113,6 +131,7 @@ func TestPodTopologyLabels_FeatureDisabled(t *testing.T) {
 type podTopologyTestCase struct {
 	name              string
 	targetNodeLabels  map[string]string
+	existingPodLabels map[string]string
 	expectedPodLabels map[string]string
 }
 
@@ -160,6 +179,7 @@ func testPodTopologyLabels(t *testing.T, tests []podTopologyTestCase) {
 			}
 
 			pod := prototypePod()
+			pod.Labels = test.existingPodLabels
 			if pod, err = client.CoreV1().Pods(ns.Name).Create(ctx, pod, metav1.CreateOptions{}); err != nil {
 				t.Errorf("Failed to create pod: %v", err)
 			}