search

hashicorp / packer-plugin-amazon

Packer plugin for Amazon AMI Builder
https://www.packer.io/docs/builders/amazon
Mozilla Public License 2.0
76 stars 112 forks source link

Skip uploading ssh pub key when AWS key pair is used #315

Closed Glyphack closed 1 year ago

Glyphack commented 1 year ago

Skip uploading SSH public key to the instance in case the AWS key pair is used.

In case key pair is generated and it's not specified by the user uploading the key is not necessary.

Since this issue can be identified with the roles running packer itself, I'm unsure if I can reflect this change in tests. Any help is appreciated. I tested it on my local machine with two configs:

Closes #313

lbajolet-hashicorp commented 1 year ago

Hi @Glyphack,

This took longer than expected since I tried to wrestle with roles for a while, and in the end settled to checking in the logs that we indeed don't attempt to upload an SSH key when none is specified in the configuration file.

I can confirm that the acceptance test I adapted does fail on main since the SSH key is being pushed, and with your commit, it does not anymore.

I'll see to have another pair of eyes take a look at the tests before merging this PR, and if/when I have an approval, I will trigger the merge.

Thanks again for the fix!

Glyphack commented 1 year ago

Thanks for your effort @lbajolet-hashicorp Do you think it makes sense to check that specific upload message at the acceptance test?

I was thinking about this but it's would be a very flakey test(easily breaks with just changing the message) pl that's why I decided to not do it.

But anyway testing this is really hard since the method has side effects that are hard to test.

lbajolet-hashicorp commented 1 year ago

Yeah unfortunately I attempted to use a profile which does not possess that permission as part of their role, but it took me way too long to understand that it still succeeded because the API call was using my session and not the SSM one, hence why I ultimately decided to settle on this approach here. I agree that it is inherently fragile, but for the moment it'll do, especially considering it's breaking users' workflow, so I would like to merge this in quickly, and follow-up with a release short after (tentatively tomorrow).

We can think of a better way to handle this test later I would think.