Repository: cloudnativelabs/kube-router
Pull request #1755 (2024-10-20 22:25:38)

Fix various issues with refactors
@mrueg @twz123 @rbrtbnfgl

Fixes a couple of things:

* During one of the recent refactors, we lost the fact that the router-id for BGP should default to the IPv4 address when it isn't otherwise specified
* Fixes a potential for the IP addresses on nodes to not obey the ordering of internal first then external. This mostly impacted tests, but would have eventually impacted users as well once a release was createdc
* Removes a potential race condition from LoadBalancerAllocator tests
* Reduces the total unit test run times by setting `t.Parrallel()` on ecmp_vip_test.go which was the longest test in the suite
  * It would be nice to set this on more tests in the `routing` suite, but unfortunately, we can't use it any time we actually start a gobgp server because it has several global vars that introduce race conditions and testing irregularity
* Minor fixes:
  * Adds a python script which parses Go unit tests, which is helpful for understanding which tests in the suite take the longest
  * Fixes import definition order in linux_networking_moq.go
  * Updates makefile to run goimports as well as gofmt when gofmt-fix target is executed


Patch:
diff --git a/Makefile b/Makefile
index 48956cf4..8ee3cd5d 100644
--- a/Makefile
+++ b/Makefile
@@ -191,6 +191,7 @@ gofmt: ## Tells you what files need to be gofmt'd.
 
 gofmt-fix: ## Fixes files that need to be gofmt'd.
 	gofmt -s -w $(shell find . -not \( \( -wholename '*/vendor/*' \) -prune \) -name '*.go')
+	goimports -w $(shell find . -not \( \( -wholename '*/vendor/*' \) -prune \) -name '*.go')
 
 # List of all file_moq.go files which would need to be regenerated
 # from file.go if changed
diff --git a/pkg/bgp/id.go b/pkg/bgp/id.go
index 3396fe71..6c9a82a5 100644
--- a/pkg/bgp/id.go
+++ b/pkg/bgp/id.go
@@ -37,7 +37,7 @@ func GenerateRouterID(nodeIPAware utils.NodeIPAware, configRouterID string) (str
 	if nodeIPAware.GetPrimaryNodeIP().To4() == nil {
 		return "", errors.New("router-id must be specified when primary node IP is an IPv6 address")
 	}
-	return configRouterID, nil
+	return nodeIPAware.GetPrimaryNodeIP().String(), nil
 }
 
 // ValidateCommunity takes in a string and attempts to parse a BGP community out of it in a way that is similar to
