begin adding unit tests for ensureLoadBalancer

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

@@ -1098,59 +1098,14 @@ func (c *Cloud) ensureLoadBalancer(namespacedName types.NamespacedName, loadBala
 		}
 
 		{
-			// Sync listeners
+			additions, removals := syncElbListeners(loadBalancerName, listeners, loadBalancer.ListenerDescriptions)
-			listenerDescriptions := loadBalancer.ListenerDescriptions
-
-			foundSet := make(map[int]bool)
-			removals := []*int64{}
-			for _, listenerDescription := range listenerDescriptions {
-				actual := listenerDescription.Listener
-				if actual == nil {
-					klog.Warning("Ignoring empty listener in AWS loadbalancer: ", loadBalancerName)
-					continue
-				}
-
-				found := -1
-				for i, expected := range listeners {
-					if !elbProtocolsAreEqual(actual.Protocol, expected.Protocol) {
-						continue
-					}
-					if !elbProtocolsAreEqual(actual.InstanceProtocol, expected.InstanceProtocol) {
-						continue
-					}
-					if aws.Int64Value(actual.InstancePort) != aws.Int64Value(expected.InstancePort) {
-						continue
-					}
-					if aws.Int64Value(actual.LoadBalancerPort) != aws.Int64Value(expected.LoadBalancerPort) {
-						continue
-					}
-					if !awsArnEquals(actual.SSLCertificateId, expected.SSLCertificateId) {
-						continue
-					}
-					found = i
-				}
-				if found != -1 {
-					foundSet[found] = true
-				} else {
-					removals = append(removals, actual.LoadBalancerPort)
-				}
-			}
-
-			additions := []*elb.Listener{}
-			for i := range listeners {
-				if foundSet[i] {
-					continue
-				}
-				additions = append(additions, listeners[i])
-			}
 
 			if len(removals) != 0 {
 				request := &elb.DeleteLoadBalancerListenersInput{}
 				request.LoadBalancerName = aws.String(loadBalancerName)
 				request.LoadBalancerPorts = removals
 				klog.V(2).Info("Deleting removed load balancer listeners")
-				_, err := c.elb.DeleteLoadBalancerListeners(request)
+				if _, err := c.elb.DeleteLoadBalancerListeners(request); err != nil {
-				if err != nil {
 					return nil, fmt.Errorf("error deleting AWS loadbalancer listeners: %q", err)
 				}
 				dirty = true
@@ -1161,8 +1116,7 @@ func (c *Cloud) ensureLoadBalancer(namespacedName types.NamespacedName, loadBala
 				request.LoadBalancerName = aws.String(loadBalancerName)
 				request.Listeners = additions
 				klog.V(2).Info("Creating added load balancer listeners")
-				_, err := c.elb.CreateLoadBalancerListeners(request)
+				if _, err := c.elb.CreateLoadBalancerListeners(request); err != nil {
-				if err != nil {
 					return nil, fmt.Errorf("error creating AWS loadbalancer listeners: %q", err)
 				}
 				dirty = true
@@ -1289,6 +1243,68 @@ func (c *Cloud) ensureLoadBalancer(namespacedName types.NamespacedName, loadBala
 	return loadBalancer, nil
 }
 
+// syncElbListeners computes a plan to reconcile the desired vs actual state of the listeners on an ELB
+// NOTE: there exists an O(nlgn) implementation for this function. However, as the default limit of
+//       listeners per elb is 100, this implementation is reduced from O(m*n) => O(n).
+func syncElbListeners(loadBalancerName string, listeners []*elb.Listener, listenerDescriptions []*elb.ListenerDescription) ([]*elb.Listener, []*int64) {
+	foundSet := make(map[int]bool)
+	removals := []*int64{}
+	additions := []*elb.Listener{}
+
+	for _, listenerDescription := range listenerDescriptions {
+		actual := listenerDescription.Listener
+		if actual == nil {
+			klog.Warning("Ignoring empty listener in AWS loadbalancer: ", loadBalancerName)
+			continue
+		}
+
+		found := false
+		for i, expected := range listeners {
+			if expected == nil {
+				klog.Warning("Ignoring empty desired listener")
+				continue
+			}
+			if elbListenersAreEqual(actual, expected) {
+				// The current listener on the actual
+				// elb is in the set of desired listeners.
+				foundSet[i] = true
+				found = true
+				break
+			}
+		}
+		if !found {
+			removals = append(removals, actual.LoadBalancerPort)
+		}
+	}
+
+	for i := range listeners {
+		if !foundSet[i] {
+			additions = append(additions, listeners[i])
+		}
+	}
+
+	return additions, removals
+}
+
+func elbListenersAreEqual(actual, expected *elb.Listener) bool {
+	if !elbProtocolsAreEqual(actual.Protocol, expected.Protocol) {
+		return false
+	}
+	if !elbProtocolsAreEqual(actual.InstanceProtocol, expected.InstanceProtocol) {
+		return false
+	}
+	if aws.Int64Value(actual.InstancePort) != aws.Int64Value(expected.InstancePort) {
+		return false
+	}
+	if aws.Int64Value(actual.LoadBalancerPort) != aws.Int64Value(expected.LoadBalancerPort) {
+		return false
+	}
+	if !awsArnEquals(actual.SSLCertificateId, expected.SSLCertificateId) {
+		return false
+	}
+	return true
+}
+
 func createSubnetMappings(subnetIDs []string) []*elbv2.SubnetMapping {
 	response := []*elbv2.SubnetMapping{}
 

pkg/cloudprovider/providers/aws/aws_loadbalancer_test.go

@@ -22,6 +22,8 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/elb"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestElbProtocolsAreEqual(t *testing.T) {
@@ -222,3 +224,135 @@ func TestSecurityGroupFiltering(t *testing.T) {
 	}
 
 }
+
+func TestSyncElbListeners(t *testing.T) {
+	tests := []struct {
+		name                 string
+		loadBalancerName     string
+		listeners            []*elb.Listener
+		listenerDescriptions []*elb.ListenerDescription
+		toCreate             []*elb.Listener
+		toDelete             []*int64
+	}{
+		{
+			name:             "no edge cases",
+			loadBalancerName: "lb_one",
+			listeners: []*elb.Listener{
+				{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")},
+				{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP"), SSLCertificateId: aws.String("def-456")},
+				{InstancePort: aws.Int64(8443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(8443), Protocol: aws.String("TCP"), SSLCertificateId: aws.String("def-456")},
+			},
+			listenerDescriptions: []*elb.ListenerDescription{
+				{Listener: &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")}},
+				{Listener: &elb.Listener{InstancePort: aws.Int64(8443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(8443), Protocol: aws.String("TCP"), SSLCertificateId: aws.String("def-456")}},
+			},
+			toDelete: []*int64{
+				aws.Int64(80),
+			},
+			toCreate: []*elb.Listener{
+				{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")},
+				{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP"), SSLCertificateId: aws.String("def-456")},
+			},
+		},
+		{
+			name:             "no listeners to delete",
+			loadBalancerName: "lb_two",
+			listeners: []*elb.Listener{
+				{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")},
+				{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP"), SSLCertificateId: aws.String("def-456")},
+			},
+			listenerDescriptions: []*elb.ListenerDescription{
+				{Listener: &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")}},
+			},
+			toCreate: []*elb.Listener{
+				{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP"), SSLCertificateId: aws.String("def-456")},
+			},
+			toDelete: []*int64{},
+		},
+		{
+			name:             "no listeners to create",
+			loadBalancerName: "lb_three",
+			listeners: []*elb.Listener{
+				{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")},
+			},
+			listenerDescriptions: []*elb.ListenerDescription{
+				{Listener: &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")}},
+				{Listener: &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")}},
+			},
+			toDelete: []*int64{
+				aws.Int64(80),
+			},
+			toCreate: []*elb.Listener{},
+		},
+		{
+			name:             "nil actual listener",
+			loadBalancerName: "lb_four",
+			listeners: []*elb.Listener{
+				{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP")},
+			},
+			listenerDescriptions: []*elb.ListenerDescription{
+				{Listener: &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP"), SSLCertificateId: aws.String("abc-123")}},
+				{Listener: nil},
+			},
+			toDelete: []*int64{
+				aws.Int64(443),
+			},
+			toCreate: []*elb.Listener{
+				{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("HTTP")},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			additions, removals := syncElbListeners(test.loadBalancerName, test.listeners, test.listenerDescriptions)
+			assert.Equal(t, additions, test.toCreate)
+			assert.Equal(t, removals, test.toDelete)
+		})
+	}
+}
+
+func TestElbListenersAreEqual(t *testing.T) {
+	tests := []struct {
+		name             string
+		expected, actual *elb.Listener
+		equal            bool
+	}{
+		{
+			name:     "should be equal",
+			expected: &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			actual:   &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			equal:    true,
+		},
+		{
+			name:     "instance port should be different",
+			expected: &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			actual:   &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			equal:    false,
+		},
+		{
+			name:     "instance protocol should be different",
+			expected: &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("HTTP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			actual:   &elb.Listener{InstancePort: aws.Int64(80), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			equal:    false,
+		},
+		{
+			name:     "load balancer port should be different",
+			expected: &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(443), Protocol: aws.String("TCP")},
+			actual:   &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			equal:    false,
+		},
+		{
+			name:     "protocol should be different",
+			expected: &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("TCP")},
+			actual:   &elb.Listener{InstancePort: aws.Int64(443), InstanceProtocol: aws.String("TCP"), LoadBalancerPort: aws.Int64(80), Protocol: aws.String("HTTP")},
+			equal:    false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			assert.Equal(t, test.equal, elbListenersAreEqual(test.expected, test.actual))
+		})
+	}
+}