Merge pull request #121072 from danwinship/kube-proxy-unit-tests

Fix regression in cmd/kube-proxy/app unit test speed

cmd/kube-proxy/app/server.go

@@ -603,7 +603,8 @@ func newProxyServer(config *kubeproxyconfig.KubeProxyConfiguration, master strin
 		return nil, err
 	}
 
-	s.PrimaryIPFamily, s.NodeIPs = detectNodeIPs(s.Client, s.Hostname, config.BindAddress)
+	rawNodeIPs := getNodeIPs(s.Client, s.Hostname)
+	s.PrimaryIPFamily, s.NodeIPs = detectNodeIPs(rawNodeIPs, config.BindAddress)
 
 	s.Broadcaster = events.NewBroadcaster(&events.EventSinkImpl{Interface: s.Client.EventsV1()})
 	s.Recorder = s.Broadcaster.NewRecorder(proxyconfigscheme.Scheme, "kube-proxy")
@@ -955,27 +956,27 @@ func (s *ProxyServer) birthCry() {
 //
 // The order of precedence is:
 //  1. if bindAddress is not 0.0.0.0 or ::, then it is used as the primary IP.
-//  2. if the Node object can be fetched, then its address(es) is/are used
+//  2. if rawNodeIPs is not empty, then its address(es) is/are used
 //  3. otherwise the node IPs are 127.0.0.1 and ::1
-func detectNodeIPs(client clientset.Interface, hostname, bindAddress string) (v1.IPFamily, map[v1.IPFamily]net.IP) {
+func detectNodeIPs(rawNodeIPs []net.IP, bindAddress string) (v1.IPFamily, map[v1.IPFamily]net.IP) {
 	primaryFamily := v1.IPv4Protocol
 	nodeIPs := map[v1.IPFamily]net.IP{
 		v1.IPv4Protocol: net.IPv4(127, 0, 0, 1),
 		v1.IPv6Protocol: net.IPv6loopback,
 	}
 
-	if ips := getNodeIPs(client, hostname); len(ips) > 0 {
+	if len(rawNodeIPs) > 0 {
-		if !netutils.IsIPv4(ips[0]) {
+		if !netutils.IsIPv4(rawNodeIPs[0]) {
 			primaryFamily = v1.IPv6Protocol
 		}
-		nodeIPs[primaryFamily] = ips[0]
+		nodeIPs[primaryFamily] = rawNodeIPs[0]
-		if len(ips) > 1 {
+		if len(rawNodeIPs) > 1 {
 			// If more than one address is returned, they are guaranteed to be of different families
 			family := v1.IPv4Protocol
-			if !netutils.IsIPv4(ips[1]) {
+			if !netutils.IsIPv4(rawNodeIPs[1]) {
 				family = v1.IPv6Protocol
 			}
-			nodeIPs[family] = ips[1]
+			nodeIPs[family] = rawNodeIPs[1]
 		}
 	}
 

cmd/kube-proxy/app/server_test.go

@@ -17,9 +17,11 @@ limitations under the License.
 package app
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"net"
 	"path"
 	"testing"
 	"time"
@@ -36,6 +38,7 @@ import (
 	componentbaseconfig "k8s.io/component-base/config"
 	logsapi "k8s.io/component-base/logs/api/v1"
 	kubeproxyconfig "k8s.io/kubernetes/pkg/proxy/apis/config"
+	netutils "k8s.io/utils/net"
 	"k8s.io/utils/ptr"
 )
 
@@ -593,11 +596,7 @@ func TestAddressFromDeprecatedFlags(t *testing.T) {
 	}
 }
 
-func makeNodeWithAddresses(name, internal, external string) *v1.Node {
+func makeNodeWithAddress(name, primaryIP string) *v1.Node {
-	if name == "" {
-		return &v1.Node{}
-	}
-
 	node := &v1.Node{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
@@ -607,26 +606,76 @@ func makeNodeWithAddresses(name, internal, external string) *v1.Node {
 		},
 	}
 
-	if internal != "" {
+	if primaryIP != "" {
 		node.Status.Addresses = append(node.Status.Addresses,
-			v1.NodeAddress{Type: v1.NodeInternalIP, Address: internal},
+			v1.NodeAddress{Type: v1.NodeInternalIP, Address: primaryIP},
-		)
-	}
-
-	if external != "" {
-		node.Status.Addresses = append(node.Status.Addresses,
-			v1.NodeAddress{Type: v1.NodeExternalIP, Address: external},
 		)
 	}
 
 	return node
 }
 
+// Test that getNodeIPs retries on failure
+func Test_getNodeIPs(t *testing.T) {
+	var chans [3]chan error
+
+	client := clientsetfake.NewSimpleClientset(
+		// node1 initially has no IP address.
+		makeNodeWithAddress("node1", ""),
+
+		// node2 initially has an invalid IP address.
+		makeNodeWithAddress("node2", "invalid-ip"),
+
+		// node3 initially does not exist.
+	)
+
+	for i := range chans {
+		chans[i] = make(chan error)
+		ch := chans[i]
+		nodeName := fmt.Sprintf("node%d", i+1)
+		expectIP := fmt.Sprintf("192.168.0.%d", i+1)
+		go func() {
+			ips := getNodeIPs(client, nodeName)
+			if len(ips) == 0 {
+				ch <- fmt.Errorf("expected IP %s for %s but got nil", expectIP, nodeName)
+			} else if ips[0].String() != expectIP {
+				ch <- fmt.Errorf("expected IP %s for %s but got %s", expectIP, nodeName, ips[0].String())
+			} else if len(ips) != 1 {
+				ch <- fmt.Errorf("expected IP %s for %s but got multiple IPs", expectIP, nodeName)
+			}
+			close(ch)
+		}()
+	}
+
+	// Give the goroutines time to fetch the bad/non-existent nodes, then fix them.
+	time.Sleep(1200 * time.Millisecond)
+
+	_, _ = client.CoreV1().Nodes().UpdateStatus(context.TODO(),
+		makeNodeWithAddress("node1", "192.168.0.1"),
+		metav1.UpdateOptions{},
+	)
+	_, _ = client.CoreV1().Nodes().UpdateStatus(context.TODO(),
+		makeNodeWithAddress("node2", "192.168.0.2"),
+		metav1.UpdateOptions{},
+	)
+	_, _ = client.CoreV1().Nodes().Create(context.TODO(),
+		makeNodeWithAddress("node3", "192.168.0.3"),
+		metav1.CreateOptions{},
+	)
+
+	// Ensure each getNodeIP completed as expected
+	for i := range chans {
+		err := <-chans[i]
+		if err != nil {
+			t.Error(err.Error())
+		}
+	}
+}
+
 func Test_detectNodeIPs(t *testing.T) {
 	cases := []struct {
 		name           string
-		nodeInfo       *v1.Node
+		rawNodeIPs     []net.IP
-		hostname       string
 		bindAddress    string
 		expectedFamily v1.IPFamily
 		expectedIPv4   string
@@ -634,8 +683,7 @@ func Test_detectNodeIPs(t *testing.T) {
 	}{
 		{
 			name:           "Bind address IPv4 unicast address and no Node object",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", ""),
+			rawNodeIPs:     nil,
-			hostname:       "fakeHost",
 			bindAddress:    "10.0.0.1",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "10.0.0.1",
@@ -643,38 +691,31 @@ func Test_detectNodeIPs(t *testing.T) {
 		},
 		{
 			name:           "Bind address IPv6 unicast address and no Node object",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", ""),
+			rawNodeIPs:     nil,
-			hostname:       "fakeHost",
 			bindAddress:    "fd00:4321::2",
 			expectedFamily: v1.IPv6Protocol,
 			expectedIPv4:   "127.0.0.1",
 			expectedIPv6:   "fd00:4321::2",
 		},
 		{
-			name:           "No Valid IP found",
+			name:           "No Valid IP found and no bind address",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", ""),
+			rawNodeIPs:     nil,
-			hostname:       "fakeHost",
 			bindAddress:    "",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "127.0.0.1",
 			expectedIPv6:   "::1",
 		},
-		// Disabled because the GetNodeIP method has a backoff retry mechanism
+		{
-		// and the test takes more than 30 seconds
+			name:           "No Valid IP found and unspecified bind address",
-		// ok  	k8s.io/kubernetes/cmd/kube-proxy/app	34.136s
+			rawNodeIPs:     nil,
-		// {
+			bindAddress:    "0.0.0.0",
-		//	name:           "No Valid IP found and unspecified bind address",
+			expectedFamily: v1.IPv4Protocol,
-		//	nodeInfo:       makeNodeWithAddresses("", "", ""),
+			expectedIPv4:   "127.0.0.1",
-		//	hostname:       "fakeHost",
+			expectedIPv6:   "::1",
-		//	bindAddress:    "0.0.0.0",
+		},
-		//	expectedFamily: v1.IPv4Protocol,
-		//	expectedIPv4:   "127.0.0.1",
-		//	expectedIPv6:   "::",
-		// },
 		{
 			name:           "Bind address 0.0.0.0 and node with IPv4 InternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "192.168.1.1", "90.90.90.90"),
+			rawNodeIPs:     []net.IP{netutils.ParseIPSloppy("192.168.1.1")},
-			hostname:       "fakeHost",
 			bindAddress:    "0.0.0.0",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "192.168.1.1",
@@ -682,8 +723,7 @@ func Test_detectNodeIPs(t *testing.T) {
 		},
 		{
 			name:           "Bind address :: and node with IPv4 InternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "192.168.1.1", "90.90.90.90"),
+			rawNodeIPs:     []net.IP{netutils.ParseIPSloppy("192.168.1.1")},
-			hostname:       "fakeHost",
 			bindAddress:    "::",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "192.168.1.1",
@@ -691,8 +731,7 @@ func Test_detectNodeIPs(t *testing.T) {
 		},
 		{
 			name:           "Bind address 0.0.0.0 and node with IPv6 InternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "fd00:1234::1", "2001:db8::2"),
+			rawNodeIPs:     []net.IP{netutils.ParseIPSloppy("fd00:1234::1")},
-			hostname:       "fakeHost",
 			bindAddress:    "0.0.0.0",
 			expectedFamily: v1.IPv6Protocol,
 			expectedIPv4:   "127.0.0.1",
@@ -700,98 +739,73 @@ func Test_detectNodeIPs(t *testing.T) {
 		},
 		{
 			name:           "Bind address :: and node with IPv6 InternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "fd00:1234::1", "2001:db8::2"),
+			rawNodeIPs:     []net.IP{netutils.ParseIPSloppy("fd00:1234::1")},
-			hostname:       "fakeHost",
 			bindAddress:    "::",
 			expectedFamily: v1.IPv6Protocol,
 			expectedIPv4:   "127.0.0.1",
 			expectedIPv6:   "fd00:1234::1",
 		},
 		{
-			name:           "Bind address 0.0.0.0 and node with only IPv4 ExternalIP set",
+			name: "Dual stack, primary IPv4",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", "90.90.90.90"),
+			rawNodeIPs: []net.IP{
-			hostname:       "fakeHost",
+				netutils.ParseIPSloppy("90.90.90.90"),
-			bindAddress:    "0.0.0.0",
+				netutils.ParseIPSloppy("2001:db8::2"),
-			expectedFamily: v1.IPv4Protocol,
+			},
-			expectedIPv4:   "90.90.90.90",
-			expectedIPv6:   "::1",
-		},
-		{
-			name:           "Bind address :: and node with only IPv4 ExternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", "90.90.90.90"),
-			hostname:       "fakeHost",
-			bindAddress:    "::",
-			expectedFamily: v1.IPv4Protocol,
-			expectedIPv4:   "90.90.90.90",
-			expectedIPv6:   "::1",
-		},
-		{
-			name:           "Bind address 0.0.0.0 and node with only IPv6 ExternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", "2001:db8::2"),
-			hostname:       "fakeHost",
-			bindAddress:    "0.0.0.0",
-			expectedFamily: v1.IPv6Protocol,
-			expectedIPv4:   "127.0.0.1",
-			expectedIPv6:   "2001:db8::2",
-		},
-		{
-			name:           "Bind address :: and node with only IPv6 ExternalIP set",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "", "2001:db8::2"),
-			hostname:       "fakeHost",
-			bindAddress:    "::",
-			expectedFamily: v1.IPv6Protocol,
-			expectedIPv4:   "127.0.0.1",
-			expectedIPv6:   "2001:db8::2",
-		},
-		{
-			name:           "Dual stack, primary IPv4",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "90.90.90.90", "2001:db8::2"),
-			hostname:       "fakeHost",
 			bindAddress:    "::",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "90.90.90.90",
 			expectedIPv6:   "2001:db8::2",
 		},
 		{
-			name:           "Dual stack, primary IPv6",
+			name: "Dual stack, primary IPv6",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "2001:db8::2", "90.90.90.90"),
+			rawNodeIPs: []net.IP{
-			hostname:       "fakeHost",
+				netutils.ParseIPSloppy("2001:db8::2"),
+				netutils.ParseIPSloppy("90.90.90.90"),
+			},
 			bindAddress:    "0.0.0.0",
 			expectedFamily: v1.IPv6Protocol,
 			expectedIPv4:   "90.90.90.90",
 			expectedIPv6:   "2001:db8::2",
 		},
 		{
-			name:           "Dual stack, override IPv4",
+			name: "Dual stack, override IPv4",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "2001:db8::2", "90.90.90.90"),
+			rawNodeIPs: []net.IP{
-			hostname:       "fakeHost",
+				netutils.ParseIPSloppy("2001:db8::2"),
+				netutils.ParseIPSloppy("90.90.90.90"),
+			},
 			bindAddress:    "80.80.80.80",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "80.80.80.80",
 			expectedIPv6:   "2001:db8::2",
 		},
 		{
-			name:           "Dual stack, override IPv6",
+			name: "Dual stack, override IPv6",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "90.90.90.90", "2001:db8::2"),
+			rawNodeIPs: []net.IP{
-			hostname:       "fakeHost",
+				netutils.ParseIPSloppy("90.90.90.90"),
+				netutils.ParseIPSloppy("2001:db8::2"),
+			},
 			bindAddress:    "2001:db8::555",
 			expectedFamily: v1.IPv6Protocol,
 			expectedIPv4:   "90.90.90.90",
 			expectedIPv6:   "2001:db8::555",
 		},
 		{
-			name:           "Dual stack, override primary family, IPv4",
+			name: "Dual stack, override primary family, IPv4",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "2001:db8::2", "90.90.90.90"),
+			rawNodeIPs: []net.IP{
-			hostname:       "fakeHost",
+				netutils.ParseIPSloppy("2001:db8::2"),
+				netutils.ParseIPSloppy("90.90.90.90"),
+			},
 			bindAddress:    "127.0.0.1",
 			expectedFamily: v1.IPv4Protocol,
 			expectedIPv4:   "127.0.0.1",
 			expectedIPv6:   "2001:db8::2",
 		},
 		{
-			name:           "Dual stack, override primary family, IPv6",
+			name: "Dual stack, override primary family, IPv6",
-			nodeInfo:       makeNodeWithAddresses("fakeHost", "90.90.90.90", "2001:db8::2"),
+			rawNodeIPs: []net.IP{
-			hostname:       "fakeHost",
+				netutils.ParseIPSloppy("90.90.90.90"),
+				netutils.ParseIPSloppy("2001:db8::2"),
+			},
 			bindAddress:    "::1",
 			expectedFamily: v1.IPv6Protocol,
 			expectedIPv4:   "90.90.90.90",
@@ -800,8 +814,7 @@ func Test_detectNodeIPs(t *testing.T) {
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			client := clientsetfake.NewSimpleClientset(c.nodeInfo)
+			primaryFamily, ips := detectNodeIPs(c.rawNodeIPs, c.bindAddress)
-			primaryFamily, ips := detectNodeIPs(client, c.hostname, c.bindAddress)
 			if primaryFamily != c.expectedFamily {
 				t.Errorf("Expected family %q got %q", c.expectedFamily, primaryFamily)
 			}