config/gobgpd: refactoring

Signed-off-by: ISHIDA Wataru <ishida.wataru@lab.ntt.co.jp>

4 files changed, 52 insertions, 72 deletions

diff --git a/config/default.go b/config/default.go
index 7431d267..6d70e9e5 100644
--- a/config/default.go
+++ b/config/default.go
@@ -17,7 +17,7 @@ const (
 	DEFAULT_MPLS_LABEL_MAX            = 1048575
 )
 
-func SetDefaultConfigValues(v *viper.Viper, b *Bgp) error {
+func SetDefaultConfigValues(v *viper.Viper, b *BgpConfigSet) error {
 	if v == nil {
 		v = viper.New()
 	}
diff --git a/config/serve.go b/config/serve.go
index afa8f340..e88cb74e 100644
--- a/config/serve.go
+++ b/config/serve.go
@@ -10,59 +10,41 @@ import (
 )
 
 type BgpConfigSet struct {
-	Bgp    Bgp
-	Policy RoutingPolicy
+	Global            Global             `mapstructure:"global"`
+	Neighbors         []Neighbor         `mapstructure:"neighbors"`
+	PeerGroups        []PeerGroup        `mapstructure:"peer-groups"`
+	RpkiServers       []RpkiServer       `mapstructure:"rpki-servers"`
+	BmpServers        []BmpServer        `mapstructure:"bmp-servers"`
+	MrtDump           []Mrt              `mapstructure:"mrt-dump"`
+	DefinedSets       DefinedSets        `mapstructure:"defined-sets"`
+	PolicyDefinitions []PolicyDefinition `mapstructure:"policy-definitions"`
 }
 
-func ReadConfigfileServe(path, format string, configCh chan BgpConfigSet) {
+func ReadConfigfileServe(path, format string, configCh chan *BgpConfigSet) {
 	sigCh := make(chan os.Signal, 1)
 	signal.Notify(sigCh, syscall.SIGHUP)
 
 	cnt := 0
 	for {
-		var b *Bgp
+		c := &BgpConfigSet{}
 		v := viper.New()
 		v.SetConfigFile(path)
 		v.SetConfigType(format)
-		err := v.ReadInConfig()
-		c := struct {
-			Global            Global             `mapstructure:"global"`
-			Neighbors         []Neighbor         `mapstructure:"neighbors"`
-			RpkiServers       []RpkiServer       `mapstructure:"rpki-servers"`
-			BmpServers        []BmpServer        `mapstructure:"bmp-servers"`
-			MrtDump           []Mrt              `mapstructure:"mrt-dump"`
-			DefinedSets       DefinedSets        `mapstructure:"defined-sets"`
-			PolicyDefinitions []PolicyDefinition `mapstructure:"policy-definitions"`
-		}{}
-		if err != nil {
+		var err error
+		if err = v.ReadInConfig(); err != nil {
 			goto ERROR
 		}
-		err = v.UnmarshalExact(&c)
-		if err != nil {
+		if err = v.UnmarshalExact(c); err != nil {
 			goto ERROR
 		}
-		b = &Bgp{
-			Global:      c.Global,
-			Neighbors:   c.Neighbors,
-			RpkiServers: c.RpkiServers,
-			BmpServers:  c.BmpServers,
-			MrtDump:     c.MrtDump,
-		}
-		err = SetDefaultConfigValues(v, b)
-		if err != nil {
+		if err = SetDefaultConfigValues(v, c); err != nil {
 			goto ERROR
 		}
 		if cnt == 0 {
 			log.Info("finished reading the config file")
 		}
 		cnt++
-		configCh <- BgpConfigSet{
-			Bgp: *b,
-			Policy: RoutingPolicy{
-				DefinedSets:       c.DefinedSets,
-				PolicyDefinitions: c.PolicyDefinitions,
-			},
-		}
+		configCh <- c
 		select {
 		case <-sigCh:
 			log.Info("reload the config file")
@@ -88,11 +70,18 @@ func inSlice(n Neighbor, b []Neighbor) int {
 	return -1
 }
 
-func UpdateConfig(curC *Bgp, newC *Bgp) (*Bgp, []Neighbor, []Neighbor, []Neighbor) {
-	bgpConfig := Bgp{}
+func ConfigSetToRoutingPolicy(c *BgpConfigSet) *RoutingPolicy {
+	return &RoutingPolicy{
+		DefinedSets:       c.DefinedSets,
+		PolicyDefinitions: c.PolicyDefinitions,
+	}
+}
+
+func UpdateConfig(curC *BgpConfigSet, newC *BgpConfigSet) (*BgpConfigSet, []Neighbor, []Neighbor, []Neighbor, bool) {
+	bgpConfig := &BgpConfigSet{}
 	if curC == nil {
 		bgpConfig.Global = newC.Global
-		curC = &bgpConfig
+		curC = bgpConfig
 	} else {
 		// can't update the global config
 		bgpConfig.Global = curC.Global
@@ -118,7 +107,7 @@ func UpdateConfig(curC *Bgp, newC *Bgp) (*Bgp, []Neighbor, []Neighbor, []Neighbo
 	}
 
 	bgpConfig.Neighbors = newC.Neighbors
-	return &bgpConfig, added, deleted, updated
+	return bgpConfig, added, deleted, updated, CheckPolicyDifference(ConfigSetToRoutingPolicy(curC), ConfigSetToRoutingPolicy(newC))
 }
 
 func CheckPolicyDifference(currentPolicy *RoutingPolicy, newPolicy *RoutingPolicy) bool {
diff --git a/gobgpd/main.go b/gobgpd/main.go
index f6788cab..d955599f 100644
--- a/gobgpd/main.go
+++ b/gobgpd/main.go
@@ -159,7 +159,7 @@ func main() {
 		log.SetFormatter(&log.JSONFormatter{})
 	}
 
-	configCh := make(chan config.BgpConfigSet)
+	configCh := make(chan *config.BgpConfigSet)
 	if opts.Dry {
 		go config.ReadConfigfileServe(opts.ConfigFile, opts.ConfigType, configCh)
 		c := <-configCh
@@ -193,26 +193,32 @@ func main() {
 		go config.ReadConfigfileServe(opts.ConfigFile, opts.ConfigType, configCh)
 	}
 
-	var bgpConfig *config.Bgp = nil
-	var policyConfig *config.RoutingPolicy = nil
+	var c *config.BgpConfigSet = nil
 	for {
 		select {
 		case newConfig := <-configCh:
 			var added, deleted, updated []config.Neighbor
 
-			if bgpConfig == nil {
-				if err := bgpServer.SetGlobalType(newConfig.Bgp.Global); err != nil {
+			if c == nil {
+				c = newConfig
+				if err := bgpServer.SetGlobalType(newConfig.Global); err != nil {
 					log.Fatalf("failed to set global config: %s", err)
 				}
-				bgpConfig = &newConfig.Bgp
-				bgpServer.SetRpkiConfig(newConfig.Bgp.RpkiServers)
-				if err := bgpServer.SetBmpConfig(newConfig.Bgp.BmpServers); err != nil {
-					log.Fatalf("failed to set global config: %s", err)
+				if err := bgpServer.SetRpkiConfig(newConfig.RpkiServers); err != nil {
+					log.Fatalf("failed to set rpki config: %s", err)
 				}
-				if err := bgpServer.SetMrtConfig(newConfig.Bgp.MrtDump); err != nil {
-					log.Fatalf("failed to set global config: %s", err)
+				if err := bgpServer.SetBmpConfig(newConfig.BmpServers); err != nil {
+					log.Fatalf("failed to set bmp config: %s", err)
+				}
+				if err := bgpServer.SetMrtConfig(newConfig.MrtDump); err != nil {
+					log.Fatalf("failed to set mrt config: %s", err)
 				}
-				added = newConfig.Bgp.Neighbors
+				p := config.ConfigSetToRoutingPolicy(newConfig)
+				if err := bgpServer.SetRoutingPolicy(*p); err != nil {
+					log.Fatalf("failed to set routing policy: %s", err)
+				}
+
+				added = newConfig.Neighbors
 				if opts.GracefulRestart {
 					for i, n := range added {
 						if n.GracefulRestart.Config.Enabled {
@@ -220,31 +226,16 @@ func main() {
 						}
 					}
 				}
-				deleted = []config.Neighbor{}
-				updated = []config.Neighbor{}
-			} else {
-				bgpConfig, added, deleted, updated = config.UpdateConfig(bgpConfig, &newConfig.Bgp)
-			}
 
-			if policyConfig == nil {
-				policyConfig = &newConfig.Policy
-				// FIXME: Currently the following code
-				// is safe because the above
-				// SetRpkiConfig will be blocked
-				// because the length of rpkiConfigCh
-				// is zero. So server.GlobalRib is
-				// allocated before the above
-				// SetPolicy. But this should be
-				// handled more cleanly.
-				if err := bgpServer.SetRoutingPolicy(newConfig.Policy); err != nil {
-					log.Fatal(err)
-				}
 			} else {
-				if config.CheckPolicyDifference(policyConfig, &newConfig.Policy) {
+				var updatePolicy bool
+				c, added, deleted, updated, updatePolicy = config.UpdateConfig(c, newConfig)
+				if updatePolicy {
 					log.Info("Policy config is updated")
-					bgpServer.UpdatePolicy(newConfig.Policy)
-					policyConfig = &newConfig.Policy
+					p := config.ConfigSetToRoutingPolicy(newConfig)
+					bgpServer.UpdatePolicy(*p)
 				}
+
 			}
 
 			for _, p := range added {
diff --git a/server/server.go b/server/server.go
index cf6f3af9..efcb97bc 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1567,7 +1567,7 @@ func (server *BgpServer) handleModConfig(grpcReq *GrpcRequest) error {
 					},
 				})
 			}
-			b := &config.Bgp{
+			b := &config.BgpConfigSet{
 				Global: config.Global{
 					Config: config.GlobalConfig{
 						As:       g.As,