Fixing update sender

protocols/bgp/packet/bgp.go

@@ -10,6 +10,7 @@ const (
 	HeaderLen         = 19
 	MinLen            = 19
 	MaxLen            = 4096
+	MinUpdateLen      = 4
 	NLRIMaxLen        = 5
 	CommunityLen      = 4
 	LargeCommunityLen = 12

protocols/bgp/server/BUILD.bazel

@@ -47,6 +47,7 @@ go_test(
         "fsm_test.go",
         "server_test.go",
         "update_helper_test.go",
+        "update_sender_test.go",
         "withdraw_test.go",
     ],
     embed = [":go_default_library"],

protocols/bgp/server/fsm_established.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"net"
+	"time"
 
 	bnet "github.com/bio-routing/bio-rd/net"
 	"github.com/bio-routing/bio-rd/protocols/bgp/packet"
@@ -85,14 +86,10 @@ func (s *establishedState) init() error {
 	clientOptions := routingtable.ClientOptions{
 		BestOnly: true,
 	}
-	if s.fsm.options.AddPathRX {
-		s.fsm.updateSender = newUpdateSenderAddPath(s.fsm)
-		clientOptions = s.fsm.peer.addPathSend
-	} else {
-		s.fsm.updateSender = newUpdateSender(s.fsm)
-	}
 
-	s.fsm.updateSender.Start()
+	s.fsm.updateSender = newUpdateSender(s.fsm)
+	s.fsm.updateSender.Start(time.Millisecond * 5)
+
 	s.fsm.adjRIBOut.Register(s.fsm.updateSender)
 	s.fsm.rib.RegisterWithOptions(s.fsm.adjRIBOut, clientOptions)
 

protocols/bgp/server/update_sender.go

@@ -4,12 +4,12 @@ import (
 	"sync"
 	"time"
 
-	log "github.com/sirupsen/logrus"
-
-	bnet "github.com/bio-routing/bio-rd/net"
 	"github.com/bio-routing/bio-rd/protocols/bgp/packet"
 	"github.com/bio-routing/bio-rd/route"
 	"github.com/bio-routing/bio-rd/routingtable"
+
+	bnet "github.com/bio-routing/bio-rd/net"
+	log "github.com/sirupsen/logrus"
 )
 
 // UpdateSender converts table changes into BGP update messages
@@ -17,7 +17,6 @@ type UpdateSender struct {
 	routingtable.ClientManager
 	fsm       *FSM
 	iBGP      bool
-	addPath   bool
 	toSendMu  sync.Mutex
 	toSend    map[string]*pathPfxs
 	destroyCh chan struct{}
@@ -28,12 +27,6 @@ type pathPfxs struct {
 	pfxs []bnet.Prefix
 }
 
-func newUpdateSenderAddPath(fsm *FSM) *UpdateSender {
-	u := newUpdateSender(fsm)
-	u.addPath = true
-	return u
-}
-
 func newUpdateSender(fsm *FSM) *UpdateSender {
 	return &UpdateSender{
 		fsm:       fsm,
@@ -44,8 +37,8 @@ func newUpdateSender(fsm *FSM) *UpdateSender {
 }
 
 // Start starts the update sender
-func (u *UpdateSender) Start() {
-	go u.sender()
+func (u *UpdateSender) Start(aggrTime time.Duration) {
+	go u.sender(aggrTime)
 }
 
 // Destroy destroys everything (with greetings to Hatebreed)
@@ -76,8 +69,8 @@ func (u *UpdateSender) AddPath(pfx bnet.Prefix, p *route.Path) error {
 }
 
 // sender serializes BGP update messages
-func (u *UpdateSender) sender() {
-	ticker := time.NewTicker(time.Millisecond * 5)
+func (u *UpdateSender) sender(aggrTime time.Duration) {
+	ticker := time.NewTicker(aggrTime)
 	var err error
 	var pathAttrs *packet.PathAttribute
 	var budget int
@@ -92,24 +85,28 @@ func (u *UpdateSender) sender() {
 		u.toSendMu.Lock()
 
 		for key, pathNLRIs := range u.toSend {
-			budget = packet.MaxLen - int(pathNLRIs.path.BGPPath.Length())
+			budget = packet.MaxLen - packet.HeaderLen - packet.MinUpdateLen - int(pathNLRIs.path.BGPPath.Length())
 			pathAttrs, err = packet.PathAttributes(pathNLRIs.path)
 			if err != nil {
 				log.Errorf("Unable to get path attributes: %v", err)
 				continue
 			}
 
-			updatesPrefixes := make([][]bnet.Prefix, 1)
-			prefixes := make([]bnet.Prefix, 1)
+			updatesPrefixes := make([][]bnet.Prefix, 0, 1)
+			prefixes := make([]bnet.Prefix, 0, 1)
 			for _, pfx := range pathNLRIs.pfxs {
-				budget -= int(packet.BytesInAddr(pfx.Pfxlen())) - 5
+				budget -= int(packet.BytesInAddr(pfx.Pfxlen())) + 1
 				if budget < 0 {
 					updatesPrefixes = append(updatesPrefixes, prefixes)
-					prefixes = make([]bnet.Prefix, 1)
+					prefixes = make([]bnet.Prefix, 0, 1)
+					budget = packet.MaxLen - int(pathNLRIs.path.BGPPath.Length())
 				}
 
 				prefixes = append(prefixes, pfx)
 			}
+			if len(prefixes) > 0 {
+				updatesPrefixes = append(updatesPrefixes, prefixes)
+			}
 
 			delete(u.toSend, key)
 			u.toSendMu.Unlock()
@@ -150,7 +147,11 @@ func (u *UpdateSender) sendUpdates(pathAttrs *packet.PathAttribute, updatePrefix
 // RemovePath withdraws prefix `pfx` from a peer
 func (u *UpdateSender) RemovePath(pfx bnet.Prefix, p *route.Path) bool {
 	err := withDrawPrefixesAddPath(u.fsm.con, u.fsm.options, pfx, p)
-	return err == nil
+	if err != nil {
+		log.Errorf("Unable to withdraw prefix: %v", err)
+		return false
+	}
+	return true
 }
 
 // UpdateNewClient does nothing

testing/conn_mock.go

 package testing
 
-import "net"
+import (
+	"net"
+)
 
 // MockConn mock an connection
 type MockConn struct {
@@ -27,6 +29,6 @@ func (m *MockConn) Read(b []byte) (n int, err error) {
 		count = len(m.Bytes)
 	}
 
-	copy(m.Bytes[0:count], b)
+	copy(b, m.Bytes[0:count])
 	return count, nil
 }