Skip to content

Commit fb7102b

Browse files
Sergey Klyausfujita
Sergey Klyaus
authored and
fujita
committed
make config parsing strict when starting gobgpd
The default policy for gobgp if config is invalid is to start with default policy ACCEPT. This is harmful for our Route Reflectors setup in which we can get spurous routes after unlucky release. This commit adds --config-strict option that makes gobgp do the fatal exit if config is invalid. If option is not specified, gobgpd will still log warning and continue working. It also introduces special facilty logging field similar to the one in syslog. This allows to implement said backward compatibility with Fatal() calls with facility "config" being downgraded to Warn(). Reload behavior is not changed as it is not as harmful (policies are retained in such case).
1 parent 1d92514 commit fb7102b

File tree

6 files changed

+166
-11
lines changed

6 files changed

+166
-11
lines changed

‎cmd/gobgpd/main.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ func main() {
5757
ConfigFile string `short:"f" long:"config-file" description:"specifying a config file"`
5858
ConfigType string `short:"t" long:"config-type" description:"specifying config type (toml, yaml, json)" default:"toml"`
5959
ConfigAutoReload bool `short:"a" long:"config-auto-reload" description:"activate config auto reload on changes"`
60+
ConfigStrict bool `long:"config-strict" description:"make any config error fatal"`
6061
LogLevel string `short:"l" long:"log-level" description:"specifying log level"`
6162
LogPlain bool `short:"p" long:"log-plain" description:"use plain format for logging (json by default)"`
6263
UseSyslog string `short:"s" long:"syslog" description:"use syslogd"`
@@ -197,7 +198,8 @@ func main() {
197198
}
198199

199200
logger.Info("gobgpd started")
200-
bgpServer := server.NewBgpServer(server.GrpcListenAddress(opts.GrpcHosts), server.GrpcOption(grpcOpts), server.LoggerOption(&builtinLogger{logger: logger}))
201+
bgpLogger := &builtinLogger{logger: logger, cfgStrict: opts.ConfigStrict}
202+
bgpServer := server.NewBgpServer(server.GrpcListenAddress(opts.GrpcHosts), server.GrpcOption(grpcOpts), server.LoggerOption(bgpLogger))
201203
prometheus.MustRegister(metrics.NewBgpCollector(bgpServer))
202204
go bgpServer.Serve()
203205

@@ -258,6 +260,8 @@ func main() {
258260
})
259261
}
260262

