More reliable public subnet detection (#560)

* More reliable public subnet detection

Look for an internet-facing route in the subnet's route table to
determine whether this subnet is public or private.

* More reliable public subnet detection

Introduce a unit test for the new behavior and fix up the old unit tests
to handle the extra ec2 calls.

Also includes some review feedback, and preserves the intent of whether
a managed subnet should be public or private as a tag to differentiate
subnets that are not public from ones that are not public yet.

* More reliable public subnet detection

Rewords what defines a subnet as "public" to match the AWS docs, and
removes the dependency on `describeVpcInternetGateways`, preferring to
check whether the route's gateway id begins with `igw` instead.

* More reliable public subnet detection

Switch to using the "cluster api role" tag for public/private-ness, and
simplify the fact-finding block with some helpers and by moving things
around..

* More reliable subnet detection

Run gofmt

pkg/apis/awsprovider/v1alpha1/awsclusterproviderconfig_types.go

@@ -127,18 +127,18 @@ type SubnetSpec struct {
 	// AvailabilityZone defines the availability zone to use for this subnet in the cluster's region.
 	AvailabilityZone string `json:"availabilityZone,omitempty"`
 
-	// IsPublic defines the subnet as a public subnet. Refer to the AWS documentation for further information.
+	// IsPublic defines the subnet as a public subnet. A subnet is public when it is associated with a route table that has a route to an internet gateway.
 	// +optional
-	IsPublic bool `json:"isPublic,omitempty"`
+	IsPublic bool `json:"isPublic"`
 
 	// RouteTableID is the routing table id associated with the subnet.
 	// +optional
 	RouteTableID *string `json:"routeTableId"`
 
 	// NatGatewayID is the NAT gateway id associated with the subnet.
-	// Ignored if the subnet is public.
+	// Ignored unless the subnet is managed by the provider, in which case this is set on the public subnet where the nat gateway resides. It is then used to determe routes for private subnets in the same AZ as the public subnet.
 	// +optional
-	NatGatewayID *string `json:"natGatewayId"`
+	NatGatewayID *string `json:"natGatewayId,omitempty"`
 
 	// Tags is a collection of tags describing the resource.
 	Tags tags.Map `json:"tags,omitempty"`

pkg/cloud/aws/services/ec2/routetables.go

@@ -155,11 +155,16 @@ func (s *Service) deleteRouteTables() error {
 }
 
 func (s *Service) describeVpcRouteTables() ([]*ec2.RouteTable, error) {
+	filters := []*ec2.Filter{
+		filter.EC2.VPC(s.scope.VPC().ID),
+	}
+
+	if !s.scope.VPC().IsProvided() {
+		filters = append(filters, filter.EC2.Cluster(s.scope.Name()))
+	}
+
 	out, err := s.scope.EC2.DescribeRouteTables(&ec2.DescribeRouteTablesInput{
-		Filters: []*ec2.Filter{
-			filter.EC2.VPC(s.scope.VPC().ID),
-			filter.EC2.Cluster(s.scope.Name()),
-		},
+		Filters: filters,
 	})
 
 	if err != nil {

pkg/cloud/aws/services/ec2/subnets.go

@@ -175,15 +175,48 @@ func (s *Service) describeVpcSubnets() (v1alpha1.Subnets, error) {
 		return nil, errors.Wrapf(err, "failed to describe subnets in vpc %q", s.scope.VPC().ID)
 	}
 
+	routeTables, err := s.describeVpcRouteTablesBySubnet()
+	if err != nil {
+		return nil, err
+	}
+
+	natGateways, err := s.describeNatGatewaysBySubnet()
+	if err != nil {
+		return nil, err
+	}
+
 	subnets := make([]*v1alpha1.SubnetSpec, 0, len(out.Subnets))
+	// Besides what the AWS API tells us directly about the subnets, we also want to discover whether the subnet is "public" (i.e. directly connected to the internet) and if there are any associated NAT gateways.
+	// We also look for a tag indicating that a particular subnet should be public, to try and determine whether a managed VPC's subnet should have such a route, but does not.
 	for _, ec2sn := range out.Subnets {
-		subnets = append(subnets, &v1alpha1.SubnetSpec{
+		spec := &v1alpha1.SubnetSpec{
 			ID:               *ec2sn.SubnetId,
 			CidrBlock:        *ec2sn.CidrBlock,
 			AvailabilityZone: *ec2sn.AvailabilityZone,
-			IsPublic:         *ec2sn.MapPublicIpOnLaunch,
 			Tags:             converters.TagsToMap(ec2sn.Tags),
-		})
+		}
+
+		// A subnet is public if it's tagged as such...
+		if spec.Tags.GetRole() == tags.ValuePublicRole {
+			spec.IsPublic = true
+		}
+
+		// ... or if it has an internet route
+		rt := routeTables[*ec2sn.SubnetId]
+		if rt != nil {
+			spec.RouteTableID = rt.RouteTableId
+			for _, route := range rt.Routes {
+				if route.GatewayId != nil && strings.HasPrefix(*route.GatewayId, "igw") {
+					spec.IsPublic = true
+				}
+			}
+		}
+
+		ngw := natGateways[*ec2sn.SubnetId]
+		if ngw != nil {
+			spec.NatGatewayID = ngw.NatGatewayId
+		}
+		subnets = append(subnets, spec)
 	}
 
 	return subnets, nil
@@ -255,21 +288,23 @@ func (s *Service) deleteSubnet(id string) error {
 }
 
 func (s *Service) getSubnetTagParams(id string, public bool) tags.BuildParams {
-	var name strings.Builder
-
-	name.WriteString(s.scope.Name())
-	name.WriteString("-subnet-")
+	var role string
 	if public {
-		name.WriteString("public")
+		role = tags.ValuePublicRole
 	} else {
-		name.WriteString("private")
+		role = tags.ValuePrivateRole
 	}
 
+	var name strings.Builder
+	name.WriteString(s.scope.Name())
+	name.WriteString("-subnet-")
+	name.WriteString(role)
+
 	return tags.BuildParams{
 		ClusterName: s.scope.Name(),
 		ResourceID:  id,
 		Lifecycle:   tags.ResourceLifecycleOwned,
 		Name:        aws.String(name.String()),
-		Role:        aws.String(tags.ValueCommonRole),
+		Role:        aws.String(role),
 	}
 }