From 1b686906a94554bed9e294f227880d85abcb18ab Mon Sep 17 00:00:00 2001 From: alemuro Date: Thu, 11 May 2023 18:28:21 +0200 Subject: [PATCH] Address PR comments --- builder/chroot/builder.go | 2 +- builder/chroot/builder.hcl2spec.go | 2 +- builder/chroot/step_attach_volume.go | 5 +- builder/chroot/step_create_volume.go | 3 +- builder/chroot/step_flock.go | 3 +- builder/chroot/step_mount_device.go | 5 +- builder/chroot/step_snapshot.go | 3 +- builder/common/access_config.go | 34 +++++------ builder/common/awserrors/utils.go | 6 +- builder/common/block_device.go | 25 ++++---- builder/common/step_pre_validate_test.go | 2 +- builder/common/step_source_ami_info.go | 3 +- builder/ebssurrogate/builder.go | 10 +-- builder/ebssurrogate/builder_test.go | 61 +++++++++++++++++++ builder/ebssurrogate/step_snapshot_volumes.go | 3 +- builder/instance/builder.go | 10 +-- builder/instance/builder_test.go | 48 +++++++++++++++ .../builder/chroot/Config-not-required.mdx | 8 +-- .../ebssurrogate/Config-not-required.mdx | 4 ++ .../builder/instance/Config-not-required.mdx | 4 ++ post-processor/import/post-processor.go | 6 -- .../import/post-processor.hcl2spec.go | 1 - 22 files changed, 164 insertions(+), 84 deletions(-) diff --git a/builder/chroot/builder.go b/builder/chroot/builder.go index e7b1bd316..ba8150eae 100644 --- 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 index 2792dd809..e4d9139d1 100644 --- a/builder/chroot/builder.hcl2spec.go +++ b/builder/chroot/builder.hcl2spec.go @@ -85,7 +85,7 @@ type FlatConfig struct { Architecture *string `mapstructure:"ami_architecture" required:"false" cty:"ami_architecture" hcl:"ami_architecture"` BootMode *string `mapstructure:"boot_mode" required:"false" cty:"boot_mode" hcl:"boot_mode"` UefiData *string `mapstructure:"uefi_data" required:"false" cty:"uefi_data" hcl:"uefi_data"` - TPMSupport *string `mapstructure:"tpm_support" required:"false" cty:"tpm_support" hcl:"tpm_support"` + TpmSupport *string `mapstructure:"tpm_support" required:"false" cty:"tpm_support" hcl:"tpm_support"` } // FlatMapstructure returns a new FlatConfig. diff --git a/builder/chroot/step_attach_volume.go b/builder/chroot/step_attach_volume.go index 0e4c47fa0..20c9587ef 100644 --- 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 index 432e47280..ba50e19af 100644 --- 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 index fc5feb8be..67d337220 100644 --- 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 index f38580267..223269c9a 100644 --- 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 index f997cb49c..038a04f0a 100644 --- 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 index 100aaa20d..47f341955 100644 --- 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 index 8e2705bdc..fad85d88e 100644 --- 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 index c82888a25..c58ab43ef 100644 --- 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 index e29d6baad..f6e9a9b4e 100644 --- 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 index 3f2a6adba..1a9870811 100644 --- 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 index b3fdb9600..0afc6fb86 100644 --- 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 index c3971e65c..83910f337 100644 --- 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 index e4239c12c..1f907043e 100644 --- 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 index a56d94504..8c93b9039 100644 --- 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 index 705d644b5..e0b7888bf 100644 --- 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 index 615c7908f..195e62800 100644 --- 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. + diff --git a/docs-partials/builder/ebssurrogate/Config-not-required.mdx b/docs-partials/builder/ebssurrogate/Config-not-required.mdx index 059750a99..c0c8187b2 100644 --- a/docs-partials/builder/ebssurrogate/Config-not-required.mdx +++ b/docs-partials/builder/ebssurrogate/Config-not-required.mdx @@ -38,4 +38,8 @@ - `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. + diff --git a/docs-partials/builder/instance/Config-not-required.mdx b/docs-partials/builder/instance/Config-not-required.mdx index 235ea58ec..a5a89eddf 100644 --- a/docs-partials/builder/instance/Config-not-required.mdx +++ b/docs-partials/builder/instance/Config-not-required.mdx @@ -37,4 +37,8 @@ okay to create this directory as part of the provisioning process. Defaults to /tmp. +- `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. + diff --git a/post-processor/import/post-processor.go b/post-processor/import/post-processor.go index 76bbbf799..e8e3219f4 100644 --- a/post-processor/import/post-processor.go +++ b/post-processor/import/post-processor.go @@ -53,7 +53,6 @@ type Config struct { Format string `mapstructure:"format"` Architecture string `mapstructure:"architecture"` BootMode string `mapstructure:"boot_mode"` - TpmSupport string `mapstructure:"tpm_support"` ctx interpolate.Context } @@ -147,11 +146,6 @@ func (p *PostProcessor) Configure(raws ...interface{}) error { errs, fmt.Errorf("invalid boot mode '%s'. Only 'uefi' and 'legacy-bios' are allowed", p.config.BootMode)) } - if p.config.TpmSupport != "v2.0" { - errs = packersdk.MultiErrorAppend( - errs, fmt.Errorf("invalid tpm support value '%s'. Only 'v2.0' is allowed", p.config.TpmSupport)) - } - if p.config.Architecture == "arm64" && p.config.BootMode != "uefi" { errs = packersdk.MultiErrorAppend( errs, fmt.Errorf("invalid boot mode '%s' for 'arm64' architecture", p.config.BootMode)) diff --git a/post-processor/import/post-processor.hcl2spec.go b/post-processor/import/post-processor.hcl2spec.go index 18c8fd564..943b1d8be 100644 --- a/post-processor/import/post-processor.hcl2spec.go +++ b/post-processor/import/post-processor.hcl2spec.go @@ -111,7 +111,6 @@ func (*FlatConfig) HCL2Spec() map[string]hcldec.Spec { "format": &hcldec.AttrSpec{Name: "format", Type: cty.String, Required: false}, "architecture": &hcldec.AttrSpec{Name: "architecture", Type: cty.String, Required: false}, "boot_mode": &hcldec.AttrSpec{Name: "boot_mode", Type: cty.String, Required: false}, - "tpm_support": &hcldec.AttrSpec{Name: "tpm_support", Type: cty.String, Required: false}, } return s }