Repository: zalando-incubator/kube-ingress-aws-controller
Pull request #431 (2021-09-01 23:02:23)

Refactors kubernetes adapter `UpdateIngressLoadBalancer` method
Update of ingress or routegroup load balancer requires only namespace and name
parameters while hostname change check could be performed upfront.

The change removes unnecessary mapping to the internal structures `*ingress` and `*routegroup`.
This also simplifies "roundtrip" ingress mapping test into just "forward"
which enables testcases for empty annotations.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>


Patch:
diff --git a/kubernetes/adapter.go b/kubernetes/adapter.go
index ba6cfc1..1f7f4f9 100644
--- a/kubernetes/adapter.go
+++ b/kubernetes/adapter.go
@@ -191,7 +191,7 @@ func (a *Adapter) newIngressFromRouteGroup(rg *routegroup) *Ingress {
 }
 
 // parseAnnotations parses the ingress configuration from the annotations of an
-// Ingress or ReouteGroup resource.
+// Ingress or RouteGroup resource.
 func (a *Adapter) parseAnnotations(annotations map[string]string) *Ingress {
 	var scheme string
 	// Set schema to default if annotation value is not valid
@@ -248,60 +248,6 @@ func (a *Adapter) parseAnnotations(annotations map[string]string) *Ingress {
 	}
 }
 
-func newMetadataForKube(i *Ingress) kubeItemMetadata {
-	shared := "true"
-	if !i.Shared {
-		shared = "false"
-	}
-
-	http2 := "true"
-	if !i.HTTP2 {
-		http2 = "false"
-	}
-
-	return kubeItemMetadata{
-		Namespace: i.Namespace,
-		Name:      i.Name,
-		Annotations: map[string]string{
-			ingressCertificateARNAnnotation:   i.CertificateARN,
-			ingressSchemeAnnotation:           i.Scheme,
-			ingressSharedAnnotation:           shared,
-			ingressHTTP2Annotation:            http2,
-			ingressSecurityGroupAnnotation:    i.SecurityGroup,
-			ingressSSLPolicyAnnotation:        i.SSLPolicy,
-			ingressALBIPAddressType:           i.IPAddressType,
-			ingressWAFWebACLIDAnnotation:      i.WAFWebACLID,
-			ingressLoadBalancerTypeAnnotation: loadBalancerTypesAWSToIngress[i.LoadBalancerType],
-		},
-	}
-}
-
-func newIngressForKube(i *Ingress) *ingress {
-	return &ingress{
-		Metadata: newMetadataForKube(i),
-		Status: ingressStatus{
-			LoadBalancer: ingressLoadBalancerStatus{
-				Ingress: []ingressLoadBalancer{
-					{Hostname: i.Hostname},
-				},
-			},
-		},
-	}
-}
-
-func newRouteGroupForKube(i *Ingress) *routegroup {
-	return &routegroup{
-		Metadata: newMetadataForKube(i),
-		Status: routegroupStatus{
-			LoadBalancer: routegroupLoadBalancerStatus{
-				Routegroup: []routegroupLoadBalancer{
-					{Hostname: i.Hostname},
-				},
-			},
-		},
-	}
-}
-
 // Get ingress class filters that are used to filter ingresses acted upon.
 func (a *Adapter) IngressFiltersString() string {
 	return strings.TrimSpace(strings.Join(a.ingressFilters, ","))
@@ -398,11 +344,15 @@ func (a *Adapter) UpdateIngressLoadBalancer(ingress *Ingress, loadBalancerDNSNam
 		loadBalancerDNSName = ""
 	}
 
+	if ingress.Hostname == loadBalancerDNSName {
+		return ErrUpdateNotNeeded
+	}
+
 	switch ingress.resourceType {
 	case ingressTypeRouteGroup:
-		return updateRoutegroupLoadBalancer(a.kubeClient, newRouteGroupForKube(ingress), loadBalancerDNSName)
+		return updateRoutegroupLoadBalancer(a.kubeClient, ingress.Namespace, ingress.Name, loadBalancerDNSName)
 	case ingressTypeIngress:
-		return a.ingressClient.updateIngressLoadBalancer(a.kubeClient, newIngressForKube(ingress), loadBalancerDNSName)
+		return a.ingressClient.updateIngressLoadBalancer(a.kubeClient, ingress.Namespace, ingress.Name, loadBalancerDNSName)
 	}
 	return fmt.Errorf("Unknown resourceType '%s', failed to update Kubernetes resource", ingress.resourceType)
 }
diff --git a/kubernetes/ingress.go b/kubernetes/ingress.go
index 71ffb13..67f9f1e 100644
--- a/kubernetes/ingress.go
+++ b/kubernetes/ingress.go
@@ -112,14 +112,7 @@ type patchIngressStatus struct {
 	Status ingressStatus `json:"status"`
 }
 
