Merge pull request #22019 from sky-uk/aws-handle-implicit-routing-tables

Auto commit by PR queue bot

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
 	}
 

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

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)