diff --git a/pkg/controller/applicationlayer/applicationlayer_controller.go b/pkg/controller/applicationlayer/applicationlayer_controller.go index d8f0aa103d..2939f8890a 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" @@ -80,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, } @@ -166,6 +168,7 @@ type ReconcileApplicationLayer struct { provider operatorv1.Provider status status.StatusManager clusterDomain string + useV3CRDs bool variant operatorv1.ProductVariant licenseAPIReady *utils.ReadyFlag } @@ -486,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" @@ -507,75 +505,43 @@ 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. -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 { - return err - } - istioNeeds := utils.IstioRequiresPolicySync(istioCR, r.variant) - - _, err = utils.PatchFelixConfiguration(ctx, r.client, 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. - tproxyMode = "Disabled" +// applicationLayerFieldManager owns the FelixConfiguration fields the application layer sets. +const applicationLayerFieldManager = "application-layer" + +// 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, + }, } - - 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 { - return false, nil + // 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 + } +} - 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, - ) - return true, nil - }) +// 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, r.declareApplicationLayerFields(al, gatewayWAFEnabled)); err != nil { + return err + } + // 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 } @@ -588,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 cd04f13341..657e7aa6ba 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 a user's own PolicySyncPathPrefix survives ALP deletion") f2 := v3.FelixConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -257,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{}) @@ -276,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") @@ -369,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() { diff --git a/pkg/controller/egressgateway/egressgateway_controller.go b/pkg/controller/egressgateway/egressgateway_controller.go index 745083fe14..bee09abf06 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" @@ -81,10 +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, - variant: opts.Variant, + opts: opts, licenseAPIReady: licenseAPIReady, } r.status.Run(opts.ShutdownContext) @@ -130,10 +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 - variant operatorv1.ProductVariant + opts options.ControllerOptions licenseAPIReady *utils.ReadyFlag } @@ -151,11 +148,23 @@ 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.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) + 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 { 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) @@ -194,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") @@ -287,27 +296,10 @@ func (r *ReconcileEgressGateway) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } - // patch and get the felix configuration - fc, err := utils.PatchFelixConfiguration(ctx, r.client, 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 - }) - 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 { - 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()) @@ -382,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/gatewayapi/gatewayapi_controller.go b/pkg/controller/gatewayapi/gatewayapi_controller.go index 07ad0001cc..7fa3d340ca 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" @@ -58,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 @@ -76,6 +73,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, @@ -180,6 +178,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 @@ -617,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 := utils.PatchFelixConfiguration(ctx, r.client, 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/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 105165e93c..1faff533ef 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,31 +1032,24 @@ 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) { - // 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) + 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 } + 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 + } + + // 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) { @@ -1204,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)) @@ -1329,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(), @@ -1443,10 +1437,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 = utils.PatchFelixConfiguration(ctx, r.client, 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 @@ -1655,196 +1647,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( @@ -1929,42 +1731,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..0cfa3aa7ac --- /dev/null +++ b/pkg/controller/installation/felixconfig.go @@ -0,0 +1,184 @@ +// 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. 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{ + "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 + + // 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} + case operatorv1.PluginGKE: + owned.RouteTableRange = &v3.RouteTableRange{Min: 10, Max: 250} + } + + owned.HealthPort = ptr.To(defaultFelixHealthPort(install)) + + 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) + } + } + owned.VXLANVNI = &vxlanVNI + owned.VXLANPort = &vxlanPort + + 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) + } + + if install.Spec.CalicoNetwork != nil && install.Spec.CalicoNetwork.LinuxDataplane != nil { + owned.NFTablesMode = ptr.To(nftablesMode(install)) + } + + // 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)) + } + + 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 + } +} + +// 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 { + 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..fab2d1ca78 --- /dev/null +++ b/pkg/controller/installation/felixconfig_test.go @@ -0,0 +1,153 @@ +// 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 + } + + 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() { + 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{ + HealthPort: ptr.To(1234), + VXLANVNI: ptr.To(9999), + VXLANPort: ptr.To(1111), + NFTablesMode: ptr.To(v3.NFTablesModeDisabled), + }}) + 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() { + 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/istio/istio_controller.go b/pkg/controller/istio/istio_controller.go index ea5d55458e..9d09f57880 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" @@ -36,11 +35,11 @@ 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" "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" @@ -116,10 +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, + Client: mgr.GetClient(), + scheme: mgr.GetScheme(), + status: status.New(mgr.GetClient(), "istio", opts.KubernetesVersion), + opts: opts, } r.status.Run(opts.ShutdownContext) @@ -129,9 +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 + scheme *runtime.Scheme + status status.StatusManager + opts options.ControllerOptions } func (r *ReconcileIstio) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { @@ -257,13 +256,15 @@ 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) { - return r.setIstioFelixConfiguration(ctx, instance, fc, false) - }) - if err != nil { + 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 } + 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() @@ -278,146 +279,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 = utils.PatchFelixConfiguration(ctx, r.Client, func(fc *v3.FelixConfiguration) (bool, error) { - return r.setIstioFelixConfiguration(ctx, instance, fc, true) - }); err != nil { + 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 + } + 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..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"}}) @@ -389,12 +390,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() { @@ -412,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"}}) @@ -432,12 +429,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 +440,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")), }, @@ -462,49 +449,22 @@ 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"}}) 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, }, @@ -512,15 +472,15 @@ 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"}}) 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() { @@ -541,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()) @@ -557,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()) @@ -582,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()) @@ -598,7 +558,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"}, @@ -611,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()) @@ -623,17 +583,15 @@ 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()) - 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()) @@ -645,10 +603,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()) }) }) @@ -662,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 @@ -679,10 +635,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 +652,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)) }) }) @@ -726,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}}) @@ -764,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"}}) @@ -797,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"}}) @@ -840,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"}}) @@ -877,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"}}) @@ -926,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"}}) diff --git a/pkg/controller/sharedconfig/apply_test.go b/pkg/controller/sharedconfig/apply_test.go new file mode 100644 index 0000000000..c8b5410030 --- /dev/null +++ b/pkg/controller/sharedconfig/apply_test.go @@ -0,0 +1,487 @@ +// 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 + } +} + +// 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 + + 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.Apply(ctx, client.ApplyConfigurationFromUnstructured(other), 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 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()) + + _, 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("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 + } + } + + // 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}, + Spec: spec, + }, 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() { + 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", matching the value the user 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.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{})) + 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(entry.FieldsV1.GetRawString()).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)}) + + 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))) + }) + + 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))) + }) + }) + }) + + 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("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) { + 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 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", + 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).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() { + _, 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 new file mode 100644 index 0000000000..ad7073d40a --- /dev/null +++ b/pkg/controller/sharedconfig/crdv1.go @@ -0,0 +1,263 @@ +// 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" + "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 +} + +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 + } + 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 { + return nil, err + } + if declaration == nil { + return current, nil + } + + payload, err := declaredPayload(declaration.Owned) + if err != nil { + return nil, err + } + // 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 + } + + merged := current.DeepCopy() + if err := mergeInto(merged, payload); err != nil { + return nil, err + } + removed, err := removeUndeclared(merged, current, declaration, payload, legacyOwned) + 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) { + 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, 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) + } + 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 + } + // 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, legacyOwned, 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 +} + +// 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, 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) + } + 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 && !legacyOwned[path] { + // The operator has no sign of writing this, so it belongs to someone else. + continue + } + changed, err := changedByOther(currentContent, lastWritten, legacyOwned, 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 + 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{} + 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) + } + + 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 { + 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..b2e0958eb5 --- /dev/null +++ b/pkg/controller/sharedconfig/crdv1_test.go @@ -0,0 +1,151 @@ +// 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, false) + }) + + 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()) + }) + + 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/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/migrate.go b/pkg/controller/sharedconfig/migrate.go new file mode 100644 index 0000000000..a76e57d989 --- /dev/null +++ b/pkg/controller/sharedconfig/migrate.go @@ -0,0 +1,112 @@ +// 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" +) + +// 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, manager string) (map[string]bool, error) { + reclaimable, others, err := updateOwnedPaths(fc) + if err != nil || len(others) == 0 || appliedBy(fc, manager) { + 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 reclaimable, err + } + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(fc) + if err != nil { + return nil, fmt.Errorf("unable to read FelixConfiguration fields: %w", err) + } + for path := range lastWritten { + if !others[path] { + continue + } + // 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 + } + if !changed { + reclaimable[path] = true + } + } + 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) { + 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.GetRawBytes(), &fields); err != nil { + 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, "", out) + } + return legacy, others, 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/payload.go b/pkg/controller/sharedconfig/payload.go new file mode 100644 index 0000000000..55646b5190 --- /dev/null +++ b/pkg/controller/sharedconfig/payload.go @@ -0,0 +1,80 @@ +// 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") + 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, ".")...) + 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/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/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/tracking.go b/pkg/controller/sharedconfig/tracking.go new file mode 100644 index 0000000000..c931973c62 --- /dev/null +++ b/pkg/controller/sharedconfig/tracking.go @@ -0,0 +1,175 @@ +// 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. 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) + } + if !found { + return false, nil + } + + written, recorded := lastWritten[path] + if !recorded { + return !legacyOwned[path], nil + } + canonical, err := canonicalize(current) + if err != nil { + return false, err + } + 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) + 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{} + } + // 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) +} + +// 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..22f8e34e8e --- /dev/null +++ b/pkg/controller/sharedconfig/v3.go @@ -0,0 +1,191 @@ +// 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" + "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" + + "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 + } + 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 { + return applied, nil + } + if !apierrors.IsConflict(err) { + return nil, err + } + + force, err := w.resolveConflicts(err, current, 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, current *v3.FelixConfiguration, d *FelixConfigurationDeclaration, payload *unstructured.Unstructured) (bool, error) { + paths := conflictPaths(applyErr) + if len(paths) == 0 { + return false, applyErr + } + + 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 + } + + force := false + var undeclared, refused []string + for _, path := range paths { + declared, policy, ok := d.policyFor(path) + if !ok { + 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 + 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 +} + +// 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 { + 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.Apply(ctx, client.ApplyConfigurationFromUnstructured(applied), 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 new file mode 100644 index 0000000000..65d8c05cd5 --- /dev/null +++ b/pkg/controller/sharedconfig/writer.go @@ -0,0 +1,44 @@ +// 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" +) + +// 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, useV3CRDs bool) Writer { + if useV3CRDs { + return &v3Writer{crdV1Writer{client: c}} + } + 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) 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{} 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 { 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