From 6df8b2a828439ee1ebabd0565db18959c3b173e7 Mon Sep 17 00:00:00 2001 From: Jarrad Whitaker Date: Thu, 14 Dec 2023 11:21:26 +1100 Subject: [PATCH 1/4] prework: refactor GetNodeIPOrName this is done to make the intention of current behaviour clearer before I go changing it. this should have no effect on current behaviour. --- internal/ingress/status/status.go | 3 ++- internal/k8s/main.go | 36 +++++++++++++++-------------- internal/k8s/main_test.go | 38 +++++++++++++++++++------------ 3 files changed, 44 insertions(+), 33 deletions(-) diff --git a/internal/ingress/status/status.go b/internal/ingress/status/status.go index 81fb9044a..2771d2755 100644 --- a/internal/ingress/status/status.go +++ b/internal/ingress/status/status.go @@ -220,7 +220,8 @@ func (s *statusSync) runningAddresses() ([]v1.IngressLoadBalancerIngress, error) continue } - name := k8s.GetNodeIPOrName(s.Client, pod.Spec.NodeName, s.UseNodeInternalIP) + preferExternal := !s.UseNodeInternalIP + name := k8s.GetNodeIPOrName(s.Client, pod.Spec.NodeName, preferExternal) if !stringInIngresses(name, addrs) { addrs = append(addrs, nameOrIPToLoadBalancerIngress(name)) } diff --git a/internal/k8s/main.go b/internal/k8s/main.go index 5e93e560d..707b21fc4 100644 --- a/internal/k8s/main.go +++ b/internal/k8s/main.go @@ -42,37 +42,39 @@ func ParseNameNS(input string) (ns, name string, err error) { return nsName[0], nsName[1], nil } -// GetNodeIPOrName returns the IP address or the name of a node in the cluster -func GetNodeIPOrName(kubeClient clientset.Interface, name string, useInternalIP bool) string { +// 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 { node, err := kubeClient.CoreV1().Nodes().Get(context.TODO(), name, metav1.GetOptions{}) if err != nil { klog.ErrorS(err, "Error getting node", "name", name) return "" } - defaultOrInternalIP := "" - for _, address := range node.Status.Addresses { - if address.Type == apiv1.NodeInternalIP { - if address.Address != "" { - defaultOrInternalIP = address.Address - break + if preferExternal { + for _, address := range node.Status.Addresses { + if address.Type != apiv1.NodeExternalIP { + continue } + if address.Address == "" { + continue + } + return address.Address } } - if useInternalIP { - return defaultOrInternalIP - } - for _, address := range node.Status.Addresses { - if address.Type == apiv1.NodeExternalIP { - if address.Address != "" { - return address.Address - } + if address.Type != apiv1.NodeInternalIP { + continue } + if address.Address == "" { + continue + } + return address.Address } - return defaultOrInternalIP + return "" } var ( diff --git a/internal/k8s/main_test.go b/internal/k8s/main_test.go index 1721c1fb2..aa7d31c2a 100644 --- a/internal/k8s/main_test.go +++ b/internal/k8s/main_test.go @@ -57,16 +57,17 @@ func TestParseNameNS(t *testing.T) { func TestGetNodeIP(t *testing.T) { fKNodes := []struct { - name string - cs *testclient.Clientset - nodeName string - ea string - useInternalIP bool + name string + cs *testclient.Clientset + nodeName string + preferExternal bool + ea string }{ { "empty node list", testclient.NewSimpleClientset(), - "demo", "", true, + "demo", false, + "", }, { "node does not exist", @@ -82,10 +83,11 @@ func TestGetNodeIP(t *testing.T) { }, }, }, - }}}), "notexistnode", "", true, + }}}), "notexistnode", false, + "", }, { - "node exist and only has an internal IP address (useInternalIP=false)", + "node exist and only has an internal IP address (preferExternal=true)", testclient.NewSimpleClientset(&apiv1.NodeList{Items: []apiv1.Node{{ ObjectMeta: metav1.ObjectMeta{ Name: "demo", @@ -98,7 +100,8 @@ func TestGetNodeIP(t *testing.T) { }, }, }, - }}}), "demo", "10.0.0.1", false, + }}}), "demo", true, + "10.0.0.1", }, { "node exist and only has an internal IP address", @@ -114,7 +117,8 @@ func TestGetNodeIP(t *testing.T) { }, }, }, - }}}), "demo", "10.0.0.1", true, + }}}), "demo", false, + "10.0.0.1", }, { "node exist and only has an external IP address", @@ -130,7 +134,8 @@ func TestGetNodeIP(t *testing.T) { }, }, }, - }}}), "demo", "10.0.0.1", false, + }}}), "demo", true, + "10.0.0.1", }, { "multiple nodes - choose the right one", @@ -162,7 +167,8 @@ func TestGetNodeIP(t *testing.T) { }, }, }}), - "demo2", "10.0.0.2", true, + "demo2", false, + "10.0.0.2", }, { "node with both IP internal and external IP address - returns external IP", @@ -182,7 +188,8 @@ func TestGetNodeIP(t *testing.T) { }, }, }}}), - "demo", "10.0.0.2", false, + "demo", true, + "10.0.0.2", }, { "node with both IP internal and external IP address - returns internal IP", @@ -202,12 +209,13 @@ func TestGetNodeIP(t *testing.T) { }, }, }}}), - "demo", "10.0.0.2", true, + "demo", false, + "10.0.0.2", }, } for _, fk := range fKNodes { - address := GetNodeIPOrName(fk.cs, fk.nodeName, fk.useInternalIP) + 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) } From 5f939ee97c45aa9d0d9a01ac0ca4614b5b014adf Mon Sep 17 00:00:00 2001 From: Jarrad Whitaker Date: Thu, 14 Dec 2023 11:21:26 +1100 Subject: [PATCH 2/4] prework: ingress status updater tests: consider dual stack nodes again, no actual behaviour change here, just augmenting tests to show current behaviour before i go changing it. I changed the node IPv4 IP because other bits of the tests used the same IP for a mock load balancer. --- internal/ingress/status/status_test.go | 37 +++++++++++++++++++------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/internal/ingress/status/status_test.go b/internal/ingress/status/status_test.go index 01419708b..7347e7f5c 100644 --- a/internal/ingress/status/status_test.go +++ b/internal/ingress/status/status_test.go @@ -143,10 +143,19 @@ func buildSimpleClientSet() *testclient.Clientset { Addresses: []apiv1.NodeAddress{ { Type: apiv1.NodeInternalIP, - Address: "10.0.0.1", - }, { + Address: "10.0.1.1", + }, + { + Type: apiv1.NodeInternalIP, + Address: "fd80::1:1", + }, + { Type: apiv1.NodeExternalIP, - Address: "10.0.0.2", + Address: "11.0.1.1", + }, + { + Type: apiv1.NodeExternalIP, + Address: "2001::1:1", }, }, }, @@ -159,11 +168,19 @@ func buildSimpleClientSet() *testclient.Clientset { Addresses: []apiv1.NodeAddress{ { Type: apiv1.NodeInternalIP, - Address: "11.0.0.1", + Address: "10.0.1.2", + }, + { + Type: apiv1.NodeInternalIP, + Address: "fd80::1:2", }, { Type: apiv1.NodeExternalIP, - Address: "11.0.0.2", + Address: "11.0.1.2", + }, + { + Type: apiv1.NodeExternalIP, + Address: "2001::1:2", }, }, }, @@ -325,10 +342,10 @@ 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: ["11.0.0.2"] - // after updated, the ingress's ip should only be "11.0.0.2" + // 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.0.2", + IP: "11.0.1.2", }} fooIngress1, err1 := fk.Client.NetworkingV1().Ingresses(apiv1.NamespaceDefault).Get(context.TODO(), "foo_ingress_1", metav1.GetOptions{}) if err1 != nil { @@ -602,8 +619,8 @@ func TestRunningAddressesWithPods(t *testing.T) { t.Fatalf("returned %v but expected %v", rl, 1) } rv := r[0] - if rv.IP != "11.0.0.2" { - t.Errorf("returned %v but expected %v", rv, networking.IngressLoadBalancerIngress{IP: "11.0.0.2"}) + if rv.IP != "11.0.1.2" { + t.Errorf("returned %v but expected %v", rv, networking.IngressLoadBalancerIngress{IP: "11.0.1.2"}) } } From e518ae45c7bd986cfafa0c00083d95ec9fda2ed5 Mon Sep 17 00:00:00 2001 From: Jarrad Whitaker Date: Thu, 14 Dec 2023 11:21:26 +1100 Subject: [PATCH 3/4] 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) } } } From 89b8125739adc33c3f1821d62cd719a8e276de0f Mon Sep 17 00:00:00 2001 From: Jarrad Whitaker Date: Thu, 14 Dec 2023 11:21:26 +1100 Subject: [PATCH 4/4] ** remove len(addrs)>1 check in syncStatus shutdown() this logic is no longer valid but I don't understand what's going on well enough to justify its removal yet - input welcome if you can tell me if this is/isn't reasonable. --- internal/ingress/status/status.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/internal/ingress/status/status.go b/internal/ingress/status/status.go index e16b50170..9b8c61192 100644 --- a/internal/ingress/status/status.go +++ b/internal/ingress/status/status.go @@ -121,12 +121,6 @@ func (s *statusSync) Shutdown() { return } - if len(addrs) > 1 { - // leave the job to the next leader - klog.InfoS("leaving status update for next leader") - return - } - if s.isRunningMultiplePods() { klog.V(2).InfoS("skipping Ingress status update (multiple pods running - another one will be elected as master)") return