RouteAdvertisements: fail if DisableMP is unset

If MultiProtocol is enabled (default) then a BGP session
carries prefixes of both IPv4 and IPv6 families. Our problem is
that with an IPv4 session, FRR can incorrectly pick the
masquerade IPv6 address (instead of the real address) as next hop
for IPv6 prefixes and that won't work. Note that with a dedicated
IPv6 session that can't happen since FRR will use the same
address that was used to stablish the session. Let's
enforce the use of DisableMP for now.

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Author: jcaamano

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

@@ -506,13 +506,16 @@ func (c *Controller) generateFRRConfigurations(ra *ratypes.RouteAdvertisements)
 		matchedNetworks := sets.New[string]()
 		for _, frrConfig := range frrConfigs {
 			// generate FRRConfiguration for each source FRRConfiguration/node combination
-			new := c.generateFRRConfiguration(
+			new, err := c.generateFRRConfiguration(
 				ra,
 				frrConfig,
 				nodeName,
 				selectedNetworks,
 				matchedNetworks,
 			)
+			if err != nil {
+				return nil, nil, err
+			}
 			if new == nil {
 				// if we got nil, we didn't match any VRF
 				return nil, nil, fmt.Errorf("%w: FRRConfiguration %q selected for node %q has no VRF matching the RouteAdvertisements target VRF or any selected network",
@@ -538,7 +541,7 @@ func (c *Controller) generateFRRConfiguration(
 	nodeName string,
 	selectedNetworks *selectedNetworks,
 	matchedNetworks sets.Set[string],
-) *frrtypes.FRRConfiguration {
+) (*frrtypes.FRRConfiguration, error) {
 	routers := []frrtypes.Router{}
 	advertisements := sets.New(ra.Spec.Advertisements...)
 
@@ -597,16 +600,30 @@ func (c *Controller) generateFRRConfiguration(
 		targetRouter.Prefixes = advertisePrefixes
 		targetRouter.Neighbors = make([]frrtypes.Neighbor, 0, len(source.Spec.BGP.Routers[i].Neighbors))
 		for _, neighbor := range source.Spec.BGP.Routers[i].Neighbors {
-			advertisePrefixes := advertisePrefixes
-			receivePrefixes := receivePrefixes
-			if neighbor.DisableMP {
-				isIPV6 := utilnet.IsIPv6String(neighbor.Address)
-				advertisePrefixes = util.MatchAllIPNetsStringFamily(isIPV6, advertisePrefixes)
-				receivePrefixes = util.MatchAllIPNetsStringFamily(isIPV6, receivePrefixes)
+			// If MultiProtocol is enabled (default) then a BGP session carries
+			// prefixes of both IPv4 and IPv6 families. Our problem is that with
+			// an IPv4 session, FRR can incorrectly pick the masquerade IPv6
+			// address (instead of the real address) as next hop for IPv6
+			// prefixes and that won't work. Note that with a dedicated IPv6
+			// session that can't happen since FRR will use the same address
+			// that was used to stablish the session. Let's enforce the use of
+			// DisableMP for now.
+			if !neighbor.DisableMP {
+				return nil, fmt.Errorf("%w: DisableMP==false not supported, seen on FRRConfiguration %s/%s neighbor %s",
+					errConfig,
+					source.Namespace,
+					source.Name,
+					neighbor.Address,
+				)
 			}
+
+			isIPV6 := utilnet.IsIPv6String(neighbor.Address)
+			advertisePrefixes := util.MatchAllIPNetsStringFamily(isIPV6, advertisePrefixes)
+			receivePrefixes := util.MatchAllIPNetsStringFamily(isIPV6, receivePrefixes)
 			if len(advertisePrefixes) == 0 {
 				continue
 			}
+
 			neighbor.ToAdvertise = frrtypes.Advertise{
 				Allowed: frrtypes.AllowedOutPrefixes{
 					Mode:     frrtypes.AllowRestricted,
@@ -686,7 +703,7 @@ func (c *Controller) generateFRRConfiguration(
 	}
 	if len(routers) == 0 {
 		// we ended up with no routers, bail out
-		return nil
+		return nil, nil
 	}
 
 	new := &frrtypes.FRRConfiguration{}
@@ -712,7 +729,7 @@ func (c *Controller) generateFRRConfiguration(
 		},
 	}
 
-	return new
+	return new, nil
 }
 
 // updateFRRConfigurations updates the FRRConfigurations that apply for a

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

@@ -22,6 +22,7 @@ import (
 	ctesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
+	"k8s.io/utils/ptr"
 
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 	controllerutil "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/controller"
@@ -102,14 +103,16 @@ func (tn testNode) Node() *corev1.Node {
 type testNeighbor struct {
 	ASN       uint32
 	Address   string
+	DisableMP *bool
 	Receive   []string
 	Advertise []string
 }
 
 func (tn testNeighbor) Neighbor() frrapi.Neighbor {
 	n := frrapi.Neighbor{
-		ASN:     tn.ASN,
-		Address: tn.Address,
+		ASN:       tn.ASN,
+		Address:   tn.Address,
+		DisableMP: true,
 		ToReceive: frrapi.Receive{
 			Allowed: frrapi.AllowedInPrefixes{
 				Mode: frrapi.AllowRestricted,
@@ -122,6 +125,9 @@ func (tn testNeighbor) Neighbor() frrapi.Neighbor {
 			},
 		},
 	}
+	if tn.DisableMP != nil {
+		n.DisableMP = *tn.DisableMP
+	}
 	for _, receive := range tn.Receive {
 		sep := strings.LastIndex(receive, "/")
 		if sep == -1 {
@@ -365,6 +371,39 @@ func TestController_reconcile(t *testing.T) {
 			},
 			expectNADAnnotations: map[string]map[string]string{"default": {types.OvnRouteAdvertisementsKey: "[\"ra\"]"}},
 		},
+		{
+			name: "reconciles dual-stack pod+eip RouteAdvertisement for a single FRR config, node and default network and target VRF",
+			ra:   &testRA{Name: "ra", AdvertisePods: true, AdvertiseEgressIPs: true},
+			frrConfigs: []*testFRRConfig{
+				{
+					Name:      "frrConfig",
+					Namespace: frrNamespace,
+					Routers: []*testRouter{
+						{ASN: 1, Prefixes: []string{"1.1.1.0/24"}, Neighbors: []*testNeighbor{
+							{ASN: 1, Address: "1.0.0.100"},
+							{ASN: 1, Address: "fd02::ffff:100:64"},
+						}},
+					},
+				},
+			},
+			nodes:                []*testNode{{Name: "node", SubnetsAnnotation: "{\"default\":[\"1.1.0.0/24\",\"fd01::/64\"]}"}},
+			eips:                 []*testEIP{{Name: "eipv4", EIPs: map[string]string{"node": "1.0.1.1"}}, {Name: "eipv6", EIPs: map[string]string{"node": "fd03::ffff:100:101"}}},
+			reconcile:            "ra",
+			expectAcceptedStatus: metav1.ConditionTrue,
+			expectFRRConfigs: []*testFRRConfig{
+				{
+					Labels:       map[string]string{types.OvnRouteAdvertisementsKey: "ra"},
+					Annotations:  map[string]string{types.OvnRouteAdvertisementsKey: "ra/frrConfig/node"},
+					NodeSelector: map[string]string{"kubernetes.io/hostname": "node"},
+					Routers: []*testRouter{
+						{ASN: 1, Prefixes: []string{"1.0.1.1/32", "1.1.0.0/24", "fd01::/64", "fd03::ffff:100:101/128"}, Neighbors: []*testNeighbor{
+							{ASN: 1, Address: "1.0.0.100", Advertise: []string{"1.0.1.1/32", "1.1.0.0/24"}, Receive: []string{"1.1.0.0/16/24"}},
+							{ASN: 1, Address: "fd02::ffff:100:64", Advertise: []string{"fd01::/64", "fd03::ffff:100:101/128"}, Receive: []string{"fd01::/48/64"}},
+						}},
+					}},
+			},
+			expectNADAnnotations: map[string]map[string]string{"default": {types.OvnRouteAdvertisementsKey: "[\"ra\"]"}},
+		},
 		{
 			name: "reconciles pod RouteAdvertisement for a single FRR config, node, non default networks and default target VRF",
 			ra:   &testRA{Name: "ra", AdvertisePods: true, NetworkSelector: map[string]string{"selected": "true"}},
@@ -785,6 +824,24 @@ func TestController_reconcile(t *testing.T) {
 			reconcile:            "ra",
 			expectAcceptedStatus: metav1.ConditionFalse,
 		},
+		{
+			name: "fails to reconcile if DisableMP is unset",
+			ra:   &testRA{Name: "ra", AdvertisePods: true},
+			frrConfigs: []*testFRRConfig{
+				{
+					Name:      "frrConfig",
+					Namespace: frrNamespace,
+					Routers: []*testRouter{
+						{ASN: 1, Prefixes: []string{"1.1.1.0/24"}, Neighbors: []*testNeighbor{
+							{ASN: 1, Address: "1.0.0.100", DisableMP: ptr.To(false)},
+						}},
+					},
+				},
+			},
+			nodes:                []*testNode{{Name: "node", SubnetsAnnotation: "{\"default\":\"1.1.0.0/24\"}"}},
+			reconcile:            "ra",
+			expectAcceptedStatus: metav1.ConditionFalse,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -798,6 +855,10 @@ func TestController_reconcile(t *testing.T) {
 					CIDR:             ovntest.MustParseIPNet("1.1.0.0/16"),
 					HostSubnetLength: 24,
 				},
+				{
+					CIDR:             ovntest.MustParseIPNet("fd01::/48"),
+					HostSubnetLength: 64,
+				},
 			}
 			config.OVNKubernetesFeature.EnableMultiNetwork = true
 			config.OVNKubernetesFeature.EnableRouteAdvertisements = true
@@ -885,7 +946,7 @@ func TestController_reconcile(t *testing.T) {
 				g.Expect(err).ToNot(gomega.HaveOccurred())
 				accepted := meta.FindStatusCondition(ra.Status.Conditions, "Accepted")
 				g.Expect(accepted).NotTo(gomega.BeNil())
-				g.Expect(accepted.Status).To(gomega.Equal(tt.expectAcceptedStatus))
+				g.Expect(accepted.Status).To(gomega.Equal(tt.expectAcceptedStatus), accepted.Message)
 			}
 
 			// verify FRRConfigurations have been created/updated/deleted as expected