server: fix to send more appropriate notification code/subcode

admin-down : 6/2
peer-as/neighbor-address : 6/3
other : 6/6

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

3 files changed, 39 insertions, 26 deletions

diff --git a/server/fsm.go b/server/fsm.go
index 3ae70856..618d3b6c 100644
--- a/server/fsm.go
+++ b/server/fsm.go
@@ -282,25 +282,28 @@ func (fsm *FSM) LocalHostPort() (string, uint16) {
 	return hostport(fsm.conn.LocalAddr())
 }
 
-func (fsm *FSM) sendNotificatonFromErrorMsg(conn net.Conn, e *bgp.MessageError) {
-	m := bgp.NewBGPNotificationMessage(e.TypeCode, e.SubTypeCode, e.Data)
-	b, _ := m.Serialize()
-	_, err := conn.Write(b)
-	if err != nil {
-		fsm.bgpMessageStateUpdate(m.Header.Type, false)
+func (fsm *FSM) sendNotificatonFromErrorMsg(e *bgp.MessageError) error {
+	if fsm.h != nil && fsm.h.conn != nil {
+		m := bgp.NewBGPNotificationMessage(e.TypeCode, e.SubTypeCode, e.Data)
+		b, _ := m.Serialize()
+		_, err := fsm.h.conn.Write(b)
+		if err != nil {
+			fsm.bgpMessageStateUpdate(m.Header.Type, false)
+		}
+		fsm.h.conn.Close()
+		log.WithFields(log.Fields{
+			"Topic": "Peer",
+			"Key":   fsm.pConf.Config.NeighborAddress,
+			"Data":  e,
+		}).Warn("sent notification")
+		return nil
 	}
-	conn.Close()
-
-	log.WithFields(log.Fields{
-		"Topic": "Peer",
-		"Key":   fsm.pConf.Config.NeighborAddress,
-		"Data":  e,
-	}).Warn("sent notification")
+	return fmt.Errorf("can't send notification to %s since TCP connection is not established", fsm.pConf.Config.NeighborAddress)
 }
 
-func (fsm *FSM) sendNotification(conn net.Conn, code, subType uint8, data []byte, msg string) {
+func (fsm *FSM) sendNotification(code, subType uint8, data []byte, msg string) error {
 	e := bgp.NewMessageError(code, subType, data, msg)
-	fsm.sendNotificatonFromErrorMsg(conn, e.(*bgp.MessageError))
+	return fsm.sendNotificatonFromErrorMsg(e.(*bgp.MessageError))
 }
 
 func (fsm *FSM) connectLoop() error {
@@ -809,7 +812,7 @@ func (h *FSMHandler) opensent() (bgp.FSMState, FsmStateReason) {
 					body := m.Body.(*bgp.BGPOpen)
 					err := bgp.ValidateOpenMsg(body, fsm.pConf.Config.PeerAs)
 					if err != nil {
-						fsm.sendNotificatonFromErrorMsg(h.conn, err.(*bgp.MessageError))
+						fsm.sendNotificatonFromErrorMsg(err.(*bgp.MessageError))
 						return bgp.BGP_FSM_IDLE, FSM_INVALID_MSG
 					}
 					fsm.peerInfo.ID = body.ID
@@ -879,7 +882,7 @@ func (h *FSMHandler) opensent() (bgp.FSMState, FsmStateReason) {
 					return bgp.BGP_FSM_IDLE, FSM_INVALID_MSG
 				}
 			case *bgp.MessageError:
-				fsm.sendNotificatonFromErrorMsg(h.conn, e.MsgData.(*bgp.MessageError))
+				fsm.sendNotificatonFromErrorMsg(e.MsgData.(*bgp.MessageError))
 				return bgp.BGP_FSM_IDLE, FSM_INVALID_MSG
 			default:
 				log.WithFields(log.Fields{
@@ -893,7 +896,7 @@ func (h *FSMHandler) opensent() (bgp.FSMState, FsmStateReason) {
 			h.conn.Close()
 			return bgp.BGP_FSM_IDLE, err
 		case <-holdTimer.C:
-			fsm.sendNotification(h.conn, bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil, "hold timer expired")
+			fsm.sendNotification(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil, "hold timer expired")
 			h.t.Kill(nil)
 			return bgp.BGP_FSM_IDLE, FSM_HOLD_TIMER_EXPIRED
 		case s := <-fsm.adminStateCh:
@@ -990,7 +993,7 @@ func (h *FSMHandler) openconfirm() (bgp.FSMState, FsmStateReason) {
 				h.conn.Close()
 				return bgp.BGP_FSM_IDLE, FSM_INVALID_MSG
 			case *bgp.MessageError:
-				fsm.sendNotificatonFromErrorMsg(h.conn, e.MsgData.(*bgp.MessageError))
+				fsm.sendNotificatonFromErrorMsg(e.MsgData.(*bgp.MessageError))
 				return bgp.BGP_FSM_IDLE, FSM_INVALID_MSG
 			default:
 				log.WithFields(log.Fields{
@@ -1004,7 +1007,7 @@ func (h *FSMHandler) openconfirm() (bgp.FSMState, FsmStateReason) {
 			h.conn.Close()
 			return bgp.BGP_FSM_IDLE, err
 		case <-holdTimer.C:
-			fsm.sendNotification(h.conn, bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil, "hold timer expired")
+			fsm.sendNotification(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil, "hold timer expired")
 			h.t.Kill(nil)
 			return bgp.BGP_FSM_IDLE, FSM_HOLD_TIMER_EXPIRED
 		case s := <-fsm.adminStateCh:
diff --git a/server/fsm_test.go b/server/fsm_test.go
index 42c61919..f175d4b5 100644
--- a/server/fsm_test.go
+++ b/server/fsm_test.go
@@ -164,6 +164,7 @@ func TestFSMHandlerOpensent_HoldTimerExpired(t *testing.T) {
 
 	// push mock connection
 	p.fsm.conn = m
+	p.fsm.h = h
 
 	// set keepalive ticker
 	p.fsm.pConf.Timers.State.NegotiatedHoldTime = 3
@@ -189,6 +190,7 @@ func TestFSMHandlerOpenconfirm_HoldTimerExpired(t *testing.T) {
 
 	// push mock connection
 	p.fsm.conn = m
+	p.fsm.h = h
 
 	// set up keepalive ticker
 	p.fsm.pConf.Timers.Config.KeepaliveInterval = 1
@@ -213,6 +215,7 @@ func TestFSMHandlerEstablish_HoldTimerExpired(t *testing.T) {
 
 	// push mock connection
 	p.fsm.conn = m
+	p.fsm.h = h
 
 	// set keepalive ticker
 	p.fsm.pConf.Timers.State.NegotiatedHoldTime = 3
@@ -250,6 +253,7 @@ func TestFSMHandlerOpenconfirm_HoldtimeZero(t *testing.T) {
 
 	// push mock connection
 	p.fsm.conn = m
+	p.fsm.h = h
 
 	// set up keepalive ticker
 	p.fsm.pConf.Timers.Config.KeepaliveInterval = 1
@@ -272,6 +276,7 @@ func TestFSMHandlerEstablished_HoldtimeZero(t *testing.T) {
 
 	// push mock connection
 	p.fsm.conn = m
+	p.fsm.h = h
 
 	// set holdtime
 	p.fsm.pConf.Timers.State.NegotiatedHoldTime = 0
diff --git a/server/server.go b/server/server.go
index d8e1d7c8..41e142fd 100644
--- a/server/server.go
+++ b/server/server.go
@@ -2167,7 +2167,7 @@ func (server *BgpServer) handleGrpc(grpcReq *GrpcRequest) []*SenderMsg {
 		}
 		close(grpcReq.ResponseCh)
 	case REQ_DEL_NEIGHBOR:
-		m, err := server.handleDelNeighbor(grpcReq.Data.(*config.Neighbor))
+		m, err := server.handleDelNeighbor(grpcReq.Data.(*config.Neighbor), bgp.BGP_ERROR_CEASE, bgp.BGP_ERROR_SUB_PEER_DECONFIGURED)
 		grpcReq.ResponseCh <- &GrpcResponse{
 			ResponseErr: err,
 		}
@@ -2343,7 +2343,7 @@ func (server *BgpServer) handleAddNeighbor(c *config.Neighbor) ([]*SenderMsg, er
 	return nil, nil
 }
 
-func (server *BgpServer) handleDelNeighbor(c *config.Neighbor) ([]*SenderMsg, error) {
+func (server *BgpServer) handleDelNeighbor(c *config.Neighbor, code, subcode uint8) ([]*SenderMsg, error) {
 	addr := c.Config.NeighborAddress
 	n, y := server.neighborMap[addr]
 	if !y {
@@ -2366,8 +2366,7 @@ func (server *BgpServer) handleDelNeighbor(c *config.Neighbor) ([]*SenderMsg, er
 	delete(server.neighborMap, addr)
 	m := server.dropPeerAllRoutes(n, n.configuredRFlist())
 
-	notification := bgp.NewBGPNotificationMessage(bgp.BGP_ERROR_CEASE, bgp.BGP_ERROR_SUB_PEER_DECONFIGURED, nil)
-	m = append(m, newSenderMsg(n, nil, notification, false))
+	n.fsm.sendNotification(code, subcode, nil, "")
 
 	return m, nil
 }
@@ -2385,7 +2384,13 @@ func (server *BgpServer) handleUpdateNeighbor(c *config.Neighbor) ([]*SenderMsg,
 	original := peer.fsm.pConf
 
 	if !original.Config.Equal(&c.Config) || !original.Transport.Config.Equal(&c.Transport.Config) {
-		msgs, err := server.handleDelNeighbor(peer.fsm.pConf)
+		sub := uint8(bgp.BGP_ERROR_SUB_OTHER_CONFIGURATION_CHANGE)
+		if original.Config.AdminDown != c.Config.AdminDown {
+			sub = bgp.BGP_ERROR_SUB_ADMINISTRATIVE_SHUTDOWN
+		} else if original.Config.PeerAs != c.Config.PeerAs {
+			sub = bgp.BGP_ERROR_SUB_PEER_DECONFIGURED
+		}
+		msgs, err := server.handleDelNeighbor(peer.fsm.pConf, bgp.BGP_ERROR_CEASE, sub)
 		if err != nil {
 			log.WithFields(log.Fields{
 				"Topic": "Peer",
@@ -2535,7 +2540,7 @@ func (server *BgpServer) handleGrpcModNeighbor(grpcReq *GrpcRequest) ([]*SenderM
 			Config: config.NeighborConfig{
 				NeighborAddress: arg.Peer.Conf.NeighborAddress,
 			},
-		})
+		}, bgp.BGP_ERROR_CEASE, bgp.BGP_ERROR_SUB_PEER_DECONFIGURED)
 	default:
 		return nil, fmt.Errorf("unsupported operation %s", arg.Operation)
 	}