Merge pull request #4946 from jcaamano/fix-disableMP

RouteAdvertisements: fail if DisableMP is unset

Author: trozet

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