Merge pull request #14255 from tpounds/cleanup-aws-elb-client-ctor

Auto commit by PR queue bot

pkg/cloudprovider/providers/aws/aws.go

@@ -172,6 +172,7 @@ type InstanceGroupInfo interface {
 type AWSCloud struct {
 	awsServices      AWSServices
 	ec2              EC2
+	elb              ELB
 	asg              ASG
 	cfg              *AWSCloudConfig
 	availabilityZone string
@@ -183,8 +184,6 @@ type AWSCloud struct {
 	selfAWSInstance *awsInstance
 
 	mutex sync.Mutex
-	// Protects elbClients
-	elbClients map[string]ELB
 }
 
 type AWSCloudConfig struct {
@@ -236,23 +235,6 @@ func (p *awsSDKProvider) Metadata() AWSMetadata {
 	return &awsSdkMetadata{}
 }
 
-// Builds an ELB client for the specified region
-func (s *AWSCloud) getELBClient(regionName string) (ELB, error) {
-	s.mutex.Lock()
-	defer s.mutex.Unlock()
-
-	elbClient, found := s.elbClients[regionName]
-	if !found {
-		var err error
-		elbClient, err = s.awsServices.LoadBalancing(regionName)
-		if err != nil {
-			return nil, err
-		}
-		s.elbClients[regionName] = elbClient
-	}
-	return elbClient, nil
-}
-
 func stringPointerArray(orig []string) []*string {
 	if orig == nil {
 		return nil
@@ -566,6 +548,11 @@ func newAWSCloud(config io.Reader, awsServices AWSServices) (*AWSCloud, error) {
 		return nil, fmt.Errorf("error creating AWS EC2 client: %v", err)
 	}
 
+	elb, err := awsServices.LoadBalancing(regionName)
+	if err != nil {
+		return nil, fmt.Errorf("error creating AWS ELB client: %v", err)
+	}
+
 	asg, err := awsServices.Autoscaling(regionName)
 	if err != nil {
 		return nil, fmt.Errorf("error creating AWS autoscaling client: %v", err)
@@ -574,11 +561,11 @@ func newAWSCloud(config io.Reader, awsServices AWSServices) (*AWSCloud, error) {
 	awsCloud := &AWSCloud{
 		awsServices:      awsServices,
 		ec2:              ec2,
+		elb:              elb,
 		asg:              asg,
 		cfg:              cfg,
 		region:           regionName,
 		availabilityZone: zone,
-		elbClients:       map[string]ELB{},
 	}
 
 	filterTags := map[string]string{}
@@ -1240,16 +1227,11 @@ func (v *AWSCloud) Release(name string) error {
 }
 
 // Gets the current load balancer state
-func (s *AWSCloud) describeLoadBalancer(region, name string) (*elb.LoadBalancerDescription, error) {
+func (s *AWSCloud) describeLoadBalancer(name string) (*elb.LoadBalancerDescription, error) {
-	elbClient, err := s.getELBClient(region)
-	if err != nil {
-		return nil, err
-	}
-
 	request := &elb.DescribeLoadBalancersInput{}
 	request.LoadBalancerNames = []*string{&name}
 
-	response, err := elbClient.DescribeLoadBalancers(request)
+	response, err := s.elb.DescribeLoadBalancers(request)
 	if err != nil {
 		if awsError, ok := err.(awserr.Error); ok {
 			if awsError.Code() == "LoadBalancerNotFound" {
@@ -1269,19 +1251,6 @@ func (s *AWSCloud) describeLoadBalancer(region, name string) (*elb.LoadBalancerD
 	return ret, nil
 }
 
-// TCPLoadBalancerExists implements TCPLoadBalancer.TCPLoadBalancerExists.
-func (self *AWSCloud) TCPLoadBalancerExists(name, region string) (bool, error) {
-	lb, err := self.describeLoadBalancer(name, region)
-	if err != nil {
-		return false, err
-	}
-
-	if lb != nil {
-		return true, nil
-	}
-	return false, nil
-}
-
 // Retrieves instance's vpc id from metadata
 func (self *AWSCloud) findVPCID() (string, error) {
 
@@ -1626,9 +1595,8 @@ func (s *AWSCloud) createTags(request *ec2.CreateTagsInput) (*ec2.CreateTagsOutp
 func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, ports []*api.ServicePort, hosts []string, affinity api.ServiceAffinity) (*api.LoadBalancerStatus, error) {
 	glog.V(2).Infof("EnsureTCPLoadBalancer(%v, %v, %v, %v, %v)", name, region, publicIP, ports, hosts)
 
-	elbClient, err := s.getELBClient(region)
+	if region != s.region {
-	if err != nil {
+		return nil, fmt.Errorf("requested load balancer region '%s' does not match cluster region '%s'", region, s.region)
-		return nil, err
 	}
 
 	if affinity != api.ServiceAffinityNone {
@@ -1733,12 +1701,12 @@ func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, p
 	}
 
 	// Build the load balancer itself
-	loadBalancer, err := s.ensureLoadBalancer(region, name, listeners, subnetIDs, securityGroupIDs)
+	loadBalancer, err := s.ensureLoadBalancer(name, listeners, subnetIDs, securityGroupIDs)
 	if err != nil {
 		return nil, err
 	}
 
-	err = s.ensureLoadBalancerHealthCheck(region, loadBalancer, listeners)
+	err = s.ensureLoadBalancerHealthCheck(loadBalancer, listeners)
 	if err != nil {
 		return nil, err
 	}
@@ -1749,7 +1717,7 @@ func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, p
 		return nil, err
 	}
 
-	err = s.ensureLoadBalancerInstances(elbClient, orEmpty(loadBalancer.LoadBalancerName), loadBalancer.Instances, instances)
+	err = s.ensureLoadBalancerInstances(orEmpty(loadBalancer.LoadBalancerName), loadBalancer.Instances, instances)
 	if err != nil {
 		glog.Warning("Error registering instances with the load balancer: %v", err)
 		return nil, err
@@ -1765,7 +1733,11 @@ func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, p
 
 // GetTCPLoadBalancer is an implementation of TCPLoadBalancer.GetTCPLoadBalancer
 func (s *AWSCloud) GetTCPLoadBalancer(name, region string) (*api.LoadBalancerStatus, bool, error) {
-	lb, err := s.describeLoadBalancer(region, name)
+	if region != s.region {
+		return nil, false, fmt.Errorf("requested load balancer region '%s' does not match cluster region '%s'", region, s.region)
+	}
+
+	lb, err := s.describeLoadBalancer(name)
 	if err != nil {
 		return nil, false, err
 	}
@@ -1925,12 +1897,11 @@ func (s *AWSCloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalan
 
 // EnsureTCPLoadBalancerDeleted implements TCPLoadBalancer.EnsureTCPLoadBalancerDeleted.
 func (s *AWSCloud) EnsureTCPLoadBalancerDeleted(name, region string) error {
-	elbClient, err := s.getELBClient(region)
+	if region != s.region {
-	if err != nil {
+		return fmt.Errorf("requested load balancer region '%s' does not match cluster region '%s'", region, s.region)
-		return err
 	}
 
-	lb, err := s.describeLoadBalancer(region, name)
+	lb, err := s.describeLoadBalancer(name)
 	if err != nil {
 		return err
 	}
@@ -1954,7 +1925,7 @@ func (s *AWSCloud) EnsureTCPLoadBalancerDeleted(name, region string) error {
 		request := &elb.DeleteLoadBalancerInput{}
 		request.LoadBalancerName = lb.LoadBalancerName
 
-		_, err = elbClient.DeleteLoadBalancer(request)
+		_, err = s.elb.DeleteLoadBalancer(request)
 		if err != nil {
 			// TODO: Check if error was because load balancer was concurrently deleted
 			glog.Error("error deleting load balancer: ", err)
@@ -2020,17 +1991,16 @@ func (s *AWSCloud) EnsureTCPLoadBalancerDeleted(name, region string) error {
 
 // UpdateTCPLoadBalancer implements TCPLoadBalancer.UpdateTCPLoadBalancer
 func (s *AWSCloud) UpdateTCPLoadBalancer(name, region string, hosts []string) error {
+	if region != s.region {
+		return fmt.Errorf("requested load balancer region '%s' does not match cluster region '%s'", region, s.region)
+	}
+
 	instances, err := s.getInstancesByNodeNames(hosts)
 	if err != nil {
 		return err
 	}
 
-	elbClient, err := s.getELBClient(region)
+	lb, err := s.describeLoadBalancer(name)
-	if err != nil {
-		return err
-	}
-
-	lb, err := s.describeLoadBalancer(region, name)
 	if err != nil {
 		return err
 	}
@@ -2039,7 +2009,7 @@ func (s *AWSCloud) UpdateTCPLoadBalancer(name, region string, hosts []string) er
 		return fmt.Errorf("Load balancer not found")
 	}
 
-	err = s.ensureLoadBalancerInstances(elbClient, orEmpty(lb.LoadBalancerName), lb.Instances, instances)
+	err = s.ensureLoadBalancerInstances(orEmpty(lb.LoadBalancerName), lb.Instances, instances)
 	if err != nil {
 		return nil
 	}

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

@@ -27,13 +27,8 @@ import (
 	"k8s.io/kubernetes/pkg/util/sets"
 )
 
-func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.Listener, subnetIDs []string, securityGroupIDs []string) (*elb.LoadBalancerDescription, error) {
+func (s *AWSCloud) ensureLoadBalancer(name string, listeners []*elb.Listener, subnetIDs []string, securityGroupIDs []string) (*elb.LoadBalancerDescription, error) {
-	elbClient, err := s.getELBClient(region)
+	loadBalancer, err := s.describeLoadBalancer(name)
-	if err != nil {
-		return nil, err
-	}
-
-	loadBalancer, err := s.describeLoadBalancer(region, name)
 	if err != nil {
 		return nil, err
 	}
@@ -53,7 +48,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 		createRequest.SecurityGroups = stringPointerArray(securityGroupIDs)
 
 		glog.Info("Creating load balancer with name: ", name)
-		_, err := elbClient.CreateLoadBalancer(createRequest)
+		_, err := s.elb.CreateLoadBalancer(createRequest)
 		if err != nil {
 			return nil, err
 		}
@@ -72,7 +67,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 				request.LoadBalancerName = aws.String(name)
 				request.Subnets = stringSetToPointers(removals)
 				glog.V(2).Info("Detaching load balancer from removed subnets")
-				_, err := elbClient.DetachLoadBalancerFromSubnets(request)
+				_, err := s.elb.DetachLoadBalancerFromSubnets(request)
 				if err != nil {
 					return nil, fmt.Errorf("error detaching AWS loadbalancer from subnets: %v", err)
 				}
@@ -84,7 +79,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 				request.LoadBalancerName = aws.String(name)
 				request.Subnets = stringSetToPointers(additions)
 				glog.V(2).Info("Attaching load balancer to added subnets")
-				_, err := elbClient.AttachLoadBalancerToSubnets(request)
+				_, err := s.elb.AttachLoadBalancerToSubnets(request)
 				if err != nil {
 					return nil, fmt.Errorf("error attaching AWS loadbalancer to subnets: %v", err)
 				}
@@ -103,7 +98,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 				request.LoadBalancerName = aws.String(name)
 				request.SecurityGroups = stringPointerArray(securityGroupIDs)
 				glog.V(2).Info("Applying updated security groups to load balancer")
-				_, err := elbClient.ApplySecurityGroupsToLoadBalancer(request)
+				_, err := s.elb.ApplySecurityGroupsToLoadBalancer(request)
 				if err != nil {
 					return nil, fmt.Errorf("error applying AWS loadbalancer security groups: %v", err)
 				}
@@ -163,7 +158,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 				request.LoadBalancerName = aws.String(name)
 				request.LoadBalancerPorts = removals
 				glog.V(2).Info("Deleting removed load balancer listeners")
-				_, err := elbClient.DeleteLoadBalancerListeners(request)
+				_, err := s.elb.DeleteLoadBalancerListeners(request)
 				if err != nil {
 					return nil, fmt.Errorf("error deleting AWS loadbalancer listeners: %v", err)
 				}
@@ -175,7 +170,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 				request.LoadBalancerName = aws.String(name)
 				request.Listeners = additions
 				glog.V(2).Info("Creating added load balancer listeners")
-				_, err := elbClient.CreateLoadBalancerListeners(request)
+				_, err := s.elb.CreateLoadBalancerListeners(request)
 				if err != nil {
 					return nil, fmt.Errorf("error creating AWS loadbalancer listeners: %v", err)
 				}
@@ -185,7 +180,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 	}
 
 	if dirty {
-		loadBalancer, err = s.describeLoadBalancer(region, name)
+		loadBalancer, err = s.describeLoadBalancer(name)
 		if err != nil {
 			glog.Warning("Unable to retrieve load balancer after creation/update")
 			return nil, err
@@ -196,12 +191,7 @@ func (s *AWSCloud) ensureLoadBalancer(region, name string, listeners []*elb.List
 }
 
 // Makes sure that the health check for an ELB matches the configured listeners
-func (s *AWSCloud) ensureLoadBalancerHealthCheck(region string, loadBalancer *elb.LoadBalancerDescription, listeners []*elb.Listener) error {
+func (s *AWSCloud) ensureLoadBalancerHealthCheck(loadBalancer *elb.LoadBalancerDescription, listeners []*elb.Listener) error {
-	elbClient, err := s.getELBClient(region)
-	if err != nil {
-		return err
-	}
-
 	actual := loadBalancer.HealthCheck
 
 	// Default AWS settings
@@ -245,7 +235,7 @@ func (s *AWSCloud) ensureLoadBalancerHealthCheck(region string, loadBalancer *el
 	request.HealthCheck = healthCheck
 	request.LoadBalancerName = loadBalancer.LoadBalancerName
 
-	_, err = elbClient.ConfigureHealthCheck(request)
+	_, err := s.elb.ConfigureHealthCheck(request)
 	if err != nil {
 		return fmt.Errorf("error configuring load-balancer health-check: %v", err)
 	}
@@ -254,7 +244,7 @@ func (s *AWSCloud) ensureLoadBalancerHealthCheck(region string, loadBalancer *el
 }
 
 // Makes sure that exactly the specified hosts are registered as instances with the load balancer
-func (s *AWSCloud) ensureLoadBalancerInstances(elbClient ELB, loadBalancerName string, lbInstances []*elb.Instance, instances []*ec2.Instance) error {
+func (s *AWSCloud) ensureLoadBalancerInstances(loadBalancerName string, lbInstances []*elb.Instance, instances []*ec2.Instance) error {
 	expected := sets.NewString()
 	for _, instance := range instances {
 		expected.Insert(orEmpty(instance.InstanceID))
@@ -286,7 +276,7 @@ func (s *AWSCloud) ensureLoadBalancerInstances(elbClient ELB, loadBalancerName s
 		registerRequest := &elb.RegisterInstancesWithLoadBalancerInput{}
 		registerRequest.Instances = addInstances
 		registerRequest.LoadBalancerName = aws.String(loadBalancerName)
-		_, err := elbClient.RegisterInstancesWithLoadBalancer(registerRequest)
+		_, err := s.elb.RegisterInstancesWithLoadBalancer(registerRequest)
 		if err != nil {
 			return err
 		}
@@ -297,7 +287,7 @@ func (s *AWSCloud) ensureLoadBalancerInstances(elbClient ELB, loadBalancerName s
 		deregisterRequest := &elb.DeregisterInstancesFromLoadBalancerInput{}
 		deregisterRequest.Instances = removeInstances
 		deregisterRequest.LoadBalancerName = aws.String(loadBalancerName)
-		_, err := elbClient.DeregisterInstancesFromLoadBalancer(deregisterRequest)
+		_, err := s.elb.DeregisterInstancesFromLoadBalancer(deregisterRequest)
 		if err != nil {
 			return err
 		}

pkg/cloudprovider/providers/aws/aws_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package aws_cloud
 
 import (
+	"fmt"
 	"io"
 	"reflect"
 	"strings"
@@ -664,3 +665,35 @@ func TestFindVPCID(t *testing.T) {
 		t.Errorf("Unexpected vpcID: %s", vpcID)
 	}
 }
+
+func TestLoadBalancerMatchesClusterRegion(t *testing.T) {
+	awsServices := NewFakeAWSServices()
+	c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
+	if err != nil {
+		t.Errorf("Error building aws cloud: %v", err)
+		return
+	}
+
+	badELBRegion := "bad-elb-region"
+	errorMessage := fmt.Sprintf("requested load balancer region '%s' does not match cluster region '%s'", badELBRegion, c.region)
+
+	_, _, err = c.GetTCPLoadBalancer("elb-name", badELBRegion)
+	if err == nil || err.Error() != errorMessage {
+		t.Errorf("Expected GetTCPLoadBalancer region mismatch error.")
+	}
+
+	_, err = c.EnsureTCPLoadBalancer("elb-name", badELBRegion, nil, nil, nil, api.ServiceAffinityNone)
+	if err == nil || err.Error() != errorMessage {
+		t.Errorf("Expected EnsureTCPLoadBalancer region mismatch error.")
+	}
+
+	err = c.EnsureTCPLoadBalancerDeleted("elb-name", badELBRegion)
+	if err == nil || err.Error() != errorMessage {
+		t.Errorf("Expected EnsureTCPLoadBalancerDeleted region mismatch error.")
+	}
+
+	err = c.UpdateTCPLoadBalancer("elb-name", badELBRegion, nil)
+	if err == nil || err.Error() != errorMessage {
+		t.Errorf("Expected UpdateTCPLoadBalancer region mismatch error.")
+	}
+}