Merge pull request #587 from lmiccini/bgp_420

OCP 4.20 frrconfiguration migration between namespaces

Author: openshift-merge-bot[bot]

config/rbac/role.yaml

@@ -24,6 +24,7 @@ rules:
   - ""
   resources:
   - endpoints
+  - namespaces
   verbs:
   - get
   - list

internal/bgp/funcs.go

@@ -2,8 +2,14 @@
 package bgp
 
 import (
+	"context"
 	"net"
 
+	corev1 "k8s.io/api/core/v1"
+	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
 	k8s_networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 	frrk8sv1 "github.com/metallb/frr-k8s/api/v1beta1"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
@@ -65,3 +71,21 @@ func GetNodesRunningPods(podNetworkDetailList []PodDetail) []string {
 
 	return nodes
 }
+
+// OpenShiftFRRNamespace is the FRR namespace used in OpenShift 4.20+
+const OpenShiftFRRNamespace = "openshift-frr-k8s"
+
+// 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) {
+	ns := &corev1.Namespace{}
+	err := c.Get(ctx, types.NamespacedName{Name: migrationNamespace}, ns)
+	if err != nil {
+		if k8s_errors.IsNotFound(err) {
+			return defaultNamespace, nil
+		}
+		return "", err
+	}
+
+	return migrationNamespace, nil
+}

internal/controller/network/bgpconfiguration_controller.go

