From 79ed96fdc1a171de4b13108c7ca9c154d6d7fcd4 Mon Sep 17 00:00:00 2001 From: ISHIDA Wataru Date: Mon, 17 Aug 2015 16:50:58 +0900 Subject: table: fix best path selection which considers local asn TableManager.localAsn wasn't used. Signed-off-by: ISHIDA Wataru --- table/destination.go | 54 +++++++++++++++++---------------------------- table/destination_test.go | 2 +- table/path.go | 10 ++++----- table/table_manager.go | 9 ++++---- table/table_manager_test.go | 7 +++--- 5 files changed, 33 insertions(+), 49 deletions(-) (limited to 'table') diff --git a/table/destination.go b/table/destination.go index 51d6b5d0..75072732 100644 --- a/table/destination.go +++ b/table/destination.go @@ -43,6 +43,7 @@ const ( type PeerInfo struct { AS uint32 ID net.IP + LocalAS uint32 LocalID net.IP Address net.IP } @@ -174,7 +175,7 @@ func (dd *Destination) validatePath(path *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(localAsn uint32) (*Path, string, error) { +func (dest *Destination) Calculate() (*Path, string, error) { // First remove the withdrawn paths. // Note: If we want to support multiple paths per destination we may @@ -215,7 +216,7 @@ func (dest *Destination) Calculate(localAsn uint32) (*Path, string, error) { } // Compute new best path - currentBestPath, reason, e := dest.computeKnownBestPath(localAsn) + currentBestPath, reason, e := dest.computeKnownBestPath() if e != nil { log.Error(e) } @@ -319,7 +320,7 @@ func (dest *Destination) removeWithdrawals() { } } -func (dest *Destination) computeKnownBestPath(localAsn uint32) (*Path, string, error) { +func (dest *Destination) computeKnownBestPath() (*Path, string, error) { // """Computes the best path among known paths. // @@ -337,8 +338,7 @@ func (dest *Destination) computeKnownBestPath(localAsn uint32) (*Path, string, e bestPathReason := BPR_ONLY_PATH for _, nextPath := range dest.knownPathList[1:] { // Compare next path with current best path. - // TODO make interface to get Local AS number - newBestPath, reason := computeBestPath(localAsn, currentBestPath, nextPath) + newBestPath, reason := computeBestPath(currentBestPath, nextPath) bestPathReason = reason if newBestPath != nil { currentBestPath = newBestPath @@ -413,12 +413,11 @@ func removeWithPath(list []*Path, path *Path) ([]*Path, bool) { return list, false } -func computeBestPath(localAsn uint32, path1, path2 *Path) (*Path, string) { +func computeBestPath(path1, path2 *Path) (*Path, string) { //Compares given paths and returns best path. // //Parameters: - // -`localAsn`: asn of local bgpspeaker // -`path1`: first path to compare // -`path2`: second path to compare // @@ -482,7 +481,7 @@ func computeBestPath(localAsn uint32, path1, path2 *Path) (*Path, string) { bestPathReason = BPR_MED } if bestPath == nil { - bestPath = compareByASNumber(localAsn, path1, path2) + bestPath = compareByASNumber(path1, path2) bestPathReason = BPR_ASN } if bestPath == nil { @@ -491,7 +490,7 @@ func computeBestPath(localAsn uint32, path1, path2 *Path) (*Path, string) { } if bestPath == nil { var e error = nil - bestPath, e = compareByRouterID(localAsn, path1, path2) + bestPath, e = compareByRouterID(path1, path2) if e != nil { log.Error(e) } @@ -673,7 +672,7 @@ func compareByMED(path1, path2 *Path) *Path { return path2 } -func compareByASNumber(localAsn uint32, path1, path2 *Path) *Path { +func compareByASNumber(path1, path2 *Path) *Path { //Select the path based on source (iBGP/eBGP) peer. // @@ -681,17 +680,12 @@ func compareByASNumber(localAsn uint32, path1, path2 *Path) *Path { //peers, return None. log.Debugf("enter compareByASNumber") - p1Asn := path1.GetSource().AS - p2Asn := path2.GetSource().AS - - log.Debugf("compareByASNumber -- p1Asn: %d, p2Asn: %d", p1Asn, p2Asn) - // If path1 is from ibgp peer and path2 is from ebgp peer. - if (p1Asn == localAsn) && (p2Asn != localAsn) { - return path2 - } - - // If path2 is from ibgp peer and path1 is from ebgp peer, - if (p2Asn == localAsn) && (p1Asn != localAsn) { + log.Debugf("compareByASNumber -- p1Asn: %d, p2Asn: %d", path1.source.AS, path2.source.AS) + // If one path is from ibgp peer and another is from ebgp peer, take the ebgp path + if path1.IsIBGP() != path2.IsIBGP() { + if path1.IsIBGP() { + return path2 + } return path1 } @@ -708,7 +702,7 @@ func compareByIGPCost(path1, path2 *Path) *Path { return nil } -func compareByRouterID(localAsn uint32, path1, path2 *Path) (*Path, error) { +func compareByRouterID(path1, path2 *Path) (*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 @@ -717,32 +711,24 @@ func compareByRouterID(localAsn uint32, path1, path2 *Path) (*Path, error) { // We pick best path between two iBGP paths as usual. log.Debugf("enter compareByRouterID") - source1 := path1.GetSource() - source2 := path2.GetSource() - // If both paths are from NC we have same router Id, hence cannot compare. if path1.IsLocal() && path2.IsLocal() { return nil, nil } - asn1 := source1.AS - asn2 := source2.AS - - isEbgp1 := asn1 != localAsn - isEbgp2 := asn2 != localAsn // If both paths are from eBGP peers, then according to RFC we need // not tie break using router id. - if isEbgp1 && isEbgp2 { + if !path1.IsIBGP() && !path2.IsIBGP() { return nil, nil } - if isEbgp1 != isEbgp2 { + if path1.IsIBGP() != path2.IsIBGP() { return nil, fmt.Errorf("This method does not support comparing ebgp with ibgp path") } // At least one path is not coming from NC, so we get local bgp id. - id1 := binary.BigEndian.Uint32(source1.ID) - id2 := binary.BigEndian.Uint32(source2.ID) + id1 := binary.BigEndian.Uint32(path1.source.ID) + id2 := binary.BigEndian.Uint32(path2.source.ID) // If both router ids are same/equal we cannot decide. // This case is possible since router ids are arbitrary. diff --git a/table/destination_test.go b/table/destination_test.go index 80eaa2a4..f46851f9 100644 --- a/table/destination_test.go +++ b/table/destination_test.go @@ -102,7 +102,7 @@ func TestDestinationCalculate(t *testing.T) { ipv4d.addNewPath(pathD[1]) ipv4d.addNewPath(pathD[2]) ipv4d.addWithdraw(pathD[2]) - _, _, e := ipv4d.Calculate(uint32(100)) + _, _, e := ipv4d.Calculate() assert.Nil(t, e) } diff --git a/table/path.go b/table/path.go index 7e131185..f4f1893b 100644 --- a/table/path.go +++ b/table/path.go @@ -135,11 +135,11 @@ func (path *Path) setTimestamp(t time.Time) { } func (path *Path) IsLocal() bool { - var ret bool - if path.source.Address == nil { - ret = true - } - return ret + return path.source.Address == nil +} + +func (path *Path) IsIBGP() bool { + return path.source.AS == path.source.LocalAS } func (path *Path) ToApiStruct() *api.Path { diff --git a/table/table_manager.go b/table/table_manager.go index e44babe7..90a12175 100644 --- a/table/table_manager.go +++ b/table/table_manager.go @@ -113,10 +113,9 @@ func ProcessMessage(m *bgp.BGPMessage, peerInfo *PeerInfo) []*Path { } type TableManager struct { - Tables map[bgp.RouteFamily]*Table - Vrfs map[string]*Vrf - localAsn uint32 - owner string + Tables map[bgp.RouteFamily]*Table + Vrfs map[string]*Vrf + owner string } func NewTableManager(owner string, rfList []bgp.RouteFamily) *TableManager { @@ -188,7 +187,7 @@ func (manager *TableManager) calculate(destinationList []*Destination) ([]*Path, "Key": destination.GetNlri().String(), }).Debug("Processing destination") - newBestPath, reason, err := destination.Calculate(manager.localAsn) + newBestPath, reason, err := destination.Calculate() if err != nil { log.Error(err) diff --git a/table/table_manager_test.go b/table/table_manager_test.go index 5a875c9b..08791867 100644 --- a/table/table_manager_test.go +++ b/table/table_manager_test.go @@ -38,6 +38,7 @@ func getLogger(lv log.Level) *log.Logger { func peerR1() *PeerInfo { peer := &PeerInfo{ AS: 65000, + LocalAS: 65000, ID: net.ParseIP("10.0.0.3").To4(), LocalID: net.ParseIP("10.0.0.1").To4(), Address: net.ParseIP("10.0.0.1").To4(), @@ -48,6 +49,7 @@ func peerR1() *PeerInfo { func peerR2() *PeerInfo { peer := &PeerInfo{ AS: 65100, + LocalAS: 65000, Address: net.ParseIP("10.0.0.2").To4(), } return peer @@ -56,6 +58,7 @@ func peerR2() *PeerInfo { func peerR3() *PeerInfo { peer := &PeerInfo{ AS: 65000, + LocalAS: 65000, ID: net.ParseIP("10.0.0.2").To4(), LocalID: net.ParseIP("10.0.0.1").To4(), Address: net.ParseIP("10.0.0.3").To4(), @@ -967,7 +970,6 @@ func TestProcessBGPUpdate_5_select_low_med_ipv6(t *testing.T) { func TestProcessBGPUpdate_6_select_ebgp_path_ipv4(t *testing.T) { tm := NewTableManager("TestProcessBGPUpdate_6_select_ebgp_path_ipv4", []bgp.RouteFamily{bgp.RF_IPv4_UC}) - tm.localAsn = uint32(65000) var err error @@ -1052,7 +1054,6 @@ func TestProcessBGPUpdate_6_select_ebgp_path_ipv4(t *testing.T) { func TestProcessBGPUpdate_6_select_ebgp_path_ipv6(t *testing.T) { tm := NewTableManager("TestProcessBGPUpdate_6_select_ebgp_path_ipv6", []bgp.RouteFamily{bgp.RF_IPv6_UC}) - tm.localAsn = uint32(65000) var err error origin1 := bgp.NewPathAttributeOrigin(0) @@ -1142,7 +1143,6 @@ func TestProcessBGPUpdate_6_select_ebgp_path_ipv6(t *testing.T) { func TestProcessBGPUpdate_7_select_low_routerid_path_ipv4(t *testing.T) { tm := NewTableManager("TestProcessBGPUpdate_7_select_low_routerid_path_ipv4", []bgp.RouteFamily{bgp.RF_IPv4_UC}) - tm.localAsn = uint32(65000) var err error @@ -1227,7 +1227,6 @@ func TestProcessBGPUpdate_7_select_low_routerid_path_ipv4(t *testing.T) { func TestProcessBGPUpdate_7_select_low_routerid_path_ipv6(t *testing.T) { tm := NewTableManager("TestProcessBGPUpdate_7_select_low_routerid_path_ipv6", []bgp.RouteFamily{bgp.RF_IPv6_UC}) - tm.localAsn = uint32(65000) var err error origin1 := bgp.NewPathAttributeOrigin(0) -- cgit v1.2.3