Refactor ingress status IP address (#4490)

internal/ingress/status/status.go

@@ -171,35 +171,12 @@ func NewStatusSyncer(podInfo *k8s.PodInfo, config Config) Syncer {
 // runningAddresses returns a list of IP addresses and/or FQDN where the
 // ingress controller is currently running
 func (s *statusSync) runningAddresses() ([]string, error) {
-	addrs := []string{}
+	if s.PublishStatusAddress != "" {
-
+		return []string{s.PublishStatusAddress}, nil
-	if s.PublishService != "" {
-		ns, name, _ := k8s.ParseNameNS(s.PublishService)
-		svc, err := s.Client.CoreV1().Services(ns).Get(name, metav1.GetOptions{})
-		if err != nil {
-			return nil, err
-		}
-
-		if svc.Spec.Type == apiv1.ServiceTypeExternalName {
-			addrs = append(addrs, svc.Spec.ExternalName)
-			return addrs, nil
-		}
-
-		for _, ip := range svc.Status.LoadBalancer.Ingress {
-			if ip.IP == "" {
-				addrs = append(addrs, ip.Hostname)
-			} else {
-				addrs = append(addrs, ip.IP)
-			}
-		}
-
-		addrs = append(addrs, svc.Spec.ExternalIPs...)
-		return addrs, nil
 	}
 
-	if s.PublishStatusAddress != "" {
+	if s.PublishService != "" {
-		addrs = append(addrs, s.PublishStatusAddress)
+		return statusAddressFromService(s.PublishService, s.Client)
-		return addrs, nil
 	}
 
 	// get information about all the pods running the ingress controller
@@ -210,6 +187,7 @@ func (s *statusSync) runningAddresses() ([]string, error) {
 		return nil, err
 	}
 
+	addrs := make([]string, 0)
 	for _, pod := range pods.Items {
 		// only Running pods are valid
 		if pod.Status.Phase != apiv1.PodRunning {
@@ -346,3 +324,35 @@ func ingressSliceEqual(lhs, rhs []apiv1.LoadBalancerIngress) bool {
 
 	return true
 }
+
+func statusAddressFromService(service string, kubeClient clientset.Interface) ([]string, error) {
+	ns, name, _ := k8s.ParseNameNS(service)
+	svc, err := kubeClient.CoreV1().Services(ns).Get(name, metav1.GetOptions{})
+	if err != nil {
+		return nil, err
+	}
+
+	switch svc.Spec.Type {
+	case apiv1.ServiceTypeExternalName:
+		return []string{svc.Spec.ExternalName}, nil
+	case apiv1.ServiceTypeClusterIP:
+		return []string{svc.Spec.ClusterIP}, nil
+	case apiv1.ServiceTypeNodePort:
+		return []string{svc.Spec.ClusterIP}, nil
+	case apiv1.ServiceTypeLoadBalancer:
+		addresses := []string{}
+		for _, ip := range svc.Status.LoadBalancer.Ingress {
+			if ip.IP == "" {
+				addresses = append(addresses, ip.Hostname)
+			} else {
+				addresses = append(addresses, ip.IP)
+			}
+		}
+
+		addresses = append(addresses, svc.Spec.ExternalIPs...)
+
+		return addresses, nil
+	}
+
+	return nil, fmt.Errorf("unable to extract IP address/es from service %v", service)
+}

internal/ingress/status/status_test.go

@@ -18,6 +18,7 @@ package status
 
 import (
 	"os"
+	"reflect"
 	"testing"
 	"time"
 
@@ -373,15 +374,154 @@ func TestKeyfunc(t *testing.T) {
 }
 
 func TestRunningAddresessWithPublishService(t *testing.T) {
-	fk := buildStatusSync()
+	testCases := map[string]struct {
-
+		fakeClient  *testclient.Clientset
-	r, _ := fk.runningAddresses()
+		expected    []string
-	if r == nil {
+		errExpected bool
-		t.Fatalf("returned nil but expected valid []string")
+	}{
+		"service type ClusterIP": {
+			testclient.NewSimpleClientset(
+				&apiv1.PodList{Items: []apiv1.Pod{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: apiv1.NamespaceDefault,
+						},
+						Spec: apiv1.PodSpec{
+							NodeName: "foo_node",
+						},
+						Status: apiv1.PodStatus{
+							Phase: apiv1.PodRunning,
+						},
+					},
+				},
+				},
+				&apiv1.ServiceList{Items: []apiv1.Service{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: apiv1.NamespaceDefault,
+						},
+						Spec: apiv1.ServiceSpec{
+							Type:      apiv1.ServiceTypeClusterIP,
+							ClusterIP: "1.1.1.1",
+						},
+					},
+				},
+				},
+			),
+			[]string{"1.1.1.1"},
+			false,
+		},
+		"service type NodePort": {
+			testclient.NewSimpleClientset(
+				&apiv1.ServiceList{Items: []apiv1.Service{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: apiv1.NamespaceDefault,
+						},
+						Spec: apiv1.ServiceSpec{
+							Type:      apiv1.ServiceTypeNodePort,
+							ClusterIP: "1.1.1.1",
+						},
+					},
+				},
+				},
+			),
+			[]string{"1.1.1.1"},
+			false,
+		},
+		"service type ExternalName": {
+			testclient.NewSimpleClientset(
+				&apiv1.ServiceList{Items: []apiv1.Service{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: apiv1.NamespaceDefault,
+						},
+						Spec: apiv1.ServiceSpec{
+							Type:         apiv1.ServiceTypeExternalName,
+							ExternalName: "foo.bar",
+						},
+					},
+				},
+				},
+			),
+			[]string{"foo.bar"},
+			false,
+		},
+		"service type LoadBalancer": {
+			testclient.NewSimpleClientset(
+				&apiv1.ServiceList{Items: []apiv1.Service{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: apiv1.NamespaceDefault,
+						},
+						Spec: apiv1.ServiceSpec{
+							Type: apiv1.ServiceTypeLoadBalancer,
+						},
+						Status: apiv1.ServiceStatus{
+							LoadBalancer: apiv1.LoadBalancerStatus{
+								Ingress: []apiv1.LoadBalancerIngress{
+									{
+										IP: "10.0.0.1",
+									},
+									{
+										IP:       "",
+										Hostname: "foo",
+									},
+								},
+							},
+						},
+					},
+				},
+				},
+			),
+			[]string{"10.0.0.1", "foo"},
+			false,
+		},
+		"invalid service type": {
+			testclient.NewSimpleClientset(
+				&apiv1.ServiceList{Items: []apiv1.Service{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: apiv1.NamespaceDefault,
+						},
+					},
+				},
+				},
+			),
+			nil,
+			true,
+		},
 	}
