diff --git a/controllers/fluentbit_controller.go b/controllers/fluentbit_controller.go index bf1d2788b..8a4ef26b5 100644 --- a/controllers/fluentbit_controller.go +++ b/controllers/fluentbit_controller.go @@ -165,11 +165,32 @@ 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 { + 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..2d993bb9a --- /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" || got["app"] != "fluentbit" { + 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) + } +}