From 3d2e9dc42e3524661c0619f964d7ad200fb397de Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 26 Jul 2026 15:34:37 -0700 Subject: [PATCH 1/2] fix: preserve immutable DaemonSet selector on FluentBit label changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What this PR does / why we need it: Motivation: When `spec.labels` on a FluentBit CR is changed after its DaemonSet already exists, the reconciler fails on every attempt with: DaemonSet.apps "fluent-bit" is invalid: spec.selector: Invalid value: ...: field is immutable `MakeDaemonSet()` derives `spec.selector.matchLabels` directly from `fb.Spec.Labels`, and the controller's `mutate()` function unconditionally overwrote the existing DaemonSet's `Spec` (including the selector) with the newly computed one on every reconcile. Kubernetes rejects any change to a DaemonSet's `spec.selector` after creation, so this update is always rejected, and the reconciler keeps retrying and failing forever. The issue also reports that other legitimate DaemonSet updates get blocked by the same rejected patch while this selector mismatch persists. Approach: In the DaemonSet branch of `mutate()`, capture the existing `Spec.Selector` before overwriting `Spec`, then restore it afterwards instead of applying the newly computed one. Since a DaemonSet's pod template labels must always be a superset of its selector's matchLabels, any selector labels that would otherwise have been dropped from the new `fb.Spec.Labels` are merged back into `Spec.Template.Labels` via a freshly built map (not written in place, since `MakeDaemonSet` reuses `fb.Spec.Labels` as the same map object for the DaemonSet's own labels and selector — mutating it in place would leak back into the FluentBit CR's own spec in memory). This does not make the DaemonSet's selector track later label changes (that would require deleting and recreating the DaemonSet, which is a separate, more invasive change already under discussion in the issue thread and is out of scope here). User-visible behavior for the selector itself is unchanged from before label changes were introduced: it simply stays pinned to whatever it was at creation time. The concrete, narrower benefit is that changing `spec.labels` (or removing RBAC/labels-triggered churn) no longer wedges the controller in a permanent error loop, and other pending DaemonSet updates are no longer blocked by it. Validation: `go build ./...` passes. Added `TestFluentBitMutateDaemonSetPreservesImmutableSelector` (label added) and `TestFluentBitMutateDaemonSetRemovedLabelDoesNotMutateSpec` (label removed, also asserts `fb.Spec.Labels` is not mutated as a side effect) to `controllers/fluentbit_controller_test.go`. Verified both new tests fail against the pre-fix code and pass against the fix. Ran: go test ./controllers/... -v All tests pass, including the pre-existing (empty) Ginkgo suite. ### Which issue(s) this PR fixes: Fixes #1944 ### Does this PR introduced a user-facing change? ```release-note Fixed a bug where changing `spec.labels` on a FluentBit resource after its DaemonSet was created caused the operator to repeatedly fail reconciliation trying to update the DaemonSet's immutable `spec.selector` field. ``` Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- controllers/fluentbit_controller.go | 23 ++++ controllers/fluentbit_controller_test.go | 134 +++++++++++++++++++++++ 2 files changed, 157 insertions(+) create mode 100644 controllers/fluentbit_controller_test.go diff --git a/controllers/fluentbit_controller.go b/controllers/fluentbit_controller.go index bf1d2788b..03d3a22df 100644 --- a/controllers/fluentbit_controller.go +++ b/controllers/fluentbit_controller.go @@ -165,11 +165,34 @@ func (r *FluentBitReconciler) mutate(obj client.Object, fb *fluentbitv1alpha2.Fl return func() error { // Preserve the kubectl.kubernetes.io/restartedAt annotation restartedAt := o.Spec.Template.Annotations["kubectl.kubernetes.io/restartedAt"] + // Preserve the existing selector: it is immutable on DaemonSets, so + // reapplying a selector derived from a changed fb.Spec.Labels fails with + // "field is immutable" and blocks reconciliation indefinitely. + existingSelector := o.Spec.Selector o.Labels = expected.Labels o.Annotations = expected.Annotations o.Spec = expected.Spec + if existingSelector != nil { + o.Spec.Selector = existingSelector + + // The pod template must still satisfy the preserved selector, even if + // the matching labels were removed from the new fb.Spec.Labels. Build a + // new map rather than writing into o.Spec.Template.Labels in place, since + // MakeDaemonSet reuses fb.Spec.Labels itself as that map. + templateLabels := make(map[string]string, len(o.Spec.Template.Labels)+len(existingSelector.MatchLabels)) + for k, v := range o.Spec.Template.Labels { + templateLabels[k] = v + } + for k, v := range existingSelector.MatchLabels { + if _, ok := templateLabels[k]; !ok { + templateLabels[k] = v + } + } + o.Spec.Template.Labels = templateLabels + } + // Restore the kubectl.kubernetes.io/restartedAt annotation if it existed if restartedAt != "" { if o.Spec.Template.Annotations == nil { diff --git a/controllers/fluentbit_controller_test.go b/controllers/fluentbit_controller_test.go new file mode 100644 index 000000000..6d82f7a25 --- /dev/null +++ b/controllers/fluentbit_controller_test.go @@ -0,0 +1,134 @@ +/* +Copyright 2021. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controllers + +import ( + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + + fluentbitv1alpha2 "github.com/fluent/fluent-operator/v3/apis/fluentbit/v1alpha2" +) + +// TestFluentBitMutateDaemonSetPreservesImmutableSelector reproduces +// https://github.com/fluent/fluent-operator/issues/1944: changing +// spec.labels on a FluentBit CR must not cause the reconciler to try to +// rewrite the DaemonSet's immutable spec.selector. +func TestFluentBitMutateDaemonSetPreservesImmutableSelector(t *testing.T) { + s := runtime.NewScheme() + if err := scheme.AddToScheme(s); err != nil { + t.Fatalf("failed to add client-go scheme: %v", err) + } + if err := fluentbitv1alpha2.AddToScheme(s); err != nil { + t.Fatalf("failed to add fluentbit scheme: %v", err) + } + r := &FluentBitReconciler{Scheme: s} + + // The DaemonSet as it exists in the cluster today, created when + // spec.labels only contained "app". + existing := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: "fluent-bit", Namespace: "default"}, + Spec: appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "fluentbit"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "fluentbit"}}, + }, + }, + } + + // The user then adds a label to the FluentBit CR. + fb := &fluentbitv1alpha2.FluentBit{ + ObjectMeta: metav1.ObjectMeta{Name: "fluent-bit", Namespace: "default"}, + Spec: fluentbitv1alpha2.FluentBitSpec{ + Labels: map[string]string{"app": "fluentbit", "fluentbit.fluent.io/enabled": "true"}, + }, + } + + if err := r.mutate(existing, fb)(); err != nil { + t.Fatalf("mutate returned an unexpected error: %v", err) + } + + if got := existing.Spec.Selector.MatchLabels; len(got) != 1 || got["app"] != "fluentbit" { + t.Fatalf("expected the immutable selector to stay unchanged, got %v", got) + } + + // The pod template must still satisfy the preserved selector. + if existing.Spec.Template.Labels["app"] != "fluentbit" { + t.Fatalf("expected pod template to retain the selector label, got %v", existing.Spec.Template.Labels) + } + // The new label should still be applied to the pod template. + if existing.Spec.Template.Labels["fluentbit.fluent.io/enabled"] != "true" { + t.Fatalf("expected new label to be applied to pod template, got %v", existing.Spec.Template.Labels) + } +} + +// TestFluentBitMutateDaemonSetRemovedLabelDoesNotMutateSpec covers removing a +// label that was part of the original selector: the pod template must still +// carry it (to satisfy the preserved selector) without mutating fb.Spec.Labels +// in place, since MakeDaemonSet reuses that map for the DaemonSet's own labels +// and selector. +func TestFluentBitMutateDaemonSetRemovedLabelDoesNotMutateSpec(t *testing.T) { + s := runtime.NewScheme() + if err := scheme.AddToScheme(s); err != nil { + t.Fatalf("failed to add client-go scheme: %v", err) + } + if err := fluentbitv1alpha2.AddToScheme(s); err != nil { + t.Fatalf("failed to add fluentbit scheme: %v", err) + } + r := &FluentBitReconciler{Scheme: s} + + existing := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{Name: "fluent-bit", Namespace: "default"}, + Spec: appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "fluentbit", "team": "logging"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "fluentbit", "team": "logging"}}, + }, + }, + } + + // The user removes "team" from the FluentBit CR's labels. + fbLabels := map[string]string{"app": "fluentbit"} + fb := &fluentbitv1alpha2.FluentBit{ + ObjectMeta: metav1.ObjectMeta{Name: "fluent-bit", Namespace: "default"}, + Spec: fluentbitv1alpha2.FluentBitSpec{Labels: fbLabels}, + } + + if err := r.mutate(existing, fb)(); err != nil { + t.Fatalf("mutate returned an unexpected error: %v", err) + } + + if got := existing.Spec.Selector.MatchLabels; len(got) != 2 || got["team"] != "logging" { + t.Fatalf("expected the immutable selector to stay unchanged, got %v", got) + } + if existing.Spec.Template.Labels["team"] != "logging" { + t.Fatalf("expected pod template to retain the removed selector label, got %v", existing.Spec.Template.Labels) + } + + // fb.Spec.Labels must not have been mutated as a side effect: MakeDaemonSet + // reuses it directly as the DaemonSet's labels/selector map. + if _, ok := fbLabels["team"]; ok { + t.Fatalf("mutate must not write back into fb.Spec.Labels, got %v", fbLabels) + } + if len(fbLabels) != 1 { + t.Fatalf("expected fb.Spec.Labels to remain untouched, got %v", fbLabels) + } +} From 77e24804955dd078ae852838c25d1c13aa563d2d Mon Sep 17 00:00:00 2001 From: Josh Baird Date: Tue, 28 Jul 2026 10:32:27 -0400 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Josh Baird --- controllers/fluentbit_controller.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/controllers/fluentbit_controller.go b/controllers/fluentbit_controller.go index 03d3a22df..8a4ef26b5 100644 --- a/controllers/fluentbit_controller.go +++ b/controllers/fluentbit_controller.go @@ -186,9 +186,7 @@ func (r *FluentBitReconciler) mutate(obj client.Object, fb *fluentbitv1alpha2.Fl templateLabels[k] = v } for k, v := range existingSelector.MatchLabels { - if _, ok := templateLabels[k]; !ok { - templateLabels[k] = v - } + templateLabels[k] = v } o.Spec.Template.Labels = templateLabels }