From 3dfeb51745b7caba1ea831e794fa894d681a9f81 Mon Sep 17 00:00:00 2001 From: "GRECO, FRANK" Date: Wed, 10 Apr 2019 10:04:25 -0700 Subject: [PATCH 1/2] begin adding unit tests for ensureLoadBalancer --- .../providers/aws/aws_loadbalancer.go | 114 ++++++++------- .../providers/aws/aws_loadbalancer_test.go | 134 ++++++++++++++++++ 2 files changed, 199 insertions(+), 49 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go index 5bdbddb6f6..f746fa5619 100644 --- a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go +++ b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go @@ -1098,59 +1098,14 @@ func (c *Cloud) ensureLoadBalancer(namespacedName types.NamespacedName, loadBala } { - // Sync listeners - 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]) - } + additions, removals := syncElbListeners(loadBalancerName, listeners, loadBalancer.ListenerDescriptions) 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 != nil { + if _, err := c.elb.DeleteLoadBalancerListeners(request); 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 != nil { + if _, err := c.elb.CreateLoadBalancerListeners(request); 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{} diff --git a/pkg/cloudprovider/providers/aws/aws_loadbalancer_test.go b/pkg/cloudprovider/providers/aws/aws_loadbalancer_test.go index cd5d79ba7a..0dc09bb7c9 100644 --- a/pkg/cloudprovider/providers/aws/aws_loadbalancer_test.go +++ b/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)) + }) + } +} From 91cdcceab5fe546abd0f5146967637ea717b4ae9 Mon Sep 17 00:00:00 2001 From: "GRECO, FRANK" Date: Sun, 14 Apr 2019 16:37:54 -0700 Subject: [PATCH 2/2] adding loadbalancer name to log message --- pkg/cloudprovider/providers/aws/aws_loadbalancer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go index f746fa5619..3d2920701f 100644 --- a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go +++ b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go @@ -1261,7 +1261,7 @@ func syncElbListeners(loadBalancerName string, listeners []*elb.Listener, listen found := false for i, expected := range listeners { if expected == nil { - klog.Warning("Ignoring empty desired listener") + klog.Warning("Ignoring empty desired listener for loadbalancer: ", loadBalancerName) continue } if elbListenersAreEqual(actual, expected) {