From 309a3c54eea99e26dc72c0fb5e94e7a1107c14de Mon Sep 17 00:00:00 2001 From: Sergey Klyaus Date: Tue, 4 Mar 2025 16:44:39 +0100 Subject: [PATCH 1/2] allow to set individual peer group policies and compute of them once Idea of per-peer-group policy is described in docs/sources/policy.md. Basically: - Instead of sending update to an individual *peer and computing policy for each of it, GoBGP will now compute policy and other path changes for a receiver, which can be either a *peer, or a peer-group (thus computing changes once even if there are 10 peers in a peer-group) - Similarly, when receiving update, per-peer-group policy is checked (stored internally as pg:PGNAME) instead of global which allows to skip neighbor set check. - Default Config/State values in BGP configs now computed both for oc.Peer and oc.PeerGroup table code now relies on PeerInfo structure that contains all relevant fields needed to change path when sending it to a peer. Per-peer-group policy for various types of servers (route-reflector, route-server) are checked by TestPeerStarTopology test. The last change revealed important problem: code which was checking presence of Local AS in received routes was checking for AS configured in Global config section even if LocalAS override was supplied for a peer group. Per https://www.cisco.com/c/en/us/support/docs/ip/border-gateway-protocol-bgp/13761-39.html idea of LocalAS was migratory period when two AS belong to a same router, thus both AS numbers should be checked. So now both checks are performed, but for a new check (LocalAS override) path is imported while warning is issued which is subject to change in future releases. Various invariants of LocalAS config are now checked by TestBgpServerLocalASOverride test. In our tests we reduced propagaging time of 1.2M routes to ~100 peers in route reflector setup from 7 minutes to 2 minutes. --- api/gobgp.pb.go | 13 +- cmd/gobgp/common.go | 2 + cmd/gobgp/neighbor.go | 10 +- cmd/gobgp/peer_group.go | 128 +++++ cmd/gobgp/root.go | 3 +- docs/sources/policy.md | 21 + internal/pkg/table/destination.go | 79 +++- internal/pkg/table/path.go | 40 +- pkg/config/oc/bgp_configs.go | 9 + pkg/config/oc/default.go | 92 +++- pkg/config/oc/util.go | 16 +- pkg/server/fsm.go | 29 +- pkg/server/grpc_server.go | 2 + pkg/server/peer.go | 618 ++++++++++++++++++++----- pkg/server/peer_test.go | 744 ++++++++++++++++++++++++++++++ pkg/server/server.go | 470 +++++++++++-------- pkg/server/server_test.go | 315 ++++++++++++- proto/api/gobgp.proto | 1 + test/lib/base.py | 5 +- tools/pyang_plugins/gobgp.yang | 16 + 20 files changed, 2239 insertions(+), 374 deletions(-) create mode 100644 cmd/gobgp/peer_group.go create mode 100644 pkg/server/peer_test.go diff --git a/api/gobgp.pb.go b/api/gobgp.pb.go index 918fa28fd..32f255df1 100644 --- a/api/gobgp.pb.go +++ b/api/gobgp.pb.go @@ -7347,6 +7347,7 @@ type PeerGroupConf struct { RouteFlapDamping bool `protobuf:"varint,8,opt,name=route_flap_damping,json=routeFlapDamping,proto3" json:"route_flap_damping,omitempty"` SendCommunity uint32 `protobuf:"varint,9,opt,name=send_community,json=sendCommunity,proto3" json:"send_community,omitempty"` SendSoftwareVersion bool `protobuf:"varint,10,opt,name=send_software_version,json=sendSoftwareVersion,proto3" json:"send_software_version,omitempty"` + SharedPolicy bool `protobuf:"varint,11,opt,name=shared_policy,json=sharedPolicy,proto3" json:"shared_policy,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -7451,6 +7452,13 @@ func (x *PeerGroupConf) GetSendSoftwareVersion() bool { return false } +func (x *PeerGroupConf) GetSharedPolicy() bool { + if x != nil { + return x.SharedPolicy + } + return false +} + type PeerGroupState struct { state protoimpl.MessageState `protogen:"open.v1"` AuthPassword string `protobuf:"bytes,1,opt,name=auth_password,json=authPassword,proto3" json:"auth_password,omitempty"` @@ -13023,7 +13031,7 @@ const file_api_gobgp_proto_rawDesc = "" + "\n" + "admin_down\x18\x0f \x01(\bR\tadminDown\x122\n" + "\x15send_software_version\x18\x10 \x01(\bR\x13sendSoftwareVersion\x125\n" + - "\x17allow_aspath_loop_local\x18\x11 \x01(\bR\x14allowAspathLoopLocal\"\x9d\x03\n" + + "\x17allow_aspath_loop_local\x18\x11 \x01(\bR\x14allowAspathLoopLocal\"\xc2\x03\n" + "\rPeerGroupConf\x12#\n" + "\rauth_password\x18\x01 \x01(\tR\fauthPassword\x12 \n" + "\vdescription\x18\x02 \x01(\tR\vdescription\x12\x1b\n" + @@ -13035,7 +13043,8 @@ const file_api_gobgp_proto_rawDesc = "" + "\x12route_flap_damping\x18\b \x01(\bR\x10routeFlapDamping\x12%\n" + "\x0esend_community\x18\t \x01(\rR\rsendCommunity\x122\n" + "\x15send_software_version\x18\n" + - " \x01(\bR\x13sendSoftwareVersion\"\xb2\x03\n" + + " \x01(\bR\x13sendSoftwareVersion\x12#\n" + + "\rshared_policy\x18\v \x01(\bR\fsharedPolicy\"\xb2\x03\n" + "\x0ePeerGroupState\x12#\n" + "\rauth_password\x18\x01 \x01(\tR\fauthPassword\x12 \n" + "\vdescription\x18\x02 \x01(\tR\vdescription\x12\x1b\n" + diff --git a/cmd/gobgp/common.go b/cmd/gobgp/common.go index 1bedd0835..f589d2ebf 100644 --- a/cmd/gobgp/common.go +++ b/cmd/gobgp/common.go @@ -45,6 +45,8 @@ const globalRIBName = "global" const ( cmdGlobal = "global" cmdNeighbor = "neighbor" + cmdPeerGroup = "peer-group" + cmdPeerGroupShort = "pg" cmdPolicy = "policy" cmdRib = "rib" cmdAdd = "add" diff --git a/cmd/gobgp/neighbor.go b/cmd/gobgp/neighbor.go index 51e877a00..ac2209066 100644 --- a/cmd/gobgp/neighbor.go +++ b/cmd/gobgp/neighbor.go @@ -623,7 +623,7 @@ func makeShowRouteArgs(p *api.Path, idx int, now time.Time, showAge, showBest, s args = append(args, teid, qfi, endpoint) } - attrs, _ := apiutil.GetNativePathAttributes(p) + attrs, attrErr := apiutil.GetNativePathAttributes(p) // Next Hop nexthop := "fictitious" if n := getNextHopFromPathAttributes(attrs); n.IsValid() { @@ -649,8 +649,12 @@ func makeShowRouteArgs(p *api.Path, idx int, now time.Time, showAge, showBest, s } // Path Attributes - pattrstr := getPathAttributeString(nlri, attrs) - args = append(args, pattrstr) + if attrErr == nil { + pattrstr := getPathAttributeString(nlri, attrs) + args = append(args, pattrstr) + } else { + args = append(args, fmt.Sprintf("error: %s", attrErr)) + } if showSendMaxFiltered { if p.SendMaxFiltered { diff --git a/cmd/gobgp/peer_group.go b/cmd/gobgp/peer_group.go new file mode 100644 index 000000000..fdabacc71 --- /dev/null +++ b/cmd/gobgp/peer_group.go @@ -0,0 +1,128 @@ +package main + +import ( + "encoding/json" + "fmt" + "io" + "sort" + "strconv" + "strings" + + api "github.com/osrg/gobgp/v4/api" + "github.com/osrg/gobgp/v4/pkg/config/oc" + "github.com/spf13/cobra" +) + +func newPeerGroupCmd() *cobra.Command { + pgCmdImpl := &cobra.Command{} + + policyCmd := &cobra.Command{ + Use: cmdPolicy, + Run: func(cmd *cobra.Command, args []string) { + key := oc.NewPeerGroupPolicyAssignmentKeyFromName(args[0]) + for _, v := range []string{cmdImport, cmdExport} { + if err := showNeighborPolicy(key, v, 4); err != nil { + exitWithError(err) + } + } + }, + } + pgCmdImpl.AddCommand(policyCmd) + + pgCmd := &cobra.Command{ + Use: cmdPeerGroup, + Aliases: []string{cmdPeerGroupShort}, + + RunE: func(cmd *cobra.Command, args []string) error { + var err error + if len(args) < 2 { + var pgName string + if len(args) == 1 { + pgName = args[0] + } + showPeerGroups(pgName) + } else { + args = append(args[1:], args[0]) + pgCmdImpl.SetArgs(args) + err = pgCmdImpl.Execute() + } + if err != nil { + exitWithError(err) + } + return nil + }, + } + + return pgCmd +} + +func showPeerGroups(name string) { + pgs, err := getPeerGroups(name) + if err != nil { + exitWithError(err) + } + + if globalOpts.Json { + j, _ := json.Marshal(pgs) + fmt.Println(string(j)) + return + } + + sort.Slice(pgs, func(i, j int) bool { + return pgs[i].Conf.PeerGroupName < pgs[j].Conf.PeerGroupName + }) + + nameColLen := 10 + for _, pg := range pgs { + if len(pg.Conf.PeerGroupName) > nameColLen { + nameColLen = len(pg.Conf.PeerGroupName) + } + } + + fmtstr := "%-" + strconv.Itoa(nameColLen) + "s %8s %5s %s\n" + fmt.Printf(fmtstr, "PeerGroup", "Type", "AS", "Info") + for _, pg := range pgs { + info := make([]string, 0, 2) + if pg.Info.LocalAsn != pg.Info.PeerAsn { + info = append(info, fmt.Sprintf("local-as %d", pg.Info.LocalAsn)) + } + + if pg.RouteReflector != nil && pg.RouteReflector.RouteReflectorClient { + info = append(info, fmt.Sprintf("route-reflector-client %s", pg.RouteReflector.RouteReflectorClusterId)) + } else if pg.RouteServer != nil && pg.RouteServer.RouteServerClient { + info = append(info, "route-server-client") + } + + fmt.Printf( + fmtstr, + pg.Conf.PeerGroupName, + api.PeerType_name[int32(pg.Info.Type)], + fmt.Sprint(pg.Info.PeerAsn), + strings.Join(info, ", "), + ) + } +} + +func getPeerGroups(name string) ([]*api.PeerGroup, error) { + stream, err := client.ListPeerGroup(ctx, &api.ListPeerGroupRequest{ + PeerGroupName: name, + }) + if err != nil { + return nil, err + } + + l := make([]*api.PeerGroup, 0, 1024) + for { + r, err := stream.Recv() + if err == io.EOF { + break + } else if err != nil { + return nil, err + } + l = append(l, r.PeerGroup) + } + if name != "" && len(l) == 0 { + return l, fmt.Errorf("not found peer group %s", name) + } + return l, err +} diff --git a/cmd/gobgp/root.go b/cmd/gobgp/root.go index 97fe404a0..292cd4c68 100644 --- a/cmd/gobgp/root.go +++ b/cmd/gobgp/root.go @@ -104,6 +104,7 @@ func newRootCmd() *cobra.Command { globalCmd := newGlobalCmd() neighborCmd := newNeighborCmd() + pgCmd := newPeerGroupCmd() vrfCmd := newVrfCmd() policyCmd := newPolicyCmd() monitorCmd := newMonitorCmd() @@ -111,6 +112,6 @@ func newRootCmd() *cobra.Command { rpkiCmd := newRPKICmd() bmpCmd := newBmpCmd() logLevelCmd := newLogLevelCmd() - rootCmd.AddCommand(globalCmd, neighborCmd, vrfCmd, policyCmd, monitorCmd, mrtCmd, rpkiCmd, bmpCmd, logLevelCmd) + rootCmd.AddCommand(globalCmd, neighborCmd, pgCmd, vrfCmd, policyCmd, monitorCmd, mrtCmd, rpkiCmd, bmpCmd, logLevelCmd) return rootCmd } diff --git a/docs/sources/policy.md b/docs/sources/policy.md index 74ebc2395..7684646d5 100644 --- a/docs/sources/policy.md +++ b/docs/sources/policy.md @@ -40,6 +40,7 @@ Assumed that you finished [Getting Started](getting-started.md). - [4.2. Attach policy to route-server-client](#42-attach-policy-to-route-server-client) - [Policy Configuration Example](#policy-configuration-example) - [Policy and Soft Reset](#policy-and-soft-reset) + - [Shared Policy in Peer Groups](#shared-policy-in-peer-groups) ## Overview @@ -996,3 +997,23 @@ When you change an import policy and reset the inbound routing table (aka soft r The outbound routing table doesn't exist for saving memory usage, it's impossible to know whether the route was actually sent to peer or the route also was rejected by the previous export policies and not sent. GoBGP doesn't send such withdraw rather than possible unwilling leaking information. Please report if other implementations such as bird work in a different way. + +## Shared Policy in Peer Groups + +By default each policy is computed individually per each receiver peer. There still an option to select policy applicable statements by using matching based on neighbor sets, but it might be ineffective for large amount of peers route reflector or server might have as each neighbor set requires source IP matching per each path received or propagated. GoBGP offers an option to group policy computation per each group which is enabled by `shared-policy` option in peer-group config and specifying individual `apply-policy` config section for it. + +With `shared-policy` option enabled specified policy is computed once per peer-group with a additional small work done on per-peer basis, such as Route Target Constraint checks or Split Horizon check. Note that this option is incompatible with peers with ADD_PATH capability. + +If shared policy is enabled, applicable policy for a peer group can be checked using `gobgp peer-group PG_NAME policy` command: + +```bash +$ gobgp peer-group control policy +Import policy: + Default: ACCEPT + Name control-import: + ... +``` + +NOTE: shared policy changes behavior of RTC: since it requires for RTC filtering to be computed on per-peer basis, +while policy is computed on per-peer-group basis, RTC starts to account for route targets appended or removed via export +policies. diff --git a/internal/pkg/table/destination.go b/internal/pkg/table/destination.go index 5705bb27c..7e984f7d4 100644 --- a/internal/pkg/table/destination.go +++ b/internal/pkg/table/destination.go @@ -78,17 +78,44 @@ func (r *BestPathReason) String() string { return BestPathReasonStringMap[*r] } +// PeerInfo contains a chunk of peer configuration that is used by table code to determine +// how to handle path attributes. PeerInfo struct can be used both for individual peers +// and peer groups when peer group is in shared policy mode and handles paths in bulk. +// +// It is also used to identify source of path to determine best path, but in that case it +// can only be a peer, not a peer group. +// +// Zero PeerInfo value denotes localSource (paths originated locally) - use localSource type PeerInfo struct { - AS uint32 - LocalAS uint32 - ID netip.Addr - LocalID netip.Addr - Address netip.Addr - LocalAddress netip.Addr - RouteReflectorClusterID netip.Addr + // PeerType: INTERNAL for iBGP peers or EXTERNAL for eBGP peers. Computed from + // a state based on AS/LocalAS Comparison + PeerType oc.PeerType + + // AS Number, Address and BGP Identifier (not specified for peer groups) of the remote peer + AS uint32 + ID netip.Addr + Address netip.Addr + + // Local AS Number, Address and BGP Identifier of local router. + // + // AS Number can be overridden using local-as config option on per-peer basis and + // used for AS_PATH prepending for eBGP peers and for path filtering + // FIXME: for now, local-as override is not fully supported for filtering, see handleUpdate() in peer.go + LocalAS uint32 + LocalID netip.Addr + LocalAddress netip.Addr + + // A view of peer/peer group configuration used to compute path attributes correctly RouteReflectorClient bool + RouteReflectorClusterID netip.Addr + RouteServerClient bool MultihopTtl uint8 Confederation bool + RemovePrivateAs oc.RemovePrivateAsOption + + // PeerGroup contains name for peer group itself if this is PeerInfo for a peer group + // or the name of peer group this peer belongs to. Used for informational purposes. + PeerGroup string } func (lhs *PeerInfo) Equal(rhs *PeerInfo) bool { @@ -107,9 +134,11 @@ func (lhs *PeerInfo) Equal(rhs *PeerInfo) bool { } func (i *PeerInfo) String() string { - if !i.Address.IsValid() { + const peerTypeUnspecified = oc.PeerType("") + if i.PeerType == peerTypeUnspecified { return "local" } + s := bytes.NewBuffer(make([]byte, 0, 64)) fmt.Fprintf(s, "{ %s | ", i.Address) fmt.Fprintf(s, "as: %d", i.AS) @@ -123,16 +152,42 @@ func (i *PeerInfo) String() string { func NewPeerInfo(g *oc.Global, p *oc.Neighbor, AS, localAS uint32, ID, localID netip.Addr, addr, localAddr netip.Addr) *PeerInfo { return &PeerInfo{ - AS: AS, - LocalAS: localAS, + PeerType: p.State.PeerType, ID: ID, - LocalID: localID, + AS: p.Config.PeerAs, Address: addr, + LocalAS: p.Config.LocalAs, + LocalID: localID, LocalAddress: localAddr, + RouteReflectorClient: p.RouteReflector.Config.RouteReflectorClient, RouteReflectorClusterID: p.RouteReflector.State.RouteReflectorClusterId, + RouteServerClient: p.RouteServer.Config.RouteServerClient, + MultihopTtl: p.EbgpMultihop.Config.MultihopTtl, + Confederation: g.IsConfederationMember(p.Config.PeerAs), + RemovePrivateAs: p.State.RemovePrivateAs, + PeerGroup: p.Config.PeerGroup, + } +} + +func NewPeerGroupInfo(g *oc.Global, p *oc.PeerGroup) *PeerInfo { + localAddr := p.Transport.Config.LocalAddress + if !localAddr.IsValid() { + localAddr = g.Config.RouterId + } + + return &PeerInfo{ + PeerType: p.State.PeerType, + AS: p.Config.PeerAs, + LocalAS: p.Config.LocalAs, + LocalID: g.Config.RouterId, + LocalAddress: localAddr, RouteReflectorClient: p.RouteReflector.Config.RouteReflectorClient, + RouteReflectorClusterID: p.RouteReflector.State.RouteReflectorClusterId, + RouteServerClient: p.RouteServer.Config.RouteServerClient, MultihopTtl: p.EbgpMultihop.Config.MultihopTtl, - Confederation: p.IsConfederationMember(g), + Confederation: g.IsConfederationMember(p.Config.PeerAs), + RemovePrivateAs: p.State.RemovePrivateAs, + PeerGroup: p.Config.PeerGroupName, } } diff --git a/internal/pkg/table/path.go b/internal/pkg/table/path.go index 62128d5dd..2691c739f 100644 --- a/internal/pkg/table/path.go +++ b/internal/pkg/table/path.go @@ -25,6 +25,7 @@ import ( "net/netip" "slices" "sort" + "sync/atomic" "time" "github.com/osrg/gobgp/v4/pkg/config/oc" @@ -223,8 +224,8 @@ func cloneAsPath(asAttr *bgp.PathAttributeAsPath) *bgp.PathAttributeAsPath { return bgp.NewPathAttributeAsPath(newASparams) } -func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, info *PeerInfo, original *Path) *Path { - if peer.RouteServer.Config.RouteServerClient { +func UpdatePathAttrs(logger log.Logger, global *oc.Global, info *PeerInfo, original *Path) *Path { + if info.RouteServerClient { return original } path := original.Clone(original.IsWithdraw) @@ -237,7 +238,7 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, in } else { switch a.GetType() { case bgp.BGP_ATTR_TYPE_CLUSTER_LIST, bgp.BGP_ATTR_TYPE_ORIGINATOR_ID: - if peer.State.PeerType != oc.PEER_TYPE_INTERNAL || !peer.RouteReflector.Config.RouteReflectorClient { + if info.PeerType != oc.PEER_TYPE_INTERNAL || !info.RouteReflectorClient { // send these attributes to only rr clients path.delPathAttr(a.GetType()) } @@ -247,7 +248,7 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, in localAddress := info.LocalAddress nexthop := path.GetNexthop() - switch peer.State.PeerType { + switch info.PeerType { case oc.PEER_TYPE_EXTERNAL: // NEXTHOP handling if !path.IsLocal() || nexthop.IsUnspecified() { @@ -255,11 +256,11 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, in } // remove-private-as handling - path.RemovePrivateAS(peer.Config.LocalAs, peer.State.RemovePrivateAs) + path.RemovePrivateAS(info.LocalAS, info.RemovePrivateAs) // AS_PATH handling - confed := peer.IsConfederationMember(global) - path.PrependAsn(peer.Config.LocalAs, 1, confed) + confed := global.IsConfederationMember(info.AS) + path.PrependAsn(info.LocalAS, 1, confed) if !confed { path.removeConfedAs() } @@ -293,8 +294,8 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, in // RFC4456: BGP Route Reflection // 8. Avoiding Routing Information Loops - info := path.GetSource() - if peer.RouteReflector.Config.RouteReflectorClient { + srcInfo := path.GetSource() + if info.RouteReflectorClient { // This attribute will carry the BGP Identifier of the originator of the route in the local AS. // A BGP speaker SHOULD NOT create an ORIGINATOR_ID attribute if one already exists. // @@ -305,21 +306,21 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, in // address for that session. if path.GetFamily() == bgp.RF_RTC_UC { path.SetNexthop(localAddress.AsSlice()) - attr, _ := bgp.NewPathAttributeOriginatorId(info.LocalID) + attr, _ := bgp.NewPathAttributeOriginatorId(srcInfo.LocalID) path.setPathAttr(attr) } else if path.getPathAttr(bgp.BGP_ATTR_TYPE_ORIGINATOR_ID) == nil { if path.IsLocal() { attr, _ := bgp.NewPathAttributeOriginatorId(global.Config.RouterId) path.setPathAttr(attr) } else { - attr, _ := bgp.NewPathAttributeOriginatorId(info.ID) + attr, _ := bgp.NewPathAttributeOriginatorId(srcInfo.ID) path.setPathAttr(attr) } } // When an RR reflects a route, it MUST prepend the local CLUSTER_ID to the CLUSTER_LIST. // If the CLUSTER_LIST is empty, it MUST create a new one. // TODO: needs to validated earlier. - clusterID := peer.RouteReflector.State.RouteReflectorClusterId + clusterID := info.RouteReflectorClusterID if p := path.getPathAttr(bgp.BGP_ATTR_TYPE_CLUSTER_LIST); p == nil { pa, _ := bgp.NewPathAttributeClusterList([]netip.Addr{clusterID}) path.setPathAttr(pa) @@ -332,10 +333,10 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, peer *oc.Neighbor, in default: logger.Warn("invalid peer type", log.Fields{ - "Topic": "Peer", - "Key": peer.State.NeighborAddress, - "Type": peer.State.PeerType, - }) + "Topic": "Peer", + "PeerGroup": info.PeerGroup, + "ID": info.ID, + "Type": info.PeerType}) } return path } @@ -1323,11 +1324,14 @@ func (p *Path) ToLocal() *Path { } func (p *Path) updateHash() { - p.attrsHash = fnv1a.Init64 + attrsHash := fnv1a.Init64 for _, a := range p.GetPathAttrs() { d, _ := a.Serialize() - p.attrsHash = fnv1a.AddBytes64(p.attrsHash, d) + attrsHash = fnv1a.AddBytes64(p.attrsHash, d) } + + // + atomic.StoreUint64(&p.attrsHash, attrsHash) } func (p *Path) SetHash(v uint64) { diff --git a/pkg/config/oc/bgp_configs.go b/pkg/config/oc/bgp_configs.go index 696205b17..478b676d4 100644 --- a/pkg/config/oc/bgp_configs.go +++ b/pkg/config/oc/bgp_configs.go @@ -1768,6 +1768,9 @@ type PeerGroupState struct { // original -> bgp-op:total-prefixes // . TotalPrefixes uint32 `mapstructure:"total-prefixes" json:"total-prefixes,omitempty"` + // original -> gobgp:shared-policy + // gobgp:shared-policy's original type is boolean. + SharedPolicy bool `mapstructure:"shared-policy" json:"shared-policy,omitempty"` } // struct for container bgp:config. @@ -1815,6 +1818,9 @@ type PeerGroupConfig struct { // original -> gobgp:send-software-version // gobgp:send-software-version's original type is boolean. SendSoftwareVersion bool `mapstructure:"send-software-version" json:"send-software-version,omitempty"` + // original -> gobgp:shared-policy + // gobgp:shared-policy's original type is boolean. + SharedPolicy bool `mapstructure:"shared-policy" json:"shared-policy,omitempty"` } func (lhs *PeerGroupConfig) Equal(rhs *PeerGroupConfig) bool { @@ -1851,6 +1857,9 @@ func (lhs *PeerGroupConfig) Equal(rhs *PeerGroupConfig) bool { if lhs.SendSoftwareVersion != rhs.SendSoftwareVersion { return false } + if lhs.SharedPolicy != rhs.SharedPolicy { + return false + } return true } diff --git a/pkg/config/oc/default.go b/pkg/config/oc/default.go index a07cd9b73..486e431c9 100644 --- a/pkg/config/oc/default.go +++ b/pkg/config/oc/default.go @@ -49,6 +49,20 @@ func defaultAfiSafi(typ AfiSafiType, enable bool) AfiSafi { } } +// SetDefaultNeighborConfigValues specifies configuration options in Neighbor struct +// filling defaults from it's peer group (if it has one) or global config. Some packagess +// might have their own representation of config such as PeerInfo in table package. +// +// It also fills state fields to corresponding computed values. +// +// Overall flow of config options looks like this: +// +// g ----> pg ----> PeerInfo +// \ | ^ +// \ v / +// \---> n ----/ +// +// (not counting viper here) func SetDefaultNeighborConfigValues(n *Neighbor, pg *PeerGroup, g *Global) error { // Determines this function is called against the same Neighbor struct, // and if already called, returns immediately. @@ -78,23 +92,16 @@ func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Glo } if n.Config.LocalAs == 0 { - n.Config.LocalAs = g.Config.As - if !g.Confederation.Config.Enabled || n.IsConfederation(g) { - n.Config.LocalAs = g.Config.As - } else { - n.Config.LocalAs = g.Confederation.Config.Identifier - } + n.Config.LocalAs = getLocalAsForPeer(g, n.Config.PeerAs) } n.State.LocalAs = n.Config.LocalAs - if n.Config.PeerAs != n.Config.LocalAs { - n.Config.PeerType = PEER_TYPE_EXTERNAL - n.State.PeerType = PEER_TYPE_EXTERNAL + n.Config.PeerType = getConfigPeerType(n.Config.PeerAs, n.Config.LocalAs) + 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 { - n.Config.PeerType = PEER_TYPE_INTERNAL - n.State.PeerType = PEER_TYPE_INTERNAL if string(n.Config.RemovePrivateAs) != "" { return fmt.Errorf("can't set remove-private-as for iBGP peer") } @@ -102,6 +109,8 @@ func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Glo return fmt.Errorf("can't set replace-peer-as for iBGP peer") } } + n.State.PeerAs = n.Config.PeerAs + n.AsPathOptions.State.AllowOwnAs = n.AsPathOptions.Config.AllowOwnAs if !n.State.NeighborAddress.IsValid() { n.State.NeighborAddress = n.Config.NeighborAddress @@ -247,17 +256,66 @@ func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Glo } if n.RouteReflector.Config.RouteReflectorClient { - if !n.RouteReflector.Config.RouteReflectorClusterId.IsValid() { - n.RouteReflector.State.RouteReflectorClusterId = g.Config.RouterId - } else { - if !n.RouteReflector.Config.RouteReflectorClusterId.IsValid() || !n.RouteReflector.Config.RouteReflectorClusterId.Is4() { - return fmt.Errorf("route-reflector-cluster-id should be specified as IPv4 address") - } + clusterId, err := getConfigClusterId(g, n.RouteReflector.Config.RouteReflectorClusterId) + if err != nil { + return err } + n.RouteReflector.State.RouteReflectorClusterId = clusterId } return nil } +// SetPeerGroupStateValues fills some of the values in State fields +// NOTE: for now we set bare minimum for table.NewPeerGroupInfo +func SetPeerGroupStateValues(pg *PeerGroup, g *Global) error { + if pg.Config.LocalAs == 0 { + pg.Config.LocalAs = getLocalAsForPeer(g, pg.Config.PeerAs) + } + pg.State.LocalAs = pg.Config.LocalAs + pg.State.PeerAs = pg.Config.PeerAs + + // Not checking validity of various as handling options here as setDefaultNeighborConfigValuesWithViper + // will do that anyway + pg.Config.PeerType = getConfigPeerType(pg.Config.PeerAs, pg.Config.LocalAs) + pg.State.PeerType = pg.Config.PeerType + + if pg.RouteReflector.Config.RouteReflectorClient { + clusterId, err := getConfigClusterId(g, pg.RouteReflector.Config.RouteReflectorClusterId) + if err != nil { + return err + } + + pg.RouteReflector.State.RouteReflectorClusterId = clusterId + } + + return nil +} + +func getLocalAsForPeer(g *Global, peerAs uint32) uint32 { + if g.Confederation.Config.Enabled && !g.IsConfederation(peerAs) { + return g.Confederation.Config.Identifier + } + return g.Config.As +} + +func getConfigPeerType(peerAs, localAs uint32) PeerType { + if peerAs != localAs { + return PEER_TYPE_EXTERNAL + } + return PEER_TYPE_INTERNAL +} + +func getConfigClusterId(g *Global, configClusterId netip.Addr) (netip.Addr, error) { + if !configClusterId.IsValid() { + return g.Config.RouterId, nil + } + + if !configClusterId.Is4() { + return netip.Addr{}, fmt.Errorf("route-reflector-cluster-id should be specified as IPv4 address") + } + return configClusterId, nil +} + func SetDefaultGlobalConfigValues(g *Global) error { if len(g.AfiSafis) == 0 { g.AfiSafis = []AfiSafi{} diff --git a/pkg/config/oc/util.go b/pkg/config/oc/util.go index 5307d6d51..c3769a63f 100644 --- a/pkg/config/oc/util.go +++ b/pkg/config/oc/util.go @@ -113,15 +113,15 @@ func (d *DynamicNeighbor) validate(b *BgpConfigSet) error { return nil } -func (n *Neighbor) IsConfederationMember(g *Global) bool { - return slices.Contains(g.Confederation.Config.MemberAsList, n.Config.PeerAs) +func (g *Global) IsConfederationMember(peerAS uint32) bool { + return slices.Contains(g.Confederation.Config.MemberAsList, peerAS) } -func (n *Neighbor) IsConfederation(g *Global) bool { - if n.Config.PeerAs == g.Config.As { +func (g *Global) IsConfederation(peerAS uint32) bool { + if peerAS == g.Config.As { return true } - return n.IsConfederationMember(g) + return g.IsConfederationMember(peerAS) } func (n *Neighbor) IsEBGPPeer(g *Global) bool { @@ -632,8 +632,10 @@ func NewPeerGroupFromConfigStruct(pconf *PeerGroup) *api.PeerGroup { Description: pconf.Config.Description, PeerGroupName: pconf.Config.PeerGroupName, SendSoftwareVersion: pconf.Config.SendSoftwareVersion, + SharedPolicy: pconf.Config.SharedPolicy, }, Info: &api.PeerGroupState{ + LocalAsn: s.LocalAs, PeerAsn: s.PeerAs, Type: toPeerType(s.PeerType), TotalPaths: s.TotalPaths, @@ -812,3 +814,7 @@ func NewAPIDefinedSetsFromConfigStruct(t *DefinedSets) ([]*api.DefinedSet, error return definedSets, nil } + +func NewPeerGroupPolicyAssignmentKeyFromName(name string) string { + return "pg:" + name +} diff --git a/pkg/server/fsm.go b/pkg/server/fsm.go index 82d59f1a2..3dfdedd4e 100644 --- a/pkg/server/fsm.go +++ b/pkg/server/fsm.go @@ -40,7 +40,11 @@ import ( ) const ( - minConnectRetryInterval = 2 + minConnectRetryInterval = 5 +) + +const ( + logOnceAllowOwnAs = 1 << iota ) type fsmStateReasonType uint8 @@ -193,6 +197,7 @@ type fsm struct { notification chan *bgp.BGPMessage deconfiguredNotification chan *bgp.BGPMessage logger log.Logger + logOnceFlags uint32 } func (fsm *fsm) bgpMessageStateUpdate(MessageType uint8, isIn bool) { @@ -1075,7 +1080,7 @@ func (h *fsmHandler) recvMessageWithError() (*fsmMsg, error) { body := m.Body.(*bgp.BGPUpdate) h.fsm.lock.RLock() isEBGP := h.fsm.pConf.IsEBGPPeer(h.fsm.gConf) - isConfed := h.fsm.pConf.IsConfederationMember(h.fsm.gConf) + isConfed := h.fsm.gConf.IsConfederationMember(h.fsm.pConf.Config.PeerAs) h.fsm.lock.RUnlock() fmsg.payload = make([]byte, len(headerBuf)+len(bodyBuf)) @@ -1803,6 +1808,16 @@ func (h *fsmHandler) sendMessageloop(ctx context.Context, wg *sync.WaitGroup) er } } default: + // This is kinda expected that established() tries to stop us + fsm.lock.RLock() + fsm.logger.Warn("Got a message without paths, stopping send message loop", + log.Fields{ + "Topic": "Peer", + "Key": fsm.pConf.State.NeighborAddress, + "State": fsm.state.String(), + "data": m, + }) + fsm.lock.RUnlock() return nil } case <-ticker.C: @@ -2126,3 +2141,13 @@ func (h *fsmHandler) changeadminState(s adminState) error { } return nil } + +func (fsm *fsm) warnOnce(flag uint32, msg string, fields log.Fields) { + if fsm.logOnceFlags&flag == 0 { + fsm.logger.Warn(msg+" Further messages will be logged with DEBUG level.", fields) + fsm.logOnceFlags |= flag + return + } + + fsm.logger.Debug(msg, fields) +} diff --git a/pkg/server/grpc_server.go b/pkg/server/grpc_server.go index 37ffb7f82..4df07ed5c 100644 --- a/pkg/server/grpc_server.go +++ b/pkg/server/grpc_server.go @@ -1155,6 +1155,8 @@ func newPeerGroupFromAPIStruct(a *api.PeerGroup) (*oc.PeerGroup, error) { readAddPathsFromAPIStruct(&afiSafi.AddPaths, af.AddPaths) pconf.AfiSafis = append(pconf.AfiSafis, afiSafi) } + + pconf.Config.SharedPolicy = a.Conf.SharedPolicy } if a.Timers != nil { diff --git a/pkg/server/peer.go b/pkg/server/peer.go index 9c44ea15c..191a36998 100644 --- a/pkg/server/peer.go +++ b/pkg/server/peer.go @@ -33,26 +33,211 @@ const ( flopThreshold = time.Second * 30 ) +type peerPathLimiterImpl struct { + afiSafis []oc.AfiSafi + + // map of path local identifiers sent for that prefix + sentPaths map[table.PathDestLocalKey]map[uint32]struct{} + sendMaxPathFiltered map[table.PathLocalKey]struct{} +} + +type peerPathLimiter interface { + getAddPathSendMax(family bgp.Family) uint8 + getRoutesCount(family bgp.Family, dstPrefix string) uint8 + updateRoutes(paths ...*table.Path) + isPathSendMaxFiltered(path *table.Path) bool + setPathSendMaxFiltered(path *table.Path) + unsetPathSendMaxFiltered(path *table.Path) bool + hasPathAlreadyBeenSent(path *table.Path) bool +} + +func (peer *peerPathLimiterImpl) init(afiSafis []oc.AfiSafi) { + peer.afiSafis = afiSafis + + peer.sentPaths = make(map[table.PathDestLocalKey]map[uint32]struct{}) + peer.sendMaxPathFiltered = make(map[table.PathLocalKey]struct{}) +} + +func (peer *peerPathLimiterImpl) getAddPathSendMax(family bgp.Family) uint8 { + for _, a := range peer.afiSafis { + if a.State.Family == family { + return a.AddPaths.Config.SendMax + } + } + return 0 +} + +func (peer *peerPathLimiterImpl) getRoutesCount(family bgp.Family, dstPrefix string) uint8 { + destLocalKey := table.NewPathDestLocalKey(family, dstPrefix) + if identifiers, ok := peer.sentPaths[*destLocalKey]; ok { + count := len(identifiers) + // the send-max config is uint8, so we need to check for overflow + if count > int(^uint8(0)) { + return ^uint8(0) + } + return uint8(count) + } + return 0 +} + +func (peer *peerPathLimiterImpl) updateRoutes(paths ...*table.Path) { + if len(paths) == 0 { + return + } + for _, path := range paths { + localKey := path.GetLocalKey() + destLocalKey := localKey.PathDestLocalKey + identifiers, destExists := peer.sentPaths[destLocalKey] + if path.IsWithdraw && destExists { + delete(identifiers, path.LocalID()) + } else if !path.IsWithdraw { + if !destExists { + peer.sentPaths[destLocalKey] = make(map[uint32]struct{}) + } + identifiers := peer.sentPaths[destLocalKey] + if len(identifiers) < int(peer.getAddPathSendMax(destLocalKey.Family)) { + identifiers[localKey.Id] = struct{}{} + } + } + } +} + +func (peer *peerPathLimiterImpl) isPathSendMaxFiltered(path *table.Path) bool { + if path == nil { + return false + } + _, found := peer.sendMaxPathFiltered[path.GetLocalKey()] + return found +} + +func (peer *peerPathLimiterImpl) setPathSendMaxFiltered(path *table.Path) { + if path == nil { + return + } + peer.sendMaxPathFiltered[path.GetLocalKey()] = struct{}{} +} + +func (peer *peerPathLimiterImpl) unsetPathSendMaxFiltered(path *table.Path) bool { + if path == nil { + return false + } + if _, ok := peer.sendMaxPathFiltered[path.GetLocalKey()]; !ok { + return false + } + delete(peer.sendMaxPathFiltered, path.GetLocalKey()) + return true +} + +func (peer *peerPathLimiterImpl) hasPathAlreadyBeenSent(path *table.Path) bool { + if path == nil { + return false + } + destLocalKey := path.GetDestLocalKey() + if _, dstExist := peer.sentPaths[destLocalKey]; !dstExist { + return false + } + _, pathExist := peer.sentPaths[destLocalKey][path.LocalID()] + return pathExist +} + +type receiver interface { + peerPathLimiter + + ID() string + PeerInfo() *table.PeerInfo + + TableID() string + AS() uint32 + + vrf() string + routingPolicy() *table.RoutingPolicy + PolicyID() string + localRib() *table.TableManager + configuredRFlist() []bgp.Family + + isIBGPPeer() bool + isEnabledFamily(rf bgp.Family) bool + isLLGREnabledFamily(rf bgp.Family) bool + replacePeerAS() (bool, uint32, uint32) + isRouteReflectorClient() bool + isAddPathSendEnabled(family bgp.Family) bool + isRouteServerClient() bool + isSecondaryRouteEnabled() bool + allowAsPathLoopLocal() bool + + isGracefulRestartEnabled() bool + resetLocalRestarting() bool + + needToAdvertise() bool + send(paths, old []*table.Path) + + logger() log.Logger +} + type peerGroup struct { - Conf *oc.PeerGroup + peerPathLimiterImpl + + l log.Logger + + Conf *oc.PeerGroup + info *table.PeerInfo + + rfList, llgrRFList []bgp.Family + loc *table.TableManager + policy *table.RoutingPolicy + members map[string]oc.Neighbor dynamicNeighbors map[string]*oc.DynamicNeighbor + neighborMap map[string]*peer } -func newPeerGroup(c *oc.PeerGroup) *peerGroup { - return &peerGroup{ - Conf: c, +func newPeerGroup(l log.Logger, policy *table.RoutingPolicy, g *oc.Global, c *oc.PeerGroup, loc *table.TableManager) (*peerGroup, error) { + pg := &peerGroup{ + l: l, + + loc: loc, + policy: policy, + members: make(map[string]oc.Neighbor), dynamicNeighbors: make(map[string]*oc.DynamicNeighbor), + neighborMap: make(map[string]*peer), } + if err := pg.Update(g, c); err != nil { + return nil, err + } + + // Initializes peerPathLimiterImpl. Using pg.peerPathLimiterImpl.init would be better, but + // golangci-lint insists overwise, so here we are + pg.init(c.AfiSafis) + return pg, nil } -func (pg *peerGroup) AddMember(c oc.Neighbor) { +func (pg *peerGroup) Update(g *oc.Global, c *oc.PeerGroup) error { + if err := oc.SetPeerGroupStateValues(c, g); err != nil { + return err + } + + pg.Conf = c + pg.info = table.NewPeerGroupInfo(g, c) + pg.rfList, _ = oc.AfiSafis(pg.Conf.AfiSafis).ToRfList() + + if c.GracefulRestart.Config.LongLivedEnabled { + pg.llgrRFList = llgrFamilies(c.AfiSafis, false) + } else { + pg.llgrRFList = nil + } + + return nil +} + +func (pg *peerGroup) AddMember(c oc.Neighbor, peer *peer) { pg.members[c.State.NeighborAddress.String()] = c + pg.neighborMap[c.State.NeighborAddress.String()] = peer } -func (pg *peerGroup) DeleteMember(c oc.Neighbor) { - delete(pg.members, c.State.NeighborAddress.String()) +func (pg *peerGroup) DeleteMember(addr string) { + delete(pg.members, addr) + delete(pg.neighborMap, addr) } func (pg *peerGroup) AddDynamicNeighbor(c *oc.DynamicNeighbor) { @@ -63,6 +248,163 @@ func (pg *peerGroup) DeleteDynamicNeighbor(prefix string) { delete(pg.dynamicNeighbors, prefix) } +func (pg *peerGroup) ID() string { + return oc.NewPeerGroupPolicyAssignmentKeyFromName(pg.Conf.Config.PeerGroupName) +} + +func (pg *peerGroup) PeerInfo() *table.PeerInfo { + return pg.info +} + +func (pg *peerGroup) TableID() string { + return table.GLOBAL_RIB_NAME +} + +func (pg *peerGroup) PolicyID() string { + if pg.Conf.Config.SharedPolicy { + return pg.ID() + } + + return table.GLOBAL_RIB_NAME +} + +func (pg *peerGroup) AS() uint32 { + return pg.Conf.Config.PeerAs +} + +func (pg *peerGroup) needToAdvertise() bool { + return true +} + +func (pg *peerGroup) vrf() string { + return "" +} + +func (pg *peerGroup) routingPolicy() *table.RoutingPolicy { + return pg.policy +} + +func (pg *peerGroup) localRib() *table.TableManager { + return pg.loc +} + +func (pg *peerGroup) configuredRFlist() []bgp.Family { + return pg.rfList +} + +func (pg *peerGroup) isIBGPPeer() bool { + return pg.info.PeerType == oc.PEER_TYPE_INTERNAL +} + +func (pg *peerGroup) isEnabledFamily(family bgp.Family) bool { + return slices.Contains(pg.rfList, family) +} + +func (pg *peerGroup) isLLGREnabledFamily(family bgp.Family) bool { + return slices.Contains(pg.llgrRFList, family) +} + +func (pg *peerGroup) replacePeerAS() (bool, uint32, uint32) { + return pg.Conf.AsPathOptions.Config.ReplacePeerAs, + pg.Conf.Config.PeerAs, + pg.Conf.Config.LocalAs +} + +func (pg *peerGroup) isAddPathSendEnabled(family bgp.Family) bool { + // Different peers can treat ADD_PATH capability differently. For now + // we do not support per-peer-group receivers with ADD_PATH-capable peers + return false +} + +func (pg *peerGroup) isRouteReflectorClient() bool { + return pg.Conf.RouteReflector.Config.RouteReflectorClient +} + +func (pg *peerGroup) isRouteServerClient() bool { + return pg.Conf.RouteServer.Config.RouteServerClient +} + +func (pg *peerGroup) isSecondaryRouteEnabled() bool { + return pg.Conf.RouteServer.Config.SecondaryRoute +} + +func (pg *peerGroup) isGracefulRestartEnabled() bool { + return pg.Conf.GracefulRestart.Config.Enabled +} + +func (pg *peerGroup) allowAsPathLoopLocal() bool { + return pg.Conf.AsPathOptions.Config.AllowAsPathLoopLocal +} + +func (pg *peerGroup) resetLocalRestarting() bool { + var anyPeerLocalRestarting bool + for _, p := range pg.neighborMap { + if p.resetLocalRestarting() { + anyPeerLocalRestarting = true + } + } + return anyPeerLocalRestarting +} + +func (pg *peerGroup) send(paths, olds []*table.Path) { + for _, peer := range pg.neighborMap { + if !peer.needToAdvertise() { + continue + } + + // Perform last checks for filtering paths to avoid sending paths back to the peer + // that originated it. + filtered := filterPathsForPeer(peer, paths, olds) + peer.send(filtered, nil) + } +} + +func filterPathsForPeer(peer *peer, paths, olds []*table.Path) []*table.Path { + var filtered []*table.Path + for idx, path := range paths { + var old *table.Path + if olds != nil { + old = olds[idx] + } + + peerPath := peerFilterpath(peer, path, old) + if peerPath != nil { + peerPath = peerPostFilterpath(peer, peerPath) + } + if peerPath != nil { + // All modifications to path are done, compute hash to avoid each peer computing it + // to avoid data race when each peer goroutine + _ = path.GetHash() + } + + if peerPath == path { + // In most cases no paths will be filtered or _altered_, so we do a lazy filtering + // and start tracking filtered paths after encontering the first filtered path + if filtered != nil { + filtered = append(filtered, peerPath) + } + continue + } + + if filtered == nil { + filtered = make([]*table.Path, idx, len(paths)) + copy(filtered, paths[:idx]) + } + if peerPath != nil { + filtered = append(filtered, peerPath) + } + } + + if filtered != nil { + return filtered + } + return paths +} + +func (pg *peerGroup) logger() log.Logger { + return pg.l +} + func newDynamicPeer(g *oc.Global, neighborAddress string, pg *oc.PeerGroup, loc *table.TableManager, policy *table.RoutingPolicy, logger log.Logger) *peer { conf := oc.Neighbor{ Config: oc.NeighborConfig{ @@ -95,7 +437,7 @@ func newDynamicPeer(g *oc.Global, neighborAddress string, pg *oc.PeerGroup, loc }) return nil } - peer := newPeer(g, &conf, loc, policy, logger) + peer := newPeer(g, &conf, pg, loc, policy, logger) peer.fsm.lock.Lock() peer.fsm.state = bgp.BGP_FSM_ACTIVE peer.fsm.lock.Unlock() @@ -103,35 +445,43 @@ func newDynamicPeer(g *oc.Global, neighborAddress string, pg *oc.PeerGroup, loc } type peer struct { + peerPathLimiterImpl + tableId string + policyId string fsm *fsm adjRibIn *table.AdjRib policy *table.RoutingPolicy - localRib *table.TableManager + loc *table.TableManager peerInfo *table.PeerInfo prefixLimitWarned map[bgp.Family]bool - // map of path local identifiers sent for that prefix - sentPaths map[table.PathDestLocalKey]map[uint32]struct{} - sendMaxPathFiltered map[table.PathLocalKey]struct{} - llgrEndChs []chan struct{} - longLivedRunning bool + + llgrEndChs []chan struct{} + longLivedRunning bool } -func newPeer(g *oc.Global, conf *oc.Neighbor, loc *table.TableManager, policy *table.RoutingPolicy, logger log.Logger) *peer { +func newPeer(g *oc.Global, conf *oc.Neighbor, pg *oc.PeerGroup, loc *table.TableManager, policy *table.RoutingPolicy, logger log.Logger) *peer { peer := &peer{ - localRib: loc, - policy: policy, - fsm: newFSM(g, conf, logger), - prefixLimitWarned: make(map[bgp.Family]bool), - sentPaths: make(map[table.PathDestLocalKey]map[uint32]struct{}), - sendMaxPathFiltered: make(map[table.PathLocalKey]struct{}), + loc: loc, + policy: policy, + fsm: newFSM(g, conf, logger), + prefixLimitWarned: make(map[bgp.Family]bool), } + if peer.isRouteServerClient() { peer.tableId = conf.State.NeighborAddress.String() } else { peer.tableId = table.GLOBAL_RIB_NAME } + + if pg != nil && pg.Config.SharedPolicy { + peer.policyId = oc.NewPeerGroupPolicyAssignmentKeyFromName(pg.Config.PeerGroupName) + } else { + peer.policyId = peer.tableId + } + rfs, _ := oc.AfiSafis(conf.AfiSafis).ToRfList() + peer.init(conf.AfiSafis) peer.adjRibIn = table.NewAdjRib(peer.fsm.logger, rfs) return peer } @@ -148,6 +498,21 @@ func (peer *peer) ID() string { return peer.fsm.pConf.State.NeighborAddress.String() } +func (peer *peer) PeerGroup() string { + peer.fsm.lock.RLock() + defer peer.fsm.lock.RUnlock() + return peer.fsm.pConf.Config.PeerGroup +} + +func (peer *peer) PeerInfo() *table.PeerInfo { + peer.fsm.lock.RLock() + defer peer.fsm.lock.RUnlock() + + // Return a copy since we're releasing lock here + peerInfo := *peer.peerInfo + return &peerInfo +} + func (peer *peer) routerID() netip.Addr { peer.fsm.lock.RLock() defer peer.fsm.lock.RUnlock() @@ -167,12 +532,38 @@ func (peer *peer) allowAsPathLoopLocal() bool { return peer.fsm.pConf.AsPathOptions.Config.AllowAsPathLoopLocal } +func (peer *peer) PolicyID() string { + return peer.policyId +} + +func (peer *peer) vrf() string { + peer.fsm.lock.RLock() + defer peer.fsm.lock.RUnlock() + return peer.fsm.pConf.Config.Vrf +} + +func (peer *peer) routingPolicy() *table.RoutingPolicy { + return peer.policy +} + +func (peer *peer) localRib() *table.TableManager { + return peer.loc +} + func (peer *peer) isIBGPPeer() bool { peer.fsm.lock.RLock() defer peer.fsm.lock.RUnlock() return peer.fsm.pConf.State.PeerType == oc.PEER_TYPE_INTERNAL } +func (peer *peer) replacePeerAS() (bool, uint32, uint32) { + peer.fsm.lock.RLock() + defer peer.fsm.lock.RUnlock() + return peer.fsm.pConf.AsPathOptions.Config.ReplacePeerAs, + peer.fsm.pConf.Config.LocalAs, + peer.fsm.pConf.Config.PeerAs +} + func (peer *peer) isRouteServerClient() bool { peer.fsm.lock.RLock() defer peer.fsm.lock.RUnlock() @@ -197,98 +588,54 @@ func (peer *peer) isGracefulRestartEnabled() bool { return peer.fsm.pConf.GracefulRestart.State.Enabled } -func (peer *peer) getAddPathMode(family bgp.Family) bgp.BGPAddPathMode { - peer.fsm.lock.RLock() - defer peer.fsm.lock.RUnlock() - if mode, y := peer.fsm.rfMap[family]; y { - return mode - } - return bgp.BGP_ADD_PATH_NONE -} +func (peer *peer) resetLocalRestarting() bool { + peer.fsm.lock.Lock() + defer peer.fsm.lock.Unlock() -func (peer *peer) isAddPathReceiveEnabled(family bgp.Family) bool { - return peer.getAddPathMode(family)&bgp.BGP_ADD_PATH_RECEIVE > 0 + peerLocalRestarting := peer.fsm.pConf.GracefulRestart.State.LocalRestarting + peer.fsm.pConf.GracefulRestart.State.LocalRestarting = false + return peerLocalRestarting } -func (peer *peer) isAddPathSendEnabled(family bgp.Family) bool { - return peer.getAddPathMode(family)&bgp.BGP_ADD_PATH_SEND > 0 +func (peer *peer) logger() log.Logger { + return peer.fsm.logger } -func (peer *peer) getAddPathSendMax(family bgp.Family) uint8 { +func (peer *peer) needToAdvertise() bool { peer.fsm.lock.RLock() - defer peer.fsm.lock.RUnlock() - for _, a := range peer.fsm.pConf.AfiSafis { - if a.State.Family == family { - return a.AddPaths.Config.SendMax - } - } - return 0 -} - -func (peer *peer) getRoutesCount(family bgp.Family, dstPrefix string) uint8 { - destLocalKey := table.NewPathDestLocalKey(family, dstPrefix) - if identifiers, ok := peer.sentPaths[*destLocalKey]; ok { - count := len(identifiers) - // the send-max config is uint8, so we need to check for overflow - if count > int(^uint8(0)) { - return ^uint8(0) - } - return uint8(count) - } - return 0 -} - -func (peer *peer) updateRoutes(paths ...*table.Path) { - if len(paths) == 0 { - return + notEstablished := peer.fsm.state != bgp.BGP_FSM_ESTABLISHED + localRestarting := peer.fsm.pConf.GracefulRestart.State.LocalRestarting + peer.fsm.lock.RUnlock() + if notEstablished { + return false } - for _, path := range paths { - localKey := path.GetLocalKey() - destLocalKey := localKey.PathDestLocalKey - identifiers, destExists := peer.sentPaths[destLocalKey] - if path.IsWithdraw && destExists { - delete(identifiers, path.LocalID()) - } else if !path.IsWithdraw { - if !destExists { - peer.sentPaths[destLocalKey] = make(map[uint32]struct{}) - } - identifiers := peer.sentPaths[destLocalKey] - if len(identifiers) < int(peer.getAddPathSendMax(destLocalKey.Family)) { - identifiers[localKey.Id] = struct{}{} - } - } + if localRestarting { + peer.fsm.lock.RLock() + peer.fsm.logger.Debug("now syncing, suppress sending updates", + log.Fields{ + "Topic": "Peer", + "Key": peer.fsm.pConf.State.NeighborAddress}) + peer.fsm.lock.RUnlock() + return false } + return true } -func (peer *peer) isPathSendMaxFiltered(path *table.Path) bool { - if path == nil { - return false +func (peer *peer) getAddPathMode(family bgp.Family) bgp.BGPAddPathMode { + peer.fsm.lock.RLock() + defer peer.fsm.lock.RUnlock() + if mode, y := peer.fsm.rfMap[family]; y { + return mode } - _, found := peer.sendMaxPathFiltered[path.GetLocalKey()] - return found + return bgp.BGP_ADD_PATH_NONE } -func (peer *peer) unsetPathSendMaxFiltered(path *table.Path) bool { - if path == nil { - return false - } - if _, ok := peer.sendMaxPathFiltered[path.GetLocalKey()]; !ok { - return false - } - delete(peer.sendMaxPathFiltered, path.GetLocalKey()) - return true +func (peer *peer) isAddPathReceiveEnabled(family bgp.Family) bool { + return peer.getAddPathMode(family)&bgp.BGP_ADD_PATH_RECEIVE > 0 } -func (peer *peer) hasPathAlreadyBeenSent(path *table.Path) bool { - if path == nil { - return false - } - destLocalKey := path.GetDestLocalKey() - if _, dstExist := peer.sentPaths[destLocalKey]; !dstExist { - return false - } - _, pathExist := peer.sentPaths[destLocalKey][path.LocalID()] - return pathExist +func (peer *peer) isAddPathSendEnabled(family bgp.Family) bool { + return peer.getAddPathMode(family)&bgp.BGP_ADD_PATH_SEND > 0 } func (peer *peer) isDynamicNeighbor() bool { @@ -350,11 +697,9 @@ func (peer *peer) negotiatedRFList() []bgp.Family { return l } -func (peer *peer) toGlobalFamilies(families []bgp.Family) []bgp.Family { +func toGlobalFamilies(peer receiver, families []bgp.Family) []bgp.Family { id := peer.ID() - peer.fsm.lock.RLock() - defer peer.fsm.lock.RUnlock() - if peer.fsm.pConf.Config.Vrf != "" { + if vrf := peer.vrf(); vrf != "" { fs := make([]bgp.Family, 0, len(families)) for _, f := range families { switch f { @@ -367,13 +712,12 @@ func (peer *peer) toGlobalFamilies(families []bgp.Family) []bgp.Family { case bgp.RF_FS_IPv6_UC: fs = append(fs, bgp.RF_FS_IPv6_VPN) default: - peer.fsm.logger.Warn("invalid family configured for neighbor with vrf", + peer.logger().Warn("invalid family configured for neighbor with vrf", log.Fields{ "Topic": "Peer", "Key": id, "Family": f, - "VRF": peer.fsm.pConf.Config.Vrf, - }) + "VRF": vrf}) } } families = fs @@ -381,6 +725,22 @@ func (peer *peer) toGlobalFamilies(families []bgp.Family) []bgp.Family { return families } +func llgrFamilies(afiSafis []oc.AfiSafi, checkState bool) []bgp.Family { + list := make([]bgp.Family, 0, len(afiSafis)) + for _, a := range afiSafis { + if !a.LongLivedGracefulRestart.Config.Enabled { + continue + } + if checkState && !a.LongLivedGracefulRestart.State.Enabled { + // NOTE: only peers have state - for peer groups we should check + // only config when filtering on peer-group level + continue + } + list = append(list, a.State.Family) + } + return list +} + func classifyFamilies(all, part []bgp.Family) ([]bgp.Family, []bgp.Family) { a := []bgp.Family{} b := []bgp.Family{} @@ -411,16 +771,15 @@ func (peer *peer) forwardingPreservedFamilies() ([]bgp.Family, []bgp.Family) { func (peer *peer) llgrFamilies() ([]bgp.Family, []bgp.Family) { peer.fsm.lock.RLock() - list := []bgp.Family{} - for _, a := range peer.fsm.pConf.AfiSafis { - if a.LongLivedGracefulRestart.State.Enabled { - list = append(list, a.State.Family) - } - } + list := llgrFamilies(peer.fsm.pConf.AfiSafis, true) peer.fsm.lock.RUnlock() return classifyFamilies(peer.configuredRFlist(), list) } +func (peer *peer) isEnabledFamily(family bgp.Family) bool { + return slices.Contains(peer.configuredRFlist(), family) +} + func (peer *peer) isLLGREnabledFamily(family bgp.Family) bool { peer.fsm.lock.RLock() llgrEnabled := peer.fsm.pConf.GracefulRestart.Config.LongLivedEnabled @@ -507,6 +866,11 @@ func (peer *peer) interestedIn(path *table.Path) bool { } func (peer *peer) filterPathFromSourcePeer(path, old *table.Path) *table.Path { + // EOR is originated by us, other peer cannot be source of it + if path.IsEOR() { + return path + } + // Consider 3 peers - A, B, C and prefix P originated by C. Parallel eBGP // sessions exist between A & B, and both have a single session with C. // @@ -556,6 +920,7 @@ func (peer *peer) filterPathFromSourcePeer(path, old *table.Path) *table.Path { "Data": path, }) } + pathExportSkipped(peer, path, "filter-from-source-peer") return nil } @@ -675,12 +1040,29 @@ func (peer *peer) handleUpdate(e *fsmMsg) ([]*table.Path, []bgp.Family, bool) { // route should be excluded from the Phase 2 decision function. if aspath := path.GetAsPath(); aspath != nil { peer.fsm.lock.RLock() - localAS := peer.fsm.pConf.Config.LocalAs + localAS := peer.fsm.gConf.Config.As + localASForPeer := peer.peerInfo.LocalAS allowOwnAS := int(peer.fsm.pConf.AsPathOptions.Config.AllowOwnAs) peer.fsm.lock.RUnlock() - if hasOwnASLoop(localAS, allowOwnAS, aspath) { - path.SetRejected(true) - continue + + hasLocalAS := hasOwnASLoop(localAS, allowOwnAS, aspath) + hasLocalASForPeer := hasOwnASLoop(localASForPeer, allowOwnAS, aspath) + if hasLocalAS || hasLocalASForPeer { + // FIXME: in GoBGPv3 only AS specified in a global config was checked + // so we issue a warning, but still allow a path to be handled + reject := hasLocalAS + if reject { + path.SetRejected(true) + continue + } + + peer.fsm.warnOnce(logOnceAllowOwnAs, + "Got a path with Local AS in AS Path."+ + " Such paths can be accepted for now, but without allow-own-as properly configured might be rejected in future.", + log.Fields{ + "Topic": "Peer", + "Key": peer.fsm.pConf.State.NeighborAddress, + "nlri": path.GetNlri().String()}) } } // RFC4456 8. Avoiding Routing Information Loops @@ -732,6 +1114,12 @@ func (peer *peer) stopFSMHandler() { peer.fsm.h.ctxCancel() } +func (peer *peer) send(paths, olds []*table.Path) { + peer.fsm.outgoingCh.In() <- &fsmOutgoingMsg{ + Paths: paths, + } +} + func (peer *peer) StaleAll(rfList []bgp.Family) []*table.Path { return peer.adjRibIn.StaleAll(rfList) } diff --git a/pkg/server/peer_test.go b/pkg/server/peer_test.go new file mode 100644 index 000000000..e43bbb8ac --- /dev/null +++ b/pkg/server/peer_test.go @@ -0,0 +1,744 @@ +package server + +import ( + "context" + "fmt" + "net" + "net/netip" + "slices" + "strings" + "testing" + "time" + + api "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/log" + "github.com/osrg/gobgp/v4/pkg/packet/bgp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + peerTopoSrvCount = 2 + + ptPolicyPrefixDefinedSet = "prefixes" + ptPolicyPrefixAccepted = "10.1.0.0" + ptPolicyPrefixRejected = "10.2.0.0" + ptPolicyNameAccept = "accept" + ptPolicyNameReject = "reject" + ptPeerGroupSenders = "senders" + ptPeerGroupReceivers = "receivers" +) + +var ( + peerTopoHubPolicies = []*api.PolicyAssignment{ + { + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, + DefaultAction: api.RouteAction_ROUTE_ACTION_ACCEPT, + }, + { + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_EXPORT, + Policies: []*api.Policy{ + { + Name: ptPolicyNameAccept, + Statements: []*api.Statement{ + { + Name: ptPolicyNameAccept + "-stmt0", + Conditions: &api.Conditions{ + PrefixSet: &api.MatchSet{ + Type: api.MatchSet_TYPE_ANY, + Name: ptPolicyPrefixDefinedSet, + }, + }, + Actions: &api.Actions{RouteAction: api.RouteAction_ROUTE_ACTION_ACCEPT}, + }, + }, + }, + { + Name: ptPolicyNameReject, + Statements: []*api.Statement{ + { + Name: ptPolicyNameReject + "-stmt0", + Actions: &api.Actions{RouteAction: api.RouteAction_ROUTE_ACTION_REJECT}, + }, + }, + }, + }, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }, + } + + peerTopoRt = bgp.NewTwoOctetAsSpecificExtended(bgp.EC_SUBTYPE_ROUTE_TARGET, 65001, 100, true) +) + +type peerTopoTest struct { + routeReflector bool + routeServer bool + rsSecondaryRts bool + sharedPolicy string + sharedPolicyImport bool + + // 0 means RTC is disabled, any positive value - only that receiver should get path + rtcReceiverIndex int + llgrReceiverIndex int + + expectedReceiverRoutes int +} + +func (t peerTopoTest) name() string { + parts := make([]string, 0, 4) + addPart := func(cond bool, part string) { + if cond { + parts = append(parts, part) + } + } + + addPart(t.routeReflector, "route-reflector") + addPart(t.routeServer, "route-server") + addPart(t.rsSecondaryRts, "secondary-routes") + addPart(t.rtcReceiverIndex > 0, "rtc") + addPart(t.llgrReceiverIndex > 0, "llgr") + addPart(t.sharedPolicy != "", t.sharedPolicy) + addPart(t.sharedPolicyImport, "import") + return strings.Join(parts, "-") +} + +type peerTopo struct { + hub *BgpServer + senders []*BgpServer + receivers []*BgpServer +} + +func (tt peerTopoTest) makeTopo() (topo peerTopo, err error) { + ctx := context.Background() + + asn := uint32(1) + addr := net.IPv4(127, 0, 0, 100).To4() + + topo.hub = NewBgpServer() + topo.hub.logger.SetLevel(log.DebugLevel) + go topo.hub.Serve() + + if err := topo.hub.StartBgp(ctx, &api.StartBgpRequest{ + Global: &api.Global{ + Asn: asn, + RouterId: "1.1.1.1", + ListenAddresses: []string{addr.String()}, + ListenPort: 10179, + }, + }); err != nil { + return topo, fmt.Errorf("error starting bgp on hub: %w", err) + } + + if err := topo.hub.AddDefinedSet(ctx, &api.AddDefinedSetRequest{ + DefinedSet: &api.DefinedSet{ + DefinedType: api.DefinedType_DEFINED_TYPE_PREFIX, + Name: ptPolicyPrefixDefinedSet, + Prefixes: []*api.Prefix{ + {IpPrefix: ptPolicyPrefixAccepted + "/24", MaskLengthMin: 24, MaskLengthMax: 32}, + }, + }, + }); err != nil { + return topo, fmt.Errorf("error adding defined set: %w", err) + } + for _, a := range peerTopoHubPolicies { + for _, pol := range a.Policies { + if err := topo.hub.AddPolicy(ctx, &api.AddPolicyRequest{ + Policy: pol, + }); err != nil { + return topo, fmt.Errorf("error adding policy: %w", err) + } + } + + if err := topo.hub.AddPolicyAssignment(ctx, &api.AddPolicyAssignmentRequest{ + Assignment: a, + }); err != nil { + return topo, fmt.Errorf("error adding policy assignment: %w", err) + } + } + + wg := waitStateMultiple(topo.hub, api.PeerState_SESSION_STATE_ESTABLISHED, 2*peerTopoSrvCount) + for pgIndex, pg := range []struct { + name string + servers *[]*BgpServer + }{ + {ptPeerGroupSenders, &topo.senders}, + {ptPeerGroupReceivers, &topo.receivers}, + } { + // Use iBGP (same ASN) in route-reflector tests, eBGP in route-server tests + if tt.routeServer { + asn++ + } + + pgConfig := tt.makePGConfig(pg.name, asn) + if err := topo.hub.AddPeerGroup(ctx, &api.AddPeerGroupRequest{ + PeerGroup: oc.NewPeerGroupFromConfigStruct(pgConfig), + }); err != nil { + return topo, fmt.Errorf("error adding peer group: %w", err) + } + + for srvIndex := range peerTopoSrvCount { + addr[3]++ + + srv := NewBgpServer() + go srv.Serve() + *pg.servers = append(*pg.servers, srv) + + var grCfg *api.GracefulRestart + if tt.llgrReceiverIndex > 0 { + grCfg = &api.GracefulRestart{ + Enabled: true, + RestartTime: 30, + LonglivedEnabled: true, + } + } + + if err := srv.StartBgp(ctx, &api.StartBgpRequest{ + Global: &api.Global{ + Asn: asn, + RouterId: fmt.Sprintf("1.2.%d.%d", pgIndex, srvIndex), + ListenAddresses: []string{addr.String()}, + ListenPort: -1, + GracefulRestart: grCfg, + }, + }); err != nil { + return topo, fmt.Errorf("error starting peer bgp: %w", err) + } + + srvNeigh := &oc.Neighbor{ + Config: tt.makeNeighborConfig(srv, pg.name), + Transport: tt.makeTransportConfig(topo.hub, srv, true), + } + if err := topo.hub.AddPeer(ctx, &api.AddPeerRequest{ + Peer: oc.NewPeerFromConfigStruct(srvNeigh), + }); err != nil { + return topo, fmt.Errorf("error adding hub peer: %w", err) + } + + hubNeigh := &oc.Neighbor{ + Config: tt.makeNeighborConfig(topo.hub, ""), + GracefulRestart: pgConfig.GracefulRestart, + Transport: tt.makeTransportConfig(srv, topo.hub, false), + AfiSafis: tt.makeHubNeighAfiSafis(pg.name, srvIndex, pgConfig.AfiSafis), + } + if err := srv.AddPeer(ctx, &api.AddPeerRequest{ + Peer: oc.NewPeerFromConfigStruct(hubNeigh), + }); err != nil { + return topo, fmt.Errorf("error adding leaf peer: %w", err) + } + } + } + wg.Wait() + + return topo, nil +} + +func (tt peerTopoTest) makeHubNeighAfiSafis(pgName string, srvIndex int, afiSafis []oc.AfiSafi) []oc.AfiSafi { + if tt.llgrReceiverIndex == 0 || pgName != ptPeerGroupReceivers { + return afiSafis + } + + // For llgr-test - do not negotiate LLGR with one of receivers + // and do not expect it to receive llgr route + if srvIndex == tt.llgrReceiverIndex { + return afiSafis + } + + hubAfiSafis := slices.Clone(afiSafis) + for i := range hubAfiSafis { + hubAfiSafis[i].LongLivedGracefulRestart.Config.Enabled = false + } + return hubAfiSafis +} + +func (tt peerTopoTest) getNeighborAddr(srv *BgpServer) netip.Addr { + return srv.bgpConfig.Global.Config.LocalAddressList[0] +} + +func (tt peerTopoTest) makeNeighborConfig(srv *BgpServer, pgName string) oc.NeighborConfig { + return oc.NeighborConfig{ + NeighborAddress: tt.getNeighborAddr(srv), + PeerAs: srv.bgpConfig.Global.Config.As, + PeerGroup: pgName, + } +} + +func (tt peerTopoTest) makeTransportConfig(local, peer *BgpServer, passiveMode bool) oc.Transport { + return oc.Transport{ + Config: oc.TransportConfig{ + LocalAddress: tt.getNeighborAddr(local), + RemotePort: uint16(peer.bgpConfig.Global.Config.Port), + PassiveMode: passiveMode, + }, + } +} + +func (tt peerTopoTest) makePGConfig(name string, asn uint32) *oc.PeerGroup { + var ( + grCfg oc.GracefulRestartConfig + mpGrCfg oc.MpGracefulRestartConfig + llgrCfg oc.LongLivedGracefulRestartConfig + ) + if tt.llgrReceiverIndex > 0 { + grCfg.Enabled = true + grCfg.RestartTime = 30 + grCfg.LongLivedEnabled = true + mpGrCfg.Enabled = true + llgrCfg.Enabled = true + llgrCfg.RestartTime = 300 + } + + pg := &oc.PeerGroup{ + Config: oc.PeerGroupConfig{ + PeerAs: asn, + PeerGroupName: name, + }, + GracefulRestart: oc.GracefulRestart{Config: grCfg}, + AfiSafis: []oc.AfiSafi{ + { + Config: oc.AfiSafiConfig{AfiSafiName: oc.AFI_SAFI_TYPE_IPV4_UNICAST, Enabled: true}, + + MpGracefulRestart: oc.MpGracefulRestart{Config: mpGrCfg}, + LongLivedGracefulRestart: oc.LongLivedGracefulRestart{Config: llgrCfg}, + }, + }, + } + + if tt.routeServer { + pg.RouteServer = oc.RouteServer{ + Config: oc.RouteServerConfig{ + RouteServerClient: true, + SecondaryRoute: tt.rsSecondaryRts, + }, + } + } else if tt.routeReflector { + pg.RouteReflector = oc.RouteReflector{ + Config: oc.RouteReflectorConfig{ + RouteReflectorClient: true, + }, + } + } + + if tt.sharedPolicy != "" { + apCfg := oc.ApplyPolicyConfig{ + DefaultImportPolicy: oc.DEFAULT_POLICY_TYPE_ACCEPT_ROUTE, + ExportPolicyList: []string{tt.sharedPolicy}, + DefaultExportPolicy: oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, + } + if tt.sharedPolicyImport { + // Test filtering on import, not export. This doesn't help with + // propagateUpdate performance, but makes configuring policies easier + // too (and allow to avoid neighbor sets) + apCfg.DefaultImportPolicy, apCfg.DefaultExportPolicy = apCfg.DefaultExportPolicy, apCfg.DefaultImportPolicy + apCfg.ImportPolicyList, apCfg.ExportPolicyList = apCfg.ExportPolicyList, apCfg.ImportPolicyList + } + + pg.ApplyPolicy = oc.ApplyPolicy{Config: apCfg} + pg.Config.SharedPolicy = true + } else if tt.routeServer { + pg.ApplyPolicy = oc.ApplyPolicy{ + Config: oc.ApplyPolicyConfig{ + DefaultImportPolicy: oc.DEFAULT_POLICY_TYPE_ACCEPT_ROUTE, + ExportPolicyList: []string{ptPolicyNameAccept}, + DefaultExportPolicy: oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, + }, + } + } + + if tt.rtcReceiverIndex > 0 && name == ptPeerGroupReceivers { + pg.AfiSafis = append(pg.AfiSafis, oc.AfiSafi{ + Config: oc.AfiSafiConfig{AfiSafiName: oc.AFI_SAFI_TYPE_RTC, Enabled: true}, + }) + } + + return pg +} + +func (topo peerTopo) dump() { + dumpTableInfo := func(tableName string, rf bgp.Family, info *table.TableInfo, err error) { + if err != nil { + fmt.Printf("\t%s/%s: %s\n", tableName, rf, err.Error()) + return + } + + if info.NumDestination > 0 { + fmt.Printf("\t%s/%s: %d destinations, %d paths\n", tableName, rf, info.NumDestination, info.NumPath) + } + } + + dumpServer := func(srv *BgpServer) { + _ = srv.mgmtOperation(func() error { + for rf, tbl := range srv.globalRib.Tables { + dumpTableInfo("global", rf, tbl.Info(), nil) + } + for rf, tbl := range srv.rsRib.Tables { + dumpTableInfo("rs", rf, tbl.Info(), nil) + } + for neighAddr, neigh := range srv.neighborMap { + for _, rf := range neigh.configuredRFlist() { + info, err := neigh.adjRibIn.TableInfo(rf) + dumpTableInfo(neighAddr+" adj-in", rf, info, err) + } + } + + return nil + }, true) + } + + fmt.Println("hub paths:") + dumpServer(topo.hub) + for i, srv := range topo.senders { + fmt.Printf("sender#%d paths:\n", i) + dumpServer(srv) + } + for i, srv := range topo.receivers { + fmt.Printf("receiver#%d paths:\n", i) + dumpServer(srv) + } +} + +func (topo peerTopo) stop() { + topo.dump() + if topo.hub != nil { + topo.hub.Stop() + } + for _, srv := range topo.senders { + srv.Stop() + } + for _, srv := range topo.receivers { + srv.Stop() + } +} + +func (topo peerTopo) makeDefaultAtrs(llgrStale bool) []bgp.PathAttributeInterface { + nh, _ := bgp.NewPathAttributeNextHop(netip.MustParseAddr("10.0.0.1")) + pattr := []bgp.PathAttributeInterface{ + bgp.NewPathAttributeOrigin(0), + nh, + } + + if llgrStale { + comm := bgp.NewPathAttributeCommunities([]uint32{uint32(bgp.COMMUNITY_LLGR_STALE)}) + pattr = append(pattr, comm) + } + + return pattr +} + +func (topo peerTopo) addPaths(family bgp.Family, llgrStale bool) error { + nlri1, _ := bgp.NewIPAddrPrefix(netip.MustParsePrefix(ptPolicyPrefixAccepted + "/24")) + nlri2, _ := bgp.NewIPAddrPrefix(netip.MustParsePrefix(ptPolicyPrefixRejected + "/24")) + extComm := bgp.NewPathAttributeExtendedCommunities([]bgp.ExtendedCommunityInterface{peerTopoRt}) + + for _, srv := range topo.senders { + for _, nlri := range []bgp.NLRI{nlri1, nlri2} { + if _, err := srv.AddPath(apiutil.AddPathRequest{ + Paths: []*apiutil.Path{ + { + Family: family, + Nlri: nlri, + Attrs: append(topo.makeDefaultAtrs(llgrStale), extComm), + }, + }, + }); err != nil { + return err + } + } + } + + return nil +} + +func (topo peerTopo) addRTCPaths(rtcIndex int) error { + srv := topo.receivers[rtcIndex] + _, err := srv.AddPath(apiutil.AddPathRequest{ + Paths: []*apiutil.Path{ + { + Family: bgp.RF_RTC_UC, + Nlri: bgp.NewRouteTargetMembershipNLRI(srv.bgpConfig.Global.Config.As, peerTopoRt), + Attrs: topo.makeDefaultAtrs(false), + }, + }, + }) + return err +} + +func (tt peerTopoTest) expectedPaths(srvIndex int) int { + if tt.rtcReceiverIndex > 0 && tt.rtcReceiverIndex != srvIndex { + return 0 + } + if tt.llgrReceiverIndex > 0 && tt.llgrReceiverIndex != srvIndex { + return 0 + } + return tt.expectedReceiverRoutes +} + +func countReachPaths(paths []*table.Path) (count int) { + for _, p := range paths { + if !p.IsEOR() && !p.IsWithdraw { + count++ + } + } + return +} + +// Topology test spawns multiple servers and tests various aspects of passing route via +// route reflector or server using different types of policy application (global policy, +// per-peer policy for route server or per-peer-group policy using shared-export-policy +// option) +// +// This should've been full-fledged scenario test, but since they lack convenience of +// debugging, here we are. +// +// TODO: for secondary route test add additional filtering and checks that proper secondary +// route is received +func TestPeerStarTopology(t *testing.T) { + for _, tt := range []peerTopoTest{ + { + routeReflector: true, + expectedReceiverRoutes: 1, + }, + { + routeReflector: true, + rtcReceiverIndex: 1, + expectedReceiverRoutes: 1, + }, + { + routeReflector: true, + llgrReceiverIndex: 1, + expectedReceiverRoutes: 1, + }, + { + routeReflector: true, + sharedPolicy: ptPolicyNameAccept, + expectedReceiverRoutes: 1, + }, + { + routeReflector: true, + sharedPolicy: ptPolicyNameAccept, + sharedPolicyImport: true, + expectedReceiverRoutes: 1, + }, + { + routeReflector: true, + sharedPolicy: ptPolicyNameReject, + expectedReceiverRoutes: 0, + }, + { + routeReflector: true, + sharedPolicy: ptPolicyNameAccept, + rtcReceiverIndex: 1, + expectedReceiverRoutes: 1, + }, + { + routeServer: true, + expectedReceiverRoutes: 1, + }, + { + routeServer: true, + rsSecondaryRts: true, + expectedReceiverRoutes: 1, + }, + { + routeServer: true, + sharedPolicy: ptPolicyNameAccept, + expectedReceiverRoutes: 1, + }, + { + routeServer: true, + sharedPolicy: ptPolicyNameAccept, + sharedPolicyImport: true, + expectedReceiverRoutes: 1, + }, + { + routeReflector: true, + sharedPolicy: ptPolicyNameAccept, + llgrReceiverIndex: 1, + expectedReceiverRoutes: 1, + }, + { + routeServer: true, + rsSecondaryRts: true, + sharedPolicy: ptPolicyNameAccept, + expectedReceiverRoutes: 1, + }, + { + routeServer: true, + sharedPolicy: ptPolicyNameReject, + expectedReceiverRoutes: 0, + }, + } { + t.Run(tt.name(), func(t *testing.T) { + topo, err := tt.makeTopo() + require.NoError(t, err, "error while making topo") + + if tt.sharedPolicy != "" { + assert.Len(t, topo.hub.receiverMap, 2) + } else { + assert.Len(t, topo.hub.receiverMap, 2*peerTopoSrvCount) + } + + if tt.rtcReceiverIndex > 0 { + err = topo.addRTCPaths(tt.rtcReceiverIndex) + require.NoError(t, err, "error while adding rtc paths") + } + + topo.hub.logger.Info("Start test", log.Fields{"test": tt.name()}) + err = topo.addPaths(bgp.RF_IPv4_UC, tt.llgrReceiverIndex > 0) + require.NoError(t, err, "error while adding paths") + + assert.EventuallyWithT(t, func(collect *assert.CollectT) { + for srvIndex, srv := range topo.receivers { + var count int + err := srv.ListPath(apiutil.ListPathRequest{ + TableType: api.TableType_TABLE_TYPE_GLOBAL, + Family: bgp.RF_IPv4_UC, + }, func(prefix bgp.NLRI, paths []*apiutil.Path) { + require.True(t, strings.HasPrefix(prefix.String(), ptPolicyPrefixAccepted), + fmt.Sprintf("route prefix %q starts with %q", prefix.String(), ptPolicyPrefixAccepted)) + count += len(paths) + }) + assert.NoError(t, err) + assert.Equal(collect, tt.expectedPaths(srvIndex), count) + } + }, 10*time.Second, 100*time.Millisecond) + + // Check that our adj-out is correct too if peer reconnects, or + // during initial state when we sync all peers at once + err = topo.hub.mgmtOperation(func() error { + checkReceiver := func(r receiver, expected int) { + pathList, _ := topo.hub.getBestFromLocal(r, r.configuredRFlist()) + assert.Equal(t, expected, countReachPaths(pathList)) + } + + recvPg := topo.hub.peerGroupMap[ptPeerGroupReceivers] + for srvIndex, srv := range topo.receivers { + addr := tt.getNeighborAddr(srv).String() + checkReceiver(recvPg.neighborMap[addr], tt.expectedPaths(srvIndex)) + } + if tt.sharedPolicy != "" { + // NOTE: getAdjOut() ignores LLGR/RTC for peer groups since LLGR/RTC is per-peer basis, + // so while we sent correct set of routes (checked earlier), we expect total + // number selected by policy here + checkReceiver(recvPg, tt.expectedReceiverRoutes) + } + + return nil + }, false) + require.NoError(t, err, "error while checking getBestFromLocalCount") + + topo.stop() + + for pgName, pg := range topo.hub.peerGroupMap { + assert.Lenf(t, pg.neighborMap, 0, + "expect that all neighbors are deleted in peer group %q", pgName) + } + }) + } +} + +func TestPeerGroupSharedPolicyUpdate(t *testing.T) { + const ( + numPeers = 3 + pgName = "g" + ) + + assert := assert.New(t) + expectReceivers := func(s *BgpServer, expected []string) { + actual := make([]string, 0, len(expected)) + err := s.mgmtOperation(func() error { + for k := range s.receiverMap { + actual = append(actual, k) + } + return nil + }, false) + assert.NoError(err) + assert.ElementsMatch(expected, actual) + } + + s := NewBgpServer() + s.logger.SetLevel(log.DebugLevel) + go s.Serve() + + err := s.StartBgp(context.Background(), &api.StartBgpRequest{ + Global: &api.Global{ + Asn: 1, + RouterId: "1.1.1.1", + ListenPort: 10179, + }, + }) + assert.NoError(err) + defer s.StopBgp(context.Background(), &api.StopBgpRequest{}) + + pg := &oc.PeerGroup{ + Config: oc.PeerGroupConfig{ + PeerAs: 2, + PeerGroupName: pgName, + }, + } + err = s.addPeerGroup(pg) + assert.NoError(err) + + peerAddrs := make([]string, 0, numPeers) + for i := range numPeers { + n := &oc.Neighbor{ + Config: oc.NeighborConfig{ + NeighborAddress: netip.MustParseAddr(fmt.Sprintf("127.0.0.%d", 100+i)), + PeerGroup: pgName, + }, + Transport: oc.Transport{ + Config: oc.TransportConfig{ + PassiveMode: true, + }, + }, + } + peerAddrs = append(peerAddrs, n.Config.NeighborAddress.String()) + + err = s.AddPeer(context.Background(), &api.AddPeerRequest{Peer: oc.NewPeerFromConfigStruct(n)}) + assert.NoError(err) + } + expectReceivers(s, peerAddrs) + + // Enable shared policy: we should have only it as a receiver + pg.Config.SharedPolicy = true + _, err = s.updatePeerGroup(pg) + assert.NoError(err) + + pgReceivers := []string{oc.NewPeerGroupPolicyAssignmentKeyFromName(pgName)} + expectReceivers(s, pgReceivers) + + // Delete one peer in peer group, this should have no effect on server's receivers + // (but should on neighborMap inside pg) + err = s.DeletePeer(context.Background(), &api.DeletePeerRequest{ + Address: peerAddrs[0], + }) + assert.NoError(err) + peerAddrs = peerAddrs[1:] + + expectReceivers(s, pgReceivers) + + _ = s.mgmtOperation(func() error { + assert.Len(s.peerGroupMap[pgName].neighborMap, len(peerAddrs)) + return nil + }, false) + + // Disable shared policy - we should enable individual peers back + pg.Config.SharedPolicy = false + _, err = s.updatePeerGroup(pg) + assert.NoError(err) + + expectReceivers(s, peerAddrs) + + // Delete rest of peers, no receivers should be left after it + for _, addr := range peerAddrs { + err = s.DeletePeer(context.Background(), &api.DeletePeerRequest{ + Address: addr, + }) + assert.NoError(err) + } + expectReceivers(s, []string{}) +} diff --git a/pkg/server/server.go b/pkg/server/server.go index 06f25d6cb..9e436f643 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -119,6 +119,7 @@ type BgpServer struct { listeners []*netutils.TCPListener neighborMap map[string]*peer peerGroupMap map[string]*peerGroup + receiverMap map[string]receiver globalRib *table.TableManager rsRib *table.TableManager roaManager *roaManager @@ -155,6 +156,7 @@ func NewBgpServer(opt ...ServerOption) *BgpServer { shared: shared, neighborMap: make(map[string]*peer), peerGroupMap: make(map[string]*peerGroup), + receiverMap: make(map[string]receiver), policy: table.NewRoutingPolicy(logger), mgmtCh: make(chan *mgmtOp, 1), watcherMap: make(map[watchEventType][]*watcher), @@ -430,25 +432,20 @@ func (s *BgpServer) matchLongestDynamicNeighborPrefix(a string) *peerGroup { return longestPG } -func sendfsmOutgoingMsg(peer *peer, paths []*table.Path) { - peer.fsm.outgoingCh.In() <- &fsmOutgoingMsg{ - Paths: paths, - } -} - -func isASLoop(peer *peer, path *table.Path) bool { +func isASLoop(peer receiver, path *table.Path) bool { return slices.Contains(path.GetAsList(), peer.AS()) } -func filterpath(peer *peer, path, old *table.Path) *table.Path { - if path == nil { - return nil - } - +// peerFilterpath is called to skip paths that cannot be sent to individual peer +// even if peer-group config allows such path. Usually called before applying +// the policy, but if shared policy enabled, called after it (can change +// behavior of RTC, see doc). +func peerFilterpath(peer *peer, path, old *table.Path) *table.Path { peer.fsm.lock.RLock() _, ok := peer.fsm.rfMap[path.GetFamily()] peer.fsm.lock.RUnlock() if !ok { + pathExportSkipped(peer, path, "rf-not-negotiated") return nil } @@ -456,8 +453,10 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { peer.fsm.lock.RLock() _, y := peer.fsm.rfMap[bgp.RF_RTC_UC] peer.fsm.lock.RUnlock() + if y && path.GetFamily() != bgp.RF_RTC_UC { if !peer.interestedIn(path) { + pathExportSkipped(peer, path, "rtc") peer.fsm.logger.Debug("Filtered by Route Target Constraint, ignore", log.Fields{ "Topic": "Peer", @@ -481,32 +480,56 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { } } - // iBGP handling - if peer.isIBGPPeer() { + if path = peer.filterPathFromSourcePeer(path, old); path == nil { + return nil + } + + return path +} + +func filterpath(r receiver, path, old *table.Path) *table.Path { + if path == nil { + return nil + } + + if peer, isPeer := r.(*peer); isPeer { + // Run source, RF and RTC checks early if we have a peer here. For peer groups, + // run them later after all checks on per-peer basis + if path = peerFilterpath(peer, path, old); path == nil { + return nil + } + } else { + // For other received types check only configured route families + if !r.isEnabledFamily(path.GetFamily()) { + pathExportSkipped(r, path, "disabled-rf") + return nil + } + } + + //iBGP handling + if r.isIBGPPeer() { ignore := false if !path.IsLocal() { ignore = true info := path.GetSource() // if the path comes from eBGP peer - if info.AS != peer.AS() { + if info.AS != r.AS() { ignore = false } if info.RouteReflectorClient { ignore = false } - if peer.isRouteReflectorClient() { + if r.isRouteReflectorClient() { // RFC4456 8. Avoiding Routing Information Loops // If the local CLUSTER_ID is found in the CLUSTER_LIST, // the advertisement received SHOULD be ignored. for _, clusterID := range path.GetClusterList() { - peer.fsm.lock.RLock() - rrClusterID := net.ParseIP(peer.fsm.pConf.RouteReflector.State.RouteReflectorClusterId.String()) - peer.fsm.lock.RUnlock() - if slices.Equal(clusterID.AsSlice(), rrClusterID.To4()) { - peer.fsm.logger.Debug("cluster list path attribute has local cluster id, ignore", + rrClusterID := r.PeerInfo().RouteReflectorClusterID + if clusterID == rrClusterID { + r.logger().Debug("cluster list path attribute has local cluster id, ignore", log.Fields{ "Topic": "Peer", - "Key": peer.ID(), + "Key": r.ID(), "ClusterID": clusterID, "Data": path, }) @@ -520,7 +543,7 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { if ignore { if !path.IsWithdraw && old != nil { oldSource := old.GetSource() - if old.IsLocal() || oldSource.Address.String() != peer.ID() && oldSource.AS != peer.AS() { + if old.IsLocal() || oldSource.Address.String() != r.ID() && oldSource.AS != r.AS() { // In this case, we suppose this peer has the same prefix // received from another iBGP peer. // So we withdraw the old best which was injected locally @@ -532,11 +555,12 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { return old.Clone(true) } } - if peer.fsm.logger.GetLevel() >= log.DebugLevel { - peer.fsm.logger.Debug("From same AS, ignore", + if r.logger().GetLevel() >= log.DebugLevel { + pathExportSkipped(r, path, "ibgp") + r.logger().Debug("From same AS, ignore", log.Fields{ "Topic": "Peer", - "Key": peer.ID(), + "Key": r.ID(), "Path": path, }) } @@ -544,15 +568,11 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { } } - if path = peer.filterPathFromSourcePeer(path, old); path == nil { - return nil - } - - if !peer.isRouteServerClient() && isASLoop(peer, path) { + if !r.isRouteServerClient() && isASLoop(r, 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() { + if !path.IsLocal() || !r.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 @@ -570,27 +590,34 @@ func filterpath(peer *peer, path, old *table.Path) *table.Path { // A2 and path stuck. So in this case B1 should send explicit withdraw for old path. return old.Clone(true) } + pathExportSkipped(r, path, "as-loop") return nil } } return path } -func (s *BgpServer) prePolicyFilterpath(peer *peer, path, old *table.Path) (*table.Path, *table.PolicyOptions, bool) { +// pathExportSkipped is a hook function (hence, noinline) that mark places if path is +// not exported to a peer idenified by receiver. Can be used via dynamic tracing, to +// have single breakpoint or redefined via build flags into full-fledged logger call +// +//go:noinline +func pathExportSkipped(r receiver, path *table.Path, reason string) { +} + +func (s *BgpServer) prePolicyFilterpath(r receiver, path, old *table.Path) (*table.Path, *table.PolicyOptions, bool) { // Special handling for RTM NLRI. if path != nil && path.GetFamily() == bgp.RF_RTC_UC && !path.IsWithdraw { // If the given "path" is locally generated and the same with "old", we // assumes "path" was already sent before. This assumption avoids the // infinite UPDATE loop between Route Reflector and its clients. if path.IsLocal() && path.Equal(old) { - peer.fsm.lock.RLock() s.logger.Debug("given rtm nlri is already sent, skipping to advertise", log.Fields{ "Topic": "Peer", - "Key": peer.fsm.pConf.State.NeighborAddress, - "Path": path, - }) - peer.fsm.lock.RUnlock() + "Key": r.ID(), + "Path": path}) + pathExportSkipped(r, path, "rtm-already-sent") return nil, nil, true } @@ -599,18 +626,18 @@ func (s *BgpServer) prePolicyFilterpath(peer *peer, path, old *table.Path) (*tab // and make old.Clone(true). The only way to get path != nil and old != nil // is to change the path without changing rt. Then we need to update path or // do nothing if path == old. - } else if peer.isRouteReflectorClient() { + } else if r.isRouteReflectorClient() { // We need to send the path even if the peer is originator of the // path in order to signal that the client should distribute route // with the given RT. } else { - // We send a path even if it is not the best path. See comments in - // (*Destination) GetChanges(). - dst := peer.localRib.GetDestination(path) + // We send a path even if it is not the best path unless we only have RTM + // from the receiver peer. See comments in (*Destination) GetChanges(). + dst := r.localRib().GetDestination(path) path = nil - for _, p := range dst.GetKnownPathList(peer.TableID(), peer.AS()) { + for _, p := range dst.GetKnownPathList(r.TableID(), r.AS()) { srcPeer := p.GetSource() - if peer.ID() != srcPeer.Address.String() { + if r.ID() != srcPeer.Address.String() { if srcPeer.RouteReflectorClient { // The path from a RR client is preferred than others // for the case that RR and non RR client peering @@ -622,59 +649,60 @@ func (s *BgpServer) prePolicyFilterpath(peer *peer, path, old *table.Path) (*tab } } } + if path == nil { + pathExportSkipped(r, path, "rtm-only-from-peer") + } } } // only allow vpnv4 and vpnv6 paths to be advertised to VRFed neighbors. // also check we can import this path using table.CanImportToVrf() // if we can, make it local path by calling (*Path).ToLocal() - peer.fsm.lock.RLock() - peerVrf := peer.fsm.pConf.Config.Vrf - peer.fsm.lock.RUnlock() + peerVrf := r.vrf() if path != nil && peerVrf != "" { if f := path.GetFamily(); f != bgp.RF_IPv4_VPN && f != bgp.RF_IPv6_VPN && f != bgp.RF_FS_IPv4_VPN && f != bgp.RF_FS_IPv6_VPN { return nil, nil, true } - vrf := peer.localRib.Vrfs[peerVrf] + vrf := r.localRib().Vrfs[peerVrf] if table.CanImportToVrf(vrf, path) { path = path.ToLocal() } else { + pathExportSkipped(r, path, "unimportable-to-vrf") return nil, nil, true } } // replace-peer-as handling - peer.fsm.lock.RLock() - if path != nil && !path.IsWithdraw && peer.fsm.pConf.AsPathOptions.State.ReplacePeerAs { - path = path.ReplaceAS(peer.fsm.pConf.Config.LocalAs, peer.fsm.pConf.Config.PeerAs) + if path != nil && !path.IsWithdraw { + if replacePeerAs, localAs, peerAs := r.replacePeerAS(); replacePeerAs { + path = path.ReplaceAS(localAs, peerAs) + } } - peer.fsm.lock.RUnlock() - if path = filterpath(peer, path, old); path == nil { + if path = filterpath(r, path, old); path == nil { return nil, nil, true } + peerInfo := r.PeerInfo() options := &table.PolicyOptions{ - Info: peer.peerInfo, + Info: peerInfo, } - peer.fsm.lock.RLock() if path.IsLocal() && path.GetNexthop().IsUnspecified() { // We need a special treatment for the locally-originated path // with unspecified nexthop (0.0.0.0 or ::). In this case, the // OldNextHop option should be set to the local address. // Otherwise, we advertise the unspecified nexthop as is when // nexthop-unchanged is configured. - options.OldNextHop = net.ParseIP(peer.fsm.pConf.Transport.State.LocalAddress.String()) + options.OldNextHop = peerInfo.LocalAddress.AsSlice() } else { options.OldNextHop = path.GetNexthop().AsSlice() } - path = table.UpdatePathAttrs(peer.fsm.logger, peer.fsm.gConf, peer.fsm.pConf, peer.peerInfo, path) - peer.fsm.lock.RUnlock() + path = table.UpdatePathAttrs(r.logger(), &s.bgpConfig.Global, peerInfo, path) return path, options, false } -func (s *BgpServer) postFilterpath(peer *peer, path *table.Path) *table.Path { +func filterLlgrPath(peer receiver, path *table.Path) *table.Path { // draft-uttaro-idr-bgp-persistence-02 // 4.3. Processing LLGR_STALE Routes // @@ -688,6 +716,11 @@ func (s *BgpServer) postFilterpath(peer *peer, path *table.Path) *table.Path { // sent the route. path = path.Clone(true) } + return path +} + +func (s *BgpServer) postFilterpath(peer receiver, path *table.Path) *table.Path { + path = filterLlgrPath(peer, path) // remove local-pref attribute // we should do this after applying export policy since policy may @@ -699,17 +732,27 @@ func (s *BgpServer) postFilterpath(peer *peer, path *table.Path) *table.Path { return path } -func (s *BgpServer) filterpath(peer *peer, path, old *table.Path) *table.Path { +func peerPostFilterpath(peer receiver, path *table.Path) *table.Path { + return filterLlgrPath(peer, path) +} + +func (s *BgpServer) filterpath(peer receiver, path, old *table.Path) *table.Path { path, options, stop := s.prePolicyFilterpath(peer, path, old) if stop { return nil } + + prePolicyPath := path options.Validate = s.roaTable.Validate - path = peer.policy.ApplyPolicy(peer.TableID(), table.POLICY_DIRECTION_EXPORT, path, options) + path = peer.routingPolicy().ApplyPolicy(peer.PolicyID(), table.POLICY_DIRECTION_EXPORT, path, options) + if path == nil { + pathExportSkipped(peer, prePolicyPath, "policy") + } + // When 'path' is filtered (path == nil), check 'old' has been sent to this peer. // If it has, send withdrawal to the peer. if path == nil && old != nil { - o := peer.policy.ApplyPolicy(peer.TableID(), table.POLICY_DIRECTION_EXPORT, old, options) + o := peer.routingPolicy().ApplyPolicy(peer.PolicyID(), table.POLICY_DIRECTION_EXPORT, old, options) if o != nil { path = old.Clone(true) } @@ -979,19 +1022,19 @@ func (s *BgpServer) notifyRecvMessageWatcher(peer *peer, timestamp time.Time, ms s.notifyMessageWatcher(peer, timestamp, msg, false) } -func (s *BgpServer) getPossibleBest(peer *peer, family bgp.Family) []*table.Path { +func (s *BgpServer) getPossibleBest(peer receiver, family bgp.Family) []*table.Path { if peer.isAddPathSendEnabled(family) { - return peer.localRib.GetPathList(peer.TableID(), peer.AS(), []bgp.Family{family}) + return peer.localRib().GetPathList(peer.TableID(), peer.AS(), []bgp.Family{family}) } - return peer.localRib.GetBestPathList(peer.TableID(), peer.AS(), []bgp.Family{family}) + return peer.localRib().GetBestPathList(peer.TableID(), peer.AS(), []bgp.Family{family}) } -func (s *BgpServer) getBestFromLocal(peer *peer, rfList []bgp.Family) ([]*table.Path, []*table.Path) { +func (s *BgpServer) getBestFromLocal(peer receiver, rfList []bgp.Family) ([]*table.Path, []*table.Path) { pathList := []*table.Path{} filtered := []*table.Path{} if peer.isSecondaryRouteEnabled() { - for _, family := range peer.toGlobalFamilies(rfList) { + for _, family := range toGlobalFamilies(peer, rfList) { dsts := s.rsRib.Tables[family].GetDestinations() dl := make([]*table.Update, 0, len(dsts)) for _, d := range dsts { @@ -1008,7 +1051,7 @@ func (s *BgpServer) getBestFromLocal(peer *peer, rfList []bgp.Family) ([]*table. return pathList, filtered } - for _, family := range peer.toGlobalFamilies(rfList) { + for _, family := range toGlobalFamilies(peer, rfList) { for _, path := range s.getPossibleBest(peer, family) { if p := s.filterpath(peer, path, nil); p != nil { pathList = append(pathList, p) @@ -1031,29 +1074,8 @@ func (s *BgpServer) getBestFromLocal(peer *peer, rfList []bgp.Family) ([]*table. return pathList, filtered } -func needToAdvertise(peer *peer) bool { - peer.fsm.lock.RLock() - notEstablished := peer.fsm.state != bgp.BGP_FSM_ESTABLISHED - localRestarting := peer.fsm.pConf.GracefulRestart.State.LocalRestarting - peer.fsm.lock.RUnlock() - if notEstablished { - return false - } - if localRestarting { - peer.fsm.lock.RLock() - peer.fsm.logger.Debug("now syncing, suppress sending updates", - log.Fields{ - "Topic": "Peer", - "Key": peer.fsm.pConf.State.NeighborAddress, - }) - peer.fsm.lock.RUnlock() - return false - } - return true -} - -func (s *BgpServer) sendSecondaryRoutes(peer *peer, newPath *table.Path, dsts []*table.Update) []*table.Path { - if !needToAdvertise(peer) { +func (s *BgpServer) sendSecondaryRoutes(peer receiver, newPath *table.Path, dsts []*table.Update) []*table.Path { + if !peer.needToAdvertise() { return nil } pl := make([]*table.Path, 0, len(dsts)) @@ -1063,12 +1085,15 @@ func (s *BgpServer) sendSecondaryRoutes(peer *peer, newPath *table.Path, dsts [] if stop { return nil } + + prePolicyPath := path options.Validate = s.roaTable.Validate - path = peer.policy.ApplyPolicy(peer.TableID(), table.POLICY_DIRECTION_EXPORT, path, options) - if path != nil { - return s.postFilterpath(peer, path) + path = peer.routingPolicy().ApplyPolicy(peer.PolicyID(), table.POLICY_DIRECTION_EXPORT, path, options) + if path == nil { + pathExportSkipped(peer, prePolicyPath, "secondary-route-policy") + return nil } - return nil + return s.postFilterpath(peer, path) } for _, dst := range dsts { @@ -1099,12 +1124,14 @@ func (s *BgpServer) sendSecondaryRoutes(peer *peer, newPath *table.Path, dsts [] return pl } -func (s *BgpServer) processOutgoingPaths(peer *peer, paths, olds []*table.Path) []*table.Path { - if !needToAdvertise(peer) { - return nil +func (s *BgpServer) processOutgoingPaths(peer receiver, paths, olds []*table.Path) ([]*table.Path, []*table.Path) { + if !peer.needToAdvertise() { + return nil, nil } outgoing := make([]*table.Path, 0, len(paths)) + filteredOlds := make([]*table.Path, 0, len(paths)) + for idx, path := range paths { var old *table.Path if olds != nil { @@ -1112,9 +1139,10 @@ func (s *BgpServer) processOutgoingPaths(peer *peer, paths, olds []*table.Path) } if p := s.filterpath(peer, path, old); p != nil { outgoing = append(outgoing, p) + filteredOlds = append(filteredOlds, old) } } - return outgoing + return outgoing, filteredOlds } func (s *BgpServer) handleRouteRefresh(peer *peer, e *fsmMsg) []*table.Path { @@ -1152,26 +1180,22 @@ func (s *BgpServer) handleRouteRefresh(peer *peer, e *fsmMsg) []*table.Path { } func (s *BgpServer) propagateUpdate(peer *peer, pathList []*table.Path) { - rs := peer != nil && peer.isRouteServerClient() - vrf := false - if peer != nil { - peer.fsm.lock.RLock() - vrf = !rs && peer.fsm.pConf.Config.Vrf != "" - peer.fsm.lock.RUnlock() - } - - tableId := table.GLOBAL_RIB_NAME + var rs, vrf bool + policyId := table.GLOBAL_RIB_NAME rib := s.globalRib - if rs { - tableId = peer.TableID() - rib = s.rsRib + + if peer != nil { + policyId = peer.PolicyID() + if peer.isRouteServerClient() { + rs, rib = true, s.rsRib + } + vrf = !rs && peer.vrf() != "" } for _, path := range pathList { if vrf { - peer.fsm.lock.RLock() - peerVrf := peer.fsm.pConf.Config.Vrf - peer.fsm.lock.RUnlock() + peerVrf := peer.vrf() + path = path.ToGlobal(rib.Vrfs[peerVrf]) if s.zclient != nil { s.zclient.pathVrfMap[path] = rib.Vrfs[peerVrf].Id @@ -1186,7 +1210,7 @@ func (s *BgpServer) propagateUpdate(peer *peer, pathList []*table.Path) { policyOptions.Info = peer.peerInfo } - if p := s.policy.ApplyPolicy(tableId, table.POLICY_DIRECTION_IMPORT, path, policyOptions); p != nil { + if p := s.policy.ApplyPolicy(policyId, table.POLICY_DIRECTION_IMPORT, path, policyOptions); p != nil { path = p } else { path = path.Clone(true) @@ -1256,10 +1280,10 @@ func (s *BgpServer) propagateUpdate(peer *peer, pathList []*table.Path) { if path.IsWithdraw { // Skips filtering because the paths are already filtered // and the withdrawal does not need the path attributes. - sendfsmOutgoingMsg(peer, paths) + peer.send(paths, nil) } else if !peer.getRtcEORWait() { - paths = s.processOutgoingPaths(peer, paths, nil) - sendfsmOutgoingMsg(peer, paths) + paths, olds := s.processOutgoingPaths(peer, paths, nil) + peer.send(paths, olds) } else { s.logger.Debug("Nothing sent in response to RT received. Waiting for RTC EOR.", log.Fields{ @@ -1304,14 +1328,14 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * s.notifyBestWatcher(gBestList, mpathList) } family := newPath.GetFamily() - for _, targetPeer := range s.neighborMap { - if source == nil && targetPeer.isRouteServerClient() || source != nil && source.isRouteServerClient() != targetPeer.isRouteServerClient() { + + for _, targetPeer := range s.receiverMap { + if (source == nil && targetPeer.isRouteServerClient()) || + (source != nil && source.isRouteServerClient() != targetPeer.isRouteServerClient()) { continue } f := func() bgp.Family { - targetPeer.fsm.lock.RLock() - peerVrf := targetPeer.fsm.pConf.Config.Vrf - targetPeer.fsm.lock.RUnlock() + peerVrf := targetPeer.vrf() if peerVrf != "" { switch family { case bgp.RF_IPv4_VPN: @@ -1402,7 +1426,7 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * } } else { bestList = []*table.Path{} - targetPeer.sendMaxPathFiltered[newPath.GetLocalKey()] = struct{}{} + targetPeer.setPathSendMaxFiltered(newPath) s.logger.Warn("exceeding max routes for prefix", log.Fields{ "Topic": "Peer", @@ -1411,14 +1435,14 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * }) } } - if needToAdvertise(targetPeer) && len(bestList) > 0 { - sendfsmOutgoingMsg(targetPeer, bestList) + if targetPeer.needToAdvertise() && len(bestList) > 0 { + targetPeer.send(bestList, nil) } } else { if targetPeer.isRouteServerClient() { if targetPeer.isSecondaryRouteEnabled() { if paths := s.sendSecondaryRoutes(targetPeer, newPath, dsts); len(paths) > 0 { - sendfsmOutgoingMsg(targetPeer, paths) + targetPeer.send(paths, nil) } continue } @@ -1430,8 +1454,8 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * if !needOld { oldList = nil } - if paths := s.processOutgoingPaths(targetPeer, bestList, oldList); len(paths) > 0 { - sendfsmOutgoingMsg(targetPeer, paths) + if paths, olds := s.processOutgoingPaths(targetPeer, bestList, oldList); len(paths) > 0 { + targetPeer.send(paths, olds) } } } @@ -1439,7 +1463,20 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * func (s *BgpServer) stopNeighbor(peer *peer, oldState bgp.FSMState, e *fsmMsg) { peer.stopPeerRestarting() - delete(s.neighborMap, peer.ID()) + + peer.fsm.lock.RLock() + addr := peer.fsm.pConf.State.NeighborAddress.String() + pgName := peer.fsm.pConf.Config.PeerGroup + peer.fsm.lock.RUnlock() + + delete(s.neighborMap, addr) + delete(s.receiverMap, addr) + if pgName != "" { + pg, hasPg := s.peerGroupMap[pgName] + if hasPg { + delete(pg.neighborMap, addr) + } + } peer.stopFSMHandler() s.broadcastPeerState(peer, oldState, e) } @@ -1673,7 +1710,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) { } if len(pathList) > 0 { - sendfsmOutgoingMsg(peer, pathList) + peer.send(pathList, nil) } } else { // RFC 4724 4.1 @@ -1693,24 +1730,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) { return true }() if allEnd { - for _, p := range s.neighborMap { - p.fsm.lock.Lock() - peerLocalRestarting := p.fsm.pConf.GracefulRestart.State.LocalRestarting - p.fsm.pConf.GracefulRestart.State.LocalRestarting = false - p.fsm.lock.Unlock() - if !p.isGracefulRestartEnabled() && !peerLocalRestarting { - continue - } - paths, _ := s.getBestFromLocal(p, p.configuredRFlist()) - if len(paths) > 0 { - sendfsmOutgoingMsg(p, paths) - } - } - s.logger.Info("sync finished", - log.Fields{ - "Topic": "Server", - "Key": peer.ID(), - }) + s.syncAllPeers() } else { peer.fsm.lock.RLock() deferral := peer.fsm.pConf.GracefulRestart.Config.DeferralTime @@ -1751,7 +1771,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) { return } if paths := s.handleRouteRefresh(peer, e); len(paths) > 0 { - sendfsmOutgoingMsg(peer, paths) + peer.send(paths, nil) return } case fsmMsgBGPMessage: @@ -1826,23 +1846,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) { return true }() if allEnd { - for _, p := range s.neighborMap { - p.fsm.lock.Lock() - peerLocalRestarting := p.fsm.pConf.GracefulRestart.State.LocalRestarting - p.fsm.pConf.GracefulRestart.State.LocalRestarting = false - p.fsm.lock.Unlock() - if !p.isGracefulRestartEnabled() && !peerLocalRestarting { - continue - } - paths, _ := s.getBestFromLocal(p, p.negotiatedRFList()) - if len(paths) > 0 { - sendfsmOutgoingMsg(p, paths) - } - } - s.logger.Info("sync finished", - log.Fields{ - "Topic": "Server", - }) + s.syncAllPeers() } // we don't delay non-route-target NLRIs when local-restarting @@ -1885,7 +1889,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) { } } if paths, _ := s.getBestFromLocal(peer, families); len(paths) > 0 { - sendfsmOutgoingMsg(peer, paths) + peer.send(paths, nil) } } } @@ -2076,25 +2080,24 @@ func (s *BgpServer) SetPolicies(ctx context.Context, r *api.SetPoliciesRequest) } return s.mgmtOperation(func() error { - ap := make(map[string]oc.ApplyPolicy, len(s.neighborMap)+1) + ap := make(map[string]oc.ApplyPolicy, len(s.receiverMap)+1) a, err := getConfig(table.GLOBAL_RIB_NAME) if err != nil { return err } ap[table.GLOBAL_RIB_NAME] = *a - for _, peer := range s.neighborMap { - peer.fsm.lock.RLock() + for _, r := range s.receiverMap { + id := r.ID() s.logger.Info("call set policy", log.Fields{ "Topic": "Peer", - "Key": peer.fsm.pConf.State.NeighborAddress, - }) - peer.fsm.lock.RUnlock() - a, err := getConfig(peer.ID()) + "Key": id}) + + a, err := getConfig(id) if err != nil { return err } - ap[peer.ID()] = *a + ap[id] = *a } return s.policy.Reset(rp, ap) }, false) @@ -2665,7 +2668,7 @@ func (s *BgpServer) softResetOut(addr string, family bgp.Family, deferral bool) peer.fsm.lock.Lock() peer.fsm.pConf.GracefulRestart.State.LocalRestarting = false peer.fsm.lock.Unlock() - s.logger.Debug("deferral timer expired", + s.logger.Info("deferral timer expired", log.Fields{ "Topic": "Peer", "Key": peer.ID(), @@ -2697,7 +2700,7 @@ func (s *BgpServer) softResetOut(addr string, family bgp.Family, deferral bool) return l }() } - sendfsmOutgoingMsg(peer, pathList) + peer.send(pathList, nil) } } return nil @@ -2734,6 +2737,30 @@ func (s *BgpServer) sReset(addr string, family bgp.Family) error { return s.softResetOut(addr, family, false) } +func (s *BgpServer) syncAllPeers() { + s.logger.Info("all EORs are received, syncing all peers", + log.Fields{"Topic": "Server"}) + for _, p := range s.receiverMap { + // NOTE: need to reset local restarting even for peers that not yet + // connected too to avoid for them to wait sync + isGREnabled := p.isGracefulRestartEnabled() + wasLocalRestarting := p.resetLocalRestarting() + if !isGREnabled && !wasLocalRestarting { + continue + } + + // NOTE: use configured list here for simplicity, even for non-peer-group receivers + // Paths will still be filtered by filterpath, so we do not send extra + paths, _ := s.getBestFromLocal(p, p.configuredRFlist()) + if len(paths) > 0 { + p.send(paths, nil) + } + } + + s.logger.Info("sync finished", + log.Fields{"Topic": "Server"}) +} + func (s *BgpServer) validateTable(r *table.Table) (v map[*table.Path]*table.Validation) { if s.roaManager.enabled() { v = make(map[*table.Path]*table.Validation, len(r.GetDestinations())) @@ -2829,7 +2856,7 @@ func (s *BgpServer) getAdjRib(addr string, family bgp.Family, in bool, enableFil options := &table.PolicyOptions{ Validate: s.roaTable.Validate, } - p := s.policy.ApplyPolicy(peer.TableID(), table.POLICY_DIRECTION_IMPORT, path, options) + p := s.policy.ApplyPolicy(peer.PolicyID(), table.POLICY_DIRECTION_IMPORT, path, options) if p == nil { filtered[pathLocalKey] = table.PolicyFiltered } else { @@ -3238,7 +3265,25 @@ func (s *BgpServer) addPeerGroup(c *oc.PeerGroup) error { "Name": name, }) - s.peerGroupMap[c.Config.PeerGroupName] = newPeerGroup(c) + rib := s.globalRib + if c.RouteServer.Config.RouteServerClient { + rib = s.rsRib + } + + peerGroup, err := newPeerGroup(s.logger, s.policy, &s.bgpConfig.Global, c, rib) + if err != nil { + return fmt.Errorf("can't configure peer-group: %s, error: %w", name, err) + } + + s.peerGroupMap[c.Config.PeerGroupName] = peerGroup + s.toggleReceiver(c.Config.SharedPolicy, peerGroup) + if c.Config.SharedPolicy { + // NOTE: even if we set policy for pg:name in assignment map in import, + // we would never use it as pg:name policies are only checked on export + if err := s.policy.SetPeerPolicy(peerGroup.PolicyID(), c.ApplyPolicy); err != nil { + return err + } + } return nil } @@ -3310,13 +3355,17 @@ func (s *BgpServer) addNeighbor(c *oc.Neighbor) error { if c.RouteServer.Config.RouteServerClient { rib = s.rsRib } - peer := newPeer(&s.bgpConfig.Global, c, rib, s.policy, s.logger) + peer := newPeer(&s.bgpConfig.Global, c, pgConf, rib, s.policy, s.logger) if err := s.policy.SetPeerPolicy(peer.ID(), c.ApplyPolicy); err != nil { return fmt.Errorf("failed to set peer policy for %s: %v", addr, err) } s.neighborMap[addr] = peer if name := c.Config.PeerGroup; name != "" { - s.peerGroupMap[name].AddMember(*c) + peerGroup := s.peerGroupMap[name] + peerGroup.AddMember(*c, peer) + s.toggleReceiver(!pgConf.Config.SharedPolicy, peer) + } else { + s.toggleReceiver(true, peer) } s.startFsmHandler(peer) s.broadcastPeerState(peer, bgp.BGP_FSM_IDLE, nil) @@ -3409,13 +3458,8 @@ func (s *BgpServer) deletePeerGroup(name string) error { } func (s *BgpServer) deleteNeighbor(c *oc.Neighbor, code, subcode uint8, sendNotification bool) error { - if c.Config.PeerGroup != "" { - _, y := s.peerGroupMap[c.Config.PeerGroup] - if y { - s.peerGroupMap[c.Config.PeerGroup].DeleteMember(*c) - } - } - + // Note: deleteNeighbor() callers provide only skeleteon neighbor config needed + // to find an address. Read the rest from *peer structure if needed addr, err := c.ExtractNeighborAddress() if err != nil { return err @@ -3453,6 +3497,21 @@ func (s *BgpServer) deleteNeighbor(c *oc.Neighbor, code, subcode uint8, sendNoti if sendNotification { n.fsm.deconfiguredNotification <- bgp.NewBGPNotificationMessage(code, subcode, nil) } + + delete(s.neighborMap, addr) + delete(s.receiverMap, addr) + if pgName := n.PeerGroup(); pgName != "" { + pg, y := s.peerGroupMap[pgName] + if y { + pg.DeleteMember(addr) + } else { + s.logger.Warn("Unexpected peer without an actual peer group", + log.Fields{ + "Topic": "Peer", + "Key": addr}) + } + } + s.propagateUpdate(n, n.DropAll(n.configuredRFlist())) s.stopNeighbor(n, -1, nil) return nil @@ -3526,14 +3585,32 @@ func (s *BgpServer) DeleteDynamicNeighbor(ctx context.Context, r *api.DeleteDyna }, true) } +func (s *BgpServer) toggleReceiver(enabled bool, r receiver) { + if enabled { + s.receiverMap[r.ID()] = r + } else { + delete(s.receiverMap, r.ID()) + } +} + func (s *BgpServer) updatePeerGroup(pg *oc.PeerGroup) (needsSoftResetIn bool, err error) { name := pg.Config.PeerGroupName - _, ok := s.peerGroupMap[name] + peerGroup, ok := s.peerGroupMap[name] if !ok { return false, fmt.Errorf("peer-group %s doesn't exist", name) } - s.peerGroupMap[name].Conf = pg + + if err := peerGroup.Update(&s.bgpConfig.Global, pg); err != nil { + return false, fmt.Errorf("cannot reconfigure peer-group %s: %w", name, err) + } + + s.toggleReceiver(pg.Config.SharedPolicy, peerGroup) + if pg.Config.SharedPolicy { + if err := s.policy.SetPeerPolicy(peerGroup.ID(), pg.ApplyPolicy); err != nil { + return false, fmt.Errorf("cannot set shared policy for peer-group %s: %w", name, err) + } + } for _, n := range s.peerGroupMap[name].members { u, err := s.updateNeighbor(&n) @@ -3598,6 +3675,10 @@ func (s *BgpServer) updateNeighbor(c *oc.Neighbor) (needsSoftResetIn bool, err e peer.fsm.pConf.ApplyPolicy = c.ApplyPolicy needsSoftResetIn = true } + if pgConf != nil { + s.toggleReceiver(!pgConf.Config.SharedPolicy, peer) + } + original := peer.fsm.pConf if !original.AsPathOptions.Config.Equal(&c.AsPathOptions.Config) { @@ -4124,12 +4205,13 @@ func (s *BgpServer) toPolicyInfo(name string, dir api.PolicyDirection) (string, if name == table.GLOBAL_RIB_NAME { name = table.GLOBAL_RIB_NAME } else { - peer, ok := s.neighborMap[name] + peer, ok := s.receiverMap[name] if !ok { return "", table.POLICY_DIRECTION_NONE, fmt.Errorf("not found peer %s", name) } - if !peer.isRouteServerClient() { - return "", table.POLICY_DIRECTION_NONE, fmt.Errorf("non-rs-client peer %s doesn't have per peer policy", name) + if peer.PolicyID() == table.GLOBAL_RIB_NAME { + return "", table.POLICY_DIRECTION_NONE, fmt.Errorf("peer %s doesn't have per peer policy"+ + " (non-rs-client or not a peer-group with shared policy)", name) } name = peer.ID() } diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 6fdac4da8..a9e979515 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -225,10 +225,11 @@ func TestListPolicyAssignment(t *testing.T) { } //nolint:errcheck // WatchEvent won't return an error here -func waitState(s *BgpServer, state api.PeerState_SessionState, expectedFamilies ...bgp.Family) *sync.WaitGroup { +func waitStateMultiple(s *BgpServer, state api.PeerState_SessionState, expectedCount int, expectedFamilies ...bgp.Family) *sync.WaitGroup { + var count int wg := &sync.WaitGroup{} watchCtxMsg, watchCancelMsg := context.WithCancel(context.Background()) - wg.Add(1) + wg.Add(expectedCount) opts := make([]WatchOption, 0) opts = append(opts, WatchPeer()) @@ -254,7 +255,11 @@ func waitState(s *BgpServer, state api.PeerState_SessionState, expectedFamilies return } } - watchCancelMsg() + + count++ + if count == expectedCount { + watchCancelMsg() + } wg.Done() } }, @@ -263,12 +268,16 @@ func waitState(s *BgpServer, state api.PeerState_SessionState, expectedFamilies return wg } +func waitState(s *BgpServer, state api.PeerState_SessionState) *sync.WaitGroup { + return waitStateMultiple(s, state, 1) +} + func waitActive(s *BgpServer) *sync.WaitGroup { return waitState(s, api.PeerState_SESSION_STATE_ACTIVE) } func waitEstablished(s *BgpServer, rfs ...bgp.Family) *sync.WaitGroup { - return waitState(s, api.PeerState_SESSION_STATE_ESTABLISHED, rfs...) + return waitStateMultiple(s, api.PeerState_SESSION_STATE_ESTABLISHED, 1, rfs...) } func TestListPathEnableFiltered(test *testing.T) { @@ -976,6 +985,7 @@ func newPeerandInfo(t *testing.T, myAs, as uint32, address string, rib *table.Ta p := newPeer( &oc.Global{Config: oc.GlobalConfig{As: myAs}}, nConf, + nil, rib, policy, logger) @@ -1073,6 +1083,86 @@ func TestFilterpathWithiBGP(t *testing.T) { assert.Nil(t, path) } +func TestFilterPathsForPeer(t *testing.T) { + const as = uint32(65000) + + rib := table.NewTableManager(logger, []bgp.Family{bgp.RF_IPv4_UC}) + p1 := newPeerandInfo(t, as, as, "192.168.0.1", rib) + p1.fsm.pConf.RouteReflector.Config.RouteReflectorClient = true + p2 := newPeerandInfo(t, as, as, "192.168.0.2", rib) + p2.fsm.pConf.RouteReflector.Config.RouteReflectorClient = true + + nlri1, _ := bgp.NewIPAddrPrefix(netip.MustParsePrefix("10.10.10.0/24")) + nlri2, _ := bgp.NewIPAddrPrefix(netip.MustParsePrefix("10.20.20.0/24")) + pattr := []bgp.PathAttributeInterface{ + bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{bgp.NewAs4PathParam(2, []uint32{as})}), + bgp.NewPathAttributeLocalPref(200), + } + + path1from1 := table.NewPath(bgp.RF_IPv4_UC, p1.peerInfo, bgp.PathNLRI{NLRI: nlri1}, + false, pattr, time.Now(), false) + path2from1 := table.NewPath(bgp.RF_IPv4_UC, p1.peerInfo, bgp.PathNLRI{NLRI: nlri2}, + false, pattr, time.Now(), false) + path1from2 := table.NewPath(bgp.RF_IPv4_UC, p2.peerInfo, bgp.PathNLRI{NLRI: nlri1}, + false, pattr, time.Now(), false) + + t.Run("SendAll", func(t *testing.T) { + // Send both paths as is since they originated from other peer + twoPaths := []*table.Path{path1from1, path2from1} + filtered := filterPathsForPeer(p2, twoPaths, twoPaths) + require.Len(t, filtered, 2) + assert.False(t, filtered[0].IsWithdraw) + assert.False(t, filtered[1].IsWithdraw) + }) + + t.Run("Filter", func(t *testing.T) { + // Do not send path 1 since it was originated from second peer, but send another path + // Test both orders + twoPaths := []*table.Path{path1from2, path2from1} + for _, reversed := range []bool{false, true} { + t.Run(fmt.Sprintf("reversed=%t", reversed), func(t *testing.T) { + if reversed { + slices.Reverse(twoPaths) + } + + filtered := filterPathsForPeer(p2, twoPaths, twoPaths) + require.Len(t, filtered, 1) + assert.Equal(t, nlri2.String(), filtered[0].GetNlri().String()) + assert.False(t, filtered[0].IsWithdraw) + }) + } + }) + + t.Run("WithdrawOld", func(t *testing.T) { + // If path1 was sent from peer1, but changed to peer2, we should + // explicitly withdraw it because it's our last chance to do so + // (all subsequent updates will be filtered, see above) + oldPaths := []*table.Path{path1from1, path2from1} + twoPaths := []*table.Path{path1from2, path2from1} + t.Run("one", func(t *testing.T) { + filtered := filterPathsForPeer(p2, twoPaths[:1], oldPaths[:1]) + require.Len(t, filtered, 1) + assert.True(t, filtered[0].IsWithdraw) + }) + + for _, reversed := range []bool{false, true} { + t.Run(fmt.Sprintf("reversed=%t", reversed), func(t *testing.T) { + withdrawnIndex, retainedIndex := 0, 1 + if reversed { + slices.Reverse(oldPaths) + slices.Reverse(twoPaths) + withdrawnIndex, retainedIndex = 1, 0 + } + + filtered := filterPathsForPeer(p2, twoPaths, oldPaths) + require.Len(t, filtered, 2) + assert.True(t, filtered[withdrawnIndex].IsWithdraw) + assert.False(t, filtered[retainedIndex].IsWithdraw) + }) + } + }) +} + func TestFilterpathWithRejectPolicy(t *testing.T) { rib1 := table.NewTableManager(logger, []bgp.Family{bgp.RF_IPv4_UC}) p1 := newPeerandInfo(t, 1, 2, "192.168.0.1", rib1) @@ -1135,6 +1225,185 @@ func TestFilterpathWithRejectPolicy(t *testing.T) { } } +func TestBgpServerLocalASOverride(test *testing.T) { + // Test that per-peer local-as override is working properly when applied to path + // for filtering on receiver or for AS_PATH prepending on sender + const ( + localAsn = 11 + localAsnOverride = 5 + middleAsn = 6 + extAsn = 7 + ) + + remote := runNewServer(test, extAsn, "1.1.1.1", 10179) + defer remote.StopBgp(context.Background(), &api.StopBgpRequest{}) + remote.logger.SetLevel(log.DebugLevel) + + local := runNewServer(test, localAsn, "2.2.2.2", 20179) + defer local.StopBgp(context.Background(), &api.StopBgpRequest{}) + local.logger.SetLevel(log.DebugLevel) + + var peeringEstablished bool + servers := []*BgpServer{remote, local} + afiSafis := []oc.AfiSafiType{ + oc.AFI_SAFI_TYPE_IPV4_UNICAST, + } + + // Only used in peer-group tests + const pgName = "g" + err := local.addPeerGroup(&oc.PeerGroup{ + Config: oc.PeerGroupConfig{ + PeerAs: extAsn, + LocalAs: localAsnOverride, + PeerGroupName: pgName, + }, + Transport: oc.Transport{ + Config: oc.TransportConfig{RemotePort: 10179}, + }, + }) + assert.NoError(test, err) + + // Path attributes commonly used by all tests + origin := bgp.NewPathAttributeOrigin(0) + nh, _ := bgp.NewPathAttributeNextHop(netip.MustParseAddr("10.0.0.1")) + + for i, tt := range []struct { + name string + sender, receiver *BgpServer + asPathSent []uint32 + asPathAccepted string + peerOption peerOption + }{ + // NOTE: tests without confOverride go first to avoid reconfiguring peers too often + // (they do not force reconfiguring peers, so be careful adding such a test in a middle) + { + // NOTE: for now such paths are accepted, but might be rejected in future + name: "local asn override received", + sender: remote, + receiver: local, + asPathSent: []uint32{middleAsn, localAsnOverride}, + asPathAccepted: fmt.Sprint(extAsn, middleAsn, localAsnOverride), + }, + { + name: "local asn dropped", + sender: remote, + receiver: local, + asPathSent: []uint32{middleAsn, localAsn}, + }, + { + name: "local asn override prepended", + sender: local, + receiver: remote, + asPathSent: nil, + asPathAccepted: strconv.Itoa(localAsnOverride), + }, + { + name: "local asn override prepended peer-group", + sender: local, + receiver: remote, + asPathSent: nil, + asPathAccepted: strconv.Itoa(localAsnOverride), + peerOption: func(peer *BgpServer, g *oc.Global, n *oc.Neighbor) { + if peer == remote { + n.Config.LocalAs = 0 + n.Config.PeerGroup = pgName + } + }, + }, + { + name: "local asn allow-own-as", + sender: remote, + receiver: local, + asPathSent: []uint32{middleAsn, localAsn}, + asPathAccepted: fmt.Sprint(extAsn, middleAsn, localAsn), + peerOption: func(peer *BgpServer, g *oc.Global, n *oc.Neighbor) { + if peer == remote { + n.AsPathOptions.Config.AllowOwnAs = 1 + } + }, + }, + { + name: "replace-private-as", + sender: local, + receiver: remote, + asPathSent: []uint32{65414, 65413}, + asPathAccepted: fmt.Sprint(localAsnOverride, localAsnOverride, localAsnOverride), + peerOption: func(peer *BgpServer, g *oc.Global, n *oc.Neighbor) { + if peer == remote { + n.Config.RemovePrivateAs = oc.REMOVE_PRIVATE_AS_OPTION_REPLACE + } + }, + }, + } { + test.Run(tt.name, func(test *testing.T) { + if tt.peerOption != nil || !peeringEstablished { + if peeringEstablished { + resetPeers(test, context.Background(), servers) + } + + wg := waitEstablished(remote) + err := peerServers(test, context.Background(), servers, afiSafis, + enableGROpt, func(peer *BgpServer, g *oc.Global, p *oc.Neighbor) { + switch peer { + case remote: + p.Config.LocalAs = localAsnOverride + case local: + p.Config.PeerAs = localAsnOverride + } + + // For the purpose of this test, sender should send it's route even if + // it might lead to AS path loop (but it might be ignored by receiver) + p.AsPathOptions.Config.AllowAsPathLoopLocal = true + + if tt.peerOption != nil { + tt.peerOption(peer, g, p) + } + }) + require.NoError(test, err) + + wg.Wait() + peeringEstablished = true + } + + asPath := bgp.NewPathAttributeAsPath([]bgp.AsPathParamInterface{ + bgp.NewAs4PathParam(bgp.BGP_ASPATH_ATTR_TYPE_SEQ, tt.asPathSent), + }) + + prefix := fmt.Sprintf("10.5.%d.0", i) + prefixStr := prefix + "/24" + nlri, _ := bgp.NewIPAddrPrefix(netip.MustParsePrefix(prefixStr)) + paths := []*apiutil.Path{ + { + Family: bgp.RF_IPv4_UC, + Nlri: nlri, + Attrs: []bgp.PathAttributeInterface{origin, nh, asPath}, + }, + } + + watcher := tt.receiver.watch(WatchUpdate(false, "", "")) + _, err := tt.sender.AddPath(apiutil.AddPathRequest{Paths: paths}) + require.NoError(test, err) + + defer tt.sender.DeletePath(apiutil.DeletePathRequest{Paths: paths}) + + if tt.asPathAccepted != "" { + acceptedPath := waitOnePath(test, watcher) + assert.Equal(test, prefixStr, acceptedPath.GetNlri().String()) + assert.Equal(test, tt.asPathAccepted, acceptedPath.GetAsString()) + } else { + time.Sleep(1 * time.Second) + err := tt.receiver.ListPath(apiutil.ListPathRequest{ + TableType: api.TableType_TABLE_TYPE_GLOBAL, + Family: bgp.RF_IPv4_UC, + }, func(prefix bgp.NLRI, paths []*apiutil.Path) { + assert.NotEqual(test, prefixStr, prefix.String()) + }) + assert.NoError(test, err) + } + }) + } +} + func TestPeerGroup(test *testing.T) { assert := assert.New(test) s := NewBgpServer() @@ -1221,6 +1490,13 @@ func TestPeerGroup(test *testing.T) { assert.NoError(err) establishedWg.Wait() + + activeWg := waitActive(s) + err = t.DeletePeer(context.Background(), &api.DeletePeerRequest{ + Address: m.Config.NeighborAddress.String(), + }) + assert.NoError(err) + activeWg.Wait() } func TestDynamicNeighbor(t *testing.T) { @@ -1596,6 +1872,10 @@ func runNewServer(t *testing.T, as uint32, routerID string, listenPort int32) *B type peerOption func(peer *BgpServer, g *oc.Global, p *oc.Neighbor) +func enableGROpt(peer *BgpServer, g *oc.Global, p *oc.Neighbor) { + p.GracefulRestart.Config.Enabled = true +} + 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] @@ -1650,6 +1930,7 @@ func peerServers(t *testing.T, ctx context.Context, servers []*BgpServer, famili if i == j { continue } + // first server to get neighbor config is passive to hopefully make handshake faster if err := peerTwoServers(t, ctx, server, peer, families, i < j, opts...); err != nil { return err @@ -1660,6 +1941,23 @@ func peerServers(t *testing.T, ctx context.Context, servers []*BgpServer, famili return nil } +func resetPeers(t *testing.T, ctx context.Context, servers []*BgpServer) { + // FIXME: use peer address instead of default as in peerServers + for i, server := range servers { + for j := range servers { + if i == j { + continue + } + } + + if err := server.DeletePeer(ctx, &api.DeletePeerRequest{ + Address: "127.0.0.1", + }); err != nil { + t.Fatal(err) + } + } +} + func parseRDRT(rdStr string) (bgp.RouteDistinguisherInterface, bgp.ExtendedCommunityInterface, error) { rd, err := bgp.ParseRouteDistinguisher(rdStr) if err != nil { @@ -1714,6 +2012,15 @@ func addVrf(t *testing.T, s *BgpServer, vrfName, rdStr string, importRtsStr []st } } +func waitOnePath(test *testing.T, w *watcher) *table.Path { + ev := <-w.Event() + updateEv, isUpdate := ev.(*watchEventUpdate) + require.True(test, isUpdate) + require.Len(test, updateEv.PathList, 1) + + return updateEv.PathList[0] +} + func TestDoNotReactToDuplicateRTCMemberships(t *testing.T) { ctx := context.Background() diff --git a/proto/api/gobgp.proto b/proto/api/gobgp.proto index ada136e07..2671d7fb6 100644 --- a/proto/api/gobgp.proto +++ b/proto/api/gobgp.proto @@ -779,6 +779,7 @@ message PeerGroupConf { bool route_flap_damping = 8; uint32 send_community = 9; bool send_software_version = 10; + bool shared_policy = 11; } message PeerGroupState { diff --git a/test/lib/base.py b/test/lib/base.py index b44d44d43..3071096fd 100644 --- a/test/lib/base.py +++ b/test/lib/base.py @@ -86,7 +86,10 @@ def local(s, capture=False): print('[localhost] local:', s) - _env = {'NOSE_NOLOGCAPTURE': '1' if capture else '0'} + _env = { + 'NOSE_NOLOGCAPTURE': '1' if capture else '0', + 'PATH': os.environ.get('PATH'), + } return subprocess.check_output(s, shell=True, env=_env).decode('utf-8').strip() diff --git a/tools/pyang_plugins/gobgp.yang b/tools/pyang_plugins/gobgp.yang index a214492b3..5297e40e0 100644 --- a/tools/pyang_plugins/gobgp.yang +++ b/tools/pyang_plugins/gobgp.yang @@ -1057,6 +1057,22 @@ module gobgp { } + augment "/bgp:bgp/bgp:peer-groups/bgp:peer-group/bgp:config" { + description "share peer-group policy for all peers"; + + leaf shared-policy { + type boolean; + } + } + + augment "/bgp:bgp/bgp:peer-groups/bgp:peer-group/bgp:state" { + description "share peer-group policy for all peers"; + + leaf shared-policy { + type boolean; + } + } + augment "/rpol:routing-policy/rpol:policy-definitions/" + "rpol:policy-definition/rpol:statements/rpol:statement/" + "rpol:actions/bgp-pol:bgp-actions/bgp-pol:set-as-path-prepend" { From 30af8e4e4f84a4a551b9a5e1ac5512017c3b852f Mon Sep 17 00:00:00 2001 From: Sergey Klyaus Date: Wed, 24 Sep 2025 16:50:37 +0200 Subject: [PATCH 2/2] golang-ci lint fix --- internal/pkg/table/path.go | 3 ++- pkg/server/peer.go | 9 ++++++--- pkg/server/peer_test.go | 2 +- pkg/server/server.go | 15 +++++++++------ 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/internal/pkg/table/path.go b/internal/pkg/table/path.go index 2691c739f..ecfc77494 100644 --- a/internal/pkg/table/path.go +++ b/internal/pkg/table/path.go @@ -336,7 +336,8 @@ func UpdatePathAttrs(logger log.Logger, global *oc.Global, info *PeerInfo, origi "Topic": "Peer", "PeerGroup": info.PeerGroup, "ID": info.ID, - "Type": info.PeerType}) + "Type": info.PeerType, + }) } return path } diff --git a/pkg/server/peer.go b/pkg/server/peer.go index 191a36998..ae69d11ae 100644 --- a/pkg/server/peer.go +++ b/pkg/server/peer.go @@ -614,7 +614,8 @@ func (peer *peer) needToAdvertise() bool { peer.fsm.logger.Debug("now syncing, suppress sending updates", log.Fields{ "Topic": "Peer", - "Key": peer.fsm.pConf.State.NeighborAddress}) + "Key": peer.fsm.pConf.State.NeighborAddress, + }) peer.fsm.lock.RUnlock() return false } @@ -717,7 +718,8 @@ func toGlobalFamilies(peer receiver, families []bgp.Family) []bgp.Family { "Topic": "Peer", "Key": id, "Family": f, - "VRF": vrf}) + "VRF": vrf, + }) } } families = fs @@ -1062,7 +1064,8 @@ func (peer *peer) handleUpdate(e *fsmMsg) ([]*table.Path, []bgp.Family, bool) { log.Fields{ "Topic": "Peer", "Key": peer.fsm.pConf.State.NeighborAddress, - "nlri": path.GetNlri().String()}) + "nlri": path.GetNlri().String(), + }) } } // RFC4456 8. Avoiding Routing Information Loops diff --git a/pkg/server/peer_test.go b/pkg/server/peer_test.go index e43bbb8ac..0a1de1cae 100644 --- a/pkg/server/peer_test.go +++ b/pkg/server/peer_test.go @@ -483,7 +483,7 @@ func countReachPaths(paths []*table.Path) (count int) { count++ } } - return + return count } // Topology test spawns multiple servers and tests various aspects of passing route via diff --git a/pkg/server/server.go b/pkg/server/server.go index 9e436f643..2177e41f6 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -506,7 +506,7 @@ func filterpath(r receiver, path, old *table.Path) *table.Path { } } - //iBGP handling + // iBGP handling if r.isIBGPPeer() { ignore := false if !path.IsLocal() { @@ -616,7 +616,8 @@ func (s *BgpServer) prePolicyFilterpath(r receiver, path, old *table.Path) (*tab log.Fields{ "Topic": "Peer", "Key": r.ID(), - "Path": path}) + "Path": path, + }) pathExportSkipped(r, path, "rtm-already-sent") return nil, nil, true } @@ -1330,8 +1331,8 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * family := newPath.GetFamily() for _, targetPeer := range s.receiverMap { - if (source == nil && targetPeer.isRouteServerClient()) || - (source != nil && source.isRouteServerClient() != targetPeer.isRouteServerClient()) { + if source == nil && targetPeer.isRouteServerClient() || + source != nil && source.isRouteServerClient() != targetPeer.isRouteServerClient() { continue } f := func() bgp.Family { @@ -2091,7 +2092,8 @@ func (s *BgpServer) SetPolicies(ctx context.Context, r *api.SetPoliciesRequest) s.logger.Info("call set policy", log.Fields{ "Topic": "Peer", - "Key": id}) + "Key": id, + }) a, err := getConfig(id) if err != nil { @@ -3508,7 +3510,8 @@ func (s *BgpServer) deleteNeighbor(c *oc.Neighbor, code, subcode uint8, sendNoti s.logger.Warn("Unexpected peer without an actual peer group", log.Fields{ "Topic": "Peer", - "Key": addr}) + "Key": addr, + }) } }