263+
// Avoid crashing gobgpd on reload - it shouldn't flush policy entirely, so it's safe to continue to run
264+
bgpLogger.cfgStrict = false
261265
for sig := range sigCh {
262266
if sig != syscall.SIGHUP {
263267
stopServer(bgpServer, opts.UseSdNotify)

‎cmd/gobgpd/util.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,14 +105,22 @@ func addSyslogHook(host, facility string) error {
105105
}
106106

107107
type builtinLogger struct {
108-
logger *logrus.Logger
108+
logger *logrus.Logger
109+
cfgStrict bool
109110
}
110111

111112
func (l *builtinLogger) Panic(msg string, fields log.Fields) {
112113
l.logger.WithFields(logrus.Fields(fields)).Panic(msg)
113114
}
114115

115116
func (l *builtinLogger) Fatal(msg string, fields log.Fields) {
117+
if facility, hasFacility := fields[log.FieldFacility];
118+
hasFacility && facility == log.FacilityConfig && !l.cfgStrict {
119+
// Backward compatibility with old behavior when any logical config error was treated as warning
120+
l.logger.WithFields(logrus.Fields(fields)).Warn(msg)
121+
return
122+
}
123+
116124
l.logger.WithFields(logrus.Fields(fields)).Fatal(msg)
117125
}
118126

‎internal/pkg/table/policy.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4027,8 +4027,10 @@ func (r *RoutingPolicy) Reset(rp *oc.RoutingPolicy, ap map[string]oc.ApplyPolicy
40274027
defer r.mu.Unlock()
40284028

40294029
if err := r.reload(*rp); err != nil {
4030-
r.logger.Error("failed to create routing policy",
4030+
r.logger.Fatal("failed to create routing policy",
40314031
log.Fields{
4032+
log.FieldFacility: log.FacilityConfig,
4033+
40324034
"Topic": "Policy",
40334035
"Error": err})
40344036
return err

‎pkg/config/config.go‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,10 @@ func addPeerGroups(ctx context.Context, bgpServer *server.BgpServer, addedPg []o
9898
if err := bgpServer.AddPeerGroup(ctx, &api.AddPeerGroupRequest{
9999
PeerGroup: oc.NewPeerGroupFromConfigStruct(&pg),
100100
}); err != nil {
101-
bgpServer.Log().Warn("Failed to add PeerGroup",
101+
bgpServer.Log().Fatal("Failed to add PeerGroup",
102102
log.Fields{
103+
log.FieldFacility: log.FacilityConfig,
104+
103105
"Topic": "config",
104106
"Key": pg.Config.PeerGroupName,
105107
"Error": err})
@@ -116,8 +118,10 @@ func deletePeerGroups(ctx context.Context, bgpServer *server.BgpServer, deletedP
116118
if err := bgpServer.DeletePeerGroup(ctx, &api.DeletePeerGroupRequest{
117119
Name: pg.Config.PeerGroupName,
118120
}); err != nil {
119-
bgpServer.Log().Warn("Failed to delete PeerGroup",
121+
bgpServer.Log().Fatal("Failed to delete PeerGroup",
120122
log.Fields{
123+
log.FieldFacility: log.FacilityConfig,
124+
121125
"Topic": "config",
122126
"Key": pg.Config.PeerGroupName,
123127
"Error": err})
@@ -134,8 +138,10 @@ func updatePeerGroups(ctx context.Context, bgpServer *server.BgpServer, updatedP
134138
if u, err := bgpServer.UpdatePeerGroup(ctx, &api.UpdatePeerGroupRequest{
135139
PeerGroup: oc.NewPeerGroupFromConfigStruct(&pg),
136140
}); err != nil {
137-
bgpServer.Log().Warn("Failed to update PeerGroup",
141+
bgpServer.Log().Fatal("Failed to update PeerGroup",
138142
log.Fields{
143+
log.FieldFacility: log.FacilityConfig,
144+
139145
"Topic": "config",
140146
"Key": pg.Config.PeerGroupName,
141147
"Error": err})
@@ -159,8 +165,10 @@ func addDynamicNeighbors(ctx context.Context, bgpServer *server.BgpServer, dynam
159165
PeerGroup: dn.Config.PeerGroup,
160166
},
161167
}); err != nil {
162-
bgpServer.Log().Warn("Failed to add Dynamic Neighbor to PeerGroup",
168+
bgpServer.Log().Fatal("Failed to add Dynamic Neighbor to PeerGroup",
163169
log.Fields{
170+
log.FieldFacility: log.FacilityConfig,
171+
164172
"Topic": "config",
165173
"Key": dn.Config.PeerGroup,
166174
"Prefix": dn.Config.Prefix,
@@ -178,8 +186,10 @@ func addNeighbors(ctx context.Context, bgpServer *server.BgpServer, added []oc.N
178186
if err := bgpServer.AddPeer(ctx, &api.AddPeerRequest{
179187
Peer: oc.NewPeerFromConfigStruct(&p),
180188
}); err != nil {
181-
bgpServer.Log().Warn("Failed to add Peer",
189+
bgpServer.Log().Fatal("Failed to add Peer",
182190
log.Fields{
191+
log.FieldFacility: log.FacilityConfig,
192+
183193
"Topic": "config",
184194
"Key": p.State.NeighborAddress,
185195
"Error": err})
@@ -196,8 +206,10 @@ func deleteNeighbors(ctx context.Context, bgpServer *server.BgpServer, deleted [
196206
if err := bgpServer.DeletePeer(ctx, &api.DeletePeerRequest{
197207
Address: p.State.NeighborAddress,
198208
}); err != nil {
199-
bgpServer.Log().Warn("Failed to delete Peer",
209+
bgpServer.Log().Fatal("Failed to delete Peer",
200210
log.Fields{
211+
log.FieldFacility: log.FacilityConfig,
212+
201213
"Topic": "config",
202214
"Key": p.State.NeighborAddress,
203215
"Error": err})
@@ -214,8 +226,10 @@ func updateNeighbors(ctx context.Context, bgpServer *server.BgpServer, updated [
214226
if u, err := bgpServer.UpdatePeer(ctx, &api.UpdatePeerRequest{
215227
Peer: oc.NewPeerFromConfigStruct(&p),
216228
}); err != nil {
217-
bgpServer.Log().Warn("Failed to update Peer",
229+
bgpServer.Log().Fatal("Failed to update Peer",
218230
log.Fields{
231+
log.FieldFacility: log.FacilityConfig,
232+
219233
"Topic": "config",
220234
"Key": p.State.NeighborAddress,
221235
"Error": err})
@@ -381,8 +395,10 @@ func UpdateConfig(ctx context.Context, bgpServer *server.BgpServer, c, newConfig
381395
p := oc.ConfigSetToRoutingPolicy(newConfig)
382396
rp, err := table.NewAPIRoutingPolicyFromConfigStruct(p)
383397
if err != nil {
384-
bgpServer.Log().Warn("failed to update policy config",
398+
bgpServer.Log().Fatal("failed to update policy config",
385399
log.Fields{
400+
log.FieldFacility: log.FacilityConfig,
401+
386402
"Topic": "config",
387403
"Error": err})
388404
} else {

‎pkg/config/server_config_test.go‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
package config
2+
3+
import (
4+
"context"
5+
api "github.com/osrg/gobgp/v3/api"
6+
"github.com/osrg/gobgp/v3/pkg/config/oc"
7+
"github.com/osrg/gobgp/v3/pkg/log"
8+
"github.com/osrg/gobgp/v3/pkg/server"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
"testing"
12+
)
13+
14+
type configErrorLogger struct {
15+
log.DefaultLogger
16+
17+
configErrors []string
18+
}
19+
20+
func (l *configErrorLogger) Fatal(msg string, fields log.Fields) {
21+
if facility, hasFacility := fields[log.FieldFacility]; hasFacility && facility == log.FacilityConfig {
22+
l.configErrors = append(l.configErrors, msg)
23+
l.DefaultLogger.Error(msg, fields)
24+
} else {
25+
l.DefaultLogger.Fatal(msg, fields)
26+
}
27+
}
28+
29+
func TestConfigErrors(t *testing.T) {
30+
globalCfg := oc.Global{
31+
Config: oc.GlobalConfig{
32+
As: 1,
33+
RouterId: "1.1.1.1",
34+
Port: 11179,
35+
},
36+
}
37+
38+
for _, tt := range []struct {
39+
name string
40+
expectedErrors []string
41+
cfg *oc.BgpConfigSet
42+
}{
43+
{
44+
name: "peer with a valid peer-group",
45+
cfg: &oc.BgpConfigSet{
46+
Global: globalCfg,
47+
Neighbors: []oc.Neighbor{
48+
{
49+
Config: oc.NeighborConfig{
50+
PeerGroup: "router",
51+
NeighborAddress: "1.1.1.2",
52+
},
53+
},
54+
},
55+
PeerGroups: []oc.PeerGroup{
56+
{
57+
Config: oc.PeerGroupConfig{
58+
PeerGroupName: "router",
59+
PeerAs: 2,
60+
},
61+
},
62+
},
63+
},
64+
},
65+
{
66+
name: "peer without peer-group",
67+
expectedErrors: []string{"Failed to add Peer"},
68+
cfg: &oc.BgpConfigSet{
69+
Global: globalCfg,
70+
Neighbors: []oc.Neighbor{
71+
{
72+
Config: oc.NeighborConfig{
73+
PeerGroup: "not-exists",
74+
NeighborAddress: "1.1.1.2",
75+
},
76+
},
77+
},
78+
},
79+
},
80+
{
81+
name: "policy without a set",
82+
expectedErrors: []string{"failed to create routing policy"},
83+
cfg: &oc.BgpConfigSet{
84+
Global: globalCfg,
85+
PolicyDefinitions: []oc.PolicyDefinition{
86+
{
87+
Name: "policy-without-a-set",
88+
Statements: []oc.Statement{
89+
{
90+
Conditions: oc.Conditions{
91+
MatchNeighborSet: oc.MatchNeighborSet{
92+
NeighborSet: "not-existing-neighbor-set",
93+
},
94+
},
95+
},
96+
},
97+
},
98+
},
99+
},
100+
},
101+
} {
102+
t.Run(tt.name, func(t *testing.T) {
103+
ctx := context.Background()
104+
logger := &configErrorLogger{
105+
DefaultLogger: *log.NewDefaultLogger(),
106+
}
107+
bgpServer := server.NewBgpServer(server.LoggerOption(logger))
108+
go bgpServer.Serve()
109+
110+
_, err := InitialConfig(ctx, bgpServer, tt.cfg, false)
111+
bgpServer.StopBgp(ctx, &api.StopBgpRequest{})
112+
require.NoError(t, err)
113+
assert.Equal(t, tt.expectedErrors, logger.configErrors)
114+
})
115+
}
116+
}

‎pkg/log/logger.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,15 @@ const (
3131
TraceLevel
3232
)
3333

34+
const (
35+
FieldFacility = "_facility"
36+
)
37+
38+
var (
39+
FacilityUnspecified interface{} = "unspecified"
40+
FacilityConfig interface{} = "config"
41+
)
42+
3443
type Fields map[string]interface{}
3544

3645
type Logger interface {

0 commit comments

Comments
 (0)