From ed8748a6b67dd11cf071391bf11bac396172bc0b Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Sun, 20 Sep 2015 22:46:08 -0700 Subject: [PATCH 1/3] Remove obsolete TCPLoadBalancer interface method. --- pkg/cloudprovider/providers/aws/aws.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 268bef4b4bf..e99d0b203ba 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -1269,19 +1269,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) { From 4775a8a5bb006c1750cf383f3354abe21d0d999a Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Sun, 20 Sep 2015 23:08:23 -0700 Subject: [PATCH 2/3] Remove superfluous ELB region lookup logic. The ELB client lookup isn't necessary because the service does not operate across regions. Instead the client should be built like the others by querying for the region from the master node's metadata service. --- pkg/cloudprovider/providers/aws/aws.go | 67 +++++-------------- .../providers/aws/aws_loadbalancer.go | 38 ++++------- 2 files changed, 31 insertions(+), 74 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index e99d0b203ba..e73257b6572 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/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) { - elbClient, err := s.getELBClient(region) - if err != nil { - return nil, err - } - +func (s *AWSCloud) describeLoadBalancer(name string) (*elb.LoadBalancerDescription, error) { 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" { @@ -1613,11 +1595,6 @@ 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 err != nil { - return nil, err - } - if affinity != api.ServiceAffinityNone { // ELB supports sticky sessions, but only when configured for HTTP/HTTPS return nil, fmt.Errorf("unsupported load balancer affinity: %v", affinity) @@ -1720,12 +1697,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 } @@ -1736,7 +1713,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 @@ -1752,7 +1729,7 @@ 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) + lb, err := s.describeLoadBalancer(name) if err != nil { return nil, false, err } @@ -1912,12 +1889,7 @@ 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 err != nil { - return err - } - - lb, err := s.describeLoadBalancer(region, name) + lb, err := s.describeLoadBalancer(name) if err != nil { return err } @@ -1941,7 +1913,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) @@ -2012,12 +1984,7 @@ func (s *AWSCloud) UpdateTCPLoadBalancer(name, region string, hosts []string) er return err } - elbClient, err := s.getELBClient(region) - if err != nil { - return err - } - - lb, err := s.describeLoadBalancer(region, name) + lb, err := s.describeLoadBalancer(name) if err != nil { return err } @@ -2026,7 +1993,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 } diff --git a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go index c3469bcddb2..c96e1155176 100644 --- a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go +++ b/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) { - elbClient, err := s.getELBClient(region) - if err != nil { - return nil, err - } - - loadBalancer, err := s.describeLoadBalancer(region, name) +func (s *AWSCloud) ensureLoadBalancer(name string, listeners []*elb.Listener, subnetIDs []string, securityGroupIDs []string) (*elb.LoadBalancerDescription, error) { + loadBalancer, err := s.describeLoadBalancer(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 { - elbClient, err := s.getELBClient(region) - if err != nil { - return err - } - +func (s *AWSCloud) ensureLoadBalancerHealthCheck(loadBalancer *elb.LoadBalancerDescription, listeners []*elb.Listener) error { 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 } From 1a7f8dc30c16b53a4518b44c7915da16642b6cd7 Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Mon, 21 Sep 2015 11:44:26 -0700 Subject: [PATCH 3/3] Check load balancer and cluster have the same region. --- pkg/cloudprovider/providers/aws/aws.go | 16 ++++++++++ pkg/cloudprovider/providers/aws/aws_test.go | 33 +++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index e73257b6572..fb85af28153 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -1595,6 +1595,10 @@ 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) + if region != s.region { + return nil, fmt.Errorf("requested load balancer region '%s' does not match cluster region '%s'", region, s.region) + } + if affinity != api.ServiceAffinityNone { // ELB supports sticky sessions, but only when configured for HTTP/HTTPS return nil, fmt.Errorf("unsupported load balancer affinity: %v", affinity) @@ -1729,6 +1733,10 @@ 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) { + 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 @@ -1889,6 +1897,10 @@ func (s *AWSCloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalan // EnsureTCPLoadBalancerDeleted implements TCPLoadBalancer.EnsureTCPLoadBalancerDeleted. func (s *AWSCloud) EnsureTCPLoadBalancerDeleted(name, region string) error { + if region != s.region { + return 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 err @@ -1979,6 +1991,10 @@ 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 diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 07171a11827..67fa86de33f 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/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.") + } +}