Merge pull request #5099 from jcaamano/egressip-bgp-v2

Add support to advertise EIPs for UDNs

Author: trozet

go-controller/pkg/clustermanager/routeadvertisements/controller.go

@@ -57,11 +57,14 @@ var (
 
 // Controller reconciles RouteAdvertisements
 type Controller struct {
-	eipLister  egressiplisters.EgressIPLister
-	frrLister  frrlisters.FRRConfigurationLister
-	nadLister  nadlisters.NetworkAttachmentDefinitionLister
-	nodeLister corelisters.NodeLister
-	raLister   ralisters.RouteAdvertisementsLister
+	wf *factory.WatchFactory
+
+	eipLister       egressiplisters.EgressIPLister
+	frrLister       frrlisters.FRRConfigurationLister
+	nadLister       nadlisters.NetworkAttachmentDefinitionLister
+	nodeLister      corelisters.NodeLister
+	raLister        ralisters.RouteAdvertisementsLister
+	namespaceLister corelisters.NamespaceLister
 
 	frrClient frrclientset.Interface
 	nadClient nadclientset.Interface
@@ -72,6 +75,7 @@ type Controller struct {
 	nadController  controllerutil.Controller
 	nodeController controllerutil.Controller
 	raController   controllerutil.Controller
+	nsController   controllerutil.Controller
 
 	nm networkmanager.Interface
 }
@@ -83,15 +87,17 @@ func NewController(
 	ovnClient *util.OVNClusterManagerClientset,
 ) *Controller {
 	c := &Controller{
-		eipLister:  wf.EgressIPInformer().Lister(),
-		frrLister:  wf.FRRConfigurationsInformer().Lister(),
-		nadLister:  wf.NADInformer().Lister(),
-		nodeLister: wf.NodeCoreInformer().Lister(),
-		raLister:   wf.RouteAdvertisementsInformer().Lister(),
-		frrClient:  ovnClient.FRRClient,
-		nadClient:  ovnClient.NetworkAttchDefClient,
-		raClient:   ovnClient.RouteAdvertisementsClient,
-		nm:         nm,
+		wf:              wf,
+		eipLister:       wf.EgressIPInformer().Lister(),
+		frrLister:       wf.FRRConfigurationsInformer().Lister(),
+		nadLister:       wf.NADInformer().Lister(),
+		nodeLister:      wf.NodeCoreInformer().Lister(),
+		raLister:        wf.RouteAdvertisementsInformer().Lister(),
+		namespaceLister: wf.NamespaceInformer().Lister(),
+		frrClient:       ovnClient.FRRClient,
+		nadClient:       ovnClient.NetworkAttchDefClient,
+		raClient:        ovnClient.RouteAdvertisementsClient,
+		nm:              nm,
 	}
 
 	handleError := func(key string, errorstatus error) error {
@@ -153,14 +159,24 @@ func NewController(
 
 	eipConfig := &controllerutil.ControllerConfig[eiptypes.EgressIP]{
 		RateLimiter:    workqueue.DefaultTypedControllerRateLimiter[string](),
-		Reconcile:      c.reconcileEgressIP,
+		Reconcile:      c.reconcileEgressIPs,
 		Threadiness:    1,
 		Informer:       wf.EgressIPInformer().Informer(),
 		Lister:         wf.EgressIPInformer().Lister().List,
 		ObjNeedsUpdate: egressIPNeedsUpdate,
 	}
 	c.eipController = controllerutil.NewController("clustermanager routeadvertisements egressip controller", eipConfig)
 
+	nsConfig := &controllerutil.ControllerConfig[corev1.Namespace]{
+		RateLimiter:    workqueue.DefaultTypedControllerRateLimiter[string](),
+		Reconcile:      c.reconcileEgressIPs,
+		Threadiness:    1,
+		Informer:       wf.NamespaceInformer().Informer(),
+		Lister:         wf.NamespaceInformer().Lister().List,
+		ObjNeedsUpdate: nsNeedsUpdate,
+	}
+	c.nsController = controllerutil.NewController("clustermanager routeadvertisements namespace controller", nsConfig)
+
 	return c
 }
 
@@ -171,6 +187,7 @@ func (c *Controller) Start() error {
 		c.frrController,
 		c.nadController,
 		c.nodeController,
+		c.nsController,
 		c.raController,
 	)
 }
@@ -181,20 +198,33 @@ func (c *Controller) Stop() {
 		c.frrController,
 		c.nadController,
 		c.nodeController,
+		c.nsController,
 		c.raController,
 	)
-	klog.Infof("Cluster manager routeadvertisements stoppedu")
+	klog.Infof("Cluster manager routeadvertisements stopped")
 }
 
 func (c *Controller) ReconcileNetwork(_ string, old, new util.NetInfo) {
-	// This controller already listens on NAD events however we skip NADs
-	// pointing to networks that network manager is still not aware of; so we
-	// only need to signal the reconciliation of new networks. Reconcile one of
-	// the NADs of the network to do s
-	if new == nil || old != nil {
-		return
-	}
-	c.nadController.Reconcile(new.GetNADs()[0])
+	// This controller already listens on NAD events but there is two additional
+	// scenarios we need to cover for:
+	// - for newly created networks, we need to wait until network manager is
+	// aware of them.
+	// - if the namespaces served by a network change.
+	oldNamespaces, newNamespaces := sets.New[string](), sets.New[string]()
+	if old != nil {
+		oldNamespaces.Insert(old.GetNADNamespaces()...)
+	}
+	if new != nil {
+		newNamespaces.Insert(new.GetNADNamespaces()...)
+	}
+	if new != nil && !newNamespaces.Equal(oldNamespaces) {
+		// we use one of the NADs of the network to reconcile it
+		c.nadController.Reconcile(new.GetNADs()[0])
+		// if the namespaces served by a network changed, it is possible that
+		// those namespaces are served or no longer served by the default
+		// network, so reconcile it as well
+		c.nadController.Reconcile(config.Kubernetes.OVNConfigNamespace + "/" + types.DefaultNetworkName)
+	}
 }
 
 // Reconcile RouteAdvertisements. For each selected FRRConfiguration and node,
@@ -206,7 +236,8 @@ func (c *Controller) ReconcileNetwork(_ string, old, new util.NetInfo) {
 //
 // - If EgressIP advertisements are enabled, the generated FRRConfiguration will
 // announce from the node the EgressIPs allocated to it on the matching target
-// VRFs.
+// VRFs. Selected EgressIP are those that serve the same namespaces as the
+// selected networks. Target VRF `auto` is not supported for EgressIPs.
 //
 // - If pod network advertisements are enabled, the generated FRRConfiguration
 // will import the target VRFs on the selected networks as required.
@@ -222,7 +253,7 @@ func (c *Controller) ReconcileNetwork(_ string, old, new util.NetInfo) {
 // Finally, it will update the status of the RouteAdvertisements.
 //
 // The controller processes selected events of RouteAdvertisements,
-// FRRConfigurations, Nodes, EgressIPs and NADs.
+// FRRConfigurations, Nodes, EgressIPs, NADs and namespaces.
 func (c *Controller) reconcile(name string) error {
 	startTime := time.Now()
 	klog.V(5).Infof("Syncing routeadvertisements %q", name)
@@ -294,6 +325,11 @@ func (c *Controller) generateFRRConfigurations(ra *ratypes.RouteAdvertisements)
 		return nil, nil, nil
 	}
 
+	advertisements := sets.New(ra.Spec.Advertisements...)
+	if advertisements.Has(ratypes.EgressIP) && ra.Spec.TargetVRF == "auto" {
+		return nil, nil, fmt.Errorf("%w: advertising EgressIP not supported with TargetVRF set to 'auto'", errConfig)
+	}
+
 	// if we are matching on the well known default network label, create an
 	// internal nad for it if it doesn't exist
 	if matchesDefaultNetworkLabel(ra.Spec.NetworkSelector) {
@@ -369,7 +405,6 @@ func (c *Controller) generateFRRConfigurations(ra *ratypes.RouteAdvertisements)
 	if err != nil {
 		return nil, nil, err
 	}
-	advertisements := sets.New(ra.Spec.Advertisements...)
 	if !nodeSelector.Empty() && advertisements.Has(ratypes.PodNetwork) {
 		return nil, nil, fmt.Errorf("%w: node selector cannot be specified if pod network is advertised", errConfig)
 	}
@@ -444,15 +479,15 @@ func (c *Controller) generateFRRConfigurations(ra *ratypes.RouteAdvertisements)
 
 	// helper to gather egress ips and cache during reconcile
 	// TODO perhaps cache across reconciles as well
-	var nodeEgressIPs map[string][]string
-	getEgressIPs := func(nodeName string) ([]string, error) {
-		if nodeEgressIPs == nil {
-			nodeEgressIPs, err = c.getEgressIPsByNode()
+	var eipsByNodesByNetworks map[string]map[string]sets.Set[string]
+	getEgressIPsByNode := func(nodeName string) (map[string]sets.Set[string], error) {
+		if eipsByNodesByNetworks == nil {
+			eipsByNodesByNetworks, err = c.getEgressIPsByNodesByNetworks(networkSet)
 			if err != nil {
 				return nil, err
 			}
 		}
-		return nodeEgressIPs[nodeName], nil
+		return eipsByNodesByNetworks[nodeName], nil
 	}
 
 	// helper to gather host subnets and egress ips as prefixes
@@ -469,13 +504,11 @@ func (c *Controller) generateFRRConfigurations(ra *ratypes.RouteAdvertisements)
 		// gather EgressIPs
 		var eips []string
 		if advertisements.Has(ratypes.EgressIP) {
-			if network != types.DefaultNetworkName {
-				return nil, fmt.Errorf("%w: can't advertise EgressIP in selected non default network %q: %w", errConfig, network, err)
-			}
-			eips, err = getEgressIPs(nodeName)
+			eipsByNode, err := getEgressIPsByNode(nodeName)
 			if err != nil {
 				return nil, err
 			}
+			eips = eipsByNode[network].UnsortedList()
 		}
 
 		prefixes := make([]string, 0, len(subnets)+len(eips))
@@ -500,8 +533,13 @@ func (c *Controller) generateFRRConfigurations(ra *ratypes.RouteAdvertisements)
 			// ordered
 			slices.Sort(selectedNetworks.hostNetworkSubnets[network])
 		}
-		// ordered
-		slices.Sort(selectedNetworks.hostSubnets)
+		// order, dedup
+		selectedNetworks.hostSubnets = sets.List(sets.New(selectedNetworks.hostSubnets...))
+
+		// if there is no prefixes to advertise for this node, skip it
+		if len(selectedNetworks.hostSubnets) == 0 {
+			continue
+		}
 
 		matchedNetworks := sets.New[string]()
 		for _, frrConfig := range frrConfigs {
@@ -985,26 +1023,69 @@ func (c *Controller) getOrCreateDefaultNetworkNAD() (*nadtypes.NetworkAttachment
 	)
 }
 
-// getEgressIPsByNode iterates all existing egress IPs and returns them indexed
-// by node
-func (c *Controller) getEgressIPsByNode() (map[string][]string, error) {
+// getEgressIPsByNodesByNetworks iterates all existing egress IPs that apply to
+// any of the provided networks and returns a "node -> network -> eips"
+// map.
+func (c *Controller) getEgressIPsByNodesByNetworks(networks sets.Set[string]) (map[string]map[string]sets.Set[string], error) {
+	eipsByNodesByNetworks := map[string]map[string]sets.Set[string]{}
+	addEgressIPsByNodesByNetwork := func(eipsByNodes map[string]string, network string) {
+		for node, eip := range eipsByNodes {
+			if eipsByNodesByNetworks[node] == nil {
+				eipsByNodesByNetworks[node] = map[string]sets.Set[string]{}
+			}
+			if eipsByNodesByNetworks[node][network] == nil {
+				eipsByNodesByNetworks[node][network] = sets.New[string]()
+			}
+			eipsByNodesByNetworks[node][network].Insert(eip)
+		}
+	}
+
+	addEgressIPsByNodesByNetworkSelector := func(eipsByNodes map[string]string, namespaceSelector *metav1.LabelSelector) error {
+		nsSelector, err := metav1.LabelSelectorAsSelector(namespaceSelector)
+		if err != nil {
+			return err
+		}
+		selected, err := c.namespaceLister.List(nsSelector)
+		if err != nil {
+			return err
+		}
+		for _, namespace := range selected {
+			namespaceNetwork := c.nm.GetActiveNetworkForNamespaceFast(namespace.Name)
+			networkName := namespaceNetwork.GetNetworkName()
+			if networks.Has(networkName) {
+				addEgressIPsByNodesByNetwork(eipsByNodes, networkName)
+			}
+		}
+		return nil
+	}
+
 	eips, err := c.eipLister.List(labels.Everything())
 	if err != nil {
 		return nil, err
 	}
 
-	eipsByNode := map[string][]string{}
 	for _, eip := range eips {
+		eipsByNodes := make(map[string]string, len(eip.Status.Items))
 		for _, item := range eip.Status.Items {
-			if item.EgressIP == "" {
+			// skip unassigned EIPs
+			if item.EgressIP == "" || item.Node == "" {
 				continue
 			}
+
 			ip := item.EgressIP + util.GetIPFullMaskString(item.EgressIP)
-			eipsByNode[item.Node] = append(eipsByNode[item.Node], ip)
+			eipsByNodes[item.Node] = ip
+		}
+		if len(eipsByNodes) == 0 {
+			continue
+		}
+
+		err = addEgressIPsByNodesByNetworkSelector(eipsByNodes, &eip.Spec.NamespaceSelector)
+		if err != nil {
+			return nil, err
 		}
 	}
 
-	return eipsByNode, nil
+	return eipsByNodesByNetworks, nil
 }
 
 // isOwnUpdate checks if an object was updated by us last, as indicated by its
@@ -1055,12 +1136,13 @@ func nadNeedsUpdate(oldObj, newObj *nadtypes.NetworkAttachmentDefinition) bool {
 func nodeNeedsUpdate(oldObj, newObj *corev1.Node) bool {
 	return oldObj == nil || newObj == nil ||
 		!reflect.DeepEqual(oldObj.Labels, newObj.Labels) ||
-		util.NodeSubnetAnnotationChanged(oldObj, newObj)
+		util.NodeSubnetAnnotationChanged(oldObj, newObj) ||
+		oldObj.Annotations[util.OvnNodeIfAddr] != newObj.Annotations[util.OvnNodeIfAddr]
 }
 
 func egressIPNeedsUpdate(oldObj, newObj *eiptypes.EgressIP) bool {
-	if oldObj != nil && newObj != nil && reflect.DeepEqual(oldObj.Status, newObj.Status) {
-		return false
+	if oldObj != nil && newObj != nil {
+		return !reflect.DeepEqual(oldObj.Status, newObj.Status) || !reflect.DeepEqual(oldObj.Spec.NamespaceSelector, newObj.Spec.NamespaceSelector)
 	}
 	if oldObj != nil && len(oldObj.Status.Items) > 0 {
 		return true
@@ -1071,6 +1153,12 @@ func egressIPNeedsUpdate(oldObj, newObj *eiptypes.EgressIP) bool {
 	return false
 }
 
+func nsNeedsUpdate(oldObj, newObj *corev1.Namespace) bool {
+	// we only care about label changes, added/deleted namespaces served by a
+	// UDN will already be reflected in a network update
+	return oldObj != nil && newObj != nil && !reflect.DeepEqual(oldObj.Labels, newObj.Labels)
+}
+
 func (c *Controller) reconcileFRRConfiguration(key string) error {
 	namespace, name, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
@@ -1133,7 +1221,7 @@ func (c *Controller) reconcileNAD(key string) error {
 	return nil
 }
 
-func (c *Controller) reconcileEgressIP(_ string) error {
+func (c *Controller) reconcileEgressIPs(string) error {
 	// reconcile RAs that advertise EIPs
 	ras, err := c.raLister.List(labels.Everything())
 	if err != nil {