Unify tagging and edge cases (openshift#156)

- Add cluster-api-provider-aws specific tag
- Add name tags to resources we create
- handle terminated instances for bastion
- attempt to avoid orphaning eips when NAT gateway creation fails

cloud/aws/services/ec2/bastion.go

@@ -25,9 +25,6 @@ import (
 )
 
 const (
-	// TagValueBastionRole describes the value associated with the role tag.
-	TagValueBastionRole = "bastion"
-
 	bastionUserData = `#!/bin/bash
 
 BASTION_BOOTSTRAP_FILE=bastion_bootstrap.sh
@@ -103,14 +100,23 @@ func (s *Service) describeBastionInstance(clusterName string, status *v1alpha1.A
 		return nil, errors.Wrap(err, "failed to describe bastion host")
 	}
 
-	if len(out.Reservations) == 0 || len(out.Reservations[0].Instances) == 0 {
-		return nil, NewNotFound(errors.New("bastion host not found"))
+	// TODO: properly handle multiple bastions found rather than just returning
+	// the first non-terminated.
+	for _, res := range out.Reservations {
+		for _, instance := range res.Instances {
+			if aws.StringValue(instance.State.Name) != ec2.InstanceStateNameTerminated {
+				return fromSDKTypeToInstance(instance), nil
+			}
+		}
 	}
 
-	return fromSDKTypeToInstance(out.Reservations[0].Instances[0]), nil
+	return nil, NewNotFound(errors.New("bastion host not found"))
 }
 
 func (s *Service) getDefaultBastion(clusterName string, region string, network v1alpha1.Network, keyName string) *v1alpha1.Instance {
+	name := fmt.Sprintf("%s-bastion", clusterName)
+	tags := s.buildTags(clusterName, ResourceLifecycleOwned, name, TagValueBastionRole, nil)
+
 	i := &v1alpha1.Instance{
 		Type:     "t2.micro",
 		SubnetID: network.Subnets.FilterPublic()[0].ID,
@@ -120,10 +126,7 @@ func (s *Service) getDefaultBastion(clusterName string, region string, network v
 		SecurityGroupIDs: []string{
 			network.SecurityGroups[v1alpha1.SecurityGroupBastion].ID,
 		},
-		Tags: map[string]string{
-			s.clusterTagKey(clusterName): string(ResourceLifecycleOwned),
-			TagNameAWSClusterAPIRole:     TagValueBastionRole,
-		},
+		Tags: tags,
 	}
 
 	return i

cloud/aws/services/ec2/eips.go

@@ -14,27 +14,55 @@
 package ec2
 
 import (
+	"fmt"
+
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
 	"sigs.k8s.io/cluster-api-provider-aws/cloud/aws/services/wait"
 )
 
-func (s *Service) allocateAddress(clusterName string) (string, error) {
+func (s *Service) getOrAllocateAddress(clusterName string, role string) (string, error) {
+	out, err := s.describeAddresses(clusterName, role)
+	if err != nil {
+		return "", errors.Wrap(err, "failed to query addresses")
+	}
+
+	// TODO: better handle multiple addresses returned
+	for _, address := range out.Addresses {
+		if address.AssociationId == nil {
+			return aws.StringValue(address.AllocationId), nil
+		}
+	}
+	return s.allocateAddress(clusterName, role)
+}
+
+func (s *Service) allocateAddress(clusterName string, role string) (string, error) {
 	out, err := s.EC2.AllocateAddress(&ec2.AllocateAddressInput{
 		Domain: aws.String("vpc"),
 	})
 
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to create Elastic IP address")
+		return "", errors.Wrap(err, "failed to create Elastic IP address")
 	}
 
-	if err := s.createTags(clusterName, *out.AllocationId, ResourceLifecycleOwned, nil); err != nil {
-		return "", errors.Wrapf(err, "failed to tag elastic IP %q", *out.AllocationId)
+	name := fmt.Sprintf("%s-eip-%s", clusterName, role)
+	if err := s.createTags(clusterName, *out.AllocationId, ResourceLifecycleOwned, name, role, nil); err != nil {
+		return "", errors.Wrapf(err, "failed to tag elastic IP %q", aws.StringValue(out.AllocationId))
 	}
 
-	return *out.AllocationId, nil
+	return aws.StringValue(out.AllocationId), nil
+}
+
+func (s *Service) describeAddresses(clusterName string, role string) (*ec2.DescribeAddressesOutput, error) {
+	filters := []*ec2.Filter{s.filterCluster(clusterName)}
+	if role != "" {
+		filters = append(filters, s.filterAWSProviderRole(role))
+	}
+	return s.EC2.DescribeAddresses(&ec2.DescribeAddressesInput{
+		Filters: filters,
+	})
 }
 
 func (s *Service) releaseAddresses(clusterName string) error {

cloud/aws/services/ec2/errors.go

@@ -58,7 +58,10 @@ func NewConflict(err error) error {
 
 // IsNotFound returns true if the error was created by NewNotFound.
 func IsNotFound(err error) bool {
-	return ReasonForError(err) == http.StatusNotFound
+	if ReasonForError(err) == http.StatusNotFound {
+		return true
+	}
+	return IsInvalidNotFoundError(err)
 }
 
 // IsConflict returns true if the error was created by NewConflict.
@@ -72,6 +75,17 @@ func IsSDKError(err error) (ok bool) {
 	return
 }
 
+// IsInvalidNotFoundError tests for common aws not found errors
+func IsInvalidNotFoundError(err error) bool {
+	if awsErr, ok := err.(awserr.Error); ok {
+		switch code := awsErr.Code(); code {
+		case "InvalidVpcID.NotFound":
+			return true
+		}
+	}
+	return false
+}
+
 // ReasonForError returns the HTTP status for a particular error.
 func ReasonForError(err error) int {
 	switch t := err.(type) {

cloud/aws/services/ec2/filters.go

@@ -1,6 +1,8 @@
 package ec2
 
 import (
+	"fmt"
+
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 )
@@ -20,6 +22,40 @@ func (s *Service) filterCluster(clusterName string) *ec2.Filter {
 	}
 }
 
+// Returns an EC2 filter using the Cluster API per-cluster tag where
+// the resource is owned
+func (s *Service) filterClusterOwned(clusterName string) *ec2.Filter {
+	return &ec2.Filter{
+		Name:   aws.String(fmt.Sprintf("tag:%s", s.clusterTagKey(clusterName))),
+		Values: aws.StringSlice([]string{string(ResourceLifecycleOwned)}),
+	}
+}
+
+// Returns an EC2 filter using the Cluster API per-cluster tag where
+// the resource is shared
+func (s *Service) filterClusterShared(clusterName string) *ec2.Filter {
+	return &ec2.Filter{
+		Name:   aws.String(fmt.Sprintf("tag:%s", s.clusterTagKey(clusterName))),
+		Values: aws.StringSlice([]string{string(ResourceLifecycleShared)}),
+	}
+}
+
+// Returns an EC2 filter using cluster-api-provider-aws managed tag
+func (s *Service) filterAWSProviderManaged() *ec2.Filter {
+	return &ec2.Filter{
+		Name:   aws.String(filterNameTagKey),
+		Values: aws.StringSlice([]string{TagNameAWSProviderManaged}),
+	}
+}
+
+// Returns an EC2 filter using cluster-api-provider-aws role tag
+func (s *Service) filterAWSProviderRole(role string) *ec2.Filter {
+	return &ec2.Filter{
+		Name:   aws.String(fmt.Sprintf("tag:%s", TagNameAWSClusterAPIRole)),
+		Values: aws.StringSlice([]string{role}),
+	}
+}
+
 // Returns an EC2 filter for the specified VPC ID
 func (s *Service) filterVpc(vpcID string) *ec2.Filter {
 	return &ec2.Filter{

cloud/aws/services/ec2/gateways.go

@@ -14,6 +14,8 @@
 package ec2
 
 import (
+	"fmt"
+
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
@@ -73,7 +75,8 @@ func (s *Service) createInternetGateway(clusterName string, vpc *v1alpha1.VPC) (
 		return nil, errors.Wrap(err, "failed to create internet gateway")
 	}
 
-	if err := s.createTags(clusterName, *ig.InternetGateway.InternetGatewayId, ResourceLifecycleOwned, nil); err != nil {
+	name := fmt.Sprintf("%s-igw", clusterName)
+	if err := s.createTags(clusterName, *ig.InternetGateway.InternetGatewayId, ResourceLifecycleOwned, name, TagValueCommonRole, nil); err != nil {
 		return nil, errors.Wrapf(err, "failed to tag internet gateway %q", *ig.InternetGateway.InternetGatewayId)
 	}
 

cloud/aws/services/ec2/gateways_test.go

@@ -76,13 +76,7 @@ func TestReconcileInternetGateways(t *testing.T) {
 					}, nil)
 
 				m.EXPECT().
-					CreateTags(gomock.Eq(&ec2.CreateTagsInput{
-						Resources: aws.StringSlice([]string{"igw-1"}),
-						Tags: []*ec2.Tag{&ec2.Tag{
-							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
-							Value: aws.String("owned"),
-						}},
-					})).
+					CreateTags(gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})).
 					Return(nil, nil)
 
 				m.EXPECT().

cloud/aws/services/ec2/natgateways.go

@@ -14,6 +14,8 @@
 package ec2
 
 import (
+	"fmt"
+
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
@@ -114,7 +116,7 @@ func (s *Service) describeNatGatewaysBySubnet(vpcID string) (map[string]*ec2.Nat
 }
 
 func (s *Service) createNatGateway(clusterName string, subnetID string) (*ec2.NatGateway, error) {
-	ip, err := s.allocateAddress(clusterName)
+	ip, err := s.getOrAllocateAddress(clusterName, TagValueAPIServerRole)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to create IP address for NAT gateway for subnet ID %q", subnetID)
 	}
@@ -128,6 +130,11 @@ func (s *Service) createNatGateway(clusterName string, subnetID string) (*ec2.Na
 		return nil, errors.Wrapf(err, "failed to create NAT gateway for subnet ID %q", subnetID)
 	}
 
+	name := fmt.Sprintf("%s-nat", clusterName)
+	if err := s.createTags(clusterName, *out.NatGateway.NatGatewayId, ResourceLifecycleOwned, name, TagValueCommonRole, nil); err != nil {
+		return nil, errors.Wrapf(err, "failed to tag nat gateway %q", *out.NatGateway.NatGatewayId)
+	}
+
 	glog.Infof("Created NAT gateway %q for subnet ID %q, waiting for it to become available...", *out.NatGateway.NatGatewayId, subnetID)
 
 	wReq := &ec2.DescribeNatGatewaysInput{NatGatewayIds: []*string{out.NatGateway.NatGatewayId}}
@@ -137,10 +144,6 @@ func (s *Service) createNatGateway(clusterName string, subnetID string) (*ec2.Na
 
 	glog.Infof("NAT gateway %q for subnet ID %q is now available", *out.NatGateway.NatGatewayId, subnetID)
 
-	if err := s.createTags(clusterName, *out.NatGateway.NatGatewayId, ResourceLifecycleOwned, nil); err != nil {
-		return nil, errors.Wrapf(err, "failed to tag nat gateway %q", *out.NatGateway.NatGatewayId)
-	}
-
 	return out.NatGateway, nil
 }
 

cloud/aws/services/ec2/natgateways_test.go

@@ -108,6 +108,10 @@ func TestReconcileNatGateways(t *testing.T) {
 						}),
 						gomock.Any()).Return(nil)
 
+				m.EXPECT().
+					DescribeAddresses(gomock.Any()).
+					Return(&ec2.DescribeAddressesOutput{}, nil)
+
 				m.EXPECT().
 					AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}).
 					Return(&ec2.AllocateAddressOutput{
@@ -130,25 +134,12 @@ func TestReconcileNatGateways(t *testing.T) {
 					}).Return(nil)
 
 				m.EXPECT().
-					CreateTags(gomock.Eq(&ec2.CreateTagsInput{
-						Resources: aws.StringSlice([]string{ElasticIPAllocationID}),
-						Tags: []*ec2.Tag{&ec2.Tag{
-							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
-							Value: aws.String("owned"),
-						}},
-					})).
+					CreateTags(gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})).
 					Return(nil, nil)
 
 				m.EXPECT().
-					CreateTags(gomock.Eq(&ec2.CreateTagsInput{
-						Resources: aws.StringSlice([]string{"natgateway"}),
-						Tags: []*ec2.Tag{&ec2.Tag{
-							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
-							Value: aws.String("owned"),
-						}},
-					})).
+					CreateTags(gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})).
 					Return(nil, nil)
-
 			},
 		},
 		{
@@ -200,6 +191,10 @@ func TestReconcileNatGateways(t *testing.T) {
 					}}}, true)
 				}).Return(nil)
 
+				m.EXPECT().
+					DescribeAddresses(gomock.Any()).
+					Return(&ec2.DescribeAddressesOutput{}, nil)
+
 				m.EXPECT().
 					AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}).
 					Return(&ec2.AllocateAddressOutput{
@@ -222,23 +217,11 @@ func TestReconcileNatGateways(t *testing.T) {
 					}).Return(nil)
 
 				m.EXPECT().
-					CreateTags(gomock.Eq(&ec2.CreateTagsInput{
-						Resources: aws.StringSlice([]string{ElasticIPAllocationID}),
-						Tags: []*ec2.Tag{&ec2.Tag{
-							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
-							Value: aws.String("owned"),
-						}},
-					})).
+					CreateTags(gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})).
 					Return(nil, nil)
 
 				m.EXPECT().
-					CreateTags(gomock.Eq(&ec2.CreateTagsInput{
-						Resources: aws.StringSlice([]string{"natgateway"}),
-						Tags: []*ec2.Tag{&ec2.Tag{
-							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
-							Value: aws.String("owned"),
-						}},
-					})).
+					CreateTags(gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})).
 					Return(nil, nil)
 			},
 		},
@@ -284,6 +267,8 @@ func TestReconcileNatGateways(t *testing.T) {
 					}}}, true)
 				}).Return(nil)
 
+				m.EXPECT().DescribeAddresses(gomock.Any()).Times(0)
+
 				m.EXPECT().AllocateAddress(gomock.Any()).Times(0)
 
 				m.EXPECT().CreateNatGateway(gomock.Any()).Times(0)

cloud/aws/services/ec2/routetables.go

@@ -14,6 +14,8 @@
 package ec2
 
 import (
+	"fmt"
+
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
@@ -59,7 +61,7 @@ func (s *Service) reconcileRouteTables(clusterName string, in *v1alpha1.Network)
 			routes = s.getDefaultPrivateRoutes(natGatewayID)
 		}
 
-		rt, err := s.createRouteTableWithRoutes(clusterName, &in.VPC, routes)
+		rt, err := s.createRouteTableWithRoutes(clusterName, &in.VPC, routes, sn.IsPublic)
 		if err != nil {
 			return err
 		}
@@ -142,12 +144,20 @@ func (s *Service) describeVpcRouteTables(clusterName string, vpcID string) ([]*e
 	return out.RouteTables, nil
 }
 
-func (s *Service) createRouteTableWithRoutes(clusterName string, vpc *v1alpha1.VPC, routes []*ec2.Route) (*v1alpha1.RouteTable, error) {
+// TODO: dedup some of the public/private logic shared with createSubnet
+func (s *Service) createRouteTableWithRoutes(clusterName string, vpc *v1alpha1.VPC, routes []*ec2.Route, isPublic bool) (*v1alpha1.RouteTable, error) {
 	out, err := s.EC2.CreateRouteTable(&ec2.CreateRouteTableInput{
 		VpcId: aws.String(vpc.ID),
 	})
 
-	if err := s.createTags(clusterName, *out.RouteTable.RouteTableId, ResourceLifecycleOwned, nil); err != nil {
+	suffix := "private"
+	role := TagValueCommonRole
+	if isPublic {
+		suffix = "public"
+		role = TagValueBastionRole
+	}
+	name := fmt.Sprintf("%s-rt-%s", clusterName, suffix)
+	if err := s.createTags(clusterName, *out.RouteTable.RouteTableId, ResourceLifecycleOwned, name, role, nil); err != nil {
 		return nil, errors.Wrapf(err, "failed to tag route table %q", *out.RouteTable.RouteTableId)
 	}