Skip to content

Commit 8fc0388

Browse files
authored
[v0.12] Fix namespace label equality check and nil map panic (#5175)
* Fix namespace label equality check before update Compare the computed desired state against the current namespace labels and annotations instead of comparing raw options directly. The previous reflect.DeepEqual check could never return true because ns.Labels always includes kubernetes.io/metadata.name (and now also preserved pod-security labels), causing an unnecessary updateNamespace call on every reconcile even when no label changes were needed. * Fix nil map panic and update addLabelsFromOptions doc comment addLabelsFromOptions panics when ns.Labels is nil (maps.Clone returns nil) and NamespaceLabels options are non-nil. Add the same nil guard already present for annotations. Update the function doc comment to reflect the actual label retention rules: options labels, kubernetes.io/metadata.name, and existing pod-security.kubernetes.io/* labels are preserved; pod-security labels from options are ignored. * Retry on conflict in HelmURLRegex migration test The controller updates GitRepo status concurrently during integration tests, bumping the resource version between the test's Get and Update calls. Wrap the manual reset in Eventually to retry on conflict.
1 parent f443e83 commit 8fc0388

2 files changed

Lines changed: 27 additions & 18 deletions

File tree

integrationtests/gitjob/controller/helm_url_regex_migration_test.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,13 @@ var _ = Describe("HelmURLRegex migration", func() {
254254
Expect(getGitRepo("run-twice").Spec.HelmRepoURLRegex).To(Equal(`^https://charts\.example\.com/`))
255255

256256
// Manually reset to simulate a hypothetical re-entry attempt.
257-
gr := getGitRepo("run-twice")
258-
gr.Spec.HelmRepoURLRegex = ""
259-
Expect(k8sClient.Update(ctx, gr)).To(Succeed())
260-
257+
// Retry on conflict: the controller may concurrently update
258+
// the GitRepo (e.g. status), bumping its resource version.
259+
Eventually(func() error {
260+
gr := getGitRepo("run-twice")
261+
gr.Spec.HelmRepoURLRegex = ""
262+
return k8sClient.Update(ctx, gr)
263+
}).Should(Succeed())
261264
// Second run: marker present, migration skipped.
262265
Expect(runHelmURLRegexMigration()).To(Succeed())
263266
Expect(getGitRepo("run-twice").Spec.HelmRepoURLRegex).To(BeEmpty())

internal/cmd/agent/deployer/deployer.go

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ package deployer
33
import (
44
"context"
55
"fmt"
6-
"reflect"
6+
"maps"
77
"regexp"
88
"strconv"
99
"strings"
@@ -182,25 +182,28 @@ func (d *Deployer) setNamespaceLabelsAndAnnotations(ctx context.Context, bd *fle
182182
return err
183183
}
184184

185-
if reflect.DeepEqual(bd.Spec.Options.NamespaceLabels, ns.Labels) && reflect.DeepEqual(bd.Spec.Options.NamespaceAnnotations, ns.Annotations) {
186-
return nil
187-
}
188-
185+
desiredLabels := maps.Clone(ns.Labels)
189186
if bd.Spec.Options.NamespaceLabels != nil {
190-
addLabelsFromOptions(log.FromContext(ctx), ns.Labels, bd.Spec.Options.NamespaceLabels)
187+
if desiredLabels == nil {
188+
desiredLabels = make(map[string]string)
189+
}
190+
addLabelsFromOptions(log.FromContext(ctx), desiredLabels, bd.Spec.Options.NamespaceLabels)
191191
}
192+
desiredAnnotations := maps.Clone(ns.Annotations)
192193
if bd.Spec.Options.NamespaceAnnotations != nil {
193-
if ns.Annotations == nil {
194-
ns.Annotations = map[string]string{}
194+
if desiredAnnotations == nil {
195+
desiredAnnotations = make(map[string]string)
195196
}
196-
addAnnotationsFromOptions(ns.Annotations, bd.Spec.Options.NamespaceAnnotations)
197+
addAnnotationsFromOptions(desiredAnnotations, bd.Spec.Options.NamespaceAnnotations)
197198
}
198-
err = d.updateNamespace(ctx, ns)
199-
if err != nil {
200-
return err
199+
200+
if maps.Equal(desiredLabels, ns.Labels) && maps.Equal(desiredAnnotations, ns.Annotations) {
201+
return nil
201202
}
202203

203-
return nil
204+
ns.Labels = desiredLabels
205+
ns.Annotations = desiredAnnotations
206+
return d.updateNamespace(ctx, ns)
204207
}
205208

206209
// updateNamespace updates a namespace resource in the cluster.
@@ -227,7 +230,10 @@ func (d *Deployer) fetchNamespace(ctx context.Context, releaseID string) (*corev
227230

228231
const podSecurityLabelPrefix = "pod-security.kubernetes.io/"
229232

230-
// addLabelsFromOptions updates nsLabels so that it only contains all labels specified in optLabels, plus the `kubernetes.io/metadata.name` labels added by kubernetes when creating the namespace.
233+
// addLabelsFromOptions updates nsLabels to contain labels from optLabels, while preserving
234+
// the `kubernetes.io/metadata.name` label added by Kubernetes when creating the namespace
235+
// and any existing `pod-security.kubernetes.io/*` labels. Labels with the
236+
// `pod-security.kubernetes.io/` prefix in optLabels are ignored.
231237
func addLabelsFromOptions(logger logr.Logger, nsLabels map[string]string, optLabels map[string]string) {
232238
for k, v := range optLabels {
233239
if strings.HasPrefix(k, podSecurityLabelPrefix) {

0 commit comments

Comments
 (0)