@@ -56,19 +56,28 @@ import (
 // BGPConfigurationReconciler reconciles a BGPConfiguration object
 type BGPConfigurationReconciler struct {
 	client.Client
-	Kclient kubernetes.Interface
-	Scheme  *runtime.Scheme
+	Kclient               kubernetes.Interface
+	Scheme                *runtime.Scheme
+	FRRMigrationNamespace string
 }
 
 // GetLogger returns a logger object with a prefix of "controller.name" and additional controller context fields
 func (r *BGPConfigurationReconciler) GetLogger(ctx context.Context) logr.Logger {
 	return log.FromContext(ctx).WithName("Controllers").WithName("BGPConfiguration")
 }
 
+func (r *BGPConfigurationReconciler) getFRRMigrationNamespace() string {
+	if r.FRRMigrationNamespace != "" {
+		return r.FRRMigrationNamespace
+	}
+	return bgp.OpenShiftFRRNamespace
+}
+
 //+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
 //+kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;patch
+//+kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch
 //+kubebuilder:rbac:groups=frrk8s.metallb.io,resources=frrconfigurations,verbs=get;list;watch;create;update;patch;delete;deletecollection
 //+kubebuilder:rbac:groups=k8s.cni.cncf.io,resources=network-attachment-definitions,verbs=get;list;watch
 
@@ -353,12 +362,17 @@ func (r *BGPConfigurationReconciler) reconcileDelete(ctx context.Context, instan
 	Log := r.GetLogger(ctx)
 	Log.Info("Reconciling Service delete")
 
-	// Delete all FRRConfiguration in the Spec.FRRConfigurationNamespace namespace,
+	frrNamespace, err := bgp.GetFRRConfigurationNamespace(ctx, r.Client, r.getFRRMigrationNamespace(), instance.Spec.FRRConfigurationNamespace)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("unable to determine FRR namespace: %w", err)
+	}
+
+	// Delete all FRRConfiguration in the determined namespace,
 	// which have the correct owner and ownernamespace label
-	err := r.DeleteAllOf(
+	err = r.DeleteAllOf(
 		ctx,
 		&frrk8sv1.FRRConfiguration{},
-		client.InNamespace(instance.Spec.FRRConfigurationNamespace),
+		client.InNamespace(frrNamespace),
 		client.MatchingLabels{
 			labels.GetOwnerNameLabelSelector(labels.GetGroupLabel("bgpconfiguration")):      instance.Name,
 			labels.GetOwnerNameSpaceLabelSelector(labels.GetGroupLabel("bgpconfiguration")): instance.Namespace,
@@ -368,6 +382,12 @@ func (r *BGPConfigurationReconciler) reconcileDelete(ctx context.Context, instan
 		return ctrl.Result{}, fmt.Errorf("error DeleteAllOf FRRConfiguration: %w", err)
 	}
 
+	if frrNamespace != instance.Spec.FRRConfigurationNamespace {
+		if err := r.cleanupOldNamespaceFRRConfigurations(ctx, instance, instance.Spec.FRRConfigurationNamespace); err != nil {
+			Log.Error(err, "Failed to cleanup FRRConfigurations from old namespace", "namespace", instance.Spec.FRRConfigurationNamespace)
+		}
+	}
+
 	// Service is deleted so remove the finalizer.
 	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
 	Log.Info("Reconciled Service delete successfully")
@@ -379,6 +399,12 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan
 	Log := r.GetLogger(ctx)
 	Log.Info("Reconciling Service")
 
+	frrNamespace, err := bgp.GetFRRConfigurationNamespace(ctx, r.Client, r.getFRRMigrationNamespace(), instance.Spec.FRRConfigurationNamespace)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("unable to determine FRR namespace: %w", err)
+	}
+	Log.Info("Using FRR namespace", "namespace", frrNamespace)
+
 	// Get a list of pods which are in the same namespace as the ctlplane
 	// to verify if a FRRConfiguration needs to be created for.
 	podList := &corev1.PodList{}
@@ -398,7 +424,7 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan
 	// get all frrconfigs
 	frrConfigList := &frrk8sv1.FRRConfigurationList{}
 	listOpts = []client.ListOption{
-		client.InNamespace(instance.Spec.FRRConfigurationNamespace), // defaults to metallb-system
+		client.InNamespace(frrNamespace),
 	}
 	if err := r.List(ctx, frrConfigList, listOpts...); err != nil {
 		return ctrl.Result{}, fmt.Errorf("unable to retrieve FRRConfigurationList %w", err)
@@ -462,6 +488,7 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan
 				map[string]string{
 					groupLabel + "/pod-name": podNetworkDetail.Name,
 				}),
+			frrNamespace,
 		)
 		if err != nil {
 			return ctrl.Result{}, err
@@ -474,6 +501,13 @@ func (r *BGPConfigurationReconciler) reconcileNormal(ctx context.Context, instan
 		return ctrl.Result{}, err
 	}
 
+	// Clean up old FRRConfigurations from the old namespace if we've migrated to a new one
+	if frrNamespace != instance.Spec.FRRConfigurationNamespace {
+		if err := r.cleanupOldNamespaceFRRConfigurations(ctx, instance, instance.Spec.FRRConfigurationNamespace); err != nil {
+			Log.Error(err, "Failed to cleanup old FRRConfigurations from namespace", "namespace", instance.Spec.FRRConfigurationNamespace)
+		}
+	}
+
 	Log.Info("Reconciled Service successfully")
 	return ctrl.Result{}, nil
 }
@@ -500,6 +534,30 @@ func (r *BGPConfigurationReconciler) deleteStaleFRRConfigurations(ctx context.Co
 	return nil
 }
 
+func (r *BGPConfigurationReconciler) cleanupOldNamespaceFRRConfigurations(
+	ctx context.Context,
+	instance *networkv1.BGPConfiguration,
+	oldNamespace string,
+) error {
+	Log := r.GetLogger(ctx)
+	Log.Info("Cleaning up old FRRConfigurations", "namespace", oldNamespace)
+
+	err := r.DeleteAllOf(
+		ctx,
+		&frrk8sv1.FRRConfiguration{},
+		client.InNamespace(oldNamespace),
+		client.MatchingLabels{
+			labels.GetOwnerNameLabelSelector(labels.GetGroupLabel("bgpconfiguration")):      instance.Name,
+			labels.GetOwnerNameSpaceLabelSelector(labels.GetGroupLabel("bgpconfiguration")): instance.Namespace,
+		},
+	)
+	if err != nil && !k8s_errors.IsNotFound(err) {
+		return fmt.Errorf("error DeleteAllOf FRRConfiguration in old namespace %s: %w", oldNamespace, err)
+	}
+
+	return nil
+}
+
 // getPodNetworkDetails - returns the podDetails for a list of pods in status.phase: Running
 // where the pod has the multus k8s_networkv1.NetworkAttachmentAnnot annotation
 // and its value is not '[]' OR has a predictableip label
@@ -650,6 +708,7 @@ func (r *BGPConfigurationReconciler) createOrPatchFRRConfiguration(
 	podDtl *bgp.PodDetail,
 	nodeFRRCfgs map[string]frrk8sv1.FRRConfiguration,
 	frrLabels map[string]string,
+	frrNamespace string,
 ) error {
 	Log := r.GetLogger(ctx)
 	Log.Info("Reconciling createOrUpdateFRRConfiguration")
@@ -662,7 +721,7 @@ func (r *BGPConfigurationReconciler) createOrPatchFRRConfiguration(
 	frrConfig := &frrk8sv1.FRRConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      instance.Namespace + "-" + podDtl.Name,
-			Namespace: instance.Spec.FRRConfigurationNamespace,
+			Namespace: frrNamespace,
 		},
 	}
 

test/functional/bgpconfiguration_controller_test.go

@@ -26,6 +26,7 @@ import (
 	frrk8sv1 "github.com/metallb/frr-k8s/api/v1beta1"
 	networkv1 "github.com/openstack-k8s-operators/infra-operator/apis/network/v1beta1"
 	corev1 "k8s.io/api/core/v1"
+	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -573,4 +574,150 @@ var _ = Describe("BGPConfiguration controller", func() {
 			})
 		})
 	})
