Default local-pref to 100 on eBGP paths w/o local-pref set

Signed-off-by: Maximilian Wilhelm <[email protected]>
bio-routing · Sep 5, 2023 · 0e48eb2 · 0e48eb2
1 parent e887d89
commit 0e48eb2
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 27 deletions.
diff --git a/protocols/bgp/server/bgp_api_test.go b/protocols/bgp/server/bgp_api_test.go
@@ -30,6 +30,8 @@ func TestDumpRIBInOut(t *testing.T) {
 	sessionAttrs := routingtable.SessionAttrs{
 		RouterID:  0,
 		ClusterID: 0,
+		LocalASN:  42,
+		PeerASN:   42,
 		AddPathRX: true,
 		AddPathTX: true,
 	}

diff --git a/protocols/bgp/server/fsm_address_family.go b/protocols/bgp/server/fsm_address_family.go
@@ -139,7 +139,8 @@ func (f *fsmAddressFamily) getSessionAttrs() routingtable.SessionAttrs {
 		RouterIP: rip,
 	}
 
-	// Only set Default Local Preference for BGP and when peer is fully set up (may not be the case in tests, to be fixed)
+	// Only set Default Local Preference for BGP and when peer is fully set up (may not be the case in all tests)
+	// Worry not, if Local Preference is 0, AdjRIBIn, will default it to 100.
 	if !f.fsm.isBMP && f.fsm.peer.server != nil {
 		sa.DefaultLocalPreference = *f.fsm.peer.server.config.DefaultLocalPreference
 	}

diff --git a/routingtable/adjRIBIn/adj_rib_in.go b/routingtable/adjRIBIn/adj_rib_in.go
@@ -30,6 +30,14 @@ func New(exportFilterChain filter.Chain, vrf *vrf.VRF, sessionAttrs routingtable
 		vrf:               vrf,
 		sessionAttrs:      sessionAttrs,
 	}
+
+	// RFC4277 Section 8. states that 100 is a - if not the - common value for Local
+	// Preference on eBGP sessions, where it would be 0 if none is set.
+	// If no value was specified via the BGP server config, default to 100 here.
+	if a.sessionAttrs.DefaultLocalPreference == 0 {
+		a.sessionAttrs.DefaultLocalPreference = 100
+	}
+
 	a.clientManager = routingtable.NewClientManager(a)
 	return a
 }
@@ -176,17 +184,19 @@ func (a *AdjRIBIn) addPath(pfx *net.Prefix, p *route.Path) error {
 		return nil
 	}
 
+	// RFC4277 Sect 8. suggest to set a  use Local Preference as default value for eBGP
+	// This needs to happen before policies are applied, so this has effect when doing
+	// relative changes to Local Preference.
+	if !a.sessionAttrs.IBGP && p.BGPPath.BGPPathA.LocalPref == 0 {
+		p.BGPPath.BGPPathA.LocalPref = a.sessionAttrs.DefaultLocalPreference
+	}
+
 	p, reject := a.exportFilterChain.Process(pfx, p)
 	if reject {
 		p.HiddenReason = route.HiddenReasonFilteredByPolicy
 		return nil
 	}
 
-	// RFC4277 Sect 8. suggest to set a  use Local Preference as default value for eBGP
-	if !a.sessionAttrs.IBGP && p.BGPPath.BGPPathA.LocalPref == 0 {
-		p.BGPPath.BGPPathA.LocalPref = a.sessionAttrs.DefaultLocalPreference
-	}
-
 	for _, client := range a.clientManager.Clients() {
 		client.AddPath(pfx, p)
 	}

diff --git a/routingtable/adjRIBIn/adj_rib_in_test.go b/routingtable/adjRIBIn/adj_rib_in_test.go
@@ -61,16 +61,14 @@ func TestAddPath(t *testing.T) {
 			},
 		},
 		{
-			name: "Add route (eBGP)",
+			name: "Add route (eBGP, w/o local pref)",
 			iBGP: false,
 			routes: []*route.Route{
 				route.NewRoute(net.NewPfx(net.IPv4FromOctets(10, 0, 0, 0), 8).Ptr(), &route.Path{
 					Type: route.BGPPathType,
 					BGPPath: &route.BGPPath{
-						ASPath: types.NewASPath([]uint32{13335}),
-						BGPPathA: &route.BGPPathA{
-							LocalPref: 100,
-						},
+						ASPath:   types.NewASPath([]uint32{13335}),
+						BGPPathA: &route.BGPPathA{},
 					},
 				}),
 			},

diff --git a/tests/redistribution_test.go b/tests/redistribution_test.go
@@ -88,10 +88,10 @@ func TestRedistribute(t *testing.T) {
 						ASPath:    types.NewASPath([]uint32{201701}),
 						ASPathLen: 1,
 						BGPPathA: &route.BGPPathA{
-							Source:  &peerAIP,
-							NextHop: &peerAIP,
-							EBGP:    true,
-							// FIXME? LocalPref: 100,
+							Source:    &peerAIP,
+							NextHop:   &peerAIP,
+							EBGP:      true,
+							LocalPref: 100,
 						},
 					},
 				}),
@@ -103,10 +103,10 @@ func TestRedistribute(t *testing.T) {
 						ASPath:    types.NewASPath([]uint32{57165, 201701}),
 						ASPathLen: 2,
 						BGPPathA: &route.BGPPathA{
-							Source:  &peerAIP,
-							NextHop: &localBIP,
-							EBGP:    true,
-							// FIXME? LocalPref: 100,
+							Source:    &peerAIP,
+							NextHop:   &localBIP,
+							EBGP:      true,
+							LocalPref: 100,
 						},
 					},
 				}),
@@ -269,10 +269,10 @@ func TestRedistribute(t *testing.T) {
 						ASPath:    types.NewASPath([]uint32{201701}),
 						ASPathLen: 1,
 						BGPPathA: &route.BGPPathA{
-							Source:  &peerAIP,
-							NextHop: &peerAIP,
-							EBGP:    true,
-							// FIXME? LocalPref: 100,
+							Source:    &peerAIP,
+							NextHop:   &peerAIP,
+							EBGP:      true,
+							LocalPref: 100,
 						},
 					},
 				}),
@@ -290,10 +290,10 @@ func TestRedistribute(t *testing.T) {
 						ASPath:    types.NewASPath([]uint32{57165, 201701}),
 						ASPathLen: 2,
 						BGPPathA: &route.BGPPathA{
-							Source:  &peerAIP,
-							NextHop: &localBIP,
-							EBGP:    true,
-							// FIXME? LocalPref: 100,
+							Source:    &peerAIP,
+							NextHop:   &localBIP,
+							EBGP:      true,
+							LocalPref: 100,
 						},
 					},
 				}),