config/oc: keep the neighbor config set through the API

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,20 +562,30 @@ 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()
 
 	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))
 	}
 }

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)
+}