Clear programClusterRoutes when clusterRoutingMode is removed

operator/pkg/controller/installation/core_controller.go

@@ -887,6 +887,9 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
 		return reconcile.Result{}, err
 	}
 
+	// The mode we last applied.  Read before the status write below overwrites it.
+	appliedClusterRoutingMode := clusterRoutingModeFromStatus(instance)
+
 	// Publish the effective config before anything below can return early, since every other
 	// controller reads it instead of the spec and stalls until it lands.
 	computed := defaulted.Spec.DeepCopy()
@@ -1068,7 +1071,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
 		}
 
 		// Configure cluster routing mode.
-		u3, err := setClusterRoutingOnFelixConfiguration(defaulted, fc, reqLogger)
+		u3, err := setClusterRoutingOnFelixConfiguration(defaulted, appliedClusterRoutingMode, fc, reqLogger)
 		if err != nil {
 			return false, err
 		}
@@ -1089,7 +1092,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
 	// Set any non-default BGPConfiguration values that we need.
 	_, err = utils.PatchBGPConfiguration(ctx, r.client, func(bgpConfig *v3.BGPConfiguration) (bool, error) {
 		// Configure cluster routing mode.
-		u, err := setClusterRoutingOnBGPConfiguration(defaulted, bgpConfig, reqLogger)
+		u, err := setClusterRoutingOnBGPConfiguration(defaulted, appliedClusterRoutingMode, bgpConfig, reqLogger)
 		if err != nil {
 			return false, err
 		}
@@ -1780,15 +1783,34 @@ func allNodesRunTargetVersion(install *operatorv1.Installation, needNsMigration,
 	return install.Status.Variant == install.Spec.Variant && install.Status.CalicoVersion == targetVersion
 }
 
+// clearClusterRoutes takes back a value we wrote, so Calico's defaults decide again.  One we never
+// wrote is the user's.
+func clearClusterRoutes(
+	applied *operatorv1.ClusterRoutingMode,
+	programClusterRoutes **string,
+	resource string,
+	reqLogger logr.Logger,
+) bool {
+	if applied == nil || *programClusterRoutes == nil {
+		return false
+	}
+
+	reqLogger.Info("Clearing programClusterRoutes, clusterRoutingMode is no longer set",
+		"resource", resource, "was", **programClusterRoutes)
+	*programClusterRoutes = nil
+	return true
+}
+
 // setClusterRoutingOnFelixConfiguration sets programClusterRoutes in the FelixConfiguration resource
 // based on the value of clusterRoutingMode in the install config.
 func setClusterRoutingOnFelixConfiguration(
 	install *operatorv1.Installation,
+	applied *operatorv1.ClusterRoutingMode,
 	fc *v3.FelixConfiguration,
 	reqLogger logr.Logger,
 ) (bool, error) {
 	if install.Spec.CalicoNetwork == nil || install.Spec.CalicoNetwork.ClusterRoutingMode == nil {
-		return false, nil
+		return clearClusterRoutes(applied, &fc.Spec.ProgramClusterRoutes, "FelixConfiguration", reqLogger), nil
 	}
 
 	updated := false
@@ -1807,11 +1829,12 @@ func setClusterRoutingOnFelixConfiguration(
 // based on the value of clusterRoutingMode in the install config.
 func setClusterRoutingOnBGPConfiguration(
 	install *operatorv1.Installation,
+	applied *operatorv1.ClusterRoutingMode,
 	bgpConfig *v3.BGPConfiguration,
 	reqLogger logr.Logger,
 ) (bool, error) {
 	if install.Spec.CalicoNetwork == nil || install.Spec.CalicoNetwork.ClusterRoutingMode == nil {
-		return false, nil
+		return clearClusterRoutes(applied, &bgpConfig.Spec.ProgramClusterRoutes, "BGPConfiguration", reqLogger), nil
 	}
 
 	updated := false
@@ -1887,6 +1910,15 @@ func clusterRoutingMode(install *operatorv1.Installation) operatorv1.ClusterRout
 	return *install.Spec.CalicoNetwork.ClusterRoutingMode
 }
 
+// clusterRoutingModeFromStatus returns the mode we last applied: our only record that
+// programClusterRoutes is ours, not the user's.
+func clusterRoutingModeFromStatus(install *operatorv1.Installation) *operatorv1.ClusterRoutingMode {
+	if install.Status.Computed == nil || install.Status.Computed.CalicoNetwork == nil {
+		return nil
+	}
+	return install.Status.Computed.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) {

operator/pkg/controller/installation/core_controller_test.go

@@ -1365,6 +1365,56 @@ var _ = Describe("Testing core-controller installation", func() {
 			Expect(*bgpConfig.Spec.ProgramClusterRoutes).To(Equal("EnabledNoEncapOnly"))
 		})
 
+		It("should clear programClusterRoutes when ClusterRoutingMode is removed", func() {
+			bird := operator.ClusterRoutingModeBIRD
+			cr.Spec.CalicoNetwork = &operator.CalicoNetworkSpec{ClusterRoutingMode: &bird}
+			Expect(c.Create(ctx, cr)).NotTo(HaveOccurred())
+			_, err := r.Reconcile(ctx, reconcile.Request{})
+			Expect(err).ShouldNot(HaveOccurred())
+
+			fc := &v3.FelixConfiguration{}
+			Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, fc)).NotTo(HaveOccurred())
+			Expect(fc.Spec.ProgramClusterRoutes).NotTo(BeNil())
+
+			// Remove the mode again; the operator has to take its own writes back out.
+			Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, cr)).NotTo(HaveOccurred())
+			cr.Spec.CalicoNetwork.ClusterRoutingMode = nil
+			Expect(c.Update(ctx, cr)).NotTo(HaveOccurred())
+			_, err = r.Reconcile(ctx, reconcile.Request{})
+			Expect(err).ShouldNot(HaveOccurred())
+
+			fc = &v3.FelixConfiguration{}
+			Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, fc)).NotTo(HaveOccurred())
+			Expect(fc.Spec.ProgramClusterRoutes).To(BeNil())
+
+			bgpConfig := &v3.BGPConfiguration{}
+			Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig)).NotTo(HaveOccurred())
+			Expect(bgpConfig.Spec.ProgramClusterRoutes).To(BeNil())
+		})
+
+		It("should leave a user's own programClusterRoutes alone when ClusterRoutingMode was never set", func() {
+			cr.Spec.CalicoNetwork = &operator.CalicoNetworkSpec{}
+			Expect(c.Create(ctx, cr)).NotTo(HaveOccurred())
+			_, err := r.Reconcile(ctx, reconcile.Request{})
+			Expect(err).ShouldNot(HaveOccurred())
+
+			// The user configures Felix directly, which is the supported route while the
+			// Installation field is unset.
+			fc := &v3.FelixConfiguration{}
+			Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, fc)).NotTo(HaveOccurred())
+			userValue := "Enabled"
+			fc.Spec.ProgramClusterRoutes = &userValue
+			Expect(c.Update(ctx, fc)).NotTo(HaveOccurred())
+
+			_, err = r.Reconcile(ctx, reconcile.Request{})
+			Expect(err).ShouldNot(HaveOccurred())
+
+			fc = &v3.FelixConfiguration{}
+			Expect(c.Get(ctx, types.NamespacedName{Name: "default"}, fc)).NotTo(HaveOccurred())
+			Expect(fc.Spec.ProgramClusterRoutes).NotTo(BeNil())
+			Expect(*fc.Spec.ProgramClusterRoutes).To(Equal(userValue))
+		})
+
 		It("should create the default BGPConfig and FelixConfig with ClusterRoutingMode set", func() {
 			bgpConfig := &v3.BGPConfiguration{}
 			err := c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig)