From 0fff22f01de106d63e8da1008699cd7331be64f7 Mon Sep 17 00:00:00 2001 From: Cameron McAvoy Date: Wed, 4 Oct 2023 16:38:20 -0500 Subject: [PATCH] Add AWSMachine fields to control vpc placement for the instance --- api/v1beta1/awsmachine_conversion.go | 4 + api/v1beta1/zz_generated.conversion.go | 2 + api/v1beta2/awsmachine_types.go | 10 + api/v1beta2/zz_generated.deepcopy.go | 12 + ...tructure.cluster.x-k8s.io_awsmachines.yaml | 37 +++ ....cluster.x-k8s.io_awsmachinetemplates.yaml | 40 ++++ pkg/cloud/services/ec2/instances.go | 47 +++- pkg/cloud/services/ec2/instances_test.go | 215 ++++++++++++++++++ 8 files changed, 362 insertions(+), 5 deletions(-) diff --git a/api/v1beta1/awsmachine_conversion.go b/api/v1beta1/awsmachine_conversion.go index 92a22d5a59..1f09d43515 100644 --- a/api/v1beta1/awsmachine_conversion.go +++ b/api/v1beta1/awsmachine_conversion.go @@ -39,6 +39,8 @@ func (src *AWSMachine) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.InstanceMetadataOptions = restored.Spec.InstanceMetadataOptions dst.Spec.PlacementGroupName = restored.Spec.PlacementGroupName dst.Spec.PrivateDNSName = restored.Spec.PrivateDNSName + dst.Spec.VPC = restored.Spec.VPC + dst.Spec.SecurityGroupOverrides = restored.Spec.SecurityGroupOverrides return nil } @@ -87,6 +89,8 @@ func (r *AWSMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Template.Spec.InstanceMetadataOptions = restored.Spec.Template.Spec.InstanceMetadataOptions dst.Spec.Template.Spec.PlacementGroupName = restored.Spec.Template.Spec.PlacementGroupName dst.Spec.Template.Spec.PrivateDNSName = restored.Spec.Template.Spec.PrivateDNSName + dst.Spec.Template.Spec.VPC = restored.Spec.Template.Spec.VPC + dst.Spec.Template.Spec.SecurityGroupOverrides = restored.Spec.Template.Spec.SecurityGroupOverrides return nil } diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 030941fb4f..af11dc0332 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -1390,6 +1390,7 @@ func autoConvert_v1beta2_AWSMachineSpec_To_v1beta1_AWSMachineSpec(in *v1beta2.AW } else { out.AdditionalSecurityGroups = nil } + // WARNING: in.VPC requires manual conversion: does not exist in peer-type if in.Subnet != nil { in, out := &in.Subnet, &out.Subnet *out = new(AWSResourceReference) @@ -1399,6 +1400,7 @@ func autoConvert_v1beta2_AWSMachineSpec_To_v1beta1_AWSMachineSpec(in *v1beta2.AW } else { out.Subnet = nil } + // WARNING: in.SecurityGroupOverrides requires manual conversion: does not exist in peer-type out.SSHKeyName = (*string)(unsafe.Pointer(in.SSHKeyName)) out.RootVolume = (*Volume)(unsafe.Pointer(in.RootVolume)) out.NonRootVolumes = *(*[]Volume)(unsafe.Pointer(&in.NonRootVolumes)) diff --git a/api/v1beta2/awsmachine_types.go b/api/v1beta2/awsmachine_types.go index 1929c79e4d..332f561c5f 100644 --- a/api/v1beta2/awsmachine_types.go +++ b/api/v1beta2/awsmachine_types.go @@ -109,11 +109,21 @@ type AWSMachineSpec struct { // +optional AdditionalSecurityGroups []AWSResourceReference `json:"additionalSecurityGroups,omitempty"` + // VPC is a reference to the VPC to use when picking a subnet to use for this + // instance. Only valid if the subnet (id or filters) is also specified. + // +optional + VPC *AWSResourceReference `json:"vpc,omitempty"` + // Subnet is a reference to the subnet to use for this instance. If not specified, // the cluster subnet will be used. // +optional Subnet *AWSResourceReference `json:"subnet,omitempty"` + // SecurityGroupOverrides is an optional set of security groups to use for the node. + // This is optional - if not provided security groups from the cluster will be used. + // +optional + SecurityGroupOverrides map[SecurityGroupRole]string `json:"securityGroupOverrides,omitempty"` + // SSHKeyName is the name of the ssh key to attach to the instance. Valid values are empty string (do not use SSH keys), a valid SSH key name, or omitted (use the default SSH key name) // +optional SSHKeyName *string `json:"sshKeyName,omitempty"` diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 60ce90eeb7..49b6658bbe 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -695,11 +695,23 @@ func (in *AWSMachineSpec) DeepCopyInto(out *AWSMachineSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.VPC != nil { + in, out := &in.VPC, &out.VPC + *out = new(AWSResourceReference) + (*in).DeepCopyInto(*out) + } if in.Subnet != nil { in, out := &in.Subnet, &out.Subnet *out = new(AWSResourceReference) (*in).DeepCopyInto(*out) } + if in.SecurityGroupOverrides != nil { + in, out := &in.SecurityGroupOverrides, &out.SecurityGroupOverrides + *out = make(map[SecurityGroupRole]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.SSHKeyName != nil { in, out := &in.SSHKeyName, &out.SSHKeyName *out = new(string) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml index fca3f66245..e23cf8547c 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachines.yaml @@ -848,6 +848,13 @@ spec: required: - size type: object + securityGroupOverrides: + additionalProperties: + type: string + description: SecurityGroupOverrides is an optional set of security + groups to use for the node. This is optional - if not provided security + groups from the cluster will be used. + type: object spotMarketOptions: description: SpotMarketOptions allows users to configure instances to be run using AWS Spot instances. @@ -905,6 +912,36 @@ spec: built-in support for gzip-compressed user data user data stored in aws secret manager is always gzip-compressed. type: boolean + vpc: + description: VPC is a reference to the VPC to use when picking a subnet + to use for this instance. Only valid if the subnet (id or filters) + is also specified. + properties: + filters: + description: 'Filters is a set of key/value pairs used to identify + a resource They are applied according to the rules defined by + the AWS API: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Filtering.html' + items: + description: Filter is a filter used to identify an AWS resource. + properties: + name: + description: Name of the filter. Filter names are case-sensitive. + type: string + values: + description: Values includes one or more filter values. + Filter values are case-sensitive. + items: + type: string + type: array + required: + - name + - values + type: object + type: array + id: + description: ID of resource + type: string + type: object required: - instanceType type: object diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml index ebdee36d0b..8c75ad94ce 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml @@ -807,6 +807,14 @@ spec: required: - size type: object + securityGroupOverrides: + additionalProperties: + type: string + description: SecurityGroupOverrides is an optional set of + security groups to use for the node. This is optional - + if not provided security groups from the cluster will be + used. + type: object spotMarketOptions: description: SpotMarketOptions allows users to configure instances to be run using AWS Spot instances. @@ -868,6 +876,38 @@ spec: cloud-init has built-in support for gzip-compressed user data user data stored in aws secret manager is always gzip-compressed. type: boolean + vpc: + description: VPC is a reference to the VPC to use when picking + a subnet to use for this instance. Only valid if the subnet + (id or filters) is also specified. + properties: + filters: + description: 'Filters is a set of key/value pairs used + to identify a resource They are applied according to + the rules defined by the AWS API: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Filtering.html' + items: + description: Filter is a filter used to identify an + AWS resource. + properties: + name: + description: Name of the filter. Filter names are + case-sensitive. + type: string + values: + description: Values includes one or more filter + values. Filter values are case-sensitive. + items: + type: string + type: array + required: + - name + - values + type: object + type: array + id: + description: ID of resource + type: string + type: object required: - instanceType type: object diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go index 9392736f99..c514702bfb 100644 --- a/pkg/cloud/services/ec2/instances.go +++ b/pkg/cloud/services/ec2/instances.go @@ -43,9 +43,13 @@ import ( func (s *Service) GetRunningInstanceByTags(scope *scope.MachineScope) (*infrav1.Instance, error) { s.scope.Debug("Looking for existing machine instance by tags") + vpcID, err := s.getVPCID(scope) + if err != nil { + return nil, err + } input := &ec2.DescribeInstancesInput{ Filters: []*ec2.Filter{ - filter.EC2.VPC(s.scope.VPC().ID), + filter.EC2.VPC(vpcID), filter.EC2.ClusterOwned(s.scope.Name()), filter.EC2.Name(scope.Name()), filter.EC2.InstanceStates(ec2.InstanceStateNamePending, ec2.InstanceStateNameRunning), @@ -309,7 +313,11 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) { filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable), } if !scope.IsExternallyManaged() { - criteria = append(criteria, filter.EC2.VPC(s.scope.VPC().ID)) + vpcID, err := s.getVPCID(scope) + if err != nil { + return "", fmt.Errorf("failed to lookup vpc for subnets: %w", err) + } + criteria = append(criteria, filter.EC2.VPC(vpcID)) } 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})}) @@ -395,6 +403,30 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) { } } +func (s *Service) getVPCID(scope *scope.MachineScope) (string, error) { + if scope.AWSMachine.Spec.VPC == nil { + return s.scope.VPC().ID, nil + } + if scope.AWSMachine.Spec.VPC.ID != nil { + return *scope.AWSMachine.Spec.VPC.ID, nil + } + filters := make([]*ec2.Filter, 0, len(scope.AWSMachine.Spec.VPC.Filters)) + for _, filter := range scope.AWSMachine.Spec.VPC.Filters { + filters = append(filters, &ec2.Filter{ + Name: aws.String(filter.Name), + Values: aws.StringSlice(filter.Values), + }) + } + out, err := s.EC2Client.DescribeVpcsWithContext(context.TODO(), &ec2.DescribeVpcsInput{Filters: filters}) + if err != nil { + return "", err + } + if out != nil && len(out.Vpcs) == 1 { + return *out.Vpcs[0].VpcId, nil + } + return "", fmt.Errorf("ambigious VPC filters, only one VPC expected, got %v", out) +} + // getFilteredSubnets fetches subnets filtered based on the criteria passed. func (s *Service) getFilteredSubnets(criteria ...*ec2.Filter) ([]*ec2.Subnet, error) { out, err := s.EC2Client.DescribeSubnetsWithContext(context.TODO(), &ec2.DescribeSubnetsInput{Filters: criteria}) @@ -440,10 +472,15 @@ func (s *Service) GetCoreSecurityGroups(scope *scope.MachineScope) ([]string, er } ids := make([]string, 0, len(sgRoles)) for _, sg := range sgRoles { - if _, ok := s.scope.SecurityGroups()[sg]; !ok { - return nil, awserrors.NewFailedDependency(fmt.Sprintf("%s security group not available", sg)) + if _, ok := scope.AWSMachine.Spec.SecurityGroupOverrides[sg]; ok { + ids = append(ids, scope.AWSMachine.Spec.SecurityGroupOverrides[sg]) + continue } - ids = append(ids, s.scope.SecurityGroups()[sg].ID) + if _, ok := s.scope.SecurityGroups()[sg]; ok { + ids = append(ids, s.scope.SecurityGroups()[sg].ID) + continue + } + return nil, awserrors.NewFailedDependency(fmt.Sprintf("%s security group not available", sg)) } return ids, nil } diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go index a90d5de5be..c294cf17ac 100644 --- a/pkg/cloud/services/ec2/instances_test.go +++ b/pkg/cloud/services/ec2/instances_test.go @@ -548,6 +548,221 @@ func TestCreateInstance(t *testing.T) { } }, }, + { + name: "with vpc and sg roles", + machine: clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"set": "node"}, + }, + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ + DataSecretName: pointer.String("bootstrap-data"), + }, + FailureDomain: aws.String("us-east-1c"), + }, + }, + machineConfig: &infrav1.AWSMachineSpec{ + AMI: infrav1.AMIReference{ + ID: aws.String("abc"), + }, + InstanceType: "m5.2xlarge", + VPC: &infrav1.AWSResourceReference{ + ID: aws.String("vpc-bar"), + }, + Subnet: &infrav1.AWSResourceReference{ + Filters: []infrav1.Filter{ + { + Name: "availability-zone", + Values: []string{"us-east-1c"}, + }, + }, + }, + SecurityGroupOverrides: map[infrav1.SecurityGroupRole]string{ + infrav1.SecurityGroupNode: "4", + }, + UncompressedUserData: &isUncompressedFalse, + }, + awsCluster: &infrav1.AWSCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: infrav1.AWSClusterSpec{ + NetworkSpec: infrav1.NetworkSpec{ + VPC: infrav1.VPCSpec{ + ID: "vpc-foo", + }, + Subnets: infrav1.Subnets{ + infrav1.SubnetSpec{ + ID: "subnet-1", + AvailabilityZone: "us-east-1a", + IsPublic: false, + }, + infrav1.SubnetSpec{ + ID: "subnet-2", + AvailabilityZone: "us-east-1b", + IsPublic: false, + }, + infrav1.SubnetSpec{ + ID: "subnet-3", + AvailabilityZone: "us-east-1c", + IsPublic: false, + }, + infrav1.SubnetSpec{ + ID: "subnet-3-public", + AvailabilityZone: "us-east-1c", + IsPublic: true, + }, + }, + }, + }, + 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.LoadBalancer{ + DNSName: "test-apiserver.us-east-1.aws", + }, + }, + }, + }, + expect: func(m *mocks.MockEC2APIMockRecorder) { + m. + DescribeInstanceTypesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeInstanceTypesInput{ + InstanceTypes: []*string{ + aws.String("m5.2xlarge"), + }, + })). + Return(&ec2.DescribeInstanceTypesOutput{ + InstanceTypes: []*ec2.InstanceTypeInfo{ + { + ProcessorInfo: &ec2.ProcessorInfo{ + SupportedArchitectures: []*string{ + aws.String("x86_64"), + }, + }, + }, + }, + }, nil) + m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("state"), + Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}), + }, + { + Name: aws.String("vpc-id"), + Values: aws.StringSlice([]string{"vpc-bar"}), + }, + { + Name: aws.String("availability-zone"), + Values: aws.StringSlice([]string{"us-east-1c"}), + }, + }})).Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{ + { + VpcId: aws.String("vpc-bar"), + SubnetId: aws.String("subnet-4"), + AvailabilityZone: aws.String("us-east-1a"), + CidrBlock: aws.String("10.0.10.0/24"), + MapPublicIpOnLaunch: aws.Bool(false), + }, + { + VpcId: aws.String("vpc-bar"), + SubnetId: aws.String("subnet-5"), + AvailabilityZone: aws.String("us-east-1c"), + CidrBlock: aws.String("10.0.11.0/24"), + }, + }, + }, nil) + m. + RunInstancesWithContext(context.TODO(), &ec2.RunInstancesInput{ + ImageId: aws.String("abc"), + InstanceType: aws.String("m5.2xlarge"), + KeyName: aws.String("default"), + SecurityGroupIds: aws.StringSlice([]string{"4", "3"}), + SubnetId: aws.String("subnet-5"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("instance"), + Tags: []*ec2.Tag{ + { + Key: aws.String("MachineName"), + Value: aws.String("/"), + }, + { + Key: aws.String("Name"), + Value: aws.String("aws-test1"), + }, + { + Key: aws.String("kubernetes.io/cluster/test1"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test1"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("node"), + }, + }, + }, + }, + UserData: aws.String(base64.StdEncoding.EncodeToString(userDataCompressed)), + MaxCount: aws.Int64(1), + MinCount: aws.Int64(1), + }).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("subnet-5"), + 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. + DescribeNetworkInterfacesWithContext(context.TODO(), gomock.Any()). + Return(&ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{}, + NextToken: nil, + }, nil) + }, + check: func(instance *infrav1.Instance, err error) { + if err != nil { + t.Fatalf("did not expect error: %v", err) + } + + if instance.SubnetID != "subnet-5" { + t.Fatalf("expected subnet-5 from availability zone us-east-1c, got %q", instance.SubnetID) + } + }, + }, { name: "with ImageLookupOrg specified at the machine level", machine: &clusterv1.Machine{