DevFW-CICD/ingress-nginx-helm
14
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Sync Hostname and IP address from service to ingress status (#7464)

Browse source 
* 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.
This commit is contained in:
Emily L Shepherd 2021-09-07 18:41:16 +01:00 committed by GitHub
parent 33061b8cdf
commit 3c86f838d4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 101 additions and 68 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

92
internal/ingress/status/status.go
View file

@ -26,7 +26,6 @@ import (
"time" "time"
"github.com/pkg/errors" "github.com/pkg/errors"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/klog/v2" "k8s.io/klog/v2"
pool "gopkg.in/go-playground/pool.v3" pool "gopkg.in/go-playground/pool.v3"
@ -143,7 +142,7 @@ func (s *statusSync) sync(key interface{}) error {
if err != nil { if err != nil {
return err return err
} }
s.updateStatus(sliceToStatus(addrs)) s.updateStatus(standardizeLoadBalancerIngresses(addrs))
return nil return nil
} }
@ -162,13 +161,25 @@ func NewStatusSyncer(config Config) Syncer {
return st 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 // runningAddresses returns a list of IP addresses and/or FQDN where the
// ingress controller is currently running // ingress controller is currently running
func (s *statusSync) runningAddresses() ([]string, error) { func (s *statusSync) runningAddresses() ([]apiv1.LoadBalancerIngress, error) {
if s.PublishStatusAddress != "" { if s.PublishStatusAddress != "" {
re := regexp.MustCompile(`,\s*`) re := regexp.MustCompile(`,\s*`)
multipleAddrs := re.Split(s.PublishStatusAddress, -1) 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 != "" { if s.PublishService != "" {
@ -183,7 +194,7 @@ func (s *statusSync) runningAddresses() ([]string, error) {
return nil, err return nil, err
} }
addrs := make([]string, 0) addrs := make([]apiv1.LoadBalancerIngress, 0)
for i := range pods.Items { for i := range pods.Items {
pod := pods.Items[i] pod := pods.Items[i]
// only Running pods are valid // 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) name := k8s.GetNodeIPOrName(s.Client, pod.Spec.NodeName, s.UseNodeInternalIP)
if !stringInSlice(name, addrs) { if !stringInIngresses(name, addrs) {
addrs = append(addrs, name) addrs = append(addrs, nameOrIPToLoadBalancerIngress(name))
} }
} }
@ -225,17 +236,9 @@ func (s *statusSync) isRunningMultiplePods() bool {
return len(pods.Items) > 1 return len(pods.Items) > 1
} }
// sliceToStatus converts a slice of IP and/or hostnames to LoadBalancerIngress // standardizeLoadBalancerIngresses sorts the list of loadbalancer by
func sliceToStatus(endpoints []string) []apiv1.LoadBalancerIngress { // IP
lbi := []apiv1.LoadBalancerIngress{} func standardizeLoadBalancerIngresses(lbi []apiv1.LoadBalancerIngress) []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})
}
}
sort.SliceStable(lbi, func(a, b int) bool { sort.SliceStable(lbi, func(a, b int) bool {
return lbi[a].IP < lbi[b].IP return lbi[a].IP < lbi[b].IP
}) })
@ -321,7 +324,7 @@ func ingressSliceEqual(lhs, rhs []apiv1.LoadBalancerIngress) bool {
return true 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) ns, name, _ := k8s.ParseNameNS(service)
svc, err := kubeClient.CoreV1().Services(ns).Get(context.TODO(), name, metav1.GetOptions{}) svc, err := kubeClient.CoreV1().Services(ns).Get(context.TODO(), name, metav1.GetOptions{})
if err != nil { if err != nil {
@ -330,39 +333,50 @@ func statusAddressFromService(service string, kubeClient clientset.Interface) ([
switch svc.Spec.Type { switch svc.Spec.Type {
case apiv1.ServiceTypeExternalName: case apiv1.ServiceTypeExternalName:
return []string{svc.Spec.ExternalName}, nil return []apiv1.LoadBalancerIngress{{
Hostname: svc.Spec.ExternalName,
}}, nil
case apiv1.ServiceTypeClusterIP: case apiv1.ServiceTypeClusterIP:
return []string{svc.Spec.ClusterIP}, nil return []apiv1.LoadBalancerIngress{{
IP: svc.Spec.ClusterIP,
}}, nil
case apiv1.ServiceTypeNodePort: case apiv1.ServiceTypeNodePort:
addresses := sets.NewString() if svc.Spec.ExternalIPs == nil {
if svc.Spec.ExternalIPs != nil { return []apiv1.LoadBalancerIngress{{
addresses.Insert(svc.Spec.ExternalIPs...) IP: svc.Spec.ClusterIP,
} else { }}, nil
addresses.Insert(svc.Spec.ClusterIP)
} }
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: case apiv1.ServiceTypeLoadBalancer:
addresses := sets.NewString() addrs := make([]apiv1.LoadBalancerIngress, len(svc.Status.LoadBalancer.Ingress))
for _, ip := range svc.Status.LoadBalancer.Ingress { for i, ingress := range svc.Status.LoadBalancer.Ingress {
if ip.IP == "" { addrs[i] = apiv1.LoadBalancerIngress{}
addresses.Insert(ip.Hostname) if ingress.Hostname != "" {
} else { addrs[i].Hostname = ingress.Hostname
addresses.Insert(ip.IP) }
if ingress.IP != "" {
addrs[i].IP = ingress.IP
} }
} }
for _, ip := range svc.Spec.ExternalIPs {
addresses.Insert(svc.Spec.ExternalIPs...) if !stringInIngresses(ip, addrs) {
addrs = append(addrs, apiv1.LoadBalancerIngress{IP: ip})
return addresses.List(), nil }
}
return addrs, nil
} }
return nil, fmt.Errorf("unable to extract IP address/es from service %v", service) return nil, fmt.Errorf("unable to extract IP address/es from service %v", service)
} }
// stringInSlice returns true if s is in list // 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 { for _, v := range list {
if v == s { if v.IP == s || v.Hostname == s {
return true return true
} }
} }

77
internal/ingress/status/status_test.go
View file

@ -382,7 +382,7 @@ func TestKeyfunc(t *testing.T) {
func TestRunningAddressesWithPublishService(t *testing.T) { func TestRunningAddressesWithPublishService(t *testing.T) {
testCases := map[string]struct { testCases := map[string]struct {
fakeClient *testclient.Clientset fakeClient *testclient.Clientset
expected []string expected []apiv1.LoadBalancerIngress
errExpected bool errExpected bool
}{ }{
"service type ClusterIP": { "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, false,
}, },
"service type NodePort": { "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, false,
}, },
"service type ExternalName": { "service type ExternalName": {
@ -454,7 +458,9 @@ func TestRunningAddressesWithPublishService(t *testing.T) {
}, },
}, },
), ),
[]string{"foo.bar"}, []apiv1.LoadBalancerIngress{
{Hostname: "foo.bar"},
},
false, false,
}, },
"service type LoadBalancer": { "service type LoadBalancer": {
@ -478,6 +484,10 @@ func TestRunningAddressesWithPublishService(t *testing.T) {
IP: "", IP: "",
Hostname: "foo", 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, false,
}, },
"service type LoadBalancer with same externalIP and ingress IP": { "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, false,
}, },
"invalid service type": { "invalid service type": {
@ -549,7 +568,7 @@ func TestRunningAddressesWithPublishService(t *testing.T) {
} }
if ra == nil { 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) { if !reflect.DeepEqual(tc.expected, ra) {
@ -565,15 +584,15 @@ func TestRunningAddressesWithPods(t *testing.T) {
r, _ := fk.runningAddresses() r, _ := fk.runningAddresses()
if r == nil { if r == nil {
t.Fatalf("returned nil but expected valid []string") t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress")
} }
rl := len(r) rl := len(r)
if len(r) != 1 { if len(r) != 1 {
t.Fatalf("returned %v but expected %v", rl, 1) t.Fatalf("returned %v but expected %v", rl, 1)
} }
rv := r[0] rv := r[0]
if rv != "11.0.0.2" { if rv.IP != "11.0.0.2" {
t.Errorf("returned %v but expected %v", rv, "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() ra, _ := fk.runningAddresses()
if ra == nil { if ra == nil {
t.Fatalf("returned nil but expected valid []string") t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress")
} }
rl := len(ra) rl := len(ra)
if len(ra) != 1 { if len(ra) != 1 {
t.Errorf("returned %v but expected %v", rl, 1) t.Errorf("returned %v but expected %v", rl, 1)
} }
rv := ra[0] rv := ra[0]
if rv != "127.0.0.1" { if rv.IP != "127.0.0.1" {
t.Errorf("returned %v but expected %v", rv, "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() ra, _ := fk.runningAddresses()
if ra == nil { if ra == nil {
t.Fatalf("returned nil but expected valid []string") t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngress")
} }
rl := len(ra) rl := len(ra)
if len(ra) != 2 { if len(ra) != 2 {
@ -609,11 +628,11 @@ func TestRunningAddressesWithPublishStatusAddresses(t *testing.T) {
} }
rv := ra[0] rv := ra[0]
rv2 := ra[1] rv2 := ra[1]
if rv != "127.0.0.1" { if rv.IP != "127.0.0.1" {
t.Errorf("returned %v but expected %v", rv, "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" { if rv2.IP != "1.1.1.1" {
t.Errorf("returned %v but expected %v", rv2, "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() ra, _ := fk.runningAddresses()
if ra == nil { if ra == nil {
t.Fatalf("returned nil but expected valid []string") t.Fatalf("returned nil but expected valid []apiv1.LoadBalancerIngresst")
} }
rl := len(ra) rl := len(ra)
if len(ra) != 2 { if len(ra) != 2 {
@ -631,22 +650,22 @@ func TestRunningAddressesWithPublishStatusAddressesAndSpaces(t *testing.T) {
} }
rv := ra[0] rv := ra[0]
rv2 := ra[1] rv2 := ra[1]
if rv != "127.0.0.1" { if rv.IP != "127.0.0.1" {
t.Errorf("returned %v but expected %v", rv, "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" { if rv2.IP != "1.1.1.1" {
t.Errorf("returned %v but expected %v", rv2, "1.1.1.1") t.Errorf("returned %v but expected %v", rv2, apiv1.LoadBalancerIngress{IP: "1.1.1.1"})
} }
} }
func TestSliceToStatus(t *testing.T) { func TestStandardizeLoadBalancerIngresses(t *testing.T) {
fkEndpoints := []string{ fkEndpoints := []apiv1.LoadBalancerIngress{
"10.0.0.1", {IP: "2001:db8::68"},
"2001:db8::68", {IP: "10.0.0.1"},
"opensource-k8s-ingress", {Hostname: "opensource-k8s-ingress"},
} }
r := sliceToStatus(fkEndpoints) r := standardizeLoadBalancerIngresses(fkEndpoints)
if r == nil { if r == nil {
t.Fatalf("returned nil but expected a valid []apiv1.LoadBalancerIngress") t.Fatalf("returned nil but expected a valid []apiv1.LoadBalancerIngress")