server: apply AS path option updates without resetting sessions

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() {

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)}

pkg/config/oc/default.go

@@ -86,23 +86,20 @@ 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() {
 		n.State.NeighborAddress = n.Config.NeighborAddress
 	}
 
 	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()
 

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{

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) ||

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{

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)
 		}

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)