diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go index 1addf55730..bf7e5ce212 100644 --- a/pkg/cloud/services/ec2/instances.go +++ b/pkg/cloud/services/ec2/instances.go @@ -265,57 +265,59 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) { failureDomain = scope.AWSMachine.Spec.FailureDomain } + // We basically have 2 sources for subnets: + // 1. If subnet.id or subnet.filters are specified, we directly query AWS + // 2. All other cases use the subnets provided in the cluster network spec without ever calling AWS + switch { - case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.ID != nil: - subnet := s.scope.Subnets().FindByID(*scope.AWSMachine.Spec.Subnet.ID) - if subnet == nil { - errMessage := fmt.Sprintf("failed to run machine %q, subnet with id %q not found", - scope.Name(), aws.StringValue(scope.AWSMachine.Spec.Subnet.ID)) - record.Warnf(scope.AWSMachine, "FailedCreate", errMessage) - return "", awserrors.NewFailedDependency(errMessage) - } - if scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP { - if !subnet.IsPublic { - errMessage := fmt.Sprintf("failed to run machine %q with public IP, a specified subnet %q is a private subnet", - scope.Name(), aws.StringValue(scope.AWSMachine.Spec.Subnet.ID)) - record.Eventf(scope.AWSMachine, "FailedCreate", errMessage) - return "", awserrors.NewFailedDependency(errMessage) - } - } - if failureDomain != nil && subnet.AvailabilityZone != *failureDomain { - errMessage := fmt.Sprintf("failed to run machine %q, subnet's availability zone %q does not match with the failure domain %q", - scope.Name(), subnet.AvailabilityZone, *failureDomain) - record.Warnf(scope.AWSMachine, "FailedCreate", errMessage) - return "", awserrors.NewFailedDependency(errMessage) - } - return subnet.ID, nil - case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.Filters != nil: + case scope.AWSMachine.Spec.Subnet != nil && (scope.AWSMachine.Spec.Subnet.ID != nil || scope.AWSMachine.Spec.Subnet.Filters != nil): criteria := []*ec2.Filter{ filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), } if !scope.IsExternallyManaged() { criteria = append(criteria, filter.EC2.VPC(s.scope.VPC().ID)) } - if failureDomain != nil { - criteria = append(criteria, filter.EC2.AvailabilityZone(*failureDomain)) - } - if scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP { - criteria = append(criteria, &ec2.Filter{Name: aws.String("map-public-ip-on-launch"), Values: aws.StringSlice([]string{"true"})}) + if scope.AWSMachine.Spec.Subnet.ID != nil { + criteria = append(criteria, &ec2.Filter{Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{*scope.AWSMachine.Spec.Subnet.ID})}) } for _, f := range scope.AWSMachine.Spec.Subnet.Filters { criteria = append(criteria, &ec2.Filter{Name: aws.String(f.Name), Values: aws.StringSlice(f.Values)}) } + subnets, err := s.getFilteredSubnets(criteria...) if err != nil { return "", errors.Wrapf(err, "failed to filter subnets for criteria %q", criteria) } if len(subnets) == 0 { - errMessage := fmt.Sprintf("failed to run machine %q, no subnets available matching filters %q", - scope.Name(), scope.AWSMachine.Spec.Subnet.Filters) + errMessage := fmt.Sprintf("failed to run machine %q, no subnets available matching criteria %q", + scope.Name(), criteria) + record.Warnf(scope.AWSMachine, "FailedCreate", errMessage) + return "", awserrors.NewFailedDependency(errMessage) + } + + var filtered []*ec2.Subnet + var errMessage string + for _, subnet := range subnets { + if failureDomain != nil && *subnet.AvailabilityZone != *failureDomain { + // we could have included the failure domain in the query criteria, but then we end up with EC2 error + // messages that don't give a good hint about what is really wrong + errMessage += fmt.Sprintf(" subnet %q availability zone %q does not match failure domain %q.", + *subnet.SubnetId, *subnet.AvailabilityZone, *failureDomain) + continue + } + if scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP && !*subnet.MapPublicIpOnLaunch { + errMessage += fmt.Sprintf(" subnet %q is a private subnet.", *subnet.SubnetId) + continue + } + filtered = append(filtered, subnet) + } + if len(filtered) == 0 { + errMessage = fmt.Sprintf("failed to run machine %q, found %d subnets matching criteria but post-filtering failed.", + scope.Name(), len(subnets)) + errMessage record.Warnf(scope.AWSMachine, "FailedCreate", errMessage) return "", awserrors.NewFailedDependency(errMessage) } - return *subnets[0].SubnetId, nil + return *filtered[0].SubnetId, nil case failureDomain != nil: if scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP { subnets := s.scope.Subnets().FilterPublic().FilterByZone(*failureDomain) diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go index 59e3245bd8..91be58fe7f 100644 --- a/pkg/cloud/services/ec2/instances_test.go +++ b/pkg/cloud/services/ec2/instances_test.go @@ -952,13 +952,13 @@ func TestCreateInstance(t *testing.T) { Filters: []*ec2.Filter{ filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), filter.EC2.VPC("vpc-id"), - filter.EC2.AvailabilityZone("us-east-1b"), {Name: aws.String("tag:some-tag"), Values: aws.StringSlice([]string{"some-value"})}, }, }). Return(&ec2.DescribeSubnetsOutput{ Subnets: []*ec2.Subnet{{ - SubnetId: aws.String("filtered-subnet-1"), + SubnetId: aws.String("filtered-subnet-1"), + AvailabilityZone: aws.String("us-east-1b"), }}, }, nil) m. @@ -1053,6 +1053,20 @@ func TestCreateInstance(t *testing.T) { }, }, expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) { + m. + DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), + filter.EC2.VPC("vpc-id"), + {Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{"matching-subnet"})}, + }, + }). + Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{{ + SubnetId: aws.String("matching-subnet"), + AvailabilityZone: aws.String("us-east-1b"), + }}, + }, nil) m. RunInstances(gomock.Any()). Return(&ec2.Reservation{ @@ -1093,7 +1107,7 @@ func TestCreateInstance(t *testing.T) { }, }, { - name: "with subnet ID that does not belong to Cluster", + name: "with subnet ID that does not exist", machine: clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{"set": "node"}, @@ -1145,9 +1159,20 @@ func TestCreateInstance(t *testing.T) { }, }, expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) { + m. + DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), + filter.EC2.VPC("vpc-id"), + {Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{"non-matching-subnet"})}, + }, + }). + Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{}, + }, nil) }, check: func(instance *infrav1.Instance, err error) { - expectedErrMsg := "failed to run machine \"aws-test1\", subnet with id \"non-matching-subnet\" not found" + expectedErrMsg := "failed to run machine \"aws-test1\", no subnets available matching criteria" if err == nil { t.Fatalf("Expected error, but got nil") } @@ -1157,6 +1182,111 @@ func TestCreateInstance(t *testing.T) { } }, }, + { + name: "with subnet ID that does not belong to Cluster", + machine: clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"set": "node"}, + }, + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ + DataSecretName: pointer.StringPtr("bootstrap-data"), + }, + }, + }, + machineConfig: &infrav1.AWSMachineSpec{ + AMI: infrav1.AMIReference{ + ID: aws.String("abc"), + }, + InstanceType: "m5.large", + Subnet: &infrav1.AWSResourceReference{ + ID: aws.String("matching-subnet"), + }, + }, + awsCluster: &infrav1.AWSCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: infrav1.AWSClusterSpec{ + NetworkSpec: infrav1.NetworkSpec{ + VPC: infrav1.VPCSpec{ + ID: "vpc-id", + }, + Subnets: infrav1.Subnets{{ + ID: "subnet-1", + }}, + }, + }, + Status: infrav1.AWSClusterStatus{ + Network: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "1", + }, + infrav1.SecurityGroupNode: { + ID: "2", + }, + infrav1.SecurityGroupLB: { + ID: "3", + }, + }, + APIServerELB: infrav1.ClassicELB{ + DNSName: "test-apiserver.us-east-1.aws", + }, + }, + }, + }, + expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) { + m. + DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), + filter.EC2.VPC("vpc-id"), + {Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{"matching-subnet"})}, + }, + }). + Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{{ + SubnetId: aws.String("matching-subnet"), + }}, + }, nil) + m. + RunInstances(gomock.Any()). + Return(&ec2.Reservation{ + Instances: []*ec2.Instance{ + { + State: &ec2.InstanceState{ + Name: aws.String(ec2.InstanceStateNamePending), + }, + IamInstanceProfile: &ec2.IamInstanceProfile{ + Arn: aws.String("arn:aws:iam::123456789012:instance-profile/foo"), + }, + InstanceId: aws.String("two"), + InstanceType: aws.String("m5.large"), + SubnetId: aws.String("matching-subnet"), + ImageId: aws.String("ami-1"), + RootDeviceName: aws.String("device-1"), + BlockDeviceMappings: []*ec2.InstanceBlockDeviceMapping{ + { + DeviceName: aws.String("device-1"), + Ebs: &ec2.EbsInstanceBlockDevice{ + VolumeId: aws.String("volume-1"), + }, + }, + }, + Placement: &ec2.Placement{ + AvailabilityZone: &az, + }, + }, + }, + }, nil) + m.WaitUntilInstanceRunningWithContext(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil) + }, + check: func(instance *infrav1.Instance, err error) { + if err != nil { + t.Fatalf("did not expect error: %v", err) + } + }, + }, { name: "subnet id and failureDomain don't match", machine: clusterv1.Machine{ @@ -1212,9 +1342,23 @@ func TestCreateInstance(t *testing.T) { }, }, expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) { + m. + DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), + filter.EC2.VPC("vpc-id"), + {Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{"subnet-1"})}, + }, + }). + Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{{ + SubnetId: aws.String("subnet-1"), + AvailabilityZone: aws.String("us-west-1b"), + }}, + }, nil) }, check: func(instance *infrav1.Instance, err error) { - expectedErrMsg := "subnet's availability zone \"us-west-1b\" does not match with the failure domain \"us-east-1b\"" + expectedErrMsg := "failed to run machine \"aws-test1\", found 1 subnets matching criteria but post-filtering failed. subnet \"subnet-1\" availability zone \"us-west-1b\" does not match failure domain \"us-east-1b\"" if err == nil { t.Fatalf("Expected error, but got nil") } @@ -1345,6 +1489,21 @@ func TestCreateInstance(t *testing.T) { }, }, expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) { + m. + DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), + filter.EC2.VPC("vpc-id"), + {Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{"public-subnet-1"})}, + }, + }). + Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{{ + SubnetId: aws.String("public-subnet-1"), + AvailabilityZone: aws.String("us-east-1b"), + MapPublicIpOnLaunch: aws.Bool(true), + }}, + }, nil) m. RunInstances(gomock.Any()). Return(&ec2.Reservation{ @@ -1439,9 +1598,24 @@ func TestCreateInstance(t *testing.T) { }, }, expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) { + m. + DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), + filter.EC2.VPC("vpc-id"), + {Name: aws.String("subnet-id"), Values: aws.StringSlice([]string{"private-subnet-1"})}, + }, + }). + Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{{ + SubnetId: aws.String("private-subnet-1"), + AvailabilityZone: aws.String("us-east-1b"), + MapPublicIpOnLaunch: aws.Bool(false), + }}, + }, nil) }, check: func(instance *infrav1.Instance, err error) { - expectedErrMsg := "failed to run machine \"aws-test1\" with public IP, a specified subnet \"private-subnet-1\" is a private subnet" + expectedErrMsg := "failed to run machine \"aws-test1\", found 1 subnets matching criteria but post-filtering failed. subnet \"private-subnet-1\" is a private subnet." if err == nil { t.Fatalf("Expected error, but got nil") } @@ -1520,13 +1694,13 @@ func TestCreateInstance(t *testing.T) { Filters: []*ec2.Filter{ filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), filter.EC2.VPC("vpc-id"), - {Name: aws.String("map-public-ip-on-launch"), Values: aws.StringSlice([]string{"true"})}, {Name: aws.String("tag:some-tag"), Values: aws.StringSlice([]string{"some-value"})}, }, }). Return(&ec2.DescribeSubnetsOutput{ Subnets: []*ec2.Subnet{{ - SubnetId: aws.String("filtered-subnet-1"), + SubnetId: aws.String("filtered-subnet-1"), + MapPublicIpOnLaunch: aws.Bool(true), }}, }, nil) m.