Merge pull request #8360 from hashicorp/fix-7167

builder/amazon: Add validation for `subnet_id` when specifying `vpc_id`

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 := req.Send(); err != nil {
-	if err != nil {
+		err = fmt.Errorf("Error querying AMI: %s", err)
-		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) {}

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)
+			}
+		})
+	}
+
+}

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)

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,

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,

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,