fix: fix route stuck on eBGP filterpath

Author: Ivan-Pokhabov

pkg/server/server.go

@@ -551,9 +551,24 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path {
 		// Do not filter local (static) routes with as-path loop
 		// if configured to bypass these checks in the peer
 		// as-path options config.
-		if path.IsLocal() && !peer.allowAsPathLoopLocal() {
-			return nil
-		} else if !path.IsLocal() {
+		if !path.IsLocal() || !peer.allowAsPathLoopLocal() {
+			if !path.IsWithdraw && old != nil {
+				// A new best path was selected, but we cannot advertise it to this peer
+				// due to as-loop. In this case, we MUST explicitly withdraw the
+				// old path we previously advertised to prevent a stucked route
+				// on the peer, which would lead to a traffic blackhole.
+				//
+				// Example: routers from AS A: A1 and A2, AS B: B1
+				// A1 <eBGP> B1, A2 <eBGP> B1, no iBGP
+				// All of them announce prefix P
+				// Paths on A1, A2: [local, B1]
+				// Paths on B1: [local, A1, A2]
+				// When B1 receive local prefix withdraw it choose new best
+				// for example from A1. Then it send. explicit withdraw for A1
+				// and try to implicit withdraw for other peers. But we have AS-Loop for
+				// A2 and path stuck. So in this case B1 should send explicit withdraw for old path.
+				return old.Clone(true)
+			}
 			return nil
 		}
 	}

pkg/server/server_test.go

@@ -1594,48 +1594,65 @@ func runNewServer(t *testing.T, as uint32, routerID string, listenPort int32) *B
 	return s
 }
 
