Repository: metallb/metallb
Pull request #2580 (2024-11-05 13:10:55)

 ffr-k8s: Make sure no duplicates prefix are passed to backend
 /kind bug


**What this PR does / why we need it**:
when we enter 
```
---
apiVersion: metallb.io/v1beta2
kind: BGPPeer
metadata:
  name: bgp-peer1v4
  namespace: metallb-system
spec:
  disableMP: false
  holdTime: 0s
  keepaliveTime: 0s
  myASN: 64500
  password: bgp-test
  passwordSecret: {}
  peerASN: 64500
  peerAddress: 10.46.186.88
  peerPort: 179
---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
  annotations:
    metallb.universe.tf/address-pool: address-pools1
  name: address-pools1
  namespace: metallb-system
spec:
  addresses:
  - |
    4.4.4.100 - 4.4.4.101
  autoAssign: true
  avoidBuggyIPs: false
---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  name: bgpadvertisement
  namespace: metallb-system
spec:
  aggregationLength: 32
  aggregationLengthV6: 128
  communities:
  - 500:500
  ipAddressPools:
  - address-pools1
  localPref: 100
---
apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  name: bgpadvertisement2
  namespace: metallb-system
spec:
  aggregationLength: 32
  aggregationLengthV6: 128
  communities:
  - 65535:65282
  ipAddressPools:
  - address-pools1
  localPref: 100

```
the frrconfiguration has duplicates ip prefix entries
**Special notes for your reviewer**:

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. Follow the instructions for writing a release note from k8s: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If no release note is required, just write "NONE".
-->

```release-note
 ffr-k8s: Make sure no duplicates prefix are passed to backend
```



Patch:
diff --git a/internal/bgp/frrk8s/frrk8s.go b/internal/bgp/frrk8s/frrk8s.go
index 68f8616f..b0d9cf1c 100644
--- a/internal/bgp/frrk8s/frrk8s.go
+++ b/internal/bgp/frrk8s/frrk8s.go
@@ -308,6 +308,7 @@ func (sm *sessionManager) updateConfig() error {
 		}
 		neighbor.ToAdvertise.PrefixesWithCommunity = toAdvertiseWithCommunity(prefixesForCommunity)
 		neighbor.ToAdvertise.PrefixesWithLocalPref = toAdvertiseWithLocalPref(prefixesForLocalPref)
+		neighbor.ToAdvertise.Allowed.Prefixes = removeDuplicates(neighbor.ToAdvertise.Allowed.Prefixes)
 		sort.Strings(neighbor.ToAdvertise.Allowed.Prefixes)
 
 		rout.neighbors[neighborName] = neighbor
