diff --git a/cmd/gobgpd/main.go b/cmd/gobgpd/main.go index 0556c131b..375eac40f 100644 --- a/cmd/gobgpd/main.go +++ b/cmd/gobgpd/main.go @@ -237,17 +237,8 @@ func main() { prometheus.MustRegister(fsmTimingCollector) go bgpServer.Serve() - if opts.UseSdNotify { - if status, err := daemon.SdNotify(false, daemon.SdNotifyReady); !status { - if err != nil { - logger.Warn("Failed to send notification via sd_notify()", slog.String("Error", err.Error())) - } else { - logger.Warn("The socket sd_notify() isn't available") - } - } - } - if opts.ConfigFile == "" { + notifyReady(opts.UseSdNotify) <-sigCh stopServer(bgpServer, opts.UseSdNotify) return @@ -283,6 +274,8 @@ func main() { }) } + notifyReady(opts.UseSdNotify) + for sig := range sigCh { if sig != syscall.SIGHUP { stopServer(bgpServer, opts.UseSdNotify) @@ -305,6 +298,20 @@ func main() { } } +func notifyReady(useSdNotify bool) { + if !useSdNotify { + return + } + + if status, err := daemon.SdNotify(false, daemon.SdNotifyReady); !status { + if err != nil { + logger.Warn("Failed to send notification via sd_notify()", slog.String("Error", err.Error())) + } else { + logger.Warn("The socket sd_notify() isn't available") + } + } +} + func stopServer(bgpServer *server.BgpServer, useSdNotify bool) { logger.Info("stopping gobgpd server") diff --git a/pkg/config/config.go b/pkg/config/config.go index 0be2856e5..efe9e8a9e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -2,6 +2,8 @@ package config import ( "context" + "errors" + "fmt" "log/slog" "github.com/osrg/gobgp/v4/api" @@ -24,6 +26,11 @@ func WatchConfigFile(configFile, configType string, callBack func()) { oc.WatchConfigFile(configFile, configType, callBack) } +const ( + stopOnConfigError = true + continueOnConfigError = false +) + func marshalRouteTargets(l []string) ([]*api.RouteTarget, error) { rtList := make([]*api.RouteTarget, 0, len(l)) for _, rtString := range l { @@ -40,7 +47,7 @@ func marshalRouteTargets(l []string) ([]*api.RouteTarget, error) { return rtList, nil } -func assignGlobalpolicy(ctx context.Context, bgpServer *server.BgpServer, a *oc.ApplyPolicyConfig) { +func assignGlobalpolicy(ctx context.Context, bgpServer *server.BgpServer, a *oc.ApplyPolicyConfig, stopOnError bool) error { toDefaultTable := func(r oc.DefaultPolicyType) table.RouteType { var def table.RouteType switch r { @@ -63,69 +70,82 @@ func assignGlobalpolicy(ctx context.Context, bgpServer *server.BgpServer, a *oc. def := toDefaultTable(a.DefaultImportPolicy) ps := toPolicies(a.ImportPolicyList) - err := bgpServer.SetPolicyAssignment(ctx, &api.SetPolicyAssignmentRequest{ + if err := bgpServer.SetPolicyAssignment(ctx, &api.SetPolicyAssignmentRequest{ Assignment: table.NewAPIPolicyAssignmentFromTableStruct(&table.PolicyAssignment{ Name: table.GLOBAL_RIB_NAME, Type: table.POLICY_DIRECTION_IMPORT, Policies: ps, Default: def, }), - }) - if err != nil { + }); err != nil { bgpServer.Log().Error("failed to set policy assignment", slog.String("Topic", "config"), slog.String("Direction", table.POLICY_DIRECTION_IMPORT.String()), slog.Any("Error", err), ) + if stopOnError { + return fmt.Errorf("failed to set policy assignment: %w", err) + } } def = toDefaultTable(a.DefaultExportPolicy) ps = toPolicies(a.ExportPolicyList) - err = bgpServer.SetPolicyAssignment(ctx, &api.SetPolicyAssignmentRequest{ + if err := bgpServer.SetPolicyAssignment(ctx, &api.SetPolicyAssignmentRequest{ Assignment: table.NewAPIPolicyAssignmentFromTableStruct(&table.PolicyAssignment{ Name: table.GLOBAL_RIB_NAME, Type: table.POLICY_DIRECTION_EXPORT, Policies: ps, Default: def, }), - }) - if err != nil { + }); err != nil { bgpServer.Log().Warn("failed to set policy assignment", slog.String("Topic", "config"), slog.String("Direction", table.POLICY_DIRECTION_EXPORT.String()), slog.Any("Error", err), ) + if stopOnError { + return fmt.Errorf("failed to set policy assignment: %w", err) + } } + + return nil } -func addPeerGroups(ctx context.Context, bgpServer *server.BgpServer, addedPg []oc.PeerGroup) { +func addPeerGroups(ctx context.Context, bgpServer *server.BgpServer, addedPg []oc.PeerGroup, stopOnError bool) error { for _, pg := range addedPg { - bgpServer.Log().Info("Add PeerGroup", + bgpServer.Log().Info("add peer group", slog.String("Topic", "config"), slog.String("Key", pg.Config.PeerGroupName), ) - if err := bgpServer.AddPeerGroup(ctx, &api.AddPeerGroupRequest{ + err := bgpServer.AddPeerGroup(ctx, &api.AddPeerGroupRequest{ PeerGroup: oc.NewPeerGroupFromConfigStruct(&pg), - }); err != nil { - bgpServer.Log().Error("Failed to add PeerGroup", + }) + if err != nil { + bgpServer.Log().Error("failed to add peer group", slog.String("Topic", "config"), slog.String("Key", pg.Config.PeerGroupName), slog.Any("Error", err)) + if stopOnError { + return fmt.Errorf("failed to add peer group: %w", err) + } } } + + return nil } func deletePeerGroups(ctx context.Context, bgpServer *server.BgpServer, deletedPg []oc.PeerGroup) { for _, pg := range deletedPg { - bgpServer.Log().Info("delete PeerGroup", + bgpServer.Log().Info("delete peer group", slog.String("Topic", "config"), slog.String("Key", pg.Config.PeerGroupName), ) - if err := bgpServer.DeletePeerGroup(ctx, &api.DeletePeerGroupRequest{ + err := bgpServer.DeletePeerGroup(ctx, &api.DeletePeerGroupRequest{ Name: pg.Config.PeerGroupName, - }); err != nil { - bgpServer.Log().Error("Failed to delete PeerGroup", + }) + if err != nil { + bgpServer.Log().Warn("failed to delete peer group", slog.String("Topic", "config"), slog.String("Key", pg.Config.PeerGroupName), slog.Any("Error", err), @@ -135,76 +155,92 @@ func deletePeerGroups(ctx context.Context, bgpServer *server.BgpServer, deletedP } func updatePeerGroups(ctx context.Context, bgpServer *server.BgpServer, updatedPg []oc.PeerGroup) bool { + needsSoftResetIn := false for _, pg := range updatedPg { - bgpServer.Log().Info("update PeerGroup", + bgpServer.Log().Info("update peer group", slog.String("Topic", "config"), slog.String("Key", pg.Config.PeerGroupName), ) - if u, err := bgpServer.UpdatePeerGroup(ctx, &api.UpdatePeerGroupRequest{ + u, err := bgpServer.UpdatePeerGroup(ctx, &api.UpdatePeerGroupRequest{ PeerGroup: oc.NewPeerGroupFromConfigStruct(&pg), - }); err != nil { - bgpServer.Log().Error("Failed to update PeerGroup", + }) + if err != nil { + bgpServer.Log().Warn("failed to update peer group", slog.String("Topic", "config"), slog.String("Key", pg.Config.PeerGroupName), slog.Any("Error", err), ) - } else { - return u.NeedsSoftResetIn + continue } + needsSoftResetIn = needsSoftResetIn || u.NeedsSoftResetIn } - return false + + return needsSoftResetIn } -func addDynamicNeighbors(ctx context.Context, bgpServer *server.BgpServer, dynamicNeighbors []oc.DynamicNeighbor) { +func addDynamicNeighbors(ctx context.Context, bgpServer *server.BgpServer, dynamicNeighbors []oc.DynamicNeighbor, stopOnError bool) error { for _, dn := range dynamicNeighbors { - bgpServer.Log().Info("Add Dynamic Neighbor to PeerGroup", + bgpServer.Log().Info("add dynamic neighbor to peer group", slog.String("Topic", "config"), slog.String("Key", dn.Config.PeerGroup), slog.String("Prefix", dn.Config.Prefix.String()), ) - if err := bgpServer.AddDynamicNeighbor(ctx, &api.AddDynamicNeighborRequest{ + err := bgpServer.AddDynamicNeighbor(ctx, &api.AddDynamicNeighborRequest{ DynamicNeighbor: &api.DynamicNeighbor{ Prefix: dn.Config.Prefix.String(), PeerGroup: dn.Config.PeerGroup, }, - }); err != nil { - bgpServer.Log().Error("Failed to add Dynamic Neighbor to PeerGroup", + }) + if err != nil { + bgpServer.Log().Error("failed to add dynamic neighbor to peer group", slog.String("Topic", "config"), slog.String("Key", dn.Config.PeerGroup), slog.String("Prefix", dn.Config.Prefix.String()), slog.Any("Error", err), ) + if stopOnError { + return fmt.Errorf("failed to add dynamic neighbor to peer group: %w", err) + } } } + + return nil } -func addNeighbors(ctx context.Context, bgpServer *server.BgpServer, added []oc.Neighbor) { +func addNeighbors(ctx context.Context, bgpServer *server.BgpServer, added []oc.Neighbor, stopOnError bool) error { for _, p := range added { - bgpServer.Log().Info("Add Peer", + bgpServer.Log().Info("add peer", slog.String("Topic", "config"), slog.String("Key", p.State.NeighborAddress.String()), ) - if err := bgpServer.AddPeer(ctx, &api.AddPeerRequest{ + err := bgpServer.AddPeer(ctx, &api.AddPeerRequest{ Peer: oc.NewPeerFromConfigStruct(&p), - }); err != nil { - bgpServer.Log().Error("Failed to add Peer", + }) + if err != nil { + bgpServer.Log().Error("failed to add peer", slog.String("Topic", "config"), slog.String("Key", p.State.NeighborAddress.String()), slog.Any("Error", err)) + if stopOnError { + return fmt.Errorf("failed to add peer: %w", err) + } } } + + return nil } func deleteNeighbors(ctx context.Context, bgpServer *server.BgpServer, deleted []oc.Neighbor) { for _, p := range deleted { - bgpServer.Log().Info("Delete Peer", + bgpServer.Log().Info("delete peer", slog.String("Topic", "config"), slog.String("Key", p.State.NeighborAddress.String()), ) - if err := bgpServer.DeletePeer(ctx, &api.DeletePeerRequest{ + err := bgpServer.DeletePeer(ctx, &api.DeletePeerRequest{ Address: p.State.NeighborAddress.String(), - }); err != nil { - bgpServer.Log().Error("Failed to delete Peer", + }) + if err != nil { + bgpServer.Log().Warn("failed to delete peer", slog.String("Topic", "config"), slog.String("Key", p.State.NeighborAddress.String()), slog.Any("Error", err), @@ -214,29 +250,33 @@ func deleteNeighbors(ctx context.Context, bgpServer *server.BgpServer, deleted [ } func updateNeighbors(ctx context.Context, bgpServer *server.BgpServer, updated []oc.Neighbor) bool { + needsSoftResetIn := false for _, p := range updated { - bgpServer.Log().Info("Update Peer", + bgpServer.Log().Info("update peer", slog.String("Topic", "config"), slog.String("Key", p.State.NeighborAddress.String())) - if u, err := bgpServer.UpdatePeer(ctx, &api.UpdatePeerRequest{ + u, err := bgpServer.UpdatePeer(ctx, &api.UpdatePeerRequest{ Peer: oc.NewPeerFromConfigStruct(&p), - }); err != nil { - bgpServer.Log().Error("Failed to update Peer", + }) + if err != nil { + bgpServer.Log().Warn("failed to update peer", slog.String("Topic", "config"), slog.String("Key", p.State.NeighborAddress.String()), slog.Any("Error", err), ) - } else { - return u.NeedsSoftResetIn + continue } + needsSoftResetIn = needsSoftResetIn || u.NeedsSoftResetIn } - return false + + return needsSoftResetIn } // InitialConfig applies initial configuration to a pristine gobgp instance. It // can only be called once for an instance. Subsequent changes to the // configuration can be applied using UpdateConfig. The BgpConfigSet can be // obtained by calling ReadConfigFile. If graceful restart behavior is desired, -// pass true for isGracefulRestart. Otherwise, pass false. +// pass true for isGracefulRestart. Otherwise, pass false. Any error applying +// the initial configuration is returned. func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig *oc.BgpConfigSet, isGracefulRestart bool) (*oc.BgpConfigSet, error) { if err := bgpServer.StartBgp(ctx, &api.StartBgpRequest{ Global: oc.NewGlobalFromConfigStruct(&newConfig.Global), @@ -248,7 +288,7 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * tps := newConfig.Zebra.Config.RedistributeRouteTypeList l := make([]string, 0, len(tps)) l = append(l, tps...) - if err := bgpServer.EnableZebra(ctx, &api.EnableZebraRequest{ + err := bgpServer.EnableZebra(ctx, &api.EnableZebraRequest{ Url: newConfig.Zebra.Config.Url, RouteTypes: l, Version: uint32(newConfig.Zebra.Config.Version), @@ -256,25 +296,30 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * NexthopTriggerDelay: uint32(newConfig.Zebra.Config.NexthopTriggerDelay), MplsLabelRangeSize: newConfig.Zebra.Config.MplsLabelRangeSize, SoftwareName: newConfig.Zebra.Config.SoftwareName, - }); err != nil { + }) + if err != nil { bgpServer.Log().Error("failed to set zebra config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set zebra config: %w", err) } } if len(newConfig.Collector.Config.Url) > 0 { bgpServer.Log().Error("collector feature is not supported", slog.String("Topic", "config")) + return nil, errors.New("collector feature is not supported") } for _, c := range newConfig.RpkiServers { - if err := bgpServer.AddRpki(ctx, &api.AddRpkiRequest{ + err := bgpServer.AddRpki(ctx, &api.AddRpkiRequest{ Address: c.Config.Address.String(), Port: c.Config.Port, Lifetime: c.Config.RecordLifetime, - }); err != nil { + }) + if err != nil { bgpServer.Log().Error("failed to set rpki config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set rpki config: %w", err) } } f := func(t oc.BmpRouteMonitoringPolicyType) api.AddBmpRequest_MonitoringPolicy { @@ -294,16 +339,18 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * } for _, c := range newConfig.BmpServers { - if err := bgpServer.AddBmp(ctx, &api.AddBmpRequest{ + err := bgpServer.AddBmp(ctx, &api.AddBmpRequest{ Address: c.Config.Address.String(), Port: c.Config.Port, SysName: c.Config.SysName, SysDescr: c.Config.SysDescr, Policy: f(c.Config.RouteMonitoringPolicy), StatisticsTimeout: int32(c.Config.StatisticsTimeout), - }); err != nil { + }) + if err != nil { bgpServer.Log().Error("failed to set bmp config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set bmp config: %w", err) } } for _, vrf := range newConfig.Vrfs { @@ -311,25 +358,29 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * if err != nil { bgpServer.Log().Error("failed to load vrf rd config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to load vrf rd config: %w", err) } importRtList, err := marshalRouteTargets(vrf.Config.ImportRtList) if err != nil { bgpServer.Log().Error("failed to load vrf import rt config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to load vrf import rt config: %w", err) } exportRtList, err := marshalRouteTargets(vrf.Config.ExportRtList) if err != nil { bgpServer.Log().Error("failed to load vrf export rt config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to load vrf export rt config: %w", err) } a, err := apiutil.MarshalRD(rd) if err != nil { bgpServer.Log().Error("failed to set vrf config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set vrf config: %w", err) } - if err := bgpServer.AddVrf(ctx, &api.AddVrfRequest{ + err = bgpServer.AddVrf(ctx, &api.AddVrfRequest{ Vrf: &api.Vrf{ Name: vrf.Config.Name, Rd: a, @@ -337,8 +388,11 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * ImportRt: importRtList, ExportRt: exportRtList, }, - }); err != nil { - bgpServer.Log().Error("failed to set vrf config", slog.String("Topic", "config"), slog.Any("Error", err)) + }) + if err != nil { + bgpServer.Log().Error("failed to set vrf config", + slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set vrf config: %w", err) } } for _, c := range newConfig.MrtDump { @@ -354,14 +408,16 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * dump_type = api.EnableMrtRequest_DUMP_TYPE_TABLE } - if err := bgpServer.EnableMrt(ctx, &api.EnableMrtRequest{ + err := bgpServer.EnableMrt(ctx, &api.EnableMrtRequest{ DumpType: dump_type, Filename: c.Config.FileName, DumpInterval: c.Config.DumpInterval, RotationInterval: c.Config.RotationInterval, - }); err != nil { + }) + if err != nil { bgpServer.Log().Error("failed to set mrt config", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set mrt config: %w", err) } } p := oc.ConfigSetToRoutingPolicy(newConfig) @@ -369,15 +425,20 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * if err != nil { bgpServer.Log().Error("failed to update policy config", slog.String("Topic", "config"), slog.Any("Error", err)) - } else if err := bgpServer.SetPolicies(ctx, &api.SetPoliciesRequest{ + return nil, fmt.Errorf("failed to update policy config: %w", err) + } + if err := bgpServer.SetPolicies(ctx, &api.SetPoliciesRequest{ DefinedSets: rp.DefinedSets, Policies: rp.Policies, }); err != nil { bgpServer.Log().Error("failed to set policies", slog.String("Topic", "config"), slog.Any("Error", err)) + return nil, fmt.Errorf("failed to set policies: %w", err) } - assignGlobalpolicy(ctx, bgpServer, &newConfig.Global.ApplyPolicy.Config) + if err := assignGlobalpolicy(ctx, bgpServer, &newConfig.Global.ApplyPolicy.Config, stopOnConfigError); err != nil { + return nil, err + } added := newConfig.Neighbors addedPg := newConfig.PeerGroups @@ -389,9 +450,15 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * } } - addPeerGroups(ctx, bgpServer, addedPg) - addDynamicNeighbors(ctx, bgpServer, newConfig.DynamicNeighbors) - addNeighbors(ctx, bgpServer, added) + if err := addPeerGroups(ctx, bgpServer, addedPg, stopOnConfigError); err != nil { + return nil, err + } + if err := addDynamicNeighbors(ctx, bgpServer, newConfig.DynamicNeighbors, stopOnConfigError); err != nil { + return nil, err + } + if err := addNeighbors(ctx, bgpServer, added, stopOnConfigError); err != nil { + return nil, err + } return newConfig, nil } @@ -401,13 +468,15 @@ func InitialConfig(ctx context.Context, bgpServer *server.BgpServer, newConfig * // hangle graceful restart and 2) requires a BgpConfigSet for the previous // configuration so that it can compute the delta between it and the new // config. The new BgpConfigSet can be obtained using ReadConfigFile. +// Reload keeps the previous runtime behavior: config apply errors are logged, +// but they do not abort the running daemon or reject the whole reload. func UpdateConfig(ctx context.Context, bgpServer *server.BgpServer, c, newConfig *oc.BgpConfigSet) (*oc.BgpConfigSet, error) { addedPg, deletedPg, updatedPg := oc.UpdatePeerGroupConfig(bgpServer.Log(), c, newConfig) added, deleted, updated := oc.UpdateNeighborConfig(bgpServer.Log(), c, newConfig) updatePolicy := oc.CheckPolicyDifference(bgpServer.Log(), oc.ConfigSetToRoutingPolicy(c), oc.ConfigSetToRoutingPolicy(newConfig)) if updatePolicy { - bgpServer.Log().Info("policy config is update", slog.String("Topic", "config")) + bgpServer.Log().Info("policy config is updated", slog.String("Topic", "config")) p := oc.ConfigSetToRoutingPolicy(newConfig) rp, err := table.NewAPIRoutingPolicyFromConfigStruct(p) if err != nil { @@ -423,16 +492,16 @@ func UpdateConfig(ctx context.Context, bgpServer *server.BgpServer, c, newConfig } // global policy update if !newConfig.Global.ApplyPolicy.Config.Equal(&c.Global.ApplyPolicy.Config) { - assignGlobalpolicy(ctx, bgpServer, &newConfig.Global.ApplyPolicy.Config) + _ = assignGlobalpolicy(ctx, bgpServer, &newConfig.Global.ApplyPolicy.Config, continueOnConfigError) updatePolicy = true } - addPeerGroups(ctx, bgpServer, addedPg) + _ = addPeerGroups(ctx, bgpServer, addedPg, continueOnConfigError) deletePeerGroups(ctx, bgpServer, deletedPg) needsSoftResetIn := updatePeerGroups(ctx, bgpServer, updatedPg) updatePolicy = updatePolicy || needsSoftResetIn - addDynamicNeighbors(ctx, bgpServer, newConfig.DynamicNeighbors) - addNeighbors(ctx, bgpServer, added) + _ = addDynamicNeighbors(ctx, bgpServer, newConfig.DynamicNeighbors, continueOnConfigError) + _ = addNeighbors(ctx, bgpServer, added, continueOnConfigError) deleteNeighbors(ctx, bgpServer, deleted) needsSoftResetIn = updateNeighbors(ctx, bgpServer, updated) updatePolicy = updatePolicy || needsSoftResetIn diff --git a/pkg/config/server_config_test.go b/pkg/config/server_config_test.go index 6b27af92f..d2f2b22bd 100644 --- a/pkg/config/server_config_test.go +++ b/pkg/config/server_config_test.go @@ -2,12 +2,12 @@ package config import ( "context" + "io" "log/slog" "net/netip" - "os" + "sync" "testing" - "github.com/osrg/gobgp/v4/api" "github.com/osrg/gobgp/v4/pkg/config/oc" "github.com/osrg/gobgp/v4/pkg/server" "github.com/stretchr/testify/assert" @@ -15,23 +15,36 @@ import ( ) type ErrorCaptureHandler struct { - configErrors []string + mu *sync.Mutex + configErrors *[]string baseHandler slog.Handler } +func newErrorCaptureHandler() *ErrorCaptureHandler { + configErrors := []string{} + return &ErrorCaptureHandler{ + mu: &sync.Mutex{}, + configErrors: &configErrors, + baseHandler: slog.NewJSONHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelDebug}), + } +} + func (h *ErrorCaptureHandler) Enabled(_ context.Context, level slog.Level) bool { return h.baseHandler.Enabled(context.Background(), level) } func (h *ErrorCaptureHandler) Handle(ctx context.Context, record slog.Record) error { if record.Level >= slog.LevelError { - h.configErrors = append(h.configErrors, record.Message) + h.mu.Lock() + *h.configErrors = append(*h.configErrors, record.Message) + h.mu.Unlock() } return h.baseHandler.Handle(ctx, record) } func (h *ErrorCaptureHandler) WithAttrs(attrs []slog.Attr) slog.Handler { return &ErrorCaptureHandler{ + mu: h.mu, configErrors: h.configErrors, baseHandler: h.baseHandler.WithAttrs(attrs), } @@ -39,98 +52,148 @@ func (h *ErrorCaptureHandler) WithAttrs(attrs []slog.Attr) slog.Handler { func (h *ErrorCaptureHandler) WithGroup(name string) slog.Handler { return &ErrorCaptureHandler{ + mu: h.mu, configErrors: h.configErrors, baseHandler: h.baseHandler.WithGroup(name), } } -func TestConfigErrors(t *testing.T) { - globalCfg := oc.Global{ +func (h *ErrorCaptureHandler) Errors() []string { + h.mu.Lock() + defer h.mu.Unlock() + + return append([]string(nil), *h.configErrors...) +} + +func newTestBgpServer(t *testing.T) (*server.BgpServer, *ErrorCaptureHandler) { + t.Helper() + + handler := newErrorCaptureHandler() + logger := slog.New(handler) + bgpServer := server.NewBgpServer(server.LoggerOption(logger, &slog.LevelVar{})) + go bgpServer.Serve() + t.Cleanup(bgpServer.Stop) + return bgpServer, handler +} + +func testGlobalConfig() oc.Global { + return oc.Global{ Config: oc.GlobalConfig{ As: 1, RouterId: netip.MustParseAddr("1.1.1.1"), - Port: 11179, + Port: -1, }, } +} - for _, tt := range []struct { - name string - expectedErrors []string - cfg *oc.BgpConfigSet - }{ +func validConfig() *oc.BgpConfigSet { + return &oc.BgpConfigSet{ + Global: testGlobalConfig(), + } +} + +func configWithValidPeerGroup() *oc.BgpConfigSet { + cfg := validConfig() + cfg.Neighbors = []oc.Neighbor{ { - name: "peer with a valid peer-group", - cfg: &oc.BgpConfigSet{ - Global: globalCfg, - Neighbors: []oc.Neighbor{ - { - Config: oc.NeighborConfig{ - PeerGroup: "router", - NeighborAddress: netip.MustParseAddr("1.1.1.2"), - }, - }, - }, - PeerGroups: []oc.PeerGroup{ - { - Config: oc.PeerGroupConfig{ - PeerGroupName: "router", - PeerAs: 2, - }, - }, - }, + Config: oc.NeighborConfig{ + PeerGroup: "router", + NeighborAddress: netip.MustParseAddr("1.1.1.2"), }, }, + } + cfg.PeerGroups = []oc.PeerGroup{ { - name: "peer without peer-group", - expectedErrors: []string{"Failed to add Peer"}, - cfg: &oc.BgpConfigSet{ - Global: globalCfg, - Neighbors: []oc.Neighbor{ - { - Config: oc.NeighborConfig{ - PeerGroup: "not-exists", - NeighborAddress: netip.MustParseAddr("1.1.1.2"), - }, - }, - }, + Config: oc.PeerGroupConfig{ + PeerGroupName: "router", + PeerAs: 2, }, }, + } + return cfg +} + +func configWithMissingPeerGroup() *oc.BgpConfigSet { + cfg := validConfig() + cfg.Neighbors = []oc.Neighbor{ { - name: "policy without a set", - expectedErrors: []string{"failed to create routing policy", "failed to set policies"}, - cfg: &oc.BgpConfigSet{ - Global: globalCfg, - PolicyDefinitions: []oc.PolicyDefinition{ - { - Name: "policy-without-a-set", - Statements: []oc.Statement{ - { - Conditions: oc.Conditions{ - MatchNeighborSet: oc.MatchNeighborSet{ - NeighborSet: "not-existing-neighbor-set", - }, - }, - }, + Config: oc.NeighborConfig{ + PeerGroup: "not-exists", + NeighborAddress: netip.MustParseAddr("1.1.1.2"), + }, + }, + } + return cfg +} + +func configWithMissingPolicySet() *oc.BgpConfigSet { + cfg := validConfig() + cfg.PolicyDefinitions = []oc.PolicyDefinition{ + { + Name: "policy-without-a-set", + Statements: []oc.Statement{ + { + Conditions: oc.Conditions{ + MatchNeighborSet: oc.MatchNeighborSet{ + NeighborSet: "not-existing-neighbor-set", }, }, }, }, }, + } + return cfg +} + +func TestInitialConfigAppliesValidConfig(t *testing.T) { + ctx := context.Background() + bgpServer, handler := newTestBgpServer(t) + + _, err := InitialConfig(ctx, bgpServer, configWithValidPeerGroup(), false) + require.NoError(t, err) + assert.Empty(t, handler.Errors()) +} + +func TestInitialConfigReturnsConfigErrors(t *testing.T) { + for _, tt := range []struct { + name string + expectedErr string + expectedLog string + cfg *oc.BgpConfigSet + }{ + { + name: "peer without peer-group", + expectedErr: "failed to add peer", + expectedLog: "failed to add peer", + cfg: configWithMissingPeerGroup(), + }, + { + name: "policy without a set", + expectedErr: "failed to set policies", + expectedLog: "failed to set policies", + cfg: configWithMissingPolicySet(), + }, } { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - basehandler := slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}) - handler := ErrorCaptureHandler{baseHandler: basehandler} - logger := slog.New(&handler) - - bgpServer := server.NewBgpServer(server.LoggerOption(logger, &slog.LevelVar{})) - go bgpServer.Serve() + bgpServer, handler := newTestBgpServer(t) _, err := InitialConfig(ctx, bgpServer, tt.cfg, false) - require.NoError(t, err) - err = bgpServer.StopBgp(ctx, &api.StopBgpRequest{}) - require.NoError(t, err) - assert.Equal(t, tt.expectedErrors, handler.configErrors) + require.ErrorContains(t, err, tt.expectedErr) + assert.Contains(t, handler.Errors(), tt.expectedLog) }) } } + +func TestUpdateConfigKeepsConfigErrorsNonFatal(t *testing.T) { + ctx := context.Background() + bgpServer, handler := newTestBgpServer(t) + + currentConfig, err := InitialConfig(ctx, bgpServer, validConfig(), false) + require.NoError(t, err) + + _, err = UpdateConfig(ctx, bgpServer, currentConfig, configWithMissingPolicySet()) + require.NoError(t, err) + + assert.Contains(t, handler.Errors(), "failed to set policies") +}