From 326dd7c1c028e5bf15604aa05341245dd1c87577 Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Tue, 22 Sep 2015 23:40:59 -0700 Subject: [PATCH 1/4] Remove private struct field. It is only used at construction. --- pkg/cloudprovider/providers/aws/aws.go | 2 -- pkg/cloudprovider/providers/aws/aws_test.go | 2 -- 2 files changed, 4 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index b21b8bce787..bceec85dddd 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -172,7 +172,6 @@ type InstanceGroupInfo interface { // AWSCloud is an implementation of Interface, TCPLoadBalancer and Instances for Amazon Web Services. type AWSCloud struct { - awsServices AWSServices ec2 EC2 elb ELB asg ASG @@ -544,7 +543,6 @@ func newAWSCloud(config io.Reader, awsServices AWSServices) (*AWSCloud, error) { } awsCloud := &AWSCloud{ - awsServices: awsServices, ec2: ec2, elb: elb, asg: asg, diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 51e97cd54a6..26126ee7dcd 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -474,7 +474,6 @@ func (a *FakeASG) DescribeAutoScalingGroups(*autoscaling.DescribeAutoScalingGrou func mockInstancesResp(instances []*ec2.Instance) *AWSCloud { awsServices := NewFakeAWSServices().withInstances(instances) return &AWSCloud{ - awsServices: awsServices, ec2: awsServices.ec2, availabilityZone: awsServices.availabilityZone, } @@ -483,7 +482,6 @@ func mockInstancesResp(instances []*ec2.Instance) *AWSCloud { func mockAvailabilityZone(region string, availabilityZone string) *AWSCloud { awsServices := NewFakeAWSServices().withAz(availabilityZone) return &AWSCloud{ - awsServices: awsServices, ec2: awsServices.ec2, availabilityZone: awsServices.availabilityZone, region: region, From f71533ce20fd41716a468fe71295bf50ea9a7012 Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Sun, 4 Oct 2015 17:55:43 -0700 Subject: [PATCH 2/4] Remove unnecessary describe VPC call. The external DescribeVPCs call is unnecessary since only the VPC ID is used and it is retrieved from the EC2 metadata service. --- pkg/cloudprovider/providers/aws/aws.go | 54 ++------------------- pkg/cloudprovider/providers/aws/aws_test.go | 11 +---- 2 files changed, 7 insertions(+), 58 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index bceec85dddd..2846c14fe97 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -91,8 +91,6 @@ type EC2 interface { AuthorizeSecurityGroupIngress(*ec2.AuthorizeSecurityGroupIngressInput) (*ec2.AuthorizeSecurityGroupIngressOutput, error) RevokeSecurityGroupIngress(*ec2.RevokeSecurityGroupIngressInput) (*ec2.RevokeSecurityGroupIngressOutput, error) - DescribeVPCs(*ec2.DescribeVpcsInput) ([]*ec2.Vpc, error) - DescribeSubnets(*ec2.DescribeSubnetsInput) ([]*ec2.Subnet, error) CreateTags(*ec2.CreateTagsInput) (*ec2.CreateTagsOutput, error) @@ -377,15 +375,6 @@ func (s *awsSdkEC2) DeleteVolume(volumeID string) (resp *ec2.DeleteVolumeOutput, return s.ec2.DeleteVolume(&request) } -func (s *awsSdkEC2) DescribeVPCs(request *ec2.DescribeVpcsInput) ([]*ec2.Vpc, error) { - // VPCs are not paged - response, err := s.ec2.DescribeVpcs(request) - if err != nil { - return nil, fmt.Errorf("error listing AWS VPCs: %v", err) - } - return response.Vpcs, nil -} - func (s *awsSdkEC2) DescribeSubnets(request *ec2.DescribeSubnetsInput) ([]*ec2.Subnet, error) { // Subnets are not paged response, err := s.ec2.DescribeSubnets(request) @@ -1256,35 +1245,6 @@ func (self *AWSCloud) findVPCID() (string, error) { return "", fmt.Errorf("Could not find VPC ID in instance metadata") } -// Find the VPC which self is attached to. -func (self *AWSCloud) findVPC() (*ec2.Vpc, error) { - request := &ec2.DescribeVpcsInput{} - - // find by vpcID from metadata - vpcID, err := self.findVPCID() - if err != nil { - return nil, err - } - filters := []*ec2.Filter{newEc2Filter("vpc-id", vpcID)} - // Don't bother adding the filterTags as we know this VPC is valid for this instance from findVPCID above. - // This is important as sharing a single regional VPC with multiple per-AZ clusters is a common deployment. - request.Filters = filters - - vpcs, err := self.ec2.DescribeVPCs(request) - if err != nil { - glog.Error("error listing VPCs", err) - return nil, err - } - - if len(vpcs) == 0 { - return nil, nil - } - if len(vpcs) == 1 { - return vpcs[0], nil - } - return nil, fmt.Errorf("Found multiple matching VPCs for vpcID = %s", vpcID) -} - // Retrieves the specified security group from the AWS API, or returns nil if not found func (s *AWSCloud) findSecurityGroup(securityGroupId string) (*ec2.SecurityGroup, error) { describeSecurityGroupsRequest := &ec2.DescribeSecurityGroupsInput{ @@ -1572,13 +1532,13 @@ func (s *AWSCloud) createTags(request *ec2.CreateTagsInput) (*ec2.CreateTagsOutp } } -func (s *AWSCloud) listSubnetIDsinVPC(vpc *ec2.Vpc) ([]string, error) { +func (s *AWSCloud) listSubnetIDsinVPC(vpcId string) ([]string, error) { subnetIds := []string{} request := &ec2.DescribeSubnetsInput{} filters := []*ec2.Filter{} - filters = append(filters, newEc2Filter("vpc-id", orEmpty(vpc.VpcId))) + filters = append(filters, newEc2Filter("vpc-id", vpcId)) // Note, this will only return subnets tagged with the cluster identifier for this Kubernetes cluster. // In the case where an AZ has public & private subnets per AWS best practices, the deployment should ensure // only the public subnet (where the ELB will go) is so tagged. @@ -1629,17 +1589,13 @@ func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, p return nil, err } - vpc, err := s.findVPC() + vpcId, err := s.findVPCID() if err != nil { - glog.Error("error finding VPC", err) return nil, err } - if vpc == nil { - return nil, fmt.Errorf("Unable to find VPC") - } // Construct list of configured subnets - subnetIDs, err := s.listSubnetIDsinVPC(vpc) + subnetIDs, err := s.listSubnetIDsinVPC(vpcId) if err != nil { glog.Error("error listing subnets in VPC", err) return nil, err @@ -1650,7 +1606,7 @@ func (s *AWSCloud) EnsureTCPLoadBalancer(name, region string, publicIP net.IP, p { sgName := "k8s-elb-" + name sgDescription := "Security group for Kubernetes ELB " + name - securityGroupID, err = s.ensureSecurityGroup(sgName, sgDescription, orEmpty(vpc.VpcId)) + securityGroupID, err = s.ensureSecurityGroup(sgName, sgDescription, vpcId) if err != nil { glog.Error("Error creating load balancer security group: ", err) return nil, err diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 26126ee7dcd..e7245caf8de 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -383,10 +383,6 @@ func (ec2 *FakeEC2) RevokeSecurityGroupIngress(*ec2.RevokeSecurityGroupIngressIn panic("Not implemented") } -func (ec2 *FakeEC2) DescribeVPCs(*ec2.DescribeVpcsInput) ([]*ec2.Vpc, error) { - panic("Not implemented") -} - func (ec2 *FakeEC2) DescribeSubnets(request *ec2.DescribeSubnetsInput) ([]*ec2.Subnet, error) { ec2.DescribeSubnetsInput = request return ec2.Subnets, nil @@ -728,9 +724,6 @@ func TestSubnetIDsinVPC(t *testing.T) { } vpcID := "vpc-deadbeef" - vpc := &ec2.Vpc{ - VpcId: &vpcID, - } // test with 3 subnets from 3 different AZs subnets := make(map[int]map[string]string) @@ -745,7 +738,7 @@ func TestSubnetIDsinVPC(t *testing.T) { subnets[2]["az"] = "af-south-1c" awsServices.ec2.Subnets = constructSubnets(subnets) - result, err := c.listSubnetIDsinVPC(vpc) + result, err := c.listSubnetIDsinVPC(vpcID) if err != nil { t.Errorf("Error listing subnets: %v", err) return @@ -775,7 +768,7 @@ func TestSubnetIDsinVPC(t *testing.T) { subnets[3]["az"] = "af-south-1c" awsServices.ec2.Subnets = constructSubnets(subnets) - result, err = c.listSubnetIDsinVPC(vpc) + result, err = c.listSubnetIDsinVPC(vpcID) if err != nil { t.Errorf("Error listing subnets: %v", err) return From 4407ca9a64ffe4477233da9fd3a0c2429bffbbc1 Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Sun, 4 Oct 2015 23:48:22 -0700 Subject: [PATCH 3/4] Remove unused method. --- pkg/cloudprovider/providers/aws/aws.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 2846c14fe97..cda6ecf4938 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -1975,22 +1975,6 @@ func (s *AWSCloud) UpdateTCPLoadBalancer(name, region string, hosts []string) er return nil } -// TODO: Make efficient -func (a *AWSCloud) getInstancesByIds(ids []string) ([]*ec2.Instance, error) { - instances := []*ec2.Instance{} - for _, id := range ids { - instance, err := a.getInstanceById(id) - if err != nil { - return nil, err - } - if instance == nil { - return nil, fmt.Errorf("unable to find instance " + id) - } - instances = append(instances, instance) - } - return instances, nil -} - // Returns the instance with the specified ID func (a *AWSCloud) getInstanceById(instanceID string) (*ec2.Instance, error) { request := &ec2.DescribeInstancesInput{ From 5d5013a7221e645071e835e9e716a6dcb1339515 Mon Sep 17 00:00:00 2001 From: Trevor Pounds Date: Thu, 8 Oct 2015 23:42:41 -0700 Subject: [PATCH 4/4] Remove unreachable panic statement. The availability zone always exist since it is retrieved from the instance's EC2 metadata service during cloud provider construction. --- pkg/cloudprovider/providers/aws/aws.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index cda6ecf4938..d4cc2d01b83 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -744,10 +744,6 @@ func (aws *AWSCloud) List(filter string) ([]string, error) { // GetZone implements Zones.GetZone func (self *AWSCloud) GetZone() (cloudprovider.Zone, error) { - if self.availabilityZone == "" { - // Should be unreachable - panic("availabilityZone not set") - } return cloudprovider.Zone{ FailureDomain: self.availabilityZone, Region: self.region,