+
+	When("FRR migration namespace exists (OpenShift 4.20+ migration)", func() {
+		var podFrrName types.NamespacedName
+		var podName types.NamespacedName
+		var metallbNS *corev1.Namespace
+		var migrationNS *corev1.Namespace
+
+		BeforeEach(func() {
+			metallbNS = th.CreateNamespace(frrCfgNamespace + "-" + namespace)
+
+			// Use a unique namespace name to avoid envtest namespace leak
+			migrationNSName := "frr-migration-" + namespace
+			migrationNS = th.CreateNamespace(migrationNSName)
+			bgpReconciler.FRRMigrationNamespace = migrationNSName
+			DeferCleanup(func() {
+				bgpReconciler.FRRMigrationNamespace = ""
+			})
+
+			migrationFRRCfgName := types.NamespacedName{Namespace: migrationNS.Name, Name: "worker-0"}
+			migrationFRRCfg := CreateFRRConfiguration(migrationFRRCfgName, GetMetalLBFRRConfigurationSpec("worker-0"))
+			Expect(migrationFRRCfg).To(Not(BeNil()))
+
+			nad := th.CreateNAD(types.NamespacedName{Namespace: namespace, Name: "internalapi"}, GetNADSpec())
+
+			bgpcfg := CreateBGPConfiguration(namespace, GetBGPConfigurationSpec(metallbNS.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 = migrationNS.Name
+
+			DeferCleanup(th.DeleteInstance, bgpcfg)
+			DeferCleanup(th.DeleteInstance, nad)
+			DeferCleanup(th.DeleteInstance, migrationFRRCfg)
+		})
+
+		It("should create FRRConfiguration in the migration namespace instead of metallb-system", func() {
+			pod := th.GetPod(podName)
+			Expect(pod).To(Not(BeNil()))
+
+			Eventually(func(g Gomega) {
+				frr := GetFRRConfiguration(types.NamespacedName{Namespace: migrationNS.Name, Name: podFrrName.Name})
+				g.Expect(frr).To(Not(BeNil()))
+				g.Expect(frr.Namespace).To(Equal(migrationNS.Name))
+				g.Expect(frr.Spec.BGP.Routers[0].Prefixes[0]).To(Equal("172.17.0.40/32"))
+			}, timeout, interval).Should(Succeed())
+
+			frr := &frrk8sv1.FRRConfiguration{}
+			Consistently(func(g Gomega) {
+				err := k8sClient.Get(ctx, types.NamespacedName{Namespace: metallbNS.Name, Name: podFrrName.Name}, frr)
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue())
+			}, timeout, interval).Should(Succeed())
+		})
+	})
+
+	When("existing FRRConfigurations in old namespace get migrated to new namespace", func() {
+		var podFrrName types.NamespacedName
+		var podName types.NamespacedName
+		var metallbNS *corev1.Namespace
+
+		BeforeEach(func() {
+			metallbNS = th.CreateNamespace(frrCfgNamespace + "-" + namespace)
+
+			meallbFRRCfgName := types.NamespacedName{Namespace: metallbNS.Name, Name: "worker-0"}
+			meallbFRRCfg := CreateFRRConfiguration(meallbFRRCfgName, GetMetalLBFRRConfigurationSpec("worker-0"))
+			Expect(meallbFRRCfg).To(Not(BeNil()))
+
+			nad := th.CreateNAD(types.NamespacedName{Namespace: namespace, Name: "internalapi"}, GetNADSpec())
+
+			bgpcfg := CreateBGPConfiguration(namespace, GetBGPConfigurationSpec(metallbNS.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 = metallbNS.Name
+
+			DeferCleanup(th.DeleteInstance, bgpcfg)
+			DeferCleanup(th.DeleteInstance, nad)
+			DeferCleanup(th.DeleteInstance, meallbFRRCfg)
+		})
+
+		It("should migrate FRRConfigurations from old namespace and clean up", func() {
+			// First verify the FRRConfiguration exists in the old namespace
+			Eventually(func(g Gomega) {
+				frr := GetFRRConfiguration(types.NamespacedName{Namespace: metallbNS.Name, 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())
+
+			// Now simulate OCP 4.20 upgrade: create the migration namespace and
+			// add the reference FRRConfiguration there
+			migrationNSName := "frr-migration-" + namespace
+			migrationNS := th.CreateNamespace(migrationNSName)
+			migrationFRRCfg := CreateFRRConfiguration(
+				types.NamespacedName{Namespace: migrationNS.Name, Name: "worker-0"},
+				GetMetalLBFRRConfigurationSpec("worker-0"),
+			)
+			Expect(migrationFRRCfg).To(Not(BeNil()))
+
+			bgpReconciler.FRRMigrationNamespace = migrationNSName
+			DeferCleanup(func() {
+				bgpReconciler.FRRMigrationNamespace = ""
+			})
+			DeferCleanup(th.DeleteInstance, migrationFRRCfg)
+
+			// Trigger a reconcile by updating the pod
+			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 FRRConfiguration is created in the new namespace
+			Eventually(func(g Gomega) {
+				frr := GetFRRConfiguration(types.NamespacedName{
+					Namespace: migrationNS.Name,
+					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())
+
+			// Verify old FRRConfiguration in metallb-system is cleaned up
+			Eventually(func(g Gomega) {
+				frr := &frrk8sv1.FRRConfiguration{}
+				err := k8sClient.Get(ctx, types.NamespacedName{
+					Namespace: metallbNS.Name,
+					Name:      podFrrName.Name,
+				}, frr)
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue())
+			}, timeout, interval).Should(Succeed())
+		})
+	})
 })