From d780d590a14992e3b665ae76439e519989d229b6 Mon Sep 17 00:00:00 2001 From: Ivan Pokhabov Date: Tue, 4 Aug 2026 18:52:10 +0300 Subject: [PATCH 1/2] feat(config): return initial apply errors Make InitialConfig return errors from failed config application instead of only logging them, so gobgpd rejects invalid startup config such as missing peer-groups or broken policy references. Keep UpdateConfig reloads non-fatal and covered by tests. --- pkg/config/config.go | 201 +++++++++++++++++++++---------- pkg/config/server_config_test.go | 199 +++++++++++++++++++----------- 2 files changed, 266 insertions(+), 134 deletions(-) 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") +} From 6ced0527aa7432a0c46eabd0163e4eef61c2382d Mon Sep 17 00:00:00 2001 From: Ivan Pokhabov Date: Tue, 4 Aug 2026 18:53:37 +0300 Subject: [PATCH 2/2] fix(cmd/gobgpd): delay sdnotify readiness Send sd_notify READY only after the startup config has been read and applied. This prevents systemd from seeing gobgpd as ready right before it exits on an invalid initial config, while preserving the no-config-file startup path. --- cmd/gobgpd/main.go | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) 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")