diff --git a/internal/pkg/table/adj.go b/internal/pkg/table/adj.go index 2ec60c027..8227972b3 100644 --- a/internal/pkg/table/adj.go +++ b/internal/pkg/table/adj.go @@ -18,6 +18,7 @@ package table import ( "fmt" "log/slog" + "slices" "github.com/osrg/gobgp/v4/pkg/packet/bgp" ) @@ -40,6 +41,22 @@ func NewAdjRib(logger *slog.Logger, rfList []bgp.Family) *AdjRib { } } +// SetRejected replaces a cached path without retaining another layer of path +// history. Attribute slices are copied so later changes cannot alter old views. +func (adj *AdjRib) SetRejected(path *Path, rejected bool) *Path { + if path.IsRejected() == rejected { + return path + } + updated := path.Clone(false) + updated.parent = path.parent + updated.info = path.info + updated.pathAttrs = slices.Clone(path.pathAttrs) + updated.dels = slices.Clone(path.dels) + updated.SetRejected(rejected) + adj.Update([]*Path{updated}) + return updated +} + func (adj *AdjRib) Update(pathList []*Path) { for _, path := range pathList { if path == nil || path.IsEOR() { diff --git a/internal/pkg/table/adj_test.go b/internal/pkg/table/adj_test.go index 2c624b0c4..38de30936 100644 --- a/internal/pkg/table/adj_test.go +++ b/internal/pkg/table/adj_test.go @@ -73,6 +73,67 @@ func TestAddPath(t *testing.T) { assert.Equal(t, 0, len(adj.table[family].GetDestinations())) } +func TestAdjRibSetRejectedPreservesPath(t *testing.T) { + for _, modified := range []bool{false, true} { + name := "received" + if modified { + name = "modified" + } + t.Run(name, func(t *testing.T) { + family := bgp.RF_IPv4_UC + families := []bgp.Family{family} + adj := NewAdjRib(logger, families) + nlri, err := bgp.NewIPAddrPrefix(netip.MustParsePrefix("10.83.0.0/24")) + require.NoError(t, err) + source := &PeerInfo{Address: netip.MustParseAddr("192.0.2.1"), AS: 65001} + path := NewPath(family, source, bgp.PathNLRI{NLRI: nlri, ID: 11}, false, []bgp.PathAttributeInterface{ + bgp.NewPathAttributeOrigin(0), bgp.NewPathAttributeMultiExitDisc(50), bgp.NewPathAttributeLocalPref(100), + }, time.Now(), true) + path.localID = 22 + path.IsNexthopInvalid = true + path.MarkStale(true) + path.SetIsFromExternal(true) + if modified { + path = path.Clone(false) + require.NoError(t, path.SetMed(60, true)) + path.RemoveLocalPref() + } + adj.Update([]*Path{path}) + initial, parent := path, path.parent + attrs, hash := path.GetPathAttrs(), path.GetHash() + for range 8 { + previous := path + rejected := !previous.IsRejected() + path = adj.SetRejected(previous, rejected) + assert.NotSame(t, previous, path) + assert.Equal(t, !rejected, previous.IsRejected()) + assert.True(t, path.parent == parent, "cached ancestry must not grow") + assert.Equal(t, attrs, path.GetPathAttrs()) + assert.Equal(t, hash, path.GetHash()) + assert.Same(t, source, path.GetSource()) + assert.Equal(t, initial.GetTimestamp(), path.GetTimestamp()) + assert.Equal(t, uint32(11), path.RemoteID()) + assert.Equal(t, uint32(22), path.LocalID()) + assert.True(t, path.IsStale()) + assert.True(t, path.IsFromExternal()) + assert.True(t, path.NoImplicitWithdraw()) + assert.True(t, path.IsNexthopInvalid) + assert.False(t, path.IsWithdraw) + assert.Equal(t, 1, adj.Count(families)) + accepted := 1 + if rejected { + accepted = 0 + } + assert.Equal(t, accepted, adj.Accepted(families)) + assert.Same(t, path, adj.SetRejected(path, rejected)) + } + // An attribute edit on the replacement must not mutate retained old views. + require.NoError(t, path.SetMed(99, true)) + assert.Equal(t, attrs, initial.GetPathAttrs()) + }) + } +} + func TestAddPathAdjOut(t *testing.T) { pi := &PeerInfo{} attrs := []bgp.PathAttributeInterface{bgp.NewPathAttributeOrigin(0)} diff --git a/pkg/config/oc/default.go b/pkg/config/oc/default.go index 3c3e59969..a47274c3d 100644 --- a/pkg/config/oc/default.go +++ b/pkg/config/oc/default.go @@ -86,14 +86,13 @@ func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Glo n.State.PeerType = n.Config.PeerType if n.Config.PeerType == PEER_TYPE_EXTERNAL { n.State.RemovePrivateAs = n.Config.RemovePrivateAs - n.AsPathOptions.State.ReplacePeerAs = n.AsPathOptions.Config.ReplacePeerAs } else { if string(n.Config.RemovePrivateAs) != "" { return fmt.Errorf("can't set remove-private-as for iBGP peer") } - if n.AsPathOptions.Config.ReplacePeerAs { - return fmt.Errorf("can't set replace-peer-as for iBGP peer") - } + } + if err := SetNeighborAsPathOptions(n, nil); err != nil { + return err } if !n.State.NeighborAddress.IsValid() { @@ -101,8 +100,6 @@ func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Glo } n.State.PeerAs = n.Config.PeerAs - n.AsPathOptions.State.AllowOwnAs = n.AsPathOptions.Config.AllowOwnAs - n.AsPathOptions.State.AllowAsPathLoopLocal = n.AsPathOptions.Config.AllowAsPathLoopLocal if !v.IsSet("neighbor.error-handling.config.treat-as-withdraw") { n.ErrorHandling.Config.TreatAsWithdraw = true @@ -525,6 +522,23 @@ func setDefaultConfigValuesWithViper(v *viper.Viper, b *BgpConfigSet) error { return nil } +// SetNeighborAsPathOptions applies AS path option inheritance and state without +// defaulting unrelated fields. The neighbor's peer type must already be set. +func SetNeighborAsPathOptions(n *Neighbor, pg *PeerGroup) error { + if pg != nil { + v := viper.New() + if fields, ok := configuredFields[n.Config.NeighborAddress.String()]; ok { + v.Set("neighbor", fields) + } + overwriteConfig(&n.AsPathOptions.Config, &pg.AsPathOptions.Config, "neighbor.as-path-options.config", v) + } + if n.Config.PeerType == PEER_TYPE_INTERNAL && n.AsPathOptions.Config.ReplacePeerAs { + return fmt.Errorf("can't set replace-peer-as for iBGP peer") + } + n.AsPathOptions.State = AsPathOptionsState(n.AsPathOptions.Config) + return nil +} + func OverwriteNeighborConfigWithPeerGroup(c *Neighbor, pg *PeerGroup) error { v := viper.New() diff --git a/pkg/config/oc/default_test.go b/pkg/config/oc/default_test.go index 8145cdf30..d40db5aee 100644 --- a/pkg/config/oc/default_test.go +++ b/pkg/config/oc/default_test.go @@ -33,6 +33,32 @@ func registerConfiguredFields(t *testing.T, addr string, fields map[string]any) configuredFields = map[string]any{addr: fields} } +func TestSetNeighborAsPathOptions(t *testing.T) { + registerConfiguredFields(t, testNeighborAddress, map[string]any{ + "as-path-options": map[string]any{"config": map[string]any{"allow-own-as": 1}}, + }) + n := &Neighbor{ + Config: NeighborConfig{NeighborAddress: netip.MustParseAddr(testNeighborAddress), PeerType: PEER_TYPE_EXTERNAL, LocalAs: 65010, PeerAs: 65001}, + AsPathOptions: AsPathOptions{Config: AsPathOptionsConfig{AllowOwnAs: 2}}, + Timers: Timers{Config: TimersConfig{HoldTime: 45}}, + } + pg := &PeerGroup{ + Config: PeerGroupConfig{LocalAs: 65020, PeerAs: 65002}, + AsPathOptions: AsPathOptions{Config: AsPathOptionsConfig{AllowOwnAs: 3, ReplacePeerAs: true, AllowAsPathLoopLocal: true}}, + Timers: Timers{Config: TimersConfig{HoldTime: 90}}, + } + config, timers := n.Config, n.Timers + require.NoError(t, SetNeighborAsPathOptions(n, pg)) + want := AsPathOptionsConfig{AllowOwnAs: 2, ReplacePeerAs: true, AllowAsPathLoopLocal: true} + assert.Equal(t, want, n.AsPathOptions.Config) + assert.Equal(t, AsPathOptionsState(want), n.AsPathOptions.State) + assert.Equal(t, config, n.Config) + assert.Equal(t, timers, n.Timers) + + n.Config.PeerType = PEER_TYPE_INTERNAL + require.EqualError(t, SetNeighborAsPathOptions(n, pg), "can't set replace-peer-as for iBGP peer") +} + func newNeighborForTcpAoInheritanceTest() *Neighbor { return &Neighbor{ Config: NeighborConfig{ diff --git a/pkg/config/oc/util.go b/pkg/config/oc/util.go index 351df17a5..87d789306 100644 --- a/pkg/config/oc/util.go +++ b/pkg/config/oc/util.go @@ -222,7 +222,6 @@ func (n *Neighbor) NeedsResendOpenMessage(new *Neighbor) bool { return !n.Config.Equal(&new.Config) || !n.Transport.Config.Equal(&new.Transport.Config) || !n.AddPaths.Config.Equal(&new.AddPaths.Config) || - !n.AsPathOptions.Config.Equal(&new.AsPathOptions.Config) || !n.GracefulRestart.Config.Equal(&new.GracefulRestart.Config) || isAfiSafiChanged(n.AfiSafis, new.AfiSafis) || !n.EbgpMultihop.Config.Equal(&new.EbgpMultihop.Config) || diff --git a/pkg/config/oc/util_test.go b/pkg/config/oc/util_test.go index 06be6286a..22da2cc1e 100644 --- a/pkg/config/oc/util_test.go +++ b/pkg/config/oc/util_test.go @@ -86,6 +86,37 @@ func TestIsAfiSafiChanged(t *testing.T) { assert.True(t, isAfiSafiChanged(old, new)) } +func TestNeedsResendOpenMessageASPathOptions(t *testing.T) { + for _, tc := range []struct { + name string + change func(*Neighbor) + hard bool + }{ + {"unchanged", func(*Neighbor) {}, false}, + {"allow-own-as", func(n *Neighbor) { n.AsPathOptions.Config.AllowOwnAs = 1 }, false}, + {"replace-peer-as", func(n *Neighbor) { n.AsPathOptions.Config.ReplacePeerAs = true }, false}, + {"allow-as-path-loop-local", func(n *Neighbor) { n.AsPathOptions.Config.AllowAsPathLoopLocal = true }, false}, + {"peer-as", func(n *Neighbor) { n.Config.PeerAs = 65001 }, true}, + {"local-as", func(n *Neighbor) { n.Config.LocalAs = 65000 }, true}, + {"admin-down", func(n *Neighbor) { n.Config.AdminDown = true }, true}, + {"transport", func(n *Neighbor) { n.Transport.Config.RemotePort = 1179 }, true}, + {"add-path", func(n *Neighbor) { n.AddPaths.Config.Receive = true }, true}, + {"graceful-restart", func(n *Neighbor) { n.GracefulRestart.Config.Enabled = true }, true}, + {"afi-safi", func(n *Neighbor) { + n.AfiSafis = []AfiSafi{{Config: AfiSafiConfig{AfiSafiName: AFI_SAFI_TYPE_IPV4_UNICAST}}} + }, true}, + {"multihop", func(n *Neighbor) { n.EbgpMultihop.Config.Enabled = true }, true}, + {"ttl-security", func(n *Neighbor) { n.TtlSecurity.Config.Enabled = true }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + old, next := &Neighbor{}, &Neighbor{} + tc.change(next) + assert.Equal(t, tc.hard, old.NeedsResendOpenMessage(next)) + assert.Equal(t, tc.hard, next.NeedsResendOpenMessage(old)) + }) + } +} + func newPeerFromConfigForBFDTest(t *testing.T, bfd Bfd) *api.Peer { t.Helper() n := &Neighbor{ diff --git a/pkg/server/peer.go b/pkg/server/peer.go index 2414323e2..5da116171 100644 --- a/pkg/server/peer.go +++ b/pkg/server/peer.go @@ -657,6 +657,35 @@ func (peer *peer) updatePrefixLimitConfig(conf *oc.Neighbor, c []oc.AfiSafi) (bo return reachLimit, nil } +// isPathRejected applies the same loop checks to received and cached routes. +func (peer *peer) isPathRejected(path *table.Path) bool { + if path.IsWithdraw { + return false + } + conf := peer.fsm.pConf.ReadOnly() + peer.fsm.lock.Lock() + confedEnabled := peer.fsm.gConf.Confederation.Config.Enabled + confedID := peer.fsm.gConf.Confederation.Config.Identifier + routerID := peer.fsm.gConf.Config.RouterId + peer.fsm.lock.Unlock() + + // RFC4271 9.1.2 and RFC5065 4: exclude AS loops from route selection, + // including occurrences of the Confederation ID. + if aspath := path.GetAsPath(); aspath != nil { + if hasOwnASLoop(conf.Config.LocalAs, int(conf.AsPathOptions.Config.AllowOwnAs), aspath, confedID, confedEnabled) { + return true + } + } + // RFC4456 8: ignore a route with our own ORIGINATOR_ID. + if conf.State.PeerType == oc.PEER_TYPE_INTERNAL && path.GetOriginatorID() == routerID { + peer.fsm.logger.Debug("Originator ID is mine, ignore", + slog.String("OriginatorID", path.GetOriginatorID().String()), + slog.String("Data", path.String())) + return true + } + return false +} + func (peer *peer) handleUpdate(e *fsmMsg) ([]*table.Path, []bgp.Family, bool) { m := e.MsgData.(*bgp.BGPMessage) update := m.Body.(*bgp.BGPUpdate) @@ -685,42 +714,9 @@ func (peer *peer) handleUpdate(e *fsmMsg) ([]*table.Path, []bgp.Family, bool) { eor = append(eor, family) continue } - // RFC4271 9.1.2 Phase 2: Route Selection - // - // If the AS_PATH attribute of a BGP route contains an AS loop, the BGP - // route should be excluded from the Phase 2 decision function. - if aspath := path.GetAsPath(); aspath != nil { - localAS := conf.Config.LocalAs - allowOwnAS := int(conf.AsPathOptions.Config.AllowOwnAs) - - // RFC 5065 Section 4: Get Confederation ID for AS loop detection - // Copy primitive values while holding the lock to avoid data race - peer.fsm.lock.Lock() - confedEnabled := peer.fsm.gConf.Confederation.Config.Enabled - confedID := peer.fsm.gConf.Confederation.Config.Identifier - peer.fsm.lock.Unlock() - - if hasOwnASLoop(localAS, allowOwnAS, aspath, confedID, confedEnabled) { - path.SetRejected(true) - continue - } - } - // RFC4456 8. Avoiding Routing Information Loops - // A router that recognizes the ORIGINATOR_ID attribute SHOULD - // ignore a route received with its BGP Identifier as the ORIGINATOR_ID. - isIBGPPeer := peer.isIBGPPeer() - peer.fsm.lock.Lock() - routerId := peer.fsm.gConf.Config.RouterId - peer.fsm.lock.Unlock() - if isIBGPPeer { - if path.GetOriginatorID() == routerId { - peer.fsm.logger.Debug("Originator ID is mine, ignore", - slog.String("OriginatorID", path.GetOriginatorID().String()), - slog.String("Data", path.String())) - - path.SetRejected(true) - continue - } + if peer.isPathRejected(path) { + path.SetRejected(true) + continue } paths = append(paths, path) } diff --git a/pkg/server/server.go b/pkg/server/server.go index 20d4e88d0..e41bb3f3d 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -2841,7 +2841,19 @@ func (s *BgpServer) softResetIn(addr string, family bgp.Family) error { return err } for _, peer := range peers { - s.propagateUpdate(peer, peer.adjRibIn.PathList(familiesForSoftreset(peer, family), true)) + paths := peer.adjRibIn.PathList(familiesForSoftreset(peer, family), false) + pathList := make([]*table.Path, 0, len(paths)) + for _, path := range paths { + rejected := peer.isPathRejected(path) + path = peer.adjRibIn.SetRejected(path, rejected) + if rejected { + // An UPDATE received after the config change may already have marked + // the cache rejected while the previously accepted route is still installed. + path = path.Clone(true) + } + pathList = append(pathList, path) + } + s.propagateUpdate(peer, pathList) } return err } @@ -3855,6 +3867,9 @@ func (s *BgpServer) updatePeerGroup(pg *oc.PeerGroup) (needsSoftResetIn bool, er s.peerGroupMap[name].Conf = pg for _, n := range s.peerGroupMap[name].members { + if err := oc.SetNeighborAsPathOptions(&n, pg); err != nil { + return needsSoftResetIn, err + } u, err := s.updateNeighbor(&n) if err != nil { return needsSoftResetIn, err @@ -3881,6 +3896,7 @@ func (s *BgpServer) UpdatePeerGroup(ctx context.Context, r *api.UpdatePeerGroupR } func (s *BgpServer) updateNeighbor(c *oc.Neighbor) (needsSoftResetIn bool, err error) { + needsSoftResetOut := false var pgConf *oc.PeerGroup if c.Config.PeerGroup != "" { if pg, ok := s.peerGroupMap[c.Config.PeerGroup]; ok { @@ -3921,7 +3937,10 @@ func (s *BgpServer) updateNeighbor(c *oc.Neighbor) (needsSoftResetIn bool, err e if !original.AsPathOptions.Config.Equal(&c.AsPathOptions.Config) { peer.fsm.logger.Info("Update aspath options") - needsSoftResetIn = true + needsSoftResetIn = needsSoftResetIn || original.AsPathOptions.Config.AllowOwnAs != c.AsPathOptions.Config.AllowOwnAs + needsSoftResetOut = original.AsPathOptions.Config.ReplacePeerAs != c.AsPathOptions.Config.ReplacePeerAs || + original.AsPathOptions.Config.AllowAsPathLoopLocal != c.AsPathOptions.Config.AllowAsPathLoopLocal + conf.AsPathOptions = c.AsPathOptions } bfdConfigChanged := !original.Bfd.Config.Equal(&c.Bfd.Config) @@ -3978,6 +3997,10 @@ func (s *BgpServer) updateNeighbor(c *oc.Neighbor) (needsSoftResetIn bool, err e if err == nil { peer.fsm.pConf.Update(&conf) peer.fsm.lock.Unlock() + if pgConf != nil { + // Retain current explicit member values for subsequent group updates. + s.peerGroupMap[conf.Config.PeerGroup].AddMember(conf) + } if bfdConfigChanged { err = s.updateBfdPeer( addr, @@ -3990,6 +4013,9 @@ func (s *BgpServer) updateNeighbor(c *oc.Neighbor) (needsSoftResetIn bool, err e err = s.setAdminState(addr, "", adminStatePfxCt) } } + if err == nil && needsSoftResetOut { + err = s.softResetOut(addr, bgp.Family(0), false) + } } else { // rollback to original ApplyPolicy peer.fsm.pConf.Update(original) diff --git a/pkg/server/server_aspath_session_test.go b/pkg/server/server_aspath_session_test.go new file mode 100644 index 000000000..0a7465078 --- /dev/null +++ b/pkg/server/server_aspath_session_test.go @@ -0,0 +1,218 @@ +package server + +import ( + "context" + "net" + "net/netip" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/osrg/gobgp/v4/api" + "github.com/osrg/gobgp/v4/pkg/apiutil" + "github.com/osrg/gobgp/v4/pkg/packet/bgp" +) + +func listASPathTestPaths(s *BgpServer, typ api.TableType, prefix string) ([]*apiutil.Path, error) { + var result []*apiutil.Path + name := "" + if typ != api.TableType_TABLE_TYPE_GLOBAL { + name = "127.0.0.1" + } + err := s.ListPath(apiutil.ListPathRequest{ + TableType: typ, Family: bgp.RF_IPv4_UC, Name: name, EnableFiltered: true, + Prefixes: []*apiutil.LookupPrefix{{Prefix: prefix, LookupOption: apiutil.LOOKUP_EXACT}}, + }, func(_ bgp.NLRI, paths []*apiutil.Path) { result = append(result, paths...) }) + return result, err +} + +func TestASPathOptionsKeepEstablishedSession(t *testing.T) { + for _, groupUpdate := range []bool{false, true} { + name := "peer" + if groupUpdate { + name = "peer-group" + } + t.Run(name, func(t *testing.T) { + ctx := context.Background() + listener, err := net.Listen("tcp4", "127.0.0.1:0") + require.NoError(t, err) + port := listener.Addr().(*net.TCPAddr).Port + require.NoError(t, listener.Close()) + receiver, sender := NewBgpServer(), NewBgpServer() + for i, s := range []*BgpServer{receiver, sender} { + go s.Serve() + listenPort := int32(-1) + routerID := "192.0.2.1" + if i == 0 { + listenPort = int32(port) + routerID = "192.0.2.254" + } + require.NoError(t, s.StartBgp(ctx, &api.StartBgpRequest{Global: &api.Global{ + Asn: uint32(65000 + i), RouterId: routerID, ListenPort: listenPort, ListenAddresses: []string{"127.0.0.1"}, + }})) + t.Cleanup(s.Stop) + } + neighbor := &api.Peer{ + Conf: &api.PeerConf{NeighborAddress: "127.0.0.1", PeerAsn: 65001}, + Transport: &api.Transport{PassiveMode: true, LocalAddress: "127.0.0.1"}, + } + group := &api.PeerGroup{ + Conf: &api.PeerGroupConf{PeerGroupName: "as-options-live", PeerAsn: 65001}, + Transport: &api.Transport{PassiveMode: true, LocalAddress: "127.0.0.1"}, + } + if groupUpdate { + require.NoError(t, receiver.AddPeerGroup(ctx, &api.AddPeerGroupRequest{PeerGroup: group})) + neighbor.Conf.PeerGroup = group.Conf.PeerGroupName + } + require.NoError(t, receiver.AddPeer(ctx, &api.AddPeerRequest{Peer: neighbor})) + require.NoError(t, sender.AddPeer(ctx, &api.AddPeerRequest{Peer: &api.Peer{ + Conf: &api.PeerConf{NeighborAddress: "127.0.0.1", PeerAsn: 65000, AllowAspathLoopLocal: true, AllowOwnAsn: 10}, + Transport: &api.Transport{RemotePort: uint32(port), LocalAddress: "127.0.0.1"}, + Timers: &api.Timers{Config: &api.TimersConfig{ConnectRetry: 1}}, + }})) + for _, s := range []*BgpServer{receiver, sender} { + waitPeerState(t, s, api.PeerState_SESSION_STATE_ESTABLISHED, 10*time.Second) + } + + getPeer := func(s *BgpServer) *api.Peer { + t.Helper() + var p *api.Peer + require.NoError(t, s.ListPeer(ctx, &api.ListPeerRequest{Address: "127.0.0.1"}, func(got *api.Peer) { p = got })) + require.NotNil(t, p) + return p + } + var leftEstablished atomic.Bool + watchCtx, cancel := context.WithCancel(ctx) + defer cancel() + before := []*api.Peer{getPeer(receiver), getPeer(sender)} + for _, s := range []*BgpServer{receiver, sender} { + require.NoError(t, s.WatchEvent(watchCtx, WatchEventMessageCallbacks{ + OnPeerUpdate: func(event *apiutil.WatchEventMessage_PeerEvent, _ time.Time) { + if event.Type == apiutil.PEER_EVENT_STATE && event.Peer.State.SessionState != bgp.BGP_FSM_ESTABLISHED { + leftEstablished.Store(true) + } + }, + }, WatchPeer())) + } + checkSession := func() { + t.Helper() + for i, s := range []*BgpServer{receiver, sender} { + got, old := getPeer(s), before[i] + assert.Equal(t, api.PeerState_SESSION_STATE_ESTABLISHED, got.State.SessionState) + assert.Equal(t, old.Timers.State.Uptime.AsTime(), got.Timers.State.Uptime.AsTime()) + assert.Equal(t, old.State.Flops, got.State.Flops) + assert.Equal(t, old.State.Messages.Received.Open, got.State.Messages.Received.Open) + assert.Equal(t, old.State.Messages.Sent.Open, got.State.Messages.Sent.Open) + assert.Equal(t, old.State.Messages.Received.Notification, got.State.Messages.Received.Notification) + assert.Equal(t, old.State.Messages.Sent.Notification, got.State.Messages.Sent.Notification) + assert.Equal(t, old.Transport.LocalPort, got.Transport.LocalPort) + assert.Equal(t, old.Transport.RemotePort, got.Transport.RemotePort) + } + assert.False(t, leftEstablished.Load(), "a state event left Established") + } + update := func(allow uint32, replace, loopLocal bool) { + t.Helper() + oldAllow := getPeer(receiver).Conf.AllowOwnAsn + var needsIn bool + if groupUpdate { + group.Conf.AllowOwnAsn, group.Conf.ReplacePeerAsn, group.Conf.AllowAspathLoopLocal = allow, replace, loopLocal + rsp, err := receiver.UpdatePeerGroup(ctx, &api.UpdatePeerGroupRequest{PeerGroup: group}) + require.NoError(t, err) + needsIn = rsp.NeedsSoftResetIn + } else { + neighbor.Conf.AllowOwnAsn, neighbor.Conf.ReplacePeerAsn, neighbor.Conf.AllowAspathLoopLocal = allow, replace, loopLocal + rsp, err := receiver.UpdatePeer(ctx, &api.UpdatePeerRequest{Peer: neighbor}) + require.NoError(t, err) + needsIn = rsp.NeedsSoftResetIn + } + assert.Equal(t, oldAllow != allow, needsIn) + got := getPeer(receiver) + assert.Equal(t, allow, got.Conf.AllowOwnAsn) + assert.Equal(t, replace, got.Conf.ReplacePeerAsn) + assert.Equal(t, loopLocal, got.Conf.AllowAspathLoopLocal) + if needsIn { + require.NoError(t, receiver.ResetPeer(ctx, &api.ResetPeerRequest{ + Address: "127.0.0.1", Soft: true, Direction: api.ResetPeerRequest_DIRECTION_IN, + })) + } + checkSession() + } + addPath := func(s *BgpServer, prefix string, asns ...uint32) { + t.Helper() + nlri, err := bgp.NewIPAddrPrefix(netip.MustParsePrefix(prefix)) + require.NoError(t, err) + nh, err := bgp.NewPathAttributeNextHop(netip.MustParseAddr("127.0.0.1")) + require.NoError(t, err) + results, err := s.AddPath(apiutil.AddPathRequest{Paths: []*apiutil.Path{{ + Family: bgp.RF_IPv4_UC, Nlri: nlri, + Attrs: []bgp.PathAttributeInterface{bgp.NewPathAttributeOrigin(0), nh, bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_SEQ, asns)})}, + }}}) + require.NoError(t, err) + require.Len(t, results, 1) + for _, result := range results { + require.NoError(t, result.Error) + } + } + waitPaths := func(s *BgpServer, typ api.TableType, prefix string, count int) { + t.Helper() + require.EventuallyWithT(t, func(c *assert.CollectT) { + paths, err := listASPathTestPaths(s, typ, prefix) + assert.NoError(c, err) + assert.Len(c, paths, count) + }, 5*time.Second, 10*time.Millisecond) + } + + const inbound = "10.0.0.0/24" + addPath(sender, inbound, 65000) + waitPaths(receiver, api.TableType_TABLE_TYPE_ADJ_IN, inbound, 1) + waitPaths(receiver, api.TableType_TABLE_TYPE_GLOBAL, inbound, 0) + for _, allow := range []uint32{1, 0, 1} { + update(allow, false, false) + waitPaths(receiver, api.TableType_TABLE_TYPE_GLOBAL, inbound, int(allow)) + p := getPeer(receiver) + require.Len(t, p.AfiSafis, 1) + assert.Equal(t, uint64(1), p.AfiSafis[0].State.Received) + assert.Equal(t, uint64(allow), p.AfiSafis[0].State.Accepted) + } + const outbound = "10.0.1.0/24" + addPath(receiver, outbound, 65001) + waitPaths(sender, api.TableType_TABLE_TYPE_GLOBAL, outbound, 0) + for _, replace := range []bool{true, false} { + update(1, replace, false) + count := 0 + if replace { + count = 1 + } + waitPaths(sender, api.TableType_TABLE_TYPE_GLOBAL, outbound, count) + if replace { + paths, err := listASPathTestPaths(sender, api.TableType_TABLE_TYPE_GLOBAL, outbound) + require.NoError(t, err) + require.Len(t, paths, 1) + found := false + for _, attr := range paths[0].Attrs { + if aspath, ok := attr.(*bgp.PathAttributeAsPath); ok { + found = true + require.Len(t, aspath.Value, 1) + assert.Equal(t, []uint32{65000, 65000}, aspath.Value[0].GetAS()) + } + } + assert.True(t, found, "the received route must contain AS_PATH") + } + } + for _, allowLocal := range []bool{true, false} { + update(1, false, allowLocal) + count := 0 + if allowLocal { + count = 1 + } + waitPaths(sender, api.TableType_TABLE_TYPE_GLOBAL, outbound, count) + } + // Check the event stream after the final route change has crossed the session. + assert.Never(t, leftEstablished.Load, 100*time.Millisecond, 10*time.Millisecond) + checkSession() + }) + } +} diff --git a/pkg/server/server_aspath_test.go b/pkg/server/server_aspath_test.go new file mode 100644 index 000000000..e1d2ead41 --- /dev/null +++ b/pkg/server/server_aspath_test.go @@ -0,0 +1,497 @@ +package server + +import ( + "context" + "net/netip" + "reflect" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/osrg/gobgp/v4/api" + "github.com/osrg/gobgp/v4/internal/pkg/table" + "github.com/osrg/gobgp/v4/pkg/apiutil" + "github.com/osrg/gobgp/v4/pkg/config/oc" + "github.com/osrg/gobgp/v4/pkg/packet/bgp" +) + +func newASPathTestPeer(t *testing.T, localAS, remoteAS uint32) (*BgpServer, *peer) { + t.Helper() + s := NewBgpServer() + go s.Serve() + require.NoError(t, s.StartBgp(context.Background(), &api.StartBgpRequest{Global: &api.Global{ + Asn: localAS, RouterId: "192.0.2.254", ListenPort: -1, + }})) + p := newPeerandInfo(t, localAS, remoteAS, "192.0.2.1", s.globalRib) + p.policy = s.policy + p.fsm.gConf.Config.RouterId = netip.MustParseAddr("192.0.2.254") + require.NoError(t, s.mgmtOperation(func() error { + s.neighborMap[netip.MustParseAddr(p.ID())] = p + return nil + }, true)) + t.Cleanup(func() { + require.NoError(t, s.mgmtOperation(func() error { + delete(s.neighborMap, netip.MustParseAddr(p.ID())) + return nil + }, false)) + cleanInfiniteChannel(p.fsm.outgoingCh) + s.Stop() + }) + return s, p +} + +func asPathTestPath(t *testing.T, p *peer, prefix string, id uint32, asns ...uint32) *table.Path { + t.Helper() + nlri, err := bgp.NewIPAddrPrefix(netip.MustParsePrefix(prefix)) + require.NoError(t, err) + family := bgp.RF_IPv4_UC + nextHop := netip.MustParseAddr("192.0.2.1") + if nlri.Prefix.Addr().Is6() { + family = bgp.RF_IPv6_UC + nextHop = netip.MustParseAddr("2001:db8::1") + } + attrs := []bgp.PathAttributeInterface{ + bgp.NewPathAttributeOrigin(0), + bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_SEQ, asns)}), + } + if family == bgp.RF_IPv4_UC { + nh, err := bgp.NewPathAttributeNextHop(nextHop) + require.NoError(t, err) + attrs = append(attrs, nh) + } else { + mp, err := bgp.NewPathAttributeMpReachNLRI(family, []bgp.PathNLRI{{NLRI: nlri, ID: id}}, nextHop) + require.NoError(t, err) + attrs = append(attrs, mp) + } + return table.NewPath(family, p.peerInfo.Load(), bgp.PathNLRI{NLRI: nlri, ID: id}, false, attrs, time.Now(), false) +} + +func asPathTestUpdate(t *testing.T, path *table.Path) *bgp.BGPMessage { + t.Helper() + nlri := []bgp.PathNLRI{{NLRI: path.GetNlri(), ID: path.RemoteID()}} + if path.GetFamily() == bgp.RF_IPv4_UC { + if path.IsWithdraw { + return bgp.NewBGPUpdateMessage(nlri, nil, nil) + } + return bgp.NewBGPUpdateMessage(nil, path.GetPathAttrs(), nlri) + } + if path.IsWithdraw { + mp, err := bgp.NewPathAttributeMpUnreachNLRI(path.GetFamily(), nlri) + require.NoError(t, err) + return bgp.NewBGPUpdateMessage(nil, []bgp.PathAttributeInterface{mp}, nil) + } + return bgp.NewBGPUpdateMessage(nil, path.GetPathAttrs(), nil) +} + +func TestSoftResetInReevaluatesASPathOptions(t *testing.T) { + for _, prefix := range []string{"10.0.0.0/24", "2001:db8:1::/48"} { + t.Run(prefix, func(t *testing.T) { + s, p := newASPathTestPeer(t, 65000, 65001) + loop := asPathTestPath(t, p, prefix, 11, 65001, 65000) + valid := asPathTestPath(t, p, prefix, 22, 65001) + family := loop.GetFamily() + p.adjRibIn = table.NewAdjRib(logger, []bgp.Family{family}) + p.fsm.lock.Lock() + conf := p.fsm.pConf.ReadCopy() + conf.AfiSafis = []oc.AfiSafi{{State: oc.AfiSafiState{Family: family}}} + p.fsm.pConf.Update(&conf) + p.fsm.lock.Unlock() + require.NoError(t, s.mgmtOperation(func() error { + for _, path := range []*table.Path{loop, valid} { + accepted, _, limit := p.handleUpdate(&fsmMsg{MsgData: asPathTestUpdate(t, path), timestamp: time.Now()}) + assert.False(t, limit) + s.propagateUpdate(p, accepted) + } + return nil + }, true)) + check := func(want int) { + t.Helper() + require.NoError(t, s.mgmtOperation(func() error { + assert.Equal(t, 2, p.adjRibIn.Count([]bgp.Family{family})) + assert.Equal(t, want, p.adjRibIn.Accepted([]bgp.Family{family})) + assert.Len(t, s.globalRib.GetPathList(table.GLOBAL_RIB_NAME, 0, []bgp.Family{family}), want) + return nil + }, true)) + } + check(1) + for _, allow := range []uint8{1, 1, 0, 0, 255, 0} { + p.fsm.lock.Lock() + conf := p.fsm.pConf.ReadCopy() + conf.AsPathOptions.Config.AllowOwnAs = allow + p.fsm.pConf.Update(&conf) + p.fsm.lock.Unlock() + require.NoError(t, s.ResetPeer(context.Background(), &api.ResetPeerRequest{ + Address: p.ID(), Soft: true, Direction: api.ResetPeerRequest_DIRECTION_IN, + })) + want := 1 + if allow > 0 { + want = 2 + } + check(want) + } + // Removing one Add-Path route must leave the other path intact. + require.NoError(t, s.mgmtOperation(func() error { + paths, _, _ := p.handleUpdate(&fsmMsg{MsgData: asPathTestUpdate(t, valid.Clone(true)), timestamp: time.Now()}) + if !assert.Len(t, paths, 1) { + return nil + } + assert.True(t, paths[0].IsWithdraw) + s.propagateUpdate(p, paths) + remaining := p.adjRibIn.PathList([]bgp.Family{family}, false) + if !assert.Len(t, remaining, 1) { + return nil + } + assert.Equal(t, uint32(11), remaining[0].RemoteID()) + assert.True(t, remaining[0].IsRejected()) + assert.Empty(t, s.globalRib.GetPathList(table.GLOBAL_RIB_NAME, 0, []bgp.Family{family})) + return nil + }, true)) + }) + } +} + +func TestSoftResetInPreservesLoopChecksAndImportPolicy(t *testing.T) { + s, p := newASPathTestPeer(t, 65000, 65000) + p.fsm.gConf.Confederation.Config.Enabled = true + p.fsm.gConf.Confederation.Config.Identifier = 65100 + originator, err := bgp.NewPathAttributeOriginatorId(p.fsm.gConf.Config.RouterId) + require.NoError(t, err) + paths := []*table.Path{ + asPathTestPath(t, p, "10.0.0.0/24", 0, 65000), + asPathTestPath(t, p, "10.0.1.0/24", 0, 65100, 65100), + asPathTestPath(t, p, "10.0.2.0/24", 0, 65000), + asPathTestPath(t, p, "10.0.3.0/24", 0, 65001), + } + for _, i := range []int{2, 3} { + paths[i] = table.NewPath(paths[i].GetFamily(), p.peerInfo.Load(), bgp.PathNLRI{NLRI: paths[i].GetNlri()}, false, append(paths[i].GetPathAttrs(), originator), time.Now(), false) + } + require.NoError(t, s.mgmtOperation(func() error { + for _, path := range paths { + accepted, _, _ := p.handleUpdate(&fsmMsg{MsgData: asPathTestUpdate(t, path), timestamp: time.Now()}) + assert.Empty(t, accepted) + } + return nil + }, true)) + setImport := func(action api.RouteAction) { + t.Helper() + require.NoError(t, s.SetPolicyAssignment(context.Background(), &api.SetPolicyAssignmentRequest{ + Assignment: &api.PolicyAssignment{Name: table.GLOBAL_RIB_NAME, Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, DefaultAction: action}, + })) + } + setImport(api.RouteAction_ROUTE_ACTION_REJECT) + p.fsm.lock.Lock() + conf := p.fsm.pConf.ReadCopy() + conf.AsPathOptions.Config.AllowOwnAs = 1 + p.fsm.pConf.Update(&conf) + p.fsm.lock.Unlock() + reset := func() { + t.Helper() + require.NoError(t, s.ResetPeer(context.Background(), &api.ResetPeerRequest{Address: p.ID(), Soft: true, Direction: api.ResetPeerRequest_DIRECTION_IN})) + } + reset() + require.NoError(t, s.mgmtOperation(func() error { + assert.Equal(t, 1, p.adjRibIn.Accepted([]bgp.Family{bgp.RF_IPv4_UC})) + assert.Empty(t, s.globalRib.GetPathList(table.GLOBAL_RIB_NAME, 0, []bgp.Family{bgp.RF_IPv4_UC})) + return nil + }, true)) + setImport(api.RouteAction_ROUTE_ACTION_ACCEPT) + reset() + require.NoError(t, s.mgmtOperation(func() error { + installed := s.globalRib.GetPathList(table.GLOBAL_RIB_NAME, 0, []bgp.Family{bgp.RF_IPv4_UC}) + if !assert.Len(t, installed, 1) { + return nil + } + assert.Equal(t, "10.0.0.0/24", installed[0].GetPrefix()) + // Treat-as-withdraw retains attributes; loop checks must not consume it. + withdrawals, _, _ := p.handleUpdate(&fsmMsg{MsgData: asPathTestUpdate(t, paths[2]), timestamp: time.Now(), handling: bgp.ERROR_HANDLING_TREAT_AS_WITHDRAW}) + if !assert.Len(t, withdrawals, 1) { + return nil + } + assert.True(t, withdrawals[0].IsWithdraw) + return nil + }, true)) +} + +func TestSoftResetInWithdrawsASLoopFromDownstream(t *testing.T) { + s, p := newASPathTestPeer(t, 65000, 65001) + downstream := newPeerandInfo(t, 65000, 65002, "192.0.2.2", s.globalRib) + downstream.policy = s.policy + downstream.fsm.state.Store(bgp.BGP_FSM_ESTABLISHED) + downstream.fsm.familyMap.Store(map[bgp.Family]bgp.BGPAddPathMode{bgp.RF_IPv4_UC: bgp.BGP_ADD_PATH_NONE}) + path := asPathTestPath(t, p, "10.0.0.0/24", 0, 65001, 65000) + path.SetRejected(true) + require.NoError(t, s.mgmtOperation(func() error { + s.neighborMap[netip.MustParseAddr(downstream.ID())] = downstream + p.adjRibIn.Update([]*table.Path{path}) + return nil + }, true)) + t.Cleanup(func() { + require.NoError(t, s.mgmtOperation(func() error { + delete(s.neighborMap, netip.MustParseAddr(downstream.ID())) + return nil + }, false)) + cleanInfiniteChannel(downstream.fsm.outgoingCh) + }) + for _, allow := range []uint8{1, 0} { + p.fsm.lock.Lock() + conf := p.fsm.pConf.ReadCopy() + conf.AsPathOptions.Config.AllowOwnAs = allow + p.fsm.pConf.Update(&conf) + p.fsm.lock.Unlock() + require.NoError(t, s.ResetPeer(context.Background(), &api.ResetPeerRequest{ + Address: p.ID(), Soft: true, Direction: api.ResetPeerRequest_DIRECTION_IN, + })) + select { + case outgoing := <-downstream.fsm.outgoingCh.Out(): + msg := outgoing.(*fsmOutgoingMsg) + require.Len(t, msg.Paths, 1) + assert.Equal(t, path.GetPrefix(), msg.Paths[0].GetPrefix()) + assert.Equal(t, allow == 0, msg.Paths[0].IsWithdraw) + case <-time.After(time.Second): + t.Fatal("timed out waiting for downstream AS path update") + } + } +} + +func TestUpdatePeerASPathOptions(t *testing.T) { + ctx := context.Background() + s := NewBgpServer() + go s.Serve() + require.NoError(t, s.StartBgp(ctx, &api.StartBgpRequest{Global: &api.Global{Asn: 65000, RouterId: "192.0.2.254", ListenPort: -1}})) + t.Cleanup(s.Stop) + p := &api.Peer{Conf: &api.PeerConf{NeighborAddress: "192.0.2.1", PeerAsn: 65001, AdminDown: true}} + require.NoError(t, s.AddPeer(ctx, &api.AddPeerRequest{Peer: p})) + var original *peer + require.NoError(t, s.mgmtOperation(func() error { + original = s.neighborMap[netip.MustParseAddr(p.Conf.NeighborAddress)] + return nil + }, true)) + for _, options := range []oc.AsPathOptionsConfig{ + {AllowOwnAs: 1}, + {AllowOwnAs: 1, ReplacePeerAs: true}, + {AllowOwnAs: 1, ReplacePeerAs: true, AllowAsPathLoopLocal: true}, + {}, + {}, + } { + previousAllow := p.Conf.AllowOwnAsn + p.Conf.AllowOwnAsn = uint32(options.AllowOwnAs) + p.Conf.ReplacePeerAsn = options.ReplacePeerAs + p.Conf.AllowAspathLoopLocal = options.AllowAsPathLoopLocal + rsp, err := s.UpdatePeer(ctx, &api.UpdatePeerRequest{Peer: p}) + require.NoError(t, err) + assert.Equal(t, previousAllow != uint32(options.AllowOwnAs), rsp.NeedsSoftResetIn) + require.NoError(t, s.ListPeer(ctx, &api.ListPeerRequest{Address: p.Conf.NeighborAddress}, func(got *api.Peer) { + assert.Equal(t, p.Conf.AllowOwnAsn, got.Conf.AllowOwnAsn) + assert.Equal(t, p.Conf.ReplacePeerAsn, got.Conf.ReplacePeerAsn) + assert.Equal(t, p.Conf.AllowAspathLoopLocal, got.Conf.AllowAspathLoopLocal) + })) + require.NoError(t, s.mgmtOperation(func() error { + current := s.neighborMap[netip.MustParseAddr(p.Conf.NeighborAddress)] + assert.Same(t, original, current, "AS path options must not recreate the peer") + c := current.fsm.pConf.ReadOnly() + assert.Equal(t, options, c.AsPathOptions.Config) + assert.Equal(t, oc.AsPathOptionsState(options), c.AsPathOptions.State) + return nil + }, true)) + } +} + +func TestUpdatePeerGroupASPathOptions(t *testing.T) { + ctx := context.Background() + s := NewBgpServer() + go s.Serve() + require.NoError(t, s.StartBgp(ctx, &api.StartBgpRequest{Global: &api.Global{Asn: 65000, RouterId: "192.0.2.254", ListenPort: -1}})) + t.Cleanup(s.Stop) + group := &api.PeerGroup{Conf: &api.PeerGroupConf{PeerGroupName: "as-options", PeerAsn: 65001}, Transport: &api.Transport{PassiveMode: true}} + require.NoError(t, s.AddPeerGroup(ctx, &api.AddPeerGroupRequest{PeerGroup: group})) + const addr = "192.0.2.19" + require.NoError(t, s.AddPeer(ctx, &api.AddPeerRequest{Peer: &api.Peer{Conf: &api.PeerConf{NeighborAddress: addr, PeerGroup: group.Conf.PeerGroupName}}})) + const overrideAddr = "192.0.2.20" + // The config-file API records explicit fields for peer-group inheritance. + oc.RegisterConfiguredFields(overrideAddr, map[string]any{ + "config": map[string]any{"local-as": 65010}, + "as-path-options": map[string]any{"config": map[string]any{ + "allow-own-as": 1, "replace-peer-as": false, "allow-as-path-loop-local": false, + }}, + "timers": map[string]any{"config": map[string]any{"hold-time": 45, "keepalive-interval": 15}}, + }) + t.Cleanup(func() { oc.RegisterConfiguredFields(overrideAddr, nil) }) + require.NoError(t, s.AddPeer(ctx, &api.AddPeerRequest{Peer: &api.Peer{ + Conf: &api.PeerConf{NeighborAddress: overrideAddr, PeerGroup: group.Conf.PeerGroupName, LocalAsn: 65010, AllowOwnAsn: 1}, + Timers: &api.Timers{Config: &api.TimersConfig{HoldTime: 45, KeepaliveInterval: 15}}, + }})) + var original *peer + require.NoError(t, s.mgmtOperation(func() error { + original = s.neighborMap[netip.MustParseAddr(addr)] + return nil + }, true)) + for _, enabled := range []bool{true, false} { + group.Conf.AllowOwnAsn = 0 + if enabled { + group.Conf.AllowOwnAsn = 2 + } + group.Conf.ReplacePeerAsn = enabled + group.Conf.AllowAspathLoopLocal = enabled + rsp, err := s.UpdatePeerGroup(ctx, &api.UpdatePeerGroupRequest{PeerGroup: group}) + require.NoError(t, err) + assert.True(t, rsp.NeedsSoftResetIn) + require.NoError(t, s.ListPeer(ctx, &api.ListPeerRequest{Address: addr}, func(got *api.Peer) { + assert.Equal(t, group.Conf.AllowOwnAsn, got.Conf.AllowOwnAsn) + assert.Equal(t, enabled, got.Conf.ReplacePeerAsn) + assert.Equal(t, enabled, got.Conf.AllowAspathLoopLocal) + })) + require.NoError(t, s.ListPeer(ctx, &api.ListPeerRequest{Address: overrideAddr}, func(got *api.Peer) { + assert.Equal(t, uint32(1), got.Conf.AllowOwnAsn) + assert.False(t, got.Conf.ReplacePeerAsn) + assert.False(t, got.Conf.AllowAspathLoopLocal) + assert.Equal(t, uint32(65010), got.Conf.LocalAsn) + assert.Equal(t, uint32(65001), got.Conf.PeerAsn) + assert.Equal(t, uint64(45), got.Timers.Config.HoldTime) + assert.Equal(t, uint64(15), got.Timers.Config.KeepaliveInterval) + })) + require.NoError(t, s.mgmtOperation(func() error { + current := s.neighborMap[netip.MustParseAddr(addr)] + assert.Same(t, original, current) + options := current.fsm.pConf.ReadOnly().AsPathOptions + assert.Equal(t, oc.AsPathOptionsState(options.Config), options.State) + return nil + }, true)) + } +} + +func TestSoftResetInAfterRepeatedASLoopUpdate(t *testing.T) { + for _, addPath := range []bool{false, true} { + name := "single-path" + if addPath { + name = "add-path" + } + t.Run(name, func(t *testing.T) { + ctx := context.Background() + s := NewBgpServer() + go s.Serve() + require.NoError(t, s.StartBgp(ctx, &api.StartBgpRequest{Global: &api.Global{Asn: 65000, RouterId: "192.0.2.254", ListenPort: -1}})) + t.Cleanup(s.Stop) + input := &api.Peer{Conf: &api.PeerConf{NeighborAddress: "192.0.2.81", PeerAsn: 65001, AdminDown: true, AllowOwnAsn: 1}} + require.NoError(t, s.AddPeer(ctx, &api.AddPeerRequest{Peer: input})) + var p *peer + require.NoError(t, s.mgmtOperation(func() error { + p = s.neighborMap[netip.MustParseAddr(input.Conf.NeighborAddress)] + return nil + }, true)) + loop := asPathTestPath(t, p, "10.81.0.0/24", 11, 65001, 65000) + deliver := func(path *table.Path) { + t.Helper() + require.NoError(t, s.mgmtOperation(func() error { + accepted, _, limit := p.handleUpdate(&fsmMsg{MsgData: asPathTestUpdate(t, path), timestamp: time.Now()}) + assert.False(t, limit) + s.propagateUpdate(p, accepted) + return nil + }, true)) + } + globalIDs := func() []uint32 { + t.Helper() + var ids []uint32 + require.NoError(t, s.ListPath(apiutil.ListPathRequest{TableType: api.TableType_TABLE_TYPE_GLOBAL, Family: bgp.RF_IPv4_UC}, func(_ bgp.NLRI, paths []*apiutil.Path) { + for _, path := range paths { + ids = append(ids, path.RemoteID) + } + })) + return ids + } + deliver(loop) + want := []uint32{} + if addPath { + deliver(asPathTestPath(t, p, "10.81.0.0/24", 22, 65001)) + want = append(want, 22) + } + require.Len(t, globalIDs(), 1+len(want)) + input.Conf.AllowOwnAsn = 0 + response, err := s.UpdatePeer(ctx, &api.UpdatePeerRequest{Peer: input}) + require.NoError(t, err) + require.True(t, response.NeedsSoftResetIn) + // A repeated UPDATE can mark the cache rejected before the caller resets IN. + deliver(loop) + for range 2 { + require.NoError(t, s.ResetPeer(ctx, &api.ResetPeerRequest{Address: input.Conf.NeighborAddress, Soft: true, Direction: api.ResetPeerRequest_DIRECTION_IN})) + assert.ElementsMatch(t, want, globalIDs()) + require.NoError(t, s.mgmtOperation(func() error { + assert.Same(t, p, s.neighborMap[netip.MustParseAddr(input.Conf.NeighborAddress)]) + families := []bgp.Family{bgp.RF_IPv4_UC} + assert.Equal(t, 1+len(want), p.adjRibIn.Count(families)) + assert.Equal(t, len(want), p.adjRibIn.Accepted(families)) + return nil + }, true)) + } + }) + } +} + +func TestUpdatePeerGroupPreservesCurrentASPathOverride(t *testing.T) { + ctx := context.Background() + s := NewBgpServer() + go s.Serve() + require.NoError(t, s.StartBgp(ctx, &api.StartBgpRequest{Global: &api.Global{Asn: 65000, RouterId: "192.0.2.254", ListenPort: -1}})) + t.Cleanup(s.Stop) + const addr = "192.0.2.82" + group := &api.PeerGroup{Conf: &api.PeerGroupConf{PeerGroupName: "current-member", PeerAsn: 65001}, Transport: &api.Transport{PassiveMode: true}} + require.NoError(t, s.AddPeerGroup(ctx, &api.AddPeerGroupRequest{PeerGroup: group})) + oc.RegisterConfiguredFields(addr, map[string]any{ + "config": map[string]any{"admin-down": true}, + "as-path-options": map[string]any{"config": map[string]any{"allow-own-as": 1}}, + }) + t.Cleanup(func() { oc.RegisterConfiguredFields(addr, nil) }) + input := &api.Peer{Conf: &api.PeerConf{NeighborAddress: addr, PeerGroup: group.Conf.PeerGroupName, AllowOwnAsn: 1, AdminDown: true}} + require.NoError(t, s.AddPeer(ctx, &api.AddPeerRequest{Peer: input})) + input.Conf.AllowOwnAsn = 2 + response, err := s.UpdatePeer(ctx, &api.UpdatePeerRequest{Peer: input}) + require.NoError(t, err) + require.True(t, response.NeedsSoftResetIn) + for _, replace := range []bool{true, false} { + group.Conf.ReplacePeerAsn = replace + response, err := s.UpdatePeerGroup(ctx, &api.UpdatePeerGroupRequest{PeerGroup: group}) + require.NoError(t, err) + assert.False(t, response.NeedsSoftResetIn) + require.NoError(t, s.ListPeer(ctx, &api.ListPeerRequest{Address: addr}, func(got *api.Peer) { + assert.Equal(t, uint32(2), got.Conf.AllowOwnAsn) + assert.Equal(t, replace, got.Conf.ReplacePeerAsn) + })) + } +} + +func TestSoftResetInASPathCacheHistoryIsBounded(t *testing.T) { + s, p := newASPathTestPeer(t, 65000, 65001) + path := asPathTestPath(t, p, "10.83.0.0/24", 0, 65001, 65000) + path.SetRejected(true) + require.NoError(t, s.mgmtOperation(func() error { + p.adjRibIn.Update([]*table.Path{path}) + return nil + }, true)) + depths := []int{0} + for step := range 8 { + allow := uint8((step + 1) % 2) + p.fsm.lock.Lock() + conf := p.fsm.pConf.ReadCopy() + conf.AsPathOptions.Config.AllowOwnAs = allow + p.fsm.pConf.Update(&conf) + p.fsm.lock.Unlock() + require.NoError(t, s.ResetPeer(context.Background(), &api.ResetPeerRequest{Address: p.ID(), Soft: true, Direction: api.ResetPeerRequest_DIRECTION_IN})) + require.NoError(t, s.mgmtOperation(func() error { + cached := p.adjRibIn.PathList([]bgp.Family{bgp.RF_IPv4_UC}, false) + if !assert.Len(t, cached, 1) { + return nil + } + assert.Equal(t, int(allow), p.adjRibIn.Accepted([]bgp.Family{bgp.RF_IPv4_UC})) + // Inspect private ancestry without adding a public API for this test. + depth := 0 + for v := reflect.ValueOf(cached[0]).Elem().FieldByName("parent"); !v.IsNil() && depth < 16; v = v.Elem().FieldByName("parent") { + depth++ + } + depths = append(depths, depth) + assert.Zero(t, depth, "admission changes must not retain old cached paths") + return nil + }, true)) + } + t.Logf("parent depth after 0..8 admission changes: %v", depths) +} diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index d917ceaa7..3b130cb9d 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -4022,7 +4022,7 @@ func TestUpdatePeer(t *testing.T) { p.Conf.ReplacePeerAsn = true resp, err = s.UpdatePeer(context.Background(), &api.UpdatePeerRequest{Peer: p}) assert.NoError(t, err) - assert.True(t, resp.NeedsSoftResetIn) + assert.False(t, resp.NeedsSoftResetIn) assert.EventuallyWithT(t, func(collect *assert.CollectT) { _ = s.ListPeer(context.Background(), &api.ListPeerRequest{}, func(peer *api.Peer) {