-func (ic *ingressClient) updateIngressLoadBalancer(c client, i *ingress, newHostName string) error {
-	ns, name := i.Metadata.Namespace, i.Metadata.Name
-	for _, ingressLb := range i.Status.LoadBalancer.Ingress {
-		if ingressLb.Hostname == newHostName {
-			return ErrUpdateNotNeeded
-		}
-	}
-
+func (ic *ingressClient) updateIngressLoadBalancer(c client, ns, name, newHostName string) error {
 	patchStatus := patchIngressStatus{
 		Status: ingressStatus{
 			LoadBalancer: ingressLoadBalancerStatus{
diff --git a/kubernetes/routegroup.go b/kubernetes/routegroup.go
index 6eaadd9..3113915 100644
--- a/kubernetes/routegroup.go
+++ b/kubernetes/routegroup.go
@@ -71,14 +71,7 @@ type patchRoutegroupStatus struct {
 	Status routegroupStatus `json:"status"`
 }
 
-func updateRoutegroupLoadBalancer(c client, rg *routegroup, newHostName string) error {
-	ns, name := rg.Metadata.Namespace, rg.Metadata.Name
-	for _, routegroupLb := range rg.Status.LoadBalancer.Routegroup {
-		if routegroupLb.Hostname == newHostName {
-			return ErrUpdateNotNeeded
-		}
-	}
-
+func updateRoutegroupLoadBalancer(c client, ns, name, newHostName string) error {
 	patchStatus := patchRoutegroupStatus{
 		Status: routegroupStatus{
 			LoadBalancer: routegroupLoadBalancerStatus{


Test patch:
diff --git a/kubernetes/adapter_test.go b/kubernetes/adapter_test.go
index ad963fa..e18240f 100644
--- a/kubernetes/adapter_test.go
+++ b/kubernetes/adapter_test.go
@@ -27,7 +27,7 @@ var (
 	testWAFWebACLID                 = "zbr-1234"
 )
 
-func TestMappingRoundtrip(tt *testing.T) {
+func TestNewIngressFromKube(tt *testing.T) {
 	for _, tc := range []struct {
 		msg         string
 		ingress     *Ingress
@@ -233,11 +233,6 @@ func TestMappingRoundtrip(tt *testing.T) {
 			got := a.newIngressFromKube(tc.kubeIngress)
 			assert.Equal(t, tc.ingress, got, "mapping from kubernetes ingress to adapter failed")
 			assert.Equal(t, got.String(), fmt.Sprintf("%s/%s", tc.ingress.Namespace, tc.ingress.Name), "wrong value from String()")
-
-			tc.kubeIngress.Status.LoadBalancer.Ingress = tc.kubeIngress.Status.LoadBalancer.Ingress[1:]
-			gotKube := newIngressForKube(got)
-			assert.Equal(t, tc.kubeIngress.Metadata, gotKube.Metadata, "mapping from adapter to kubernetes ingress failed")
-			assert.Equal(t, tc.kubeIngress.Status, gotKube.Status, "mapping from adapter to kubernetes ingress failed")
 		})
 	}
 }
@@ -316,7 +311,7 @@ func TestListIngress(t *testing.T) {
 	}
 }
 
-func TestUpdateIngressLoadBalancer(t *testing.T) {
+func TestAdapterUpdateIngressLoadBalancer(t *testing.T) {
 	a, _ := NewAdapter(testConfig, IngressAPIVersionNetworking, testIngressFilter, testSecurityGroup, testSSLPolicy, testLoadBalancerTypeAWS, DefaultClusterLocalDomain, false)
 	client := &mockClient{}
 	a.kubeClient = client
@@ -327,6 +322,9 @@ func TestUpdateIngressLoadBalancer(t *testing.T) {
 		CertificateARN: "zbr",
 		resourceType:   ingressTypeIngress,
 	}
+	if err := a.UpdateIngressLoadBalancer(ing, "bar"); err != ErrUpdateNotNeeded {
+		t.Error("expected ErrUpdateNotNeeded")
+	}
 	if err := a.UpdateIngressLoadBalancer(ing, "xpto"); err != nil {
 		t.Error(err)
 	}
@@ -353,6 +351,9 @@ func TestUpdateRouteGroupLoadBalancer(t *testing.T) {
 		CertificateARN: "zbr",
 		resourceType:   ingressTypeRouteGroup,
 	}
+	if err := a.UpdateIngressLoadBalancer(ing, "bar"); err != ErrUpdateNotNeeded {
+		t.Error("expected ErrUpdateNotNeeded")
+	}
 	if err := a.UpdateIngressLoadBalancer(ing, "xpto"); err != nil {
 		t.Error(err)
 	}
diff --git a/kubernetes/ingress_test.go b/kubernetes/ingress_test.go
index 03dcc60..2d4724a 100644
--- a/kubernetes/ingress_test.go
+++ b/kubernetes/ingress_test.go
@@ -63,7 +63,7 @@ func TestListIngressFailureScenarios(t *testing.T) {
 	}
 }
 
-func TestUpdateIngressLoaBalancer(t *testing.T) {
+func TestUpdateIngressLoadBalancer(t *testing.T) {
 	expectedContentType := map[string]bool{
 		"application/json-patch+json":            true,
 		"application/merge-patch+json":           true,
@@ -95,14 +95,8 @@ func TestUpdateIngressLoaBalancer(t *testing.T) {
 	cfg := &Config{BaseURL: testServer.URL}
 	kubeClient, _ := newSimpleClient(cfg, false)
 	ingressClient := &ingressClient{apiVersion: IngressAPIVersionNetworking}
-	ing := &ingress{
-		Metadata: kubeItemMetadata{
-			Namespace: "foo",
-			Name:      "bar",
-		},
-	}
 
-	if err := ingressClient.updateIngressLoadBalancer(kubeClient, ing, "example.org"); err != nil {
+	if err := ingressClient.updateIngressLoadBalancer(kubeClient, "foo", "bar", "example.org"); err != nil {
 		t.Error("unexpected result from update call:", err)
 	}
 }
@@ -118,12 +112,11 @@ func TestUpdateIngressFailureScenarios(t *testing.T) {
 	for _, test := range []struct {
 		ing *ingress
 	}{
-		{newIngress("foo", nil, "example.com", "")},
 		{newIngress("foo", nil, "example.org", "")},
 	} {
 		arn := getAnnotationsString(test.ing.Metadata.Annotations, ingressCertificateARNAnnotation, "<missing>")
 		t.Run(fmt.Sprintf("%v/%v", test.ing.Status.LoadBalancer.Ingress[0].Hostname, arn), func(t *testing.T) {
-			err := ingressClient.updateIngressLoadBalancer(kubeClient, test.ing, "example.com")
+			err := ingressClient.updateIngressLoadBalancer(kubeClient, test.ing.Metadata.Namespace, test.ing.Metadata.Name, "example.com")
 			if err == nil {
 				t.Error("expected an error but update ingress call succeeded")
 			}
diff --git a/kubernetes/routegroup_test.go b/kubernetes/routegroup_test.go
index 9620c71..5e1caf8 100644
--- a/kubernetes/routegroup_test.go
+++ b/kubernetes/routegroup_test.go
@@ -61,7 +61,7 @@ func TestListRoutegroupFailureScenarios(t *testing.T) {
 	}
 }
 
-func TestUpdateRoutegroupLoaBalancer(t *testing.T) {
+func TestUpdateRoutegroupLoadBalancer(t *testing.T) {
 	expectedContentType := map[string]bool{
 		"application/json-patch+json":            true,
 		"application/merge-patch+json":           true,
@@ -92,14 +92,8 @@ func TestUpdateRoutegroupLoaBalancer(t *testing.T) {
 	defer testServer.Close()
 	cfg := &Config{BaseURL: testServer.URL}
 	kubeClient, _ := newSimpleClient(cfg, false)
-	ing := &routegroup{
-		Metadata: kubeItemMetadata{
-			Namespace: "foo",
-			Name:      "bar",
-		},
-	}
 
-	if err := updateRoutegroupLoadBalancer(kubeClient, ing, "example.org"); err != nil {
+	if err := updateRoutegroupLoadBalancer(kubeClient, "foo", "bar", "example.org"); err != nil {
 		t.Error("unexpected result from update call:", err)
 	}
 }
@@ -114,12 +108,11 @@ func TestUpdateRoutegroupFailureScenarios(t *testing.T) {
 	for _, test := range []struct {
 		rg *routegroup
 	}{
-		{newRoutegroup("foo", nil, "example.com", "")},
 		{newRoutegroup("foo", nil, "example.org", "")},
 	} {
 		arn := getAnnotationsString(test.rg.Metadata.Annotations, ingressCertificateARNAnnotation, "<missing>")
 		t.Run(fmt.Sprintf("%v/%v", test.rg.Status.LoadBalancer.Routegroup[0].Hostname, arn), func(t *testing.T) {
-			err := updateRoutegroupLoadBalancer(kubeClient, test.rg, "example.com")
+			err := updateRoutegroupLoadBalancer(kubeClient, test.rg.Metadata.Namespace, test.rg.Metadata.Name, "example.com")
 			if err == nil {
 				t.Error("expected an error but update routegroup call succeeded")
 			}