Add peer selector to BGPAdvertisement object #1171

sabinaaledort · 2022-01-18T11:38:40Z

A service using an IP from a pool with a list of bgp-peers will not advertise the IP to peers outside that list.

In order to be able to select a specific set of peers for a given pool we need to:

Add a name field to the Peer structure so it can be selected by an AddressPool.
Add an optional list of bgp-peers, each one with its own (optional) node selector.

fedepaol · 2022-01-18T11:43:00Z

I think it's worth waiting until #942 settles, it may have substantial impacts on this.

fedepaol · 2022-04-08T16:23:20Z

internal/bgp/frr/config.go

  address-family ipv6 unicast
    neighbor {{$n.Addr}} activate
    neighbor {{$n.Addr}} route-map {{$n.Addr}}-in in
+    neighbor {{$n.Addr}} route-map {{$n.Addr}}-out out


we have this below already

It's only added if the neighbor has advertisements. Not setting a route-map for a neighbor without advertisements advertises all ips to that neighbor, while setting it for the neighbor without actually creating it is doing the filtering.

internal/bgp/frr/frr.go

fedepaol · 2022-04-08T16:40:01Z

internal/bgp/native/native.go

-		err := validate(adv)
-		if err != nil {
-			return err
+		addAdv := true


Same as above

fedepaol · 2022-04-08T17:08:24Z

e2etest/bgptests/bgp.go

+		table.DescribeTable("A service IP will not be advertised to peers outside the BGPAdvertisement peers list", func(addressRange string, ipFamily ipfamily.Family) {
+			var testContainers []*frrcontainer.FRR
+			for _, c := range FRRContainers {
+				if !strings.Contains(c.Name, "multi") {


Why this? If you want to pick just two, be explicit and take the first two of the array (after the len check)

This is still unresolved I think?

fedepaol · 2022-04-08T17:11:00Z

e2etest/bgptests/bgp.go

+				}
+			}
+		},
+			table.Entry("IPV4", "192.168.10.0/24", ipfamily.IPv4),


I would add a test where you have the same service, two different advertisements advertised to two different peers (you can check the community for example)

fedepaol · 2022-04-08T17:13:56Z

internal/config/config.go


+	if len(crdAd.Spec.Peers) > 0 {
+		ad.Peers = make([]string, 0, len(crdAd.Spec.Peers))
+		ad.Peers = append(ad.Peers, crdAd.Spec.Peers...)


Can you extend the unit tests with this?

what do you mean?

I mean, this part is not covered by unit tests (I think).

fedepaol · 2022-04-08T17:16:15Z

Looks good overall, need to do another pass and focus on the frr config.

fedepaol · 2022-04-12T15:36:02Z

internal/bgp/frr/config.go

 route-map {{$n.Addr}}-out permit {{counter $n.Addr}}
-  match ip address prefix-list {{.LocalPreference}}-v4localpref-prefixes
-  set local-preference {{.LocalPreference}}
+  match ip address prefix-list {{$n.Addr}}-{{$pl.LocalPreference}}-v4localpref-prefixes


having a prefix per addr per local pref is gonna make the configuration explode. How about a custom function isPrefixValidForNeighbor that checks that (all the advertisements configured for the neigh) and then we add the route map or not? Same for community.

fedepaol · 2022-04-12T15:57:57Z

internal/bgp/frr/config.go

 {{- end }}
-{{- range $.PrefixesV4ForCommunity }}
+{{- if eq ($a.IPFamily.String) "ipv4" }}
+ip prefix-list {{$n.Addr}}-plv4 permit {{$a.Prefix}}


Here we create multiple records of the shape of:

route-map 10.2.2.255-out permit 2 match ip address prefix-list 10.2.2.255-plv4 on-match next route-map 10.2.2.255-out permit 5 match ip address prefix-list 10.2.2.255-plv4 on-match next

I think building one prefix-list with all the addresses and doing the route map once will be more readable, and will add probably less churn on frr (because all the route maps are different, so I don't think it's gonna squash them).

I'd suggest building such list in go and adding to the structure, so you can just leverage it.

fedepaol · 2022-04-12T15:59:21Z

internal/bgp/frr/config.go

  address-family ipv4 unicast
    neighbor {{$n.Addr}} activate
    neighbor {{$n.Addr}} route-map {{$n.Addr}}-in in
+	neighbor {{$n.Addr}} route-map {{$n.Addr}}-out out


do we need this if we don't have any advertisement?

Yes, if the neighbor doesn't have a route-map all ips are advertised to that neighbor, while setting it for the neighbor without actually creating it, is doing the filtering.

fedepaol · 2022-04-12T16:02:38Z

internal/bgp/frr/frr.go

+				found := false
+				for _, peer := range adv.Peers {
+					if peer == s.name {
+						found = true


nit: you can break here

fedepaol · 2022-04-12T16:03:00Z

internal/bgp/native/native.go

+			found := false
+			for _, peer := range adv.Peers {
+				if peer == s.name {
+					found = true


fedepaol · 2022-04-12T17:32:27Z

e2etest/bgptests/bgp.go

+				err = ConfigUpdater.Update(resources)
+				framework.ExpectNoError(err)
+
+				for _, c := range FRRContainers {


See my suggestoin above to pick named peers.
For the notAdvertised, I would add a function that filters FRRContainers out of a container and iterate over those.

fedepaol · 2022-04-12T17:34:59Z

e2etest/bgptests/bgp.go

+			}
+
+			ginkgo.By("checking same service advertised through two different advertisements to two different peers")
+			for i := 0; i < 2; i++ {


Same considerations as above, following by index is hard. Be explicit on naming.
One pool, PeerForAdv1 , PeerForAdv2, etc

Also, this should be a different test and not a by in the same. It's already long.

fedepaol · 2022-04-12T17:37:00Z

e2etest/bgptests/bgp.go

+				}
+			}
+
+			for i := 0; i < 2; i++ {


Same as above, avoid indexes, give peers a name

fedepaol · 2022-04-12T17:39:17Z

e2etest/bgptests/validate.go

+	}
+}
+
+func validateServiceInRoutesForCommunity(c *frrcontainer.FRR, community string, family ipfamily.Family, svc *corev1.Service) error {


Put the eventually inside here, it will make the test body lighter

i use this function twice, once the eventually expects no error, the other time it expects an error.

You can wrap it with two functinos. Also, you may want to check that the error is what we expect with MatchError(ContainSubstring("not in routes")

fedepaol · 2022-04-12T17:40:11Z

e2etest/bgptests/validate.go

+		ingressIP := e2eservice.GetIngressPoint(&ip)
+
+		Eventually(func() bool {
+			frrRoutesV4, frrRoutesV6, err := frr.Routes(c)


you can use frr.RoutesForFamily here (it's new)

fedepaol · 2022-04-13T15:27:24Z

internal/bgp/frr/config.go

+{{- range $a := .Advertisements }}
+{{- range $pl := prefixesForLocalPref $a $n.IPFamily.String }}
+{{- range $p := $pl.Prefixes }}
+{{ipCmd $n.IPFamily.String}} prefix-list {{$n.Addr}}-{{$pl.LocalPreference}}-{{ipCmd $n.IPFamily.String}}-localpref-prefixes permit {{$p}}


ipCmd can take IPFamily instead of string

also, how about renaming ipCmd to frrIPFamily?

fedepaol · 2022-04-13T15:35:59Z

internal/bgp/frr/config.go

 route-map {{$n.Addr}}-out permit {{counter $n.Addr}}
-  match ipv6 address prefix-list {{.LocalPreference}}-v6localpref-prefixes
-  set local-preference {{.LocalPreference}}
+  match {{ipCmd $n.IPFamily.String}} address prefix-list {{$n.Addr}}-{{$pl.LocalPreference}}-{{ipCmd $n.IPFamily.String}}-localpref-prefixes


I wonder if we should hide names like
{{$n.Addr}}-{{$pl.LocalPreference}}-{{ipCmd $n.IPFamily.String}}-localpref-prefixes behind a function that takes the neighbour and the local pref.
Something like localPrefPrefixList (neighbour, localpref)

Same for the others: communityPrefixListName() , allowedPrefixListName.
Wdyt?

fedepaol · 2022-04-13T15:40:38Z

internal/bgp/frr/config.go

+				}
+				return "ip"
+			},
+			"prefixesForLocalPref": func(advConfig advertisementConfig, ipFamily string) []localPrefPrefixes {


This is more "localPrefPrefixesForAdv". Also, can't you use the ipfamily in advertisementConfig?

i need the neighbor ip family to avoid mixing ipv4 prefixes with neighbor ipv6 addresses

which is something we do not support today right? So that can be solved by filtering when we build the adv list for neighbour?

fedepaol · 2022-04-13T15:40:46Z

internal/bgp/frr/config.go

+				}
+				return advConfig.PrefixesV6ForLocalPref
+			},
+			"prefixesForCommunity": func(advConfig advertisementConfig, ipFamily string) []communityPrefixes {


fedepaol · 2022-04-13T15:46:23Z

internal/bgp/frr/frr.go

 				addPrefixForLocalPref(prefixesV4ForLocalPref, prefixesV6ForLocalPref, adv.Prefix.String(), family, adv.LocalPref)
 			}

+			// The maps must be converted in sorted slice to make the rendering stable


this can be embedded in the advertisementConfig literal init above (same for prefixesV4ForLocalPref, moving the addPrefixForLocalPref before the init)

fedepaol · 2022-04-13T16:22:44Z

e2etest/bgptests/bgppeer_selector.go

+
+			ginkgo.By(fmt.Sprintf("setting peer selector for addresspool number 1 to peer %s", peerForAdv1.Name))
+			peer := ""
+			for _, p := range resources.Peers {


This is repeating and distracting. How about initializing peer1 / peer2 in beforeEach (and peers: metallb.PeersForContainers(FRRContainers, ipFamily))?

I'd rename peerForAdv1 to something else too, because we must distinguish between peer - the container and peer - the BGPPeer we give to metallb

we need the ipfamily for PeersForContainers which we don't have in before each

oh right. Wrapping it in a function?

fedepaol · 2022-04-13T16:23:35Z

e2etest/bgptests/bgppeer_selector.go

+					ginkgo.By(fmt.Sprintf("checking service in routes of peer %s for community 1", c.Name))
+					Eventually(func() error {
+						return validateServiceInRoutesForCommunity(c, community1, ipFamily, svc)
+					}, 4*time.Minute, 1*time.Second).Should(BeNil())


With errors we use Should(HaveOccurred)

A service using an IP from a pool with a list of bgp-peers will not advertise the IP to peers outside that list.

A service IP from a pool with a list of bgp-peers will not be advertised to peers outside that list.

Need to fix the test config files due to the change in the frr config to support the new BGP Peer Selector.

fedepaol · 2022-04-14T12:24:09Z

LGTM
@cgoncalves would you mind giving a look at the config part?

cgoncalves · 2022-04-14T13:57:50Z

LGTM @cgoncalves would you mind giving a look at the config part?

LGTM. Just a few nits I spotted, I wouldn't take issue merging this PR without addressing them.

fedepaol · 2022-04-14T14:02:55Z

LGTM @cgoncalves would you mind giving a look at the config part?

LGTM. Just a few nits I spotted, I wouldn't take issue merging this PR without addressing them.

Ack, let's discuss them offline so we can follow up

sabinaaledort requested review from fedepaol, johananl, rata and russellb as code owners January 18, 2022 11:38

sabinaaledort changed the title ~~Bgp peer selector~~ Add BGP Peer Selector to AddressPool object Jan 18, 2022

sabinaaledort changed the title ~~Add BGP Peer Selector to AddressPool object~~ WIP: Add BGP Peer Selector to AddressPool object Jan 18, 2022

sabinaaledort force-pushed the bgp_peer_selector branch from da90110 to 2b6dd7f Compare January 18, 2022 11:40

sabinaaledort force-pushed the bgp_peer_selector branch 2 times, most recently from 0be9247 to 2c53451 Compare January 18, 2022 12:46

sabinaaledort force-pushed the bgp_peer_selector branch 5 times, most recently from 335e246 to 17b13e6 Compare April 6, 2022 15:10

sabinaaledort force-pushed the bgp_peer_selector branch 6 times, most recently from d6499c0 to 5c04fda Compare April 7, 2022 14:53

fedepaol reviewed Apr 8, 2022

View reviewed changes

sabinaaledort force-pushed the bgp_peer_selector branch 4 times, most recently from cf16b56 to 2b98cc6 Compare April 10, 2022 13:28

sabinaaledort changed the title ~~WIP: Add BGP Peer Selector to AddressPool object~~ Add BGP Peer Selector to AddressPool object Apr 10, 2022

sabinaaledort force-pushed the bgp_peer_selector branch 2 times, most recently from 7cf80c7 to e61780e Compare April 11, 2022 14:56

sabinaaledort force-pushed the bgp_peer_selector branch from b2986a3 to 4090676 Compare April 12, 2022 10:47

fedepaol reviewed Apr 12, 2022

View reviewed changes

sabinaaledort force-pushed the bgp_peer_selector branch 5 times, most recently from 8347a48 to 1c64bfe Compare April 13, 2022 15:09

fedepaol reviewed Apr 13, 2022

View reviewed changes

sabinaaledort changed the title ~~Add BGP Peer Selector to AddressPool object~~ Add peer selector to BGPAdvertisement object Apr 13, 2022

fedepaol reviewed Apr 13, 2022

View reviewed changes

sabinaaledort force-pushed the bgp_peer_selector branch 4 times, most recently from 1a0f766 to 19d6c2c Compare April 14, 2022 09:30

sabinaaledort added 6 commits April 14, 2022 14:01

Add peer selector to BGPAdvertisement object

5fc8d5c

A service using an IP from a pool with a list of bgp-peers will not advertise the IP to peers outside that list.

Manifests: Add Peers selection to BGPAdvertisement object

35a68cb

Add E2E tests to verify the functionality of BGP Peer Selector

34f559d

A service IP from a pool with a list of bgp-peers will not be advertised to peers outside that list.

Helm: Add Peers selection to BGPAdvertisement object

48790fc

Unit tests: Fix FRR testdata

009621f

Need to fix the test config files due to the change in the frr config to support the new BGP Peer Selector.

Unit tests: Add tests to validate BGPAdvertisement peer selector

3d934fd

sabinaaledort force-pushed the bgp_peer_selector branch from 19d6c2c to 3d934fd Compare April 14, 2022 11:01

fedepaol merged commit f7a062f into metallb:main Apr 14, 2022

This was referenced Apr 20, 2022

Limit announcement of address pools to specific BGP peers #596

Closed

BGP peers per address-pool #521

Closed

cprivitere mentioned this pull request Oct 12, 2022

Add MetalLB 0.13 support kubernetes-sigs/cloud-provider-equinix-metal#322

Closed

eyenx mentioned this pull request Oct 19, 2022

chore(misc/metallb): update to 0.13.x adfinis/helm-charts#834

Merged

6 tasks

Add peer selector to BGPAdvertisement object #1171

Add peer selector to BGPAdvertisement object #1171

Uh oh!

Conversation

sabinaaledort commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fedepaol commented Jan 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedepaol commented Apr 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabinaaledort commented Jan 18, 2022 •

edited

Loading