diff --git a/pkg/server/server.go b/pkg/server/server.go index 20d4e88d0..677a14676 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -520,6 +520,12 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { conf := peer.fsm.pConf.ReadOnly() rrClusterID := net.ParseIP(conf.RouteReflector.State.RouteReflectorClusterId.String()) if slices.Equal(clusterID.AsSlice(), rrClusterID.To4()) { + if !path.IsWithdraw && old != nil && peer.hasPathAlreadyBeenSent(old) { + // The new best path cannot be reflected to this RR client, + // but the old best path was previously advertised. Send an + // explicit withdrawal so it is not left stale on the peer. + return old.Clone(true) + } peer.fsm.logger.Debug("cluster list path attribute has local cluster id, ignore", slog.String("ClusterID", clusterID.String()), slog.Any("Path", path)) diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index d917ceaa7..026a5de60 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -4943,3 +4943,61 @@ func TestResetPeerEstablishedSendsNotification(t *testing.T) { n.ErrorSubcode == bgp.BGP_ERROR_SUB_ADMINISTRATIVE_RESET }, 10*time.Second, 10*time.Millisecond) } + +// TestFilterPathRRWithdrawalOnClusterLoop verifies that rejecting a newly +// selected path due to a local RR cluster-loop withdraws the previously +// advertised path to not leave a stale route on the peer. +func TestFilterPathRRWithdrawalOnClusterLoop(t *testing.T) { + const as = uint32(65000) + clusterID := netip.MustParseAddr("255.0.0.2") + + rib := table.NewTableManager(logger, []bgp.Family{bgp.RF_IPv4_UC}) + rrPeer := newPeerandInfo(t, as, as, "192.0.2.1", rib) + + conf := rrPeer.fsm.pConf.ReadCopy() + conf.RouteReflector.Config.RouteReflectorClient = true + conf.RouteReflector.Config.RouteReflectorClusterId = clusterID + conf.RouteReflector.State.RouteReflectorClient = true + conf.RouteReflector.State.RouteReflectorClusterId = clusterID + rrPeer.fsm.pConf.Update(&conf) + + nlri, err := bgp.NewIPAddrPrefix(netip.MustParsePrefix("10.62.0.0/24")) + require.NoError(t, err) + nextHop, err := bgp.NewPathAttributeNextHop(netip.MustParseAddr("0.0.0.0")) + require.NoError(t, err) + clusterList, err := bgp.NewPathAttributeClusterList([]netip.Addr{clusterID}) + require.NoError(t, err) + + oldLocal := table.NewPath( + bgp.RF_IPv4_UC, + nil, + bgp.PathNLRI{NLRI: nlri}, + false, + []bgp.PathAttributeInterface{ + bgp.NewPathAttributeOrigin(bgp.BGP_ORIGIN_ATTR_TYPE_IGP), + nextHop, + }, + time.Now(), + false, + ) + newReflected := table.NewPath( + bgp.RF_IPv4_UC, + rrPeer.peerInfo.Load(), + bgp.PathNLRI{NLRI: nlri}, + false, + []bgp.PathAttributeInterface{ + bgp.NewPathAttributeOrigin(bgp.BGP_ORIGIN_ATTR_TYPE_IGP), + bgp.NewPathAttributeLocalPref(100), + clusterList, + }, + time.Now(), + false, + ) + + rrPeer.updateRoutes(oldLocal) + got := filterpath(rrPeer, newReflected, oldLocal) + require.NotNil(t, got) + require.True(t, got.IsWithdraw, + "a cluster-loop-filtered new best path must withdraw the old advertised path") + require.Equal(t, oldLocal.GetNlri().String(), got.GetNlri().String()) +}