Repository: metallb/metallb
Pull request #1806 (2023-01-26 18:02:04)

FRR Mode: allow overlapping ips in case of vrfs
With FRR, it's fair to have the same ip for different peers if they belong to different vrfs.

liornoy: Maybe we can also add a test:
"duplicate bgp address - same vrf" 
where `mustFail: true` ?
fedepaol: > Maybe we can also add a test to validation_test.go: "duplicate bgp address - same vrf" where `mustFail: true` ?

In theory this is the same as "duplicate bgp address" one (where we have the default vrf), but I can add another one.
mlguerrero12: lgtm

Patch:
diff --git a/internal/bgp/frr/frr.go b/internal/bgp/frr/frr.go
index d80347d1..ff4852cc 100644
--- a/internal/bgp/frr/frr.go
+++ b/internal/bgp/frr/frr.go
@@ -44,8 +44,12 @@ var osHostname = os.Hostname
 
 // sessionName() defines the format of the key of the 'sessions' map in
 // the 'frrState' struct.
-func sessionName(myAddr string, myAsn uint32, addr string, asn uint32) string {
-	return fmt.Sprintf("%d@%s-%d@%s", asn, addr, myAsn, myAddr)
+func sessionName(s session) string {
+	baseName := fmt.Sprintf("%d@%s-%d@%s", s.PeerASN, s.PeerAddress, s.MyASN, s.SourceAddress)
+	if s.VRFName == "" {
+		return baseName
+	}
+	return baseName + "/" + s.VRFName
 }
 
 func validate(adv *bgp.Advertisement) error {
@@ -58,7 +62,7 @@ func validate(adv *bgp.Advertisement) error {
 func (s *session) Set(advs ...*bgp.Advertisement) error {
 	s.sessionManager.Lock()
 	defer s.sessionManager.Unlock()
-	sessionName := sessionName(s.SourceAddress.String(), s.MyASN, s.PeerAddress, s.PeerASN)
+	sessionName := sessionName(*s)
 	if _, found := s.sessionManager.sessions[sessionName]; !found {
 		return fmt.Errorf("session %s not established before advertisement", sessionName)
 	}
@@ -133,7 +137,7 @@ func (sm *sessionManager) addSession(s *session) error {
 	if s == nil {
 		return fmt.Errorf("invalid session")
 	}
-	sessionName := sessionName(s.SourceAddress.String(), s.MyASN, s.PeerAddress, s.PeerASN)
+	sessionName := sessionName(*s)
 	sm.sessions[sessionName] = s
 
 	return nil
@@ -143,7 +147,7 @@ func (sm *sessionManager) deleteSession(s *session) error {
 	if s == nil {
 		return fmt.Errorf("invalid session")
 	}
-	sessionName := sessionName(s.SourceAddress.String(), s.MyASN, s.PeerAddress, s.PeerASN)
+	sessionName := sessionName(*s)
 	delete(sm.sessions, sessionName)
 
 	return nil
diff --git a/internal/config/validation.go b/internal/config/validation.go
index f4fff6f1..982c9633 100644
--- a/internal/config/validation.go
+++ b/internal/config/validation.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	"github.com/pkg/errors"
+	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
 )
 
 type Validate func(ClusterResources) error
@@ -70,15 +71,17 @@ func DiscardNativeOnly(c ClusterResources) error {
 	if len(c.Peers) > 1 {
 		peerAddr := make(map[string]bool)
 		routerID := c.Peers[0].Spec.RouterID
-		peerAddr[c.Peers[0].Spec.Address] = true
+		peer0 := peerAddressKey(c.Peers[0].Spec)
+		peerAddr[peer0] = true
 		for _, p := range c.Peers[1:] {
 			if p.Spec.RouterID != routerID {
 				return fmt.Errorf("peer %s has RouterID different from %s, in FRR mode all RouterID must be equal", p.Spec.RouterID, c.Peers[0].Spec.RouterID)
 			}
-			if _, ok := peerAddr[p.Spec.Address]; ok {
+			peerKey := peerAddressKey(p.Spec)
+			if _, ok := peerAddr[peerKey]; ok {
 				return fmt.Errorf("peer %s already exists, FRR mode doesn't support duplicate BGPPeers", p.Spec.Address)
 			}
-			peerAddr[p.Spec.Address] = true
+			peerAddr[peerKey] = true
 		}
 	}
 	for _, p := range c.Peers {
@@ -92,3 +95,7 @@ func DiscardNativeOnly(c ClusterResources) error {
 	}
 	return nil
 }
+
+func peerAddressKey(peer metallbv1beta2.BGPPeerSpec) string {
+	return fmt.Sprintf("%s-%s", peer.Address, peer.VRFName)
+}


Test patch:
diff --git a/internal/bgp/frr/frr_vrf_test.go b/internal/bgp/frr/frr_vrf_test.go
index 05d15a65..bc17a9ea 100644
--- a/internal/bgp/frr/frr_vrf_test.go
+++ b/internal/bgp/frr/frr_vrf_test.go
@@ -164,6 +164,49 @@ func TestTwoSessionsSameIPVRF(t *testing.T) {
 	testCheckConfigFile(t)
 }
 
+func TestTwoSessionsSameIPRouterIDASNVRF(t *testing.T) {
+	testSetup(t)
+
+	l := log.NewNopLogger()
+	sessionManager := NewSessionManager(l, logging.LevelInfo)
+	defer close(sessionManager.reloadConfig)
+	session1, err := sessionManager.NewSession(l,
+		bgp.SessionParameters{
+			PeerAddress:   "10.2.2.254:179",
+			MyASN:         100,
+			PeerASN:       200,
+			HoldTime:      time.Second,
+			KeepAliveTime: time.Second,
+			Password:      "password",
+			CurrentNode:   "hostname",
+			EBGPMultiHop:  true,
+			SessionName:   "test-peer1"})
+
+	if err != nil {
+		t.Fatalf("Could not create session: %s", err)
+	}
+	defer session1.Close()
+	session2, err := sessionManager.NewSession(l,
+		bgp.SessionParameters{
+			PeerAddress:   "10.2.2.254:179",
+			MyASN:         100,
+			PeerASN:       200,
+			HoldTime:      time.Second,
+			KeepAliveTime: time.Second,
+			Password:      "password",
+			CurrentNode:   "hostname",
+			EBGPMultiHop:  true,
+			SessionName:   "test-peer2",
+			VRFName:       "red"})
+
+	if err != nil {
+		t.Fatalf("Could not create session: %s", err)
+	}
+	defer session2.Close()
+
+	testCheckConfigFile(t)
+}
+
 func TestSingleAdvertisementVRF(t *testing.T) {
 	testSetup(t)
 
diff --git a/internal/bgp/frr/testdata/TestTwoSessionsSameIPRouterIDASNVRF.golden b/internal/bgp/frr/testdata/TestTwoSessionsSameIPRouterIDASNVRF.golden
new file mode 100644
index 00000000..4b64fbae
--- /dev/null
+++ b/internal/bgp/frr/testdata/TestTwoSessionsSameIPRouterIDASNVRF.golden
@@ -0,0 +1,69 @@
+log file /etc/frr/frr.log 
+log timestamp precision 3
+hostname dummyhostname
+ip nht resolve-via-default
+ipv6 nht resolve-via-default
+route-map 10.2.2.254-in deny 20
+
+route-map 10.2.2.254-out permit 1
+  match ip address prefix-list 10.2.2.254-pl-ipv4
+route-map 10.2.2.254-out permit 2
+  match ipv6 address prefix-list 10.2.2.254-pl-ipv4
+
+ip prefix-list 10.2.2.254-pl-ipv4 deny any
+ipv6 prefix-list 10.2.2.254-pl-ipv4 deny any
+route-map 10.2.2.254-red-in deny 20
+
+route-map 10.2.2.254-red-out permit 1
+  match ip address prefix-list 10.2.2.254-red-pl-ipv4
+route-map 10.2.2.254-red-out permit 2
+  match ipv6 address prefix-list 10.2.2.254-red-pl-ipv4
+
+ip prefix-list 10.2.2.254-red-pl-ipv4 deny any
+ipv6 prefix-list 10.2.2.254-red-pl-ipv4 deny any
+
+router bgp 100
+  no bgp ebgp-requires-policy
+  no bgp network import-check
+  no bgp default ipv4-unicast
+
+  neighbor 10.2.2.254 remote-as 200
+  neighbor 10.2.2.254 ebgp-multihop
+  neighbor 10.2.2.254 port 179
+  neighbor 10.2.2.254 timers 1 1
+  neighbor 10.2.2.254 password password
+  
+
+  address-family ipv4 unicast
+    neighbor 10.2.2.254 activate
+    neighbor 10.2.2.254 route-map 10.2.2.254-in in
+    neighbor 10.2.2.254 route-map 10.2.2.254-out out
+  exit-address-family
+  address-family ipv6 unicast
+    neighbor 10.2.2.254 activate
+    neighbor 10.2.2.254 route-map 10.2.2.254-in in
+    neighbor 10.2.2.254 route-map 10.2.2.254-out out
+  exit-address-family
+router bgp 100 vrf red
+  no bgp ebgp-requires-policy
+  no bgp network import-check
+  no bgp default ipv4-unicast
+
+  neighbor 10.2.2.254 remote-as 200
+  neighbor 10.2.2.254 ebgp-multihop
+  neighbor 10.2.2.254 port 179
+  neighbor 10.2.2.254 timers 1 1
+  neighbor 10.2.2.254 password password
+  
+
+  address-family ipv4 unicast
+    neighbor 10.2.2.254 activate
+    neighbor 10.2.2.254 route-map 10.2.2.254-red-in in
+    neighbor 10.2.2.254 route-map 10.2.2.254-red-out out
+  exit-address-family
+  address-family ipv6 unicast
+    neighbor 10.2.2.254 activate
+    neighbor 10.2.2.254 route-map 10.2.2.254-red-in in
+    neighbor 10.2.2.254 route-map 10.2.2.254-red-out out
+  exit-address-family
+
diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go
index cb34a9a1..eaa9fbed 100644
--- a/internal/config/validation_test.go
+++ b/internal/config/validation_test.go
@@ -320,6 +320,52 @@ func TestValidateFRR(t *testing.T) {
 				},
 			},
 			mustFail: true,
+		}, {
+			desc: "duplicate bgp address, different vrfs",
+			config: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							Address: "1.2.3.4",
+						},
+					},
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							Address: "1.2.3.4",
+							VRFName: "red",
+						},
+					}, {
+						Spec: v1beta2.BGPPeerSpec{
+							Address: "1.2.3.4",
+							VRFName: "green",
+						},
+					},
+				},
+			},
+			mustFail: false,
+		}, {
+			desc: "duplicate bgp address, same vrf",
+			config: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							Address: "1.2.3.4",
+						},
+					},
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							Address: "1.2.3.4",
+							VRFName: "red",
+						},
+					}, {
+						Spec: v1beta2.BGPPeerSpec{
+							Address: "1.2.3.4",
+							VRFName: "red",
+						},
+					},
+				},
+			},
+			mustFail: true,
 		},
 	}