peer: support hold time 0

4 files changed, 119 insertions, 19 deletions

diff --git a/server/fsm.go b/server/fsm.go
index f66e8f57..22744ada 100644
--- a/server/fsm.go
+++ b/server/fsm.go
@@ -487,6 +487,13 @@ func (h *FSMHandler) openconfirm() bgp.FSMState {
 	// sets the HoldTimer according to the negotiated value
 	h.holdTimer = time.NewTimer(time.Second * time.Duration(fsm.negotiatedHoldTime))
 
+	log.Debug("negotiatedHoldTime : ", fsm.negotiatedHoldTime)
+	var isHoldtimeZero bool = fsm.negotiatedHoldTime == float64(0)
+	if isHoldtimeZero {
+		fsm.keepaliveTicker.Stop()
+		h.holdTimer.Stop()
+	}
+
 	for {
 		select {
 		case <-h.t.Dying():
@@ -499,11 +506,15 @@ func (h *FSMHandler) openconfirm() bgp.FSMState {
 				"Key":   fsm.peerConfig.NeighborAddress,
 			}).Warn("Closed an accepted connection")
 		case <-fsm.keepaliveTicker.C:
-			m := bgp.NewBGPKeepAliveMessage()
-			b, _ := m.Serialize()
-			// TODO: check error
-			fsm.passiveConn.Write(b)
-			fsm.bgpMessageStateUpdate(m.Header.Type, false)
+			if !isHoldtimeZero {
+				m := bgp.NewBGPKeepAliveMessage()
+				b, _ := m.Serialize()
+				// TODO: check error
+				fsm.passiveConn.Write(b)
+				fsm.bgpMessageStateUpdate(m.Header.Type, false)
+			} else {
+				log.Debug("keepaliveTicker expired, but negotiatedHoldTime is zero")
+			}
 		case e := <-h.msgCh:
 			switch e.MsgData.(type) {
 			case *bgp.BGPMessage:
@@ -530,9 +541,13 @@ func (h *FSMHandler) openconfirm() bgp.FSMState {
 			h.conn.Close()
 			return bgp.BGP_FSM_IDLE
 		case <-h.holdTimer.C:
-			fsm.sendNotification(h.conn, bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil, "hold timer expired")
-			h.t.Kill(nil)
-			return bgp.BGP_FSM_IDLE
+			if !isHoldtimeZero {
+				fsm.sendNotification(h.conn, bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil, "hold timer expired")
+				h.t.Kill(nil)
+				return bgp.BGP_FSM_IDLE
+			} else {
+				log.Debug("holdTimer expired, but negotiatedHoldTime is zero")
+			}
 		case s := <-fsm.adminStateCh:
 			err := h.changeAdminState(s)
 			if err == nil {
@@ -625,6 +640,10 @@ func (h *FSMHandler) established() bgp.FSMState {
 
 	// restart HoldTimer
 	h.holdTimer = time.NewTimer(time.Second * time.Duration(fsm.negotiatedHoldTime))
+	var isHoldtimeZero bool = fsm.negotiatedHoldTime == float64(0)
+	if isHoldtimeZero {
+		h.holdTimer.Stop()
+	}
 
 	for {
 		select {
@@ -641,14 +660,18 @@ func (h *FSMHandler) established() bgp.FSMState {
 			h.t.Kill(nil)
 			return bgp.BGP_FSM_IDLE
 		case <-h.holdTimer.C:
-			log.WithFields(log.Fields{
-				"Topic": "Peer",
-				"Key":   fsm.peerConfig.NeighborAddress,
-				"data":  bgp.BGP_FSM_ESTABLISHED,
-			}).Warn("hold timer expired")
-			m := bgp.NewBGPNotificationMessage(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil)
-			h.outgoing <- m
-			return bgp.BGP_FSM_IDLE
+			if !isHoldtimeZero {
+				log.WithFields(log.Fields{
+					"Topic": "Peer",
+					"Key":   fsm.peerConfig.NeighborAddress,
+					"data":  bgp.BGP_FSM_ESTABLISHED,
+				}).Warn("hold timer expired")
+				m := bgp.NewBGPNotificationMessage(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil)
+				h.outgoing <- m
+				return bgp.BGP_FSM_IDLE
+			} else {
+				log.Debug("holdTimer expired, but negotiatedHoldTime is zero")
+			}
 		case s := <-fsm.adminStateCh:
 			err := h.changeAdminState(s)
 			if err == nil {
diff --git a/server/fsm_test.go b/server/fsm_test.go
index 21659600..c0159d22 100644
--- a/server/fsm_test.go
+++ b/server/fsm_test.go
@@ -17,6 +17,7 @@ package server
 
 import (
 	"fmt"
+	log "github.com/Sirupsen/logrus"
 	"github.com/osrg/gobgp/config"
 	"github.com/osrg/gobgp/packet"
 	"github.com/stretchr/testify/assert"
@@ -39,7 +40,7 @@ type MockConnection struct {
 func NewMockConnection() *MockConnection {
 	m := &MockConnection{
 		recvCh:   make(chan chan byte, 128),
-		sendBuf:  make([][]byte, 128),
+		sendBuf:  make([][]byte, 0),
 		isClosed: false,
 	}
 	return m
@@ -236,6 +237,54 @@ func TestFSMHandlerEstablish_HoldTimerExpired(t *testing.T) {
 	assert.Equal(uint8(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED), sent.Body.(*bgp.BGPNotification).ErrorCode)
 }
 
+func TestFSMHandlerOpenconfirm_HoldtimeZero(t *testing.T) {
+	log.SetLevel(log.DebugLevel)
+	assert := assert.New(t)
+	m := NewMockConnection()
+
+	p, h := makePeerAndHandler()
+
+	// push mock connection
+	p.fsm.passiveConn = m
+
+	// set up keepalive ticker
+	p.fsm.peerConfig.Timers.KeepaliveInterval = 1
+	// set holdtime
+	p.fsm.negotiatedHoldTime = 0
+	go h.openconfirm()
+
+	time.Sleep(100 * time.Millisecond)
+
+	assert.False(h.holdTimer.Stop())
+	assert.Equal(0, len(m.sendBuf))
+
+}
+
+func TestFSMHandlerEstablished_HoldtimeZero(t *testing.T) {
+	log.SetLevel(log.DebugLevel)
+	assert := assert.New(t)
+	m := NewMockConnection()
+
+	p, h := makePeerAndHandler()
+
+	// push mock connection
+	p.fsm.passiveConn = m
+
+	// set up keepalive ticker
+	sec := time.Second * 1
+	p.fsm.keepaliveTicker = time.NewTicker(sec)
+	p.fsm.keepaliveTicker.Stop()
+
+	// set holdtime
+	p.fsm.negotiatedHoldTime = 0
+	go h.established()
+
+	time.Sleep(100 * time.Millisecond)
+
+	assert.False(h.holdTimer.Stop())
+	assert.Equal(0, len(m.sendBuf))
+}
+
 func makePeerAndHandler() (*Peer, *FSMHandler) {
 	globalConfig := config.GlobalType{}
 	neighborConfig := config.NeighborType{}
diff --git a/server/peer.go b/server/peer.go
index 61a229ee..c635b2b1 100644
--- a/server/peer.go
+++ b/server/peer.go
@@ -127,9 +127,9 @@ func (peer *Peer) handleBGPmessage(m *bgp.BGPMessage) {
 		holdTime := float64(body.HoldTime)
 		myHoldTime := peer.fsm.peerConfig.Timers.HoldTime
 		if holdTime > myHoldTime {
-			peer.fsm.negotiatedHoldTime = holdTime
-		} else {
 			peer.fsm.negotiatedHoldTime = myHoldTime
+		} else {
+			peer.fsm.negotiatedHoldTime = holdTime
 		}
 
 	case bgp.BGP_MSG_ROUTE_REFRESH:
diff --git a/server/peer_test.go b/server/peer_test.go
index 9e406e3e..9aa892dd 100644
--- a/server/peer_test.go
+++ b/server/peer_test.go
@@ -429,6 +429,34 @@ func TestPeerAdminShutdownReject(t *testing.T) {
 
 }
 
+func TestPeerSelectSmallerHoldtime(t *testing.T) {
+	log.SetLevel(log.DebugLevel)
+	assert := assert.New(t)
+	m := NewMockConnection()
+
+	globalConfig := config.GlobalType{}
+	peerConfig := config.NeighborType{}
+	peerConfig.PeerAs = 65001
+	peerConfig.Timers.KeepaliveInterval = 5
+	peer := makePeer(globalConfig, peerConfig)
+	peer.fsm.opensentHoldTime = 1
+	peerConfig.Timers.HoldTime = 5
+	peer.t.Go(peer.loop)
+
+	pushPackets := func() {
+		opn := bgp.NewBGPOpenMessage(65001, 0, "10.0.0.1", []bgp.OptionParameterInterface{})
+		o, _ := opn.Serialize()
+		m.setData(o)
+	}
+	go pushPackets()
+
+	waitUntil(assert, bgp.BGP_FSM_ACTIVE, peer, 1000)
+	peer.acceptedConnCh <- m
+	waitUntil(assert, bgp.BGP_FSM_OPENCONFIRM, peer, 1000)
+
+	assert.Equal(float64(0), peer.fsm.negotiatedHoldTime)
+}
+
 func assertCounter(assert *assert.Assertions, counter config.BgpNeighborCommonStateType) {
 	assert.Equal(uint32(0), counter.OpenIn)
 	assert.Equal(uint32(0), counter.OpenOut)