Fix adj-out display with add path enabled

5 files changed, 103 insertions, 16 deletions

diff --git a/internal/pkg/table/adj.go b/internal/pkg/table/adj.go
index d63537f8..9ae1e156 100644
--- a/internal/pkg/table/adj.go
+++ b/internal/pkg/table/adj.go
@@ -89,6 +89,43 @@ func (adj *AdjRib) Update(pathList []*Path) {
 	}
 }
 
+/* The provided pathList is expected to be the real candidate routes after policy evaluation.
+   For routes that are filtered by policy, there could be a mismatch between display
+   and actual rib sent to the peer (if softreset out was not run).
+   Only used to display adj-out because we do not maintain a separate adj-out table
+*/
+func (adj *AdjRib) UpdateAdjRibOut(pathList []*Path) {
+	for _, path := range pathList {
+		if path == nil || path.IsEOR() {
+			continue
+		}
+		t := adj.table[path.GetRouteFamily()]
+		d := t.getOrCreateDest(path.GetNlri(), 0)
+
+		var old *Path
+		idx := -1
+		for i, p := range d.knownPathList {
+			if p.GetNlri().PathLocalIdentifier() == path.GetNlri().PathLocalIdentifier() {
+				idx = i
+				break
+			}
+		}
+		if idx != -1 {
+			old = d.knownPathList[idx]
+		}
+
+		// No withdraw use case for adj-out
+		if idx != -1 {
+			if old.Equal(path) {
+				path.setTimestamp(old.GetTimestamp())
+			}
+			d.knownPathList[idx] = path
+		} else {
+			d.knownPathList = append(d.knownPathList, path)
+		}
+	}
+}
+
 func (adj *AdjRib) walk(families []bgp.RouteFamily, fn func(*Destination) bool) {
 	for _, f := range families {
 		if t, ok := adj.table[f]; ok {
diff --git a/internal/pkg/table/adj_test.go b/internal/pkg/table/adj_test.go
index 9e48538f..99aba0ff 100644
--- a/internal/pkg/table/adj_test.go
+++ b/internal/pkg/table/adj_test.go
@@ -61,6 +61,35 @@ func TestAddPath(t *testing.T) {
 	assert.Equal(t, 0, len(adj.table[family].destinations))
 }
 
+func TestAddPathAdjOut(t *testing.T) {
+	pi := &PeerInfo{}
+	attrs := []bgp.PathAttributeInterface{bgp.NewPathAttributeOrigin(0)}
+
+	nlri1 := bgp.NewIPAddrPrefix(24, "20.20.20.0")
+	nlri1.SetPathIdentifier(1)
+	nlri1.SetPathLocalIdentifier(1)
+	p1 := NewPath(pi, nlri1, false, attrs, time.Now(), false)
+	nlri2 := bgp.NewIPAddrPrefix(24, "20.20.20.0")
+	nlri2.SetPathIdentifier(1)
+	nlri2.SetPathLocalIdentifier(2)
+	p2 := NewPath(pi, nlri2, false, attrs, time.Now(), false)
+	nlri3 := bgp.NewIPAddrPrefix(24, "20.20.20.0")
+	nlri3.SetPathIdentifier(2)
+	nlri3.SetPathLocalIdentifier(2)
+	p3 := NewPath(pi, nlri3, false, attrs, time.Now(), false)
+	nlri4 := bgp.NewIPAddrPrefix(24, "20.20.20.0")
+	nlri4.SetPathIdentifier(3)
+	nlri4.SetPathLocalIdentifier(2)
+	p4 := NewPath(pi, nlri4, false, attrs, time.Now(), false)
+	family := p1.GetRouteFamily()
+	families := []bgp.RouteFamily{family}
+
+	adj := NewAdjRib(families)
+	adj.UpdateAdjRibOut([]*Path{p1, p2, p3, p4})
+	assert.Equal(t, len(adj.table[family].destinations), 1)
+	assert.Equal(t, adj.Count([]bgp.RouteFamily{family}), 2)
+}
+
 func TestStale(t *testing.T) {
 	pi := &PeerInfo{}
 	attrs := []bgp.PathAttributeInterface{bgp.NewPathAttributeOrigin(0)}
diff --git a/pkg/server/server.go b/pkg/server/server.go
index fa0f5225..966f48b7 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -2485,11 +2485,11 @@ func (s *BgpServer) getAdjRib(addr string, family bgp.RouteFamily, in bool, enab
 					if p == nil {
 						filtered[path.GetNlri().String()] = path
 					}
-					adjRib.Update([]*table.Path{path})
+					adjRib.UpdateAdjRibOut([]*table.Path{path})
 				}
 			} else {
 				accepted, _ := s.getBestFromLocal(peer, peer.configuredRFlist())
-				adjRib.Update(accepted)
+				adjRib.UpdateAdjRibOut(accepted)
 			}
 		}
 		rib, err = adjRib.Select(family, false, table.TableSelectOption{ID: id, AS: as, LookupPrefixes: prefixes})
@@ -2616,7 +2616,7 @@ func (s *BgpServer) getAdjRibInfo(addr string, family bgp.RouteFamily, in bool)
 		} else {
 			adjRib = table.NewAdjRib(peer.configuredRFlist())
 			accepted, _ := s.getBestFromLocal(peer, peer.configuredRFlist())
-			adjRib.Update(accepted)
+			adjRib.UpdateAdjRibOut(accepted)
 		}
 		info, err = adjRib.TableInfo(family)
 		return err
diff --git a/test/lib/gobgp.py b/test/lib/gobgp.py
index 1abdb782..6ffa6024 100644
--- a/test/lib/gobgp.py
+++ b/test/lib/gobgp.py
@@ -277,12 +277,15 @@ class GoBGPContainer(BGPContainer):
         t.daemon = True
         t.start()
 
-    def _get_adj_rib(self, adj_type, peer, prefix='', rf='ipv4'):
+    def _get_adj_rib(self, adj_type, peer, prefix='', rf='ipv4', add_path_enabled=False):
         peer_addr = self.peer_name(peer)
         cmd = 'gobgp neighbor {0} adj-{1} {2} -a {3} -j'.format(peer_addr,
                                                                 adj_type,
                                                                 prefix, rf)
         output = self.local(cmd, capture=True)
+        if add_path_enabled:
+            return self._get_rib(json.loads(output))
+
         ret = [p[0] for p in json.loads(output).values()]
         for p in ret:
             p["nexthop"] = self._get_nexthop(p)
@@ -292,11 +295,11 @@ class GoBGPContainer(BGPContainer):
             p["med"] = self._get_med(p)
         return ret
 
-    def get_adj_rib_in(self, peer, prefix='', rf='ipv4'):
-        return self._get_adj_rib('in', peer, prefix, rf)
+    def get_adj_rib_in(self, peer, prefix='', rf='ipv4', add_path_enabled=False):
+        return self._get_adj_rib('in', peer, prefix, rf, add_path_enabled)
 
-    def get_adj_rib_out(self, peer, prefix='', rf='ipv4'):
-        return self._get_adj_rib('out', peer, prefix, rf)
+    def get_adj_rib_out(self, peer, prefix='', rf='ipv4', add_path_enabled=False):
+        return self._get_adj_rib('out', peer, prefix, rf, add_path_enabled)
 
     def get_neighbor(self, peer):
         cmd = 'gobgp -j neighbor {0}'.format(self.peer_name(peer))
diff --git a/test/scenario_test/addpath_test.py b/test/scenario_test/addpath_test.py
index c8a8491b..23d3b5cb 100644
--- a/test/scenario_test/addpath_test.py
+++ b/test/scenario_test/addpath_test.py
@@ -164,8 +164,17 @@ class GoBGPTestBase(unittest.TestCase):
 
         assert_several_times(f)
 
+    def test_11_check_g1_adj_out(self):
+        adj_out = self.g1.get_adj_rib_out(self.g2, add_path_enabled=True)
+        self.assertEqual(len(adj_out), 1)
+        self.assertEqual(len(adj_out[0]['paths']), 1)
+
+        adj_out = self.g1.get_adj_rib_out(self.g3, add_path_enabled=True)
+        self.assertEqual(len(adj_out), 1)
+        self.assertEqual(len(adj_out[0]['paths']), 3)
+
     # test the best path is replaced due to the CLI route from g1 rib
-    def test_11_check_g2_global_rib(self):
+    def test_12_check_g2_global_rib(self):
         def f():
             rib = self.g2.get_global_rib()
             self.assertEqual(len(rib), 1)
@@ -175,7 +184,7 @@ class GoBGPTestBase(unittest.TestCase):
         assert_several_times(f)
 
     # test the route from CLI is advertised from g1
-    def test_12_check_g3_global_rib(self):
+    def test_13_check_g3_global_rib(self):
         def f():
             rib = self.g3.get_global_rib()
             self.assertEqual(len(rib), 1)
@@ -190,13 +199,13 @@ class GoBGPTestBase(unittest.TestCase):
         assert_several_times(f)
 
     # remove non-existing route with path_id via GoBGP CLI (no error check)
-    def test_13_remove_non_existing_add_paths_route_via_cli(self):
+    def test_14_remove_non_existing_add_paths_route_via_cli(self):
         # specify locally non-existing identifier which has the same value
         # with the identifier of the route from e1
         self.g1.del_route(route='192.168.100.0/24', identifier=20)
 
     # test none of route is removed by non-existing path_id via CLI
-    def test_14_check_g1_global_rib(self):
+    def test_15_check_g1_global_rib(self):
         def f():
             rib = self.g1.get_global_rib()
             self.assertEqual(len(rib), 1)
@@ -211,11 +220,20 @@ class GoBGPTestBase(unittest.TestCase):
         assert_several_times(f)
 
     # remove route with path_id via GoBGP CLI (no error check)
-    def test_15_remove_add_paths_route_via_cli(self):
+    def test_16_remove_add_paths_route_via_cli(self):
         self.g1.del_route(route='192.168.100.0/24', identifier=10)
 
+    def test_17_check_g1_adj_out(self):
+        adj_out = self.g1.get_adj_rib_out(self.g2, add_path_enabled=True)
+        self.assertEqual(len(adj_out), 1)
+        self.assertEqual(len(adj_out[0]['paths']), 1)
+
+        adj_out = self.g1.get_adj_rib_out(self.g3, add_path_enabled=True)
+        self.assertEqual(len(adj_out), 1)
+        self.assertEqual(len(adj_out[0]['paths']), 2)
+
     # test the route is removed from the rib via CLI
-    def test_16_check_g1_global_rib(self):
+    def test_18_check_g1_global_rib(self):
         def f():
             rib = self.g1.get_global_rib()
             self.assertEqual(len(rib), 1)
@@ -227,7 +245,7 @@ class GoBGPTestBase(unittest.TestCase):
         assert_several_times(f)
 
     # test the best path is replaced the removal from g1 rib
-    def test_17_check_g2_global_rib(self):
+    def test_19_check_g2_global_rib(self):
         def f():
             rib = self.g2.get_global_rib()
             self.assertEqual(len(rib), 1)
@@ -237,7 +255,7 @@ class GoBGPTestBase(unittest.TestCase):
         assert_several_times(f)
 
     # test the removed route from CLI is withdrawn by g1
-    def test_18_check_g3_global_rib(self):
+    def test_20_check_g3_global_rib(self):
         def f():
             rib = self.g3.get_global_rib()
             self.assertEqual(len(rib), 1)