Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tpm support #339

Closed
wants to merge 2 commits into from
Closed

Tpm support #339

wants to merge 2 commits into from

Conversation

ccravens
Copy link
Contributor

@ccravens ccravens commented Mar 6, 2023

Adding AWS NitroTPM support to AMI. More information can be found here:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-support-on-ami.html
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/enable-nitrotpm-prerequisites.html

There is a flag that is not currently supported by the Packer AMI Builder: --tpm-support v2.0

This update is meant to resolve that. I am not a packer plugin developer, but added what I believe to be the basic necessary code and tried my best to maintain the compatibility of the code based on the other options that are currently there. I want to see this feature as soon as possible so if there are updates that need to be made please let me know.

Both go build and make dev seem to work.

Testing results in the following output:

$ make test
go: downloading github.com/stretchr/testify v1.7.0
go: downloading gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
?   	github.com/hashicorp/packer-plugin-amazon	[no test files]
ok  	github.com/hashicorp/packer-plugin-amazon/builder/chroot	1.781s
ok  	github.com/hashicorp/packer-plugin-amazon/builder/common	0.520s
?   	github.com/hashicorp/packer-plugin-amazon/builder/common/awserrors	[no test files]
?   	github.com/hashicorp/packer-plugin-amazon/builder/common/ssm	[no test files]
ok  	github.com/hashicorp/packer-plugin-amazon/builder/ebs	5.617s
?   	github.com/hashicorp/packer-plugin-amazon/builder/ebs/acceptance	[no test files]
ok  	github.com/hashicorp/packer-plugin-amazon/builder/ebssurrogate	2.065s
ok  	github.com/hashicorp/packer-plugin-amazon/builder/ebsvolume	1.657s
ok  	github.com/hashicorp/packer-plugin-amazon/builder/instance	2.584s
ok  	github.com/hashicorp/packer-plugin-amazon/datasource/ami	1.382s
ok  	github.com/hashicorp/packer-plugin-amazon/datasource/parameterstore	1.011s
ok  	github.com/hashicorp/packer-plugin-amazon/datasource/secretsmanager	0.606s
?   	github.com/hashicorp/packer-plugin-amazon/post-processor/import	[no test files]
?   	github.com/hashicorp/packer-plugin-amazon/version	[no test files]

Closes #314

@ccravens ccravens requested a review from a team as a code owner March 6, 2023 16:16
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ccravens,

Thanks for the PR! The code overall looks good to me, I left a couple of nitpicks on the docs and the changes to the code documentation, while linting is appreciated, in this case since it is intertwined with the proposed enhancement, I think this is detrimental to the comprehension of the change, and therefore suggest that this is moved to a separate PR so we can independently take a look at it.

Aside from that, I see that the ebs builder is left out of this PR, I presume since it does not use the RegisterAMI API call to finalise the AMI creation. This is not the first time a feature is left out for this specific builder unfortunately, I think we should track this work independently, and refactor the logic for all builders so they're on an equal footing regarding this method to create AMIs.

If anything regarding testing, while this won't work locally completely, I would suggest adding an acceptance test to ensure the feature works as expected. We don't have anything for builders like chroot or instance, but we do have an acceptance test structure for ebssurrogate.
Since this involves creating resources (and using IAM roles that are permissive in the environment tests are run), I understand if this is not something you want to do, in this case let me know, and I will take some time to do this in the coming days.

Pre-approving the PR for a later merge, I will circle back on it when you have addressed my comments.

Thanks again for the contribution!

@@ -159,6 +159,10 @@
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc snippet looks good to me, for completeness I'd suggest adding this to all the builders that support the feature, so in addition to the chroot builder, the ebssurrogate and the instance builder docs should add this to their sections.

@@ -50,6 +50,7 @@ type Config struct {
Format string `mapstructure:"format"`
Architecture string `mapstructure:"architecture"`
BootMode string `mapstructure:"boot_mode"`
TpmSupport string `mapstructure:"tpm_support"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is added to the post-processor here, but I don't see the related step being updated, is this voluntary?

// encrypted = true
// kms_key_id = "1a2b3c4d-5e6f-1a2b-3c4d-5e6f1a2b3c4d"
// }
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I generally am not against using a linter for cleaning-up files, in this case, updating the documentation feels out of the scope of this PR, and makes the diff harder to read.
Could you please leave the doc changes out of this PR? If you think they are relevant, maybe open a separate PR for us to review, so this doesn't become a blocker for this one?

@alemuro alemuro mentioned this pull request May 11, 2023
@lbajolet-hashicorp
Copy link
Contributor

Superseded by #379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configuration to enable NitroTPM support when registering an AMI
3 participants