UDN: Design routes and policies on L2's GR correctly.

go-controller/pkg/libovsdb/ops/router.go

@@ -644,7 +644,7 @@ func CreateOrReplaceLogicalRouterStaticRouteWithPredicate(nbClient libovsdbclien
 	lr := &nbdb.LogicalRouter{Name: routerName}
 	router, err := GetLogicalRouter(nbClient, lr)
 	if err != nil {
-		return err
+		return fmt.Errorf("unable to get logical router %s: %w", routerName, err)
 	}
 	newPredicate := func(item *nbdb.LogicalRouterStaticRoute) bool {
 		for _, routeUUID := range router.StaticRoutes {
@@ -656,7 +656,7 @@ func CreateOrReplaceLogicalRouterStaticRouteWithPredicate(nbClient libovsdbclien
 	}
 	routes, err := FindLogicalRouterStaticRoutesWithPredicate(nbClient, newPredicate)
 	if err != nil {
-		return err
+		return fmt.Errorf("unable to get logical router static routes with predicate on router %s: %w", routerName, err)
 	}
 
 	var ops []libovsdb.Operation
@@ -697,7 +697,7 @@ func CreateOrReplaceLogicalRouterStaticRouteWithPredicate(nbClient libovsdbclien
 
 	ops, err = CreateOrUpdateLogicalRouterStaticRoutesWithPredicateOps(nbClient, ops, routerName, lrsr, nil, fields...)
 	if err != nil {
-		return err
+		return fmt.Errorf("unable to get create or update logical router static routes on router %s: %w", routerName, err)
 	}
 	_, err = TransactAndCheck(nbClient, ops)
 	return err

go-controller/pkg/ovn/base_network_controller.go

@@ -615,7 +615,7 @@ func (bnc *BaseNetworkController) deleteNamespaceLocked(ns string) (*namespaceIn
 	return nsInfo, nil
 }
 
-func (bnc *BaseNetworkController) syncNodeManagementPort(node *kapi.Node, switchName string, hostSubnets []*net.IPNet, routeHostSubnets bool) ([]net.IP, error) {
+func (bnc *BaseNetworkController) syncNodeManagementPort(node *kapi.Node, switchName, routerName string, hostSubnets []*net.IPNet) ([]net.IP, error) {
 	macAddress, err := util.ParseNodeManagementPortMACAddresses(node, bnc.GetNetworkName())
 	if err != nil {
 		return nil, err
@@ -636,19 +636,25 @@ func (bnc *BaseNetworkController) syncNodeManagementPort(node *kapi.Node, switch
 		if !utilnet.IsIPv6CIDR(hostSubnet) {
 			v4Subnet = hostSubnet
 		}
-		if config.Gateway.Mode == config.GatewayModeLocal && routeHostSubnets {
+		if config.Gateway.Mode == config.GatewayModeLocal {
 			lrsr := nbdb.LogicalRouterStaticRoute{
 				Policy:   &nbdb.LogicalRouterStaticRoutePolicySrcIP,
 				IPPrefix: hostSubnet.String(),
 				Nexthop:  mgmtIfAddr.IP.String(),
 			}
+			if bnc.IsSecondary() {
+				lrsr.ExternalIDs = map[string]string{
+					ovntypes.NetworkExternalID:  bnc.GetNetworkName(),
+					ovntypes.TopologyExternalID: bnc.TopologyType(),
+				}
+			}
 			p := func(item *nbdb.LogicalRouterStaticRoute) bool {
 				return item.IPPrefix == lrsr.IPPrefix && libovsdbops.PolicyEqualPredicate(lrsr.Policy, item.Policy)
 			}
-			err := libovsdbops.CreateOrReplaceLogicalRouterStaticRouteWithPredicate(bnc.nbClient, bnc.GetNetworkScopedClusterRouterName(),
+			err := libovsdbops.CreateOrReplaceLogicalRouterStaticRouteWithPredicate(bnc.nbClient, routerName,
 				&lrsr, p, &lrsr.Nexthop)
 			if err != nil {
-				return nil, fmt.Errorf("error creating static route %+v on router %s: %v", lrsr, bnc.GetNetworkScopedClusterRouterName(), err)
+				return nil, fmt.Errorf("error creating static route %+v on router %s: %v", lrsr, routerName, err)
 			}
 		}
 	}
@@ -680,14 +686,6 @@ func (bnc *BaseNetworkController) syncNodeManagementPort(node *kapi.Node, switch
 	return mgmtPortIPs, nil
 }
 
-func (bnc *BaseNetworkController) syncNodeManagementPortRouteHostSubnets(node *kapi.Node, switchName string, hostSubnets []*net.IPNet) ([]net.IP, error) {
-	return bnc.syncNodeManagementPort(node, switchName, hostSubnets, true)
-}
-
-func (bnc *BaseNetworkController) syncNodeManagementPortNoRouteHostSubnets(node *kapi.Node, switchName string, hostSubnets []*net.IPNet) ([]net.IP, error) {
-	return bnc.syncNodeManagementPort(node, switchName, hostSubnets, false)
-}
-
 // WatchNodes starts the watching of the nodes resource and calls back the appropriate handler logic
 func (bnc *BaseNetworkController) WatchNodes() error {
 	if bnc.nodeHandler != nil {

go-controller/pkg/ovn/gateway.go

@@ -333,6 +333,15 @@ func (gw *GatewayManager) GatewayInit(
 	gwSwitchPort := types.JoinSwitchToGWRouterPrefix + gatewayRouter
 	gwRouterPort := types.GWRouterToJoinSwitchPrefix + gatewayRouter
 
+	// In Layer2 networks there is no join switch and the gw.joinSwitchName points to the cluster switch.
+	// Ensure that the ports are named appropriately, this is important for the logical router policies
+	// created for local node access.
+	// TODO(kyrtapz): Clean this up for clarity as part of https://github.com/ovn-org/ovn-kubernetes/issues/4689
+	if gw.netInfo.TopologyType() == types.Layer2Topology {
+		gwSwitchPort = types.SwitchToRouterPrefix + gw.joinSwitchName
+		gwRouterPort = types.RouterToSwitchPrefix + gw.joinSwitchName
+	}
+
 	logicalSwitchPort := nbdb.LogicalSwitchPort{
 		Name:      gwSwitchPort,
 		Type:      "router",
@@ -1001,6 +1010,17 @@ func (gw *GatewayManager) Cleanup() error {
 	var nextHops []net.IP
 
 	gwRouterToJoinSwitchPortName := types.GWRouterToJoinSwitchPrefix + gw.gwRouterName
+	portName := types.JoinSwitchToGWRouterPrefix + gw.gwRouterName
+
+	// In Layer2 networks there is no join switch and the gw.joinSwitchName points to the cluster switch.
+	// Ensure that the ports are named appropriately, this is important for the logical router policies
+	// created for local node access.
+	// TODO(kyrtapz): Clean this up for clarity as part of https://github.com/ovn-org/ovn-kubernetes/issues/4689
+	if gw.netInfo.TopologyType() == types.Layer2Topology {
+		gwRouterToJoinSwitchPortName = types.RouterToSwitchPrefix + gw.joinSwitchName
+		portName = types.SwitchToRouterPrefix + gw.joinSwitchName
+	}
+
 	gwIPAddrs, err := libovsdbutil.GetLRPAddrs(gw.nbClient, gwRouterToJoinSwitchPortName)
 	if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {
 		return fmt.Errorf(
@@ -1018,7 +1038,6 @@ func (gw *GatewayManager) Cleanup() error {
 	gw.policyRouteCleanup(nextHops)
 
 	// Remove the patch port that connects join switch to gateway router
-	portName := types.JoinSwitchToGWRouterPrefix + gw.gwRouterName
 	lsp := nbdb.LogicalSwitchPort{Name: portName}
 	sw := nbdb.LogicalSwitch{Name: gw.joinSwitchName}
 	err = libovsdbops.DeleteLogicalSwitchPorts(gw.nbClient, &sw, &lsp)
@@ -1207,22 +1226,28 @@ func (gw *GatewayManager) syncGatewayLogicalNetwork(
 		routerName = gw.gwRouterName
 	}
 	for _, subnet := range hostSubnets {
-		hostIfAddr := util.GetNodeManagementIfAddr(subnet)
-		if hostIfAddr == nil {
-			return fmt.Errorf("host interface address not found for subnet %q on network %q", subnet, gw.netInfo.GetNetworkName())
+		mgmtIfAddr := util.GetNodeManagementIfAddr(subnet)
+		if mgmtIfAddr == nil {
+			return fmt.Errorf("management interface address not found for subnet %q on network %q", subnet, gw.netInfo.GetNetworkName())
 		}
-		l3GatewayConfigIP, err := util.MatchFirstIPNetFamily(utilnet.IsIPv6(hostIfAddr.IP), l3GatewayConfig.IPAddresses)
+		l3GatewayConfigIP, err := util.MatchFirstIPNetFamily(utilnet.IsIPv6(mgmtIfAddr.IP), l3GatewayConfig.IPAddresses)
 		if err != nil {
 			return fmt.Errorf("failed to extract the gateway IP addr for network %q: %v", gw.netInfo.GetNetworkName(), err)
 		}
-		relevantHostIPs, err := util.MatchAllIPStringFamily(utilnet.IsIPv6(hostIfAddr.IP), hostAddrs)
+		relevantHostIPs, err := util.MatchAllIPStringFamily(utilnet.IsIPv6(mgmtIfAddr.IP), hostAddrs)
 		if err != nil && err != util.ErrorNoIP {
 			return fmt.Errorf("failed to extract the host IP addrs for network %q: %v", gw.netInfo.GetNetworkName(), err)
 		}
 		pbrMngr := gatewayrouter.NewPolicyBasedRoutesManager(gw.nbClient, routerName, gw.netInfo)
-		if err := pbrMngr.Add(node.Name, hostIfAddr.IP.String(), l3GatewayConfigIP, relevantHostIPs); err != nil {
+		if err := pbrMngr.AddSameNodeIPPolicy(node.Name, mgmtIfAddr.IP.String(), l3GatewayConfigIP, relevantHostIPs); err != nil {
 			return fmt.Errorf("failed to configure the policy based routes for network %q: %v", gw.netInfo.GetNetworkName(), err)
 		}
+		if gw.netInfo.TopologyType() == types.Layer2Topology && config.Gateway.Mode == config.GatewayModeLocal {
+			if err := pbrMngr.AddHostCIDRPolicy(node, mgmtIfAddr.IP.String(), subnet.String()); err != nil {
+				return fmt.Errorf("failed to configure the hostCIDR policy for L2 network %q on local gateway: %v",
+					gw.netInfo.GetNetworkName(), err)
+			}
+		}
 	}
 
 	return nil

go-controller/pkg/ovn/gatewayrouter/policybasedroutes.go

@@ -6,6 +6,7 @@ import (
 	"strconv"
 	"strings"
 
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	utilnet "k8s.io/utils/net"
 
@@ -31,7 +32,7 @@ func NewPolicyBasedRoutesManager(nbClient client.Client, clusterRouterName strin
 	}
 }
 
-func (pbr *PolicyBasedRoutesManager) Add(nodeName, mgmtPortIP string, hostIfCIDR *net.IPNet, otherHostAddrs []string) error {
+func (pbr *PolicyBasedRoutesManager) AddSameNodeIPPolicy(nodeName, mgmtPortIP string, hostIfCIDR *net.IPNet, otherHostAddrs []string) error {
 	if hostIfCIDR == nil {
 		return fmt.Errorf("<nil> host interface CIDR")
 	}
@@ -44,13 +45,13 @@ func (pbr *PolicyBasedRoutesManager) Add(nodeName, mgmtPortIP string, hostIfCIDR
 	if !isHostIPsValid(otherHostAddrs) {
 		return fmt.Errorf("invalid other host address(es): %v", otherHostAddrs)
 	}
-	l3Prefix := getIPPrefix(hostIfCIDR.IP)
+	l3Prefix := getIPCIDRPrefix(hostIfCIDR)
 	matches := sets.New[string]()
 	for _, hostIP := range append(otherHostAddrs, hostIfCIDR.IP.String()) {
 		// embed nodeName as comment so that it is easier to delete these rules later on.
 		// logical router policy doesn't support external_ids to stash metadata
 		networkScopedSwitchName := pbr.netInfo.GetNetworkScopedSwitchName(nodeName)
-		matchStr := generateMatch(networkScopedSwitchName, l3Prefix, hostIP)
+		matchStr := generateNodeIPMatch(networkScopedSwitchName, l3Prefix, hostIP)
 		matches = matches.Insert(matchStr)
 	}
 
@@ -66,6 +67,46 @@ func (pbr *PolicyBasedRoutesManager) Add(nodeName, mgmtPortIP string, hostIfCIDR
 	return nil
 }
 
+// AddHostCIDRPolicy adds the following policy in local-gateway-mode for UDN L2 topology to the GR
+// 99 ip4.dst == 172.18.0.0/16 && ip4.src == 10.100.200.0/24         reroute              10.100.200.2
+// Since rtoe of GR is directly connected to the hostCIDR range in LGW even with the following
+// reroute to mp0 src-ip route on GR that we add from syncNodeManagementPort:
+// 10.100.200.0/24    10.100.200.2 src-ip
+// the dst-ip based default OVN route takes precedence because the primary nodeCIDR range is a
+// directly attached network to the OVN router and sends the traffic destined for other nodes to rtoe
+// and via br-ex to outside in LGW which is not desired.
+// Hence we need a LRP that sends all traffic destined to that primary nodeCIDR range that reroutes
+// it to mp0 in LGW mode to override this directly attached network OVN route.
+func (pbr *PolicyBasedRoutesManager) AddHostCIDRPolicy(node *v1.Node, mgmtPortIP, clusterPodSubnet string) error {
+	if mgmtPortIP == "" || net.ParseIP(mgmtPortIP) == nil {
+		return fmt.Errorf("invalid management port IP address: %q", mgmtPortIP)
+	}
+	// we only care about the primary node family since GR's port has that IP
+	// we don't care about secondary nodeIPs here which is why we are not using
+	// the hostCIDR annotation
+	primaryIfAddrs, err := util.GetNodeIfAddrAnnotation(node)
+	if err != nil {
+		return fmt.Errorf("failed to get primaryIP for node %s, err: %v", node.Name, err)
+	}
+	nodePrimaryStringPrefix := primaryIfAddrs.IPv4
+	if utilnet.IsIPv6String(mgmtPortIP) {
+		nodePrimaryStringPrefix = primaryIfAddrs.IPv6
+	}
+	_, nodePrimaryCIDRPrefix, err := net.ParseCIDR(nodePrimaryStringPrefix)
+	if nodePrimaryStringPrefix == "" || err != nil || nodePrimaryCIDRPrefix == nil {
+		return fmt.Errorf("invalid host CIDR prefix: prefixString: %q, prefixCIDR: %q, error: %v",
+			nodePrimaryStringPrefix, nodePrimaryCIDRPrefix, err)
+	}
+	ovnPrefix := getIPCIDRPrefix(nodePrimaryCIDRPrefix)
+	matchStr := generateHostCIDRMatch(ovnPrefix, nodePrimaryCIDRPrefix.String(), clusterPodSubnet)
+	if err := pbr.createPolicyBasedRoutes(matchStr, ovntypes.UDNHostCIDRPolicyPriority, mgmtPortIP); err != nil {
+		return fmt.Errorf("failed to add host-cidr policy route '%s' on host %q on %s "+
+			"error: %v", matchStr, node.Name, pbr.clusterRouterName, err)
+	}
+
+	return nil
+}
+
 // This function syncs logical router policies given various criteria
 // This function compares the following ovn-nbctl output:
 
@@ -218,12 +259,16 @@ func (pbr *PolicyBasedRoutesManager) createPolicyBasedRoutes(match, priority, ne
 	return nil
 }
 
-func generateMatch(switchName, ipPrefix, hostIP string) string {
+func generateNodeIPMatch(switchName, ipPrefix, hostIP string) string {
 	return fmt.Sprintf(`inport == "%s%s" && %s.dst == %s /* %s */`, ovntypes.RouterToSwitchPrefix, switchName, ipPrefix, hostIP, switchName)
 }
 
-func getIPPrefix(ip net.IP) string {
-	if utilnet.IsIPv6(ip) {
+func generateHostCIDRMatch(ipPrefix, nodePrimaryCIDRPrefix, clusterPodSubnetPrefix string) string {
+	return fmt.Sprintf(`%s.dst == %s && %s.src == %s`, ipPrefix, nodePrimaryCIDRPrefix, ipPrefix, clusterPodSubnetPrefix)
+}
+
+func getIPCIDRPrefix(cidr *net.IPNet) string {
+	if utilnet.IsIPv6CIDR(cidr) {
 		return "ip6"
 	}
 	return "ip4"