From e518ae45c7bd986cfafa0c00083d95ec9fda2ed5 Mon Sep 17 00:00:00 2001 From: Jarrad Whitaker Date: Thu, 14 Dec 2023 11:21:26 +1100 Subject: [PATCH] return multiple IPs for nodes when updating status, not just the first the main consideration I had for doing this was to make sure both IPv4 and IPv6 addresses are attached to my ingress, however the approach here has nothing to do with IP Families: the change is to return all matching addresses instead of just the first, regardless of family. --- internal/ingress/status/status.go | 8 ++-- internal/ingress/status/status_test.go | 21 +++++----- internal/k8s/main.go | 20 ++++++---- internal/k8s/main_test.go | 53 +++++++++++++++++--------- 4 files changed, 61 insertions(+), 41 deletions(-) diff --git a/internal/ingress/status/status.go b/internal/ingress/status/status.go index 2771d2755..e16b50170 100644 --- a/internal/ingress/status/status.go +++ b/internal/ingress/status/status.go @@ -221,9 +221,11 @@ func (s *statusSync) runningAddresses() ([]v1.IngressLoadBalancerIngress, error) } preferExternal := !s.UseNodeInternalIP - name := k8s.GetNodeIPOrName(s.Client, pod.Spec.NodeName, preferExternal) - if !stringInIngresses(name, addrs) { - addrs = append(addrs, nameOrIPToLoadBalancerIngress(name)) + addresses := k8s.GetNodeAddresses(s.Client, pod.Spec.NodeName, preferExternal) + for _, addr := range addresses { + if !stringInIngresses(addr, addrs) { + addrs = append(addrs, nameOrIPToLoadBalancerIngress(addr)) + } } } diff --git a/internal/ingress/status/status_test.go b/internal/ingress/status/status_test.go index 7347e7f5c..1cdab9c63 100644 --- a/internal/ingress/status/status_test.go +++ b/internal/ingress/status/status_test.go @@ -342,11 +342,12 @@ func TestStatusActions(t *testing.T) { if err := fk.sync("just-test"); err != nil { t.Errorf("unexpected error: %v", err) } - // PublishService is empty, so the running address is the node's: ["11.0.1.2"] - // after updated, the ingress's ip should only be "11.0.1.2" - newIPs := []networking.IngressLoadBalancerIngress{{ - IP: "11.0.1.2", - }} + // PublishService is empty, so the running address is the node's: ["11.0.1.2", "2001::1:2"] + // after updated, the ingress's ip should only be "11.0.1.2", "2001::1:2" + newIPs := []networking.IngressLoadBalancerIngress{ + {IP: "11.0.1.2"}, + {IP: "2001::1:2"}, + } fooIngress1, err1 := fk.Client.NetworkingV1().Ingresses(apiv1.NamespaceDefault).Get(context.TODO(), "foo_ingress_1", metav1.GetOptions{}) if err1 != nil { t.Fatalf("unexpected error") @@ -614,13 +615,11 @@ func TestRunningAddressesWithPods(t *testing.T) { if r == nil { t.Fatalf("returned nil but expected valid []networking.IngressLoadBalancerIngress") } - rl := len(r) - if len(r) != 1 { - t.Fatalf("returned %v but expected %v", rl, 1) + er := []networking.IngressLoadBalancerIngress{ + {IP: "11.0.1.2"}, {IP: "2001::1:2"}, } - rv := r[0] - if rv.IP != "11.0.1.2" { - t.Errorf("returned %v but expected %v", rv, networking.IngressLoadBalancerIngress{IP: "11.0.1.2"}) + if !reflect.DeepEqual(r, er) { + t.Errorf("returned %v but expected %v", r, er) } } diff --git a/internal/k8s/main.go b/internal/k8s/main.go index 707b21fc4..4dcefdfc4 100644 --- a/internal/k8s/main.go +++ b/internal/k8s/main.go @@ -42,16 +42,17 @@ func ParseNameNS(input string) (ns, name string, err error) { return nsName[0], nsName[1], nil } -// GetNodeIPOrName returns the IP address or name of a node in the cluster. -// If preferExternal==true AND any non-empty NodeExternalIP addresses exist, the first one will be returned. -// Otherwise, the node's first non-empty NodeInternalIP address will be returned. -func GetNodeIPOrName(kubeClient clientset.Interface, name string, preferExternal bool) string { +// GetNodeAddresses returns the IP address or name of a node in the cluster. +// If preferExternal==true AND any non-empty NodeExternalIP addresses exist, they will be returned. +// Otherwise, the node's non-empty NodeInternalIP addresses will be returned. +func GetNodeAddresses(kubeClient clientset.Interface, name string, preferExternal bool) []string { node, err := kubeClient.CoreV1().Nodes().Get(context.TODO(), name, metav1.GetOptions{}) if err != nil { klog.ErrorS(err, "Error getting node", "name", name) - return "" + return nil } + addresses := make([]string, 0) if preferExternal { for _, address := range node.Status.Addresses { if address.Type != apiv1.NodeExternalIP { @@ -60,7 +61,10 @@ func GetNodeIPOrName(kubeClient clientset.Interface, name string, preferExternal if address.Address == "" { continue } - return address.Address + addresses = append(addresses, address.Address) + } + if len(addresses) > 0 { + return addresses } } @@ -71,10 +75,10 @@ func GetNodeIPOrName(kubeClient clientset.Interface, name string, preferExternal if address.Address == "" { continue } - return address.Address + addresses = append(addresses, address.Address) } - return "" + return addresses } var ( diff --git a/internal/k8s/main_test.go b/internal/k8s/main_test.go index aa7d31c2a..e616dbf80 100644 --- a/internal/k8s/main_test.go +++ b/internal/k8s/main_test.go @@ -17,6 +17,7 @@ limitations under the License. package k8s import ( + "reflect" "testing" apiv1 "k8s.io/api/core/v1" @@ -55,19 +56,19 @@ func TestParseNameNS(t *testing.T) { } } -func TestGetNodeIP(t *testing.T) { +func TestGetNodeAddresses(t *testing.T) { fKNodes := []struct { name string cs *testclient.Clientset nodeName string preferExternal bool - ea string + ea []string }{ { "empty node list", testclient.NewSimpleClientset(), "demo", false, - "", + nil, }, { "node does not exist", @@ -84,7 +85,7 @@ func TestGetNodeIP(t *testing.T) { }, }, }}}), "notexistnode", false, - "", + nil, }, { "node exist and only has an internal IP address (preferExternal=true)", @@ -101,7 +102,7 @@ func TestGetNodeIP(t *testing.T) { }, }, }}}), "demo", true, - "10.0.0.1", + []string{"10.0.0.1"}, }, { "node exist and only has an internal IP address", @@ -118,7 +119,7 @@ func TestGetNodeIP(t *testing.T) { }, }, }}}), "demo", false, - "10.0.0.1", + []string{"10.0.0.1"}, }, { "node exist and only has an external IP address", @@ -135,7 +136,7 @@ func TestGetNodeIP(t *testing.T) { }, }, }}}), "demo", true, - "10.0.0.1", + []string{"10.0.0.1"}, }, { "multiple nodes - choose the right one", @@ -168,10 +169,10 @@ func TestGetNodeIP(t *testing.T) { }, }}), "demo2", false, - "10.0.0.2", + []string{"10.0.0.2"}, }, { - "node with both IP internal and external IP address - returns external IP", + "node with both IP internal and external IP addresses - returns external IPs", testclient.NewSimpleClientset(&apiv1.NodeList{Items: []apiv1.Node{{ ObjectMeta: metav1.ObjectMeta{ Name: "demo", @@ -181,18 +182,27 @@ func TestGetNodeIP(t *testing.T) { { Type: apiv1.NodeInternalIP, Address: "10.0.0.1", - }, { + }, + { + Type: apiv1.NodeInternalIP, + Address: "fe80::1", + }, + { Type: apiv1.NodeExternalIP, - Address: "10.0.0.2", + Address: "8.0.0.1", + }, + { + Type: apiv1.NodeExternalIP, + Address: "2001::1", }, }, }, }}}), "demo", true, - "10.0.0.2", + []string{"8.0.0.1", "2001::1"}, }, { - "node with both IP internal and external IP address - returns internal IP", + "node with both IP internal and external IP addresses - returns internal IPs", testclient.NewSimpleClientset(&apiv1.NodeList{Items: []apiv1.Node{{ ObjectMeta: metav1.ObjectMeta{ Name: "demo", @@ -202,22 +212,27 @@ func TestGetNodeIP(t *testing.T) { { Type: apiv1.NodeExternalIP, Address: "", - }, { + }, + { Type: apiv1.NodeInternalIP, - Address: "10.0.0.2", + Address: "10.0.0.1", + }, + { + Type: apiv1.NodeInternalIP, + Address: "fe80::1", }, }, }, }}}), "demo", false, - "10.0.0.2", + []string{"10.0.0.1", "fe80::1"}, }, } for _, fk := range fKNodes { - address := GetNodeIPOrName(fk.cs, fk.nodeName, fk.preferExternal) - if address != fk.ea { - t.Errorf("%v - expected %s, but returned %s", fk.name, fk.ea, address) + addresses := GetNodeAddresses(fk.cs, fk.nodeName, fk.preferExternal) + if !reflect.DeepEqual(addresses, fk.ea) { + t.Errorf("%v - expected %v, but returned %v", fk.name, fk.ea, addresses) } } }