Repository: cloudnativelabs/kube-router
Pull request #1756 (2024-10-20 22:46:33)

fix(dsr): change grpc resolver to passthrough
@mrueg 

When we used DialContext() before it was deprecated, it manually set the default resolver to passthrough. NewClient() appears to not do this, and then several versions of grpc ago, they defaulted to the DNS resolver which broke our DSR implementation with CRI compatible container engines.

I'm not 100% sure that there isn't a better way to specify this, but I spent 20 - 30 minutes poking around the gRPC code base and I don't think that I can see an easy way to do this outside of specifying it this way.

Furthermore, the project itself seems to advocate for this approach in comments like those on: https://github.com/grpc/grpc-go/issues/1846

Fixes: #1754

aauren: Yes, sorry, I should have prepped you for that. If you used the build from this pipeline instead of just applying the patch, then you would have hit the regression that I fixed in another PR: https://github.com/cloudnativelabs/kube-router/pull/1755

Since both have been reviewed, I plan on merging both today, and then cutting a new release.

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/cri/remote_runtime.go b/pkg/cri/remote_runtime.go
index b870af3a..8d882916 100644
--- a/pkg/cri/remote_runtime.go
+++ b/pkg/cri/remote_runtime.go
@@ -41,7 +41,11 @@ func NewRemoteRuntimeService(endpoint string, connectionTimeout time.Duration) (
 
 	klog.V(4).Infof("[RuntimeService] got endpoint %s (proto=%s, path=%s)", endpoint, proto, addr)
 
-	if proto != "unix" {
+	if proto == "unix" {
+		// Every since grpc.DialContext was deprecated, we no longer get the passthrough resolver for free, so we need
+		// to add it manually. See: https://github.com/grpc/grpc-go/issues/1846 for more context
+		addr = "passthrough:///" + addr
+	} else {
 		return nil, errors.New("[RuntimeService] only unix socket is currently supported")
 	}
 
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()