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

Include only AMI's owned by caller during pre-validate #354

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

cartermckinnon
Copy link
Contributor

@cartermckinnon cartermckinnon commented Apr 7, 2023

I maintain the EKS-optimized AMI, and as a result I build a lot of AMI's. 😄

Our current naming convention for AMI's uses a date of the form YYYYMMDD. For better or worse, that's what we do. We have internal processes that build AMI's using this naming convention, and subsequently grant permissions for those AMI's to various other AWS accounts.

The pre-validate step in the amazon-ebs builder will fail if any existing AMI returned by a ec2.DescribeImages call uses the proposed name. This makes sense, because the later ec2.CreateImage call will fail with a InvalidAMIName.Duplicate code if there is such a collision, after we've already executed the time-consuming steps.

But, this ec2.CreateImage error only occurs when the caller has an existing AMI with the same name, whereas the pre-validate step currently fails even when an existing AMI that the caller has launch permissions for uses the same name (including public AMI's).

This PR modifies the pre-validate step to only check for existing AMI's that are owned by the caller.

@cartermckinnon cartermckinnon requested a review from a team as a code owner April 7, 2023 00:37
@hashicorp-cla
Copy link

hashicorp-cla commented Apr 7, 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 @cartermckinnon,

Out of curiosity, have you tested the change on your environment?
This seems harmless, but I just want to make sure this works as intended before we merge it and release a new version of the plugin.

Outside of this check, this change looks good to me, I'm pre-approving this PR, and we can merge it once we've had confirmation.

Thanks again for the fix!

@cartermckinnon
Copy link
Contributor Author

cartermckinnon commented Apr 25, 2023

I've tested the following scenarios:

  1. An AWS account (A) builds an AMI named foo and gives another AWS account (B) launch permissions on that AMI. AWS account B attempts to use Packer to build an AMI named foo.
  2. An existing public AMI is named foo, and Packer attempts to build an AMI named foo.

Without this change, both scenarios fail on the pre-validation step, because ec2.DescribeImages returns results for AMI's named foo.

With this change, both scenarios succeed and an AMI named foo is able to be created by the caller.


I tested this by building this branch locally (make), then symlinking the resulting plugin into Packer's search path, like:

ln -s $PWD/packer-plugin-amazon ~/packer.d/plugins/packer-plugin-amazon-cartermckinnon

Then I used amazon-cartermckinnon-ebs as the builders[].type in my AMI template.

@lbajolet-hashicorp
Copy link
Contributor

Sounds good, thanks for the confirmation @cartermckinnon!

I'll merge this now.

@lbajolet-hashicorp lbajolet-hashicorp merged commit 49cb7b6 into hashicorp:main Apr 25, 2023
@cartermckinnon cartermckinnon deleted the ami-name-owners branch June 22, 2023 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants