From 3c86f838d4a833ee8905cce6d5ef06d4b534f786 Mon Sep 17 00:00:00 2001 From: Emily L Shepherd Date: Tue, 7 Sep 2021 18:41:16 +0100 Subject: [PATCH] Sync Hostname and IP address from service to ingress status (#7464) * Change statusSync.runningAddresses() return type Previously, this method returning a string slice containing the resolved IP addresses / FQDNs to sync onto the Ingress. It was then converted just before use into a slice of LoadBalancerIngresses. This commit changes this logic so that this method generates LoadBalancerIngress objects directly, and returns these. This has two main benefits: - Future work in syncing _both_ hostname and IP, or any other fields that may be used in future (eg Ports), is now supported. - There is less need to rely on net.ParseIP() to determine if a value is an IP address or Hostname, as this can be correctly assigned at generation time based on where each value came from. * Sync both IP and Hostname to Ingress Status Previously, if the IP address was set on a PublishService's LoadBalancerIngress entries, only that would be synced. Hostname was only synced as a fallback when the IP address was missing. Now, both fields are checked independantly and both are synced if present. --- internal/ingress/status/status.go | 92 +++++++++++++++----------- internal/ingress/status/status_test.go | 77 +++++++++++++-------- 2 files changed, 101 insertions(+), 68 deletions(-) diff --git a/internal/ingress/status/status.go b/internal/ingress/status/status.go index cabc6b3a2..7e2db9189 100644 --- a/internal/ingress/status/status.go +++ b/internal/ingress/status/status.go @@ -26,7 +26,6 @@ import ( "time" "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" pool "gopkg.in/go-playground/pool.v3" @@ -143,7 +142,7 @@ func (s *statusSync) sync(key interface{}) error { if err != nil { return err } - s.updateStatus(sliceToStatus(addrs)) + s.updateStatus(standardizeLoadBalancerIngresses(addrs)) return nil } @@ -162,13 +161,25 @@ func NewStatusSyncer(config Config) Syncer { return st } +func nameOrIPToLoadBalancerIngress(nameOrIP string) apiv1.LoadBalancerIngress { + if net.ParseIP(nameOrIP) != nil { + return apiv1.LoadBalancerIngress{IP: nameOrIP} + } + + return apiv1.LoadBalancerIngress{Hostname: nameOrIP} +} + // runningAddresses returns a list of IP addresses and/or FQDN where the // ingress controller is currently running -func (s *statusSync) runningAddresses() ([]string, error) { +func (s *statusSync) runningAddresses() ([]apiv1.LoadBalancerIngress, error) { if s.PublishStatusAddress != "" { re := regexp.MustCompile(`,\s*`) multipleAddrs := re.Split(s.PublishStatusAddress, -1) - return multipleAddrs, nil + addrs := make([]apiv1.LoadBalancerIngress, len(multipleAddrs)) + for i, addr := range multipleAddrs { + addrs[i] = nameOrIPToLoadBalancerIngress(addr) + } + return addrs, nil } if s.PublishService != "" { @@ -183,7 +194,7 @@ func (s *statusSync) runningAddresses() ([]string, error) { return nil, err } - addrs := make([]string, 0) + addrs := make([]apiv1.LoadBalancerIngress, 0) for i := range pods.Items { pod := pods.Items[i] // only Running pods are valid @@ -206,8 +217,8 @@ func (s *statusSync) runningAddresses() ([]string, error) { } name := k8s.GetNodeIPOrName(s.Client, pod.Spec.NodeName, s.UseNodeInternalIP) - if !stringInSlice(name, addrs) { - addrs = append(addrs, name) + if !stringInIngresses(name, addrs) { + addrs = append(addrs, nameOrIPToLoadBalancerIngress(name)) } } @@ -225,17 +236,9 @@ func (s *statusSync) isRunningMultiplePods() bool { return len(pods.Items) > 1 } -// sliceToStatus converts a slice of IP and/or hostnames to LoadBalancerIngress -func sliceToStatus(endpoints []string) []apiv1.LoadBalancerIngress { - lbi := []apiv1.LoadBalancerIngress{} - for _, ep := range endpoints { - if net.ParseIP(ep) == nil { - lbi = append(lbi, apiv1.LoadBalancerIngress{Hostname: ep}) - } else { - lbi = append(lbi, apiv1.LoadBalancerIngress{IP: ep}) - } - } - +// standardizeLoadBalancerIngresses sorts the list of loadbalancer by +// IP +func standardizeLoadBalancerIngresses(lbi []apiv1.LoadBalancerIngress) []apiv1.LoadBalancerIngress { sort.SliceStable(lbi, func(a, b int) bool { return lbi[a].IP < lbi[b].IP }) @@ -321,7 +324,7 @@ func ingressSliceEqual(lhs, rhs []apiv1.LoadBalancerIngress) bool { return true } -func statusAddressFromService(service string, kubeClient clientset.Interface) ([]string, error) { +func statusAddressFromService(service string, kubeClient clientset.Interface) ([]apiv1.LoadBalancerIngress, error) { ns, name, _ := k8s.ParseNameNS(service) svc, err := kubeClient.CoreV1().Services(ns).Get(context.TODO(), name, metav1.GetOptions{}) if err != nil { @@ -330,39 +333,50 @@ func statusAddressFromService(service string, kubeClient clientset.Interface) ([ switch svc.Spec.Type { case apiv1.ServiceTypeExternalName: - return []string{svc.Spec.ExternalName}, nil + return []apiv1.LoadBalancerIngress{{ + Hostname: svc.Spec.ExternalName, + }}, nil case apiv1.ServiceTypeClusterIP: - return []string{svc.Spec.ClusterIP}, nil + return []apiv1.LoadBalancerIngress{{ + IP: svc.Spec.ClusterIP, + }}, nil case apiv1.ServiceTypeNodePort: - addresses := sets.NewString() - if svc.Spec.ExternalIPs != nil { - addresses.Insert(svc.Spec.ExternalIPs...) - } else { - addresses.Insert(svc.Spec.ClusterIP) + if svc.Spec.ExternalIPs == nil { + return []apiv1.LoadBalancerIngress{{ + IP: svc.Spec.ClusterIP, + }}, nil } - return addresses.List(), nil + addrs := make([]apiv1.LoadBalancerIngress, len(svc.Spec.ExternalIPs)) + for i, ip := range svc.Spec.ExternalIPs { + addrs[i] = apiv1.LoadBalancerIngress{IP: ip} + } + return addrs, nil case apiv1.ServiceTypeLoadBalancer: - addresses := sets.NewString() - for _, ip := range svc.Status.LoadBalancer.Ingress { - if ip.IP == "" { - addresses.Insert(ip.Hostname) - } else { - addresses.Insert(ip.IP) + addrs := make([]apiv1.LoadBalancerIngress, len(svc.Status.LoadBalancer.Ingress)) + for i, ingress := range svc.Status.LoadBalancer.Ingress { + addrs[i] = apiv1.LoadBalancerIngress{} + if ingress.Hostname != "" { + addrs[i].Hostname = ingress.Hostname + } + if ingress.IP != "" { + addrs[i].IP = ingress.IP } } - - addresses.Insert(svc.Spec.ExternalIPs...) - - return addresses.List(), nil + for _, ip := range svc.Spec.ExternalIPs { + if !stringInIngresses(ip, addrs) { + addrs = append(addrs, apiv1.LoadBalancerIngress{IP: ip}) + } + } + return addrs, nil } return nil, fmt.Errorf("unable to extract IP address/es from service %v", service) } // stringInSlice returns true if s is in list -func stringInSlice(s string, list []string) bool { +func stringInIngresses(s string, list []apiv1.LoadBalancerIngress) bool { for _, v := range list { - if v == s { + if v.IP == s || v.Hostname == s { return true } } diff --git a/internal/ingress/status/status_test.go b/internal/ingress/status/status_test.go index 79bb85891..fefca5ff2 100644 --- a/internal/ingress/status/status_test.go +++ b/internal/ingress/status/status_test.go @@ -382,7 +382,7 @@ func TestKeyfunc(t *testing.T) { func TestRunningAddressesWithPublishService(t *testing.T) { testCases := map[string]struct { fakeClient *testclient.Clientset - expected []string + expected []apiv1.LoadBalancerIngress errExpected bool }{ "service type ClusterIP": { @@ -416,7 +416,9 @@ func TestRunningAddressesWithPublishService(t *testing.T) { }, }, ), - []string{"1.1.1.1"}, + []apiv1.LoadBalancerIngress{ + {IP: "1.1.1.1"}, + }, false, }, "service type NodePort": { @@ -435,7 +437,9 @@ func TestRunningAddressesWithPublishService(t *testing.T) { }, }, ), - []string{"1.1.1.1"}, + []apiv1.LoadBalancerIngress{ + {IP: "1.1.1.1"}, + }, false, }, "service type ExternalName": { @@ -454,7 +458,9 @@ func TestRunningAddressesWithPublishService(t *testing.T) { }, }, ), - []string{"foo.bar"}, + []apiv1.LoadBalancerIngress{ + {Hostname: "foo.bar"}, + }, false, }, "service type LoadBalancer": { @@ -478,6 +484,10 @@ func TestRunningAddressesWithPublishService(t *testing.T) { IP: "", Hostname: "foo", }, + { + IP: "10.0.0.2", + Hostname: "10-0-0-2.cloudprovider.example.net", + }, }, }, }, @@ -485,7 +495,14 @@ func TestRunningAddressesWithPublishService(t *testing.T) { }, }, ), - []string{"10.0.0.1", "foo"}, + []apiv1.LoadBalancerIngress{ + {IP: "10.0.0.1"}, + {Hostname: "foo"}, + { + IP: "10.0.0.2", + Hostname: "10-0-0-2.cloudprovider.example.net", + }, + }, false, }, "service type LoadBalancer with same externalIP and ingress IP": { @@ -513,7 +530,9 @@ func TestRunningAddressesWithPublishService(t *testing.T) { }, }, ), - []string{"10.0.0.1"}, + []apiv1.LoadBalancerIngress{ + {IP: "10.0.0.1"}, + }, false, }, "invalid service type": { @@ -549,7 +568,7 @@ func TestRunningAddressesWithPublishService(t *testing.T) { } if ra == nil { - t.Fatalf("returned nil but expected valid []string") + t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress") } if !reflect.DeepEqual(tc.expected, ra) { @@ -565,15 +584,15 @@ func TestRunningAddressesWithPods(t *testing.T) { r, _ := fk.runningAddresses() if r == nil { - t.Fatalf("returned nil but expected valid []string") + t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress") } rl := len(r) if len(r) != 1 { t.Fatalf("returned %v but expected %v", rl, 1) } rv := r[0] - if rv != "11.0.0.2" { - t.Errorf("returned %v but expected %v", rv, "11.0.0.2") + if rv.IP != "11.0.0.2" { + t.Errorf("returned %v but expected %v", rv, apiv1.LoadBalancerIngress{IP: "11.0.0.2"}) } } @@ -583,15 +602,15 @@ func TestRunningAddressesWithPublishStatusAddress(t *testing.T) { ra, _ := fk.runningAddresses() if ra == nil { - t.Fatalf("returned nil but expected valid []string") + t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress") } rl := len(ra) if len(ra) != 1 { t.Errorf("returned %v but expected %v", rl, 1) } rv := ra[0] - if rv != "127.0.0.1" { - t.Errorf("returned %v but expected %v", rv, "127.0.0.1") + if rv.IP != "127.0.0.1" { + t.Errorf("returned %v but expected %v", rv, apiv1.LoadBalancerIngress{IP: "127.0.0.1"}) } } @@ -601,7 +620,7 @@ func TestRunningAddressesWithPublishStatusAddresses(t *testing.T) { ra, _ := fk.runningAddresses() if ra == nil { - t.Fatalf("returned nil but expected valid []string") + t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress") } rl := len(ra) if len(ra) != 2 { @@ -609,11 +628,11 @@ func TestRunningAddressesWithPublishStatusAddresses(t *testing.T) { } rv := ra[0] rv2 := ra[1] - if rv != "127.0.0.1" { - t.Errorf("returned %v but expected %v", rv, "127.0.0.1") + if rv.IP != "127.0.0.1" { + t.Errorf("returned %v but expected %v", rv, apiv1.LoadBalancerIngress{IP: "127.0.0.1"}) } - if rv2 != "1.1.1.1" { - t.Errorf("returned %v but expected %v", rv2, "1.1.1.1") + if rv2.IP != "1.1.1.1" { + t.Errorf("returned %v but expected %v", rv2, apiv1.LoadBalancerIngress{IP: "1.1.1.1"}) } } @@ -623,7 +642,7 @@ func TestRunningAddressesWithPublishStatusAddressesAndSpaces(t *testing.T) { ra, _ := fk.runningAddresses() if ra == nil { - t.Fatalf("returned nil but expected valid []string") + t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngresst") } rl := len(ra) if len(ra) != 2 { @@ -631,22 +650,22 @@ func TestRunningAddressesWithPublishStatusAddressesAndSpaces(t *testing.T) { } rv := ra[0] rv2 := ra[1] - if rv != "127.0.0.1" { - t.Errorf("returned %v but expected %v", rv, "127.0.0.1") + if rv.IP != "127.0.0.1" { + t.Errorf("returned %v but expected %v", rv, apiv1.LoadBalancerIngress{IP: "127.0.0.1"}) } - if rv2 != "1.1.1.1" { - t.Errorf("returned %v but expected %v", rv2, "1.1.1.1") + if rv2.IP != "1.1.1.1" { + t.Errorf("returned %v but expected %v", rv2, apiv1.LoadBalancerIngress{IP: "1.1.1.1"}) } } -func TestSliceToStatus(t *testing.T) { - fkEndpoints := []string{ - "10.0.0.1", - "2001:db8::68", - "opensource-k8s-ingress", +func TestStandardizeLoadBalancerIngresses(t *testing.T) { + fkEndpoints := []apiv1.LoadBalancerIngress{ + {IP: "2001:db8::68"}, + {IP: "10.0.0.1"}, + {Hostname: "opensource-k8s-ingress"}, } - r := sliceToStatus(fkEndpoints) + r := standardizeLoadBalancerIngresses(fkEndpoints) if r == nil { t.Fatalf("returned nil but expected a valid []apiv1.LoadBalancerIngress")