Cleanup code and address @BarbarossaTM pull request remarks

- Remove Makefile for adding it later in a seperate PR
- Remove main files which where added by accident
- Add constants for IPv4 and IPv6
- Improve log messages
- Simplify netlink_reader addPathsToClients and removePathsFromClients
- Refactor Route's Equal function
- Return false for ECMP in netlink_path since ecmp is currently not
implemented in netlink

Makefile

-NAME=bio-rd
-
-all: test
-
-$(NAME): gazelle
-	bazel build //:bio-rd
-
-gazelle:
-	bazel run //:gazelle -- update
-
-test: $(NAME)
-	bazel test //...
-
-dep:
-	bazel build //vendor/github.com/golang/dep/cmd/dep
-
-dep-clean:
-	# hack: dep of dep gives us these, and it breaks gazelle
-	rm -rf vendor/github.com/golang/dep/cmd/dep/testdata
-	rm -rf vendor/github.com/golang/dep/internal/fs/testdata/symlinks/dir-symlink
-
-clean: dep-clean
-	bazel clean
-	rm $(NAME)
-
-.PHONY: $(NAME) gazelle clean dep dep-clean

main.go

-package main
-
-import (
-	"net"
-	"os"
-	"time"
-
-	"github.com/bio-routing/bio-rd/config"
-	"github.com/bio-routing/bio-rd/protocols/bgp/server"
-	"github.com/bio-routing/bio-rd/protocols/netlink"
-	"github.com/bio-routing/bio-rd/routingtable/locRIB"
-	log "github.com/sirupsen/logrus"
-
-	bnet "github.com/bio-routing/bio-rd/net"
-)
-
-func strAddr(s string) uint32 {
-	ret, _ := bnet.StrToAddr(s)
-	return ret
-}
-
-func main() {
-	log.SetLevel(log.DebugLevel)
-
-	f, err := os.OpenFile("/var/log/bio-rd.log", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
-	if err != nil {
-		log.Fatalf("error opening file: %v", err)
-	}
-	defer f.Close()
-
-	log.SetOutput(f)
-
-	log.Info("bio-routing started...\n")
-
-	cfg := &config.Global{
-		Listen: true,
-		LocalAddressList: []net.IP{
-			net.IPv4(169, 254, 0, 2),
-		},
-	}
-
-	rib := locRIB.New()
-	b := server.NewBgpServer()
-	startBGPServer(b, rib, cfg)
-
-	// Netlink communication
-	n := protocolnetlink.NewNetlink(&config.Netlink{
-		HoldTime:       time.Second * 15,
-		UpdateInterval: time.Second * 15,
-		RoutingTable:   config.RtMain,
-	}, rib)
-	n.Start()
-
-	go func() {
-		for {
-			log.Debugf("LocRIB count: %d", rib.Count())
-			log.Debugf(rib.Print())
-			time.Sleep(time.Second * 10)
-		}
-	}()
-
-	select {}
-}

main_ipv4.go

-package main
-
-import (
-	"time"
-
-	"github.com/bio-routing/bio-rd/routingtable"
-	"github.com/bio-routing/bio-rd/routingtable/locRIB"
-
-	"github.com/bio-routing/bio-rd/config"
-	"github.com/bio-routing/bio-rd/protocols/bgp/server"
-	"github.com/bio-routing/bio-rd/routingtable/filter"
-	log "github.com/sirupsen/logrus"
-
-	bnet "github.com/bio-routing/bio-rd/net"
-)
-
-func startBGPServer(b server.BGPServer, rib *locRIB.LocRIB, cfg *config.Global) {
-	err := b.Start(cfg)
-	if err != nil {
-		log.Fatalf("Unable to start BGP server: %v", err)
-	}
-
-	b.AddPeer(config.Peer{
-		AdminEnabled:      true,
-		LocalAS:           65200,
-		PeerAS:            65100,
-		PeerAddress:       bnet.IPv4FromOctets(169, 254, 0, 1),
-		LocalAddress:      bnet.IPv4FromOctets(169, 254, 0, 2),
-		ReconnectInterval: time.Second * 20,
-		HoldTime:          time.Second * 20,
-		KeepAlive:         time.Second * 20,
-		Passive:           false,
-		RouterID:          b.RouterID(),
-
-		//AddPathSend: routingtable.ClientOptions{
-		//	MaxPaths: 10,
-		//},
-		//RouteServerClient: true,
-		IPv4: &config.AddressFamilyConfig{
-			RIB:          rib,
-			ImportFilter: filter.NewAcceptAllFilter(),
-			ExportFilter: filter.NewAcceptAllFilter(),
-			AddPathSend: routingtable.ClientOptions{
-				MaxPaths: 10,
-			},
-			AddPathRecv: true,
-		},
-	})
-}

main_ipv6.go

-// +build ipv6
-
-package main
-
-import (
-	"net"
-	"time"
-
-	"github.com/bio-routing/bio-rd/config"
-	"github.com/bio-routing/bio-rd/protocols/bgp/server"
-	"github.com/bio-routing/bio-rd/routingtable"
-	"github.com/bio-routing/bio-rd/routingtable/filter"
-	"github.com/bio-routing/bio-rd/routingtable/locRIB"
-	"github.com/sirupsen/logrus"
-
-	bnet "github.com/bio-routing/bio-rd/net"
-)
-
-func startServer(b server.BGPServer, rib *locRIB.LocRIB) {
-
-	err := b.Start(&config.Global{
-		Listen: true,
-		LocalAddressList: []net.IP{
-			net.IP{0x20, 0x01, 0x6, 0x78, 0x1, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0, 0xca, 0xfe},
-		},
-	})
-	if err != nil {
-		logrus.Fatalf("Unable to start BGP server: %v", err)
-	}
-
-	b.AddPeer(config.Peer{
-		AdminEnabled:      true,
-		LocalAS:           65200,
-		PeerAS:            202739,
-		PeerAddress:       bnet.IPv6FromBlocks(0x2001, 0x678, 0x1e0, 0, 0, 0, 0, 1),
-		LocalAddress:      bnet.IPv6FromBlocks(0x2001, 0x678, 0x1e0, 0, 0, 0, 0, 0xcafe),
-		ReconnectInterval: time.Second * 15,
-		HoldTime:          time.Second * 90,
-		KeepAlive:         time.Second * 30,
-		Passive:           true,
-		RouterID:          b.RouterID(),
-		IPv6: &config.AddressFamilyConfig{
-			RIB:          rib,
-			ImportFilter: filter.NewAcceptAllFilter(),
-			ExportFilter: filter.NewDrainFilter(),
-			AddPathSend: routingtable.ClientOptions{
-				BestOnly: true,
-			},
-		},
-	})
-
-	b.AddPeer(config.Peer{
-		AdminEnabled:      true,
-		LocalAS:           65200,
-		PeerAS:            65400,
-		PeerAddress:       bnet.IPv6FromBlocks(0x2001, 0x678, 0x1e0, 0xcafe, 0, 0, 0, 5),
-		LocalAddress:      bnet.IPv6FromBlocks(0x2001, 0x678, 0x1e0, 0, 0, 0, 0, 0xcafe),
-		ReconnectInterval: time.Second * 15,
-		HoldTime:          time.Second * 90,
-		KeepAlive:         time.Second * 30,
-		Passive:           true,
-		RouterID:          b.RouterID(),
-		IPv6: &config.AddressFamilyConfig{
-			RIB:          rib,
-			ImportFilter: filter.NewDrainFilter(),
-			ExportFilter: filter.NewAcceptAllFilter(),
-			AddPathSend: routingtable.ClientOptions{
-				BestOnly: true,
-			},
-		},
-	})
-}

protocols/netlink/netlink.go

@@ -37,9 +37,9 @@ func (n *Netlink) Start() {
 	// 1. from locRib to Kernel
 	n.locRib.ClientManager.RegisterWithOptions(n.writer, options)
 
-	// 2. vom Kernel to locRib
+	// 2. from Kernel to locRib
 	n.reader.ClientManager.RegisterWithOptions(n.locRib, options)
 
-	// Listn for new routes from kernel
+	// Listen for new routes from kernel
 	go n.reader.Read()
 }

protocols/netlink/netlink_reader.go

@@ -14,6 +14,12 @@ import (
 	"github.com/vishvananda/netlink"
 )
 
+// Constants for IP family
+const (
+	IPFamily4 = 4 // IPv4
+	IPFamily6 = 6 // IPv6
+)
+
 // NetlinkReader read routes from the Linux Kernel and propagates it to the locRIB
 type NetlinkReader struct {
 	options *config.Netlink
@@ -45,7 +51,7 @@ func (nr *NetlinkReader) Read() {
 
 	for {
 		// Family doesn't matter. I only filter by the rt_table here
-		routes, err := netlink.RouteListFiltered(4, &netlink.Route{Table: nr.options.RoutingTable}, netlink.RT_FILTER_TABLE)
+		routes, err := netlink.RouteListFiltered(IPFamily4, &netlink.Route{Table: nr.options.RoutingTable}, netlink.RT_FILTER_TABLE)
 		if err != nil {
 			log.WithError(err).Panic("Failed to read routes from kernel")
 		}
@@ -84,9 +90,7 @@ func (nr *NetlinkReader) propagateChanges(routes []netlink.Route) {
 
 // Add given paths to clients
 func (nr *NetlinkReader) addPathsToClients(routes []netlink.Route) {
-	for _, client := range nr.ClientManager.Clients() {
 	// only advertise changed routes
-
 	nr.mu.RLock()
 	advertise := route.NetlinkRouteDiff(routes, nr.routes)
 	nr.mu.RUnlock()
@@ -102,7 +106,10 @@ func (nr *NetlinkReader) addPathsToClients(routes []netlink.Route) {
 		pfx := bnet.NewPfxFromIPNet(r.Dst)
 		path, err := createPathFromRoute(&r)
 		if err != nil {
-				log.WithError(err).Error("Unable to create path")
+			log.WithError(err).WithFields(log.Fields{
+				"prefix": pfx.String(),
+				"path":   path.String(),
+			}).Error("Unable to create path")
 			continue
 		}
 
@@ -112,11 +119,16 @@ func (nr *NetlinkReader) addPathsToClients(routes []netlink.Route) {
 			// TODO: Implement filter that cann handle netlinkRoute objects
 			path, reject = nr.filter.ProcessTerms(pfx, path)
 			if reject {
-					log.Debug("Skipping route due to filter")
+				log.WithError(err).WithFields(log.Fields{
+					"prefix": pfx.String(),
+					"path":   path.String(),
+				}).Debug("Skipping route due to filter")
+
 				continue
 			}
 		}
 
+		for _, client := range nr.ClientManager.Clients() {
 			log.WithFields(log.Fields{
 				"pfx":  pfx,
 				"path": path,
@@ -128,12 +140,15 @@ func (nr *NetlinkReader) addPathsToClients(routes []netlink.Route) {
 
 // Remove given paths from clients
 func (nr *NetlinkReader) removePathsFromClients(routes []netlink.Route) {
-	for _, client := range nr.ClientManager.Clients() {
-		// If there where no routes yet, just skip this funktion. There's nothing to delete
 	nr.mu.RLock()
-		if len(nr.routes) == 0 {
+
+	// get the number of routes
+	routeLength := len(nr.routes)
+
+	// If there where no routes yet, just skip this funktion. There's nothing to delete
+	if routeLength == 0 {
 		nr.mu.RUnlock()
-			break
+		return
 	}
 
 	// only withdraw changed routes
@@ -164,6 +179,7 @@ func (nr *NetlinkReader) removePathsFromClients(routes []netlink.Route) {
 			}
 		}
 
+		for _, client := range nr.ClientManager.Clients() {
 			log.WithFields(log.Fields{
 				"pfx":  pfx,
 				"path": path,

route/netlink_path.go

@@ -133,7 +133,7 @@ func (s *NetlinkPath) Select(t *NetlinkPath) int8 {
 
 // ECMP determines if path s and t are equal in terms of ECMP
 func (s *NetlinkPath) ECMP(t *NetlinkPath) bool {
-	return true
+	return false
 }
 
 // Copy duplicates the current object

route/route.go

@@ -199,36 +199,38 @@ func (r *Route) PathSelection() {
 	r.updateEqualPathCount()
 }
 
-// Euql compares Are two routes and return true if they are equal
+// Equal compares if two routes are the same
 func (r *Route) Equal(other *Route) bool {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
-	a := r.pfx.Equal(other.pfx)
-	b := r.ecmpPaths == other.ecmpPaths
-	c := true
+	pfxEqual := r.pfx.Equal(other.pfx)
+	ecmpPathsEqual := r.ecmpPaths == other.ecmpPaths
+	pathsEqual := comparePathSlice(r.paths, other.paths)
 
-	if r.paths == nil && other.paths == nil {
-		c = true
-		return a && b && c
+	return pfxEqual && ecmpPathsEqual && pathsEqual
 }
 
-	if len(r.paths) != len(other.paths) {
-		c = false
-		return a && b && c
+// Compare two path pointer slices if they are equal
+func comparePathSlice(left, right []*Path) bool {
+	if left == nil && right == nil {
+		return true
+	}
+
+	if len(left) != len(right) {
+		return false
 	}
 
-	for _, myP := range r.paths {
-		if !r.compareItemExists(myP, other.paths) {
-			c = false
-			return a && b && c
+	for _, leftPath := range left {
+		if !compareItemExists(leftPath, right) {
+			return false
 		}
 	}
 
-	return a && b && c
+	return true
 }
 
-func (r *Route) compareItemExists(needle *Path, haystack []*Path) bool {
+func compareItemExists(needle *Path, haystack []*Path) bool {
 	for _, compare := range haystack {
 		if needle.Equal(compare) {
 			return true