@@ -363,7 +364,7 @@ func toAdvertiseWithCommunity(prefixesForCommunity map[string][]string) []frrv1b
 	res := []frrv1beta1.CommunityPrefixes{}
 	for c, prefixes := range prefixesForCommunity {
 		sort.Strings(prefixes)
-		res = append(res, frrv1beta1.CommunityPrefixes{Community: c, Prefixes: prefixes})
+		res = append(res, frrv1beta1.CommunityPrefixes{Community: c, Prefixes: removeDuplicates(prefixes)})
 	}
 	sort.Slice(res, func(i, j int) bool {
 		return res[i].Community < res[j].Community
@@ -371,11 +372,24 @@ func toAdvertiseWithCommunity(prefixesForCommunity map[string][]string) []frrv1b
 	return res
 }
 
+func removeDuplicates(input []string) []string {
+	seen := make(map[string]struct{})
+	result := []string{}
+
+	for _, value := range input {
+		if _, ok := seen[value]; !ok {
+			seen[value] = struct{}{}
+			result = append(result, value)
+		}
+	}
+	return result
+}
+
 func toAdvertiseWithLocalPref(prefixesForLocalPref map[uint32][]string) []frrv1beta1.LocalPrefPrefixes {
 	res := []frrv1beta1.LocalPrefPrefixes{}
 	for p, prefixes := range prefixesForLocalPref {
 		sort.Strings(prefixes)
-		res = append(res, frrv1beta1.LocalPrefPrefixes{LocalPref: p, Prefixes: prefixes})
+		res = append(res, frrv1beta1.LocalPrefPrefixes{LocalPref: p, Prefixes: removeDuplicates(prefixes)})
 	}
 	sort.Slice(res, func(i, j int) bool {
 		return res[i].LocalPref < res[j].LocalPref


Test patch:
diff --git a/internal/bgp/frrk8s/frrk8s_test.go b/internal/bgp/frrk8s/frrk8s_test.go
index 6bd69044..a7cafdd4 100644
--- a/internal/bgp/frrk8s/frrk8s_test.go
+++ b/internal/bgp/frrk8s/frrk8s_test.go
@@ -506,6 +506,59 @@ func TestSingleAdvertisement(t *testing.T) {
 	testCheckConfigFile(t)
 }
 
+func TestTwoAdvertisementsWithSamePrefix(t *testing.T) {
+	l := log.NewNopLogger()
+	sessionManager := newTestSessionManager(t)
+	session, err := sessionManager.NewSession(l,
+		bgp.SessionParameters{
+			PeerAddress:   "10.2.2.254:179",
+			SourceAddress: net.ParseIP("10.1.1.254"),
+			MyASN:         100,
+			RouterID:      net.ParseIP("10.1.1.254"),
+			PeerASN:       200,
+			HoldTime:      ptr.To(time.Second),
+			KeepAliveTime: ptr.To(time.Second),
+			Password:      "password",
+			CurrentNode:   "hostname",
+			EBGPMultiHop:  true,
+			SessionName:   "test-peer"})
+	if err != nil {
+		t.Fatalf("Could not create session: %s", err)
+	}
+	defer session.Close()
+
+	prefix := &net.IPNet{
+		IP:   net.ParseIP("172.16.1.10"),
+		Mask: classCMask,
+	}
+	communities := []community.BGPCommunity{}
+	community1, _ := community.New("1111:2222")
+	communities = append(communities, community1)
+	community2, _ := community.New("3333:4444")
+	communities = append(communities, community2)
+	adv := &bgp.Advertisement{
+		Prefix:      prefix,
+		Communities: communities,
+		LocalPref:   300,
+		Peers:       []string{"peer1"},
+	}
+	adv2 := &bgp.Advertisement{
+		Prefix:      prefix,
+		Communities: communities,
+		LocalPref:   300,
+	}
+	advs := []*bgp.Advertisement{adv, adv2}
+	rand.Shuffle(len(advs), func(i, j int) {
+		advs[i], advs[j] = advs[j], advs[i]
+	})
+	err = session.Set(advs[0], advs[1])
+	if err != nil {
+		t.Fatalf("Could not advertise prefix: %s", err)
+	}
+
+	testCheckConfigFile(t)
+}
+
 func TestSingleAdvertisementNoRouterID(t *testing.T) {
 	l := log.NewNopLogger()
 	sessionManager := newTestSessionManager(t)
diff --git a/internal/bgp/frrk8s/testdata/TestTwoAdvertisementsWithSamePrefix.golden b/internal/bgp/frrk8s/testdata/TestTwoAdvertisementsWithSamePrefix.golden
new file mode 100644
index 00000000..33da5863
--- /dev/null
+++ b/internal/bgp/frrk8s/testdata/TestTwoAdvertisementsWithSamePrefix.golden
@@ -0,0 +1,71 @@
+{
+    "metadata": {
+        "name": "metallb-testnodename",
+        "namespace": "testnamespace",
+        "creationTimestamp": null
+    },
+    "spec": {
+        "bgp": {
+            "routers": [
+                {
+                    "asn": 100,
+                    "id": "10.1.1.254",
+                    "neighbors": [
+                        {
+                            "asn": 200,
+                            "address": "10.2.2.254",
+                            "port": 179,
+                            "password": "password",
+                            "passwordSecret": {},
+                            "holdTime": "1s",
+                            "keepaliveTime": "1s",
+                            "ebgpMultiHop": true,
+                            "toAdvertise": {
+                                "allowed": {
+                                    "prefixes": [
+                                        "172.16.1.10/24"
+                                    ]
+                                },
+                                "withLocalPref": [
+                                    {
+                                        "prefixes": [
+                                            "172.16.1.10/24"
+                                        ],
+                                        "localPref": 300
+                                    }
+                                ],
+                                "withCommunity": [
+                                    {
+                                        "prefixes": [
+                                            "172.16.1.10/24"
+                                        ],
+                                        "community": "1111:2222"
+                                    },
+                                    {
+                                        "prefixes": [
+                                            "172.16.1.10/24"
+                                        ],
+                                        "community": "3333:4444"
+                                    }
+                                ]
+                            },
+                            "toReceive": {
+                                "allowed": {}
+                            }
+                        }
+                    ],
+                    "prefixes": [
+                        "172.16.1.10/24"
+                    ]
+                }
+            ]
+        },
+        "raw": {},
+        "nodeSelector": {
+            "matchLabels": {
+                "kubernetes.io/hostname": "testnodename"
+            }
+        }
+    },
+    "status": {}
+}
\ No newline at end of file