Skip to content

Commit 8cfb9ad

Browse files
committed
overwrite existing labels during pod Binding storage
1 parent a490960 commit 8cfb9ad

File tree

4 files changed

+28
-17
lines changed

4 files changed

+28
-17
lines changed

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

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -247,12 +247,10 @@ func (r *BindingREST) setPodNodeAndMetadata(ctx context.Context, podUID types.UI
247247
for k, v := range annotations {
248248
pod.Annotations[k] = v
249249
}
250-
// Copy all labels from the Binding over to the Pod object, but do not
251-
// overwrite any existing labels that have been previously set, to avoid
252-
// changing any existing behaviour for pods that may be defined with a
253-
// template by users and created by higher-level controllers.
250+
// Copy all labels from the Binding over to the Pod object, overwriting
251+
// any existing labels set on the Pod.
254252
if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.PodTopologyLabelsAdmission) {
255-
copyLabelsWithoutOverwriting(pod, labels)
253+
copyLabelsWithOverwriting(pod, labels)
256254
}
257255
podutil.UpdatePodCondition(&pod.Status, &api.PodCondition{
258256
Type: api.PodScheduled,
@@ -264,7 +262,7 @@ func (r *BindingREST) setPodNodeAndMetadata(ctx context.Context, podUID types.UI
264262
return finalPod, err
265263
}
266264

267-
func copyLabelsWithoutOverwriting(pod *api.Pod, labels map[string]string) {
265+
func copyLabelsWithOverwriting(pod *api.Pod, labels map[string]string) {
268266
if len(labels) == 0 {
269267
// nothing to do
270268
return
@@ -274,10 +272,6 @@ func copyLabelsWithoutOverwriting(pod *api.Pod, labels map[string]string) {
274272
}
275273
// Iterate over the binding's labels and copy them across to the Pod.
276274
for k, v := range labels {
277-
if _, ok := pod.Labels[k]; ok {
278-
// don't overwrite labels that are already present on the Pod
279-
continue
280-
}
281275
pod.Labels[k] = v
282276
}
283277
}

plugin/pkg/admission/podtopologylabels/admission.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ func (p *Plugin) admitPod(ctx context.Context, a admission.Attributes, o admissi
172172
if pod.Labels == nil {
173173
pod.Labels = make(map[string]string)
174174
}
175-
// avoid overwriting any existing labels on the Pod.
175+
// overwrite any existing labels on the pod
176176
mergeLabels(pod.Labels, labelsToCopy)
177177

178178
return nil
@@ -199,12 +199,9 @@ func (p *Plugin) topologyLabelsForNodeName(nodeName string) (map[string]string,
199199
return labels, nil
200200
}
201201

202-
// mergeLabels merges new into existing, without overwriting existing keys.
202+
// mergeLabels merges new into existing, overwriting existing keys.
203203
func mergeLabels(existing, new map[string]string) {
204204
for k, v := range new {
205-
if _, exists := existing[k]; exists {
206-
continue
207-
}
208205
existing[k] = v
209206
}
210207
}

plugin/pkg/admission/podtopologylabels/admission_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,15 +89,15 @@ func TestPodTopology(t *testing.T) {
8989
existingBindingLabels: map[string]string{},
9090
},
9191
{
92-
name: "does not overwrite existing topology labels on Binding objects",
92+
name: "overwrites existing topology labels",
9393
existingBindingLabels: map[string]string{
9494
"topology.k8s.io/zone": "oldValue",
9595
},
9696
targetNodeLabels: map[string]string{
9797
"topology.k8s.io/zone": "newValue",
9898
},
9999
expectedBindingLabels: map[string]string{
100-
"topology.k8s.io/zone": "oldValue",
100+
"topology.k8s.io/zone": "newValue",
101101
},
102102
},
103103
{

test/integration/pods/pods_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,24 @@ func TestPodTopologyLabels(t *testing.T) {
7979
"topology.k8s.io/region": "region",
8080
},
8181
},
82+
{
83+
name: "labels from Bindings overwriting existing labels on Pod",
84+
existingPodLabels: map[string]string{
85+
"topology.k8s.io/zone": "bad-zone",
86+
"topology.k8s.io/region": "bad-region",
87+
"topology.k8s.io/abc": "123",
88+
},
89+
targetNodeLabels: map[string]string{
90+
"topology.k8s.io/zone": "zone",
91+
"topology.k8s.io/region": "region",
92+
"topology.k8s.io/abc": "456", // this label isn't in (zone, region) so isn't copied
93+
},
94+
expectedPodLabels: map[string]string{
95+
"topology.k8s.io/zone": "zone",
96+
"topology.k8s.io/region": "region",
97+
"topology.k8s.io/abc": "123",
98+
},
99+
},
82100
}
83101
// Enable the feature BEFORE starting the test server, as the admission plugin only checks feature gates
84102
// on start up and not on each invocation at runtime.
@@ -113,6 +131,7 @@ func TestPodTopologyLabels_FeatureDisabled(t *testing.T) {
113131
type podTopologyTestCase struct {
114132
name string
115133
targetNodeLabels map[string]string
134+
existingPodLabels map[string]string
116135
expectedPodLabels map[string]string
117136
}
118137

@@ -160,6 +179,7 @@ func testPodTopologyLabels(t *testing.T, tests []podTopologyTestCase) {
160179
}
161180

162181
pod := prototypePod()
182+
pod.Labels = test.existingPodLabels
163183
if pod, err = client.CoreV1().Pods(ns.Name).Create(ctx, pod, metav1.CreateOptions{}); err != nil {
164184
t.Errorf("Failed to create pod: %v", err)
165185
}

0 commit comments

Comments
 (0)