zebra: Introducing MIN_ZAPIVER and MAX_ZAPIVER. And avoiding double close channel when sequential retry to connect zebra.

- Introducing MIN_ZAPIVER and MAX_ZAPIVER for avoiding editing files except for zapi.go when new ZAPI version will be supported in future.
- Fix a bug avoiding panic by double close for a channel.
- Changing algorithm for sequential retry to connect zebra.

3 files changed, 65 insertions, 15 deletions

diff --git a/internal/pkg/config/default.go b/internal/pkg/config/default.go
index 08f9f865..13e1267c 100644
--- a/internal/pkg/config/default.go
+++ b/internal/pkg/config/default.go
@@ -8,6 +8,7 @@ import (
 	"reflect"
 	"strconv"
 
+	"github.com/osrg/gobgp/internal/pkg/zebra"
 	"github.com/osrg/gobgp/pkg/packet/bgp"
 	"github.com/osrg/gobgp/pkg/packet/bmp"
 	"github.com/osrg/gobgp/pkg/packet/rtr"
@@ -400,10 +401,10 @@ func setDefaultConfigValuesWithViper(v *viper.Viper, b *BgpConfigSet) error {
 	if b.Zebra.Config.Url == "" {
 		b.Zebra.Config.Url = "unix:/var/run/quagga/zserv.api"
 	}
-	if b.Zebra.Config.Version < 2 {
-		b.Zebra.Config.Version = 2
-	} else if b.Zebra.Config.Version > 6 {
-		b.Zebra.Config.Version = 6
+	if b.Zebra.Config.Version < zebra.MinZapiVer {
+		b.Zebra.Config.Version = zebra.MinZapiVer
+	} else if b.Zebra.Config.Version > zebra.MaxZapiVer {
+		b.Zebra.Config.Version = zebra.MaxZapiVer
 	}
 	if !v.IsSet("zebra.config.nexthop-trigger-enable") && !b.Zebra.Config.NexthopTriggerEnable && b.Zebra.Config.Version > 2 {
 		b.Zebra.Config.NexthopTriggerEnable = true
diff --git a/internal/pkg/zebra/zapi.go b/internal/pkg/zebra/zapi.go
index b0f8cc3a..e4391ca6 100644
--- a/internal/pkg/zebra/zapi.go
+++ b/internal/pkg/zebra/zapi.go
@@ -17,6 +17,7 @@ package zebra
 
 import (
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"io"
 	"math"
@@ -35,6 +36,11 @@ const (
 	INTERFACE_NAMSIZ  = 20
 )
 
+const (
+	MinZapiVer uint8 = 2
+	MaxZapiVer uint8 = 6
+)
+
 // Internal Interface Status.
 type INTERFACE_STATUS uint8
 
@@ -970,10 +976,10 @@ func NewClient(network, address string, typ ROUTE_TYPE, version uint8) (*Client,
 	}
 	outgoing := make(chan *Message)
 	incoming := make(chan *Message, 64)
-	if version < 2 {
-		version = 2
-	} else if version > 6 {
-		version = 6
+	if version < MinZapiVer {
+		version = MinZapiVer
+	} else if version > MaxZapiVer {
+		version = MaxZapiVer
 	}
 
 	c := &Client{
@@ -1001,7 +1007,8 @@ func NewClient(network, address string, typ ROUTE_TYPE, version uint8) (*Client,
 					log.WithFields(log.Fields{
 						"Topic": "Zebra",
 					}).Errorf("failed to write: %s", err)
-					close(outgoing)
+					ChannelClose(outgoing)
+					return
 				}
 			} else {
 				log.Debug("finish outgoing loop")
@@ -1026,6 +1033,12 @@ func NewClient(network, address string, typ ROUTE_TYPE, version uint8) (*Client,
 
 		hd := &Header{}
 		err = hd.DecodeFromBytes(headerBuf)
+		if c.Version != hd.Version {
+			log.WithFields(log.Fields{
+				"Topic": "Zebra",
+			}).Warnf("ZAPI version mismatch. configured version: %d, version of received message:%d", c.Version, hd.Version)
+			return nil, errors.New("ZAPI version mismatch")
+		}
 		if err != nil {
 			log.WithFields(log.Fields{
 				"Topic": "Zebra",
@@ -1077,7 +1090,7 @@ func NewClient(network, address string, typ ROUTE_TYPE, version uint8) (*Client,
 
 	// Start receive loop only when the first message successfully received.
 	go func() {
-		defer close(incoming)
+		defer ChannelClose(incoming)
 		for {
 			if m, err := receiveSingleMsg(); err != nil {
 				return
@@ -1298,8 +1311,21 @@ func (c *Client) SendNexthopRegister(vrfId uint32, body *NexthopRegisterBody, is
 	return c.SendCommand(command, vrfId, body)
 }
 
+// for avoiding double close
+func ChannelClose(ch chan *Message) bool {
+	select {
+	case _, ok := <-ch:
+		if ok {
+			close(ch)
+			return true
+		}
+	default:
+	}
+	return false
+}
+
 func (c *Client) Close() error {
-	close(c.outgoing)
+	ChannelClose(c.outgoing)
 	return c.conn.Close()
 }
 
diff --git a/pkg/server/zclient.go b/pkg/server/zclient.go
index a814e20a..3e4fb3e6 100644
--- a/pkg/server/zclient.go
+++ b/pkg/server/zclient.go
@@ -341,6 +341,9 @@ func (z *zebraClient) loop() {
 		case <-z.dead:
 			return
 		case msg := <-z.client.Receive():
+			if msg == nil {
+				break
+			}
 			switch body := msg.Body.(type) {
 			case *zebra.IPRouteBody:
 				if path := newPathFromIPRouteMessage(msg, z.client.Version); path != nil {
@@ -419,19 +422,39 @@ func newZebraClient(s *BgpServer, url string, protos []string, version uint8, nh
 	}
 	var cli *zebra.Client
 	var err error
-	for _, ver := range []uint8{version, 2, 3, 4, 5, 6} {
+	var usingVersion uint8
+	var zapivers [zebra.MaxZapiVer - zebra.MinZapiVer + 1]uint8
+	zapivers[0] = version
+	for elem, ver := 1, zebra.MinZapiVer; elem < len(zapivers) && ver <= zebra.MaxZapiVer; elem++ {
+		if version == ver && ver < zebra.MaxZapiVer {
+			ver++
+		}
+		zapivers[elem] = ver
+		ver++
+	}
+	for elem, ver := range zapivers {
 		cli, err = zebra.NewClient(l[0], l[1], zebra.ROUTE_BGP, ver)
-		if err == nil {
+		if cli != nil && err == nil {
+			usingVersion = ver
 			break
 		}
 		// Retry with another Zebra message version
 		log.WithFields(log.Fields{
 			"Topic": "Zebra",
-		}).Warnf("cannot connect to Zebra with message version %d. going to retry another version...", ver)
+		}).Warnf("cannot connect to Zebra with message version %d.", ver)
+		if elem < len(zapivers)-1 {
+			log.WithFields(log.Fields{
+				"Topic": "Zebra",
+			}).Warnf("going to retry another version %d.", zapivers[elem+1])
+		}
 	}
-	if cli == nil {
+	if cli == nil || err != nil {
 		return nil, err
 	}
+	log.WithFields(log.Fields{
+		"Topic": "Zebra",
+	}).Infof("success to connect to Zebra with message version %d.", usingVersion)
+
 	// Note: HELLO/ROUTER_ID_ADD messages are automatically sent to negotiate
 	// the Zebra message version in zebra.NewClient().
 	// cli.SendHello()