From f568b6511a08495cf3778733c4f7a3dbc9c88129 Mon Sep 17 00:00:00 2001 From: James Ravn Date: Thu, 25 Feb 2016 22:25:35 +0000 Subject: [PATCH] Handle aws implicit and shared routing tables Fix the AWS subnet lookup that checks if a subnet is public, which was missing a few cases: - Subnets without explicit routing tables, which use the main VPC routing table. - Routing tables not tagged with KubernetesCluster. The filter for this is now removed. --- pkg/cloudprovider/providers/aws/aws.go | 71 +++++++++++++------ .../providers/aws/aws_loadbalancer.go | 2 +- pkg/cloudprovider/providers/aws/aws_test.go | 35 +++++++++ 3 files changed, 84 insertions(+), 24 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index e78566ee245..19a9485675d 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -1852,11 +1852,10 @@ func (s *AWSCloud) createTags(resourceID string, tags map[string]string) error { } func (s *AWSCloud) listPublicSubnetIDsinVPC(vpcId string) ([]string, error) { - subnetIds := []string{} - sRequest := &ec2.DescribeSubnetsInput{} - filters := []*ec2.Filter{} - filters = append(filters, newEc2Filter("vpc-id", vpcId)) + vpcIdFilter := newEc2Filter("vpc-id", vpcId) + var filters []*ec2.Filter + filters = append(filters, vpcIdFilter) filters = s.addFilters(filters) sRequest.Filters = filters @@ -1867,7 +1866,7 @@ func (s *AWSCloud) listPublicSubnetIDsinVPC(vpcId string) ([]string, error) { } rRequest := &ec2.DescribeRouteTablesInput{} - rRequest.Filters = filters + rRequest.Filters = []*ec2.Filter{vpcIdFilter} rt, err := s.ec2.DescribeRouteTables(rRequest) if err != nil { @@ -1875,18 +1874,26 @@ func (s *AWSCloud) listPublicSubnetIDsinVPC(vpcId string) ([]string, error) { return nil, err } + var subnetIds []string availabilityZones := sets.NewString() for _, subnet := range subnets { az := orEmpty(subnet.AvailabilityZone) id := orEmpty(subnet.SubnetId) - if !isSubnetPublic(rt, id) { + + isPublic, err := isSubnetPublic(rt, id) + if err != nil { + return nil, err + } + if !isPublic { glog.V(2).Infof("Ignoring private subnet %q", id) continue } + if availabilityZones.Has(az) { glog.Warning("Found multiple subnets per AZ '", az, "', ignoring subnet '", id, "'") continue } + subnetIds = append(subnetIds, id) availabilityZones.Insert(az) } @@ -1894,31 +1901,49 @@ func (s *AWSCloud) listPublicSubnetIDsinVPC(vpcId string) ([]string, error) { return subnetIds, nil } -func isSubnetPublic(rt []*ec2.RouteTable, subnetID string) bool { +func isSubnetPublic(rt []*ec2.RouteTable, subnetID string) (bool, error) { + var subnetTable *ec2.RouteTable for _, table := range rt { - var found bool for _, assoc := range table.Associations { if aws.StringValue(assoc.SubnetId) == subnetID { - found = true + subnetTable = table break } } - if !found { - continue - } - for _, route := range table.Routes { - // There is no direct way in the AWS API to determine if a subnet is public or private. - // A public subnet is one which has an internet gateway route - // we look for the gatewayId and make sure it has the prefix of igw to differentiate - // from the default in-subnet route which is called "local" - // or other virtual gateway (starting with vgv) - // or vpc peering connections (starting with pcx). - if strings.HasPrefix(aws.StringValue(route.GatewayId), "igw") { - return true + } + + if subnetTable == nil { + // If there is no explicit association, the subnet will be implicitly + // associated with the VPC's main routing table. + for _, table := range rt { + for _, assoc := range table.Associations { + if aws.BoolValue(assoc.Main) == true { + glog.V(4).Infof("Assuming implicit use of main routing table %s for %s", + aws.StringValue(table.RouteTableId), subnetID) + subnetTable = table + break + } } } } - return false + + if subnetTable == nil { + return false, fmt.Errorf("Could not locate routing table for subnet %s", subnetID) + } + + for _, route := range subnetTable.Routes { + // There is no direct way in the AWS API to determine if a subnet is public or private. + // A public subnet is one which has an internet gateway route + // we look for the gatewayId and make sure it has the prefix of igw to differentiate + // from the default in-subnet route which is called "local" + // or other virtual gateway (starting with vgv) + // or vpc peering connections (starting with pcx). + if strings.HasPrefix(aws.StringValue(route.GatewayId), "igw") { + return true, nil + } + } + + return false, nil } // EnsureLoadBalancer implements LoadBalancer.EnsureLoadBalancer @@ -1963,7 +1988,7 @@ func (s *AWSCloud) EnsureLoadBalancer(name, region string, publicIP net.IP, port // Construct list of configured subnets subnetIDs, err := s.listPublicSubnetIDsinVPC(vpcId) if err != nil { - glog.Error("Error listing subnets in VPC", err) + glog.Error("Error listing subnets in VPC: ", err) return nil, err } diff --git a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go index 83b299df844..f0aa2a53736 100644 --- a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go +++ b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go @@ -53,7 +53,7 @@ func (s *AWSCloud) ensureLoadBalancer(namespacedName types.NamespacedName, name {Key: aws.String(TagNameKubernetesService), Value: aws.String(namespacedName.String())}, } - glog.Infof("Creating load balancer for %v with name: ", namespacedName, name) + glog.Infof("Creating load balancer for %v with name: %s", namespacedName, name) _, err := s.elb.CreateLoadBalancer(createRequest) if err != nil { return nil, err diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index c982b3b5084..f8282bc155c 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -769,6 +769,15 @@ func constructSubnet(id string, az string) *ec2.Subnet { } func constructRouteTables(routeTablesIn map[string]bool) (routeTablesOut []*ec2.RouteTable) { + routeTablesOut = append(routeTablesOut, + &ec2.RouteTable{ + Associations: []*ec2.RouteTableAssociation{{Main: aws.Bool(true)}}, + Routes: []*ec2.Route{{ + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-main"), + }}, + }) + for subnetID := range routeTablesIn { routeTablesOut = append( routeTablesOut, @@ -850,6 +859,32 @@ func TestSubnetIDsinVPC(t *testing.T) { } } + // test implicit routing table - when subnets are not explicitly linked to a table they should use main + awsServices.ec2.RouteTables = constructRouteTables(map[string]bool{}) + + result, err = c.listPublicSubnetIDsinVPC(vpcID) + if err != nil { + t.Errorf("Error listing subnets: %v", err) + return + } + + if len(result) != 3 { + t.Errorf("Expected 3 subnets but got %d", len(result)) + return + } + + result_set = make(map[string]bool) + for _, v := range result { + result_set[v] = true + } + + for i := range subnets { + if !result_set[subnets[i]["id"]] { + t.Errorf("Expected subnet%d '%s' in result: %v", i, subnets[i]["id"], result) + return + } + } + // test with 4 subnets from 3 different AZs // add duplicate az subnet subnets[3] = make(map[string]string)