From 7c33286b7492b6000900704106ae1ca1df5f4592 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 21 Apr 2023 13:21:28 -0400 Subject: [PATCH 1/7] builder: consider machine type for picking subnet When a configuration enables associate_public_ip_address, we pick a subnet from the default VPC, should one not be specified in the configs. However, based on the type on machine being created, we may not always pick the most available subnet out of all the possible ones exposed. This commit filters out the subnets based on the AZ they're linked to, and whether or not the AZ supports the instance type requested in the template. --- builder/common/step_network_info.go | 89 ++++++++++++++++++++++++++++- builder/ebs/builder.go | 1 + builder/ebssurrogate/builder.go | 1 + builder/ebsvolume/builder.go | 1 + builder/instance/builder.go | 1 + 5 files changed, 90 insertions(+), 3 deletions(-) diff --git a/builder/common/step_network_info.go b/builder/common/step_network_info.go index f5b1d9a83..a01d4bd59 100644 --- a/builder/common/step_network_info.go +++ b/builder/common/step_network_info.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/ec2/ec2iface" "github.com/hashicorp/packer-plugin-sdk/multistep" packersdk "github.com/hashicorp/packer-plugin-sdk/packer" confighelper "github.com/hashicorp/packer-plugin-sdk/template/config" @@ -31,6 +32,10 @@ type StepNetworkInfo struct { AvailabilityZone string SecurityGroupIds []string SecurityGroupFilter SecurityGroupFilterOptions + // RequestedMachineType is the machine type of the instance we want to create. + // This is used for selecting a subnet/AZ which supports the type of instance + // selected, and not just the most available / random one. + RequestedMachineType string } type subnetsSort []*ec2.Subnet @@ -49,7 +54,7 @@ func mostFreeSubnet(subnets []*ec2.Subnet) *ec2.Subnet { } func (s *StepNetworkInfo) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { - ec2conn := state.Get("ec2").(*ec2.EC2) + ec2conn := state.Get("ec2").(ec2iface.EC2API) ui := state.Get("ui").(packersdk.Ui) // Set VpcID if none was specified but filters are defined in the template. @@ -191,7 +196,7 @@ func (s *StepNetworkInfo) Run(ctx context.Context, state multistep.StateBag) mul ui.Error(err.Error()) return multistep.ActionHalt } - subnets, err := ec2conn.DescribeSubnets(params) + subnetOut, err := ec2conn.DescribeSubnets(params) if err != nil { err := fmt.Errorf("Failed to describe subnets: %s", err) state.Put("error", err) @@ -199,7 +204,22 @@ func (s *StepNetworkInfo) Run(ctx context.Context, state multistep.StateBag) mul return multistep.ActionHalt } - subnet := mostFreeSubnet(subnets.Subnets) + subnets := subnetOut.Subnets + + // Filter by AZ with support for machine type + azs := getAZFromSubnets(subnets) + azs, err = filterAZByMachineType(azs, s.RequestedMachineType, ec2conn) + if err != nil { + err := fmt.Errorf("Failed to filter subnets by AZ for machine type %q: %s", + s.RequestedMachineType, + err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + subnets = filterSubnetsByAZ(subnets, azs) + + subnet := mostFreeSubnet(subnets) s.SubnetId = *subnet.SubnetId ui.Say(fmt.Sprintf("Set subnet as %q", s.SubnetId)) @@ -231,4 +251,67 @@ func (s *StepNetworkInfo) Run(ctx context.Context, state multistep.StateBag) mul return multistep.ActionContinue } +func getAZFromSubnets(subnets []*ec2.Subnet) []string { + azs := map[string]struct{}{} + for _, sub := range subnets { + azs[*sub.AvailabilityZone] = struct{}{} + } + + retAZ := make([]string, 0, len(azs)) + for az := range azs { + retAZ = append(retAZ, az) + } + + return retAZ +} + +func filterAZByMachineType(azs []string, machineType string, ec2conn ec2iface.EC2API) ([]string, error) { + var retAZ []string + + for _, az := range azs { + resp, err := ec2conn.DescribeInstanceTypeOfferings(&ec2.DescribeInstanceTypeOfferingsInput{ + LocationType: aws.String("availability-zone"), + Filters: []*ec2.Filter{ + { + Name: aws.String("location"), + Values: []*string{&az}, + }, + }, + }) + if err != nil { + err = fmt.Errorf("failed to get offerings for AZ %q: %s", az, err) + return nil, err + } + + for _, off := range resp.InstanceTypeOfferings { + if *off.InstanceType == machineType { + retAZ = append(retAZ, az) + break + } + } + } + + if retAZ == nil { + return nil, fmt.Errorf("no AZ match the requested machine type %q", machineType) + } + + return retAZ, nil +} + +func filterSubnetsByAZ(subnets []*ec2.Subnet, azs []string) []*ec2.Subnet { + var retSubs []*ec2.Subnet + +outLoop: + for _, sub := range subnets { + for _, az := range azs { + if *sub.AvailabilityZone == az { + retSubs = append(retSubs, sub) + continue outLoop + } + } + } + + return retSubs +} + func (s *StepNetworkInfo) Cleanup(multistep.StateBag) {} diff --git a/builder/ebs/builder.go b/builder/ebs/builder.go index 1c5bef3a2..048a5ff14 100644 --- a/builder/ebs/builder.go +++ b/builder/ebs/builder.go @@ -305,6 +305,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) SubnetFilter: b.config.SubnetFilter, AvailabilityZone: b.config.AvailabilityZone, AssociatePublicIpAddress: b.config.AssociatePublicIpAddress, + RequestedMachineType: b.config.InstanceType, }, &awscommon.StepKeyPair{ Debug: b.config.PackerDebug, diff --git a/builder/ebssurrogate/builder.go b/builder/ebssurrogate/builder.go index b0d79353d..6f36010e6 100644 --- a/builder/ebssurrogate/builder.go +++ b/builder/ebssurrogate/builder.go @@ -333,6 +333,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) SubnetFilter: b.config.SubnetFilter, AvailabilityZone: b.config.AvailabilityZone, AssociatePublicIpAddress: b.config.AssociatePublicIpAddress, + RequestedMachineType: b.config.InstanceType, }, &awscommon.StepKeyPair{ Debug: b.config.PackerDebug, diff --git a/builder/ebsvolume/builder.go b/builder/ebsvolume/builder.go index 47c0a62f6..9d8929377 100644 --- a/builder/ebsvolume/builder.go +++ b/builder/ebsvolume/builder.go @@ -280,6 +280,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) SubnetFilter: b.config.SubnetFilter, AvailabilityZone: b.config.AvailabilityZone, AssociatePublicIpAddress: b.config.AssociatePublicIpAddress, + RequestedMachineType: b.config.InstanceType, }, &awscommon.StepKeyPair{ Debug: b.config.PackerDebug, diff --git a/builder/instance/builder.go b/builder/instance/builder.go index e917bb621..3eaa837b7 100644 --- a/builder/instance/builder.go +++ b/builder/instance/builder.go @@ -350,6 +350,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) SubnetFilter: b.config.SubnetFilter, AvailabilityZone: b.config.AvailabilityZone, AssociatePublicIpAddress: b.config.AssociatePublicIpAddress, + RequestedMachineType: b.config.InstanceType, }, &awscommon.StepKeyPair{ Debug: b.config.PackerDebug, From 2d791175f201f865c70754b5cac3ad603e09d359 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 21 Apr 2023 13:24:47 -0400 Subject: [PATCH 2/7] step_network_info: add tests for AZ/subnet select The step_network_info step was only tested via acceptance tests, which considering the complexity of the code, is not enough. To remedy this, we add some unit tests on the functions added for filtering the subnets based on their AZ and the machine type, and for testing the step itself in this configuration. --- builder/common/step_network_info.go | 4 + builder/common/step_network_info_test.go | 355 +++++++++++++++++++++++ 2 files changed, 359 insertions(+) create mode 100644 builder/common/step_network_info_test.go diff --git a/builder/common/step_network_info.go b/builder/common/step_network_info.go index a01d4bd59..ea79316b6 100644 --- a/builder/common/step_network_info.go +++ b/builder/common/step_network_info.go @@ -276,6 +276,10 @@ func filterAZByMachineType(azs []string, machineType string, ec2conn ec2iface.EC Name: aws.String("location"), Values: []*string{&az}, }, + { + Name: aws.String("instance-type"), + Values: []*string{&machineType}, + }, }, }) if err != nil { diff --git a/builder/common/step_network_info_test.go b/builder/common/step_network_info_test.go new file mode 100644 index 000000000..f301318f3 --- /dev/null +++ b/builder/common/step_network_info_test.go @@ -0,0 +1,355 @@ +package common + +import ( + "context" + "fmt" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/ec2/ec2iface" + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/packer-plugin-sdk/multistep" + packersdk "github.com/hashicorp/packer-plugin-sdk/packer" + confighelper "github.com/hashicorp/packer-plugin-sdk/template/config" +) + +type mockEC2ClientStepNetworkTests struct { + ec2iface.EC2API + + describeInstanceTypeOfferings func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) + describeVpcs func(*ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) + describeSubnets func(*ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) +} + +func (m *mockEC2ClientStepNetworkTests) DescribeInstanceTypeOfferings(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + if m.describeInstanceTypeOfferings != nil { + return m.describeInstanceTypeOfferings(in) + } + + return nil, fmt.Errorf("unimplemented: describeInstanceTypeOfferings") +} + +func (m *mockEC2ClientStepNetworkTests) DescribeVpcs(in *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { + if m.describeVpcs != nil { + return m.describeVpcs(in) + } + + return nil, fmt.Errorf("unimplemented: describeVpcs") +} + +func (m *mockEC2ClientStepNetworkTests) DescribeSubnets(in *ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) { + if m.describeSubnets != nil { + return m.describeSubnets(in) + } + + return nil, fmt.Errorf("unimplemented: describeSubnets") +} + +func TestGetFilterAZByMachineType(t *testing.T) { + testcases := []struct { + name string + describeImpl func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) + machineType string + inputAZs []string + expectedAZs []string + expectError bool + }{ + { + "Fail: describe returns an error", + func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + return nil, fmt.Errorf("STOP") + }, + "t2.micro", + []string{"us-east-1a", "us-east-1b"}, + nil, + true, + }, + { + "Fail, no AZ match machine type", + func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + return &ec2.DescribeInstanceTypeOfferingsOutput{ + InstanceTypeOfferings: []*ec2.InstanceTypeOffering{ + { + InstanceType: aws.String("t3.mini"), + }, + { + InstanceType: aws.String("t2.mini"), + }, + }, + }, nil + }, + "t2.micro", + []string{"us-east-1b", "us-east-1c"}, + nil, + true, + }, + { + "OK, found at least one AZ matching machine type", + func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + return &ec2.DescribeInstanceTypeOfferingsOutput{ + InstanceTypeOfferings: []*ec2.InstanceTypeOffering{ + { + InstanceType: aws.String("t2.micro"), + }, + }, + }, nil + }, + "t2.micro", + []string{"us-east-1a", "us-east-1b"}, + []string{"us-east-1a", "us-east-1b"}, + false, + }, + } + + for _, tt := range testcases { + t.Run(tt.name, func(t *testing.T) { + conn := &mockEC2ClientStepNetworkTests{} + conn.describeInstanceTypeOfferings = tt.describeImpl + + retAZ, err := filterAZByMachineType(tt.inputAZs, tt.machineType, conn) + + diff := cmp.Diff(retAZ, tt.expectedAZs) + if diff != "" { + t.Errorf("AZ mismatch between computed and expected: %s", diff) + } + + if (err != nil) != tt.expectError { + t.Errorf("Error mismatch, got %t, expected %t", err != nil, tt.expectError) + } + + if err != nil { + t.Logf("Got an error: %s", err) + } + }) + } +} + +func TestFilterSubnetsByAZ(t *testing.T) { + testcases := []struct { + name string + inSubnets []*ec2.Subnet + azs []string + outSubnets []*ec2.Subnet + }{ + { + "No subnet matching", + []*ec2.Subnet{ + { + AvailabilityZone: aws.String("us-east-1-c"), + }, + }, + []string{"us-east-1a"}, + nil, + }, + { + "Found subnet matching", + []*ec2.Subnet{ + { + SubnetId: aws.String("subnet1"), + AvailabilityZone: aws.String("us-east-1c"), + }, + { + SubnetId: aws.String("subnet2"), + AvailabilityZone: aws.String("us-east-1b"), + }, + }, + []string{"us-east-1c"}, + []*ec2.Subnet{ + { + SubnetId: aws.String("subnet1"), + AvailabilityZone: aws.String("us-east-1c"), + }, + }, + }, + { + "Found multiple subnets matching", + []*ec2.Subnet{ + { + SubnetId: aws.String("subnet1"), + AvailabilityZone: aws.String("us-east-1c"), + }, + { + SubnetId: aws.String("subnet2"), + AvailabilityZone: aws.String("us-east-1c"), + }, + }, + []string{"us-east-1c"}, + []*ec2.Subnet{ + { + SubnetId: aws.String("subnet1"), + AvailabilityZone: aws.String("us-east-1c"), + }, + { + SubnetId: aws.String("subnet2"), + AvailabilityZone: aws.String("us-east-1c"), + }, + }, + }, + } + + for _, tt := range testcases { + t.Run(tt.name, func(t *testing.T) { + subnets := filterSubnetsByAZ(tt.inSubnets, tt.azs) + diff := cmp.Diff(subnets, tt.outSubnets) + if diff != "" { + t.Errorf("subnet mismatch between computed and expected: %s", diff) + } + }) + } +} + +func TestStepNetworkWithPublicIPSetAndNoVPCOrSubnet(t *testing.T) { + mockConn := &mockEC2ClientStepNetworkTests{ + describeVpcs: func(dvi *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { + ok := false + for _, filter := range dvi.Filters { + if *filter.Name == "is-default" { + ok = true + break + } + } + + if !ok { + return nil, fmt.Errorf("expected to filter on default VPC = true, did not find that filter") + } + + return &ec2.DescribeVpcsOutput{ + Vpcs: []*ec2.Vpc{ + { + VpcId: aws.String("default-vpc"), + }, + }, + }, nil + }, + describeSubnets: func(dsi *ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) { + if dsi.SubnetIds != nil { + sub := dsi.SubnetIds[0] + if *sub != "subnet1" { + return nil, fmt.Errorf("expected selected subnet to be us-east-1a, but was %q", *sub) + } + + return &ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{ + { + SubnetId: aws.String("subnet1"), + AvailabilityZone: aws.String("us-east-1a"), + VpcId: aws.String("default-vpc"), + }, + }, + }, nil + } + + vpcFilterFound := false + for _, filter := range dsi.Filters { + if *filter.Name != "vpc-id" { + continue + } + filterVal := *filter.Values[0] + if filterVal != "default-vpc" { + return nil, fmt.Errorf("expected vpc-id filter to be %q, got %q", "default-vpc", filterVal) + } + + vpcFilterFound = true + } + + if !vpcFilterFound { + return nil, fmt.Errorf("expected to find vpc-id filter, but did not find it") + } + + return &ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{ + { + AvailabilityZone: aws.String("us-east-1a"), + SubnetId: aws.String("subnet1"), + AvailableIpAddressCount: aws.Int64(256), + }, + { + AvailabilityZone: aws.String("us-east-1b"), + SubnetId: aws.String("subnet2"), + AvailableIpAddressCount: aws.Int64(512), + }, + }, + }, nil + }, + describeInstanceTypeOfferings: func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + if *in.LocationType != "availability-zone" { + return nil, fmt.Errorf("called DescribeInstanceTypeOfferings with LocationType = %q, expected availability_zone", *in.LocationType) + } + + var machines []*ec2.InstanceTypeOffering + + foundLocation := false + for _, filter := range in.Filters { + if *filter.Name != "location" { + continue + } + foundLocation = true + + filterVal := *filter.Values[0] + switch filterVal { + case "us-east-1a": + machines = []*ec2.InstanceTypeOffering{ + { + InstanceType: aws.String("t2.mini"), + }, + { + InstanceType: aws.String("t3.large"), + }, + } + case "us-east-1b": + machines = []*ec2.InstanceTypeOffering{ + { + InstanceType: aws.String("t2.mini"), + }, + { + InstanceType: aws.String("t2.micro"), + }, + } + default: + return nil, fmt.Errorf("error: location %q not expected", filterVal) + } + } + + if !foundLocation { + return nil, fmt.Errorf("couldn't find location in filters") + } + + return &ec2.DescribeInstanceTypeOfferingsOutput{ + InstanceTypeOfferings: machines, + }, nil + }, + } + + stepConfig := &StepNetworkInfo{ + AssociatePublicIpAddress: confighelper.TriTrue, + RequestedMachineType: "t3.large", + } + + state := &multistep.BasicStateBag{} + state.Put("ec2", mockConn) + state.Put("ui", &packersdk.MockUi{}) + + actRet := stepConfig.Run(context.Background(), state) + if actRet == multistep.ActionHalt { + t.Fatalf("running the step failed: %s", state.Get("error").(error)) + } + + vpcid, ok := state.GetOk("vpc_id") + if !ok || vpcid != "default-vpc" { + t.Errorf("error: vpc should be 'default-vpc', but is %q", vpcid) + } + t.Logf("set vpc is %q", vpcid) + + subnetid, ok := state.GetOk("subnet_id") + if !ok || subnetid != "subnet1" { + t.Errorf("error: subnet should be 'subnet1', but is %q", subnetid) + } + t.Logf("set subnet is %q", subnetid) + + az, ok := state.GetOk("availability_zone") + if !ok || az != "us-east-1a" { + t.Errorf("error: availability_zone should be 'us-east-1a', but is %q", az) + } + t.Logf("set AZ is %q", az) +} From 55cf4bc595b7de291693ed89bd89f66c8daec694 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 21 Apr 2023 14:01:06 -0400 Subject: [PATCH 3/7] network: don't fail when getting default VPC fails When the associate_public_ip_address attribute is set in a template, without a VPC or a Subnet, we attempt to get the default VPC for the account. This may however fail if the `ec2:DescribeVpcs' permission is not set. This is a regression compared to older versions of the plugin, where the builds succeeded, and the associate_public_ip_address was silently ignored. To avoid breaking more clients, we decide to not treat errors when attempting to infer a default subnet or VPC from the account, and instead warn that the option will be ignored. --- builder/common/step_network_info.go | 142 +++++++++++------------ builder/common/step_network_info_test.go | 54 +++++++++ builder/ebs/builder_acc_test.go | 4 - 3 files changed, 125 insertions(+), 75 deletions(-) diff --git a/builder/common/step_network_info.go b/builder/common/step_network_info.go index ea79316b6..cf044a7c0 100644 --- a/builder/common/step_network_info.go +++ b/builder/common/step_network_info.go @@ -150,79 +150,12 @@ func (s *StepNetworkInfo) Run(ctx context.Context, state multistep.StateBag) mul // Set VPC/Subnet if we explicitely enable or disable public IP assignment to the instance // and we did not set or get a subnet ID before if s.AssociatePublicIpAddress != confighelper.TriUnset && s.SubnetId == "" { - ui.Say(fmt.Sprintf("Setting public IP address to %t on instance without a subnet ID", - *s.AssociatePublicIpAddress.ToBoolPointer())) - - if s.VpcId == "" { - ui.Say("No VPC ID provided, Packer will use the default VPC") - vpcs, err := ec2conn.DescribeVpcs(&ec2.DescribeVpcsInput{ - Filters: []*ec2.Filter{ - { - Name: aws.String("is-default"), - Values: []*string{aws.String("true")}, - }, - }, - }) - if err != nil { - err := fmt.Errorf("Failed to describe VPCs: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - - if len(vpcs.Vpcs) != 1 { - err := fmt.Errorf("No default VPC found") - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - defaultVPC := vpcs.Vpcs[0] - - s.VpcId = *defaultVPC.VpcId - } - - var err error - - ui.Say(fmt.Sprintf("Inferring subnet from the selected VPC %q", s.VpcId)) - params := &ec2.DescribeSubnetsInput{} - filters := map[string]string{ - "vpc-id": s.VpcId, - "state": "available", - } - params.Filters, err = buildEc2Filters(filters) - if err != nil { - err := fmt.Errorf("Failed to prepare subnet filters: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - subnetOut, err := ec2conn.DescribeSubnets(params) - if err != nil { - err := fmt.Errorf("Failed to describe subnets: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - - subnets := subnetOut.Subnets - - // Filter by AZ with support for machine type - azs := getAZFromSubnets(subnets) - azs, err = filterAZByMachineType(azs, s.RequestedMachineType, ec2conn) + err := s.GetDefaultVPCAndSubnet(ui, ec2conn, state) if err != nil { - err := fmt.Errorf("Failed to filter subnets by AZ for machine type %q: %s", - s.RequestedMachineType, - err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt + ui.Say("associate_public_ip_address is set without a subnet_id.") + ui.Say(fmt.Sprintf("Packer attempted to infer a subnet from default VPC (if unspecified), but failed due to: %s", err)) + ui.Say("The associate_public_ip_address will be ignored for the remainder of the build, and a public IP will only be associated if the VPC chosen enables it by default.") } - subnets = filterSubnetsByAZ(subnets, azs) - - subnet := mostFreeSubnet(subnets) - s.SubnetId = *subnet.SubnetId - - ui.Say(fmt.Sprintf("Set subnet as %q", s.SubnetId)) } // Try to find AZ and VPC Id from Subnet if they are not yet found/given @@ -251,6 +184,73 @@ func (s *StepNetworkInfo) Run(ctx context.Context, state multistep.StateBag) mul return multistep.ActionContinue } +func (s *StepNetworkInfo) GetDefaultVPCAndSubnet(ui packersdk.Ui, ec2conn ec2iface.EC2API, state multistep.StateBag) error { + ui.Say(fmt.Sprintf("Setting public IP address to %t on instance without a subnet ID", + *s.AssociatePublicIpAddress.ToBoolPointer())) + + var vpc = s.VpcId + if vpc == "" { + ui.Say("No VPC ID provided, Packer will use the default VPC") + vpcs, err := ec2conn.DescribeVpcs(&ec2.DescribeVpcsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("is-default"), + Values: []*string{aws.String("true")}, + }, + }, + }) + if err != nil { + return fmt.Errorf("Failed to describe VPCs: %s", err) + } + + if len(vpcs.Vpcs) != 1 { + return fmt.Errorf("No default VPC found") + } + vpc = *vpcs.Vpcs[0].VpcId + } + + var err error + + ui.Say(fmt.Sprintf("Inferring subnet from the selected VPC %q", vpc)) + params := &ec2.DescribeSubnetsInput{} + filters := map[string]string{ + "vpc-id": vpc, + "state": "available", + } + params.Filters, err = buildEc2Filters(filters) + if err != nil { + return fmt.Errorf("Failed to prepare subnet filters: %s", err) + } + subnetOut, err := ec2conn.DescribeSubnets(params) + if err != nil { + return fmt.Errorf("Failed to describe subnets: %s", err) + } + + subnets := subnetOut.Subnets + + // Filter by AZ with support for machine type + azs := getAZFromSubnets(subnets) + azs, err = filterAZByMachineType(azs, s.RequestedMachineType, ec2conn) + if err != nil { + return fmt.Errorf("Failed to filter subnets by AZ for machine type %q: %s", + s.RequestedMachineType, + err) + } + subnets = filterSubnetsByAZ(subnets, azs) + if subnets == nil { + return fmt.Errorf("Failed to get subnets for the filtered AZs") + } + + subnet := mostFreeSubnet(subnets) + + s.SubnetId = *subnet.SubnetId + s.VpcId = vpc + + ui.Say(fmt.Sprintf("Set subnet as %q", s.SubnetId)) + + return nil +} + func getAZFromSubnets(subnets []*ec2.Subnet) []string { azs := map[string]struct{}{} for _, sub := range subnets { diff --git a/builder/common/step_network_info_test.go b/builder/common/step_network_info_test.go index f301318f3..682f4fcc4 100644 --- a/builder/common/step_network_info_test.go +++ b/builder/common/step_network_info_test.go @@ -353,3 +353,57 @@ func TestStepNetworkWithPublicIPSetAndNoVPCOrSubnet(t *testing.T) { } t.Logf("set AZ is %q", az) } + +func TestGetDefaultVPCFailDueToPermissions(t *testing.T) { + mockConn := &mockEC2ClientStepNetworkTests{ + describeVpcs: func(dvi *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { + return nil, fmt.Errorf("Insufficient permissions: missing ec2:DescribeVpcs") + }, + } + + stepConfig := &StepNetworkInfo{ + AssociatePublicIpAddress: confighelper.TriTrue, + RequestedMachineType: "t3.large", + } + + ui := &packersdk.MockUi{} + + state := &multistep.BasicStateBag{} + state.Put("ec2", mockConn) + state.Put("ui", ui) + + actRet := stepConfig.Run(context.Background(), state) + if actRet == multistep.ActionHalt { + t.Fatalf("running the step failed: %s", state.Get("error").(error)) + } + + vpcid, ok := state.GetOk("vpc_id") + if !ok || vpcid != "" { + t.Errorf("error: vpc should be empty, but is %q", vpcid) + } + t.Logf("set vpc is %q", vpcid) + + subnetid, ok := state.GetOk("subnet_id") + if !ok || subnetid != "" { + t.Errorf("error: subnet should be empty, but is %q", subnetid) + } + t.Logf("set subnet is %q", subnetid) + + az, ok := state.GetOk("availability_zone") + if !ok || az != "" { + t.Errorf("error: availability_zone should be empty, but is %q", az) + } + t.Logf("set AZ is %q", az) + + ok = false + for _, msg := range ui.SayMessages { + if msg.Message == "associate_public_ip_address is set without a subnet_id." { + t.Log("found warning on associate_public_ip_address") + ok = true + } + } + + if !ok { + t.Errorf("failed to find a message that states that associate_public_ip_address will be ignored.") + } +} diff --git a/builder/ebs/builder_acc_test.go b/builder/ebs/builder_acc_test.go index a91447339..8a85e2859 100644 --- a/builder/ebs/builder_acc_test.go +++ b/builder/ebs/builder_acc_test.go @@ -1287,10 +1287,6 @@ func TestAccBuilder_AssociatePublicIPWithoutSubnet(t *testing.T) { return fmt.Errorf("did not pick the default VPC when setting subnet") } - if !strings.Contains(string(logs), "AvailabilityZone found") { - return fmt.Errorf("did not get AZ from subnet") - } - return nil }, } From 43c7309ccd581f9a1902087bcd4e3ef345c1d67c Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 21 Apr 2023 15:00:48 -0400 Subject: [PATCH 4/7] builder: fix message in acceptance tests --- builder/ebs/builder_acc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/ebs/builder_acc_test.go b/builder/ebs/builder_acc_test.go index 8a85e2859..6b620a505 100644 --- a/builder/ebs/builder_acc_test.go +++ b/builder/ebs/builder_acc_test.go @@ -1283,7 +1283,7 @@ func TestAccBuilder_AssociatePublicIPWithoutSubnet(t *testing.T) { return fmt.Errorf("did not change the public IP setting for the instance") } - if !strings.Contains(string(logs), "No VPC ID provided, Packer will choose one from the provided or default VPC") { + if !strings.Contains(string(logs), "No VPC ID provided, Packer will use the default VPC") { return fmt.Errorf("did not pick the default VPC when setting subnet") } From e2b2c198000cf7fe1be889d1b7187ee03f3cd022 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Mon, 24 Apr 2023 15:33:09 -0400 Subject: [PATCH 5/7] network: don't fail if can't pick AZ by capability When selecting a default subnet, we describe the instance type offering of each AZ linked to the subnets available in the VPC we picked. This may fail if we don't have the permissions to do so, and in this case, rather than failing to pick one, and ignore the associate_public_ip_address, we continue without filtering by AZ capability, and end-up picking the most available subnet out of all the available ones. We also print a message so users know which permission may be missing from their IAM configs, and they can decide to fix that. --- builder/common/step_network_info.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/builder/common/step_network_info.go b/builder/common/step_network_info.go index cf044a7c0..3ff231a5a 100644 --- a/builder/common/step_network_info.go +++ b/builder/common/step_network_info.go @@ -231,20 +231,24 @@ func (s *StepNetworkInfo) GetDefaultVPCAndSubnet(ui packersdk.Ui, ec2conn ec2ifa // Filter by AZ with support for machine type azs := getAZFromSubnets(subnets) azs, err = filterAZByMachineType(azs, s.RequestedMachineType, ec2conn) - if err != nil { - return fmt.Errorf("Failed to filter subnets by AZ for machine type %q: %s", - s.RequestedMachineType, - err) - } - subnets = filterSubnetsByAZ(subnets, azs) - if subnets == nil { - return fmt.Errorf("Failed to get subnets for the filtered AZs") + if err == nil { + subnets = filterSubnetsByAZ(subnets, azs) + if subnets == nil { + return fmt.Errorf("Failed to get subnets for the filtered AZs") + } + } else { + ui.Say(fmt.Sprintf( + "Failed to filter subnets/AZ for the requested machine type %q: %s", + s.RequestedMachineType, err)) + ui.Say("This may result in Packer picking a subnet/AZ that can't host the requested machine type") + ui.Say("Please check that you have the permissions required to run DescribeInstanceTypeOfferings and try again.") } subnet := mostFreeSubnet(subnets) s.SubnetId = *subnet.SubnetId s.VpcId = vpc + s.AvailabilityZone = *subnet.AvailabilityZone ui.Say(fmt.Sprintf("Set subnet as %q", s.SubnetId)) From 1a6fb304feb6dc535acf90e480266a2b61c3f693 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Mon, 24 Apr 2023 15:35:59 -0400 Subject: [PATCH 6/7] network: fix test verbosity, and add a few more --- builder/common/step_network_info_test.go | 241 +++++++++++++++++++---- 1 file changed, 201 insertions(+), 40 deletions(-) diff --git a/builder/common/step_network_info_test.go b/builder/common/step_network_info_test.go index 682f4fcc4..eb48837f7 100644 --- a/builder/common/step_network_info_test.go +++ b/builder/common/step_network_info_test.go @@ -3,6 +3,7 @@ package common import ( "context" "fmt" + "strings" "testing" "github.com/aws/aws-sdk-go/aws" @@ -46,7 +47,7 @@ func (m *mockEC2ClientStepNetworkTests) DescribeSubnets(in *ec2.DescribeSubnetsI return nil, fmt.Errorf("unimplemented: describeSubnets") } -func TestGetFilterAZByMachineType(t *testing.T) { +func TestStepNetwork_GetFilterAZByMachineType(t *testing.T) { testcases := []struct { name string describeImpl func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) @@ -56,18 +57,18 @@ func TestGetFilterAZByMachineType(t *testing.T) { expectError bool }{ { - "Fail: describe returns an error", - func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + name: "Fail: describe returns an error", + describeImpl: func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { return nil, fmt.Errorf("STOP") }, - "t2.micro", - []string{"us-east-1a", "us-east-1b"}, - nil, - true, + machineType: "t2.micro", + inputAZs: []string{"us-east-1a", "us-east-1b"}, + expectedAZs: nil, + expectError: true, }, { - "Fail, no AZ match machine type", - func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + name: "Fail, no AZ match machine type", + describeImpl: func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { return &ec2.DescribeInstanceTypeOfferingsOutput{ InstanceTypeOfferings: []*ec2.InstanceTypeOffering{ { @@ -79,14 +80,14 @@ func TestGetFilterAZByMachineType(t *testing.T) { }, }, nil }, - "t2.micro", - []string{"us-east-1b", "us-east-1c"}, - nil, - true, + machineType: "t2.micro", + inputAZs: []string{"us-east-1b", "us-east-1c"}, + expectedAZs: nil, + expectError: true, }, { - "OK, found at least one AZ matching machine type", - func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + name: "OK, found at least one AZ matching machine type", + describeImpl: func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { return &ec2.DescribeInstanceTypeOfferingsOutput{ InstanceTypeOfferings: []*ec2.InstanceTypeOffering{ { @@ -95,10 +96,10 @@ func TestGetFilterAZByMachineType(t *testing.T) { }, }, nil }, - "t2.micro", - []string{"us-east-1a", "us-east-1b"}, - []string{"us-east-1a", "us-east-1b"}, - false, + machineType: "t2.micro", + inputAZs: []string{"us-east-1a", "us-east-1b"}, + expectedAZs: []string{"us-east-1a", "us-east-1b"}, + expectError: false, }, } @@ -125,7 +126,7 @@ func TestGetFilterAZByMachineType(t *testing.T) { } } -func TestFilterSubnetsByAZ(t *testing.T) { +func TestStepNetwork_FilterSubnetsByAZ(t *testing.T) { testcases := []struct { name string inSubnets []*ec2.Subnet @@ -133,18 +134,18 @@ func TestFilterSubnetsByAZ(t *testing.T) { outSubnets []*ec2.Subnet }{ { - "No subnet matching", - []*ec2.Subnet{ + name: "No subnet matching", + inSubnets: []*ec2.Subnet{ { AvailabilityZone: aws.String("us-east-1-c"), }, }, - []string{"us-east-1a"}, - nil, + azs: []string{"us-east-1a"}, + outSubnets: nil, }, { - "Found subnet matching", - []*ec2.Subnet{ + name: "Found subnet matching", + inSubnets: []*ec2.Subnet{ { SubnetId: aws.String("subnet1"), AvailabilityZone: aws.String("us-east-1c"), @@ -154,8 +155,8 @@ func TestFilterSubnetsByAZ(t *testing.T) { AvailabilityZone: aws.String("us-east-1b"), }, }, - []string{"us-east-1c"}, - []*ec2.Subnet{ + azs: []string{"us-east-1c"}, + outSubnets: []*ec2.Subnet{ { SubnetId: aws.String("subnet1"), AvailabilityZone: aws.String("us-east-1c"), @@ -163,8 +164,8 @@ func TestFilterSubnetsByAZ(t *testing.T) { }, }, { - "Found multiple subnets matching", - []*ec2.Subnet{ + name: "Found multiple subnets matching", + inSubnets: []*ec2.Subnet{ { SubnetId: aws.String("subnet1"), AvailabilityZone: aws.String("us-east-1c"), @@ -174,8 +175,8 @@ func TestFilterSubnetsByAZ(t *testing.T) { AvailabilityZone: aws.String("us-east-1c"), }, }, - []string{"us-east-1c"}, - []*ec2.Subnet{ + azs: []string{"us-east-1c"}, + outSubnets: []*ec2.Subnet{ { SubnetId: aws.String("subnet1"), AvailabilityZone: aws.String("us-east-1c"), @@ -199,7 +200,7 @@ func TestFilterSubnetsByAZ(t *testing.T) { } } -func TestStepNetworkWithPublicIPSetAndNoVPCOrSubnet(t *testing.T) { +func TestStepNetwork_WithPublicIPSetAndNoVPCOrSubnet(t *testing.T) { mockConn := &mockEC2ClientStepNetworkTests{ describeVpcs: func(dvi *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { ok := false @@ -354,7 +355,7 @@ func TestStepNetworkWithPublicIPSetAndNoVPCOrSubnet(t *testing.T) { t.Logf("set AZ is %q", az) } -func TestGetDefaultVPCFailDueToPermissions(t *testing.T) { +func TestStepNetwork_GetDefaultVPCFailDueToPermissions(t *testing.T) { mockConn := &mockEC2ClientStepNetworkTests{ describeVpcs: func(dvi *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { return nil, fmt.Errorf("Insufficient permissions: missing ec2:DescribeVpcs") @@ -381,29 +382,189 @@ func TestGetDefaultVPCFailDueToPermissions(t *testing.T) { if !ok || vpcid != "" { t.Errorf("error: vpc should be empty, but is %q", vpcid) } - t.Logf("set vpc is %q", vpcid) subnetid, ok := state.GetOk("subnet_id") if !ok || subnetid != "" { t.Errorf("error: subnet should be empty, but is %q", subnetid) } - t.Logf("set subnet is %q", subnetid) az, ok := state.GetOk("availability_zone") if !ok || az != "" { t.Errorf("error: availability_zone should be empty, but is %q", az) } - t.Logf("set AZ is %q", az) - ok = false + var foundMsg bool + for _, msg := range ui.SayMessages { if msg.Message == "associate_public_ip_address is set without a subnet_id." { t.Log("found warning on associate_public_ip_address") - ok = true + foundMsg = true } } - if !ok { + if !foundMsg { t.Errorf("failed to find a message that states that associate_public_ip_address will be ignored.") } } + +func TestStepNetwork_SetVPCAndSubnetWithoutAssociatePublicIP(t *testing.T) { + mockConn := &mockEC2ClientStepNetworkTests{} + + stepConfig := &StepNetworkInfo{ + AssociatePublicIpAddress: confighelper.TriUnset, + RequestedMachineType: "t3.large", + VpcId: "default-vpc", + SubnetId: "subnet1", + AvailabilityZone: "us-east-1a", + } + + ui := &packersdk.MockUi{} + + state := &multistep.BasicStateBag{} + state.Put("ec2", mockConn) + state.Put("ui", ui) + + actRet := stepConfig.Run(context.Background(), state) + if actRet == multistep.ActionHalt { + t.Fatalf("running the step failed: %s", state.Get("error").(error)) + } + + vpcid, ok := state.GetOk("vpc_id") + if !ok || vpcid != "default-vpc" { + t.Errorf("error: vpc should be 'default-vpc', but is %q", vpcid) + } + + subnetid, ok := state.GetOk("subnet_id") + if !ok || subnetid != "subnet1" { + t.Errorf("error: subnet should be 'subnet_id', but is %q", subnetid) + } + + az, ok := state.GetOk("availability_zone") + if !ok || az != "us-east-1a" { + t.Errorf("error: availability_zone should be 'us-east-1a', but is %q", az) + } + + var foundDefaultVPCMsg bool + for _, msg := range ui.SayMessages { + if strings.Contains(msg.Message, "Setting public IP address to") { + foundDefaultVPCMsg = true + } + } + + if foundDefaultVPCMsg { + t.Errorf("Should not have found a message that stated that we need to process public IP address setting") + } +} + +func TestStepNetwork_SetPublicIPAddressWithoutSubnetAndMissingDescribeInstanceTypeOfferings(t *testing.T) { + mockConn := &mockEC2ClientStepNetworkTests{ + describeVpcs: func(dvi *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { + ok := false + for _, filter := range dvi.Filters { + if *filter.Name == "is-default" { + ok = true + break + } + } + + if !ok { + return nil, fmt.Errorf("expected to filter on default VPC = true, did not find that filter") + } + + return &ec2.DescribeVpcsOutput{ + Vpcs: []*ec2.Vpc{ + { + VpcId: aws.String("default-vpc"), + }, + }, + }, nil + }, + describeSubnets: func(dsi *ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) { + if dsi.SubnetIds != nil { + sub := dsi.SubnetIds[0] + if *sub != "subnet1" { + return nil, fmt.Errorf("expected selected subnet to be us-east-1a, but was %q", *sub) + } + + return &ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{ + { + SubnetId: aws.String("subnet1"), + AvailabilityZone: aws.String("us-east-1a"), + VpcId: aws.String("default-vpc"), + }, + }, + }, nil + } + + vpcFilterFound := false + for _, filter := range dsi.Filters { + if *filter.Name != "vpc-id" { + continue + } + filterVal := *filter.Values[0] + if filterVal != "default-vpc" { + return nil, fmt.Errorf("expected vpc-id filter to be %q, got %q", "default-vpc", filterVal) + } + + vpcFilterFound = true + } + + if !vpcFilterFound { + return nil, fmt.Errorf("expected to find vpc-id filter, but did not find it") + } + + return &ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{ + { + AvailabilityZone: aws.String("us-east-1a"), + SubnetId: aws.String("subnet1"), + AvailableIpAddressCount: aws.Int64(256), + }, + { + AvailabilityZone: aws.String("us-east-1b"), + SubnetId: aws.String("subnet2"), + AvailableIpAddressCount: aws.Int64(512), + }, + }, + }, nil + }, + describeInstanceTypeOfferings: func(in *ec2.DescribeInstanceTypeOfferingsInput) (*ec2.DescribeInstanceTypeOfferingsOutput, error) { + return nil, fmt.Errorf("Missing permission: ec2:DescribeInstanceTypeOfferings") + }, + } + + stepConfig := &StepNetworkInfo{ + AssociatePublicIpAddress: confighelper.TriTrue, + RequestedMachineType: "t3.large", + } + + ui := &packersdk.MockUi{} + + state := &multistep.BasicStateBag{} + state.Put("ec2", mockConn) + state.Put("ui", ui) + + actRet := stepConfig.Run(context.Background(), state) + if actRet == multistep.ActionHalt { + t.Fatalf("running the step failed: %s", state.Get("error").(error)) + } + + vpcid, ok := state.GetOk("vpc_id") + if !ok || vpcid != "default-vpc" { + t.Errorf("error: vpc should be 'default-vpc', but is %q", vpcid) + } + t.Logf("set vpc is %q", vpcid) + + subnetid, ok := state.GetOk("subnet_id") + if !ok || subnetid != "subnet2" { + t.Errorf("error: subnet should be 'subnet2', but is %q", subnetid) + } + t.Logf("set subnet is %q", subnetid) + + az, ok := state.GetOk("availability_zone") + if !ok || az != "us-east-1b" { + t.Errorf("error: availability_zone should be 'us-east-1a', but is %q", az) + } + t.Logf("set AZ is %q", az) +} From 8f6493395fc95840441c89922e52c003e7b34e45 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Mon, 24 Apr 2023 15:53:15 -0400 Subject: [PATCH 7/7] run_config: add documentation on extra permissions Since the DescribeInstanceTypeOfferings API call may require an extra action to be allowed in the IAM roles defined, we add an extra exerpt in the documentation for the `associate_public_ip_address' configuration flag, so that users understand the implication of not being able to perform this call. --- builder/common/run_config.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/builder/common/run_config.go b/builder/common/run_config.go index f9df06e55..d76d75b95 100644 --- a/builder/common/run_config.go +++ b/builder/common/run_config.go @@ -105,6 +105,12 @@ type RunConfig struct { // // * ec2:DescribeVpcs // * ec2:DescribeSubnets + // + // Additionally, since we filter subnets/AZs by their capability to host + // an instance of the selected type, you may also want to define the + // `ec2:DescribeInstanceTypeOfferings` action to the role running the build. + // Otherwise, Packer will pick the most available subnet in the VPC selected, + // which may not be able to host the instance type you provided. AssociatePublicIpAddress confighelper.Trilean `mapstructure:"associate_public_ip_address" required:"false"` // Destination availability zone to launch // instance in. Leave this empty to allow Amazon to auto-assign.