From 8cfb9adbf60397fd1264ea7b0900419884a09719 Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 20 Mar 2025 22:02:40 +0000 Subject: [PATCH] overwrite existing labels during pod Binding storage --- pkg/registry/core/pod/storage/storage.go | 14 ++++--------- .../admission/podtopologylabels/admission.go | 7 ++----- .../podtopologylabels/admission_test.go | 4 ++-- test/integration/pods/pods_test.go | 20 +++++++++++++++++++ 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/pkg/registry/core/pod/storage/storage.go b/pkg/registry/core/pod/storage/storage.go index b93a38cd192..7323efe5faa 100644 --- a/pkg/registry/core/pod/storage/storage.go +++ b/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 - // overwrite any existing labels that have been previously set, to avoid - // changing any existing behaviour for pods that may be defined with a - // template by users and created by higher-level controllers. + // Copy all labels from the Binding over to the Pod object, overwriting + // any existing labels set on the Pod. 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 } } diff --git a/plugin/pkg/admission/podtopologylabels/admission.go b/plugin/pkg/admission/podtopologylabels/admission.go index a2c22e786a9..3b010df6fe6 100644 --- a/plugin/pkg/admission/podtopologylabels/admission.go +++ b/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 } } diff --git a/plugin/pkg/admission/podtopologylabels/admission_test.go b/plugin/pkg/admission/podtopologylabels/admission_test.go index f85aba70c54..956c2a934c2 100644 --- a/plugin/pkg/admission/podtopologylabels/admission_test.go +++ b/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", }, }, { diff --git a/test/integration/pods/pods_test.go b/test/integration/pods/pods_test.go index c1c88dc2b71..bc77f0b04aa 100644 --- a/test/integration/pods/pods_test.go +++ b/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) }