config: Properly set config of PeerGroup member

Currently, the config of PeerGroup members are not set properly.

For example, if LocalAs of the neighbor is not set in the config file,
LocalAs will be set to Global.As as a default value.
After this, however, if the neighbor is a member of a peer group,
LocalAs will be unexpectedly overwritten by the peer group config.

This commit fixes this by setting default values after
applying the peer group config.

Adding to this, this commit avoids unnecessary applying
the peer group config.

Signed-off-by: Satoshi Fujimoto <satoshi.fujimoto7@gmail.com>

5 files changed, 76 insertions, 65 deletions

diff --git a/config/default.go b/config/default.go
index 0cf1ef8a..0739b40e 100644
--- a/config/default.go
+++ b/config/default.go
@@ -101,11 +101,17 @@ func isLocalLinkLocalAddress(ifindex int, addr net.IP) (bool, error) {
 	return false, nil
 }
 
-func SetDefaultNeighborConfigValues(n *Neighbor, g *Global) error {
-	return setDefaultNeighborConfigValuesWithViper(nil, n, g)
+func SetDefaultNeighborConfigValues(n *Neighbor, pg *PeerGroup, g *Global) error {
+	// Determines this function is called against the same Neighbor struct,
+	// and if already called, returns immediately.
+	if n.State.LocalAs != 0 {
+		return nil
+	}
+
+	return setDefaultNeighborConfigValuesWithViper(nil, n, g, pg)
 }
 
-func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Global) error {
+func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Global, pg *PeerGroup) error {
 	if n == nil {
 		return fmt.Errorf("neighbor config is nil")
 	}
@@ -114,14 +120,19 @@ func setDefaultNeighborConfigValuesWithViper(v *viper.Viper, n *Neighbor, g *Glo
 	}
 
 	if v == nil {
-		// Determines this function is called against the same Neighbor struct,
-		// and if already called, returns immediately.
-		if n.State.LocalAs != 0 {
-			return nil
-		}
 		v = viper.New()
 	}
 
+	if pg != nil {
+		if err := OverwriteNeighborConfigWithPeerGroup(n, pg); err != nil {
+			return err
+		}
+	}
+
+	if n.Config.PeerAs == 0 {
+		return fmt.Errorf("peer-as is not set for %s", n.Config.NeighborAddress)
+	}
+
 	if n.Config.LocalAs == 0 {
 		n.Config.LocalAs = g.Config.As
 		if !g.Confederation.Config.Enabled || n.IsConfederation(g) {
@@ -328,45 +339,6 @@ func setDefaultPolicyConfigValuesWithViper(v *viper.Viper, p *PolicyDefinition)
 	return nil
 }
 
-func validatePeerGroupConfig(n *Neighbor, b *BgpConfigSet) error {
-	name := n.Config.PeerGroup
-	if name == "" {
-		return nil
-	}
-
-	pg, err := getPeerGroup(name, b)
-	if err != nil {
-		return err
-	}
-
-	if pg.Config.PeerAs != 0 && n.Config.PeerAs != 0 {
-		return fmt.Errorf("Cannot configure remote-as for members. PeerGroup AS %d.", pg.Config.PeerAs)
-	}
-	return nil
-}
-
-func getPeerGroup(n string, b *BgpConfigSet) (*PeerGroup, error) {
-	if n == "" {
-		return nil, fmt.Errorf("peer-group name is not configured")
-	}
-	for _, pg := range b.PeerGroups {
-		if n == pg.Config.PeerGroupName {
-			return &pg, nil
-		}
-	}
-	return nil, fmt.Errorf("No such peer-group: %s", n)
-}
-
-func validateDynamicNeighborConfig(d *DynamicNeighborConfig, b *BgpConfigSet) error {
-	if _, err := getPeerGroup(d.PeerGroup, b); err != nil {
-		return err
-	}
-	if _, _, err := net.ParseCIDR(d.Prefix); err != nil {
-		return fmt.Errorf("Invalid Dynamic Neighbor prefix %s", d.Prefix)
-	}
-	return nil
-}
-
 func setDefaultConfigValuesWithViper(v *viper.Viper, b *BgpConfigSet) error {
 	if v == nil {
 		v = viper.New()
@@ -411,26 +383,32 @@ func setDefaultConfigValuesWithViper(v *viper.Viper, b *BgpConfigSet) error {
 	}
 
 	for idx, n := range b.Neighbors {
-		if err := validatePeerGroupConfig(&n, b); err != nil {
-			return err
-		}
-
 		vv := viper.New()
 		if len(list) > idx {
 			vv.Set("neighbor", list[idx])
 		}
-		if err := setDefaultNeighborConfigValuesWithViper(vv, &n, &b.Global); err != nil {
-			return err
+
+		pg, err := b.getPeerGroup(n.Config.PeerGroup)
+		if err != nil {
+			return nil
 		}
-		b.Neighbors[idx] = n
 
-		if n.Config.PeerGroup != "" {
-			RegisterConfiguredFields(vv.Get("neighbor.config.neighbor-address").(string), list[idx])
+		if pg != nil {
+			identifier := vv.Get("neighbor.config.neighbor-address")
+			if identifier == nil {
+				identifier = vv.Get("neighbor.config.neighbor-interface")
+			}
+			RegisterConfiguredFields(identifier.(string), list[idx])
 		}
+
+		if err := setDefaultNeighborConfigValuesWithViper(vv, &n, &b.Global, pg); err != nil {
+			return err
+		}
+		b.Neighbors[idx] = n
 	}
 
 	for _, d := range b.DynamicNeighbors {
-		if err := validateDynamicNeighborConfig(&d.Config, b); err != nil {
+		if err := d.validate(b); err != nil {
 			return err
 		}
 	}
@@ -463,7 +441,7 @@ func setDefaultConfigValuesWithViper(v *viper.Viper, b *BgpConfigSet) error {
 func OverwriteNeighborConfigWithPeerGroup(c *Neighbor, pg *PeerGroup) error {
 	v := viper.New()
 
-	val, ok := configuredFields[c.State.NeighborAddress]
+	val, ok := configuredFields[c.Config.NeighborAddress]
 	if ok {
 		v.Set("neighbor", val)
 	} else {
diff --git a/config/util.go b/config/util.go
index 3bbddca9..bcb5f2a7 100644
--- a/config/util.go
+++ b/config/util.go
@@ -40,6 +40,32 @@ func detectConfigFileType(path, def string) string {
 	}
 }
 
+func (b *BgpConfigSet) getPeerGroup(n string) (*PeerGroup, error) {
+	if n == "" {
+		return nil, nil
+	}
+	for _, pg := range b.PeerGroups {
+		if n == pg.Config.PeerGroupName {
+			return &pg, nil
+		}
+	}
+	return nil, fmt.Errorf("no such peer-group: %s", n)
+}
+
+func (d *DynamicNeighbor) validate(b *BgpConfigSet) error {
+	if d.Config.PeerGroup == "" {
+		return fmt.Errorf("dynamic neighbor requires the peer group config")
+	}
+
+	if _, err := b.getPeerGroup(d.Config.PeerGroup); err != nil {
+		return err
+	}
+	if _, _, err := net.ParseCIDR(d.Config.Prefix); err != nil {
+		return fmt.Errorf("invalid dynamic neighbor prefix %s", d.Config.Prefix)
+	}
+	return nil
+}
+
 func (n *Neighbor) IsConfederationMember(g *Global) bool {
 	for _, member := range g.Confederation.Config.MemberAsList {
 		if member == n.Config.PeerAs {
diff --git a/server/peer.go b/server/peer.go
index 135713f9..44bb89db 100644
--- a/server/peer.go
+++ b/server/peer.go
@@ -79,7 +79,7 @@ func newDynamicPeer(g *config.Global, neighborAddress string, pg *config.PeerGro
 		}).Debugf("Can't overwrite neighbor config: %s", err)
 		return nil
 	}
-	if err := config.SetDefaultNeighborConfigValues(&conf, g); err != nil {
+	if err := config.SetDefaultNeighborConfigValues(&conf, pg, g); err != nil {
 		log.WithFields(log.Fields{
 			"Topic": "Peer",
 			"Key":   neighborAddress,
diff --git a/server/server.go b/server/server.go
index bcecdd59..7d6194f4 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1762,13 +1762,16 @@ func (server *BgpServer) addNeighbor(c *config.Neighbor) error {
 		return fmt.Errorf("Can't overwrite the existing peer: %s", addr)
 	}
 
+	var pgConf *config.PeerGroup
 	if c.Config.PeerGroup != "" {
-		if err := config.OverwriteNeighborConfigWithPeerGroup(c, server.peerGroupMap[c.Config.PeerGroup].Conf); err != nil {
-			return err
+		pg, ok := server.peerGroupMap[c.Config.PeerGroup]
+		if !ok {
+			return fmt.Errorf("no such peer-group: %s", c.Config.PeerGroup)
 		}
+		pgConf = pg.Conf
 	}
 
-	if err := config.SetDefaultNeighborConfigValues(c, &server.bgpConfig.Global); err != nil {
+	if err := config.SetDefaultNeighborConfigValues(c, pgConf, &server.bgpConfig.Global); err != nil {
 		return err
 	}
 
@@ -1965,8 +1968,12 @@ func (s *BgpServer) UpdatePeerGroup(pg *config.PeerGroup) (needsSoftResetIn bool
 
 func (s *BgpServer) updateNeighbor(c *config.Neighbor) (needsSoftResetIn bool, err error) {
 	if c.Config.PeerGroup != "" {
-		if err := config.OverwriteNeighborConfigWithPeerGroup(c, s.peerGroupMap[c.Config.PeerGroup].Conf); err != nil {
-			return needsSoftResetIn, err
+		if pg, ok := s.peerGroupMap[c.Config.PeerGroup]; ok {
+			if err := config.SetDefaultNeighborConfigValues(c, pg.Conf, &s.bgpConfig.Global); err != nil {
+				return needsSoftResetIn, err
+			}
+		} else {
+			return needsSoftResetIn, fmt.Errorf("no such peer-group: %s", c.Config.PeerGroup)
 		}
 	}
 
diff --git a/server/server_test.go b/server/server_test.go
index bb9e7ff1..7624bb04 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -201,7 +201,7 @@ func TestNumGoroutineWithAddDeleteNeighbor(t *testing.T) {
 func newPeerandInfo(myAs, as uint32, address string, rib *table.TableManager) (*Peer, *table.PeerInfo) {
 	nConf := &config.Neighbor{Config: config.NeighborConfig{PeerAs: as, NeighborAddress: address}}
 	gConf := &config.Global{Config: config.GlobalConfig{As: myAs}}
-	config.SetDefaultNeighborConfigValues(nConf, gConf)
+	config.SetDefaultNeighborConfigValues(nConf, nil, gConf)
 	policy := table.NewRoutingPolicy()
 	policy.Reset(&config.RoutingPolicy{}, nil)
 	p := NewPeer(