diff --git a/pkg/config/oc/default.go b/pkg/config/oc/default.go index 3c3e59969..dff66d354 100644 --- a/pkg/config/oc/default.go +++ b/pkg/config/oc/default.go @@ -528,29 +528,32 @@ func setDefaultConfigValuesWithViper(v *viper.Viper, b *BgpConfigSet) error { func OverwriteNeighborConfigWithPeerGroup(c *Neighbor, pg *PeerGroup) error { v := viper.New() - val, ok := configuredFields[c.Config.NeighborAddress.String()] - if ok { + // configuredFields is only filled while reading a configuration file. For a + // neighbor added through the gRPC API viper reports every field as unset, so + // overwriteConfig treats a non-zero value as explicitly set instead. + val, configured := configuredFields[c.Config.NeighborAddress.String()] + if configured { v.Set("neighbor", val) } else { v.Set("neighbor.config.peer-group", c.Config.PeerGroup) } - overwriteConfig(&c.Config, &pg.Config, "neighbor.config", v) - overwriteConfig(&c.Timers.Config, &pg.Timers.Config, "neighbor.timers.config", v) - overwriteConfig(&c.Transport.Config, &pg.Transport.Config, "neighbor.transport.config", v) - overwriteConfig(&c.TcpAo.Config, &pg.TcpAo.Config, "neighbor.tcp-ao.config", v) - overwriteConfig(&c.ErrorHandling.Config, &pg.ErrorHandling.Config, "neighbor.error-handling.config", v) - overwriteConfig(&c.LoggingOptions.Config, &pg.LoggingOptions.Config, "neighbor.logging-options.config", v) - overwriteConfig(&c.EbgpMultihop.Config, &pg.EbgpMultihop.Config, "neighbor.ebgp-multihop.config", v) - overwriteConfig(&c.RouteReflector.Config, &pg.RouteReflector.Config, "neighbor.route-reflector.config", v) - overwriteConfig(&c.AsPathOptions.Config, &pg.AsPathOptions.Config, "neighbor.as-path-options.config", v) - overwriteConfig(&c.AddPaths.Config, &pg.AddPaths.Config, "neighbor.add-paths.config", v) - overwriteConfig(&c.GracefulRestart.Config, &pg.GracefulRestart.Config, "neighbor.gradeful-restart.config", v) - overwriteConfig(&c.ApplyPolicy.Config, &pg.ApplyPolicy.Config, "neighbor.apply-policy.config", v) - overwriteConfig(&c.UseMultiplePaths.Config, &pg.UseMultiplePaths.Config, "neighbor.use-multiple-paths.config", v) - overwriteConfig(&c.RouteServer.Config, &pg.RouteServer.Config, "neighbor.route-server.config", v) - overwriteConfig(&c.TtlSecurity.Config, &pg.TtlSecurity.Config, "neighbor.ttl-security.config", v) - overwriteConfig(&c.Bfd.Config, &pg.Bfd.Config, "neighbor.bfd.config", v) + overwriteConfig(&c.Config, &pg.Config, "neighbor.config", v, configured) + overwriteConfig(&c.Timers.Config, &pg.Timers.Config, "neighbor.timers.config", v, configured) + overwriteConfig(&c.Transport.Config, &pg.Transport.Config, "neighbor.transport.config", v, configured) + overwriteConfig(&c.TcpAo.Config, &pg.TcpAo.Config, "neighbor.tcp-ao.config", v, configured) + overwriteConfig(&c.ErrorHandling.Config, &pg.ErrorHandling.Config, "neighbor.error-handling.config", v, configured) + overwriteConfig(&c.LoggingOptions.Config, &pg.LoggingOptions.Config, "neighbor.logging-options.config", v, configured) + overwriteConfig(&c.EbgpMultihop.Config, &pg.EbgpMultihop.Config, "neighbor.ebgp-multihop.config", v, configured) + overwriteConfig(&c.RouteReflector.Config, &pg.RouteReflector.Config, "neighbor.route-reflector.config", v, configured) + overwriteConfig(&c.AsPathOptions.Config, &pg.AsPathOptions.Config, "neighbor.as-path-options.config", v, configured) + overwriteConfig(&c.AddPaths.Config, &pg.AddPaths.Config, "neighbor.add-paths.config", v, configured) + overwriteConfig(&c.GracefulRestart.Config, &pg.GracefulRestart.Config, "neighbor.gradeful-restart.config", v, configured) + overwriteConfig(&c.ApplyPolicy.Config, &pg.ApplyPolicy.Config, "neighbor.apply-policy.config", v, configured) + overwriteConfig(&c.UseMultiplePaths.Config, &pg.UseMultiplePaths.Config, "neighbor.use-multiple-paths.config", v, configured) + overwriteConfig(&c.RouteServer.Config, &pg.RouteServer.Config, "neighbor.route-server.config", v, configured) + overwriteConfig(&c.TtlSecurity.Config, &pg.TtlSecurity.Config, "neighbor.ttl-security.config", v, configured) + overwriteConfig(&c.Bfd.Config, &pg.Bfd.Config, "neighbor.bfd.config", v, configured) if !v.IsSet("neighbor.afi-safis") { c.AfiSafis = append([]AfiSafi{}, pg.AfiSafis...) @@ -559,7 +562,10 @@ func OverwriteNeighborConfigWithPeerGroup(c *Neighbor, pg *PeerGroup) error { return nil } -func overwriteConfig(c, pg any, tagPrefix string, v *viper.Viper) { +// overwriteConfig copies peer-group values into the neighbor config for every +// field the neighbor did not set itself. configured selects how "set" is +// decided: viper's view of the config file, or a non-zero value for API peers. +func overwriteConfig(c, pg any, tagPrefix string, v *viper.Viper, configured bool) { nValue := reflect.Indirect(reflect.ValueOf(c)) pgValue := reflect.Indirect(reflect.ValueOf(pg)) pgType := reflect.Indirect(pgValue).Type() @@ -567,12 +573,19 @@ func overwriteConfig(c, pg any, tagPrefix string, v *viper.Viper) { for i := range pgType.NumField() { field := pgType.Field(i).Name tag := tagPrefix + "." + pgType.Field(i).Tag.Get("mapstructure") - if func() bool { - return slices.Contains(forcedOverwrittenConfig, tag) - }() || !v.IsSet(tag) { - if nField := nValue.FieldByName(field); nField.IsValid() { - nField.Set(pgValue.FieldByName(field)) + nField := nValue.FieldByName(field) + if !nField.IsValid() { + continue + } + if !slices.Contains(forcedOverwrittenConfig, tag) { + if configured { + if v.IsSet(tag) { + continue + } + } else if !nField.IsZero() { + continue } } + nField.Set(pgValue.FieldByName(field)) } } diff --git a/pkg/config/oc/default_test.go b/pkg/config/oc/default_test.go index 8145cdf30..c906f198a 100644 --- a/pkg/config/oc/default_test.go +++ b/pkg/config/oc/default_test.go @@ -113,3 +113,112 @@ func TestOverwriteNeighborConfigWithPeerGroupTcpAo(t *testing.T) { assert.Equal(t, uint8(1), n.TcpAo.Config.SendId) }) } + +// clearConfiguredFields drops the config file record, as if the neighbor had +// been added through the gRPC API. configuredFields is package state, so it is +// restored afterwards. +func clearConfiguredFields(t *testing.T) { + t.Helper() + saved := configuredFields + t.Cleanup(func() { configuredFields = saved }) + configuredFields = nil +} + +// A neighbor added through the gRPC API is not recorded in configuredFields, so +// viper reports every field as unset. Only the fields the caller left empty may +// be taken from the peer group. +func TestOverwriteNeighborConfigWithPeerGroupFromAPI(t *testing.T) { + t.Run("caller_values_kept_and_empty_ones_inherited", func(t *testing.T) { + clearConfiguredFields(t) + + pg := &PeerGroup{ + Config: PeerGroupConfig{ + PeerGroupName: "g", + LocalAs: 65000, + Description: "group description", + AuthPassword: "group password", + }, + Timers: Timers{Config: TimersConfig{HoldTime: 90, KeepaliveInterval: 30}}, + } + n := &Neighbor{ + Config: NeighborConfig{ + NeighborAddress: netip.MustParseAddr(testNeighborAddress), + PeerGroup: "g", + Description: "neighbor description", + }, + Timers: Timers{Config: TimersConfig{HoldTime: 180}}, + } + + require.NoError(t, OverwriteNeighborConfigWithPeerGroup(n, pg)) + + assert.Equal(t, "neighbor description", n.Config.Description) + assert.Equal(t, float64(180), n.Timers.Config.HoldTime) + assert.Equal(t, "group password", n.Config.AuthPassword) + assert.Equal(t, uint32(65000), n.Config.LocalAs) + assert.Equal(t, float64(30), n.Timers.Config.KeepaliveInterval) + }) + + // TCP-AO reaches the neighbor config through the API as well. + t.Run("tcp_ao_kept", func(t *testing.T) { + clearConfiguredFields(t) + + n := newNeighborForTcpAoInheritanceTest() + n.TcpAo.Config = TcpAoConfig{Keychain: "peer-chain", SendId: 2} + require.NoError(t, OverwriteNeighborConfigWithPeerGroup(n, newPeerGroupForTcpAoInheritanceTest())) + assert.Equal(t, KeychainRef("peer-chain"), n.TcpAo.Config.Keychain) + assert.Equal(t, uint8(2), n.TcpAo.Config.SendId) + }) + + // peer-as is listed in forcedOverwrittenConfig, so the peer group wins over + // whatever the caller passed. + t.Run("forced_fields_still_come_from_the_group", func(t *testing.T) { + clearConfiguredFields(t) + + pg := &PeerGroup{Config: PeerGroupConfig{PeerGroupName: "g", PeerAs: 65001}} + n := &Neighbor{ + Config: NeighborConfig{ + NeighborAddress: netip.MustParseAddr(testNeighborAddress), + PeerGroup: "g", + PeerAs: 65002, + }, + } + + require.NoError(t, OverwriteNeighborConfigWithPeerGroup(n, pg)) + + assert.Equal(t, uint32(65001), n.Config.PeerAs) + }) +} + +// A neighbor read from a configuration file keeps the fields the operator +// spelled out, even when they hold the zero value of their type. +func TestOverwriteNeighborConfigWithPeerGroupKeepsConfiguredZeroValues(t *testing.T) { + registerConfiguredFields(t, testNeighborAddress, map[string]any{ + "config": map[string]any{ + "neighbor-address": testNeighborAddress, + "peer-group": "g", + "description": "", + "route-flap-damping": false, + }, + }) + + pg := &PeerGroup{ + Config: PeerGroupConfig{ + PeerGroupName: "g", + Description: "group description", + RouteFlapDamping: true, + AuthPassword: "group password", + }, + } + n := &Neighbor{ + Config: NeighborConfig{ + NeighborAddress: netip.MustParseAddr(testNeighborAddress), + PeerGroup: "g", + }, + } + + require.NoError(t, OverwriteNeighborConfigWithPeerGroup(n, pg)) + + assert.Equal(t, "", n.Config.Description) + assert.False(t, n.Config.RouteFlapDamping) + assert.Equal(t, "group password", n.Config.AuthPassword) +}