-func peerServers(t *testing.T, ctx context.Context, servers []*BgpServer, families []oc.AfiSafiType) error {
+type peerOption func(peer *BgpServer, g *oc.Global, p *oc.Neighbor)
+
+func setPeerAddressOpt(peer *BgpServer, g *oc.Global, p *oc.Neighbor) {
+	p.Transport.Config.LocalAddress = g.Config.LocalAddressList[0]
+	p.Config.NeighborAddress = peer.bgpConfig.Global.Config.LocalAddressList[0]
+}
+
+func peerTwoServers(t *testing.T, ctx context.Context, server, peer *BgpServer, families []oc.AfiSafiType, isPassive bool, opts ...peerOption) error {
+	neighborConfig := &oc.Neighbor{
+		Config: oc.NeighborConfig{
+			NeighborAddress: netip.MustParseAddr("127.0.0.1"),
+			PeerAs:          peer.bgpConfig.Global.Config.As,
+		},
+		AfiSafis: oc.AfiSafis{},
+		Transport: oc.Transport{
+			Config: oc.TransportConfig{
+				RemotePort: uint16(peer.bgpConfig.Global.Config.Port),
+			},
+		},
+		Timers: oc.Timers{
+			Config: oc.TimersConfig{
+				ConnectRetry:           1,
+				IdleHoldTimeAfterReset: 1,
+			},
+		},
+	}
+
+	for _, opt := range opts {
+		opt(peer, &server.bgpConfig.Global, neighborConfig)
+	}
+
+	if isPassive {
+		neighborConfig.Transport.Config.PassiveMode = true
+	}
+
+	for _, family := range families {
+		neighborConfig.AfiSafis = append(neighborConfig.AfiSafis, oc.AfiSafi{
+			Config: oc.AfiSafiConfig{
+				AfiSafiName: family,
+				Enabled:     true,
+			},
+		})
+	}
+
+	if err := server.AddPeer(ctx, &api.AddPeerRequest{Peer: oc.NewPeerFromConfigStruct(neighborConfig)}); err != nil {
+		t.Fatal(err)
+	}
+	return nil
+}
+
+func peerServers(t *testing.T, ctx context.Context, servers []*BgpServer, families []oc.AfiSafiType, opts ...peerOption) error {
 	for i, server := range servers {
 		for j, peer := range servers {
 			if i == j {
 				continue
 			}
-
-			neighborConfig := &oc.Neighbor{
-				Config: oc.NeighborConfig{
-					NeighborAddress: netip.MustParseAddr("127.0.0.1"),
-					PeerAs:          peer.bgpConfig.Global.Config.As,
-				},
-				AfiSafis: oc.AfiSafis{},
-				Transport: oc.Transport{
-					Config: oc.TransportConfig{
-						RemotePort: uint16(peer.bgpConfig.Global.Config.Port),
-					},
-				},
-				Timers: oc.Timers{
-					Config: oc.TimersConfig{
-						ConnectRetry:           1,
-						IdleHoldTimeAfterReset: 1,
-					},
-				},
-			}
-
 			// first server to get neighbor config is passive to hopefully make handshake faster
-			if j > i {
-				neighborConfig.Transport.Config.PassiveMode = true
-			}
-
-			for _, family := range families {
-				neighborConfig.AfiSafis = append(neighborConfig.AfiSafis, oc.AfiSafi{
-					Config: oc.AfiSafiConfig{
-						AfiSafiName: family,
-						Enabled:     true,
-					},
-				})
-			}
-
-			if err := server.AddPeer(ctx, &api.AddPeerRequest{Peer: oc.NewPeerFromConfigStruct(neighborConfig)}); err != nil {
-				t.Fatal(err)
+			if err := peerTwoServers(t, ctx, server, peer, families, i < j, opts...); err != nil {
+				return err
 			}
 		}
 	}
@@ -3003,3 +3020,114 @@ func TestAddDefinedSetReplace(t *testing.T) {
 	assert.Equal("replaceme", ns[0].Name)
 	assert.Equal([]string{"203.0.113.2/32"}, ns[0].List)
 }
+
+func TestEBGPRouteStuck(test *testing.T) {
+	var peers []*BgpServer
+	for i, s := range []struct {
+		routerId string
+		asn      uint32
+	}{
+		{routerId: "1.1.1.1", asn: 1},
+		{routerId: "2.2.2.1", asn: 2},
+		{routerId: "2.2.2.2", asn: 2},
+	} {
+		peer := NewBgpServer()
+		peer.logger.SetLevel(log.InfoLevel)
+		go peer.Serve()
+
+		peers = append(peers, peer)
+
+		err := peer.StartBgp(context.Background(), &api.StartBgpRequest{
+			Global: &api.Global{
+				Asn:             s.asn,
+				RouterId:        s.routerId,
+				ListenAddresses: []string{fmt.Sprintf("127.0.0.%d", 100+i)},
+				ListenPort:      10179,
+			},
+		})
+		require.NoError(test, err)
+	}
+
+	wg := waitEstablished(peers[0])
+	wg1 := waitEstablished(peers[1])
+	wg2 := waitEstablished(peers[2])
+	// Use only eBGP
+	for i, server := range peers {
+		for j, peer := range peers {
+			if i == j || server.bgpConfig.Global.Config.As == peer.bgpConfig.Global.Config.As {
+				continue
+			}
+			ctx := context.Background()
+			if err := peerTwoServers(test, ctx, server, peer, []oc.AfiSafiType{oc.AFI_SAFI_TYPE_IPV4_UNICAST}, i < j, setPeerAddressOpt); err != nil {
+				assert.NoError(test, err)
+			}
+		}
+	}
+
+	wg.Wait()
+	wg1.Wait()
+	wg2.Wait()
+
+	family4 := &api.Family{
+		Afi:  api.Family_AFI_IP,
+		Safi: api.Family_SAFI_UNICAST,
+	}
+	attrs := []*api.Attribute{
+		{
+			Attr: &api.Attribute_Origin{Origin: &api.OriginAttribute{
+				Origin: 0,
+			}},
+		},
+		{
+			Attr: &api.Attribute_NextHop{NextHop: &api.NextHopAttribute{
+				NextHop: "10.0.0.1",
+			}},
+		},
+	}
+
+	nlri := &api.NLRI{Nlri: &api.NLRI_Prefix{Prefix: &api.IPAddressPrefix{
+		Prefix:    "10.1.0.0",
+		PrefixLen: 24,
+	}}}
+
+	assertPathCount := func(t assert.TestingT, peer *BgpServer, expected int) {
+		var info *table.TableInfo
+		if peer.active() == nil {
+			info, _ = peer.getRibInfo("", bgp.RF_IPv4_UC)
+		} else {
+			info = peer.globalRib.Tables[bgp.RF_IPv4_UC].Info()
+		}
+		if assert.NotNil(t, info) {
+			assert.Equal(t, expected, info.NumPath)
+		}
+	}
+
+	path := &api.Path{
+		Family: family4,
+		Nlri:   nlri,
+		Pattrs: attrs,
+	}
+	addPaths := func(peers []*BgpServer, path *api.Path) {
+		for _, peer := range peers {
+			_, err := peer.AddPath(apiutil.AddPathRequest{Paths: []*apiutil.Path{mustApi2apiutilPath(path)}})
+			assert.NoError(test, err)
+		}
+	}
+
+	addPaths(peers, path)
+
+	assert.EventuallyWithT(test, func(collect *assert.CollectT) {
+		assertPathCount(collect, peers[0], 3)
+		assertPathCount(collect, peers[1], 2)
+		assertPathCount(collect, peers[2], 2)
+	}, 5*time.Second, 1*time.Millisecond)
+
+	err := peers[0].DeletePath(apiutil.DeletePathRequest{Paths: []*apiutil.Path{mustApi2apiutilPath(path)}})
+	assert.NoError(test, err)
+
+	assert.EventuallyWithT(test, func(collect *assert.CollectT) {
+		assertPathCount(collect, peers[0], 2)
+		assertPathCount(collect, peers[1], 1)
+		assertPathCount(collect, peers[2], 1)
+	}, 20*time.Second, 1*time.Millisecond)
+}