-	rl := len(r)
+
-	if len(r) != 4 {
+	for title, tc := range testCases {
-		t.Errorf("returned %v but expected %v", rl, 4)
+		t.Run(title, func(t *testing.T) {
+
+			fk := buildStatusSync()
+			fk.Config.Client = tc.fakeClient
+
+			ra, err := fk.runningAddresses()
+			if err != nil {
+				if tc.errExpected {
+					return
+				}
+
+				t.Fatalf("%v: unexpected error obtaining running address/es: %v", title, err)
+			}
+
+			if ra == nil {
+				t.Fatalf("returned nil but expected valid []string")
+			}
+
+			if !reflect.DeepEqual(tc.expected, ra) {
+				t.Errorf("returned %v but expected %v", ra, tc.expected)
+			}
+		})
 	}
 }
 
@@ -405,49 +545,21 @@ func TestRunningAddresessWithPods(t *testing.T) {
 
 func TestRunningAddresessWithPublishStatusAddress(t *testing.T) {
 	fk := buildStatusSync()
-	fk.PublishService = ""
 	fk.PublishStatusAddress = "127.0.0.1"
 
-	r, _ := fk.runningAddresses()
+	ra, _ := fk.runningAddresses()
-	if r == nil {
+	if ra == nil {
 		t.Fatalf("returned nil but expected valid []string")
 	}
-	rl := len(r)
+	rl := len(ra)
-	if len(r) != 1 {
+	if len(ra) != 1 {
 		t.Errorf("returned %v but expected %v", rl, 1)
 	}
-	rv := r[0]
+	rv := ra[0]
 	if rv != "127.0.0.1" {
 		t.Errorf("returned %v but expected %v", rv, "127.0.0.1")
 	}
 }
-
-/*
-TODO: this test requires a refactoring
-func TestUpdateStatus(t *testing.T) {
-	fk := buildStatusSync()
-	newIPs := buildLoadBalancerIngressByIP()
-	fk.updateStatus(newIPs)
-
-	fooIngress1, err1 := fk.Client.Extensions().Ingresses(apiv1.NamespaceDefault).Get("foo_ingress_1", metav1.GetOptions{})
-	if err1 != nil {
-		t.Fatalf("unexpected error")
-	}
-	fooIngress1CurIPs := fooIngress1.Status.LoadBalancer.Ingress
-	if !ingressSliceEqual(fooIngress1CurIPs, newIPs) {
-		t.Fatalf("returned %v but expected %v", fooIngress1CurIPs, newIPs)
-	}
-
-	fooIngress2, err2 := fk.Client.Extensions().Ingresses(apiv1.NamespaceDefault).Get("foo_ingress_2", metav1.GetOptions{})
-	if err2 != nil {
-		t.Fatalf("unexpected error")
-	}
-	fooIngress2CurIPs := fooIngress2.Status.LoadBalancer.Ingress
-	if !ingressSliceEqual(fooIngress2CurIPs, []apiv1.LoadBalancerIngress{}) {
-		t.Fatalf("returned %v but expected %v", fooIngress2CurIPs, []apiv1.LoadBalancerIngress{})
-	}
-}
-*/
 func TestSliceToStatus(t *testing.T) {
 	fkEndpoints := []string{
 		"10.0.0.1",