Skip to content

Commit

Permalink
Move SelectionOptions and UseMultiplePath to server-local vars
Browse files Browse the repository at this point in the history 
In destination.go there were two global variables used for controlling
path and multi-path support for BGP servers, which were populated and
modified by gRPC/public API calls. In scenarios where multiple BGP
servers are running using the public API it would mean that the last
server created would override the options for all running servers. It
also lead to data races (Confirmed by using Go's data race detector)
that could crash Go's runtime as the value would be overridden
unexpectedly.

To resolve this, both options have been moved into the TableManager
itself which takes a pointer option to both structs populated via the
StartBgp API call. These are then passed to the internal methods as new
function parameters, ensuring that every server has a unique copy of
these structs.

All tests were updated to conform to the new API.
  • Loading branch information
@dawn-minion
dawn-minion committed Oct 3, 2023
1 parent 146b2b8 commit 00d297c
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 110 deletions.
45 changes: 21 additions & 24 deletions internal/pkg/table/destination.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,6 @@ import (
"github.com/osrg/gobgp/v3/pkg/packet/bgp"
)

var SelectionOptions oc.RouteSelectionOptionsConfig
var UseMultiplePaths oc.UseMultiplePathsConfig

type BestPathReason uint8

const (
Expand Down Expand Up @@ -228,7 +225,7 @@ func (dd *Destination) GetMultiBestPath(id string) []*Path {
//
// Modifies destination's state related to stored paths. Removes withdrawn
// paths from known paths. Also, adds new paths to known paths.
func (dest *Destination) Calculate(logger log.Logger, newPath *Path) *Update {
func (dest *Destination) Calculate(logger log.Logger, newPath *Path, selectionOptions *oc.RouteSelectionOptionsConfig) *Update {
oldKnownPathList := make([]*Path, len(dest.knownPathList))
copy(oldKnownPathList, dest.knownPathList)

Expand All @@ -255,7 +252,7 @@ func (dest *Destination) Calculate(logger log.Logger, newPath *Path) *Update {
}
}
// Compute new best path
dest.computeKnownBestPath()
dest.computeKnownBestPath(selectionOptions)

l := make([]*Path, len(dest.knownPathList))
copy(l, dest.knownPathList)
Expand Down Expand Up @@ -344,8 +341,8 @@ func (dest *Destination) implicitWithdraw(logger log.Logger, newPath *Path) {
}
}

func (dest *Destination) computeKnownBestPath() (*Path, BestPathReason, error) {
if SelectionOptions.DisableBestPathSelection {
func (dest *Destination) computeKnownBestPath(selectionOptions *oc.RouteSelectionOptionsConfig) (*Path, BestPathReason, error) {
if selectionOptions.DisableBestPathSelection {
return nil, BPR_DISABLED, nil
}

Expand All @@ -366,7 +363,7 @@ func (dest *Destination) computeKnownBestPath() (*Path, BestPathReason, error) {
}
return dest.knownPathList[0], BPR_ONLY_PATH, nil
}
reason := dest.sort()
reason := dest.sort(selectionOptions)
newBest := dest.knownPathList[0]
// If the first path has the invalidated next-hop, which evaluated by IGP,
// returns no path with the reason of the next-hop reachability.
Expand All @@ -376,7 +373,7 @@ func (dest *Destination) computeKnownBestPath() (*Path, BestPathReason, error) {
return newBest, reason, nil
}

func (dst *Destination) sort() BestPathReason {
func (dst *Destination) sort(selectionOptions *oc.RouteSelectionOptionsConfig) BestPathReason {
reason := BPR_UNKNOWN

sort.SliceStable(dst.knownPathList, func(i, j int) bool {
Expand Down Expand Up @@ -438,15 +435,15 @@ func (dst *Destination) sort() BestPathReason {
reason = BPR_LOCAL_ORIGIN
}
if better == nil {
better = compareByASPath(path1, path2)
better = compareByASPath(path1, path2, selectionOptions)
reason = BPR_ASPATH
}
if better == nil {
better = compareByOrigin(path1, path2)
reason = BPR_ORIGIN
}
if better == nil {
better = compareByMED(path1, path2)
better = compareByMED(path1, path2, selectionOptions)
reason = BPR_MED
}
if better == nil {
Expand All @@ -457,11 +454,11 @@ func (dst *Destination) sort() BestPathReason {
// compareByIGPCost was a no-op and was removed.

if better == nil {
better = compareByAge(path1, path2)
better = compareByAge(path1, path2, selectionOptions)
reason = BPR_OLDER
}
if better == nil {
better, _ = compareByRouterID(path1, path2)
better, _ = compareByRouterID(path1, path2, selectionOptions)
reason = BPR_ROUTER_ID
}
if better == nil {
Expand Down Expand Up @@ -521,7 +518,7 @@ func (u *Update) GetWithdrawnPath() []*Path {
return l
}

func (u *Update) GetChanges(id string, as uint32, peerDown bool) (*Path, *Path, []*Path) {
func (u *Update) GetChanges(id string, as uint32, peerDown bool, useMultiplePaths *oc.UseMultiplePathsConfig) (*Path, *Path, []*Path) {
best, old := func(id string) (*Path, *Path) {
old := getBestPath(id, as, u.OldKnownPathList)
best := getBestPath(id, as, u.KnownPathList)
Expand Down Expand Up @@ -564,7 +561,7 @@ func (u *Update) GetChanges(id string, as uint32, peerDown bool) (*Path, *Path,

var multi []*Path

if id == GLOBAL_RIB_NAME && UseMultiplePaths.Enabled {
if id == GLOBAL_RIB_NAME && useMultiplePaths.Enabled {
diff := func(lhs, rhs []*Path) bool {
if len(lhs) != len(rhs) {
return true
Expand Down Expand Up @@ -659,12 +656,12 @@ func compareByLocalOrigin(path1, path2 *Path) *Path {
return nil
}

func compareByASPath(path1, path2 *Path) *Path {
func compareByASPath(path1, path2 *Path, selectionOptions *oc.RouteSelectionOptionsConfig) *Path {
// Calculated the best-paths by comparing as-path lengths.
//
// Shortest as-path length is preferred. If both path have same lengths,
// we return None.
if SelectionOptions.IgnoreAsPathLength {
if selectionOptions.IgnoreAsPathLength {
return nil
}

Expand Down Expand Up @@ -706,7 +703,7 @@ func compareByOrigin(path1, path2 *Path) *Path {
}
}

func compareByMED(path1, path2 *Path) *Path {
func compareByMED(path1, path2 *Path, selectionOptions *oc.RouteSelectionOptionsConfig) *Path {
// Select the path based with lowest MED value.
//
// If both paths have same MED, return None.
Expand Down Expand Up @@ -739,7 +736,7 @@ func compareByMED(path1, path2 *Path) *Path {
return firstAS(path1) != 0 && firstAS(path1) == firstAS(path2)
}()

if SelectionOptions.AlwaysCompareMed || isInternal || isSameAS {
if selectionOptions.AlwaysCompareMed || isInternal || isSameAS {
getMed := func(path *Path) uint32 {
attribute := path.getPathAttr(bgp.BGP_ATTR_TYPE_MULTI_EXIT_DISC)
if attribute == nil {
Expand Down Expand Up @@ -784,7 +781,7 @@ func compareByASNumber(path1, path2 *Path) *Path {
return nil
}

func compareByRouterID(path1, path2 *Path) (*Path, error) {
func compareByRouterID(path1, path2 *Path, selectionOptions *oc.RouteSelectionOptionsConfig) (*Path, error) {
// Select the route received from the peer with the lowest BGP router ID.
//
// If both paths are eBGP paths, then we do not do any tie breaking, i.e we do
Expand All @@ -799,11 +796,11 @@ func compareByRouterID(path1, path2 *Path) (*Path, error) {

// If both paths are from eBGP peers, then according to RFC we need
// not tie break using router id.
if !SelectionOptions.ExternalCompareRouterId && !path1.IsIBGP() && !path2.IsIBGP() {
if !selectionOptions.ExternalCompareRouterId && !path1.IsIBGP() && !path2.IsIBGP() {
return nil, nil
}

if !SelectionOptions.ExternalCompareRouterId && path1.IsIBGP() != path2.IsIBGP() {
if !selectionOptions.ExternalCompareRouterId && path1.IsIBGP() != path2.IsIBGP() {
return nil, fmt.Errorf("this method does not support comparing ebgp with ibgp path")
}

Expand Down Expand Up @@ -844,8 +841,8 @@ func compareByNeighborAddress(path1, path2 *Path) *Path {
return nil
}

func compareByAge(path1, path2 *Path) *Path {
if !path1.IsIBGP() && !path2.IsIBGP() && !SelectionOptions.ExternalCompareRouterId {
func compareByAge(path1, path2 *Path, selectionOptions *oc.RouteSelectionOptionsConfig) *Path {
if !path1.IsIBGP() && !path2.IsIBGP() && !selectionOptions.ExternalCompareRouterId {
age1 := path1.GetTimestamp().UnixNano()
age2 := path2.GetTimestamp().UnixNano()
if age1 == age2 {
Expand Down
49 changes: 26 additions & 23 deletions internal/pkg/table/destination_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"testing"
"time"

"github.com/osrg/gobgp/v3/pkg/config/oc"
"github.com/osrg/gobgp/v3/pkg/packet/bgp"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -83,7 +84,7 @@ func TestCalculate2(t *testing.T) {
path1 := ProcessMessage(update1, peer1, time.Now())[0]

d := NewDestination(nlri, 0)
d.Calculate(logger, path1)
d.Calculate(logger, path1, &oc.RouteSelectionOptionsConfig {})

// suppose peer2 sends grammaatically correct but semantically flawed update message
// which has a withdrawal nlri not advertised before
Expand All @@ -92,7 +93,7 @@ func TestCalculate2(t *testing.T) {
path2 := ProcessMessage(update2, peer2, time.Now())[0]
assert.Equal(t, path2.IsWithdraw, true)

d.Calculate(logger, path2)
d.Calculate(logger, path2, &oc.RouteSelectionOptionsConfig {})

// we have a path from peer1 here
assert.Equal(t, len(d.knownPathList), 1)
Expand All @@ -102,7 +103,7 @@ func TestCalculate2(t *testing.T) {
path3 := ProcessMessage(update3, peer2, time.Now())[0]
assert.Equal(t, path3.IsWithdraw, false)

d.Calculate(logger, path3)
d.Calculate(logger, path3, &oc.RouteSelectionOptionsConfig {})

// this time, we have paths from peer1 and peer2
assert.Equal(t, len(d.knownPathList), 2)
Expand All @@ -112,7 +113,7 @@ func TestCalculate2(t *testing.T) {
update4 := bgp.NewBGPUpdateMessage(nil, pathAttributes, []*bgp.IPAddrPrefix{nlri})
path4 := ProcessMessage(update4, peer3, time.Now())[0]

d.Calculate(logger, path4)
d.Calculate(logger, path4, &oc.RouteSelectionOptionsConfig {})

// we must have paths from peer1, peer2 and peer3
assert.Equal(t, len(d.knownPathList), 3)
Expand Down Expand Up @@ -156,7 +157,7 @@ func TestMedTieBreaker(t *testing.T) {
}()

// same AS
assert.Equal(t, compareByMED(p0, p1), p0)
assert.Equal(t, compareByMED(p0, p1, &oc.RouteSelectionOptionsConfig {}), p0)

p2 := func() *Path {
aspath := bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_SEQ, []uint32{65003})})
Expand All @@ -165,7 +166,7 @@ func TestMedTieBreaker(t *testing.T) {
}()

// different AS
assert.Equal(t, compareByMED(p0, p2), (*Path)(nil))
assert.Equal(t, compareByMED(p0, p2, &oc.RouteSelectionOptionsConfig {}), (*Path)(nil))

p3 := func() *Path {
aspath := bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_CONFED_SEQ, []uint32{65003, 65004}), bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_SEQ, []uint32{65001, 65003})})
Expand All @@ -180,7 +181,7 @@ func TestMedTieBreaker(t *testing.T) {
}()

// ignore confed
assert.Equal(t, compareByMED(p3, p4), p3)
assert.Equal(t, compareByMED(p3, p4, &oc.RouteSelectionOptionsConfig {}), p3)

p5 := func() *Path {
attrs := []bgp.PathAttributeInterface{bgp.NewPathAttributeMultiExitDisc(0)}
Expand All @@ -193,7 +194,7 @@ func TestMedTieBreaker(t *testing.T) {
}()

// no aspath
assert.Equal(t, compareByMED(p5, p6), p5)
assert.Equal(t, compareByMED(p5, p6, &oc.RouteSelectionOptionsConfig {}), p5)
}

func TestTimeTieBreaker(t *testing.T) {
Expand All @@ -212,17 +213,19 @@ func TestTimeTieBreaker(t *testing.T) {
path2 := ProcessMessage(updateMsg, peer2, time.Now().Add(-1*time.Hour))[0] // older than path1

d := NewDestination(nlri, 0)
d.Calculate(logger, path1)
d.Calculate(logger, path2)
d.Calculate(logger, path1, &oc.RouteSelectionOptionsConfig {})
d.Calculate(logger, path2, &oc.RouteSelectionOptionsConfig {})

assert.Equal(t, len(d.knownPathList), 2)
assert.Equal(t, true, d.GetBestPath("", 0).GetSource().ID.Equal(net.IP{2, 2, 2, 2})) // path from peer2 win

// this option disables tie breaking by age
SelectionOptions.ExternalCompareRouterId = true
selectionOptions := oc.RouteSelectionOptionsConfig {
ExternalCompareRouterId: true,
}
d = NewDestination(nlri, 0)
d.Calculate(logger, path1)
d.Calculate(logger, path2)
d.Calculate(logger, path1, &selectionOptions)
d.Calculate(logger, path2, &selectionOptions)

assert.Equal(t, len(d.knownPathList), 2)
assert.Equal(t, true, d.GetBestPath("", 0).GetSource().ID.Equal(net.IP{1, 1, 1, 1})) // path from peer1 win
Expand Down Expand Up @@ -315,7 +318,9 @@ func updateMsgD3() *bgp.BGPMessage {
}

func TestMultipath(t *testing.T) {
UseMultiplePaths.Enabled = true
useMultiplePaths := oc.UseMultiplePathsConfig {
Enabled: true,
}
origin := bgp.NewPathAttributeOrigin(0)
aspathParam := []bgp.AsPathParamInterface{bgp.NewAs4PathParam(2, []uint32{65000})}
aspath := bgp.NewPathAttributeAsPath(aspathParam)
Expand Down Expand Up @@ -347,17 +352,17 @@ func TestMultipath(t *testing.T) {
path2 := ProcessMessage(updateMsg, peer2, time.Now())[0]

d := NewDestination(nlri[0], 0)
d.Calculate(logger, path2)
d.Calculate(logger, path2, &oc.RouteSelectionOptionsConfig{})

best, old, multi := d.Calculate(logger, path1).GetChanges(GLOBAL_RIB_NAME, 0, false)
best, old, multi := d.Calculate(logger, path1, &oc.RouteSelectionOptionsConfig{}).GetChanges(GLOBAL_RIB_NAME, 0, false, &useMultiplePaths)
assert.NotNil(t, best)
assert.Equal(t, old, path2)
assert.Equal(t, len(multi), 2)
assert.Equal(t, len(d.GetKnownPathList(GLOBAL_RIB_NAME, 0)), 2)

path3 := path2.Clone(true)
dd := d.Calculate(logger, path3)
best, old, multi = dd.GetChanges(GLOBAL_RIB_NAME, 0, false)
dd := d.Calculate(logger, path3, &oc.RouteSelectionOptionsConfig{})
best, old, multi = dd.GetChanges(GLOBAL_RIB_NAME, 0, false, &useMultiplePaths)
assert.Nil(t, best)
assert.Equal(t, old, path1)
assert.Equal(t, len(multi), 1)
Expand All @@ -374,8 +379,8 @@ func TestMultipath(t *testing.T) {
}
updateMsg = bgp.NewBGPUpdateMessage(nil, pathAttributes, nlri)
path4 := ProcessMessage(updateMsg, peer3, time.Now())[0]
dd = d.Calculate(logger, path4)
best, _, multi = dd.GetChanges(GLOBAL_RIB_NAME, 0, false)
dd = d.Calculate(logger, path4, &oc.RouteSelectionOptionsConfig{})
best, _, multi = dd.GetChanges(GLOBAL_RIB_NAME, 0, false, &useMultiplePaths)
assert.NotNil(t, best)
assert.Equal(t, len(multi), 1)
assert.Equal(t, len(d.GetKnownPathList(GLOBAL_RIB_NAME, 0)), 2)
Expand All @@ -389,12 +394,10 @@ func TestMultipath(t *testing.T) {
}
updateMsg = bgp.NewBGPUpdateMessage(nil, pathAttributes, nlri)
path5 := ProcessMessage(updateMsg, peer2, time.Now())[0]
best, _, multi = d.Calculate(logger, path5).GetChanges(GLOBAL_RIB_NAME, 0, false)
best, _, multi = d.Calculate(logger, path5, &oc.RouteSelectionOptionsConfig{}).GetChanges(GLOBAL_RIB_NAME, 0, false, &useMultiplePaths)
assert.NotNil(t, best)
assert.Equal(t, len(multi), 2)
assert.Equal(t, len(d.GetKnownPathList(GLOBAL_RIB_NAME, 0)), 3)

UseMultiplePaths.Enabled = false
}

func TestIdMap(t *testing.T) {
Expand Down
13 changes: 9 additions & 4 deletions internal/pkg/table/table_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

farm "github.com/dgryski/go-farm"

"github.com/osrg/gobgp/v3/pkg/config/oc"
"github.com/osrg/gobgp/v3/pkg/log"
"github.com/osrg/gobgp/v3/pkg/packet/bgp"
)
Expand Down Expand Up @@ -114,14 +115,18 @@ func ProcessMessage(m *bgp.BGPMessage, peerInfo *PeerInfo, timestamp time.Time)
type TableManager struct {
Tables map[bgp.RouteFamily]*Table
Vrfs map[string]*Vrf
SelectionOptions *oc.RouteSelectionOptionsConfig
UseMultiplePaths *oc.UseMultiplePathsConfig
rfList []bgp.RouteFamily
logger log.Logger
}

func NewTableManager(logger log.Logger, rfList []bgp.RouteFamily) *TableManager {
func NewTableManager(logger log.Logger, rfList []bgp.RouteFamily, selectionOptions *oc.RouteSelectionOptionsConfig, useMultiplePaths *oc.UseMultiplePathsConfig) *TableManager {
t := &TableManager{
Tables: make(map[bgp.RouteFamily]*Table),
Vrfs: make(map[string]*Vrf),
SelectionOptions: selectionOptions,
UseMultiplePaths: useMultiplePaths,
rfList: rfList,
logger: logger,
}
Expand Down Expand Up @@ -192,7 +197,7 @@ func (manager *TableManager) update(newPath *Path) *Update {
t := manager.Tables[newPath.GetRouteFamily()]
t.validatePath(newPath)
dst := t.getOrCreateDest(newPath.GetNlri(), 64)
u := dst.Calculate(manager.logger, newPath)
u := dst.Calculate(manager.logger, newPath, manager.SelectionOptions)
if len(dst.knownPathList) == 0 {
t.deleteDest(dst)
}
Expand Down Expand Up @@ -292,7 +297,7 @@ func (manager *TableManager) getDestinationCount(rfList []bgp.RouteFamily) int {
}

func (manager *TableManager) GetBestPathList(id string, as uint32, rfList []bgp.RouteFamily) []*Path {
if SelectionOptions.DisableBestPathSelection {
if manager.SelectionOptions.DisableBestPathSelection {
// Note: If best path selection disabled, there is no best path.
return nil
}
Expand All @@ -304,7 +309,7 @@ func (manager *TableManager) GetBestPathList(id string, as uint32, rfList []bgp.
}

func (manager *TableManager) GetBestMultiPathList(id string, rfList []bgp.RouteFamily) [][]*Path {
if !UseMultiplePaths.Enabled || SelectionOptions.DisableBestPathSelection {
if !manager.UseMultiplePaths.Enabled || manager.SelectionOptions.DisableBestPathSelection {
// Note: If multi path not enabled or best path selection disabled,
// there is no best multi path.
return nil
Expand Down
Loading

0 comments on commit 00d297c

Please sign in to comment.