diff --git a/pkg/controllers/lballoc/lballoc.go b/pkg/controllers/lballoc/lballoc.go
index 7ae2fdbb..12bd1a25 100644
--- a/pkg/controllers/lballoc/lballoc.go
+++ b/pkg/controllers/lballoc/lballoc.go
@@ -38,6 +38,7 @@ type LoadBalancerController struct {
 	clientset    kubernetes.Interface
 	isDefault    bool
 	syncPeriod   time.Duration
+	unitTestWG   *sync.WaitGroup
 }
 
 func getNamespace() (namespace string, err error) {
@@ -346,6 +347,12 @@ func appendIngressIP(svc *v1core.Service, ip net.IP) {
 }
 
 func (lbc *LoadBalancerController) updateService(svc *v1core.Service, ips ...net.IP) {
+	// This is only non-nil during certain unit tests that need to understand when this goroutine is finished to remove
+	// chance of race conditions
+	if lbc.unitTestWG != nil {
+		defer lbc.unitTestWG.Done()
+	}
+
 	if lbc.clientset == nil {
 		panic("clientset")
 	}
@@ -399,6 +406,11 @@ func (lbc *LoadBalancerController) allocateService(svc *v1core.Service) error {
 		return errors.New("unable to allocate dual-stack addresses: " + err.Error())
 	}
 
+	// This is only non-nil during certain unit tests that need to understand when this goroutine is finished to remove
+	// chance of race conditions
+	if lbc.unitTestWG != nil {
+		lbc.unitTestWG.Add(1)
+	}
 	go lbc.updateService(svc, ipv4, ipv6)
 	return nil
 }
diff --git a/pkg/controllers/proxy/linux_networking_moq.go b/pkg/controllers/proxy/linux_networking_moq.go
index 940044fc..8a13c38c 100644
--- a/pkg/controllers/proxy/linux_networking_moq.go
+++ b/pkg/controllers/proxy/linux_networking_moq.go
@@ -4,11 +4,12 @@
 package proxy
 
 import (
+	"net"
+	"sync"
+
 	"github.com/moby/ipvs"
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netns"
-	"net"
-	"sync"
 )
 
 // Ensure, that LinuxNetworkingMock does implement LinuxNetworking.
diff --git a/pkg/utils/node.go b/pkg/utils/node.go
index b580de13..87da1dd3 100644
--- a/pkg/utils/node.go
+++ b/pkg/utils/node.go
@@ -85,18 +85,16 @@ type NodeAware interface {
 // GetNodeIPv4Addrs returns the node's IPv4 addresses as defined by the Kubernetes Node Object.
 func (n *KRNode) GetNodeIPv4Addrs() []net.IP {
 	var nodeIPs []net.IP
-	for _, ip := range n.NodeIPv4Addrs {
-		nodeIPs = append(nodeIPs, ip...)
-	}
+	nodeIPs = append(nodeIPs, n.NodeIPv4Addrs[apiv1.NodeInternalIP]...)
+	nodeIPs = append(nodeIPs, n.NodeIPv4Addrs[apiv1.NodeExternalIP]...)
 	return nodeIPs
 }
 
 // GetNodeIPv6Addrs returns the node's IPv6 addresses as defined by the Kubernetes Node Object.
 func (n *KRNode) GetNodeIPv6Addrs() []net.IP {
 	var nodeIPs []net.IP
-	for _, ip := range n.NodeIPv6Addrs {
-		nodeIPs = append(nodeIPs, ip...)
-	}
+	nodeIPs = append(nodeIPs, n.NodeIPv6Addrs[apiv1.NodeInternalIP]...)
+	nodeIPs = append(nodeIPs, n.NodeIPv6Addrs[apiv1.NodeExternalIP]...)
 	return nodeIPs
 }
 
@@ -196,12 +194,10 @@ func (n *LocalKRNode) GetNodeMTU() (int, error) {
 // Node Object.
 func (n *KRNode) GetNodeIPAddrs() []net.IP {
 	var nodeIPs []net.IP
-	for _, ip := range n.NodeIPv4Addrs {
-		nodeIPs = append(nodeIPs, ip...)
-	}
-	for _, ip := range n.NodeIPv6Addrs {
-		nodeIPs = append(nodeIPs, ip...)
-	}
+	ipv4IPs := n.GetNodeIPv4Addrs()
+	nodeIPs = append(nodeIPs, ipv4IPs...)
+	ipv6IPs := n.GetNodeIPv6Addrs()
+	nodeIPs = append(nodeIPs, ipv6IPs...)
 	return nodeIPs
 }
 


Test patch:
diff --git a/build/test-scripts/unit_test_timing.py b/build/test-scripts/unit_test_timing.py
new file mode 100755
index 00000000..db254463
--- /dev/null
+++ b/build/test-scripts/unit_test_timing.py
@@ -0,0 +1,34 @@
+#!/usr/bin/env python
+
+# Taken from: https://rotational.io/blog/speeding-up-go-tests/ this python script assists in parsing Golang JSON unit
+# tests and sorting them by the amount of time taken
+#
+# To use, run Go unit tests via the following:
+# go test -v -json -count 1 github.com/cloudnativelabs/kube-router/v2/cmd/kube-router/ github.com/cloudnativelabs/kube-router/v2/pkg/... >testing_output.json
+#
+# Then run this script via:
+# build/test-scripts/unit_test_timing.py testing_output.json
+
+import json
+import sys
+
+if __name__ == "__main__":
+    tests = []
+
+    with open(sys.argv[1], 'r') as f:
+        for line in f:
+            data = json.loads(line)
+            if data['Action'] != 'pass':
+                continue
+
+            if 'Test' not in data:
+                continue
+
+            if data['Elapsed'] < 0.1:
+                continue
+
+            tests.append(data)
+
+    tests.sort(key=lambda d: d['Elapsed'], reverse=True)
+    for t in tests:
+        print(f"{t['Elapsed']:0.3f}s\t{t['Package']} {t['Test']}")
diff --git a/pkg/controllers/lballoc/lballoc_test.go b/pkg/controllers/lballoc/lballoc_test.go
index ef2a8234..c8547e9c 100644
--- a/pkg/controllers/lballoc/lballoc_test.go
+++ b/pkg/controllers/lballoc/lballoc_test.go
@@ -3,6 +3,7 @@ package lballoc
 import (
 	"errors"
 	"net"
+	"sync"
 	"testing"
 	"time"
 
@@ -572,7 +573,8 @@ func TestAppendIngressIP(t *testing.T) {
 
 func TestAllocateService(t *testing.T) {
 	mlbc := &LoadBalancerController{
-		clientset: fake.NewSimpleClientset(),
+		clientset:  fake.NewSimpleClientset(),
+		unitTestWG: &sync.WaitGroup{},
 	}
 	ir4, ir6 := makeIPRanges("127.127.127.127/30", "ffff::/80")
 	mlbc.ipv4Ranges = ir4
@@ -586,6 +588,7 @@ func TestAllocateService(t *testing.T) {
 		t.Fatalf("expected %v, got %s", nil, err)
 	}
 
+	mlbc.unitTestWG.Wait()
 	svc = makeTestService()
 	mlbc.ipv4Ranges = newipRanges(nil)
 	fp := v1core.IPFamilyPolicyRequireDualStack
diff --git a/pkg/controllers/routing/ecmp_vip_test.go b/pkg/controllers/routing/ecmp_vip_test.go
index 9ea00e1d..5fa53bb6 100644
--- a/pkg/controllers/routing/ecmp_vip_test.go
+++ b/pkg/controllers/routing/ecmp_vip_test.go
@@ -903,6 +903,7 @@ func Test_getVIPsForService(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
 			for _, serviceAdvertisedIP := range test.serviceAdvertisedIPs {
 				endpoints := serviceAdvertisedIP.endpoints
 				clientset := fake.NewSimpleClientset()