Move SelectionOptions and UseMultiplePath to server-local vars

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.

Author: dawn-minion

internal/pkg/table/destination.go

@@ -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 (
@@ -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)
 
@@ -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)
@@ -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
 	}
 
@@ -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.
@@ -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 {
@@ -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 {
@@ -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 {
@@ -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)
@@ -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
@@ -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
 	}
 
@@ -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.
@@ -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 {
@@ -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
@@ -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")
 	}
 
@@ -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 {

internal/pkg/table/destination_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/osrg/gobgp/v3/internal/pkg/config"
 	"github.com/osrg/gobgp/v3/pkg/packet/bgp"
 
 	"github.com/stretchr/testify/assert"
@@ -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, &config.RouteSelectionOptionsConfig {})
 
 	// suppose peer2 sends grammaatically correct but semantically flawed update message
 	// which has a withdrawal nlri not advertised before
@@ -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, &config.RouteSelectionOptionsConfig {})
 
 	// we have a path from peer1 here
 	assert.Equal(t, len(d.knownPathList), 1)
@@ -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, &config.RouteSelectionOptionsConfig {})
 
 	// this time, we have paths from peer1 and peer2
 	assert.Equal(t, len(d.knownPathList), 2)
@@ -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, &config.RouteSelectionOptionsConfig {})
 
 	// we must have paths from peer1, peer2 and peer3
 	assert.Equal(t, len(d.knownPathList), 3)
@@ -156,7 +157,7 @@ func TestMedTieBreaker(t *testing.T) {
 	}()
 
 	// same AS
-	assert.Equal(t, compareByMED(p0, p1), p0)
+	assert.Equal(t, compareByMED(p0, p1, &config.RouteSelectionOptionsConfig {}), p0)
 
 	p2 := func() *Path {
 		aspath := bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_SEQ, []uint32{65003})})
@@ -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, &config.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})})
@@ -180,7 +181,7 @@ func TestMedTieBreaker(t *testing.T) {
 	}()
 
 	// ignore confed
-	assert.Equal(t, compareByMED(p3, p4), p3)
+	assert.Equal(t, compareByMED(p3, p4, &config.RouteSelectionOptionsConfig {}), p3)
 
 	p5 := func() *Path {
 		attrs := []bgp.PathAttributeInterface{bgp.NewPathAttributeMultiExitDisc(0)}
@@ -193,7 +194,7 @@ func TestMedTieBreaker(t *testing.T) {
 	}()
 
 	// no aspath
-	assert.Equal(t, compareByMED(p5, p6), p5)
+	assert.Equal(t, compareByMED(p5, p6, &config.RouteSelectionOptionsConfig {}), p5)
 }
 
 func TestTimeTieBreaker(t *testing.T) {
@@ -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, &config.RouteSelectionOptionsConfig {})
+	d.Calculate(logger, path2, &config.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 := config.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
@@ -315,7 +318,9 @@ func updateMsgD3() *bgp.BGPMessage {
 }
 
 func TestMultipath(t *testing.T) {
-	UseMultiplePaths.Enabled = true
+	useMultiplePaths := config.UseMultiplePathsConfig {
+		Enabled: true,
+	}
 	origin := bgp.NewPathAttributeOrigin(0)
 	aspathParam := []bgp.AsPathParamInterface{bgp.NewAs4PathParam(2, []uint32{65000})}
 	aspath := bgp.NewPathAttributeAsPath(aspathParam)
@@ -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, &config.RouteSelectionOptionsConfig{})
 
-	best, old, multi := d.Calculate(logger, path1).GetChanges(GLOBAL_RIB_NAME, 0, false)
+	best, old, multi := d.Calculate(logger, path1, &config.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, &config.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)
@@ -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, &config.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)
@@ -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, &config.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) {

internal/pkg/table/table_manager.go

@@ -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"
 )
@@ -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,
 	}
@@ -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)
 	}
@@ -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
 	}
@@ -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