diff --git a/protocols/bgp/server/bgp_api_test.go b/protocols/bgp/server/bgp_api_test.go index 44dbe964..4737edbc 100644 --- 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 index 77b12ae5..b7771b84 100644 --- 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 index 9fd8e3e4..6f4253f1 100644 --- 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 index 76d5bf98..483e2928 100644 --- 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 index 432042f5..74ac910f 100644 --- 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, }, }, }),