Merge pull request #592 from lmiccini/bgp-default-openshift-frr-k8s-namespace

Change BGPConfiguration default namespace to openshift-frr-k8s and add legacy cleanup

Author: openshift-merge-bot[bot]

apis/bases/network.openstack.org_bgpconfigurations.yaml

@@ -44,9 +44,9 @@ spec:
             description: BGPConfigurationSpec defines the desired state of BGPConfiguration
             properties:
               frrConfigurationNamespace:
-                default: metallb-system
+                default: openshift-frr-k8s
                 description: FRRConfigurationNamespace - namespace where to create
-                  the FRRConfiguration. Defaults to metallb-system.
+                  the FRRConfiguration. Defaults to openshift-frr-k8s.
                 type: string
               frrNodeConfigurationSelector:
                 description: |-

apis/network/v1beta1/bgpconfiguration_types.go

@@ -35,8 +35,8 @@ type FRRNodeConfigurationSelectorType struct {
 // BGPConfigurationSpec defines the desired state of BGPConfiguration
 type BGPConfigurationSpec struct {
 	// +kubebuilder:validation:Optional
-	// +kubebuilder:default="metallb-system"
-	// FRRConfigurationNamespace - namespace where to create the FRRConfiguration. Defaults to metallb-system.
+	// +kubebuilder:default="openshift-frr-k8s"
+	// FRRConfigurationNamespace - namespace where to create the FRRConfiguration. Defaults to openshift-frr-k8s.
 	FRRConfigurationNamespace string `json:"frrConfigurationNamespace"`
 
 	// +kubebuilder:validation:Optional

config/crd/bases/network.openstack.org_bgpconfigurations.yaml

@@ -44,9 +44,9 @@ spec:
             description: BGPConfigurationSpec defines the desired state of BGPConfiguration
             properties:
               frrConfigurationNamespace:
-                default: metallb-system
+                default: openshift-frr-k8s
                 description: FRRConfigurationNamespace - namespace where to create
-                  the FRRConfiguration. Defaults to metallb-system.
+                  the FRRConfiguration. Defaults to openshift-frr-k8s.
                 type: string
               frrNodeConfigurationSelector:
                 description: |-

internal/bgp/funcs.go

@@ -75,6 +75,9 @@ func GetNodesRunningPods(podNetworkDetailList []PodDetail) []string {
 // OpenShiftFRRNamespace is the FRR namespace used in OpenShift 4.20+
 const OpenShiftFRRNamespace = "openshift-frr-k8s"
 
+// LegacyMetalLBNamespace is the FRR namespace used before OpenShift 4.20
+const LegacyMetalLBNamespace = "metallb-system"
+
 // GetFRRConfigurationNamespace checks whether migrationNamespace exists and returns it,
 // otherwise falls back to defaultNamespace.
 func GetFRRConfigurationNamespace(ctx context.Context, c client.Client, migrationNamespace, defaultNamespace string) (string, error) {

internal/controller/network/bgpconfiguration_controller.go

@@ -56,9 +56,10 @@ import (
 // BGPConfigurationReconciler reconciles a BGPConfiguration object
 type BGPConfigurationReconciler struct {
 	client.Client
-	Kclient               kubernetes.Interface
-	Scheme                *runtime.Scheme
-	FRRMigrationNamespace string
+	Kclient                kubernetes.Interface
+	Scheme                 *runtime.Scheme
+	FRRMigrationNamespace  string
+	LegacyMetalLBNamespace string
 }
 
 // GetLogger returns a logger object with a prefix of "controller.name" and additional controller context fields
@@ -73,6 +74,13 @@ func (r *BGPConfigurationReconciler) getFRRMigrationNamespace() string {
 	return bgp.OpenShiftFRRNamespace
 }
 
+func (r *BGPConfigurationReconciler) getLegacyMetalLBNamespace() string {
+	if r.LegacyMetalLBNamespace != "" {
+		return r.LegacyMetalLBNamespace
+	}
+	return bgp.LegacyMetalLBNamespace
+}
+
 //+kubebuilder:rbac:groups=network.openstack.org,resources=bgpconfigurations,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=network.openstack.org,resources=bgpconfigurations/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=network.openstack.org,resources=bgpconfigurations/finalizers,verbs=update
@@ -388,6 +396,13 @@ func (r *BGPConfigurationReconciler) reconcileDelete(ctx context.Context, instan
 		}
 	}
 
+	legacyNS := r.getLegacyMetalLBNamespace()
+	if frrNamespace != legacyNS {
+		if err := r.cleanupOldNamespaceFRRConfigurations(ctx, instance, legacyNS); err != nil {
+			return ctrl.Result{}, fmt.Errorf("error cleaning up FRRConfigurations from legacy namespace %s: %w", legacyNS, err)
+		}
+	}
+
 	// Service is deleted so remove the finalizer.
 	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
 	Log.Info("Reconciled Service delete successfully")
@@ -508,6 +523,14 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan
 		}
 	}
 
+	// Clean up FRRConfigurations from the legacy metallb-system namespace (pre-OpenShift 4.20)
+	legacyNS := r.getLegacyMetalLBNamespace()
+	if frrNamespace != legacyNS {
+		if err := r.cleanupOldNamespaceFRRConfigurations(ctx, instance, legacyNS); err != nil {
+			Log.Error(err, "Failed to cleanup legacy FRRConfigurations from namespace", "namespace", legacyNS)
+		}
+	}
+
 	Log.Info("Reconciled Service successfully")
 	return ctrl.Result{}, nil
 }

test/functional/bgpconfiguration_controller_test.go

@@ -33,7 +33,7 @@ import (
 var _ = Describe("BGPConfiguration controller", func() {
 	var bgpcfgName types.NamespacedName
 	var meallbFRRCfgName types.NamespacedName
-	frrCfgNamespace := "metallb-system"
+	frrCfgNamespace := "openshift-frr-k8s"
 
 	When("a default BGPConfiguration gets created", func() {
 		BeforeEach(func() {
@@ -720,4 +720,92 @@ var _ = Describe("BGPConfiguration controller", func() {
 			}, timeout, interval).Should(Succeed())
 		})
 	})
+
+	When("legacy FRRConfigurations in metallb-system are cleaned up after default namespace change", func() {
+		var podFrrName types.NamespacedName
+		var podName types.NamespacedName
+		var legacyNS *corev1.Namespace
+
+		BeforeEach(func() {
+			// Create the new default namespace (openshift-frr-k8s)
+			defaultNS := th.CreateNamespace(frrCfgNamespace + "-" + namespace)
+
+			defaultFRRCfgName := types.NamespacedName{Namespace: defaultNS.Name, Name: "worker-0"}
+			defaultFRRCfg := CreateFRRConfiguration(defaultFRRCfgName, GetMetalLBFRRConfigurationSpec("worker-0"))
+			Expect(defaultFRRCfg).To(Not(BeNil()))
+
+			// Create the legacy metallb-system namespace with an owned FRRConfiguration
+			legacyNS = th.CreateNamespace("metallb-system-" + namespace)
+			bgpReconciler.LegacyMetalLBNamespace = legacyNS.Name
+			DeferCleanup(func() {
+				bgpReconciler.LegacyMetalLBNamespace = ""
+			})
+
+			nad := th.CreateNAD(types.NamespacedName{Namespace: namespace, Name: "internalapi"}, GetNADSpec())
+
+			bgpcfg := CreateBGPConfiguration(namespace, GetBGPConfigurationSpec(defaultNS.Name))
+			bgpcfgName.Name = bgpcfg.GetName()
+			bgpcfgName.Namespace = bgpcfg.GetNamespace()
+
+			podName = types.NamespacedName{Namespace: namespace, Name: uuid.New().String()}
+			th.CreatePod(podName, GetPodAnnotation(namespace), GetPodSpec("worker-0"))
+			th.SimulatePodPhaseRunning(podName)
+
+			podFrrName.Name = podName.Namespace + "-" + podName.Name
+			podFrrName.Namespace = defaultNS.Name
+
+			DeferCleanup(th.DeleteInstance, bgpcfg)
+			DeferCleanup(th.DeleteInstance, nad)
+			DeferCleanup(th.DeleteInstance, defaultFRRCfg)
+		})
+
+		It("should clean up owned FRRConfigurations from the legacy metallb-system namespace", func() {
+			// Wait for the FRRConfiguration to be created in the new default namespace
+			Eventually(func(g Gomega) {
+				frr := GetFRRConfiguration(types.NamespacedName{Namespace: podFrrName.Namespace, Name: podFrrName.Name})
+				g.Expect(frr).To(Not(BeNil()))
+				g.Expect(frr.Spec.BGP.Routers[0].Prefixes[0]).To(Equal("172.17.0.40/32"))
+			}, timeout, interval).Should(Succeed())
+
+			// Create an owned FRRConfiguration in the legacy namespace to simulate a pre-upgrade state
+			legacyFRR := CreateFRRConfiguration(
+				types.NamespacedName{Namespace: legacyNS.Name, Name: podFrrName.Name},
+				GetMetalLBFRRConfigurationSpec("worker-0"),
+			)
+			Expect(legacyFRR).To(Not(BeNil()))
+
+			// Add owner labels matching the BGPConfiguration so cleanup will target it
+			frr := &frrk8sv1.FRRConfiguration{}
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: legacyNS.Name, Name: podFrrName.Name}, frr)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			bgpcfg := GetBGPConfiguration(bgpcfgName)
+			frr.Labels = map[string]string{
+				"bgpconfiguration.openstack.org/name":      bgpcfg.Name,
+				"bgpconfiguration.openstack.org/namespace": bgpcfg.Namespace,
+			}
+			Expect(k8sClient.Update(ctx, frr)).Should(Succeed())
+
+			// Trigger reconcile
+			pod := th.GetPod(podName)
+			if pod.Annotations == nil {
+				pod.Annotations = map[string]string{}
+			}
+			pod.Annotations["trigger-reconcile"] = "true"
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Update(ctx, pod)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			// Verify the legacy FRRConfiguration is cleaned up
+			Eventually(func(g Gomega) {
+				err := k8sClient.Get(ctx, types.NamespacedName{
+					Namespace: legacyNS.Name,
+					Name:      podFrrName.Name,
+				}, &frrk8sv1.FRRConfiguration{})
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue())
+			}, timeout, interval).Should(Succeed())
+		})
+	})
 })