Address PR comments

hashicorp · May 12, 2023 · 1b68690 · 1b68690
1 parent c339cef
commit 1b68690
Show file tree

Hide file tree

Showing 22 changed files with 164 additions and 84 deletions.
diff --git a/builder/chroot/builder.go b/builder/chroot/builder.go
@@ -381,7 +381,7 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 	}
 
 	if b.config.TpmSupport != "" && b.config.TpmSupport != ec2.TpmSupportValuesV20 {
-		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support values are %q or the empty string`, ec2.TpmSupportValuesV20))
+		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support value is %q`, ec2.TpmSupportValuesV20))
 	}
 
 	if b.config.BootMode != "" {

diff --git a/builder/chroot/builder.hcl2spec.go b/builder/chroot/builder.hcl2spec.go
diff --git a/builder/chroot/step_attach_volume.go b/builder/chroot/step_attach_volume.go
@@ -19,9 +19,8 @@ import (
 // available device location.
 //
 // Produces:
-//
-//	device string - The location where the volume was attached.
-//	attach_cleanup CleanupFunc
+//   device string - The location where the volume was attached.
+//   attach_cleanup CleanupFunc
 type StepAttachVolume struct {
 	PollingConfig *awscommon.AWSPollingConfig
 	attached      bool

diff --git a/builder/chroot/step_create_volume.go b/builder/chroot/step_create_volume.go
@@ -22,8 +22,7 @@ import (
 // device of the AMI.
 //
 // Produces:
-//
-//	volume_id string - The ID of the created volume
+//   volume_id string - The ID of the created volume
 type StepCreateVolume struct {
 	PollingConfig         *awscommon.AWSPollingConfig
 	volumeId              string

diff --git a/builder/chroot/step_flock.go b/builder/chroot/step_flock.go
@@ -17,8 +17,7 @@ import (
 // StepFlock provisions the instance within a chroot.
 //
 // Produces:
-//
-//	flock_cleanup Cleanup - To perform early cleanup
+//   flock_cleanup Cleanup - To perform early cleanup
 type StepFlock struct {
 	fh *os.File
 }

diff --git a/builder/chroot/step_mount_device.go b/builder/chroot/step_mount_device.go
@@ -27,9 +27,8 @@ type mountPathData struct {
 // StepMountDevice mounts the attached device.
 //
 // Produces:
-//
-//	mount_path string - The location where the volume was mounted.
-//	mount_device_cleanup CleanupFunc - To perform early cleanup
+//   mount_path string - The location where the volume was mounted.
+//   mount_device_cleanup CleanupFunc - To perform early cleanup
 type StepMountDevice struct {
 	MountOptions   []string
 	MountPartition string

diff --git a/builder/chroot/step_snapshot.go b/builder/chroot/step_snapshot.go
@@ -17,8 +17,7 @@ import (
 // StepSnapshot creates a snapshot of the created volume.
 //
 // Produces:
-//
-//	snapshot_id string - ID of the created snapshot
+//   snapshot_id string - ID of the created snapshot
 type StepSnapshot struct {
 	PollingConfig *awscommon.AWSPollingConfig
 	snapshotId    string

diff --git a/builder/common/access_config.go b/builder/common/access_config.go
@@ -35,30 +35,26 @@ import (
 // HCL config example:
 //
 // ```HCL
-//
-//	source "amazon-ebs" "example" {
-//		assume_role {
-//			role_arn     = "arn:aws:iam::ACCOUNT_ID:role/ROLE_NAME"
-//			session_name = "SESSION_NAME"
-//			external_id  = "EXTERNAL_ID"
-//		}
-//	}
-//
+// source "amazon-ebs" "example" {
+// 	assume_role {
+// 		role_arn     = "arn:aws:iam::ACCOUNT_ID:role/ROLE_NAME"
+// 		session_name = "SESSION_NAME"
+// 		external_id  = "EXTERNAL_ID"
+// 	}
+// }
 // ```
 //
 // JSON config example:
 //
 // ```json
-//
-//	builder{
-//		"type": "amazon-ebs",
-//		"assume_role": {
-//			"role_arn"    :  "arn:aws:iam::ACCOUNT_ID:role/ROLE_NAME",
-//			"session_name":  "SESSION_NAME",
-//			"external_id" :  "EXTERNAL_ID"
-//		}
-//	}
-//
+// builder{
+// 	"type": "amazon-ebs",
+// 	"assume_role": {
+// 		"role_arn"    :  "arn:aws:iam::ACCOUNT_ID:role/ROLE_NAME",
+// 		"session_name":  "SESSION_NAME",
+// 		"external_id" :  "EXTERNAL_ID"
+// 	}
+// }
 // ```
 type AssumeRoleConfig struct {
 	// Amazon Resource Name (ARN) of the IAM Role to assume.

diff --git a/builder/common/awserrors/utils.go b/builder/common/awserrors/utils.go
@@ -10,9 +10,9 @@ import (
 )
 
 // Returns true if the err matches all these conditions:
-//   - err is of type awserr.Error
-//   - Error.Code() matches code
-//   - Error.Message() contains message
+//  * err is of type awserr.Error
+//  * Error.Code() matches code
+//  * Error.Message() contains message
 func Matches(err error, code string, message string) bool {
 	if err, ok := err.(awserr.Error); ok {
 		return err.Code() == code && strings.Contains(err.Message(), message)

diff --git a/builder/common/block_device.go b/builder/common/block_device.go
@@ -36,25 +36,21 @@ const (
 // HCL2 example:
 //
 // ```hcl
-//
-//	launch_block_device_mappings {
-//	    device_name = "/dev/sda1"
-//	    encrypted = true
-//	    kms_key_id = "1a2b3c4d-5e6f-1a2b-3c4d-5e6f1a2b3c4d"
-//	}
-//
+// launch_block_device_mappings {
+//     device_name = "/dev/sda1"
+//     encrypted = true
+//     kms_key_id = "1a2b3c4d-5e6f-1a2b-3c4d-5e6f1a2b3c4d"
+// }
 // ```
 //
 // JSON example:
 // ```json
 // "launch_block_device_mappings": [
-//
-//	{
-//	   "device_name": "/dev/sda1",
-//	   "encrypted": true,
-//	   "kms_key_id": "1a2b3c4d-5e6f-1a2b-3c4d-5e6f1a2b3c4d"
-//	}
-//
+//   {
+//      "device_name": "/dev/sda1",
+//      "encrypted": true,
+//      "kms_key_id": "1a2b3c4d-5e6f-1a2b-3c4d-5e6f1a2b3c4d"
+//   }
 // ]
 // ```
 //
@@ -63,6 +59,7 @@ const (
 //
 // Documentation for Block Devices Mappings can be found here:
 // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/block-device-mapping-concepts.html
+//
 type BlockDevice struct {
 	// Indicates whether the EBS volume is deleted on instance termination.
 	// Default false. NOTE: If this value is not explicitly set to true and

diff --git a/builder/common/step_pre_validate_test.go b/builder/common/step_pre_validate_test.go
@@ -12,7 +12,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 )
 
-// DescribeVpcs mocks an ec2.DescribeVpcsOutput for a given input
+//DescribeVpcs mocks an ec2.DescribeVpcsOutput for a given input
 func (m *mockEC2Conn) DescribeVpcs(input *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) {
 
 	if input == nil || aws.StringValue(input.VpcIds[0]) == "" {

diff --git a/builder/common/step_source_ami_info.go b/builder/common/step_source_ami_info.go
@@ -19,8 +19,7 @@ import (
 // that is used throughout the AMI creation process.
 //
 // Produces:
-//
-//	source_image *ec2.Image - the source AMI info
+//   source_image *ec2.Image - the source AMI info
 type StepSourceAMIInfo struct {
 	SourceAmi                string
 	EnableAMISriovNetSupport bool

diff --git a/builder/ebssurrogate/builder.go b/builder/ebssurrogate/builder.go
@@ -81,18 +81,10 @@ type Config struct {
 	// Base64 representation of the non-volatile UEFI variable store. For more information
 	// see [AWS documentation](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/uefi-secure-boot-optionB.html).
 	UefiData string `mapstructure:"uefi_data" required:"false"`
-<<<<<<< HEAD
-=======
-	// Enforce version of the Instance Metadata Service on the built AMI.
-	// Valid options are unset (legacy) and `v2.0`. See the documentation on
-	// [IMDS](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html)
-	// for more information. Defaults to legacy.
-	IMDSSupport string `mapstructure:"imds_support" required:"false"`
 	// NitroTPM Support. Valid options are `v2.0`. See the documentation on
 	// [NitroTPM Support](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-support-on-ami.html) for
 	// more information. Only enabled if a valid option is provided, otherwise ignored.
 	TpmSupport string `mapstructure:"tpm_support" required:"false"`
->>>>>>> 5bc11e8a (Added TPM as configuration option)
 
 	ctx interpolate.Context
 }
@@ -196,7 +188,7 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 	}
 
 	if b.config.TpmSupport != "" && b.config.TpmSupport != ec2.TpmSupportValuesV20 {
-		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support values are %q or the empty string`, ec2.TpmSupportValuesV20))
+		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support value is %q`, ec2.TpmSupportValuesV20))
 	}
 
 	if b.config.BootMode != "" {

diff --git a/builder/ebssurrogate/builder_test.go b/builder/ebssurrogate/builder_test.go
@@ -243,3 +243,64 @@ func TestBuilderPrepare_IMDSSupportValue(t *testing.T) {
 		})
 	}
 }
+
+func TestBuilderPrepare_TpmSupportValue(t *testing.T) {
+	tests := []struct {
+		name        string
+		optValue    string
+		expectError bool
+	}{
+		{
+			name:        "OK - no value set",
+			optValue:    "",
+			expectError: false,
+		},
+		{
+			name:        "OK - v2.0",
+			optValue:    "v2.0",
+			expectError: false,
+		},
+		{
+			name:        "Error - bad value set",
+			optValue:    "v3.0",
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			config := testConfig()
+			config["ami_name"] = "name"
+			config["ami_virtualization_type"] = "kvm"
+
+			config["tpm_support"] = tt.optValue
+
+			b := &Builder{}
+			// Basic configuration
+			b.config.RootDevice = RootBlockDevice{
+				SourceDeviceName: "device name",
+				DeviceName:       "device name",
+			}
+			b.config.LaunchMappings = BlockDevices{
+				BlockDevice{
+					BlockDevice: common.BlockDevice{
+						DeviceName: "device name",
+					},
+					OmitFromArtifact: false,
+				},
+			}
+
+			_, _, err := b.Prepare(config)
+			if err != nil && !tt.expectError {
+				t.Fatalf("got unexpected error: %s", err)
+			}
+			if err == nil && tt.expectError {
+				t.Fatalf("expected an error, got a success instead")
+			}
+
+			if err != nil {
+				t.Logf("OK: b.Prepare produced expected error: %s", err)
+			}
+		})
+	}
+}
diff --git a/builder/ebssurrogate/step_snapshot_volumes.go b/builder/ebssurrogate/step_snapshot_volumes.go
@@ -21,8 +21,7 @@ import (
 // StepSnapshotVolumes creates snapshots of the created volumes.
 //
 // Produces:
-//
-//	snapshot_ids map[string]string - IDs of the created snapshots
+//   snapshot_ids map[string]string - IDs of the created snapshots
 type StepSnapshotVolumes struct {
 	PollingConfig   *awscommon.AWSPollingConfig
 	LaunchDevices   []*ec2.BlockDeviceMapping

diff --git a/builder/instance/builder.go b/builder/instance/builder.go
@@ -91,18 +91,10 @@ type Config struct {
 	// okay to create this directory as part of the provisioning process.
 	// Defaults to /tmp.
 	X509UploadPath string `mapstructure:"x509_upload_path" required:"false"`
-<<<<<<< HEAD
-=======
-	// Enforce version of the Instance Metadata Service on the built AMI.
-	// Valid options are unset (legacy) and `v2.0`. See the documentation on
-	// [IMDS](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html)
-	// for more information. Defaults to legacy.
-	IMDSSupport string `mapstructure:"imds_support" required:"false"`
 	// NitroTPM Support. Valid options are `v2.0`. See the documentation on
 	// [NitroTPM Support](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-support-on-ami.html) for
 	// more information. Only enabled if a valid option is provided, otherwise ignored.
 	TpmSupport string `mapstructure:"tpm_support" required:"false"`
->>>>>>> 5bc11e8a (Added TPM as configuration option)
 
 	ctx interpolate.Context
 }
@@ -249,7 +241,7 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, []string, error) {
 	}
 
 	if b.config.TpmSupport != "" && b.config.TpmSupport != ec2.TpmSupportValuesV20 {
-		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support values are %q or the empty string`, ec2.TpmSupportValuesV20))
+		errs = packersdk.MultiErrorAppend(errs, fmt.Errorf(`The only valid tpm_support value is %q`, ec2.TpmSupportValuesV20))
 	}
 
 	if errs != nil && len(errs.Errors) > 0 {

diff --git a/builder/instance/builder_test.go b/builder/instance/builder_test.go
@@ -392,3 +392,51 @@ func TestBuilderPrepare_IMDSSupportValue(t *testing.T) {
 		})
 	}
 }
+
+func TestBuilderPrepare_TpmSupportValue(t *testing.T) {
+	tests := []struct {
+		name        string
+		optValue    string
+		expectError bool
+	}{
+		{
+			name:        "OK - no value set",
+			optValue:    "",
+			expectError: false,
+		},
+		{
+			name:        "OK - v2.0",
+			optValue:    "v2.0",
+			expectError: false,
+		},
+		{
+			name:        "Error - bad value set",
+			optValue:    "v3.0",
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			config, _ := testConfig()
+			config["ami_name"] = "name"
+			config["skip_region_validation"] = true
+
+			config["tpm_support"] = tt.optValue
+
+			b := &Builder{}
+
+			_, _, err := b.Prepare(config)
+			if err != nil && !tt.expectError {
+				t.Fatalf("got unexpected error: %s", err)
+			}
+			if err == nil && tt.expectError {
+				t.Fatalf("expected an error, got a success instead")
+			}
+
+			if err != nil {
+				t.Logf("OK: b.Prepare produced expected error: %s", err)
+			}
+		})
+	}
+}
diff --git a/docs-partials/builder/chroot/Config-not-required.mdx b/docs-partials/builder/chroot/Config-not-required.mdx
@@ -159,11 +159,11 @@
   more information. Defaults to `legacy-bios` when `ami_architecture` is `x86_64` and
   `uefi` when `ami_architecture` is `arm64`.
 
-- `tpm_support` (string) - The TPM Support mode. Valid options are `v2.0`. See the documentation on
-  [NitroTPM Support](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-support-on-ami.html) for
-  more information. Defaults to `v2.0`.
-
 - `uefi_data` (string) - Base64 representation of the non-volatile UEFI variable store. For more information
   see [AWS documentation](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/uefi-secure-boot-optionB.html).
 
+- `tpm_support` (string) - NitroTPM Support. Valid options are `v2.0`. See the documentation on
+  [NitroTPM Support](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-support-on-ami.html) for
+  more information. Only enabled if a valid option is provided, otherwise ignored.
+
 <!-- End of code generated from the comments of the Config struct in builder/chroot/builder.go; -->