From dfc722b8987f07e4b33c2d14417530c4dec38f2c Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Fri, 14 Aug 2026 15:19:59 -0400 Subject: [PATCH 01/10] Move the FelixConfiguration write path behind an interface One implementation per API group, so the v3 path can later use server-side apply without changing aggregated apiserver behavior. --- .../applicationlayer_controller.go | 3 +- .../egressgateway/egressgateway_controller.go | 3 +- .../gatewayapi/gatewayapi_controller.go | 3 +- .../installation/core_controller.go | 5 +- pkg/controller/istio/istio_controller.go | 5 +- pkg/controller/sharedconfig/crdv1.go | 71 ++++++++++++ pkg/controller/sharedconfig/crdv1_test.go | 103 ++++++++++++++++++ .../sharedconfig/sharedconfig_suite_test.go | 32 ++++++ pkg/controller/sharedconfig/writer.go | 35 ++++++ pkg/controller/utils/felix_configuration.go | 37 ------- 10 files changed, 253 insertions(+), 44 deletions(-) create mode 100644 pkg/controller/sharedconfig/crdv1.go create mode 100644 pkg/controller/sharedconfig/crdv1_test.go create mode 100644 pkg/controller/sharedconfig/sharedconfig_suite_test.go create mode 100644 pkg/controller/sharedconfig/writer.go diff --git a/pkg/controller/applicationlayer/applicationlayer_controller.go b/pkg/controller/applicationlayer/applicationlayer_controller.go index d8f0aa103d..01e0817814 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller.go @@ -24,6 +24,7 @@ import ( "github.com/tigera/operator/pkg/common" "github.com/tigera/operator/pkg/controller/gatewayapi" "github.com/tigera/operator/pkg/controller/options" + "github.com/tigera/operator/pkg/controller/sharedconfig" "github.com/tigera/operator/pkg/controller/status" "github.com/tigera/operator/pkg/controller/utils" "github.com/tigera/operator/pkg/controller/utils/imageset" @@ -521,7 +522,7 @@ func (r *ReconcileApplicationLayer) patchFelixConfiguration(ctx context.Context, } istioNeeds := utils.IstioRequiresPolicySync(istioCR, r.variant) - _, err = utils.PatchFelixConfiguration(ctx, r.client, func(fc *v3.FelixConfiguration) (bool, error) { + _, err = sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { wafEventLogsFileEnabled := wafEventLogsFileRequired(al, gatewayWAFEnabled) var tproxyMode string diff --git a/pkg/controller/egressgateway/egressgateway_controller.go b/pkg/controller/egressgateway/egressgateway_controller.go index 745083fe14..62013a6086 100644 --- a/pkg/controller/egressgateway/egressgateway_controller.go +++ b/pkg/controller/egressgateway/egressgateway_controller.go @@ -40,6 +40,7 @@ import ( "github.com/tigera/operator/pkg/components" "github.com/tigera/operator/pkg/controller/options" + "github.com/tigera/operator/pkg/controller/sharedconfig" "github.com/tigera/operator/pkg/controller/status" "github.com/tigera/operator/pkg/controller/utils" "github.com/tigera/operator/pkg/controller/utils/imageset" @@ -288,7 +289,7 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil } // patch and get the felix configuration - fc, err := utils.PatchFelixConfiguration(ctx, r.client, func(fc *v3.FelixConfiguration) (bool, error) { + fc, err := sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { if fc.Spec.PolicySyncPathPrefix != "" { return false, nil // don't proceed with the patch } diff --git a/pkg/controller/gatewayapi/gatewayapi_controller.go b/pkg/controller/gatewayapi/gatewayapi_controller.go index 07ad0001cc..53605f878a 100644 --- a/pkg/controller/gatewayapi/gatewayapi_controller.go +++ b/pkg/controller/gatewayapi/gatewayapi_controller.go @@ -47,6 +47,7 @@ import ( "github.com/tigera/operator/pkg/common" "github.com/tigera/operator/pkg/controller/certificatemanager" "github.com/tigera/operator/pkg/controller/options" + "github.com/tigera/operator/pkg/controller/sharedconfig" "github.com/tigera/operator/pkg/controller/status" "github.com/tigera/operator/pkg/controller/utils" "github.com/tigera/operator/pkg/controller/utils/imageset" @@ -619,7 +620,7 @@ func GetGatewayAPI(ctx context.Context, client client.Client) (*operatorv1.Gatew // patchFelixConfiguration patches the FelixConfiguration resource with the desired policy sync path prefix. func (r *ReconcileGatewayAPI) patchFelixConfiguration(ctx context.Context) error { - _, err := utils.PatchFelixConfiguration(ctx, r.client, func(fc *v3.FelixConfiguration) (bool, error) { + _, err := sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { policySyncPrefix := r.getPolicySyncPathPrefix(&fc.Spec) policySyncPrefixSetDesired := DefaultPolicySyncPrefix == policySyncPrefix diff --git a/pkg/controller/installation/core_controller.go b/pkg/controller/installation/core_controller.go index 105165e93c..7c00a37add 100644 --- a/pkg/controller/installation/core_controller.go +++ b/pkg/controller/installation/core_controller.go @@ -64,6 +64,7 @@ import ( "github.com/tigera/operator/pkg/controller/migration/convert" "github.com/tigera/operator/pkg/controller/migration/datastoremigration" "github.com/tigera/operator/pkg/controller/options" + "github.com/tigera/operator/pkg/controller/sharedconfig" "github.com/tigera/operator/pkg/controller/status" "github.com/tigera/operator/pkg/controller/typhaautoscaler" "github.com/tigera/operator/pkg/controller/utils" @@ -1031,7 +1032,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile } // Set any non-default FelixConfiguration values that we need. - felixConfiguration, err := utils.PatchFelixConfiguration(ctx, r.client, func(fc *v3.FelixConfiguration) (bool, error) { + felixConfiguration, err := sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { // Configure defaults. u, err := r.setDefaultsOnFelixConfiguration(ctx, instance, fc, reqLogger, needsNamespaceMigration) if err != nil { @@ -1444,7 +1445,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile certificateManager.AddToStatusManager(r.status, common.CalicoNamespace) // If eBPF is enabled in the operator API, patch FelixConfiguration to enable it within Felix. - _, err = utils.PatchFelixConfiguration(ctx, r.client, func(fc *v3.FelixConfiguration) (bool, error) { + _, err = sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { return r.setBPFUpdatesOnFelixConfiguration(ctx, instance, fc, reqLogger) }) if err != nil { diff --git a/pkg/controller/istio/istio_controller.go b/pkg/controller/istio/istio_controller.go index ea5d55458e..2fd246e26b 100644 --- a/pkg/controller/istio/istio_controller.go +++ b/pkg/controller/istio/istio_controller.go @@ -36,6 +36,7 @@ import ( operatorv1 "github.com/tigera/operator/api/v1" "github.com/tigera/operator/pkg/controller/istio/waypoint" "github.com/tigera/operator/pkg/controller/options" + "github.com/tigera/operator/pkg/controller/sharedconfig" "github.com/tigera/operator/pkg/controller/status" "github.com/tigera/operator/pkg/controller/utils" "github.com/tigera/operator/pkg/controller/utils/imageset" @@ -257,7 +258,7 @@ func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Reques return reconcile.Result{}, err } - _, err = utils.PatchFelixConfiguration(ctx, r.Client, func(fc *v3.FelixConfiguration) (bool, error) { + _, err = sharedconfig.NewWriter(r.Client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { return r.setIstioFelixConfiguration(ctx, instance, fc, false) }) if err != nil { @@ -415,7 +416,7 @@ func (r *ReconcileIstio) configurePolicySyncPathPrefix(ctx context.Context, inst func (r *ReconcileIstio) maintainFinalizer(ctx context.Context, instance *operatorv1.Istio, reqLogger logr.Logger) (res reconcile.Result, err error, finalized bool) { // Executing clean up on finalizing if !instance.DeletionTimestamp.IsZero() { - if _, err = utils.PatchFelixConfiguration(ctx, r.Client, func(fc *v3.FelixConfiguration) (bool, error) { + if _, err = sharedconfig.NewWriter(r.Client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { return r.setIstioFelixConfiguration(ctx, instance, fc, true) }); err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error cleaning up felix configuration", err, reqLogger) diff --git a/pkg/controller/sharedconfig/crdv1.go b/pkg/controller/sharedconfig/crdv1.go new file mode 100644 index 0000000000..b64780915e --- /dev/null +++ b/pkg/controller/sharedconfig/crdv1.go @@ -0,0 +1,71 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "context" + "fmt" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/tigera/operator/pkg/controller/utils" +) + +// crdV1Writer writes through crd.projectcalico.org/v1, the API group used in aggregated apiserver mode. +type crdV1Writer struct { + client client.Client +} + +var _ Writer = &crdV1Writer{} + +func (w *crdV1Writer) UpdateFelixConfiguration(ctx context.Context, updateFn func(fc *v3.FelixConfiguration) (bool, error)) (*v3.FelixConfiguration, error) { + // Fetch any existing default FelixConfiguration object. + fc := &v3.FelixConfiguration{} + err := w.client.Get(ctx, types.NamespacedName{Name: "default"}, fc) + if err != nil && !errors.IsNotFound(err) { + return nil, fmt.Errorf("unable to read FelixConfiguration: %w", err) + } + + // Create a base state for the upcoming patch operation. + patchFrom := client.MergeFrom(fc.DeepCopy()) + + if err = utils.RestoreV3Metadata(fc); err != nil { + return nil, err + } + + // Apply desired changes to the FelixConfiguration. + updated, err := updateFn(fc) + if err != nil { + return nil, err + } + if updated { + // Apply the patch. + if fc.ResourceVersion == "" { + fc.Name = "default" + if err := w.client.Create(ctx, fc); err != nil { + return nil, err + } + } else { + if err := w.client.Patch(ctx, fc, patchFrom); err != nil { + return nil, err + } + } + } + + return fc, nil +} diff --git a/pkg/controller/sharedconfig/crdv1_test.go b/pkg/controller/sharedconfig/crdv1_test.go new file mode 100644 index 0000000000..1e5f602350 --- /dev/null +++ b/pkg/controller/sharedconfig/crdv1_test.go @@ -0,0 +1,103 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig_test + +import ( + "context" + "errors" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/tigera/operator/pkg/apis" + "github.com/tigera/operator/pkg/controller/sharedconfig" + ctrlrfake "github.com/tigera/operator/pkg/ctrlruntime/client/fake" +) + +var _ = Describe("crd.projectcalico.org/v1 writer", func() { + var c client.Client + var ctx context.Context + var w sharedconfig.Writer + + getFelixConfig := func() *v3.FelixConfiguration { + fc := &v3.FelixConfiguration{} + Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, fc)).NotTo(HaveOccurred()) + return fc + } + + BeforeEach(func() { + scheme := runtime.NewScheme() + Expect(apis.AddToScheme(scheme, false)).NotTo(HaveOccurred()) + c = ctrlrfake.DefaultFakeClientBuilder(scheme).Build() + ctx = context.Background() + w = sharedconfig.NewWriter(c) + }) + + It("should create the default FelixConfiguration when it doesn't exist", func() { + _, err := w.UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + fc.Spec.HealthPort = ptr.To(9099) + return true, nil + }) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9099))) + }) + + It("should patch an existing FelixConfiguration", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: v3.FelixConfigurationSpec{HealthPort: ptr.To(9099)}, + })).NotTo(HaveOccurred()) + + _, err := w.UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + fc.Spec.BPFEnabled = ptr.To(true) + return true, nil + }) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + Expect(fc.Spec.BPFEnabled).To(Equal(ptr.To(true))) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + }) + + It("should not write when the update function reports no change", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}})).NotTo(HaveOccurred()) + before := getFelixConfig().ResourceVersion + + _, err := w.UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + fc.Spec.BPFEnabled = ptr.To(true) + return false, nil + }) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + Expect(fc.ResourceVersion).To(Equal(before)) + Expect(fc.Spec.BPFEnabled).To(BeNil()) + }) + + It("should return the update function's error without writing", func() { + _, err := w.UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + fc.Spec.BPFEnabled = ptr.To(true) + return true, errors.New("user modified bpfEnabled") + }) + Expect(err).To(MatchError("user modified bpfEnabled")) + Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, &v3.FelixConfiguration{})).To(HaveOccurred()) + }) +}) diff --git a/pkg/controller/sharedconfig/sharedconfig_suite_test.go b/pkg/controller/sharedconfig/sharedconfig_suite_test.go new file mode 100644 index 0000000000..59be00d9c2 --- /dev/null +++ b/pkg/controller/sharedconfig/sharedconfig_suite_test.go @@ -0,0 +1,32 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig_test + +import ( + "testing" + + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestSharedConfig(t *testing.T) { + logf.SetLogger(zap.New(zap.WriteTo(ginkgo.GinkgoWriter), zap.UseDevMode(true))) + gomega.RegisterFailHandler(ginkgo.Fail) + suiteConfig, reporterConfig := ginkgo.GinkgoConfiguration() + reporterConfig.JUnitReport = "../../../report/ut/sharedconfig_suite.xml" + ginkgo.RunSpecs(t, "pkg/controller/sharedconfig Suite", suiteConfig, reporterConfig) +} diff --git a/pkg/controller/sharedconfig/writer.go b/pkg/controller/sharedconfig/writer.go new file mode 100644 index 0000000000..622e323eb7 --- /dev/null +++ b/pkg/controller/sharedconfig/writer.go @@ -0,0 +1,35 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig writes operator-owned fields to Calico resources that +// users also modify. One implementation per API group. +package sharedconfig + +import ( + "context" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Writer persists operator-owned fields on shared Calico configuration resources. +type Writer interface { + // UpdateFelixConfiguration applies updateFn to the default FelixConfiguration and persists the result. + UpdateFelixConfiguration(ctx context.Context, updateFn func(fc *v3.FelixConfiguration) (bool, error)) (*v3.FelixConfiguration, error) +} + +// NewWriter returns a Writer for the API group the operator writes through. +func NewWriter(c client.Client) Writer { + return &crdV1Writer{client: c} +} diff --git a/pkg/controller/utils/felix_configuration.go b/pkg/controller/utils/felix_configuration.go index af1d098c9e..a8e4101fd6 100644 --- a/pkg/controller/utils/felix_configuration.go +++ b/pkg/controller/utils/felix_configuration.go @@ -24,43 +24,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func PatchFelixConfiguration(ctx context.Context, c client.Client, patchFn func(fc *v3.FelixConfiguration) (bool, error)) (*v3.FelixConfiguration, error) { - // Fetch any existing default FelixConfiguration object. - fc := &v3.FelixConfiguration{} - err := c.Get(ctx, types.NamespacedName{Name: "default"}, fc) - if err != nil && !errors.IsNotFound(err) { - return nil, fmt.Errorf("unable to read FelixConfiguration: %w", err) - } - - // Create a base state for the upcoming patch operation. - patchFrom := client.MergeFrom(fc.DeepCopy()) - - if err = RestoreV3Metadata(fc); err != nil { - return nil, err - } - - // Apply desired changes to the FelixConfiguration. - updated, err := patchFn(fc) - if err != nil { - return nil, err - } - if updated { - // Apply the patch. - if fc.ResourceVersion == "" { - fc.Name = "default" - if err := c.Create(ctx, fc); err != nil { - return nil, err - } - } else { - if err := c.Patch(ctx, fc, patchFrom); err != nil { - return nil, err - } - } - } - - return fc, nil -} - func GetFelixConfiguration(ctx context.Context, c client.Client) (*v3.FelixConfiguration, error) { fc := &v3.FelixConfiguration{} err := c.Get(ctx, types.NamespacedName{Name: "default"}, fc) From 1b09fa13d763d88394c5af23cc80f300a1284199 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Fri, 14 Aug 2026 15:56:21 -0400 Subject: [PATCH 02/10] Add a server-side apply implementation of the FelixConfiguration writer Field ownership is tracked by the API server in v3 mode, and by a last-written-value annotation in crd.projectcalico.org/v1 mode. --- .../applicationlayer_controller.go | 4 +- .../egressgateway/egressgateway_controller.go | 4 +- .../gatewayapi/gatewayapi_controller.go | 4 +- .../installation/core_controller.go | 4 +- pkg/controller/istio/istio_controller.go | 20 +- pkg/controller/sharedconfig/apply_test.go | 284 ++++++++++++++++++ pkg/controller/sharedconfig/crdv1.go | 107 +++++++ pkg/controller/sharedconfig/crdv1_test.go | 2 +- pkg/controller/sharedconfig/declaration.go | 74 +++++ pkg/controller/sharedconfig/payload.go | 75 +++++ pkg/controller/sharedconfig/tracking.go | 168 +++++++++++ pkg/controller/sharedconfig/v3.go | 131 ++++++++ pkg/controller/sharedconfig/writer.go | 11 +- 13 files changed, 872 insertions(+), 16 deletions(-) create mode 100644 pkg/controller/sharedconfig/apply_test.go create mode 100644 pkg/controller/sharedconfig/declaration.go create mode 100644 pkg/controller/sharedconfig/payload.go create mode 100644 pkg/controller/sharedconfig/tracking.go create mode 100644 pkg/controller/sharedconfig/v3.go diff --git a/pkg/controller/applicationlayer/applicationlayer_controller.go b/pkg/controller/applicationlayer/applicationlayer_controller.go index 01e0817814..41735a1ade 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller.go @@ -81,6 +81,7 @@ func newReconciler(mgr manager.Manager, opts options.ControllerOptions, licenseA provider: opts.DetectedProvider, status: status.New(mgr.GetClient(), "applicationlayer", opts.KubernetesVersion), clusterDomain: opts.ClusterDomain, + useV3CRDs: opts.UseV3CRDs, variant: opts.Variant, licenseAPIReady: licenseAPIReady, } @@ -167,6 +168,7 @@ type ReconcileApplicationLayer struct { provider operatorv1.Provider status status.StatusManager clusterDomain string + useV3CRDs bool variant operatorv1.ProductVariant licenseAPIReady *utils.ReadyFlag } @@ -522,7 +524,7 @@ func (r *ReconcileApplicationLayer) patchFelixConfiguration(ctx context.Context, } istioNeeds := utils.IstioRequiresPolicySync(istioCR, r.variant) - _, err = sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + _, err = sharedconfig.NewWriter(r.client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { wafEventLogsFileEnabled := wafEventLogsFileRequired(al, gatewayWAFEnabled) var tproxyMode string diff --git a/pkg/controller/egressgateway/egressgateway_controller.go b/pkg/controller/egressgateway/egressgateway_controller.go index 62013a6086..5d3d837995 100644 --- a/pkg/controller/egressgateway/egressgateway_controller.go +++ b/pkg/controller/egressgateway/egressgateway_controller.go @@ -85,6 +85,7 @@ func newReconciler(mgr manager.Manager, opts options.ControllerOptions, licenseA provider: opts.DetectedProvider, status: status.New(mgr.GetClient(), "egressgateway", opts.KubernetesVersion), clusterDomain: opts.ClusterDomain, + useV3CRDs: opts.UseV3CRDs, variant: opts.Variant, licenseAPIReady: licenseAPIReady, } @@ -134,6 +135,7 @@ type ReconcileEgressGateway struct { provider operatorv1.Provider status status.StatusManager clusterDomain string + useV3CRDs bool variant operatorv1.ProductVariant licenseAPIReady *utils.ReadyFlag } @@ -289,7 +291,7 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil } // patch and get the felix configuration - fc, err := sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + fc, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { if fc.Spec.PolicySyncPathPrefix != "" { return false, nil // don't proceed with the patch } diff --git a/pkg/controller/gatewayapi/gatewayapi_controller.go b/pkg/controller/gatewayapi/gatewayapi_controller.go index 53605f878a..db299cd5bd 100644 --- a/pkg/controller/gatewayapi/gatewayapi_controller.go +++ b/pkg/controller/gatewayapi/gatewayapi_controller.go @@ -77,6 +77,7 @@ func Add(mgr manager.Manager, opts options.ControllerOptions) error { tierWatchReady: &utils.ReadyFlag{}, status: status.New(mgr.GetClient(), "gatewayapi", opts.KubernetesVersion), clusterDomain: opts.ClusterDomain, + useV3CRDs: opts.UseV3CRDs, variant: opts.Variant, multiTenant: opts.MultiTenant, newComponentHandler: utils.NewComponentHandler, @@ -181,6 +182,7 @@ type ReconcileGatewayAPI struct { tierWatchReady *utils.ReadyFlag status status.StatusManager clusterDomain string + useV3CRDs bool variant operatorv1.ProductVariant multiTenant bool newComponentHandler func(log logr.Logger, client client.Client, scheme *runtime.Scheme, cr metav1.Object, opts ...utils.ComponentHandlerOption) utils.ComponentHandler @@ -620,7 +622,7 @@ func GetGatewayAPI(ctx context.Context, client client.Client) (*operatorv1.Gatew // patchFelixConfiguration patches the FelixConfiguration resource with the desired policy sync path prefix. func (r *ReconcileGatewayAPI) patchFelixConfiguration(ctx context.Context) error { - _, err := sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + _, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { policySyncPrefix := r.getPolicySyncPathPrefix(&fc.Spec) policySyncPrefixSetDesired := DefaultPolicySyncPrefix == policySyncPrefix diff --git a/pkg/controller/installation/core_controller.go b/pkg/controller/installation/core_controller.go index 7c00a37add..ef16cc52cc 100644 --- a/pkg/controller/installation/core_controller.go +++ b/pkg/controller/installation/core_controller.go @@ -1032,7 +1032,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile } // Set any non-default FelixConfiguration values that we need. - felixConfiguration, err := sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + felixConfiguration, err := sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { // Configure defaults. u, err := r.setDefaultsOnFelixConfiguration(ctx, instance, fc, reqLogger, needsNamespaceMigration) if err != nil { @@ -1445,7 +1445,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile certificateManager.AddToStatusManager(r.status, common.CalicoNamespace) // If eBPF is enabled in the operator API, patch FelixConfiguration to enable it within Felix. - _, err = sharedconfig.NewWriter(r.client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + _, err = sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { return r.setBPFUpdatesOnFelixConfiguration(ctx, instance, fc, reqLogger) }) if err != nil { diff --git a/pkg/controller/istio/istio_controller.go b/pkg/controller/istio/istio_controller.go index 2fd246e26b..32f135e52c 100644 --- a/pkg/controller/istio/istio_controller.go +++ b/pkg/controller/istio/istio_controller.go @@ -117,10 +117,11 @@ func Add(mgr manager.Manager, opts options.ControllerOptions) error { // newReconciler returns a new reconcile.Reconciler func newReconciler(mgr manager.Manager, opts options.ControllerOptions) *ReconcileIstio { r := &ReconcileIstio{ - Client: mgr.GetClient(), - scheme: mgr.GetScheme(), - status: status.New(mgr.GetClient(), "istio", opts.KubernetesVersion), - provider: opts.DetectedProvider, + Client: mgr.GetClient(), + scheme: mgr.GetScheme(), + status: status.New(mgr.GetClient(), "istio", opts.KubernetesVersion), + provider: opts.DetectedProvider, + useV3CRDs: opts.UseV3CRDs, } r.status.Run(opts.ShutdownContext) @@ -130,9 +131,10 @@ func newReconciler(mgr manager.Manager, opts options.ControllerOptions) *Reconci // ReconcileIstio reconciles a Istio object type ReconcileIstio struct { client.Client - scheme *runtime.Scheme - status status.StatusManager - provider operatorv1.Provider + scheme *runtime.Scheme + status status.StatusManager + provider operatorv1.Provider + useV3CRDs bool } func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { @@ -258,7 +260,7 @@ func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Reques return reconcile.Result{}, err } - _, err = sharedconfig.NewWriter(r.Client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + _, err = sharedconfig.NewWriter(r.Client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { return r.setIstioFelixConfiguration(ctx, instance, fc, false) }) if err != nil { @@ -416,7 +418,7 @@ func (r *ReconcileIstio) configurePolicySyncPathPrefix(ctx context.Context, inst func (r *ReconcileIstio) maintainFinalizer(ctx context.Context, instance *operatorv1.Istio, reqLogger logr.Logger) (res reconcile.Result, err error, finalized bool) { // Executing clean up on finalizing if !instance.DeletionTimestamp.IsZero() { - if _, err = sharedconfig.NewWriter(r.Client).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + if _, err = sharedconfig.NewWriter(r.Client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { return r.setIstioFelixConfiguration(ctx, instance, fc, true) }); err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error cleaning up felix configuration", err, reqLogger) diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go new file mode 100644 index 0000000000..89460b3a36 --- /dev/null +++ b/pkg/controller/sharedconfig/apply_test.go @@ -0,0 +1,284 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/tigera/operator/pkg/apis" + "github.com/tigera/operator/pkg/controller/sharedconfig" + ctrlrfake "github.com/tigera/operator/pkg/ctrlruntime/client/fake" + "github.com/tigera/operator/pkg/render" +) + +// declare returns a declaration of healthPort and vxlanPort, with a policy per field. +func declare(healthPolicy, vxlanPolicy sharedconfig.ConflictPolicy) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: "installation", + Owned: &v3.FelixConfiguration{ + Spec: v3.FelixConfigurationSpec{ + HealthPort: ptr.To(9099), + VXLANPort: ptr.To(4789), + }, + }, + Policies: map[string]sharedconfig.ConflictPolicy{ + "spec.healthPort": healthPolicy, + "spec.vxlanPort": vxlanPolicy, + }, + }, nil + } +} + +var _ = Describe("Applying declared FelixConfiguration fields", func() { + var c client.Client + var ctx context.Context + + getFelixConfig := func() *v3.FelixConfiguration { + fc := &v3.FelixConfiguration{} + Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, fc)).NotTo(HaveOccurred()) + return fc + } + + Context("projectcalico.org/v3, where the API server tracks ownership", func() { + var w sharedconfig.Writer + + // applyAs writes healthPort as another field manager, taking the field if it has to. + applyAs := func(manager string, healthPort int64) { + other := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "projectcalico.org/v3", + "kind": "FelixConfiguration", + "metadata": map[string]any{"name": "default"}, + "spec": map[string]any{"healthPort": healthPort}, + }} + Expect(c.Patch(ctx, other, client.Apply, client.FieldOwner(manager), client.ForceOwnership)).NotTo(HaveOccurred()) + } + + BeforeEach(func() { + scheme := runtime.NewScheme() + Expect(apis.AddToScheme(scheme, true)).NotTo(HaveOccurred()) + c = ctrlrfake.DefaultFakeClientBuilder(scheme).WithReturnManagedFields().Build() + ctx = context.Background() + w = sharedconfig.NewWriter(c, true) + }) + + It("should create the FelixConfiguration owning only the declared fields", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + Expect(fc.Spec.VXLANPort).To(Equal(ptr.To(4789))) + Expect(fc.ManagedFields).To(HaveLen(1)) + Expect(fc.ManagedFields[0].Manager).To(Equal("tigera-operator/installation")) + Expect(fc.ManagedFields[0].Operation).To(Equal(metav1.ManagedFieldsOperationApply)) + }) + + It("should keep the same values when it applies the same declaration twice", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + Expect(fc.Spec.VXLANPort).To(Equal(ptr.To(4789))) + Expect(getFelixConfig().ManagedFields).To(HaveLen(1)) + }) + + It("should leave a deferred field with the other owner and still write the rest", func() { + applyAs("kubectl", 9100) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9100))) + Expect(fc.Spec.VXLANPort).To(Equal(ptr.To(4789))) + + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9100))) + }) + + It("should take an overridden field back", func() { + applyAs("kubectl", 9100) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictOverride, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9099))) + }) + + It("should report a conflict on a field it refuses to take", func() { + applyAs("kubectl", 9100) + + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictError, sharedconfig.ConflictDefer)) + Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) + Expect(err.(*sharedconfig.ConflictingFieldsError).Paths).To(ConsistOf("spec.healthPort")) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9100))) + }) + + It("should delete a field it stops declaring, so the declared set has to stay stable", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: "installation", + Owned: &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{HealthPort: ptr.To(9099)}}, + Policies: map[string]sharedconfig.ConflictPolicy{"spec.healthPort": sharedconfig.ConflictDefer}, + }, nil + }) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.VXLANPort).To(BeNil()) + }) + }) + + Context("crd.projectcalico.org/v1, where the operator tracks what it wrote", func() { + var w sharedconfig.Writer + + BeforeEach(func() { + scheme := runtime.NewScheme() + Expect(apis.AddToScheme(scheme, false)).NotTo(HaveOccurred()) + c = ctrlrfake.DefaultFakeClientBuilder(scheme).Build() + ctx = context.Background() + w = sharedconfig.NewWriter(c, false) + }) + + It("should create the FelixConfiguration and record the values it wrote", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + Expect(fc.Spec.VXLANPort).To(Equal(ptr.To(4789))) + Expect(fc.Annotations).To(HaveKeyWithValue("operator.tigera.io/owned-fields", + `{"spec.healthPort":9099,"spec.vxlanPort":4789}`)) + }) + + It("should not write again when the declaration has not changed", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + before := getFelixConfig().ResourceVersion + + _, err = w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().ResourceVersion).To(Equal(before)) + }) + + It("should leave a deferred field alone and drop it from the record", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + fc.Spec.HealthPort = ptr.To(9100) + Expect(c.Update(ctx, fc)).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc = getFelixConfig() + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9100))) + Expect(fc.Annotations).To(HaveKeyWithValue("operator.tigera.io/owned-fields", `{"spec.vxlanPort":4789}`)) + }) + + It("should take an overridden field back", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictOverride, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + fc.Spec.HealthPort = ptr.To(9100) + Expect(c.Update(ctx, fc)).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictOverride, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9099))) + }) + + It("should report a conflict on a field it refuses to take", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictError, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + fc.Spec.HealthPort = ptr.To(9100) + Expect(c.Update(ctx, fc)).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictError, sharedconfig.ConflictDefer)) + Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9100))) + }) + + It("should treat a value it has no record of as someone else's", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: v3.FelixConfigurationSpec{HealthPort: ptr.To(9100)}, + })).NotTo(HaveOccurred()) + + _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictError, sharedconfig.ConflictDefer)) + Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) + }) + + Context("bpfEnabled, which older operators recorded in their own annotation", func() { + declareBPF := func(policy sharedconfig.ConflictPolicy) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: "installation", + Owned: &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}}, + Policies: map[string]sharedconfig.ConflictPolicy{"spec.bpfEnabled": policy}, + }, nil + } + } + + It("should accept the legacy annotation as its own record", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Annotations: map[string]string{render.BPFOperatorAnnotation: "true"}, + }, + Spec: v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}, + })).NotTo(HaveOccurred()) + + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(true))) + }) + + It("should report a conflict when the legacy annotation disagrees with the field", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Annotations: map[string]string{render.BPFOperatorAnnotation: "false"}, + }, + Spec: v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}, + })).NotTo(HaveOccurred()) + + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).To(MatchError(ContainSubstring("spec.bpfEnabled"))) + }) + + It("should keep the legacy annotation in step with what it writes", func() { + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Annotations).To(HaveKeyWithValue(render.BPFOperatorAnnotation, "true")) + }) + }) + }) +}) diff --git a/pkg/controller/sharedconfig/crdv1.go b/pkg/controller/sharedconfig/crdv1.go index b64780915e..233b6c2858 100644 --- a/pkg/controller/sharedconfig/crdv1.go +++ b/pkg/controller/sharedconfig/crdv1.go @@ -17,15 +17,25 @@ package sharedconfig import ( "context" "fmt" + "sort" v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/tigera/operator/pkg/controller/utils" ) +// ownedFieldsAnnotation records the values the operator last wrote, so it can spot changes by others. +const ownedFieldsAnnotation = "operator.tigera.io/owned-fields" + +// bpfEnabledPath is tracked by its own legacy annotation, which predates ownedFieldsAnnotation. +const bpfEnabledPath = "spec.bpfEnabled" + // crdV1Writer writes through crd.projectcalico.org/v1, the API group used in aggregated apiserver mode. type crdV1Writer struct { client client.Client @@ -33,6 +43,103 @@ type crdV1Writer struct { var _ Writer = &crdV1Writer{} +// ApplyFelixConfiguration writes the declared fields, comparing each against the value the operator +// last wrote to spot changes made by others. +func (w *crdV1Writer) ApplyFelixConfiguration(ctx context.Context, declare DeclareFelixConfiguration) (*v3.FelixConfiguration, error) { + current, err := utils.GetFelixConfiguration(ctx, w.client) + if err != nil { + return nil, err + } + patchFrom := client.MergeFrom(current.DeepCopy()) + if err := utils.RestoreV3Metadata(current); err != nil { + return nil, err + } + + declaration, err := declare(current) + if err != nil { + return nil, err + } + if declaration == nil { + return current, nil + } + + payload, err := declaredPayload(declaration.Owned) + if err != nil { + return nil, err + } + deferred, err := resolveTrackedConflicts(current, declaration, payload) + if err != nil { + return nil, err + } + + merged := current.DeepCopy() + if err := mergeInto(merged, payload); err != nil { + return nil, err + } + if err := recordWrittenValues(merged, payload, declaration, deferred); err != nil { + return nil, err + } + if equality.Semantic.DeepEqual(current, merged) { + return current, nil + } + return w.persist(ctx, merged, patchFrom) +} + +// resolveTrackedConflicts drops deferred fields from payload and returns the paths it dropped. +func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) ([]string, error) { + currentContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(current) + if err != nil { + return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + lastWritten, err := lastWrittenValues(current) + if err != nil { + return nil, err + } + + var deferred, refused []string + for path := range d.Policies { + if !pathSet(payload.Object, path) { + continue + } + changed, err := changedByOther(currentContent, lastWritten, path) + if err != nil { + return nil, err + } + if !changed { + continue + } + + switch d.Policies[path] { + case ConflictDefer: + removePath(payload.Object, path) + deferred = append(deferred, path) + case ConflictOverride: + default: + refused = append(refused, path) + } + } + + if len(refused) > 0 { + sort.Strings(refused) + return nil, &ConflictingFieldsError{Paths: refused} + } + return deferred, nil +} + +func (w *crdV1Writer) persist(ctx context.Context, fc *v3.FelixConfiguration, patchFrom client.Patch) (*v3.FelixConfiguration, error) { + if fc.ResourceVersion == "" { + fc.Name = defaultFelixConfigName + if err := w.client.Create(ctx, fc); err != nil { + return nil, err + } + return fc, nil + } + if err := w.client.Patch(ctx, fc, patchFrom); err != nil { + return nil, err + } + return fc, nil +} + func (w *crdV1Writer) UpdateFelixConfiguration(ctx context.Context, updateFn func(fc *v3.FelixConfiguration) (bool, error)) (*v3.FelixConfiguration, error) { // Fetch any existing default FelixConfiguration object. fc := &v3.FelixConfiguration{} diff --git a/pkg/controller/sharedconfig/crdv1_test.go b/pkg/controller/sharedconfig/crdv1_test.go index 1e5f602350..6094ba0248 100644 --- a/pkg/controller/sharedconfig/crdv1_test.go +++ b/pkg/controller/sharedconfig/crdv1_test.go @@ -48,7 +48,7 @@ var _ = Describe("crd.projectcalico.org/v1 writer", func() { Expect(apis.AddToScheme(scheme, false)).NotTo(HaveOccurred()) c = ctrlrfake.DefaultFakeClientBuilder(scheme).Build() ctx = context.Background() - w = sharedconfig.NewWriter(c) + w = sharedconfig.NewWriter(c, false) }) It("should create the default FelixConfiguration when it doesn't exist", func() { diff --git a/pkg/controller/sharedconfig/declaration.go b/pkg/controller/sharedconfig/declaration.go new file mode 100644 index 0000000000..911d86eb11 --- /dev/null +++ b/pkg/controller/sharedconfig/declaration.go @@ -0,0 +1,74 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "fmt" + "strings" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" +) + +// ConflictPolicy resolves a field that both the operator and someone else set. +type ConflictPolicy string + +const ( + // ConflictError reports the conflict to the caller, which should degrade. + ConflictError ConflictPolicy = "Error" + + // ConflictDefer leaves the other writer's value in place. + ConflictDefer ConflictPolicy = "Defer" + + // ConflictOverride takes the field back and writes the operator's value. + ConflictOverride ConflictPolicy = "Override" +) + +// FelixConfigurationDeclaration is one field manager's statement of what it owns. +type FelixConfigurationDeclaration struct { + // Manager is the field manager name, and has to stay the same across reconciles. + Manager string + + // Owned carries the declared fields and nothing else. Fields left nil are not owned. + Owned *v3.FelixConfiguration + + // Policies is keyed by field path, e.g. "spec.healthPort". Every declared field needs an entry. + Policies map[string]ConflictPolicy +} + +// policyFor returns the policy governing path, which may name a field below a declared one. +func (d *FelixConfigurationDeclaration) policyFor(path string) (string, ConflictPolicy, bool) { + best := "" + for declared := range d.Policies { + if path != declared && !strings.HasPrefix(path, declared+".") { + continue + } + if len(declared) > len(best) { + best = declared + } + } + if best == "" { + return "", "", false + } + return best, d.Policies[best], true +} + +// ConflictingFieldsError reports fields the operator declares that someone else owns. +type ConflictingFieldsError struct { + Paths []string +} + +func (e *ConflictingFieldsError) Error() string { + return fmt.Sprintf("FelixConfiguration fields modified outside the operator: %s", strings.Join(e.Paths, ", ")) +} diff --git a/pkg/controller/sharedconfig/payload.go b/pkg/controller/sharedconfig/payload.go new file mode 100644 index 0000000000..f2f2887b62 --- /dev/null +++ b/pkg/controller/sharedconfig/payload.go @@ -0,0 +1,75 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "errors" + "fmt" + "strings" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +// defaultFelixConfigName is the only FelixConfiguration the operator writes. +const defaultFelixConfigName = "default" + +// declaredPayload renders the declared fields as an object carrying no other state. +func declaredPayload(owned *v3.FelixConfiguration) (*unstructured.Unstructured, error) { + if owned == nil { + owned = &v3.FelixConfiguration{} + } + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(owned) + if err != nil { + return nil, fmt.Errorf("unable to render FelixConfiguration fields: %w", err) + } + + u := &unstructured.Unstructured{Object: content} + unstructured.RemoveNestedField(u.Object, "metadata") + unstructured.RemoveNestedField(u.Object, "status") + u.SetName(defaultFelixConfigName) + return u, nil +} + +// pathSet reports whether path holds a value in obj. +func pathSet(obj map[string]any, path string) bool { + _, found, err := unstructured.NestedFieldNoCopy(obj, strings.Split(path, ".")...) + return err == nil && found +} + +// removePath drops path from obj, so the operator stops claiming it. +func removePath(obj map[string]any, path string) { + unstructured.RemoveNestedField(obj, strings.Split(path, ".")...) +} + +// conflictPaths lists the fields an apply was rejected for, normalized to "spec.field" form. +func conflictPaths(err error) []string { + var status apierrors.APIStatus + if !errors.As(err, &status) || status.Status().Details == nil { + return nil + } + + var paths []string + for _, cause := range status.Status().Details.Causes { + if cause.Type != metav1.CauseTypeFieldManagerConflict { + continue + } + paths = append(paths, strings.TrimPrefix(cause.Field, ".")) + } + return paths +} diff --git a/pkg/controller/sharedconfig/tracking.go b/pkg/controller/sharedconfig/tracking.go new file mode 100644 index 0000000000..b1fb1ca39e --- /dev/null +++ b/pkg/controller/sharedconfig/tracking.go @@ -0,0 +1,168 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "encoding/json" + "fmt" + "reflect" + "strconv" + "strings" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + + "github.com/tigera/operator/pkg/render" +) + +// lastWrittenValues reads back the values the operator recorded on its previous write. +func lastWrittenValues(fc *v3.FelixConfiguration) (map[string]any, error) { + values := map[string]any{} + if raw := fc.Annotations[ownedFieldsAnnotation]; raw != "" { + if err := json.Unmarshal([]byte(raw), &values); err != nil { + return nil, fmt.Errorf("unable to parse %s annotation: %w", ownedFieldsAnnotation, err) + } + } + + // Clusters last written by an older operator only have the legacy annotation. + if _, ok := values[bpfEnabledPath]; !ok { + if raw := fc.Annotations[render.BPFOperatorAnnotation]; raw != "" { + enabled, err := strconv.ParseBool(raw) + if err != nil { + return nil, fmt.Errorf("unable to parse %s annotation: %w", render.BPFOperatorAnnotation, err) + } + values[bpfEnabledPath] = enabled + } + } + return values, nil +} + +// changedByOther reports whether path holds a value the operator did not write. +func changedByOther(currentContent map[string]any, lastWritten map[string]any, path string) (bool, error) { + current, found, err := unstructured.NestedFieldNoCopy(currentContent, strings.Split(path, ".")...) + if err != nil { + return false, fmt.Errorf("unable to read %s: %w", path, err) + } + if !found { + return false, nil + } + + written, recorded := lastWritten[path] + if !recorded { + return true, nil + } + canonical, err := canonicalize(current) + if err != nil { + return false, err + } + return !reflect.DeepEqual(canonical, written), nil +} + +// recordWrittenValues stores the values being written so the next reconcile can compare against them. +func recordWrittenValues(fc *v3.FelixConfiguration, payload *unstructured.Unstructured, d *FelixConfigurationDeclaration, deferred []string) error { + values, err := lastWrittenValues(fc) + if err != nil { + return err + } + for _, path := range deferred { + delete(values, path) + } + + for path := range d.Policies { + written, found, err := unstructured.NestedFieldNoCopy(payload.Object, strings.Split(path, ".")...) + if err != nil { + return fmt.Errorf("unable to read %s: %w", path, err) + } + if !found { + continue + } + if values[path], err = canonicalize(written); err != nil { + return err + } + } + + encoded, err := json.Marshal(values) + if err != nil { + return fmt.Errorf("unable to record written fields: %w", err) + } + annotations := fc.Annotations + if annotations == nil { + annotations = map[string]string{} + } + annotations[ownedFieldsAnnotation] = string(encoded) + + // Keep the legacy annotation in step, so a rollback to an older operator still reads it. + if enabled, ok := values[bpfEnabledPath].(bool); ok { + annotations[render.BPFOperatorAnnotation] = strconv.FormatBool(enabled) + } else { + delete(annotations, render.BPFOperatorAnnotation) + } + fc.SetAnnotations(annotations) + return nil +} + +// mergeInto overlays the declared fields onto fc, leaving every other field alone. +func mergeInto(fc *v3.FelixConfiguration, payload *unstructured.Unstructured) error { + declared, _, err := unstructured.NestedMap(payload.Object, "spec") + if err != nil { + return fmt.Errorf("unable to read declared fields: %w", err) + } + if len(declared) == 0 { + return nil + } + + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(fc) + if err != nil { + return fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + spec, _, err := unstructured.NestedMap(content, "spec") + if err != nil { + return fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + if spec == nil { + spec = map[string]any{} + } + mergeMaps(spec, declared) + if err := unstructured.SetNestedMap(content, spec, "spec"); err != nil { + return err + } + return runtime.DefaultUnstructuredConverter.FromUnstructured(content, fc) +} + +func mergeMaps(dst, src map[string]any) { + for key, value := range src { + if srcMap, ok := value.(map[string]any); ok { + if dstMap, ok := dst[key].(map[string]any); ok { + mergeMaps(dstMap, srcMap) + continue + } + } + dst[key] = value + } +} + +// canonicalize renders a value the way it will read back out of the annotation. +func canonicalize(value any) (any, error) { + encoded, err := json.Marshal(value) + if err != nil { + return nil, fmt.Errorf("unable to encode field value: %w", err) + } + var decoded any + if err := json.Unmarshal(encoded, &decoded); err != nil { + return nil, fmt.Errorf("unable to decode field value: %w", err) + } + return decoded, nil +} diff --git a/pkg/controller/sharedconfig/v3.go b/pkg/controller/sharedconfig/v3.go new file mode 100644 index 0000000000..e7534e25a1 --- /dev/null +++ b/pkg/controller/sharedconfig/v3.go @@ -0,0 +1,131 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "context" + "fmt" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + + "github.com/tigera/operator/pkg/controller/utils" +) + +// fieldManagerPrefix namespaces the operator's field managers away from other writers. +const fieldManagerPrefix = "tigera-operator/" + +// v3Writer writes through projectcalico.org/v3, where the API server tracks the operator's fields. +type v3Writer struct { + crdV1Writer +} + +var _ Writer = &v3Writer{} + +func (w *v3Writer) ApplyFelixConfiguration(ctx context.Context, declare DeclareFelixConfiguration) (*v3.FelixConfiguration, error) { + current, err := utils.GetFelixConfiguration(ctx, w.client) + if err != nil { + return nil, err + } + + declaration, err := declare(current) + if err != nil { + return nil, err + } + if declaration == nil { + return current, nil + } + + payload, err := declaredPayload(declaration.Owned) + if err != nil { + return nil, err + } + + applied, err := w.apply(ctx, payload, declaration.Manager, false) + if err == nil { + return applied, nil + } + if !apierrors.IsConflict(err) { + return nil, err + } + + force, err := w.resolveConflicts(err, declaration, payload) + if err != nil { + return nil, err + } + return w.apply(ctx, payload, declaration.Manager, force) +} + +// resolveConflicts drops deferred fields from payload and reports whether the retry must force. +func (w *v3Writer) resolveConflicts(applyErr error, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) (bool, error) { + paths := conflictPaths(applyErr) + if len(paths) == 0 { + return false, applyErr + } + + force := false + var undeclared, refused []string + for _, path := range paths { + declared, policy, ok := d.policyFor(path) + if !ok { + undeclared = append(undeclared, path) + continue + } + switch policy { + case ConflictDefer: + removePath(payload.Object, declared) + case ConflictOverride: + force = true + default: + refused = append(refused, declared) + } + } + + if len(undeclared) > 0 { + return false, fmt.Errorf("conflict on fields with no declared policy %v: %w", undeclared, applyErr) + } + if len(refused) > 0 { + return false, &ConflictingFieldsError{Paths: refused} + } + return force, nil +} + +func (w *v3Writer) apply(ctx context.Context, payload *unstructured.Unstructured, manager string, force bool) (*v3.FelixConfiguration, error) { + opts := []client.PatchOption{client.FieldOwner(fieldManagerPrefix + manager)} + if force { + opts = append(opts, client.ForceOwnership) + } + + gvk, err := apiutil.GVKForObject(&v3.FelixConfiguration{}, w.client.Scheme()) + if err != nil { + return nil, err + } + + applied := payload.DeepCopy() + applied.SetGroupVersionKind(gvk) + if err := w.client.Patch(ctx, applied, client.Apply, opts...); err != nil { + return nil, err + } + + fc := &v3.FelixConfiguration{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(applied.Object, fc); err != nil { + return nil, fmt.Errorf("unable to read back applied FelixConfiguration: %w", err) + } + return fc, nil +} diff --git a/pkg/controller/sharedconfig/writer.go b/pkg/controller/sharedconfig/writer.go index 622e323eb7..65d8c05cd5 100644 --- a/pkg/controller/sharedconfig/writer.go +++ b/pkg/controller/sharedconfig/writer.go @@ -23,13 +23,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// DeclareFelixConfiguration states which FelixConfiguration fields the caller owns, given the current object. +type DeclareFelixConfiguration func(current *v3.FelixConfiguration) (*FelixConfigurationDeclaration, error) + // Writer persists operator-owned fields on shared Calico configuration resources. type Writer interface { // UpdateFelixConfiguration applies updateFn to the default FelixConfiguration and persists the result. UpdateFelixConfiguration(ctx context.Context, updateFn func(fc *v3.FelixConfiguration) (bool, error)) (*v3.FelixConfiguration, error) + + // ApplyFelixConfiguration writes the declared fields and returns the whole resulting object. + ApplyFelixConfiguration(ctx context.Context, declare DeclareFelixConfiguration) (*v3.FelixConfiguration, error) } // NewWriter returns a Writer for the API group the operator writes through. -func NewWriter(c client.Client) Writer { +func NewWriter(c client.Client, useV3CRDs bool) Writer { + if useV3CRDs { + return &v3Writer{crdV1Writer{client: c}} + } return &crdV1Writer{client: c} } From 259567d0142cec63feda55296a635c38de43c045 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Fri, 14 Aug 2026 16:17:36 -0400 Subject: [PATCH 03/10] Migrate the installation controller onto declarative FelixConfiguration writes Fields are now declared unconditionally with a conflict policy, since a field an owner stops declaring gets deleted. --- pkg/controller/installation/bpf.go | 61 ---- pkg/controller/installation/bpf_test.go | 137 --------- .../installation/core_controller.go | 262 +----------------- pkg/controller/installation/felixconfig.go | 179 ++++++++++++ .../installation/felixconfig_test.go | 101 +++++++ pkg/controller/sharedconfig/apply_test.go | 16 +- pkg/controller/sharedconfig/crdv1.go | 9 + pkg/controller/sharedconfig/tracking.go | 14 + pkg/enterprise/installation/core.go | 24 +- pkg/enterprise/installation/core_test.go | 36 ++- pkg/extensions/installation.go | 10 +- 11 files changed, 365 insertions(+), 484 deletions(-) create mode 100644 pkg/controller/installation/felixconfig.go create mode 100644 pkg/controller/installation/felixconfig_test.go diff --git a/pkg/controller/installation/bpf.go b/pkg/controller/installation/bpf.go index b310ed4f6c..e6fb70b629 100644 --- a/pkg/controller/installation/bpf.go +++ b/pkg/controller/installation/bpf.go @@ -15,7 +15,6 @@ package installation import ( - "errors" "reflect" "strconv" @@ -26,34 +25,8 @@ import ( "github.com/tigera/operator/pkg/render" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/utils/ptr" ) -// bpfValidateAnnotations validate Felix Configuration annotations match BPF Enabled spec for all scenarios. -func bpfValidateAnnotations(fc *v3.FelixConfiguration) error { - var annotationValue *bool - if fc.Annotations[render.BPFOperatorAnnotation] != "" { - v, err := strconv.ParseBool(fc.Annotations[render.BPFOperatorAnnotation]) - annotationValue = &v - if err != nil { - return err - } - } - - // The values are considered matching if one of the following is true: - // - Both values are nil - // - Neither are nil and they have the same value. - // Otherwise, the we consider the annotation to not match the spec field. - match := annotationValue == nil && fc.Spec.BPFEnabled == nil - match = match || annotationValue != nil && fc.Spec.BPFEnabled != nil && *annotationValue == *fc.Spec.BPFEnabled - - if !match { - return errors.New(`unable to set bpfEnabled: FelixConfiguration "default" has been modified by someone else, refusing to override potential user configuration`) - } - - return nil -} - // isRolloutCompleteWithBPFVolumes checks if the calico-node DaemonSet // rollout process is completed with BPF volume mount been created. // If the Installation resource has been patched to dataplane: BPF then the @@ -83,28 +56,6 @@ func isRolloutCompleteWithBPFVolumes(ds *appsv1.DaemonSet) bool { return false } -func setBPFEnabledOnFelixConfiguration(fc *v3.FelixConfiguration, bpfEnabled bool) error { - err := bpfValidateAnnotations(fc) - if err != nil { - return err - } - - text := strconv.FormatBool(bpfEnabled) - - // Add an annotation matching the field value. This allows the operator to compare the annotation to the field - // when performing an update to determine if another entity has modified the value since the last write. - var fcAnnotations map[string]string - if fc.Annotations == nil { - fcAnnotations = make(map[string]string) - } else { - fcAnnotations = fc.Annotations - } - fcAnnotations[render.BPFOperatorAnnotation] = text - fc.SetAnnotations(fcAnnotations) - fc.Spec.BPFEnabled = &bpfEnabled - return nil -} - func bpfEnabledOnDaemonsetWithEnvVar(ds *appsv1.DaemonSet) (bool, error) { bpfEnabledStatus := false var err error @@ -125,15 +76,3 @@ func bpfEnabledOnDaemonsetWithEnvVar(ds *appsv1.DaemonSet) (bool, error) { func bpfEnabledOnFelixConfig(fc *v3.FelixConfiguration) bool { return fc.Spec.BPFEnabled != nil && *fc.Spec.BPFEnabled } - -func disableBPFHostConntrackBypass(fc *v3.FelixConfiguration) { - hostConntrackBypassDisabled := false - fc.Spec.BPFHostConntrackBypass = &hostConntrackBypassDisabled -} - -// disableBPFKubeProxyHealthz disables Felix's BPF kube-proxy healthz server by setting -// BPFKubeProxyHealthzPort to 0. Use when Calico runs in BPF mode but the platform's -// kube-proxy is still running (e.g. AKS) and holds the default port (10256). -func disableBPFKubeProxyHealthz(fc *v3.FelixConfiguration) { - fc.Spec.BPFKubeProxyHealthzPort = ptr.To(0) -} diff --git a/pkg/controller/installation/bpf_test.go b/pkg/controller/installation/bpf_test.go index 7f42d95ccc..54b7aa5850 100644 --- a/pkg/controller/installation/bpf_test.go +++ b/pkg/controller/installation/bpf_test.go @@ -15,8 +15,6 @@ package installation import ( - "strconv" - v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" "github.com/tigera/operator/pkg/common" @@ -31,81 +29,6 @@ import ( ) var _ = Describe("BPF functional tests", func() { - Context("Annotations validation tests", func() { - var fc *v3.FelixConfiguration - var textTrue, textFalse string - var enabled, notEnabled bool - - textTrue = strconv.FormatBool(true) - textFalse = strconv.FormatBool(false) - - enabled = true - notEnabled = false - - BeforeEach(func() { - fc = &v3.FelixConfiguration{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Annotations: map[string]string{"foo": "bar"}, - }, - Spec: v3.FelixConfigurationSpec{}, - } - }) - - It("should return error if the value is not a boolean", func() { - fc.Annotations[render.BPFOperatorAnnotation] = "NotBoolean" - err := bpfValidateAnnotations(fc) - Expect(err).Should(HaveOccurred()) - }) - - It("should return error if the annotation is nil and the spec field is not", func() { - fc.Annotations = nil - fc.Spec.BPFEnabled = &enabled - err := bpfValidateAnnotations(fc) - Expect(err).Should(HaveOccurred()) - }) - - It("should return error if the annotation is not nil and the spec field is", func() { - fc.Annotations[render.BPFOperatorAnnotation] = textFalse - err := bpfValidateAnnotations(fc) - Expect(err).Should(HaveOccurred()) - }) - - It("should return error if the annotation is true and the spec field is false", func() { - fc.Annotations[render.BPFOperatorAnnotation] = textTrue - fc.Spec.BPFEnabled = ¬Enabled - err := bpfValidateAnnotations(fc) - Expect(err).Should(HaveOccurred()) - }) - - It("should return error if the annotation is false and the spec field is true", func() { - fc.Annotations[render.BPFOperatorAnnotation] = textFalse - fc.Spec.BPFEnabled = &enabled - err := bpfValidateAnnotations(fc) - Expect(err).Should(HaveOccurred()) - }) - - It("should return valid if both annotation and the spec field are nil", func() { - fc.Annotations = nil - err := bpfValidateAnnotations(fc) - Expect(err).ShouldNot(HaveOccurred()) - }) - - It("should return valid if the annotation is false and the spec field is false", func() { - fc.Annotations[render.BPFOperatorAnnotation] = textFalse - fc.Spec.BPFEnabled = ¬Enabled - err := bpfValidateAnnotations(fc) - Expect(err).ShouldNot(HaveOccurred()) - }) - - It("should return valid if the annotation is true and the spec field is true", func() { - fc.Annotations[render.BPFOperatorAnnotation] = textTrue - fc.Spec.BPFEnabled = &enabled - err := bpfValidateAnnotations(fc) - Expect(err).ShouldNot(HaveOccurred()) - }) - }) - Context("Daemonset rollout completion tests", func() { var ds *appsv1.DaemonSet var bpfVolume corev1.Volume @@ -251,64 +174,4 @@ var _ = Describe("BPF functional tests", func() { }) }) - Context("setBPFEnabledOnFelixConfiguration tests", func() { - var fc *v3.FelixConfiguration - - BeforeEach(func() { - fc = &v3.FelixConfiguration{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - }, - Spec: v3.FelixConfigurationSpec{}, - } - }) - - It("should return error if annotation validation failed", func() { - fc.Annotations = make(map[string]string) - fc.Annotations[render.BPFOperatorAnnotation] = "NotBoolean" - err := bpfValidateAnnotations(fc) - Expect(err).Should(HaveOccurred()) - err = setBPFEnabledOnFelixConfiguration(fc, true) - Expect(err).Should(HaveOccurred()) - }) - - It("should set correct annotation", func() { - err := setBPFEnabledOnFelixConfiguration(fc, true) - Expect(err).ShouldNot(HaveOccurred()) - - annotations := fc.Annotations[render.BPFOperatorAnnotation] - Expect(annotations).To(Equal("true")) - Expect(*fc.Spec.BPFEnabled).To(Equal(true)) - - err = setBPFEnabledOnFelixConfiguration(fc, false) - Expect(err).ShouldNot(HaveOccurred()) - - annotations = fc.Annotations[render.BPFOperatorAnnotation] - Expect(annotations).To(Equal("false")) - Expect(*fc.Spec.BPFEnabled).To(Equal(false)) - }) - }) - - Context("disableBPFKubeProxyHealthz tests", func() { - It("should set BPFKubeProxyHealthzPort to 0", func() { - fc := &v3.FelixConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "default"}, - Spec: v3.FelixConfigurationSpec{}, - } - disableBPFKubeProxyHealthz(fc) - Expect(fc.Spec.BPFKubeProxyHealthzPort).ShouldNot(BeNil()) - Expect(*fc.Spec.BPFKubeProxyHealthzPort).To(Equal(0)) - }) - - It("should overwrite an existing value", func() { - existing := 12345 - fc := &v3.FelixConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "default"}, - Spec: v3.FelixConfigurationSpec{BPFKubeProxyHealthzPort: &existing}, - } - disableBPFKubeProxyHealthz(fc) - Expect(fc.Spec.BPFKubeProxyHealthzPort).ShouldNot(BeNil()) - Expect(*fc.Spec.BPFKubeProxyHealthzPort).To(Equal(0)) - }) - }) }) diff --git a/pkg/controller/installation/core_controller.go b/pkg/controller/installation/core_controller.go index ef16cc52cc..29aa977651 100644 --- a/pkg/controller/installation/core_controller.go +++ b/pkg/controller/installation/core_controller.go @@ -1032,29 +1032,15 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile } // Set any non-default FelixConfiguration values that we need. - felixConfiguration, err := sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - // Configure defaults. - u, err := r.setDefaultsOnFelixConfiguration(ctx, instance, fc, reqLogger, needsNamespaceMigration) - if err != nil { - return false, err - } - - // Configure nftables mode. - u2, err := r.setNftablesMode(ctx, instance, fc, reqLogger) - if err != nil { - return false, err - } - - // Configure cluster routing mode. - u3, err := setClusterRoutingOnFelixConfiguration(instance, fc, reqLogger) - if err != nil { - return false, err - } - - updated := u || u2 || u3 - return updated, nil - }) + felixWriter := sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs) + _, err = felixWriter.ApplyFelixConfiguration(ctx, r.declareFelixConfiguration(instance)) if err != nil { + r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error updating FelixConfiguration", err, reqLogger) + return reconcile.Result{}, err + } + felixConfiguration, err := felixWriter.ApplyFelixConfiguration(ctx, r.declareBPFEnabled(ctx, instance, needsNamespaceMigration)) + if err != nil { + r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error updating FelixConfiguration", err, reqLogger) return reconcile.Result{}, err } @@ -1444,10 +1430,8 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile r.status.AddDeployments([]types.NamespacedName{{Name: common.KubeControllersDeploymentName, Namespace: common.CalicoNamespace}}) certificateManager.AddToStatusManager(r.status, common.CalicoNamespace) - // If eBPF is enabled in the operator API, patch FelixConfiguration to enable it within Felix. - _, err = sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - return r.setBPFUpdatesOnFelixConfiguration(ctx, instance, fc, reqLogger) - }) + // Now that calico-node has rolled out, re-check whether eBPF can be enabled within Felix. + _, err = felixWriter.ApplyFelixConfiguration(ctx, r.declareBPFEnabled(ctx, instance, needsNamespaceMigration)) if err != nil { r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error updating resource", err, reqLogger) return reconcile.Result{}, err @@ -1656,196 +1640,6 @@ func getOrCreateTyphaNodeTLSConfig(cli client.Client, certificateManager certifi }, nil } -func (r *ReconcileInstallation) setNftablesMode(_ context.Context, install *operatorv1.Installation, fc *v3.FelixConfiguration, reqLogger logr.Logger) (bool, error) { - updated := false - - // Set the FelixConfiguration nftables dataplane mode based on the operator configuration. We do this unconditonally because - // we don't need to handle upgrades from versions that were previously FelixConfiguration only - nftables mode has always - // been controlled by the operator. - if install.Spec.CalicoNetwork.LinuxDataplane != nil { - nftablesMode := v3.NFTablesModeDisabled - if install.Spec.IsNftables() { - // The operator is configured to use the nftables dataplane. - if install.Spec.BPFEnabled() { - // For BPF mode, we always use nftables, as we don't use the upstream kube-proxy and so don't need to - // worry about compatibility with its mode of operation. - nftablesMode = v3.NFTablesModeEnabled - } else { - // Otherwise, kube-proxy is running - configure Felix to auto-detect whether it should use nftables or iptables on - // a per-node basis, allowing for smoother upgrades. - nftablesMode = v3.NFTablesModeAuto - } - } - updated = fc.Spec.NFTablesMode == nil || *fc.Spec.NFTablesMode != nftablesMode - fc.Spec.NFTablesMode = &nftablesMode - } - if updated { - reqLogger.Info("Patching nftables mode", "nftablesMode", *fc.Spec.NFTablesMode) - } - return updated, nil -} - -// setDefaultOnFelixConfiguration will take the passed in fc and add any defaulting needed -// based on the install config. -func (r *ReconcileInstallation) setDefaultsOnFelixConfiguration(ctx context.Context, install *operatorv1.Installation, fc *v3.FelixConfiguration, reqLogger logr.Logger, needNsMigration bool) (bool, error) { - updated := false - - switch install.Spec.CNI.Type { - // If we're using the AWS CNI plugin we need to ensure the route tables that calico-node - // uses do not conflict with the ones the AWS CNI plugin uses so default them - // in the FelixConfiguration if they are not already set. - case operatorv1.PluginAmazonVPC: - if fc.Spec.RouteTableRange == nil { - updated = true - // Defaulting based on that AWS might be using the following: - // - The ENI device number + 1 - // Currently the max number of ENIs for any host is 15. - // p4d.24xlarge is reported to support 4x15 ENI but it uses 4 cards - // and AWS CNI only uses ENIs on card 0. - // - The VLAN table ID + 100 (there is doubt if this is true) - fc.Spec.RouteTableRange = &v3.RouteTableRange{ - Min: 65, - Max: 99, - } - } - case operatorv1.PluginGKE: - if fc.Spec.RouteTableRange == nil { - updated = true - // Don't conflict with the GKE CNI plugin's routes. - fc.Spec.RouteTableRange = &v3.RouteTableRange{ - Min: 10, - Max: 250, - } - } - } - - // Determine the felix health port to use. Prefer the configuration from FelixConfiguration, - // but default to 9099 (or 9199 on OpenShift). We will also write back whatever we select to FelixConfiguration. - felixHealthPort := 9099 - if install.Spec.KubernetesProvider.IsOpenShift() { - felixHealthPort = 9199 - } - if fc.Spec.HealthPort == nil { - fc.Spec.HealthPort = &felixHealthPort - updated = true - } - vxlanVNI := 4096 - vxlanPort := 4789 - // MKE uses a vxlanVNI:4096 and vxlanPort:4789 for its docker swarm vxlan. - // This results in a conflict with calico's VXLAN and the vxlan.calico interface - // gets deleted. To fix this we change the vxlanVNI to 10000 as recommended by - // MKE docs (https://docs.mirantis.com/mke/3.7/cli-ref/mke-cli-install.html). - if install.Spec.KubernetesProvider == operatorv1.ProviderDockerEE { - vxlanVNI = 10000 - // We are using a flow based VXLAN device for - // ebpf dataplane. This requires changing the default VXLAN port to - // 8472 to avoid conflict with the host's VXLAN interface. - if install.Spec.BPFEnabled() { - vxlanPort = 8472 - } - } - - if fc.Spec.VXLANVNI == nil { - fc.Spec.VXLANVNI = &vxlanVNI - updated = true - } - - if fc.Spec.VXLANPort == nil { - fc.Spec.VXLANPort = &vxlanPort - updated = true - } - - if install.Spec.KubernetesProvider == operatorv1.ProviderDockerEE { - // Set bpfHostConntrackBypass to false for eBPF dataplane to work with MKE - if install.Spec.BPFEnabled() && fc.Spec.BPFHostConntrackBypass == nil { - disableBPFHostConntrackBypass(fc) - updated = true - } - } - - // When BPF is enabled but the operator is not managing kube-proxy (e.g. on AKS, where - // the platform owns the kube-proxy DaemonSet), the platform's kube-proxy keeps the - // default healthz port (10256), and Felix's BPF kube-proxy healthz server would fail - // to bind. Default the port to 0 (disabled) so calico-node starts cleanly. Users can - // still override by setting BPFKubeProxyHealthzPort explicitly on FelixConfiguration. - if install.Spec.BPFEnabled() && !install.Spec.KubeProxyManagementEnabled() && fc.Spec.BPFKubeProxyHealthzPort == nil { - disableBPFKubeProxyHealthz(fc) - updated = true - } - - // Variant-specific FelixConfiguration defaults (e.g. the Enterprise - // provider-specific dnsTrustedServers) are owned by the variant extension. - extUpdated, err := r.ext.DefaultFelixConfiguration(&install.Spec, fc) - if err != nil { - return updated, err - } - updated = updated || extUpdated - - // If BPF is enabled, but not set on FelixConfiguration, do so here. This could happen when an older - // version of operator is replaced by the new one. Older versions of the operator used an - // environment variable to enable BPF, but we no longer do so. In order to prevent disruption - // when the environment variable is removed by the render code of the new operator, make sure - // FelixConfiguration has the correct value set. - - // If calico-node daemonset exists, we need to check the ENV VAR and set FelixConfiguration accordingly. - // Otherwise, this is a fresh install in eBPF mode, set the felix config. - ds := &appsv1.DaemonSet{} - err = r.client.Get(ctx, types.NamespacedName{Namespace: common.CalicoNamespace, Name: common.NodeDaemonSetName}, ds) - if err != nil { - if !apierrors.IsNotFound(err) { - reqLogger.Error(err, "An error occurred when getting the Daemonset resource") - return false, err - } - if !needNsMigration && install.Spec.BPFEnabled() { - err = setBPFEnabledOnFelixConfiguration(fc, true) - if err != nil { - reqLogger.Error(err, "Unable to enable eBPF data plane with a fresh install") - return false, err - } - updated = true - } - } else { - bpfEnabledOnDaemonsetWithEnvVar, err := bpfEnabledOnDaemonsetWithEnvVar(ds) - if err != nil { - reqLogger.Error(err, "An error occurred when querying the Daemonset resource") - return false, err - } else if bpfEnabledOnDaemonsetWithEnvVar && !bpfEnabledOnFelixConfig(fc) { - err = setBPFEnabledOnFelixConfiguration(fc, true) - if err != nil { - reqLogger.Error(err, "Unable to enable eBPF data plane") - return false, err - } else { - updated = true - } - } - } - - return updated, nil -} - -// setClusterRoutingOnFelixConfiguration sets programClusterRoutes in the FelixConfiguration resource -// based on the value of clusterRoutingMode in the install config. -func setClusterRoutingOnFelixConfiguration( - install *operatorv1.Installation, - fc *v3.FelixConfiguration, - reqLogger logr.Logger, -) (bool, error) { - if install.Spec.CalicoNetwork == nil || install.Spec.CalicoNetwork.ClusterRoutingMode == nil { - return false, nil - } - - updated := false - desiredValue := felixProgramClusterRoutesValue(*install.Spec.CalicoNetwork.ClusterRoutingMode) - - if fc.Spec.ProgramClusterRoutes == nil || *fc.Spec.ProgramClusterRoutes != desiredValue { - fc.Spec.ProgramClusterRoutes = &desiredValue - updated = true - reqLogger.Info("Patching FelixConfiguration", "programClusterRoutes", desiredValue) - } - - return updated, nil -} - // setClusterRoutingOnBGPConfiguration sets programClusterRoutes in the BGPConfiguration resource // based on the value of clusterRoutingMode in the install config. func setClusterRoutingOnBGPConfiguration( @@ -1930,42 +1724,6 @@ func clusterRoutingMode(install *operatorv1.Installation) operatorv1.ClusterRout return *install.Spec.CalicoNetwork.ClusterRoutingMode } -// setBPFUpdatesOnFelixConfiguration will take the passed in fc and update any BPF properties needed -// based on the install config and the daemonset. -func (r *ReconcileInstallation) setBPFUpdatesOnFelixConfiguration(ctx context.Context, install *operatorv1.Installation, fc *v3.FelixConfiguration, reqLogger logr.Logger) (bool, error) { - updated := false - - bpfEnabledOnInstall := install.Spec.BPFEnabled() - if bpfEnabledOnInstall { - ds := &appsv1.DaemonSet{} - err := r.client.Get(ctx, types.NamespacedName{Namespace: common.CalicoNamespace, Name: common.NodeDaemonSetName}, ds) - if err != nil { - return false, err - } - if !bpfEnabledOnFelixConfig(fc) && isRolloutCompleteWithBPFVolumes(ds) { - err := setBPFEnabledOnFelixConfiguration(fc, bpfEnabledOnInstall) - if err != nil { - reqLogger.Error(err, "Unable to enable eBPF data plane") - return false, err - } else { - updated = true - } - } - } else { - if fc.Spec.BPFEnabled == nil || *fc.Spec.BPFEnabled { - err := setBPFEnabledOnFelixConfiguration(fc, bpfEnabledOnInstall) - if err != nil { - reqLogger.Error(err, "Unable to disable eBPF data plane") - return false, err - } else { - updated = true - } - } - } - - return updated, nil -} - // serviceIPsAndPorts extracts the service IPs and ports from the Service and returns them as a slice of k8sapi.ServiceEndpoint. func serviceIPsAndPorts(svc *corev1.Service) []k8sapi.ServiceEndpoint { if svc == nil { diff --git a/pkg/controller/installation/felixconfig.go b/pkg/controller/installation/felixconfig.go new file mode 100644 index 0000000000..a516820e2f --- /dev/null +++ b/pkg/controller/installation/felixconfig.go @@ -0,0 +1,179 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 installation + +import ( + "context" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + appsv1 "k8s.io/api/apps/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + + operatorv1 "github.com/tigera/operator/api/v1" + "github.com/tigera/operator/pkg/common" + "github.com/tigera/operator/pkg/controller/sharedconfig" +) + +const ( + // felixConfigFieldManager owns the FelixConfiguration fields defaulted from the Installation. + felixConfigFieldManager = "installation" + + // bpfFieldManager owns spec.bpfEnabled, which both installation write sites declare. + bpfFieldManager = "installation-bpf" +) + +// declareFelixConfiguration declares the fields defaulted from the Installation spec, always +// declaring every one so the field set stays stable. +func (r *ReconcileInstallation) declareFelixConfiguration(install *operatorv1.Installation) sharedconfig.DeclareFelixConfiguration { + return func(current *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + d := &sharedconfig.FelixConfigurationDeclaration{ + Manager: felixConfigFieldManager, + Owned: &v3.FelixConfiguration{}, + Policies: map[string]sharedconfig.ConflictPolicy{}, + } + owned := &d.Owned.Spec + + // Keep calico-node's route tables clear of the ones the CNI plugin uses. + switch install.Spec.CNI.Type { + case operatorv1.PluginAmazonVPC: + // AWS uses the ENI device number + 1, and the VLAN table ID + 100. + owned.RouteTableRange = &v3.RouteTableRange{Min: 65, Max: 99} + d.Policies["spec.routeTableRange"] = sharedconfig.ConflictDefer + case operatorv1.PluginGKE: + owned.RouteTableRange = &v3.RouteTableRange{Min: 10, Max: 250} + d.Policies["spec.routeTableRange"] = sharedconfig.ConflictDefer + } + + healthPort := 9099 + if install.Spec.KubernetesProvider.IsOpenShift() { + healthPort = 9199 + } + owned.HealthPort = &healthPort + d.Policies["spec.healthPort"] = sharedconfig.ConflictDefer + + vxlanVNI, vxlanPort := 4096, 4789 + if install.Spec.KubernetesProvider == operatorv1.ProviderDockerEE { + // MKE's docker swarm VXLAN uses 4096/4789, and the clash deletes vxlan.calico. + // MKE's docs recommend 10000. + vxlanVNI = 10000 + if install.Spec.BPFEnabled() { + // The eBPF dataplane's flow-based VXLAN device clashes with the host's VXLAN interface. + vxlanPort = 8472 + + // The eBPF dataplane only works with MKE when conntrack bypass is off. + owned.BPFHostConntrackBypass = ptr.To(false) + d.Policies["spec.bpfHostConntrackBypass"] = sharedconfig.ConflictDefer + } + } + owned.VXLANVNI = &vxlanVNI + owned.VXLANPort = &vxlanPort + d.Policies["spec.vxlanVNI"] = sharedconfig.ConflictDefer + d.Policies["spec.vxlanPort"] = sharedconfig.ConflictDefer + + if install.Spec.BPFEnabled() && !install.Spec.KubeProxyManagementEnabled() { + // The platform's kube-proxy holds 10256, so Felix's healthz server would fail to bind. + owned.BPFKubeProxyHealthzPort = ptr.To(0) + d.Policies["spec.bpfKubeProxyHealthzPort"] = sharedconfig.ConflictDefer + } + + if install.Spec.CalicoNetwork != nil && install.Spec.CalicoNetwork.LinuxDataplane != nil { + owned.NFTablesMode = ptr.To(nftablesMode(install)) + d.Policies["spec.nftablesMode"] = sharedconfig.ConflictOverride + } + + // Gated on the field being set, so leaving it unset keeps meaning "whatever Calico + // defaults to" rather than pinning today's default into the datastore. + if install.Spec.CalicoNetwork != nil && install.Spec.CalicoNetwork.ClusterRoutingMode != nil { + mode := *install.Spec.CalicoNetwork.ClusterRoutingMode + owned.ProgramClusterRoutes = ptr.To(felixProgramClusterRoutesValue(mode)) + d.Policies["spec.programClusterRoutes"] = sharedconfig.ConflictOverride + } + + extPaths, err := r.ext.DeclareFelixConfiguration(&install.Spec, current, d.Owned) + if err != nil { + return nil, err + } + for _, path := range extPaths { + d.Policies[path] = sharedconfig.ConflictOverride + } + + return d, nil + } +} + +// nftablesMode is the dataplane mode Felix should run in. The operator has always owned it, +// so nothing older needs preserving. +func nftablesMode(install *operatorv1.Installation) v3.NFTablesMode { + if !install.Spec.IsNftables() { + return v3.NFTablesModeDisabled + } + if install.Spec.BPFEnabled() { + // BPF mode replaces kube-proxy, so nftables needs no compatibility with its mode. + return v3.NFTablesModeEnabled + } + // kube-proxy is running, so let Felix pick per node and keep upgrades smooth. + return v3.NFTablesModeAuto +} + +// declareBPFEnabled declares spec.bpfEnabled. Both installation write sites use it so the field +// stays under one manager with the same value. +func (r *ReconcileInstallation) declareBPFEnabled(ctx context.Context, install *operatorv1.Installation, needNsMigration bool) sharedconfig.DeclareFelixConfiguration { + return func(current *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + enabled, err := r.bpfEnabledValue(ctx, install, current, needNsMigration) + if err != nil { + return nil, err + } + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: bpfFieldManager, + Owned: &v3.FelixConfiguration{ + Spec: v3.FelixConfigurationSpec{BPFEnabled: &enabled}, + }, + Policies: map[string]sharedconfig.ConflictPolicy{ + // A user who changed this by hand gets a degraded status, not an override. + "spec.bpfEnabled": sharedconfig.ConflictError, + }, + }, nil + } +} + +// bpfEnabledValue resolves the dataplane Felix should run. Turning eBPF on waits for the +// calico-node rollout to mount the BPF volumes. +func (r *ReconcileInstallation) bpfEnabledValue(ctx context.Context, install *operatorv1.Installation, current *v3.FelixConfiguration, needNsMigration bool) (bool, error) { + if !install.Spec.BPFEnabled() { + return false, nil + } + + ds := &appsv1.DaemonSet{} + err := r.client.Get(ctx, types.NamespacedName{Namespace: common.CalicoNamespace, Name: common.NodeDaemonSetName}, ds) + if apierrors.IsNotFound(err) { + // A fresh install in eBPF mode has no calico-node rollout to wait for. + return !needNsMigration, nil + } + if err != nil { + return false, err + } + + // Operators before the FelixConfiguration field enabled eBPF through a calico-node env var. + envVarEnabled, err := bpfEnabledOnDaemonsetWithEnvVar(ds) + if err != nil { + return false, err + } + if envVarEnabled || isRolloutCompleteWithBPFVolumes(ds) { + return true, nil + } + return bpfEnabledOnFelixConfig(current), nil +} diff --git a/pkg/controller/installation/felixconfig_test.go b/pkg/controller/installation/felixconfig_test.go new file mode 100644 index 0000000000..2577a89e18 --- /dev/null +++ b/pkg/controller/installation/felixconfig_test.go @@ -0,0 +1,101 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 installation + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + "k8s.io/utils/ptr" + + operatorv1 "github.com/tigera/operator/api/v1" + "github.com/tigera/operator/pkg/controller/sharedconfig" +) + +var _ = Describe("FelixConfiguration declarations", func() { + var r ReconcileInstallation + + nftables := operatorv1.LinuxDataplaneNftables + + BeforeEach(func() { + r = ReconcileInstallation{ext: testExtensions.Installation()} + }) + + install := func() *operatorv1.Installation { + return &operatorv1.Installation{Spec: operatorv1.InstallationSpec{ + CNI: &operatorv1.CNISpec{Type: operatorv1.PluginCalico}, + CalicoNetwork: &operatorv1.CalicoNetworkSpec{LinuxDataplane: &nftables}, + }} + } + + declaredPaths := func(i *operatorv1.Installation, current *v3.FelixConfiguration) []string { + d, err := r.declareFelixConfiguration(i)(current) + Expect(err).NotTo(HaveOccurred()) + paths := []string{} + for path := range d.Policies { + paths = append(paths, path) + } + return paths + } + + It("declares the same fields no matter what the current object holds", func() { + empty := declaredPaths(install(), &v3.FelixConfiguration{}) + Expect(empty).To(ConsistOf( + "spec.healthPort", + "spec.vxlanVNI", + "spec.vxlanPort", + "spec.nftablesMode", + )) + + // Every field the operator defaults is already set, by the operator or by anyone else. + populated := declaredPaths(install(), &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{ + HealthPort: ptr.To(1234), + VXLANVNI: ptr.To(9999), + VXLANPort: ptr.To(1111), + NFTablesMode: ptr.To(v3.NFTablesModeDisabled), + }}) + Expect(populated).To(ConsistOf(empty)) + }) + + It("declares the values it wants, not the values already there", func() { + current := &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{HealthPort: ptr.To(1234)}} + d, err := r.declareFelixConfiguration(install())(current) + Expect(err).NotTo(HaveOccurred()) + Expect(d.Owned.Spec.HealthPort).To(Equal(ptr.To(9099))) + Expect(d.Policies["spec.healthPort"]).To(Equal(sharedconfig.ConflictDefer)) + }) + + It("defers to a user on defaults and overrides them on modes it owns outright", func() { + i := install() + i.Spec.CalicoNetwork.ClusterRoutingMode = ptr.To(operatorv1.ClusterRoutingModeFelix) + d, err := r.declareFelixConfiguration(i)(&v3.FelixConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(d.Manager).To(Equal(felixConfigFieldManager)) + Expect(d.Policies["spec.programClusterRoutes"]).To(Equal(sharedconfig.ConflictOverride)) + Expect(d.Owned.Spec.ProgramClusterRoutes).To(Equal(ptr.To("Enabled"))) + }) + + It("declares bpfEnabled under its own manager, refusing to fight over it", func() { + d, err := r.declareBPFEnabled(context.Background(), install(), false)(&v3.FelixConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(d.Manager).To(Equal(bpfFieldManager)) + Expect(d.Policies).To(HaveLen(1)) + Expect(d.Policies["spec.bpfEnabled"]).To(Equal(sharedconfig.ConflictError)) + Expect(d.Owned.Spec.BPFEnabled).To(Equal(ptr.To(false))) + }) +}) diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go index 89460b3a36..a59052d1d1 100644 --- a/pkg/controller/sharedconfig/apply_test.go +++ b/pkg/controller/sharedconfig/apply_test.go @@ -261,7 +261,7 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(true))) }) - It("should report a conflict when the legacy annotation disagrees with the field", func() { + It("should take over a value someone else set, when it wanted that value anyway", func() { Expect(c.Create(ctx, &v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -270,8 +270,22 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Spec: v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}, })).NotTo(HaveOccurred()) + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).NotTo(HaveOccurred()) + fc := getFelixConfig() + Expect(fc.Spec.BPFEnabled).To(Equal(ptr.To(true))) + Expect(fc.Annotations).To(HaveKeyWithValue(render.BPFOperatorAnnotation, "true")) + }) + + It("should refuse to change a value someone else set", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: v3.FelixConfigurationSpec{BPFEnabled: ptr.To(false)}, + })).NotTo(HaveOccurred()) + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) Expect(err).To(MatchError(ContainSubstring("spec.bpfEnabled"))) + Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(false))) }) It("should keep the legacy annotation in step with what it writes", func() { diff --git a/pkg/controller/sharedconfig/crdv1.go b/pkg/controller/sharedconfig/crdv1.go index 233b6c2858..82c0754ea0 100644 --- a/pkg/controller/sharedconfig/crdv1.go +++ b/pkg/controller/sharedconfig/crdv1.go @@ -101,6 +101,15 @@ func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurati if !pathSet(payload.Object, path) { continue } + // Writing the value that is already there needs no arbitration, whoever put it there. + agree, err := valuesAgree(currentContent, payload.Object, path) + if err != nil { + return nil, err + } + if agree { + continue + } + changed, err := changedByOther(currentContent, lastWritten, path) if err != nil { return nil, err diff --git a/pkg/controller/sharedconfig/tracking.go b/pkg/controller/sharedconfig/tracking.go index b1fb1ca39e..65b776f8d6 100644 --- a/pkg/controller/sharedconfig/tracking.go +++ b/pkg/controller/sharedconfig/tracking.go @@ -71,6 +71,20 @@ func changedByOther(currentContent map[string]any, lastWritten map[string]any, p return !reflect.DeepEqual(canonical, written), nil } +// valuesAgree reports whether the value about to be written is already there. +func valuesAgree(currentContent, payloadObj map[string]any, path string) (bool, error) { + keys := strings.Split(path, ".") + current, found, err := unstructured.NestedFieldNoCopy(currentContent, keys...) + if err != nil || !found { + return false, err + } + written, found, err := unstructured.NestedFieldNoCopy(payloadObj, keys...) + if err != nil || !found { + return false, err + } + return reflect.DeepEqual(current, written), nil +} + // recordWrittenValues stores the values being written so the next reconcile can compare against them. func recordWrittenValues(fc *v3.FelixConfiguration, payload *unstructured.Unstructured, d *FelixConfigurationDeclaration, deferred []string) error { values, err := lastWrittenValues(fc) diff --git a/pkg/enterprise/installation/core.go b/pkg/enterprise/installation/core.go index fd6236b5d6..98703c95af 100644 --- a/pkg/enterprise/installation/core.go +++ b/pkg/enterprise/installation/core.go @@ -16,7 +16,6 @@ package installation import ( "context" - "strings" v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" rbacv1 "k8s.io/api/rbac/v1" @@ -99,11 +98,9 @@ func (e *Extension) ProductVersion() string { return components.EnterpriseRelease } -// DefaultFelixConfiguration sets the Enterprise-only FelixConfiguration defaults. -// Some platforms run a DNS service that isn't named "kube-dns", so dnsTrustedServers -// needs a provider-specific default for Enterprise DNS logging to work. Returns -// whether it changed fc. -func (e *Extension) DefaultFelixConfiguration(install *operatorv1.InstallationSpec, fc *v3.FelixConfiguration) (bool, error) { +// DeclareFelixConfiguration defaults dnsTrustedServers per provider, since some platforms +// name their DNS service something other than "kube-dns". +func (e *Extension) DeclareFelixConfiguration(install *operatorv1.InstallationSpec, current, owned *v3.FelixConfiguration) ([]string, error) { dnsService := "" switch install.KubernetesProvider { case operatorv1.ProviderOpenShift: @@ -112,27 +109,22 @@ func (e *Extension) DefaultFelixConfiguration(install *operatorv1.InstallationSp dnsService = "k8s-service:kube-system/rke2-coredns-rke2-coredns" } if dnsService == "" { - return false, nil + return nil, nil } felixDefault := "k8s-service:kube-dns" trustedServers := []string{dnsService} // Keep any other values that are already configured, excepting the value we are // setting and the kube-dns default. - existingSetting := "" - if fc.Spec.DNSTrustedServers != nil { - existingSetting = strings.Join(*fc.Spec.DNSTrustedServers, ",") - for _, server := range *fc.Spec.DNSTrustedServers { + if current.Spec.DNSTrustedServers != nil { + for _, server := range *current.Spec.DNSTrustedServers { if server != felixDefault && server != dnsService { trustedServers = append(trustedServers, server) } } } - if strings.Join(trustedServers, ",") == existingSetting { - return false, nil - } - fc.Spec.DNSTrustedServers = &trustedServers - return true, nil + owned.Spec.DNSTrustedServers = &trustedServers + return []string{"spec.dnsTrustedServers"}, nil } // Watches registers the enterprise resources the installation controller diff --git a/pkg/enterprise/installation/core_test.go b/pkg/enterprise/installation/core_test.go index 256591407b..f0f000df5f 100644 --- a/pkg/enterprise/installation/core_test.go +++ b/pkg/enterprise/installation/core_test.go @@ -57,31 +57,43 @@ var _ = Describe("installation controller extension", func() { Expect(reason).To(Equal(operatorv1.InvalidConfigurationError)) }) - DescribeTable("defaults dnsTrustedServers for providers whose DNS service isn't kube-dns", + DescribeTable("declares dnsTrustedServers for providers whose DNS service isn't kube-dns", func(provider operatorv1.Provider, expected []string) { - fc := &v3.FelixConfiguration{} + owned := &v3.FelixConfiguration{} install := &operatorv1.InstallationSpec{Variant: operatorv1.CalicoEnterprise, KubernetesProvider: provider} - updated, err := ext.Installation().DefaultFelixConfiguration(install, fc) + paths, err := ext.Installation().DeclareFelixConfiguration(install, &v3.FelixConfiguration{}, owned) Expect(err).NotTo(HaveOccurred()) if expected == nil { - Expect(updated).To(BeFalse()) - Expect(fc.Spec.DNSTrustedServers).To(BeNil()) + Expect(paths).To(BeEmpty()) + Expect(owned.Spec.DNSTrustedServers).To(BeNil()) return } - Expect(updated).To(BeTrue()) - Expect(*fc.Spec.DNSTrustedServers).To(ConsistOf(expected)) + Expect(paths).To(ConsistOf("spec.dnsTrustedServers")) + Expect(*owned.Spec.DNSTrustedServers).To(ConsistOf(expected)) }, Entry("OpenShift", operatorv1.ProviderOpenShift, []string{"k8s-service:openshift-dns/dns-default"}), Entry("RKE2", operatorv1.ProviderRKE2, []string{"k8s-service:kube-system/rke2-coredns-rke2-coredns"}), Entry("other providers keep the felix default", operatorv1.ProviderNone, nil), ) - It("does no felix defaulting when the operator runs as Calico", func() { - fc := &v3.FelixConfiguration{} - updated, err := calicoExt.Installation().DefaultFelixConfiguration(&operatorv1.InstallationSpec{Variant: operatorv1.Calico, KubernetesProvider: operatorv1.ProviderOpenShift}, fc) + It("keeps trusted servers a user configured, dropping the felix default", func() { + current := &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{ + DNSTrustedServers: &[]string{"k8s-service:kube-dns", "k8s-service:other/dns"}, + }} + owned := &v3.FelixConfiguration{} + install := &operatorv1.InstallationSpec{Variant: operatorv1.CalicoEnterprise, KubernetesProvider: operatorv1.ProviderOpenShift} + _, err := ext.Installation().DeclareFelixConfiguration(install, current, owned) Expect(err).NotTo(HaveOccurred()) - Expect(updated).To(BeFalse()) - Expect(fc.Spec.DNSTrustedServers).To(BeNil()) + Expect(*owned.Spec.DNSTrustedServers).To(ConsistOf("k8s-service:openshift-dns/dns-default", "k8s-service:other/dns")) + }) + + It("declares nothing when the operator runs as Calico", func() { + owned := &v3.FelixConfiguration{} + install := &operatorv1.InstallationSpec{Variant: operatorv1.Calico, KubernetesProvider: operatorv1.ProviderOpenShift} + paths, err := calicoExt.Installation().DeclareFelixConfiguration(install, &v3.FelixConfiguration{}, owned) + Expect(err).NotTo(HaveOccurred()) + Expect(paths).To(BeEmpty()) + Expect(owned.Spec.DNSTrustedServers).To(BeNil()) }) It("manages the node prometheus and kube-controllers metrics keypairs for the enterprise variant", func() { diff --git a/pkg/extensions/installation.go b/pkg/extensions/installation.go index 91436a6d15..b089f36a56 100644 --- a/pkg/extensions/installation.go +++ b/pkg/extensions/installation.go @@ -38,9 +38,9 @@ type InstallationExtension interface { // Watches registers the watches the extension needs. Watches(c ctrlruntime.Controller) error - // DefaultFelixConfiguration defaults FelixConfiguration fields, reporting whether - // it changed fc. It runs before Felix defaulting persists. - DefaultFelixConfiguration(install *operatorv1.InstallationSpec, fc *v3.FelixConfiguration) (bool, error) + // DeclareFelixConfiguration writes the variant's defaults into owned, merging with current + // where needed, and returns the paths it declared. + DeclareFelixConfiguration(install *operatorv1.InstallationSpec, current, owned *v3.FelixConfiguration) ([]string, error) // ProductVersion is the version the operator writes to the Installation status. ProductVersion() string @@ -63,8 +63,8 @@ func (noopInstallation) Watches(ctrlruntime.Controller) error { return nil } -func (noopInstallation) DefaultFelixConfiguration(*operatorv1.InstallationSpec, *v3.FelixConfiguration) (bool, error) { - return false, nil +func (noopInstallation) DeclareFelixConfiguration(*operatorv1.InstallationSpec, *v3.FelixConfiguration, *v3.FelixConfiguration) ([]string, error) { + return nil, nil } func (noopInstallation) ProductVersion() string { From 434d0a58ee897d8d23128ca4704625b6dc5da041 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Fri, 14 Aug 2026 17:31:03 -0400 Subject: [PATCH 04/10] Take FelixConfiguration fields over from the operator's pre-apply writes A field owned through a plain update blocks an apply, so the operator forces ownership across once for the values its own record accounts for. --- pkg/controller/sharedconfig/apply_test.go | 73 ++++++++++++++++++ pkg/controller/sharedconfig/migrate.go | 91 +++++++++++++++++++++++ pkg/controller/sharedconfig/v3.go | 14 +++- 3 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 pkg/controller/sharedconfig/migrate.go diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go index a59052d1d1..3d1b84fe71 100644 --- a/pkg/controller/sharedconfig/apply_test.go +++ b/pkg/controller/sharedconfig/apply_test.go @@ -150,6 +150,79 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Expect(err).NotTo(HaveOccurred()) Expect(getFelixConfig().Spec.VXLANPort).To(BeNil()) }) + + Context("a cluster the operator wrote before it applied", func() { + declareBPF := func(policy sharedconfig.ConflictPolicy) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: "installation-bpf", + Owned: &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{BPFEnabled: ptr.To(false)}}, + Policies: map[string]sharedconfig.ConflictPolicy{"spec.bpfEnabled": policy}, + }, nil + } + } + + // createByUpdate writes the way the operator's merge patch used to, recorded against a + // manager that never applied. + createByUpdate := func(annotations map[string]string, spec v3.FelixConfigurationSpec) { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default", Annotations: annotations}, + Spec: spec, + })).NotTo(HaveOccurred()) + } + + It("should take over a field it recorded as its own", func() { + createByUpdate(map[string]string{render.BPFOperatorAnnotation: "true"}, + v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}) + + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + Expect(fc.Spec.BPFEnabled).To(Equal(ptr.To(false))) + Expect(fc.ManagedFields).To(ContainElement(SatisfyAll( + HaveField("Manager", "tigera-operator/installation-bpf"), + HaveField("Operation", metav1.ManagedFieldsOperationApply), + ))) + }) + + It("should refuse a field it has no record of writing", func() { + createByUpdate(nil, v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}) + + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) + Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(true))) + }) + + It("should stop trusting its old record once ownership has moved", func() { + createByUpdate(map[string]string{render.BPFOperatorAnnotation: "true"}, + v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}) + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).NotTo(HaveOccurred()) + + // The stale annotation still reads "true", which is what a user now applies. + other := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "projectcalico.org/v3", + "kind": "FelixConfiguration", + "metadata": map[string]any{"name": "default"}, + "spec": map[string]any{"bpfEnabled": true}, + }} + Expect(c.Patch(ctx, other, client.Apply, client.FieldOwner("kubectl"), client.ForceOwnership)).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) + Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(true))) + }) + + It("should defer on a field it never recorded, leaving the value alone", func() { + createByUpdate(nil, v3.FelixConfigurationSpec{HealthPort: ptr.To(9100)}) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9100))) + Expect(fc.Spec.VXLANPort).To(Equal(ptr.To(4789))) + }) + }) }) Context("crd.projectcalico.org/v1, where the operator tracks what it wrote", func() { diff --git a/pkg/controller/sharedconfig/migrate.go b/pkg/controller/sharedconfig/migrate.go new file mode 100644 index 0000000000..75be4707dc --- /dev/null +++ b/pkg/controller/sharedconfig/migrate.go @@ -0,0 +1,91 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "encoding/json" + "fmt" + "strings" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// reclaimablePaths lists fields a plain update owns that hold the operator's own value. +// An apply must force ownership across once. +func reclaimablePaths(fc *v3.FelixConfiguration) (map[string]bool, error) { + updated, err := updateOwnedPaths(fc) + if err != nil || len(updated) == 0 { + return nil, err + } + lastWritten, err := lastWrittenValues(fc) + if err != nil || len(lastWritten) == 0 { + return nil, err + } + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(fc) + if err != nil { + return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + + reclaimable := map[string]bool{} + for path := range lastWritten { + if !updated[path] { + continue + } + changed, err := changedByOther(content, lastWritten, path) + if err != nil { + return nil, err + } + if !changed { + reclaimable[path] = true + } + } + return reclaimable, nil +} + +// updateOwnedPaths lists the fields owned through a plain update rather than an apply. +func updateOwnedPaths(fc *v3.FelixConfiguration) (map[string]bool, error) { + owned := map[string]bool{} + for _, entry := range fc.ManagedFields { + if entry.Operation != metav1.ManagedFieldsOperationUpdate || entry.FieldsV1 == nil { + continue + } + fields := map[string]any{} + if err := json.Unmarshal(entry.FieldsV1.Raw, &fields); err != nil { + return nil, fmt.Errorf("unable to parse the fields managed by %q: %w", entry.Manager, err) + } + collectFieldPaths(fields, "", owned) + } + return owned, nil +} + +// collectFieldPaths flattens a managed field set into paths of the "spec.field" form. +func collectFieldPaths(fields map[string]any, prefix string, out map[string]bool) { + for key, value := range fields { + name, found := strings.CutPrefix(key, "f:") + if !found { + continue + } + path := name + if prefix != "" { + path = prefix + "." + name + } + out[path] = true + if children, ok := value.(map[string]any); ok { + collectFieldPaths(children, path, out) + } + } +} diff --git a/pkg/controller/sharedconfig/v3.go b/pkg/controller/sharedconfig/v3.go index e7534e25a1..de58ba03a8 100644 --- a/pkg/controller/sharedconfig/v3.go +++ b/pkg/controller/sharedconfig/v3.go @@ -65,7 +65,7 @@ func (w *v3Writer) ApplyFelixConfiguration(ctx context.Context, declare DeclareF return nil, err } - force, err := w.resolveConflicts(err, declaration, payload) + force, err := w.resolveConflicts(err, current, declaration, payload) if err != nil { return nil, err } @@ -73,12 +73,17 @@ func (w *v3Writer) ApplyFelixConfiguration(ctx context.Context, declare DeclareF } // resolveConflicts drops deferred fields from payload and reports whether the retry must force. -func (w *v3Writer) resolveConflicts(applyErr error, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) (bool, error) { +func (w *v3Writer) resolveConflicts(applyErr error, current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) (bool, error) { paths := conflictPaths(applyErr) if len(paths) == 0 { return false, applyErr } + reclaimable, err := reclaimablePaths(current) + if err != nil { + return false, err + } + force := false var undeclared, refused []string for _, path := range paths { @@ -87,6 +92,11 @@ func (w *v3Writer) resolveConflicts(applyErr error, d *FelixConfigurationDeclara undeclared = append(undeclared, path) continue } + if reclaimable[declared] || reclaimable[path] { + // The operator wrote this before it applied, so take the field rather than arbitrate. + force = true + continue + } switch policy { case ConflictDefer: removePath(payload.Object, declared) From 500db9cf8978df68533c41b5c4a4722e9981d6a9 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Mon, 17 Aug 2026 09:49:58 -0400 Subject: [PATCH 05/10] Recognize the operator's legacy field manager when reclaiming FelixConfiguration fields A plain update keeps its claim on a field even after an apply writes the same value, so an upgraded cluster needs the operator to force ownership across before it can ever change the field again. The recorded values only cover spec.bpfEnabled, so match the legacy manager name as well. --- pkg/controller/sharedconfig/apply_test.go | 27 ++++++++++++++--- pkg/controller/sharedconfig/migrate.go | 37 ++++++++++++++--------- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go index 3d1b84fe71..af29c46b1c 100644 --- a/pkg/controller/sharedconfig/apply_test.go +++ b/pkg/controller/sharedconfig/apply_test.go @@ -162,13 +162,17 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { } } - // createByUpdate writes the way the operator's merge patch used to, recorded against a - // manager that never applied. - createByUpdate := func(annotations map[string]string, spec v3.FelixConfigurationSpec) { + // createAsManager writes the way the operator's merge patch used to, against a manager + // that never applied. + createAsManager := func(manager string, annotations map[string]string, spec v3.FelixConfigurationSpec) { Expect(c.Create(ctx, &v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{Name: "default", Annotations: annotations}, Spec: spec, - })).NotTo(HaveOccurred()) + }, client.FieldOwner(manager))).NotTo(HaveOccurred()) + } + + createByUpdate := func(annotations map[string]string, spec v3.FelixConfigurationSpec) { + createAsManager("someone-else", annotations, spec) } It("should take over a field it recorded as its own", func() { @@ -214,6 +218,21 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(true))) }) + It("should take over a field its own legacy manager still owns", func() { + createAsManager("operator", nil, v3.FelixConfigurationSpec{HealthPort: ptr.To(9098)}) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + + // Taking the field over moves it out of the legacy manager's field set. + for _, entry := range getFelixConfig().ManagedFields { + if entry.Manager == "operator" { + Expect(string(entry.FieldsV1.Raw)).NotTo(ContainSubstring("healthPort")) + } + } + }) + It("should defer on a field it never recorded, leaving the value alone", func() { createByUpdate(nil, v3.FelixConfigurationSpec{HealthPort: ptr.To(9100)}) diff --git a/pkg/controller/sharedconfig/migrate.go b/pkg/controller/sharedconfig/migrate.go index 75be4707dc..13a661e76d 100644 --- a/pkg/controller/sharedconfig/migrate.go +++ b/pkg/controller/sharedconfig/migrate.go @@ -24,25 +24,29 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) -// reclaimablePaths lists fields a plain update owns that hold the operator's own value. +// legacyFieldManager is what the API server derives from the /usr/bin/operator user agent, +// so it records the operator's pre-apply writes. +const legacyFieldManager = "operator" + +// reclaimablePaths lists fields a plain update owns that the operator wrote itself. // An apply must force ownership across once. func reclaimablePaths(fc *v3.FelixConfiguration) (map[string]bool, error) { - updated, err := updateOwnedPaths(fc) - if err != nil || len(updated) == 0 { - return nil, err + reclaimable, others, err := updateOwnedPaths(fc) + if err != nil || len(others) == 0 { + return reclaimable, err } + + // Ownership moves on a plain update too, so fall back to the values the operator recorded. lastWritten, err := lastWrittenValues(fc) if err != nil || len(lastWritten) == 0 { - return nil, err + return reclaimable, err } content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(fc) if err != nil { return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) } - - reclaimable := map[string]bool{} for path := range lastWritten { - if !updated[path] { + if !others[path] { continue } changed, err := changedByOther(content, lastWritten, path) @@ -56,20 +60,25 @@ func reclaimablePaths(fc *v3.FelixConfiguration) (map[string]bool, error) { return reclaimable, nil } -// updateOwnedPaths lists the fields owned through a plain update rather than an apply. -func updateOwnedPaths(fc *v3.FelixConfiguration) (map[string]bool, error) { - owned := map[string]bool{} +// updateOwnedPaths splits the fields owned through a plain update by whether the operator's own +// legacy field manager holds them. +func updateOwnedPaths(fc *v3.FelixConfiguration) (legacy, others map[string]bool, err error) { + legacy, others = map[string]bool{}, map[string]bool{} for _, entry := range fc.ManagedFields { if entry.Operation != metav1.ManagedFieldsOperationUpdate || entry.FieldsV1 == nil { continue } fields := map[string]any{} if err := json.Unmarshal(entry.FieldsV1.Raw, &fields); err != nil { - return nil, fmt.Errorf("unable to parse the fields managed by %q: %w", entry.Manager, err) + return nil, nil, fmt.Errorf("unable to parse the fields managed by %q: %w", entry.Manager, err) + } + out := others + if entry.Manager == legacyFieldManager { + out = legacy } - collectFieldPaths(fields, "", owned) + collectFieldPaths(fields, "", out) } - return owned, nil + return legacy, others, nil } // collectFieldPaths flattens a managed field set into paths of the "spec.field" form. From 3a1296bfef434d06b4f32451d49f073f30f9a8b5 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Mon, 17 Aug 2026 10:19:40 -0400 Subject: [PATCH 06/10] Apply FelixConfiguration from the istio, application layer, gateway, and egress gateway controllers policySyncPathPrefix moves under one field manager computed from all four CRs, and the crd.projectcalico.org/v1 writer learns to delete fields a declaration drops. --- .../applicationlayer_controller.go | 102 ++++------- .../applicationlayer_controller_test.go | 10 +- .../egressgateway/egressgateway_controller.go | 11 +- .../gatewayapi/gatewayapi_controller.go | 36 +--- .../gatewayapi/gatewayapi_controller_test.go | 4 +- pkg/controller/istio/istio_controller.go | 171 ++++-------------- pkg/controller/istio/istio_controller_test.go | 71 +------- pkg/controller/sharedconfig/crdv1.go | 66 ++++++- pkg/controller/sharedconfig/crdv1_test.go | 48 +++++ pkg/controller/sharedconfig/policysync.go | 105 +++++++++++ .../sharedconfig/policysync_test.go | 120 ++++++++++++ pkg/controller/utils/policy_sync.go | 46 ++--- pkg/controller/utils/policy_sync_test.go | 23 --- pkg/controller/utils/utils.go | 30 +++ 14 files changed, 477 insertions(+), 366 deletions(-) create mode 100644 pkg/controller/sharedconfig/policysync.go create mode 100644 pkg/controller/sharedconfig/policysync_test.go diff --git a/pkg/controller/applicationlayer/applicationlayer_controller.go b/pkg/controller/applicationlayer/applicationlayer_controller.go index 41735a1ade..3b43c5ebd2 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller.go @@ -489,11 +489,6 @@ func (r *ReconcileApplicationLayer) isSidecarInjectionEnabled(applicationLayerSp *applicationLayerSpec.SidecarInjection == operatorv1.SidecarEnabled } -func (r *ReconcileApplicationLayer) getPolicySyncPathPrefix(fcSpec *v3.FelixConfigurationSpec, al *operatorv1.ApplicationLayer, istioNeeds bool) string { - alNeeds := utils.ApplicationLayerRequiresPolicySync(al) - return utils.DesiredPolicySyncPathPrefix(fcSpec.PolicySyncPathPrefix, alNeeds, istioNeeds) -} - func (r *ReconcileApplicationLayer) getTProxyMode(al *operatorv1.ApplicationLayer) (bool, string) { if al == nil { return false, "Disabled" @@ -510,75 +505,56 @@ func (r *ReconcileApplicationLayer) getTProxyMode(al *operatorv1.ApplicationLaye return true, "Disabled" } -// patchFelixConfiguration takes all application layer specs as arguments and patches felix config. -// If at least one of the specs requires TPROXYMode as "Enabled" it'll be patched as "Enabled" otherwise it is "Disabled". -// gatewayWAFEnabled reflects the GatewayAPI WAF data-plane extension (design-25): its audit events flow through -// Felix's WAF event log, so it shares the WAFEventLogsFileEnabled toggle with the ApplicationLayer WAF. +// applicationLayerFieldManager owns the FelixConfiguration fields the application layer sets. +const applicationLayerFieldManager = "application-layer" + +// declareWAFEventLogsFile declares the WAF event log toggle, driven by the ApplicationLayer WAF +// and the gateway data plane. +func declareWAFEventLogsFile(al *operatorv1.ApplicationLayer, gatewayWAFEnabled bool) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + enabled := wafEventLogsFileRequired(al, gatewayWAFEnabled) + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: applicationLayerFieldManager, + Owned: &v3.FelixConfiguration{ + Spec: v3.FelixConfigurationSpec{WAFEventLogsFileEnabled: &enabled}, + }, + Policies: map[string]sharedconfig.ConflictPolicy{ + "spec.wafEventLogsFileEnabled": sharedconfig.ConflictOverride, + }, + }, nil + } +} + +// patchFelixConfiguration writes the fields the application layer drives. TPROXYMode stays on the +// update path for the upgrade workaround below. func (r *ReconcileApplicationLayer) patchFelixConfiguration(ctx context.Context, al *operatorv1.ApplicationLayer, gatewayWAFEnabled bool) error { - // Fetch the Istio CR and Installation variant so DesiredPolicySyncPathPrefix - // can see whether the istio side still needs the field. Both reads tolerate - // NotFound — the istio side has no claim if either is absent. - istioCR, err := utils.GetIstio(ctx, r.client) - if err != nil { + writer := sharedconfig.NewWriter(r.client, r.useV3CRDs) + + if _, err := writer.ApplyFelixConfiguration(ctx, declareWAFEventLogsFile(al, gatewayWAFEnabled)); err != nil { + return err + } + if _, err := writer.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)); err != nil { return err } - istioNeeds := utils.IstioRequiresPolicySync(istioCR, r.variant) - - _, err = sharedconfig.NewWriter(r.client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - wafEventLogsFileEnabled := wafEventLogsFileRequired(al, gatewayWAFEnabled) - - var tproxyMode string - if ok, v := r.getTProxyMode(al); ok { - tproxyMode = v - } else { - if fc.Spec.TPROXYMode == "" { - // Workaround: we'd like to always force the value to be the correct one, matching the operator's - // configuration. However, during an upgrade from a version that predates the TPROXYMode option, - // Felix hits a bug and gets confused by the new config parameter, which in turn triggers a restart. - // Work around that by relying on Disabled being the default value for the field instead. - // - // The felix bug was fixed in v3.16, v3.15.1 and v3.14.4; it should be safe to set new config fields - // once we know we're only upgrading from those versions and above. - // - // WAFEventLogsFileEnabled is an independent field: still enable it when a WAF producer - // (ApplicationLayer or the gateway data plane) requires it, without touching TPROXYMode. - if wafEventLogsFileEnabled && (fc.Spec.WAFEventLogsFileEnabled == nil || !*fc.Spec.WAFEventLogsFileEnabled) { - fc.Spec.WAFEventLogsFileEnabled = &wafEventLogsFileEnabled - log.Info("Patching FelixConfiguration: ", "wafEventLogsFileEnabled", wafEventLogsFileEnabled) - return true, nil - } - return false, nil - } - // If the mode is already set, fall through to the normal logic, it's safe to force-set the field now. - // This also avoids churning the config if a previous version of the operator set it to Disabled already, - // we avoid setting it back to nil. + _, err := writer.UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { + ok, tproxyMode := r.getTProxyMode(al) + if !ok && fc.Spec.TPROXYMode == "" { + // Setting this during an upgrade from before the field existed makes Felix restart, + // so rely on the default. + return false, nil + } + if !ok { tproxyMode = "Disabled" } - policySyncPrefix := r.getPolicySyncPathPrefix(&fc.Spec, al, istioNeeds) - policySyncPrefixSetDesired := fc.Spec.PolicySyncPathPrefix == policySyncPrefix - tproxyModeSetDesired := fc.Spec.TPROXYMode != "" && fc.Spec.TPROXYMode == string(tproxyMode) - wafEventLogsFileEnabledDesired := fc.Spec.WAFEventLogsFileEnabled != nil && *fc.Spec.WAFEventLogsFileEnabled == wafEventLogsFileEnabled - - // If tproxy mode is already set to desired state return false to indicate patch not needed. - if policySyncPrefixSetDesired && tproxyModeSetDesired && wafEventLogsFileEnabledDesired { + if fc.Spec.TPROXYMode == tproxyMode { return false, nil } - - fc.Spec.TPROXYMode = string(tproxyMode) - fc.Spec.PolicySyncPathPrefix = policySyncPrefix - fc.Spec.WAFEventLogsFileEnabled = &wafEventLogsFileEnabled - - log.Info( - "Patching FelixConfiguration: ", - "policySyncPathPrefix", fc.Spec.PolicySyncPathPrefix, - "tproxyMode", string(tproxyMode), - "wafEventLogsFileEnabled", wafEventLogsFileEnabled, - ) + fc.Spec.TPROXYMode = tproxyMode + log.Info("Patching FelixConfiguration: ", "tproxyMode", tproxyMode) return true, nil }) - return err } diff --git a/pkg/controller/applicationlayer/applicationlayer_controller_test.go b/pkg/controller/applicationlayer/applicationlayer_controller_test.go index cd04f13341..25e13c4b2e 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller_test.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller_test.go @@ -150,17 +150,15 @@ var _ = Describe("Application layer controller tests", func() { _, err = r.Reconcile(ctx, reconcile.Request{}) Expect(err).ShouldNot(HaveOccurred()) - By("ensuring that felix configuration PolicySyncPathPrefix is left as is, even after ALP deletion") + By("ensuring that felix configuration PolicySyncPathPrefix is cleared after ALP deletion") f2 := v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "default", }, } Expect(test.GetResource(c, &f2)).To(BeNil()) - // The operator-managed default is shared with egressgateway and - // Gateway API, which never clear it; the AL controller must not - // clear a value it may not own, so it is preserved here. - Expect(f2.Spec.PolicySyncPathPrefix).To(Equal("/var/run/nodeagent")) + // One field manager owns the path for every consumer, so the last one going away clears it. + Expect(f2.Spec.PolicySyncPathPrefix).To(BeEmpty()) }) It("should leave PolicySyncPathPrefix set on AL deletion when Istio CR still needs it", func() { @@ -246,7 +244,7 @@ var _ = Describe("Application layer controller tests", func() { _, err = r.Reconcile(ctx, reconcile.Request{}) Expect(err).ShouldNot(HaveOccurred()) - By("ensuring that felix configuration PolicySyncPathPrefix is left as is, even after ALP deletion") + By("ensuring that felix configuration PolicySyncPathPrefix is cleared after ALP deletion") f2 := v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "default", diff --git a/pkg/controller/egressgateway/egressgateway_controller.go b/pkg/controller/egressgateway/egressgateway_controller.go index 5d3d837995..ed69610d51 100644 --- a/pkg/controller/egressgateway/egressgateway_controller.go +++ b/pkg/controller/egressgateway/egressgateway_controller.go @@ -290,14 +290,9 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } - // patch and get the felix configuration - fc, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - if fc.Spec.PolicySyncPathPrefix != "" { - return false, nil // don't proceed with the patch - } - fc.Spec.PolicySyncPathPrefix = "/var/run/nodeagent" - return true, nil // proceed with this patch - }) + // Write and read back the felix configuration, which carries the policy sync path the + // egress gateway pods mount. + fc, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) if err != nil { reqLogger.Error(err, "Error patching felix configuration") r.status.SetDegraded(operatorv1.ResourcePatchError, "Error patching felix configuration", err, reqLogger) diff --git a/pkg/controller/gatewayapi/gatewayapi_controller.go b/pkg/controller/gatewayapi/gatewayapi_controller.go index db299cd5bd..7fa3d340ca 100644 --- a/pkg/controller/gatewayapi/gatewayapi_controller.go +++ b/pkg/controller/gatewayapi/gatewayapi_controller.go @@ -59,10 +59,6 @@ import ( "github.com/tigera/operator/pkg/tls/certificatemanagement" ) -const ( - DefaultPolicySyncPrefix = "/var/run/nodeagent" -) - var log = logf.Log.WithName("controller_gatewayapi") // Add creates a new GatewayAPI Controller and adds it to the Manager. The Manager will set fields on the Controller @@ -620,39 +616,13 @@ func GetGatewayAPI(ctx context.Context, client client.Client) (*operatorv1.Gatew return resource, "", nil } -// patchFelixConfiguration patches the FelixConfiguration resource with the desired policy sync path prefix. +// patchFelixConfiguration sets the policy sync path the gateway data plane needs. func (r *ReconcileGatewayAPI) patchFelixConfiguration(ctx context.Context) error { - _, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - policySyncPrefix := r.getPolicySyncPathPrefix(&fc.Spec) - policySyncPrefixSetDesired := DefaultPolicySyncPrefix == policySyncPrefix - - if !policySyncPrefixSetDesired && policySyncPrefix != "" { - return false, nil - } - - fc.Spec.PolicySyncPathPrefix = DefaultPolicySyncPrefix - - log.Info( - "Patching FelixConfiguration: ", - "policySyncPathPrefix", fc.Spec.PolicySyncPathPrefix, - ) - return true, nil - }) - + writer := sharedconfig.NewWriter(r.client, r.useV3CRDs) + _, err := writer.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) return err } -func (r *ReconcileGatewayAPI) getPolicySyncPathPrefix(fcSpec *v3.FelixConfigurationSpec) string { - // Respect existing policySyncPathPrefix if it's already set (e.g. EGW) - // This will cause policySyncPathPrefix value to remain when ApplicationLayer is disabled. - existing := fcSpec.PolicySyncPathPrefix - if existing != "" { - return existing - } - - return DefaultPolicySyncPrefix -} - // maintainFinalizer manages this controller's finalizer on the Installation resource. // We add a finalizer to the Installation when the API server has been installed, and only remove that finalizer when // the API server has been deleted and its pods have stopped running. This allows for a graceful cleanup of API server resources diff --git a/pkg/controller/gatewayapi/gatewayapi_controller_test.go b/pkg/controller/gatewayapi/gatewayapi_controller_test.go index 90f9fd6058..e558fc9109 100644 --- a/pkg/controller/gatewayapi/gatewayapi_controller_test.go +++ b/pkg/controller/gatewayapi/gatewayapi_controller_test.go @@ -674,7 +674,7 @@ var _ = Describe("Gateway API controller tests", func() { actualFelixConfig := &v3.FelixConfiguration{} err = c.Get(ctx, client.ObjectKey{Name: "default"}, actualFelixConfig) Expect(err).NotTo(HaveOccurred()) - Expect(actualFelixConfig.Spec.PolicySyncPathPrefix).To(Equal(DefaultPolicySyncPrefix)) + Expect(actualFelixConfig.Spec.PolicySyncPathPrefix).To(Equal(utils.DefaultPolicySyncPrefix)) }) It("Check felix configuration patching is set if it's not set", func() { @@ -704,7 +704,7 @@ var _ = Describe("Gateway API controller tests", func() { actualFelixConfig := &v3.FelixConfiguration{} err = c.Get(ctx, client.ObjectKey{Name: "default"}, actualFelixConfig) Expect(err).NotTo(HaveOccurred()) - Expect(actualFelixConfig.Spec.PolicySyncPathPrefix).ToNot(Equal(DefaultPolicySyncPrefix)) + Expect(actualFelixConfig.Spec.PolicySyncPathPrefix).ToNot(Equal(utils.DefaultPolicySyncPrefix)) Expect(actualFelixConfig.Spec.PolicySyncPathPrefix).To(Equal("/dev/null")) }) diff --git a/pkg/controller/istio/istio_controller.go b/pkg/controller/istio/istio_controller.go index 32f135e52c..1acf942021 100644 --- a/pkg/controller/istio/istio_controller.go +++ b/pkg/controller/istio/istio_controller.go @@ -17,7 +17,6 @@ package istio import ( "context" "fmt" - "strconv" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -41,7 +40,6 @@ import ( "github.com/tigera/operator/pkg/controller/utils" "github.com/tigera/operator/pkg/controller/utils/imageset" "github.com/tigera/operator/pkg/ctrlruntime" - eutils "github.com/tigera/operator/pkg/enterprise/utils" "github.com/tigera/operator/pkg/render" "github.com/tigera/operator/pkg/render/gatewayapi" "github.com/tigera/operator/pkg/render/istio" @@ -260,13 +258,15 @@ func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Reques return reconcile.Result{}, err } - _, err = sharedconfig.NewWriter(r.Client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - return r.setIstioFelixConfiguration(ctx, instance, fc, false) - }) - if err != nil { + writer := sharedconfig.NewWriter(r.Client, r.useV3CRDs) + if _, err = writer.ApplyFelixConfiguration(ctx, r.declareIstioFelixConfiguration(instance, false)); err != nil { r.status.SetDegraded(operatorv1.ResourceCreateError, "Error patching felix configuration with Istio settings", err, log) return reconcile.Result{}, err } + if _, err = writer.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.Client)); err != nil { + r.status.SetDegraded(operatorv1.ResourceCreateError, "Error patching felix configuration with the policy sync path", err, log) + return reconcile.Result{}, err + } // Clear the degraded bit if we've reached this far. r.status.ClearDegraded() @@ -281,146 +281,45 @@ func updateDefaults(istio *operatorv1.Istio) { } } -func (r *ReconcileIstio) setIstioFelixConfiguration(ctx context.Context, instance *operatorv1.Istio, fc *v3.FelixConfiguration, remove bool) (bool, error) { - ambientChanged, err := r.configureIstioAmbientMode(fc, remove) - if err != nil { - return false, err - } - dscpChanged, err := r.configureIstioDSCPMark(instance, fc, remove) - if err != nil { - return false, err - } - policySyncChanged, err := r.configurePolicySyncPathPrefix(ctx, instance, fc, remove) - if err != nil { - return false, err - } - return ambientChanged || dscpChanged || policySyncChanged, nil -} - -func (r *ReconcileIstio) configureIstioAmbientMode(fc *v3.FelixConfiguration, remove bool) (bool, error) { - var annotationMode *string - if fc.Annotations[istio.IstioOperatorAnnotationMode] != "" { - value := fc.Annotations[istio.IstioOperatorAnnotationMode] - annotationMode = &value - } - - // If the annotation does not match the spec value (ignoring both nil), it indicates a misconfiguration. - match := annotationMode == nil && fc.Spec.IstioAmbientMode == nil || - annotationMode != nil && fc.Spec.IstioAmbientMode != nil && *annotationMode == string(*fc.Spec.IstioAmbientMode) - - if !match { - return false, fmt.Errorf("felixconfig IstioAmbientMode modified by user") - } - - if remove { - if annotationMode == nil && fc.Spec.IstioAmbientMode == nil { - return false, nil +// istioFieldManager owns the FelixConfiguration fields the Istio integration sets. +const istioFieldManager = "istio" + +// declareIstioFelixConfiguration declares the Istio dataplane fields, or nothing while the +// integration is going away. +func (r *ReconcileIstio) declareIstioFelixConfiguration(instance *operatorv1.Istio, remove bool) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + d := &sharedconfig.FelixConfigurationDeclaration{ + Manager: istioFieldManager, + Owned: &v3.FelixConfiguration{}, + // A user who changes either field by hand gets a degraded status, not an override. + Policies: map[string]sharedconfig.ConflictPolicy{ + "spec.istioAmbientMode": sharedconfig.ConflictError, + "spec.istioDSCPMark": sharedconfig.ConflictError, + }, } - delete(fc.Annotations, istio.IstioOperatorAnnotationMode) - fc.Spec.IstioAmbientMode = nil - return true, nil - } - - istioModeDesired := v3.IstioAmbientModeEnabled - if fc.Spec.IstioAmbientMode != nil && *fc.Spec.IstioAmbientMode == istioModeDesired && - annotationMode != nil && *annotationMode == string(istioModeDesired) { - return false, nil - } - fc.Spec.IstioAmbientMode = &istioModeDesired - if fc.Annotations == nil { - fc.Annotations = make(map[string]string) - } - fc.Annotations[istio.IstioOperatorAnnotationMode] = string(istioModeDesired) - return true, nil -} - -func (r *ReconcileIstio) configureIstioDSCPMark(instance *operatorv1.Istio, fc *v3.FelixConfiguration, remove bool) (bool, error) { - var annotationDSCP *numorstring.DSCP - if fc.Annotations[istio.IstioOperatorAnnotationDSCP] != "" { - value, err := strconv.ParseUint(fc.Annotations[istio.IstioOperatorAnnotationDSCP], 10, 6) - if err != nil { - return false, err + if remove { + return d, nil } - dscp := numorstring.DSCPFromInt(uint8(value)) - annotationDSCP = &dscp - } - - // Return an error if it appears that FelixConfiguration has been modified out of band. - match := annotationDSCP == nil && fc.Spec.IstioDSCPMark == nil || - annotationDSCP != nil && fc.Spec.IstioDSCPMark != nil && annotationDSCP.ToUint8() == fc.Spec.IstioDSCPMark.ToUint8() - if !match { - return false, fmt.Errorf("felixconfig IstioDSCPMark modified by user") - } - - if remove || instance.Spec.DSCPMark == nil { - if annotationDSCP == nil && fc.Spec.IstioDSCPMark == nil { - return false, nil + mode := v3.IstioAmbientModeEnabled + d.Owned.Spec.IstioAmbientMode = &mode + if instance.Spec.DSCPMark != nil { + mark := *instance.Spec.DSCPMark + d.Owned.Spec.IstioDSCPMark = &mark } - delete(fc.Annotations, istio.IstioOperatorAnnotationDSCP) - fc.Spec.IstioDSCPMark = nil - return true, nil + return d, nil } - - istioDSCPMarkDesired := *instance.Spec.DSCPMark - if fc.Spec.IstioDSCPMark != nil && annotationDSCP != nil && - fc.Spec.IstioDSCPMark.ToUint8() == istioDSCPMarkDesired.ToUint8() && - annotationDSCP.ToUint8() == istioDSCPMarkDesired.ToUint8() { - return false, nil - } - fc.Spec.IstioDSCPMark = &istioDSCPMarkDesired - if fc.Annotations == nil { - fc.Annotations = make(map[string]string) - } - fc.Annotations[istio.IstioOperatorAnnotationDSCP] = strconv.FormatUint(uint64(istioDSCPMarkDesired.ToUint8()), 10) - return true, nil -} - -// configurePolicySyncPathPrefix reconciles FelixConfiguration.policySyncPathPrefix -// for the Istio side. The L7 ambient waypoint pod's l7-collector sidecar -// dials Felix's nodeagent socket, which Felix only opens when this field -// is set. The applicationlayer controller writes this same field for the -// Dikastes/sidecar/WAF flow; both controllers consult each other's state -// (via utils.{ApplicationLayerRequiresPolicySync,IstioRequiresPolicySync}) -// so that deleting one CR does not strand the other. -func (r *ReconcileIstio) configurePolicySyncPathPrefix(ctx context.Context, instance *operatorv1.Istio, fc *v3.FelixConfiguration, remove bool) (bool, error) { - var istioNeeds bool - if !remove { - // Mirror the renderer gate at pkg/render/istio/istio.go: it reads - // installationSpec.Variant (i.e. Installation.Spec.Variant), so the - // policy-sync field tracks the renderer's decision to ship the L7 - // waypoint sidecar even before Status.Variant catches up. - installationSpec, err := utils.GetInstallationSpec(ctx, r.Client) - if err != nil && !errors.IsNotFound(err) { - return false, err - } - var variant operatorv1.ProductVariant - if installationSpec != nil { - variant = installationSpec.Variant - } - istioNeeds = utils.IstioRequiresPolicySync(instance, variant) - } - - al, err := eutils.GetApplicationLayer(ctx, r.Client) - if err != nil { - return false, err - } - alNeeds := utils.ApplicationLayerRequiresPolicySync(al) - - desired := utils.DesiredPolicySyncPathPrefix(fc.Spec.PolicySyncPathPrefix, alNeeds, istioNeeds) - if fc.Spec.PolicySyncPathPrefix == desired { - return false, nil - } - fc.Spec.PolicySyncPathPrefix = desired - return true, nil } func (r *ReconcileIstio) maintainFinalizer(ctx context.Context, instance *operatorv1.Istio, reqLogger logr.Logger) (res reconcile.Result, err error, finalized bool) { // Executing clean up on finalizing if !instance.DeletionTimestamp.IsZero() { - if _, err = sharedconfig.NewWriter(r.Client, r.useV3CRDs).UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - return r.setIstioFelixConfiguration(ctx, instance, fc, true) - }); err != nil { + writer := sharedconfig.NewWriter(r.Client, r.useV3CRDs) + if _, err = writer.ApplyFelixConfiguration(ctx, r.declareIstioFelixConfiguration(instance, true)); err != nil { + r.status.SetDegraded(operatorv1.ResourceReadError, "Error cleaning up felix configuration", err, reqLogger) + return + } + if _, err = writer.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.Client)); err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error cleaning up felix configuration", err, reqLogger) return } diff --git a/pkg/controller/istio/istio_controller_test.go b/pkg/controller/istio/istio_controller_test.go index 33dcba186e..d3217ec7e6 100644 --- a/pkg/controller/istio/istio_controller_test.go +++ b/pkg/controller/istio/istio_controller_test.go @@ -389,12 +389,8 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Get(ctx, types.NamespacedName{Name: "default"}, updatedFC)).NotTo(HaveOccurred()) Expect(updatedFC.Spec.IstioAmbientMode).NotTo(BeNil()) Expect(*updatedFC.Spec.IstioAmbientMode).To(Equal(v3.IstioAmbientModeEnabled)) - Expect(updatedFC.Annotations).To(HaveKey(istio.IstioOperatorAnnotationMode)) - Expect(updatedFC.Annotations[istio.IstioOperatorAnnotationMode]).To(Equal("Enabled")) Expect(updatedFC.Spec.IstioDSCPMark).NotTo(BeNil()) Expect(updatedFC.Spec.IstioDSCPMark.ToUint8()).To(Equal(uint8(23))) - Expect(updatedFC.Annotations).To(HaveKey(istio.IstioOperatorAnnotationDSCP)) - Expect(updatedFC.Annotations[istio.IstioOperatorAnnotationDSCP]).To(Equal("23")) }) It("should preserve existing DSCPMark value", func() { @@ -432,12 +428,8 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Get(ctx, types.NamespacedName{Name: "default"}, updatedFC)).NotTo(HaveOccurred()) Expect(updatedFC.Spec.IstioAmbientMode).NotTo(BeNil()) Expect(*updatedFC.Spec.IstioAmbientMode).To(Equal(v3.IstioAmbientModeEnabled)) - Expect(updatedFC.Annotations).To(HaveKey(istio.IstioOperatorAnnotationMode)) - Expect(updatedFC.Annotations[istio.IstioOperatorAnnotationMode]).To(Equal("Enabled")) Expect(updatedFC.Spec.IstioDSCPMark).NotTo(BeNil()) Expect(updatedFC.Spec.IstioDSCPMark.ToUint8()).To(Equal(uint8(10))) - Expect(updatedFC.Annotations).To(HaveKey(istio.IstioOperatorAnnotationDSCP)) - Expect(updatedFC.Annotations[istio.IstioOperatorAnnotationDSCP]).To(Equal("10")) }) }) @@ -447,14 +439,8 @@ var _ = Describe("Istio controller tests", func() { }) It("should detect user modification of IstioAmbientMode in FelixConfiguration", func() { - // Create FelixConfiguration with mismatched annotation and spec fc := &v3.FelixConfiguration{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Annotations: map[string]string{ - istio.IstioOperatorAnnotationMode: "Enabled", - }, - }, + ObjectMeta: metav1.ObjectMeta{Name: "default"}, Spec: v3.FelixConfigurationSpec{ IstioAmbientMode: ptr.To(v3.IstioAmbientMode("Disabled")), }, @@ -470,41 +456,14 @@ var _ = Describe("Istio controller tests", func() { _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("felixconfig IstioAmbientMode modified by user")) - }) - - It("initializes nil Annotations when writing the DSCP mark (no nil-map panic)", func() { - // configureIstioAmbientMode only initializes fc.Annotations when it - // writes the mode annotation and can return without doing so, so - // configureIstioDSCPMark must guard the nil map itself before - // writing the DSCP annotation. - dscp := numorstring.DSCPFromInt(23) - instance := &operatorv1.Istio{ - ObjectMeta: metav1.ObjectMeta{Name: "default"}, - Spec: operatorv1.IstioSpec{DSCPMark: &dscp}, - } - // FelixConfiguration with nil Annotations (zero value). - fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}} - - r := &ReconcileIstio{} - changed, err := r.configureIstioDSCPMark(instance, fc, false) - Expect(err).NotTo(HaveOccurred()) - Expect(changed).To(BeTrue()) - Expect(fc.Annotations).To(HaveKeyWithValue(istio.IstioOperatorAnnotationDSCP, "23")) - Expect(fc.Spec.IstioDSCPMark).NotTo(BeNil()) - Expect(fc.Spec.IstioDSCPMark.ToUint8()).To(Equal(uint8(23))) + Expect(err.Error()).To(ContainSubstring("FelixConfiguration fields modified outside the operator: spec.istioAmbientMode")) }) It("should detect user modification of IstioDSCPMark in FelixConfiguration", func() { // Create FelixConfiguration with mismatched annotation and spec userModifiedDSCP := numorstring.DSCPFromInt(50) fc := &v3.FelixConfiguration{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Annotations: map[string]string{ - istio.IstioOperatorAnnotationDSCP: "23", - }, - }, + ObjectMeta: metav1.ObjectMeta{Name: "default"}, Spec: v3.FelixConfigurationSpec{ IstioDSCPMark: &userModifiedDSCP, }, @@ -520,7 +479,7 @@ var _ = Describe("Istio controller tests", func() { _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("felixconfig IstioDSCPMark modified by user")) + Expect(err.Error()).To(ContainSubstring("FelixConfiguration fields modified outside the operator: spec.istioDSCPMark")) }) Context("policySyncPathPrefix coordination", func() { @@ -598,7 +557,7 @@ var _ = Describe("Istio controller tests", func() { Expect(cleaned.Spec.PolicySyncPathPrefix).To(Equal("/var/run/nodeagent")) }) - It("leaves policySyncPathPrefix set on Istio deletion when ApplicationLayer features are all disabled", func() { + It("clears policySyncPathPrefix on Istio deletion when ApplicationLayer features are all disabled", func() { disabled := operatorv1.L7LogCollectionDisabled al := &operatorv1.ApplicationLayer{ ObjectMeta: metav1.ObjectMeta{Name: "tigera-secure"}, @@ -623,13 +582,11 @@ var _ = Describe("Istio controller tests", func() { cleaned := &v3.FelixConfiguration{} Expect(cli.Get(ctx, types.NamespacedName{Name: "default"}, cleaned)).NotTo(HaveOccurred()) - // Never clear a value we may not own: egressgateway and Gateway - // API share this default and never clear it, so Istio deletion - // preserves it rather than wiping it out from under them. - Expect(cleaned.Spec.PolicySyncPathPrefix).To(Equal("/var/run/nodeagent")) + // One field manager owns this for every consumer, so the last one going away clears it. + Expect(cleaned.Spec.PolicySyncPathPrefix).To(BeEmpty()) }) - It("leaves policySyncPathPrefix set on Istio deletion when ApplicationLayer is absent", func() { + It("clears policySyncPathPrefix on Istio deletion when ApplicationLayer is absent", func() { fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}} Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) @@ -645,10 +602,8 @@ var _ = Describe("Istio controller tests", func() { cleaned := &v3.FelixConfiguration{} Expect(cli.Get(ctx, types.NamespacedName{Name: "default"}, cleaned)).NotTo(HaveOccurred()) - // Never clear a value we may not own: egressgateway and Gateway - // API share this default and never clear it, so Istio deletion - // preserves it rather than wiping it out from under them. - Expect(cleaned.Spec.PolicySyncPathPrefix).To(Equal("/var/run/nodeagent")) + // One field manager owns this for every consumer, so the last one going away clears it. + Expect(cleaned.Spec.PolicySyncPathPrefix).To(BeEmpty()) }) }) @@ -679,10 +634,6 @@ var _ = Describe("Istio controller tests", func() { Expect(*patchedFC.Spec.IstioAmbientMode).To(Equal(v3.IstioAmbientModeEnabled)) Expect(patchedFC.Spec.IstioDSCPMark).NotTo(BeNil()) Expect(patchedFC.Spec.IstioDSCPMark.ToUint8()).To(Equal(uint8(23))) - Expect(patchedFC.Annotations).To(HaveKey(istio.IstioOperatorAnnotationMode)) - Expect(patchedFC.Annotations[istio.IstioOperatorAnnotationMode]).To(Equal("Enabled")) - Expect(patchedFC.Annotations).To(HaveKey(istio.IstioOperatorAnnotationDSCP)) - Expect(patchedFC.Annotations[istio.IstioOperatorAnnotationDSCP]).To(Equal("23")) // Get the Istio CR and delete it updatedIstio := &operatorv1.Istio{} @@ -700,8 +651,6 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Get(ctx, types.NamespacedName{Name: "default"}, clearedFC)).NotTo(HaveOccurred()) Expect(clearedFC.Spec.IstioAmbientMode).To(BeNil()) Expect(clearedFC.Spec.IstioDSCPMark).To(BeNil()) - Expect(clearedFC.Annotations).NotTo(HaveKey(istio.IstioOperatorAnnotationMode)) - Expect(clearedFC.Annotations).NotTo(HaveKey(istio.IstioOperatorAnnotationDSCP)) }) }) diff --git a/pkg/controller/sharedconfig/crdv1.go b/pkg/controller/sharedconfig/crdv1.go index 82c0754ea0..5eaa018226 100644 --- a/pkg/controller/sharedconfig/crdv1.go +++ b/pkg/controller/sharedconfig/crdv1.go @@ -76,7 +76,11 @@ func (w *crdV1Writer) ApplyFelixConfiguration(ctx context.Context, declare Decla if err := mergeInto(merged, payload); err != nil { return nil, err } - if err := recordWrittenValues(merged, payload, declaration, deferred); err != nil { + removed, err := removeUndeclared(merged, current, declaration, payload) + if err != nil { + return nil, err + } + if err := recordWrittenValues(merged, payload, declaration, append(deferred, removed...)); err != nil { return nil, err } if equality.Semantic.DeepEqual(current, merged) { @@ -135,6 +139,66 @@ func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurati return deferred, nil } +// removeUndeclared deletes governed fields the declaration left out, matching the way a sole +// apply owner drops them. +func removeUndeclared(merged, current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) ([]string, error) { + currentContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(current) + if err != nil { + return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + lastWritten, err := lastWrittenValues(current) + if err != nil { + return nil, err + } + + var remove, refused []string + for path := range d.Policies { + if pathSet(payload.Object, path) || !pathSet(currentContent, path) { + continue + } + if _, recorded := lastWritten[path]; !recorded { + // The operator has no record of writing this, so it belongs to someone else. + continue + } + changed, err := changedByOther(currentContent, lastWritten, path) + if err != nil { + return nil, err + } + if changed { + switch d.Policies[path] { + case ConflictDefer: + continue + case ConflictOverride: + default: + refused = append(refused, path) + continue + } + } + remove = append(remove, path) + } + + if len(refused) > 0 { + sort.Strings(refused) + return nil, &ConflictingFieldsError{Paths: refused} + } + return remove, deletePaths(merged, remove) +} + +// deletePaths clears the named fields on fc. +func deletePaths(fc *v3.FelixConfiguration, paths []string) error { + if len(paths) == 0 { + return nil + } + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(fc) + if err != nil { + return fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + for _, path := range paths { + removePath(content, path) + } + return runtime.DefaultUnstructuredConverter.FromUnstructured(content, fc) +} + func (w *crdV1Writer) persist(ctx context.Context, fc *v3.FelixConfiguration, patchFrom client.Patch) (*v3.FelixConfiguration, error) { if fc.ResourceVersion == "" { fc.Name = defaultFelixConfigName diff --git a/pkg/controller/sharedconfig/crdv1_test.go b/pkg/controller/sharedconfig/crdv1_test.go index 6094ba0248..b2e0958eb5 100644 --- a/pkg/controller/sharedconfig/crdv1_test.go +++ b/pkg/controller/sharedconfig/crdv1_test.go @@ -100,4 +100,52 @@ var _ = Describe("crd.projectcalico.org/v1 writer", func() { Expect(err).To(MatchError("user modified bpfEnabled")) Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, &v3.FelixConfiguration{})).To(HaveOccurred()) }) + + Context("a declaration that stops declaring a field", func() { + declare := func(port *int) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: "test", + Owned: &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{HealthPort: port}}, + Policies: map[string]sharedconfig.ConflictPolicy{ + "spec.healthPort": sharedconfig.ConflictDefer, + }, + }, nil + } + } + + It("should delete a field it wrote itself", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(ptr.To(9099))) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9099))) + + _, err = w.ApplyFelixConfiguration(ctx, declare(nil)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.HealthPort).To(BeNil()) + }) + + It("should leave a value it never wrote", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: v3.FelixConfigurationSpec{HealthPort: ptr.To(9199)}, + })).NotTo(HaveOccurred()) + + _, err := w.ApplyFelixConfiguration(ctx, declare(nil)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9199))) + }) + + It("should leave a value someone else changed", func() { + _, err := w.ApplyFelixConfiguration(ctx, declare(ptr.To(9099))) + Expect(err).NotTo(HaveOccurred()) + + fc := getFelixConfig() + fc.Spec.HealthPort = ptr.To(9199) + Expect(c.Update(ctx, fc)).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, declare(nil)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9199))) + }) + }) }) diff --git a/pkg/controller/sharedconfig/policysync.go b/pkg/controller/sharedconfig/policysync.go new file mode 100644 index 0000000000..0c9d4c1e15 --- /dev/null +++ b/pkg/controller/sharedconfig/policysync.go @@ -0,0 +1,105 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig + +import ( + "context" + + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + operatorv1 "github.com/tigera/operator/api/v1" + "github.com/tigera/operator/pkg/controller/utils" + eutils "github.com/tigera/operator/pkg/enterprise/utils" +) + +// PolicySyncFieldManager owns spec.policySyncPathPrefix for every feature needing it, so +// no controller can clear another's claim. +const PolicySyncFieldManager = "policy-sync" + +const policySyncPath = "spec.policySyncPathPrefix" + +// DeclarePolicySyncPathPrefix declares the socket path. Every caller reads all four CRs, so +// one field manager can own it. +func DeclarePolicySyncPathPrefix(ctx context.Context, c client.Client) DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*FelixConfigurationDeclaration, error) { + needed, err := policySyncRequired(ctx, c) + if err != nil { + return nil, err + } + + d := &FelixConfigurationDeclaration{ + Manager: PolicySyncFieldManager, + Owned: &v3.FelixConfiguration{}, + // A user who points Felix somewhere else keeps their path. + Policies: map[string]ConflictPolicy{policySyncPath: ConflictDefer}, + } + if needed { + d.Owned.Spec.PolicySyncPathPrefix = utils.DefaultPolicySyncPrefix + } + return d, nil + } +} + +// policySyncRequired reports whether any feature still needs Felix's policy-sync socket. +func policySyncRequired(ctx context.Context, c client.Client) (bool, error) { + al, err := eutils.GetApplicationLayer(ctx, c) + if err != nil { + return false, err + } + if utils.ApplicationLayerRequiresPolicySync(al) { + return true, nil + } + + gw, err := utils.GetGatewayAPI(ctx, c) + if err != nil { + return false, err + } + if utils.GatewayAPIRequiresPolicySync(gw) { + return true, nil + } + + egws, err := utils.ListEgressGateways(ctx, c) + if err != nil { + return false, err + } + for _, egw := range egws { + if egw.DeletionTimestamp.IsZero() { + return true, nil + } + } + + return istioRequiresPolicySync(ctx, c) +} + +// istioRequiresPolicySync reads the variant from the Installation spec, not its status, to +// track the renderer. +func istioRequiresPolicySync(ctx context.Context, c client.Client) (bool, error) { + istioCR, err := utils.GetIstio(ctx, c) + if err != nil || istioCR == nil { + return false, err + } + + installationSpec, err := utils.GetInstallationSpec(ctx, c) + if err != nil && !errors.IsNotFound(err) { + return false, err + } + var variant operatorv1.ProductVariant + if installationSpec != nil { + variant = installationSpec.Variant + } + return utils.IstioRequiresPolicySync(istioCR, variant), nil +} diff --git a/pkg/controller/sharedconfig/policysync_test.go b/pkg/controller/sharedconfig/policysync_test.go new file mode 100644 index 0000000000..ac1ff50bcf --- /dev/null +++ b/pkg/controller/sharedconfig/policysync_test.go @@ -0,0 +1,120 @@ +// Copyright (c) 2026 Tigera, Inc. All rights reserved. + +// 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 sharedconfig_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + operatorv1 "github.com/tigera/operator/api/v1" + "github.com/tigera/operator/pkg/apis" + "github.com/tigera/operator/pkg/controller/sharedconfig" + ctrlrfake "github.com/tigera/operator/pkg/ctrlruntime/client/fake" +) + +var _ = Describe("policySyncPathPrefix", func() { + var c client.Client + var ctx context.Context + var w sharedconfig.Writer + + apply := func() string { + fc, err := w.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, c)) + Expect(err).NotTo(HaveOccurred()) + return fc.Spec.PolicySyncPathPrefix + } + + BeforeEach(func() { + scheme := runtime.NewScheme() + Expect(apis.AddToScheme(scheme, false)).NotTo(HaveOccurred()) + c = ctrlrfake.DefaultFakeClientBuilder(scheme).Build() + ctx = context.Background() + w = sharedconfig.NewWriter(c, false) + }) + + It("should stay unset when no feature needs it", func() { + Expect(apply()).To(BeEmpty()) + }) + + It("should be set while an egress gateway exists", func() { + Expect(c.Create(ctx, &operatorv1.EgressGateway{ + ObjectMeta: metav1.ObjectMeta{Name: "egw", Namespace: "default"}, + })).NotTo(HaveOccurred()) + Expect(apply()).To(Equal("/var/run/nodeagent")) + }) + + It("should be set while the GatewayAPI CR exists", func() { + Expect(c.Create(ctx, &operatorv1.GatewayAPI{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + })).NotTo(HaveOccurred()) + Expect(apply()).To(Equal("/var/run/nodeagent")) + }) + + It("should be set while the application layer needs it", func() { + enabled := operatorv1.ApplicationLayerPolicyEnabled + Expect(c.Create(ctx, &operatorv1.ApplicationLayer{ + ObjectMeta: metav1.ObjectMeta{Name: "tigera-secure"}, + Spec: operatorv1.ApplicationLayerSpec{ApplicationLayerPolicy: &enabled}, + })).NotTo(HaveOccurred()) + Expect(apply()).To(Equal("/var/run/nodeagent")) + }) + + It("should be set while Istio needs it on Enterprise", func() { + Expect(c.Create(ctx, &operatorv1.Installation{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: operatorv1.InstallationSpec{Variant: operatorv1.CalicoEnterprise}, + })).NotTo(HaveOccurred()) + Expect(c.Create(ctx, &operatorv1.Istio{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + })).NotTo(HaveOccurred()) + Expect(apply()).To(Equal("/var/run/nodeagent")) + }) + + It("should stay unset while Istio is the only consumer on Calico", func() { + Expect(c.Create(ctx, &operatorv1.Installation{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: operatorv1.InstallationSpec{Variant: operatorv1.Calico}, + })).NotTo(HaveOccurred()) + Expect(c.Create(ctx, &operatorv1.Istio{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + })).NotTo(HaveOccurred()) + Expect(apply()).To(BeEmpty()) + }) + + It("should be cleared when the last consumer goes away", func() { + gw := &operatorv1.GatewayAPI{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + Expect(c.Create(ctx, gw)).NotTo(HaveOccurred()) + Expect(apply()).To(Equal("/var/run/nodeagent")) + + Expect(c.Delete(ctx, gw)).NotTo(HaveOccurred()) + Expect(apply()).To(BeEmpty()) + }) + + It("should keep a user's own path", func() { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: v3.FelixConfigurationSpec{PolicySyncPathPrefix: "/var/run/customer"}, + })).NotTo(HaveOccurred()) + Expect(c.Create(ctx, &operatorv1.GatewayAPI{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + })).NotTo(HaveOccurred()) + Expect(apply()).To(Equal("/var/run/customer")) + }) +}) diff --git a/pkg/controller/utils/policy_sync.go b/pkg/controller/utils/policy_sync.go index ba37ff2865..50c7ab329d 100644 --- a/pkg/controller/utils/policy_sync.go +++ b/pkg/controller/utils/policy_sync.go @@ -18,19 +18,16 @@ import ( operatorv1 "github.com/tigera/operator/api/v1" ) -// DefaultPolicySyncPrefix is the operator-managed value for -// FelixConfiguration.policySyncPathPrefix. The applicationlayer and istio -// controllers both write this value when their respective features need a -// running policy-sync gRPC server on the host (Dikastes sidecar, Istio -// ambient waypoint l7-collector, EGW). +// DefaultPolicySyncPrefix is where Felix opens the gRPC socket the Dikastes sidecar, +// the Istio waypoint l7-collector, and egress gateways dial. const DefaultPolicySyncPrefix = "/var/run/nodeagent" // ApplicationLayerRequiresPolicySync reports whether the given // ApplicationLayer CR has any feature enabled that requires -// policySyncPathPrefix to be set on FelixConfiguration. A nil receiver -// returns false (the AL CR is absent or being deleted). +// policySyncPathPrefix to be set on FelixConfiguration. A CR that is absent +// or being deleted returns false. func ApplicationLayerRequiresPolicySync(al *operatorv1.ApplicationLayer) bool { - if al == nil { + if al == nil || !al.DeletionTimestamp.IsZero() { return false } spec := &al.Spec @@ -61,31 +58,14 @@ func ApplicationLayerRequiresPolicySync(al *operatorv1.ApplicationLayer) bool { // so the FelixConfiguration field tracks the renderer — including when // waypoint logging is explicitly Disabled. func IstioRequiresPolicySync(istio *operatorv1.Istio, variant operatorv1.ProductVariant) bool { - return istio != nil && variant.IsEnterprise() && istio.WaypointLoggingEnabled() + if istio == nil || !istio.DeletionTimestamp.IsZero() { + return false + } + return variant.IsEnterprise() && istio.WaypointLoggingEnabled() } -// DesiredPolicySyncPathPrefix returns the value FelixConfiguration's -// policySyncPathPrefix should hold given the currently set value and -// whether either the applicationlayer or istio controllers need it. -// -// - Any non-empty existing value is preserved. This covers both a customer -// override and the operator-managed default claimed by another controller -// that shares this field (egressgateway, Gateway API) and never clears it. -// Those controllers only ever set the default or leave it; clearing it here -// would break them, so the applicationlayer and istio controllers likewise -// never clear a value they may not own. -// - When the field is empty and either controller needs it, the -// operator-managed default is returned. -// - Otherwise the field stays empty. -// -// Both the applicationlayer and istio controllers call this from their set and -// cleanup paths to keep coordination explicit and symmetric. -func DesiredPolicySyncPathPrefix(existing string, alNeeds, istioNeeds bool) string { - if existing != "" { - return existing - } - if alNeeds || istioNeeds { - return DefaultPolicySyncPrefix - } - return "" +// GatewayAPIRequiresPolicySync reports whether a GatewayAPI CR is present, which is when +// the gateway data plane needs Felix's policy-sync socket. +func GatewayAPIRequiresPolicySync(gw *operatorv1.GatewayAPI) bool { + return gw != nil && gw.DeletionTimestamp.IsZero() } diff --git a/pkg/controller/utils/policy_sync_test.go b/pkg/controller/utils/policy_sync_test.go index fe919b39cf..6ea5a90489 100644 --- a/pkg/controller/utils/policy_sync_test.go +++ b/pkg/controller/utils/policy_sync_test.go @@ -91,27 +91,4 @@ var _ = Describe("policySyncPathPrefix coordination predicates", func() { }, operatorv1.CalicoEnterprise)).To(BeFalse()) }) }) - - Describe("DesiredPolicySyncPathPrefix", func() { - It("preserves a customer override regardless of need flags", func() { - Expect(utils.DesiredPolicySyncPathPrefix("/var/run/customer", false, false)).To(Equal("/var/run/customer")) - Expect(utils.DesiredPolicySyncPathPrefix("/var/run/customer", true, true)).To(Equal("/var/run/customer")) - }) - - It("returns the operator default when either side needs it", func() { - Expect(utils.DesiredPolicySyncPathPrefix("", true, false)).To(Equal("/var/run/nodeagent")) - Expect(utils.DesiredPolicySyncPathPrefix("", false, true)).To(Equal("/var/run/nodeagent")) - }) - - It("leaves the field empty when nothing is set and neither side needs it", func() { - Expect(utils.DesiredPolicySyncPathPrefix("", false, false)).To(Equal("")) - }) - - It("preserves the operator default even when neither side needs it", func() { - // egressgateway and Gateway API set the same default and never clear - // it, so the applicationlayer/istio path must not clear a value it - // may not own. - Expect(utils.DesiredPolicySyncPathPrefix("/var/run/nodeagent", false, false)).To(Equal("/var/run/nodeagent")) - }) - }) }) diff --git a/pkg/controller/utils/utils.go b/pkg/controller/utils/utils.go index 41701f1cda..56c0bce898 100644 --- a/pkg/controller/utils/utils.go +++ b/pkg/controller/utils/utils.go @@ -31,6 +31,7 @@ import ( certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -445,6 +446,35 @@ func GetIstio(ctx context.Context, c client.Client) (*operatorv1.Istio, error) { return istio, nil } +// GetGatewayAPI returns the CR under its default or legacy name. Duplicate detection is +// left to the gatewayapi controller. +func GetGatewayAPI(ctx context.Context, c client.Client) (*operatorv1.GatewayAPI, error) { + for _, key := range []client.ObjectKey{DefaultInstanceKey, DefaultEnterpriseInstanceKey} { + gw := &operatorv1.GatewayAPI{} + err := c.Get(ctx, key, gw) + if err == nil { + return gw, nil + } + if !errors.IsNotFound(err) && !meta.IsNoMatchError(err) { + return nil, err + } + } + return nil, nil +} + +// ListEgressGateways returns every EgressGateway in the cluster. A cluster without the CRD +// registered has none. +func ListEgressGateways(ctx context.Context, c client.Client) ([]operatorv1.EgressGateway, error) { + egws := &operatorv1.EgressGatewayList{} + if err := c.List(ctx, egws); err != nil { + if meta.IsNoMatchError(err) { + return nil, nil + } + return nil, err + } + return egws.Items, nil +} + // Return the ManagementClusterConnection CR if present. No error is returned if it was not found. func GetManagementClusterConnection(ctx context.Context, c client.Client) (*operatorv1.ManagementClusterConnection, error) { managementClusterConnection := &operatorv1.ManagementClusterConnection{} From f655315c692e9addcca9fc7eda47e7917aa9c745 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Mon, 17 Aug 2026 11:18:43 -0400 Subject: [PATCH 07/10] Use the non-deprecated apply and managed field APIs in the shared config writer --- pkg/controller/sharedconfig/apply_test.go | 6 +++--- pkg/controller/sharedconfig/migrate.go | 2 +- pkg/controller/sharedconfig/v3.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go index af29c46b1c..f12da4ae40 100644 --- a/pkg/controller/sharedconfig/apply_test.go +++ b/pkg/controller/sharedconfig/apply_test.go @@ -73,7 +73,7 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { "metadata": map[string]any{"name": "default"}, "spec": map[string]any{"healthPort": healthPort}, }} - Expect(c.Patch(ctx, other, client.Apply, client.FieldOwner(manager), client.ForceOwnership)).NotTo(HaveOccurred()) + Expect(c.Apply(ctx, client.ApplyConfigurationFromUnstructured(other), client.FieldOwner(manager), client.ForceOwnership)).NotTo(HaveOccurred()) } BeforeEach(func() { @@ -211,7 +211,7 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { "metadata": map[string]any{"name": "default"}, "spec": map[string]any{"bpfEnabled": true}, }} - Expect(c.Patch(ctx, other, client.Apply, client.FieldOwner("kubectl"), client.ForceOwnership)).NotTo(HaveOccurred()) + Expect(c.Apply(ctx, client.ApplyConfigurationFromUnstructured(other), client.FieldOwner("kubectl"), client.ForceOwnership)).NotTo(HaveOccurred()) _, err = w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) @@ -228,7 +228,7 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { // Taking the field over moves it out of the legacy manager's field set. for _, entry := range getFelixConfig().ManagedFields { if entry.Manager == "operator" { - Expect(string(entry.FieldsV1.Raw)).NotTo(ContainSubstring("healthPort")) + Expect(entry.FieldsV1.GetRawString()).NotTo(ContainSubstring("healthPort")) } } }) diff --git a/pkg/controller/sharedconfig/migrate.go b/pkg/controller/sharedconfig/migrate.go index 13a661e76d..9e0eee8872 100644 --- a/pkg/controller/sharedconfig/migrate.go +++ b/pkg/controller/sharedconfig/migrate.go @@ -69,7 +69,7 @@ func updateOwnedPaths(fc *v3.FelixConfiguration) (legacy, others map[string]bool continue } fields := map[string]any{} - if err := json.Unmarshal(entry.FieldsV1.Raw, &fields); err != nil { + if err := json.Unmarshal(entry.FieldsV1.GetRawBytes(), &fields); err != nil { return nil, nil, fmt.Errorf("unable to parse the fields managed by %q: %w", entry.Manager, err) } out := others diff --git a/pkg/controller/sharedconfig/v3.go b/pkg/controller/sharedconfig/v3.go index de58ba03a8..e66cc5f0cb 100644 --- a/pkg/controller/sharedconfig/v3.go +++ b/pkg/controller/sharedconfig/v3.go @@ -117,7 +117,7 @@ func (w *v3Writer) resolveConflicts(applyErr error, current *v3.FelixConfigurati } func (w *v3Writer) apply(ctx context.Context, payload *unstructured.Unstructured, manager string, force bool) (*v3.FelixConfiguration, error) { - opts := []client.PatchOption{client.FieldOwner(fieldManagerPrefix + manager)} + opts := []client.ApplyOption{client.FieldOwner(fieldManagerPrefix + manager)} if force { opts = append(opts, client.ForceOwnership) } @@ -129,7 +129,7 @@ func (w *v3Writer) apply(ctx context.Context, payload *unstructured.Unstructured applied := payload.DeepCopy() applied.SetGroupVersionKind(gvk) - if err := w.client.Patch(ctx, applied, client.Apply, opts...); err != nil { + if err := w.client.Apply(ctx, client.ApplyConfigurationFromUnstructured(applied), opts...); err != nil { return nil, err } From ef0d77c6ab94551fa0ae0c2b4f816a2313f3aa14 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Thu, 20 Aug 2026 11:26:51 -0400 Subject: [PATCH 08/10] Address review findings in the shared FelixConfiguration writer Ownership falls back to the operator's pre-apply field manager, so fields it wrote before it kept records are still its own. --- .../applicationlayer_controller.go | 19 ++-- .../applicationlayer_controller_test.go | 2 +- .../egressgateway/egressgateway_controller.go | 24 ++-- .../installation/core_controller.go | 13 ++- pkg/controller/installation/felixconfig.go | 41 ++++--- .../installation/felixconfig_test.go | 68 ++++++++++-- pkg/controller/sharedconfig/apply_test.go | 103 +++++++++++++++++- pkg/controller/sharedconfig/crdv1.go | 36 ++++-- pkg/controller/sharedconfig/migrate.go | 18 ++- pkg/controller/sharedconfig/payload.go | 7 +- pkg/controller/sharedconfig/tracking.go | 25 ++--- pkg/controller/sharedconfig/v3.go | 52 ++++++++- pkg/render/istio/istio.go | 2 - 13 files changed, 320 insertions(+), 90 deletions(-) diff --git a/pkg/controller/applicationlayer/applicationlayer_controller.go b/pkg/controller/applicationlayer/applicationlayer_controller.go index 3b43c5ebd2..7aa95fa2da 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller.go @@ -512,16 +512,19 @@ const applicationLayerFieldManager = "application-layer" // and the gateway data plane. func declareWAFEventLogsFile(al *operatorv1.ApplicationLayer, gatewayWAFEnabled bool) sharedconfig.DeclareFelixConfiguration { return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { - enabled := wafEventLogsFileRequired(al, gatewayWAFEnabled) - return &sharedconfig.FelixConfigurationDeclaration{ + d := &sharedconfig.FelixConfigurationDeclaration{ Manager: applicationLayerFieldManager, - Owned: &v3.FelixConfiguration{ - Spec: v3.FelixConfigurationSpec{WAFEventLogsFileEnabled: &enabled}, - }, + Owned: &v3.FelixConfiguration{}, Policies: map[string]sharedconfig.ConflictPolicy{ "spec.wafEventLogsFileEnabled": sharedconfig.ConflictOverride, }, - }, nil + } + // Declared without a value when nothing needs it, rather than written as false: an + // upgrade from before the field existed restarts every node over a value Felix cannot read. + if enabled := wafEventLogsFileRequired(al, gatewayWAFEnabled); enabled { + d.Owned.Spec.WAFEventLogsFileEnabled = &enabled + } + return d, nil } } @@ -544,10 +547,6 @@ func (r *ReconcileApplicationLayer) patchFelixConfiguration(ctx context.Context, // so rely on the default. return false, nil } - if !ok { - tproxyMode = "Disabled" - } - if fc.Spec.TPROXYMode == tproxyMode { return false, nil } diff --git a/pkg/controller/applicationlayer/applicationlayer_controller_test.go b/pkg/controller/applicationlayer/applicationlayer_controller_test.go index 25e13c4b2e..cd9e1ea704 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller_test.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller_test.go @@ -244,7 +244,7 @@ var _ = Describe("Application layer controller tests", func() { _, err = r.Reconcile(ctx, reconcile.Request{}) Expect(err).ShouldNot(HaveOccurred()) - By("ensuring that felix configuration PolicySyncPathPrefix is cleared after ALP deletion") + By("ensuring that a user's own PolicySyncPathPrefix survives ALP deletion") f2 := v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "default", diff --git a/pkg/controller/egressgateway/egressgateway_controller.go b/pkg/controller/egressgateway/egressgateway_controller.go index ed69610d51..94844b040d 100644 --- a/pkg/controller/egressgateway/egressgateway_controller.go +++ b/pkg/controller/egressgateway/egressgateway_controller.go @@ -154,6 +154,18 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } + // Ahead of every early return below, because the last egress gateway going away is what + // clears the policy sync path. + fc, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) + if err != nil { + reqLogger.Error(err, "Error patching felix configuration") + r.status.SetDegraded(operatorv1.ResourcePatchError, "Error patching felix configuration", err, reqLogger) + for _, egw := range egws { + setDegraded(r.client, ctx, &egw, reconcileErr, fmt.Sprintf("Error patching felix configuration err = %s", err.Error())) + } + return reconcile.Result{}, err + } + // If there are no Egress Gateway resources, return. ch := utils.NewComponentHandler(log, r.client, r.scheme, nil) if len(egws) == 0 { @@ -290,18 +302,6 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } - // Write and read back the felix configuration, which carries the policy sync path the - // egress gateway pods mount. - fc, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) - if err != nil { - reqLogger.Error(err, "Error patching felix configuration") - r.status.SetDegraded(operatorv1.ResourcePatchError, "Error patching felix configuration", err, reqLogger) - for _, egw := range egwsToReconcile { - setDegraded(r.client, ctx, &egw, reconcileErr, fmt.Sprintf("Error patching felix configuration err = %s", err.Error())) - } - return reconcile.Result{}, err - } - // Reconcile all the EGWs var errMsgs []string for _, egw := range egwsToReconcile { diff --git a/pkg/controller/installation/core_controller.go b/pkg/controller/installation/core_controller.go index 29aa977651..1faff533ef 100644 --- a/pkg/controller/installation/core_controller.go +++ b/pkg/controller/installation/core_controller.go @@ -1033,7 +1033,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile // Set any non-default FelixConfiguration values that we need. felixWriter := sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs) - _, err = felixWriter.ApplyFelixConfiguration(ctx, r.declareFelixConfiguration(instance)) + defaulted, err := felixWriter.ApplyFelixConfiguration(ctx, r.declareFelixConfiguration(instance)) if err != nil { r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error updating FelixConfiguration", err, reqLogger) return reconcile.Result{}, err @@ -1044,6 +1044,13 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile return reconcile.Result{}, err } + // spec.healthPort comes from the write above, which a user is free to defer, so take the port + // from that write rather than from a read that may not have caught up with it. + felixHealthPort := defaultFelixHealthPort(instance) + if defaulted.Spec.HealthPort != nil { + felixHealthPort = *defaulted.Spec.HealthPort + } + // Set any non-default BGPConfiguration values that we need. _, err = utils.PatchBGPConfiguration(ctx, r.client, func(bgpConfig *v3.BGPConfiguration) (bool, error) { // Configure cluster routing mode. @@ -1191,7 +1198,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile TLS: typhaNodeTLS, MigrateNamespaces: needsNamespaceMigration, ClusterDomain: r.opts.ClusterDomain, - FelixHealthPort: *felixConfiguration.Spec.HealthPort, + FelixHealthPort: felixHealthPort, } components = append(components, render.Typha(&typhaCfg)) @@ -1316,7 +1323,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile NodeAppArmorProfile: nodeAppArmorProfile, MigrateNamespaces: needsNamespaceMigration, CanRemoveCNIFinalizer: canRemoveCNI, - FelixHealthPort: *felixConfiguration.Spec.HealthPort, + FelixHealthPort: felixHealthPort, NodeCgroupV2Path: felixConfiguration.Spec.CgroupV2Path, V3CRDs: r.opts.UseV3CRDs, ImageOverrides: r.ext.Images(), diff --git a/pkg/controller/installation/felixconfig.go b/pkg/controller/installation/felixconfig.go index a516820e2f..0cfa3aa7ac 100644 --- a/pkg/controller/installation/felixconfig.go +++ b/pkg/controller/installation/felixconfig.go @@ -37,13 +37,23 @@ const ( ) // declareFelixConfiguration declares the fields defaulted from the Installation spec, always -// declaring every one so the field set stays stable. +// declaring every one so the field set stays stable. A field the spec stops asking for is +// declared without a value, which clears whatever the operator wrote there. func (r *ReconcileInstallation) declareFelixConfiguration(install *operatorv1.Installation) sharedconfig.DeclareFelixConfiguration { return func(current *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { d := &sharedconfig.FelixConfigurationDeclaration{ - Manager: felixConfigFieldManager, - Owned: &v3.FelixConfiguration{}, - Policies: map[string]sharedconfig.ConflictPolicy{}, + Manager: felixConfigFieldManager, + Owned: &v3.FelixConfiguration{}, + Policies: map[string]sharedconfig.ConflictPolicy{ + "spec.routeTableRange": sharedconfig.ConflictDefer, + "spec.healthPort": sharedconfig.ConflictDefer, + "spec.vxlanVNI": sharedconfig.ConflictDefer, + "spec.vxlanPort": sharedconfig.ConflictDefer, + "spec.bpfHostConntrackBypass": sharedconfig.ConflictDefer, + "spec.bpfKubeProxyHealthzPort": sharedconfig.ConflictDefer, + "spec.nftablesMode": sharedconfig.ConflictOverride, + "spec.programClusterRoutes": sharedconfig.ConflictOverride, + }, } owned := &d.Owned.Spec @@ -52,18 +62,11 @@ func (r *ReconcileInstallation) declareFelixConfiguration(install *operatorv1.In case operatorv1.PluginAmazonVPC: // AWS uses the ENI device number + 1, and the VLAN table ID + 100. owned.RouteTableRange = &v3.RouteTableRange{Min: 65, Max: 99} - d.Policies["spec.routeTableRange"] = sharedconfig.ConflictDefer case operatorv1.PluginGKE: owned.RouteTableRange = &v3.RouteTableRange{Min: 10, Max: 250} - d.Policies["spec.routeTableRange"] = sharedconfig.ConflictDefer } - healthPort := 9099 - if install.Spec.KubernetesProvider.IsOpenShift() { - healthPort = 9199 - } - owned.HealthPort = &healthPort - d.Policies["spec.healthPort"] = sharedconfig.ConflictDefer + owned.HealthPort = ptr.To(defaultFelixHealthPort(install)) vxlanVNI, vxlanPort := 4096, 4789 if install.Spec.KubernetesProvider == operatorv1.ProviderDockerEE { @@ -76,23 +79,18 @@ func (r *ReconcileInstallation) declareFelixConfiguration(install *operatorv1.In // The eBPF dataplane only works with MKE when conntrack bypass is off. owned.BPFHostConntrackBypass = ptr.To(false) - d.Policies["spec.bpfHostConntrackBypass"] = sharedconfig.ConflictDefer } } owned.VXLANVNI = &vxlanVNI owned.VXLANPort = &vxlanPort - d.Policies["spec.vxlanVNI"] = sharedconfig.ConflictDefer - d.Policies["spec.vxlanPort"] = sharedconfig.ConflictDefer if install.Spec.BPFEnabled() && !install.Spec.KubeProxyManagementEnabled() { // The platform's kube-proxy holds 10256, so Felix's healthz server would fail to bind. owned.BPFKubeProxyHealthzPort = ptr.To(0) - d.Policies["spec.bpfKubeProxyHealthzPort"] = sharedconfig.ConflictDefer } if install.Spec.CalicoNetwork != nil && install.Spec.CalicoNetwork.LinuxDataplane != nil { owned.NFTablesMode = ptr.To(nftablesMode(install)) - d.Policies["spec.nftablesMode"] = sharedconfig.ConflictOverride } // Gated on the field being set, so leaving it unset keeps meaning "whatever Calico @@ -100,7 +98,6 @@ func (r *ReconcileInstallation) declareFelixConfiguration(install *operatorv1.In if install.Spec.CalicoNetwork != nil && install.Spec.CalicoNetwork.ClusterRoutingMode != nil { mode := *install.Spec.CalicoNetwork.ClusterRoutingMode owned.ProgramClusterRoutes = ptr.To(felixProgramClusterRoutesValue(mode)) - d.Policies["spec.programClusterRoutes"] = sharedconfig.ConflictOverride } extPaths, err := r.ext.DeclareFelixConfiguration(&install.Spec, current, d.Owned) @@ -115,6 +112,14 @@ func (r *ReconcileInstallation) declareFelixConfiguration(install *operatorv1.In } } +// defaultFelixHealthPort is the port the operator defaults Felix's health server to. +func defaultFelixHealthPort(install *operatorv1.Installation) int { + if install.Spec.KubernetesProvider.IsOpenShift() { + return 9199 + } + return 9099 +} + // nftablesMode is the dataplane mode Felix should run in. The operator has always owned it, // so nothing older needs preserving. func nftablesMode(install *operatorv1.Installation) v3.NFTablesMode { diff --git a/pkg/controller/installation/felixconfig_test.go b/pkg/controller/installation/felixconfig_test.go index 2577a89e18..fab2d1ca78 100644 --- a/pkg/controller/installation/felixconfig_test.go +++ b/pkg/controller/installation/felixconfig_test.go @@ -53,14 +53,19 @@ var _ = Describe("FelixConfiguration declarations", func() { return paths } + governed := []string{ + "spec.routeTableRange", + "spec.healthPort", + "spec.vxlanVNI", + "spec.vxlanPort", + "spec.bpfHostConntrackBypass", + "spec.bpfKubeProxyHealthzPort", + "spec.nftablesMode", + "spec.programClusterRoutes", + } + It("declares the same fields no matter what the current object holds", func() { - empty := declaredPaths(install(), &v3.FelixConfiguration{}) - Expect(empty).To(ConsistOf( - "spec.healthPort", - "spec.vxlanVNI", - "spec.vxlanPort", - "spec.nftablesMode", - )) + Expect(declaredPaths(install(), &v3.FelixConfiguration{})).To(ConsistOf(governed)) // Every field the operator defaults is already set, by the operator or by anyone else. populated := declaredPaths(install(), &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{ @@ -69,7 +74,54 @@ var _ = Describe("FelixConfiguration declarations", func() { VXLANPort: ptr.To(1111), NFTablesMode: ptr.To(v3.NFTablesModeDisabled), }}) - Expect(populated).To(ConsistOf(empty)) + Expect(populated).To(ConsistOf(governed)) + }) + + It("declares the same fields no matter what the Installation asks for", func() { + bpf := operatorv1.LinuxDataplaneBPF + specs := []struct { + name string + install *operatorv1.Installation + // extra holds the paths the extension declares for this provider, on top of the + // fields the installation controller governs itself. + extra []string + }{ + {name: "the default install", install: install()}, + {name: "iptables on AWS", install: &operatorv1.Installation{Spec: operatorv1.InstallationSpec{ + CNI: &operatorv1.CNISpec{Type: operatorv1.PluginAmazonVPC}, + KubernetesProvider: operatorv1.ProviderEKS, + }}}, + {name: "eBPF on MKE", install: &operatorv1.Installation{Spec: operatorv1.InstallationSpec{ + CNI: &operatorv1.CNISpec{Type: operatorv1.PluginCalico}, + KubernetesProvider: operatorv1.ProviderDockerEE, + CalicoNetwork: &operatorv1.CalicoNetworkSpec{LinuxDataplane: &bpf}, + }}}, + {name: "OpenShift with cluster routing set", install: &operatorv1.Installation{Spec: operatorv1.InstallationSpec{ + CNI: &operatorv1.CNISpec{Type: operatorv1.PluginCalico}, + KubernetesProvider: operatorv1.ProviderOpenShift, + CalicoNetwork: &operatorv1.CalicoNetworkSpec{ + LinuxDataplane: &nftables, + ClusterRoutingMode: ptr.To(operatorv1.ClusterRoutingModeFelix), + }, + }}, extra: []string{"spec.dnsTrustedServers"}}, + } + for _, spec := range specs { + Expect(declaredPaths(spec.install, &v3.FelixConfiguration{})).To(ConsistOf(append(spec.extra, governed...)), spec.name) + } + }) + + It("clears a field the Installation stops asking for", func() { + i := install() + i.Spec.CalicoNetwork.ClusterRoutingMode = ptr.To(operatorv1.ClusterRoutingModeFelix) + d, err := r.declareFelixConfiguration(i)(&v3.FelixConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(d.Owned.Spec.ProgramClusterRoutes).NotTo(BeNil()) + + // Declared with no value, which is what clears whatever the operator wrote there. + d, err = r.declareFelixConfiguration(install())(&v3.FelixConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(d.Owned.Spec.ProgramClusterRoutes).To(BeNil()) + Expect(d.Policies).To(HaveKey("spec.programClusterRoutes")) }) It("declares the values it wants, not the values already there", func() { diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go index f12da4ae40..c8b5410030 100644 --- a/pkg/controller/sharedconfig/apply_test.go +++ b/pkg/controller/sharedconfig/apply_test.go @@ -52,6 +52,17 @@ func declare(healthPolicy, vxlanPolicy sharedconfig.ConflictPolicy) sharedconfig } } +// declarePolicySync governs spec.policySyncPathPrefix, declaring a value only when prefix is set. +func declarePolicySync(prefix string) sharedconfig.DeclareFelixConfiguration { + return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { + return &sharedconfig.FelixConfigurationDeclaration{ + Manager: "policy-sync", + Owned: &v3.FelixConfiguration{Spec: v3.FelixConfigurationSpec{PolicySyncPathPrefix: prefix}}, + Policies: map[string]sharedconfig.ConflictPolicy{"spec.policySyncPathPrefix": sharedconfig.ConflictDefer}, + }, nil + } +} + var _ = Describe("Applying declared FelixConfiguration fields", func() { var c client.Client var ctx context.Context @@ -136,6 +147,18 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Expect(getFelixConfig().Spec.HealthPort).To(Equal(ptr.To(9100))) }) + It("should take a field that already holds the declared value, without arbitrating", func() { + applyAs("kubectl", 9099) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictError, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + Expect(getFelixConfig().ManagedFields).To(ContainElement(SatisfyAll( + HaveField("Manager", "tigera-operator/installation"), + HaveField("Operation", metav1.ManagedFieldsOperationApply), + ))) + }) + It("should delete a field it stops declaring, so the declared set has to stay stable", func() { _, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictDefer, sharedconfig.ConflictDefer)) Expect(err).NotTo(HaveOccurred()) @@ -162,8 +185,8 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { } } - // createAsManager writes the way the operator's merge patch used to, against a manager - // that never applied. + // createAsManager writes the way a plain update does, under a manager with no apply + // of its own. createAsManager := func(manager string, annotations map[string]string, spec v3.FelixConfigurationSpec) { Expect(c.Create(ctx, &v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{Name: "default", Annotations: annotations}, @@ -204,7 +227,7 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) Expect(err).NotTo(HaveOccurred()) - // The stale annotation still reads "true", which is what a user now applies. + // The stale annotation still reads "true", matching the value the user applies. other := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "projectcalico.org/v3", "kind": "FelixConfiguration", @@ -241,6 +264,39 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9100))) Expect(fc.Spec.VXLANPort).To(Equal(ptr.To(4789))) }) + + It("should clear a field its legacy manager holds that the declaration dropped", func() { + createAsManager("operator", nil, v3.FelixConfigurationSpec{PolicySyncPathPrefix: "/var/run/nodeagent"}) + + _, err := w.ApplyFelixConfiguration(ctx, declarePolicySync("")) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.PolicySyncPathPrefix).To(BeEmpty()) + }) + + It("should leave a dropped field alone when someone else wrote it", func() { + createByUpdate(nil, v3.FelixConfigurationSpec{PolicySyncPathPrefix: "/var/run/customer"}) + + _, err := w.ApplyFelixConfiguration(ctx, declarePolicySync("")) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.PolicySyncPathPrefix).To(Equal("/var/run/customer")) + }) + + It("should stop using its record once it has applied the field itself", func() { + createByUpdate(map[string]string{render.BPFOperatorAnnotation: "true"}, + v3.FelixConfigurationSpec{BPFEnabled: ptr.To(true)}) + _, err := w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(false))) + + // A user turns it back on by hand, to the value the stale annotation still names. + fc := getFelixConfig() + fc.Spec.BPFEnabled = ptr.To(true) + Expect(c.Update(ctx, fc, client.FieldOwner("kubectl"))).NotTo(HaveOccurred()) + + _, err = w.ApplyFelixConfiguration(ctx, declareBPF(sharedconfig.ConflictError)) + Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) + Expect(getFelixConfig().Spec.BPFEnabled).To(Equal(ptr.To(true))) + }) }) }) @@ -328,6 +384,47 @@ var _ = Describe("Applying declared FelixConfiguration fields", func() { Expect(err).To(BeAssignableToTypeOf(&sharedconfig.ConflictingFieldsError{})) }) + Context("a cluster the operator wrote before it recorded its writes", func() { + BeforeEach(func() { + scheme := runtime.NewScheme() + Expect(apis.AddToScheme(scheme, false)).NotTo(HaveOccurred()) + c = ctrlrfake.DefaultFakeClientBuilder(scheme).WithReturnManagedFields().Build() + ctx = context.Background() + w = sharedconfig.NewWriter(c, false) + }) + + createAsManager := func(manager string, spec v3.FelixConfigurationSpec) { + Expect(c.Create(ctx, &v3.FelixConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Spec: spec, + }, client.FieldOwner(manager))).NotTo(HaveOccurred()) + } + + It("should take over a field its own legacy manager holds", func() { + createAsManager("operator", v3.FelixConfigurationSpec{HealthPort: ptr.To(9100)}) + + fc, err := w.ApplyFelixConfiguration(ctx, declare(sharedconfig.ConflictError, sharedconfig.ConflictDefer)) + Expect(err).NotTo(HaveOccurred()) + Expect(fc.Spec.HealthPort).To(Equal(ptr.To(9099))) + }) + + It("should clear a field its legacy manager holds that the declaration dropped", func() { + createAsManager("operator", v3.FelixConfigurationSpec{PolicySyncPathPrefix: "/var/run/nodeagent"}) + + _, err := w.ApplyFelixConfiguration(ctx, declarePolicySync("")) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.PolicySyncPathPrefix).To(BeEmpty()) + }) + + It("should leave a dropped field alone when someone else wrote it", func() { + createAsManager("someone-else", v3.FelixConfigurationSpec{PolicySyncPathPrefix: "/var/run/customer"}) + + _, err := w.ApplyFelixConfiguration(ctx, declarePolicySync("")) + Expect(err).NotTo(HaveOccurred()) + Expect(getFelixConfig().Spec.PolicySyncPathPrefix).To(Equal("/var/run/customer")) + }) + }) + Context("bpfEnabled, which older operators recorded in their own annotation", func() { declareBPF := func(policy sharedconfig.ConflictPolicy) sharedconfig.DeclareFelixConfiguration { return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { diff --git a/pkg/controller/sharedconfig/crdv1.go b/pkg/controller/sharedconfig/crdv1.go index 5eaa018226..ad7073d40a 100644 --- a/pkg/controller/sharedconfig/crdv1.go +++ b/pkg/controller/sharedconfig/crdv1.go @@ -50,10 +50,11 @@ func (w *crdV1Writer) ApplyFelixConfiguration(ctx context.Context, declare Decla if err != nil { return nil, err } - patchFrom := client.MergeFrom(current.DeepCopy()) if err := utils.RestoreV3Metadata(current); err != nil { return nil, err } + // Diff against the restored object, so the patch leaves the v3 metadata stash alone. + patchFrom := client.MergeFrom(current.DeepCopy()) declaration, err := declare(current) if err != nil { @@ -67,7 +68,13 @@ func (w *crdV1Writer) ApplyFelixConfiguration(ctx context.Context, declare Decla if err != nil { return nil, err } - deferred, err := resolveTrackedConflicts(current, declaration, payload) + // Fields the operator's pre-apply manager still owns are its own, whether or not it kept a + // record of writing them. + legacyOwned, _, err := updateOwnedPaths(current) + if err != nil { + return nil, err + } + deferred, err := resolveTrackedConflicts(current, declaration, payload, legacyOwned) if err != nil { return nil, err } @@ -76,7 +83,7 @@ func (w *crdV1Writer) ApplyFelixConfiguration(ctx context.Context, declare Decla if err := mergeInto(merged, payload); err != nil { return nil, err } - removed, err := removeUndeclared(merged, current, declaration, payload) + removed, err := removeUndeclared(merged, current, declaration, payload, legacyOwned) if err != nil { return nil, err } @@ -86,11 +93,15 @@ func (w *crdV1Writer) ApplyFelixConfiguration(ctx context.Context, declare Decla if equality.Semantic.DeepEqual(current, merged) { return current, nil } + if current.ResourceVersion == "" && !declaresSpec(payload) { + // The declaration holds nothing to write, so don't create an object carrying only a record. + return current, nil + } return w.persist(ctx, merged, patchFrom) } // resolveTrackedConflicts drops deferred fields from payload and returns the paths it dropped. -func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) ([]string, error) { +func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured, legacyOwned map[string]bool) ([]string, error) { currentContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(current) if err != nil { return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) @@ -114,7 +125,7 @@ func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurati continue } - changed, err := changedByOther(currentContent, lastWritten, path) + changed, err := changedByOther(currentContent, lastWritten, legacyOwned, path) if err != nil { return nil, err } @@ -141,7 +152,7 @@ func resolveTrackedConflicts(current *v3.FelixConfiguration, d *FelixConfigurati // removeUndeclared deletes governed fields the declaration left out, matching the way a sole // apply owner drops them. -func removeUndeclared(merged, current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) ([]string, error) { +func removeUndeclared(merged, current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured, legacyOwned map[string]bool) ([]string, error) { currentContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(current) if err != nil { return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) @@ -156,11 +167,11 @@ func removeUndeclared(merged, current *v3.FelixConfiguration, d *FelixConfigurat if pathSet(payload.Object, path) || !pathSet(currentContent, path) { continue } - if _, recorded := lastWritten[path]; !recorded { - // The operator has no record of writing this, so it belongs to someone else. + if _, recorded := lastWritten[path]; !recorded && !legacyOwned[path] { + // The operator has no sign of writing this, so it belongs to someone else. continue } - changed, err := changedByOther(currentContent, lastWritten, path) + changed, err := changedByOther(currentContent, lastWritten, legacyOwned, path) if err != nil { return nil, err } @@ -221,13 +232,14 @@ func (w *crdV1Writer) UpdateFelixConfiguration(ctx context.Context, updateFn fun return nil, fmt.Errorf("unable to read FelixConfiguration: %w", err) } - // Create a base state for the upcoming patch operation. - patchFrom := client.MergeFrom(fc.DeepCopy()) - if err = utils.RestoreV3Metadata(fc); err != nil { return nil, err } + // Create a base state for the upcoming patch operation, diffing against the restored object so + // the patch leaves the v3 metadata stash alone. + patchFrom := client.MergeFrom(fc.DeepCopy()) + // Apply desired changes to the FelixConfiguration. updated, err := updateFn(fc) if err != nil { diff --git a/pkg/controller/sharedconfig/migrate.go b/pkg/controller/sharedconfig/migrate.go index 9e0eee8872..a76e57d989 100644 --- a/pkg/controller/sharedconfig/migrate.go +++ b/pkg/controller/sharedconfig/migrate.go @@ -30,9 +30,9 @@ const legacyFieldManager = "operator" // reclaimablePaths lists fields a plain update owns that the operator wrote itself. // An apply must force ownership across once. -func reclaimablePaths(fc *v3.FelixConfiguration) (map[string]bool, error) { +func reclaimablePaths(fc *v3.FelixConfiguration, manager string) (map[string]bool, error) { reclaimable, others, err := updateOwnedPaths(fc) - if err != nil || len(others) == 0 { + if err != nil || len(others) == 0 || appliedBy(fc, manager) { return reclaimable, err } @@ -49,7 +49,8 @@ func reclaimablePaths(fc *v3.FelixConfiguration) (map[string]bool, error) { if !others[path] { continue } - changed, err := changedByOther(content, lastWritten, path) + // Legacy ownership is beside the point here: these paths belong to another manager. + changed, err := changedByOther(content, lastWritten, nil, path) if err != nil { return nil, err } @@ -60,6 +61,17 @@ func reclaimablePaths(fc *v3.FelixConfiguration) (map[string]bool, error) { return reclaimable, nil } +// appliedBy reports whether manager has already applied to fc. The operator's records only speak +// for the writes that came before its first apply, so they stop counting once it has one. +func appliedBy(fc *v3.FelixConfiguration, manager string) bool { + for _, entry := range fc.ManagedFields { + if entry.Operation == metav1.ManagedFieldsOperationApply && entry.Manager == manager { + return true + } + } + return false +} + // updateOwnedPaths splits the fields owned through a plain update by whether the operator's own // legacy field manager holds them. func updateOwnedPaths(fc *v3.FelixConfiguration) (legacy, others map[string]bool, err error) { diff --git a/pkg/controller/sharedconfig/payload.go b/pkg/controller/sharedconfig/payload.go index f2f2887b62..55646b5190 100644 --- a/pkg/controller/sharedconfig/payload.go +++ b/pkg/controller/sharedconfig/payload.go @@ -41,11 +41,16 @@ func declaredPayload(owned *v3.FelixConfiguration) (*unstructured.Unstructured, u := &unstructured.Unstructured{Object: content} unstructured.RemoveNestedField(u.Object, "metadata") - unstructured.RemoveNestedField(u.Object, "status") u.SetName(defaultFelixConfigName) return u, nil } +// declaresSpec reports whether the payload sets any field at all. +func declaresSpec(payload *unstructured.Unstructured) bool { + spec, found, err := unstructured.NestedMap(payload.Object, "spec") + return err == nil && found && len(spec) > 0 +} + // pathSet reports whether path holds a value in obj. func pathSet(obj map[string]any, path string) bool { _, found, err := unstructured.NestedFieldNoCopy(obj, strings.Split(path, ".")...) diff --git a/pkg/controller/sharedconfig/tracking.go b/pkg/controller/sharedconfig/tracking.go index 65b776f8d6..c931973c62 100644 --- a/pkg/controller/sharedconfig/tracking.go +++ b/pkg/controller/sharedconfig/tracking.go @@ -50,8 +50,9 @@ func lastWrittenValues(fc *v3.FelixConfiguration) (map[string]any, error) { return values, nil } -// changedByOther reports whether path holds a value the operator did not write. -func changedByOther(currentContent map[string]any, lastWritten map[string]any, path string) (bool, error) { +// changedByOther reports whether path holds a value the operator did not write. Fields the +// operator wrote before it kept records are still its own, marked by its pre-apply field manager. +func changedByOther(currentContent map[string]any, lastWritten map[string]any, legacyOwned map[string]bool, path string) (bool, error) { current, found, err := unstructured.NestedFieldNoCopy(currentContent, strings.Split(path, ".")...) if err != nil { return false, fmt.Errorf("unable to read %s: %w", path, err) @@ -62,7 +63,7 @@ func changedByOther(currentContent map[string]any, lastWritten map[string]any, p written, recorded := lastWritten[path] if !recorded { - return true, nil + return !legacyOwned[path], nil } canonical, err := canonicalize(current) if err != nil { @@ -149,25 +150,17 @@ func mergeInto(fc *v3.FelixConfiguration, payload *unstructured.Unstructured) er if spec == nil { spec = map[string]any{} } - mergeMaps(spec, declared) + // Overlay whole fields rather than merging into them, so a struct field lands the way an + // apply would place it. + for field, value := range declared { + spec[field] = value + } if err := unstructured.SetNestedMap(content, spec, "spec"); err != nil { return err } return runtime.DefaultUnstructuredConverter.FromUnstructured(content, fc) } -func mergeMaps(dst, src map[string]any) { - for key, value := range src { - if srcMap, ok := value.(map[string]any); ok { - if dstMap, ok := dst[key].(map[string]any); ok { - mergeMaps(dstMap, srcMap) - continue - } - } - dst[key] = value - } -} - // canonicalize renders a value the way it will read back out of the annotation. func canonicalize(value any) (any, error) { encoded, err := json.Marshal(value) diff --git a/pkg/controller/sharedconfig/v3.go b/pkg/controller/sharedconfig/v3.go index e66cc5f0cb..22f8e34e8e 100644 --- a/pkg/controller/sharedconfig/v3.go +++ b/pkg/controller/sharedconfig/v3.go @@ -16,12 +16,16 @@ package sharedconfig import ( "context" + "encoding/json" "fmt" + "strings" v3 "github.com/tigera/api/pkg/apis/projectcalico/v3" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -56,6 +60,9 @@ func (w *v3Writer) ApplyFelixConfiguration(ctx context.Context, declare DeclareF if err != nil { return nil, err } + if err := w.clearLegacyOwned(ctx, current, declaration, payload); err != nil { + return nil, err + } applied, err := w.apply(ctx, payload, declaration.Manager, false) if err == nil { @@ -79,7 +86,11 @@ func (w *v3Writer) resolveConflicts(applyErr error, current *v3.FelixConfigurati return false, applyErr } - reclaimable, err := reclaimablePaths(current) + currentContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(current) + if err != nil { + return false, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + reclaimable, err := reclaimablePaths(current, fieldManagerPrefix+d.Manager) if err != nil { return false, err } @@ -92,6 +103,16 @@ func (w *v3Writer) resolveConflicts(applyErr error, current *v3.FelixConfigurati undeclared = append(undeclared, path) continue } + // An apply conflicts on ownership, not on value. Taking a field that already holds the + // declared value changes nothing, so there is nothing to arbitrate. + agree, err := valuesAgree(currentContent, payload.Object, declared) + if err != nil { + return false, err + } + if agree { + force = true + continue + } if reclaimable[declared] || reclaimable[path] { // The operator wrote this before it applied, so take the field rather than arbitrate. force = true @@ -116,6 +137,35 @@ func (w *v3Writer) resolveConflicts(applyErr error, current *v3.FelixConfigurati return force, nil } +// clearLegacyOwned deletes governed fields the operator's pre-apply field manager still holds and +// the declaration does not set. An apply cannot drop a field it does not own. +func (w *v3Writer) clearLegacyOwned(ctx context.Context, current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) error { + legacyOwned, _, err := updateOwnedPaths(current) + if err != nil || len(legacyOwned) == 0 { + return err + } + + remove := map[string]any{} + for path := range d.Policies { + if !legacyOwned[path] || pathSet(payload.Object, path) { + continue + } + if err := unstructured.SetNestedField(remove, nil, strings.Split(path, ".")...); err != nil { + return err + } + } + if len(remove) == 0 { + return nil + } + + encoded, err := json.Marshal(remove) + if err != nil { + return fmt.Errorf("unable to render the fields to clear: %w", err) + } + fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: defaultFelixConfigName}} + return w.client.Patch(ctx, fc, client.RawPatch(types.MergePatchType, encoded)) +} + func (w *v3Writer) apply(ctx context.Context, payload *unstructured.Unstructured, manager string, force bool) (*v3.FelixConfiguration, error) { opts := []client.ApplyOption{client.FieldOwner(fieldManagerPrefix + manager)} if force { diff --git a/pkg/render/istio/istio.go b/pkg/render/istio/istio.go index 98d2fb9141..7b62b43ac6 100644 --- a/pkg/render/istio/istio.go +++ b/pkg/render/istio/istio.go @@ -69,8 +69,6 @@ const ( IstioCNIDaemonSetName = "istio-cni-node" IstioZTunnelDaemonSetName = "ztunnel" IstioSidecarInjectorConfigMapName = "istio-sidecar-injector" - IstioOperatorAnnotationMode = "operator.tigera.io/istioAmbientMode" - IstioOperatorAnnotationDSCP = "operator.tigera.io/istioDSCPMark" IstioFinalizer = "operator.tigera.io/calico-istio" IstioIstiodPolicyName = networkpolicy.CalicoComponentPolicyPrefix + IstioIstiodDeploymentName IstioCNIPolicyName = networkpolicy.CalicoComponentPolicyPrefix + IstioCNIDaemonSetName From 5a6eeef7936e1a0c1f520e48f0d5fb649d4c0c4a Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Thu, 20 Aug 2026 11:33:49 -0400 Subject: [PATCH 09/10] Store controller options on the egress gateway and istio reconcilers Drops the per-field copies, two of which nothing read. --- .../egressgateway/egressgateway_controller.go | 20 +- .../egressgateway_controller_test.go | 2 +- pkg/controller/istio/istio_controller.go | 20 +- pkg/controller/istio/istio_controller_test.go | 179 +++++++++--------- 4 files changed, 107 insertions(+), 114 deletions(-) diff --git a/pkg/controller/egressgateway/egressgateway_controller.go b/pkg/controller/egressgateway/egressgateway_controller.go index 94844b040d..bee09abf06 100644 --- a/pkg/controller/egressgateway/egressgateway_controller.go +++ b/pkg/controller/egressgateway/egressgateway_controller.go @@ -82,11 +82,8 @@ func newReconciler(mgr manager.Manager, opts options.ControllerOptions, licenseA r := &ReconcileEgressGateway{ client: mgr.GetClient(), scheme: mgr.GetScheme(), - provider: opts.DetectedProvider, status: status.New(mgr.GetClient(), "egressgateway", opts.KubernetesVersion), - clusterDomain: opts.ClusterDomain, - useV3CRDs: opts.UseV3CRDs, - variant: opts.Variant, + opts: opts, licenseAPIReady: licenseAPIReady, } r.status.Run(opts.ShutdownContext) @@ -132,11 +129,8 @@ type ReconcileEgressGateway struct { // that reads objects from the cache and writes to the apiserver. client client.Client scheme *runtime.Scheme - provider operatorv1.Provider status status.StatusManager - clusterDomain string - useV3CRDs bool - variant operatorv1.ProductVariant + opts options.ControllerOptions licenseAPIReady *utils.ReadyFlag } @@ -156,7 +150,7 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil // Ahead of every early return below, because the last egress gateway going away is what // clears the policy sync path. - fc, err := sharedconfig.NewWriter(r.client, r.useV3CRDs).ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) + fc, err := sharedconfig.NewWriter(r.client, r.opts.UseV3CRDs).ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) if err != nil { reqLogger.Error(err, "Error patching felix configuration") r.status.SetDegraded(operatorv1.ResourcePatchError, "Error patching felix configuration", err, reqLogger) @@ -170,7 +164,7 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil ch := utils.NewComponentHandler(log, r.client, r.scheme, nil) if len(egws) == 0 { var objects []client.Object - if r.provider.IsOpenShift() { + if r.opts.DetectedProvider.IsOpenShift() { objects = append(objects, egressgateway.SecurityContextConstraints()) } err := ch.CreateOrUpdateOrDelete(ctx, render.NewDeletionPassthrough(objects...), r.status) @@ -209,7 +203,7 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil // In the case of OpenShift, we are using a single SCC. // Whenever a EGW resource is deleted, remove the corresponding user from the SCC // and update the resource. - if r.provider.IsOpenShift() { + if r.opts.DetectedProvider.IsOpenShift() { scc, err := getOpenShiftSCC(ctx, r.client) if err != nil { reqLogger.Error(err, "Error querying SecurityContextConstraints") @@ -305,7 +299,7 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil // Reconcile all the EGWs var errMsgs []string for _, egw := range egwsToReconcile { - err = r.reconcileEgressGateway(ctx, &egw, reqLogger, r.variant, fc, pullSecrets, installationSpec, namespaceAndNames) + err = r.reconcileEgressGateway(ctx, &egw, reqLogger, r.opts.Variant, fc, pullSecrets, installationSpec, namespaceAndNames) if err != nil { reqLogger.Error(err, "Error reconciling egress gateway") errMsgs = append(errMsgs, err.Error()) @@ -380,7 +374,7 @@ func (r *ReconcileEgressGateway) reconcileEgressGateway(ctx context.Context, egw VXLANPort: egwVXLANPort, VXLANVNI: egwVXLANVNI, IptablesBackend: ipTablesBackend, - OpenShift: r.provider.IsOpenShift(), + OpenShift: r.opts.DetectedProvider.IsOpenShift(), NamespaceAndNames: namespaceAndNames, } diff --git a/pkg/controller/egressgateway/egressgateway_controller_test.go b/pkg/controller/egressgateway/egressgateway_controller_test.go index e5b5231b79..e46e2e6c85 100644 --- a/pkg/controller/egressgateway/egressgateway_controller_test.go +++ b/pkg/controller/egressgateway/egressgateway_controller_test.go @@ -418,7 +418,7 @@ var _ = Describe("Egress Gateway controller tests", func() { mockStatus.On("ReadyToMonitor") Expect(c.Create(ctx, installation)).NotTo(HaveOccurred()) - r.provider = operatorv1.ProviderOpenShift + r.opts.DetectedProvider = operatorv1.ProviderOpenShift logSeverity := operatorv1.LogSeverityInfo egw_red := &operatorv1.EgressGateway{ ObjectMeta: metav1.ObjectMeta{Name: "calico-red", Namespace: "calico-egress"}, diff --git a/pkg/controller/istio/istio_controller.go b/pkg/controller/istio/istio_controller.go index 1acf942021..9d09f57880 100644 --- a/pkg/controller/istio/istio_controller.go +++ b/pkg/controller/istio/istio_controller.go @@ -115,11 +115,10 @@ func Add(mgr manager.Manager, opts options.ControllerOptions) error { // newReconciler returns a new reconcile.Reconciler func newReconciler(mgr manager.Manager, opts options.ControllerOptions) *ReconcileIstio { r := &ReconcileIstio{ - Client: mgr.GetClient(), - scheme: mgr.GetScheme(), - status: status.New(mgr.GetClient(), "istio", opts.KubernetesVersion), - provider: opts.DetectedProvider, - useV3CRDs: opts.UseV3CRDs, + Client: mgr.GetClient(), + scheme: mgr.GetScheme(), + status: status.New(mgr.GetClient(), "istio", opts.KubernetesVersion), + opts: opts, } r.status.Run(opts.ShutdownContext) @@ -129,10 +128,9 @@ func newReconciler(mgr manager.Manager, opts options.ControllerOptions) *Reconci // ReconcileIstio reconciles a Istio object type ReconcileIstio struct { client.Client - scheme *runtime.Scheme - status status.StatusManager - provider operatorv1.Provider - useV3CRDs bool + scheme *runtime.Scheme + status status.StatusManager + opts options.ControllerOptions } func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { @@ -258,7 +256,7 @@ func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Reques return reconcile.Result{}, err } - writer := sharedconfig.NewWriter(r.Client, r.useV3CRDs) + writer := sharedconfig.NewWriter(r.Client, r.opts.UseV3CRDs) if _, err = writer.ApplyFelixConfiguration(ctx, r.declareIstioFelixConfiguration(instance, false)); err != nil { r.status.SetDegraded(operatorv1.ResourceCreateError, "Error patching felix configuration with Istio settings", err, log) return reconcile.Result{}, err @@ -314,7 +312,7 @@ func (r *ReconcileIstio) declareIstioFelixConfiguration(instance *operatorv1.Ist func (r *ReconcileIstio) maintainFinalizer(ctx context.Context, instance *operatorv1.Istio, reqLogger logr.Logger) (res reconcile.Result, err error, finalized bool) { // Executing clean up on finalizing if !instance.DeletionTimestamp.IsZero() { - writer := sharedconfig.NewWriter(r.Client, r.useV3CRDs) + writer := sharedconfig.NewWriter(r.Client, r.opts.UseV3CRDs) if _, err = writer.ApplyFelixConfiguration(ctx, r.declareIstioFelixConfiguration(instance, true)); err != nil { r.status.SetDegraded(operatorv1.ResourceReadError, "Error cleaning up felix configuration", err, reqLogger) return diff --git a/pkg/controller/istio/istio_controller_test.go b/pkg/controller/istio/istio_controller_test.go index d3217ec7e6..e427fdfee0 100644 --- a/pkg/controller/istio/istio_controller_test.go +++ b/pkg/controller/istio/istio_controller_test.go @@ -41,6 +41,7 @@ import ( "github.com/tigera/operator/pkg/common" "github.com/tigera/operator/pkg/components" "github.com/tigera/operator/pkg/controller/certificatemanager" + "github.com/tigera/operator/pkg/controller/options" "github.com/tigera/operator/pkg/controller/status" ctrlrfake "github.com/tigera/operator/pkg/ctrlruntime/client/fake" "github.com/tigera/operator/pkg/render/istio" @@ -130,10 +131,10 @@ var _ = Describe("Istio controller tests", func() { createResources() r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -149,10 +150,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, istioCR)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -165,10 +166,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, installation)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -184,10 +185,10 @@ var _ = Describe("Istio controller tests", func() { It("should handle basic Istio spec configuration", func() { r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -214,10 +215,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Update(ctx, istioCR)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -240,10 +241,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Update(ctx, istioCR)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -260,10 +261,10 @@ var _ = Describe("Istio controller tests", func() { It("should update status when reconciliation is successful", func() { r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -275,10 +276,10 @@ var _ = Describe("Istio controller tests", func() { It("should handle reconciliation without errors", func() { r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -293,10 +294,10 @@ var _ = Describe("Istio controller tests", func() { createResources() r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: provider, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: provider}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -326,10 +327,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } // First reconcile to add finalizer @@ -369,10 +370,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, istioNoDSCP)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -408,10 +409,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, istioCustomDSCP)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -448,10 +449,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -471,10 +472,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -500,7 +501,7 @@ var _ = Describe("Istio controller tests", func() { fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}} Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) - r := &ReconcileIstio{Client: cli, scheme: scheme, provider: operatorv1.ProviderNone, status: mockStatus} + r := &ReconcileIstio{Client: cli, scheme: scheme, opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, status: mockStatus} _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).ShouldNot(HaveOccurred()) @@ -516,7 +517,7 @@ var _ = Describe("Istio controller tests", func() { } Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) - r := &ReconcileIstio{Client: cli, scheme: scheme, provider: operatorv1.ProviderNone, status: mockStatus} + r := &ReconcileIstio{Client: cli, scheme: scheme, opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, status: mockStatus} _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).ShouldNot(HaveOccurred()) @@ -541,7 +542,7 @@ var _ = Describe("Istio controller tests", func() { fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}} Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) - r := &ReconcileIstio{Client: cli, scheme: scheme, provider: operatorv1.ProviderNone, status: mockStatus} + r := &ReconcileIstio{Client: cli, scheme: scheme, opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, status: mockStatus} _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).ShouldNot(HaveOccurred()) @@ -570,7 +571,7 @@ var _ = Describe("Istio controller tests", func() { fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}} Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) - r := &ReconcileIstio{Client: cli, scheme: scheme, provider: operatorv1.ProviderNone, status: mockStatus} + r := &ReconcileIstio{Client: cli, scheme: scheme, opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, status: mockStatus} _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).ShouldNot(HaveOccurred()) @@ -590,7 +591,7 @@ var _ = Describe("Istio controller tests", func() { fc := &v3.FelixConfiguration{ObjectMeta: metav1.ObjectMeta{Name: "default"}} Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) - r := &ReconcileIstio{Client: cli, scheme: scheme, provider: operatorv1.ProviderNone, status: mockStatus} + r := &ReconcileIstio{Client: cli, scheme: scheme, opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, status: mockStatus} _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) Expect(err).ShouldNot(HaveOccurred()) @@ -617,10 +618,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, fc)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } // First reconcile to add finalizer and set FelixConfiguration values @@ -675,10 +676,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, ts)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: IstioName}}) @@ -713,10 +714,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, istioCR)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -746,10 +747,10 @@ var _ = Describe("Istio controller tests", func() { It("should create expected Istio resources", func() { r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -789,10 +790,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, imageSet)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -826,10 +827,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Update(ctx, installation)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) @@ -875,10 +876,10 @@ var _ = Describe("Istio controller tests", func() { Expect(cli.Create(ctx, imageSet)).NotTo(HaveOccurred()) r := &ReconcileIstio{ - Client: cli, - scheme: scheme, - provider: operatorv1.ProviderNone, - status: mockStatus, + Client: cli, + scheme: scheme, + opts: options.ControllerOptions{DetectedProvider: operatorv1.ProviderNone}, + status: mockStatus, } _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: "default"}}) From e5c9aad716add0372dc06bab393452185d20f75e Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Thu, 20 Aug 2026 11:49:18 -0400 Subject: [PATCH 10/10] Declare tproxy mode instead of updating it Drops the guard for Felix versions that restarted on an unknown config field; that was fixed in v3.16, v3.15.1 and v3.14.4. --- .../applicationlayer_controller.go | 44 +++++++------------ .../applicationlayer_controller_test.go | 14 +++--- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/pkg/controller/applicationlayer/applicationlayer_controller.go b/pkg/controller/applicationlayer/applicationlayer_controller.go index 7aa95fa2da..2939f8890a 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller.go @@ -508,52 +508,40 @@ func (r *ReconcileApplicationLayer) getTProxyMode(al *operatorv1.ApplicationLaye // applicationLayerFieldManager owns the FelixConfiguration fields the application layer sets. const applicationLayerFieldManager = "application-layer" -// declareWAFEventLogsFile declares the WAF event log toggle, driven by the ApplicationLayer WAF -// and the gateway data plane. -func declareWAFEventLogsFile(al *operatorv1.ApplicationLayer, gatewayWAFEnabled bool) sharedconfig.DeclareFelixConfiguration { +// declareApplicationLayerFields declares the fields the application layer drives: the WAF event log +// toggle it shares with the gateway data plane, and Felix's tproxy mode. +func (r *ReconcileApplicationLayer) declareApplicationLayerFields(al *operatorv1.ApplicationLayer, gatewayWAFEnabled bool) sharedconfig.DeclareFelixConfiguration { return func(_ *v3.FelixConfiguration) (*sharedconfig.FelixConfigurationDeclaration, error) { d := &sharedconfig.FelixConfigurationDeclaration{ Manager: applicationLayerFieldManager, Owned: &v3.FelixConfiguration{}, Policies: map[string]sharedconfig.ConflictPolicy{ "spec.wafEventLogsFileEnabled": sharedconfig.ConflictOverride, + "spec.tproxyMode": sharedconfig.ConflictOverride, }, } - // Declared without a value when nothing needs it, rather than written as false: an - // upgrade from before the field existed restarts every node over a value Felix cannot read. + // Both fields are declared without a value when nothing asks for them, which clears them + // rather than pinning Felix to the disabled setting. if enabled := wafEventLogsFileRequired(al, gatewayWAFEnabled); enabled { d.Owned.Spec.WAFEventLogsFileEnabled = &enabled } + if ok, mode := r.getTProxyMode(al); ok { + d.Owned.Spec.TPROXYMode = mode + } return d, nil } } -// patchFelixConfiguration writes the fields the application layer drives. TPROXYMode stays on the -// update path for the upgrade workaround below. +// patchFelixConfiguration writes the fields the application layer drives. func (r *ReconcileApplicationLayer) patchFelixConfiguration(ctx context.Context, al *operatorv1.ApplicationLayer, gatewayWAFEnabled bool) error { writer := sharedconfig.NewWriter(r.client, r.useV3CRDs) - if _, err := writer.ApplyFelixConfiguration(ctx, declareWAFEventLogsFile(al, gatewayWAFEnabled)); err != nil { - return err - } - if _, err := writer.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)); err != nil { + if _, err := writer.ApplyFelixConfiguration(ctx, r.declareApplicationLayerFields(al, gatewayWAFEnabled)); err != nil { return err } - - _, err := writer.UpdateFelixConfiguration(ctx, func(fc *v3.FelixConfiguration) (bool, error) { - ok, tproxyMode := r.getTProxyMode(al) - if !ok && fc.Spec.TPROXYMode == "" { - // Setting this during an upgrade from before the field existed makes Felix restart, - // so rely on the default. - return false, nil - } - if fc.Spec.TPROXYMode == tproxyMode { - return false, nil - } - fc.Spec.TPROXYMode = tproxyMode - log.Info("Patching FelixConfiguration: ", "tproxyMode", tproxyMode) - return true, nil - }) + // TODO(CORE-13394): drop the client here by having each feature apply the path under its own + // field manager, so no declaration has to read the other features' resources. + _, err := writer.ApplyFelixConfiguration(ctx, sharedconfig.DeclarePolicySyncPathPrefix(ctx, r.client)) return err } @@ -566,8 +554,8 @@ func wafEventLogsFileRequired(al *operatorv1.ApplicationLayer, gatewayWAFEnabled } // isGatewayWAFEnabled reports whether the GatewayAPI WAF data-plane extension is enabled. A missing -// GatewayAPI CR is treated as disabled (no error); any other read error is returned so the caller can -// requeue rather than spuriously treating WAF as disabled and flapping FelixConfiguration. +// GatewayAPI CR reads as disabled; any other read error goes back to the caller, which requeues +// rather than flapping FelixConfiguration. func (r *ReconcileApplicationLayer) isGatewayWAFEnabled(ctx context.Context) (bool, error) { gw, msg, err := gatewayapi.GetGatewayAPI(ctx, r.client) if err != nil { diff --git a/pkg/controller/applicationlayer/applicationlayer_controller_test.go b/pkg/controller/applicationlayer/applicationlayer_controller_test.go index cd9e1ea704..657e7aa6ba 100644 --- a/pkg/controller/applicationlayer/applicationlayer_controller_test.go +++ b/pkg/controller/applicationlayer/applicationlayer_controller_test.go @@ -255,9 +255,8 @@ var _ = Describe("Application layer controller tests", func() { }) It("should leave TPROXYMode unset if log collection is disabled", func() { - // This test verifies a workaround for upgrade from versions that don't support TPROXY to versions - // that do. Setting an unknown felix config field causes older versions of felix to cyclicly restart, - // which causes a disruptive upgrade. + // With no ApplicationLayer resource, the field is declared without a value, so Felix + // falls back to its own default rather than reading one the operator picked. By("reconciling before without an app layer resource") mockStatus.On("OnCRNotFound").Return() _, err := r.Reconcile(ctx, reconcile.Request{}) @@ -274,9 +273,8 @@ var _ = Describe("Application layer controller tests", func() { }) It("should enable WAFEventLogsFileEnabled when the GatewayAPI WAF extension is enabled (no ApplicationLayer CR)", func() { - // The gateway data-plane WAF (design-25) emits audit events that flow through Felix's WAF event - // log, so it requires the same FelixConfiguration toggle as the legacy ApplicationLayer WAF — even - // when no ApplicationLayer CR is present. + // The gateway data-plane WAF emits audit events through Felix's WAF event log, so it needs + // the same toggle as the legacy ApplicationLayer WAF, with no ApplicationLayer CR present. mockStatus.On("OnCRNotFound").Return() By("creating a GatewayAPI CR with the WAF extension enabled") @@ -367,14 +365,14 @@ var _ = Describe("Application layer controller tests", func() { _, err = r.Reconcile(ctx, reconcile.Request{}) Expect(err).ShouldNot(HaveOccurred()) - By("ensuring that felix configuration updated to disabled") + By("ensuring that felix configuration cleared the mode") fc = v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "default", }, } Expect(test.GetResource(c, &fc)).To(BeNil()) - Expect(fc.Spec.TPROXYMode).To(Equal("Disabled")) + Expect(fc.Spec.TPROXYMode).To(Equal("")) }) It("should render proper SidecarWebhook status", func() {