From f9f4726effe22a1c561e6677f57711d82119b6c7 Mon Sep 17 00:00:00 2001 From: nywilken Date: Fri, 8 Nov 2019 16:13:45 -0500 Subject: [PATCH] builder/amazon/step_pre_validate: Add check for non-default VPCs Subnet information is only really needed when the specified `vpc_id` is not the default VPC for the region where the builder is being executed. This change uses the AWS API to determine if the VPC provided is a non-default VPC and only validates the existence of a `subnet_id` if a user has provided a non-default `vpc_id`. Tests after change ``` > make test TEST=./builder/amazon/... TESTARGS='-count=1 -v -run=TestStepPreValidate_checkVpc' ... === RUN TestStepPreValidate_checkVpc === RUN TestStepPreValidate_checkVpc/DefaultVpc === RUN TestStepPreValidate_checkVpc/NonDefaultVpcNoSubnet === RUN TestStepPreValidate_checkVpc/NonDefaultVpcWithSubnet === RUN TestStepPreValidate_checkVpc/SubnetWithNoVpc === RUN TestStepPreValidate_checkVpc/NoVpcInformation --- PASS: TestStepPreValidate_checkVpc (0.00s) --- PASS: TestStepPreValidate_checkVpc/DefaultVpc (0.00s) --- PASS: TestStepPreValidate_checkVpc/NonDefaultVpcNoSubnet (0.00s) --- PASS: TestStepPreValidate_checkVpc/NonDefaultVpcWithSubnet (0.00s) --- PASS: TestStepPreValidate_checkVpc/SubnetWithNoVpc (0.00s) --- PASS: TestStepPreValidate_checkVpc/NoVpcInformation (0.00s) PASS ... ``` --- builder/amazon/common/step_pre_validate.go | 38 +++++++++- .../amazon/common/step_pre_validate_test.go | 72 +++++++++++++++++++ .../amazon/common/step_run_source_instance.go | 8 +++ builder/amazon/ebs/builder.go | 2 + builder/amazon/ebssurrogate/builder.go | 2 + builder/amazon/instance/builder.go | 2 + 6 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 builder/amazon/common/step_pre_validate_test.go diff --git a/builder/amazon/common/step_pre_validate.go b/builder/amazon/common/step_pre_validate.go index 8b5c46322..ae0c6d41c 100644 --- a/builder/amazon/common/step_pre_validate.go +++ b/builder/amazon/common/step_pre_validate.go @@ -8,6 +8,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/common/retry" "github.com/hashicorp/packer/helper/multistep" "github.com/hashicorp/packer/packer" @@ -20,6 +21,8 @@ type StepPreValidate struct { DestAmiName string ForceDeregister bool AMISkipBuildRegion bool + VpcId string + SubnetId string } func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { @@ -76,6 +79,7 @@ func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul ui.Say("Force Deregister flag found, skipping prevalidating AMI Name") return multistep.ActionContinue } + if s.AMISkipBuildRegion { ui.Say("skip_build_region was set; not prevalidating AMI name") return multistep.ActionContinue @@ -83,6 +87,14 @@ func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul ec2conn := state.Get("ec2").(*ec2.EC2) + // Validate VPC settings for non-default VPCs + ui.Say("Prevalidating any provided VPC information") + if err := s.checkVpc(ec2conn); err != nil { + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + ui.Say(fmt.Sprintf("Prevalidating AMI Name: %s", s.DestAmiName)) req, resp := ec2conn.DescribeImagesRequest(&ec2.DescribeImagesInput{ Filters: []*ec2.Filter{{ @@ -91,13 +103,13 @@ func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul }}}) req.RetryCount = 11 - err := req.Send() - if err != nil { - err := fmt.Errorf("Error querying AMI: %s", err) + if err := req.Send(); err != nil { + err = fmt.Errorf("Error querying AMI: %s", err) state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt } + if len(resp.Images) > 0 { err := fmt.Errorf("Error: AMI Name: '%s' is used by an existing AMI: %s", *resp.Images[0].Name, *resp.Images[0].ImageId) state.Put("error", err) @@ -108,4 +120,24 @@ func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul return multistep.ActionContinue } +func (s *StepPreValidate) checkVpc(conn ec2iface.EC2API) error { + if s.VpcId != "" && s.SubnetId != "" { + // skip validation if both VpcId and SubnetId are provided; AWS API will error if something is wrong. + return nil + } + + res, err := conn.DescribeVpcs(&ec2.DescribeVpcsInput{VpcIds: []*string{aws.String(s.VpcId)}}) + if isAWSErr(err, "InvalidVpcID.NotFound", "") || err != nil { + return fmt.Errorf("Error retrieving VPC information for vpc_id %q", s.VpcId) + } + + if res != nil && len(res.Vpcs) == 1 && res.Vpcs[0] != nil { + if isDefault := aws.BoolValue(res.Vpcs[0].IsDefault); !isDefault { + return fmt.Errorf("Error: subnet_id must be provided for non-default VPCs (%s)", s.VpcId) + } + } + return nil +} + +// Cleanup ... func (s *StepPreValidate) Cleanup(multistep.StateBag) {} diff --git a/builder/amazon/common/step_pre_validate_test.go b/builder/amazon/common/step_pre_validate_test.go new file mode 100644 index 000000000..95448fc21 --- /dev/null +++ b/builder/amazon/common/step_pre_validate_test.go @@ -0,0 +1,72 @@ +package common + +import ( + "fmt" + "strings" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" +) + +//DescribeVpcs mocks an ec2.DescribeVpcsOutput for a given input +func (m *mockEC2Conn) DescribeVpcs(input *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { + m.lock.Lock() + m.copyImageCount++ + m.lock.Unlock() + + if input == nil || len(input.VpcIds) == 0 { + return nil, fmt.Errorf("oops looks like we need more input") + } + + var isDefault bool + vpcID := aws.StringValue(input.VpcIds[0]) + + //only one default VPC per region + if strings.Contains("vpc-default-id", vpcID) { + isDefault = true + } + + output := &ec2.DescribeVpcsOutput{ + Vpcs: []*ec2.Vpc{ + &ec2.Vpc{IsDefault: aws.Bool(isDefault), + VpcId: aws.String(vpcID), + }, + }, + } + return output, nil +} + +func TestStepPreValidate_checkVpc(t *testing.T) { + tt := []struct { + name string + step StepPreValidate + errorExpected bool + }{ + {"DefaultVpc", StepPreValidate{VpcId: "vpc-default-id"}, false}, + {"NonDefaultVpcNoSubnet", StepPreValidate{VpcId: "vpc-1234567890"}, true}, + {"NonDefaultVpcWithSubnet", StepPreValidate{VpcId: "vpc-1234567890", SubnetId: "subnet-1234567890"}, false}, + {"SubnetWithNoVpc", StepPreValidate{SubnetId: "subnet-1234567890"}, false}, + {"NoVpcInformation", StepPreValidate{}, false}, + } + + mockConn, err := getMockConn(nil, "") + if err != nil { + t.Fatal("unable to get a mock connection") + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + err := tc.step.checkVpc(mockConn) + + if tc.errorExpected && err == nil { + t.Errorf("expected a validation error for %q but got %q", tc.name, err) + } + + if !tc.errorExpected && err != nil { + t.Errorf("expected a validation to pass for %q but got %q", tc.name, err) + } + }) + } + +} diff --git a/builder/amazon/common/step_run_source_instance.go b/builder/amazon/common/step_run_source_instance.go index f13ff3da3..ed0cefe53 100644 --- a/builder/amazon/common/step_run_source_instance.go +++ b/builder/amazon/common/step_run_source_instance.go @@ -177,6 +177,14 @@ func (s *StepRunSourceInstance) Run(ctx context.Context, state multistep.StateBa runReq, runResp := ec2conn.RunInstancesRequest(runOpts) runReq.RetryCount = 11 err = runReq.Send() + + if isAWSErr(err, "VPCIdNotSpecified", "No default VPC for this user") && subnetId == "" { + err := fmt.Errorf("Error launching source instance: a valid Subnet Id was not specified") + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + if err != nil { err := fmt.Errorf("Error launching source instance: %s", err) state.Put("error", err) diff --git a/builder/amazon/ebs/builder.go b/builder/amazon/ebs/builder.go index d2638d806..ab9f8a0e2 100644 --- a/builder/amazon/ebs/builder.go +++ b/builder/amazon/ebs/builder.go @@ -190,6 +190,8 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack DestAmiName: b.config.AMIName, ForceDeregister: b.config.AMIForceDeregister, AMISkipBuildRegion: b.config.AMISkipBuildRegion, + VpcId: b.config.VpcId, + SubnetId: b.config.SubnetId, }, &awscommon.StepSourceAMIInfo{ SourceAmi: b.config.SourceAmi, diff --git a/builder/amazon/ebssurrogate/builder.go b/builder/amazon/ebssurrogate/builder.go index 3e65bd6c4..2e93d19bc 100644 --- a/builder/amazon/ebssurrogate/builder.go +++ b/builder/amazon/ebssurrogate/builder.go @@ -232,6 +232,8 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack &awscommon.StepPreValidate{ DestAmiName: b.config.AMIName, ForceDeregister: b.config.AMIForceDeregister, + VpcId: b.config.VpcId, + SubnetId: b.config.SubnetId, }, &awscommon.StepSourceAMIInfo{ SourceAmi: b.config.SourceAmi, diff --git a/builder/amazon/instance/builder.go b/builder/amazon/instance/builder.go index 78ccbdd1a..395fac197 100644 --- a/builder/amazon/instance/builder.go +++ b/builder/amazon/instance/builder.go @@ -286,6 +286,8 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack &awscommon.StepPreValidate{ DestAmiName: b.config.AMIName, ForceDeregister: b.config.AMIForceDeregister, + VpcId: b.config.VpcId, + SubnetId: b.config.SubnetId, }, &awscommon.StepSourceAMIInfo{ SourceAmi: b.config.SourceAmi,