allow AWS-managed ssh key pairs to be disabled #392

cheeseplus · 2018-03-14T19:10:02Z

Fixes #391. When adding the new auto-generation feature of keys/security groups, the option to not pass any SSH key id at all was lost. This means that aws_ssh_key_id now has three states:

if nil we auto-create a key-pair
if "/path/to/key.pem" or ENV["AWS_SSH_KEY_ID"] is set, we use specified key
if "_disable", don't provide any key-pair at all - this requires the remote image/instance to have this configured out of band (baking, user_data)

TODO

Update the docs once the triple-option is settled upon.

robbkidd · 2018-03-14T19:31:43Z

spec/kitchen/driver/ec2_spec.rb

+    context "with key pair configured to false" do
+      before do
+        config[:aws_ssh_key_id] = false
+        expect(driver).to receive(:submit_server).and_return(server)


Maybe include a negative expectation for setting aws_ssh_key_id to false?

expect(driver).to_not receive(:create_key)

Looking at the spec, I found myself wondering what was different.

The key won't get created if you provide a path either so not sure checking this is any more meaningful but I do see what you are saying. Initially I'd added this to the instance_generator but that felt like more code that wasn't really doing much.

ghost · 2018-03-15T04:42:25Z

Just throwing this out there .... should ENV["AWS_SSH_KEY_ID"] also be allowed to be set to false.

cheeseplus · 2018-03-15T11:41:23Z

No, env vars have no concept of boolean - they are always strings and we're going to respect whatever a person sets there and fail if it's bad data.

cheeseplus · 2018-03-15T14:56:57Z

@pantocrator27 so we've figured out the fix (mostly) but bike shedding on the option, would you prefer:

aws_ssh_key_id: false

OR

aws_ssh_key_id: disable #perhaps a symbol even, :disable

We definitely want to keep auto-generation as the default when Nil and given this is a relatively niche use case you kinda get to pick what sounds best!

robbkidd · 2018-03-15T15:39:30Z

@pantocrator27 Updated this PR with what :disable might look like.

ghost · 2018-03-15T16:06:45Z

@cheeseplus @robbkidd Thanks for all of your work on this guys! It honestly feels to me that "disable" may sound better than "false". It semantically feels better to me if this makes sense because we are essentially disabling this feature ....

Thoughts?

cheeseplus · 2018-03-15T16:07:34Z

That was pretty much our thinking too but just wanted to shop it out since, as of now, you are the primary use case.

robbkidd · 2018-03-15T16:14:45Z

spec/kitchen/driver/ec2_spec.rb

-        expect(fake_file).to receive(:write).with("RSA PRIVATE KEY")
+      context "with no AWS-managed ssh key pair configured, creates a key pair to use" do
+        before do
+          config.delete(:aws_ssh_key_id)


🤔 This should probably be config[:aws_ssh_key_id] = nil to accurately reflect the default config where the key is present and its value is nil. config.delete(:aws_ssh_key_id) as setup is technically testing when the key is not present.

coderanger · 2018-03-15T16:15:09Z

Hmm, I am :-/ on using disable since we shouldn't do stuff that requires symbols (as opposed to strings) and magic string values are usually a thing that will bite people later.

robbkidd · 2018-03-15T16:24:03Z

@coderanger Do you have a suggestion for a better way to disable the current SET KEY NAME || GENERATE KEY behavior? @cheeseplus and I talked about Yet Another Config Option and didn't like the taste of that either.

we shouldn't do stuff that requires symbols (as opposed to strings)

The current code handles either with a config[:aws_ssh_key_id].to_sym == :disable tucked in there after the nil check.

relevant specs

Run options: include {:focus=>true, :locations=>{"./spec/kitchen/driver/ec2_spec.rb"=>[686]}}

Randomized with seed 62956

Kitchen::Driver::Ec2
  #create
    and AWS SSH keys
      with AWS-managed ssh key pair disabled, does not create a key pair or pass a key id
        (disable as a string)
          successfully creates and tags the instance
        (disable as a symbol)
          successfully creates and tags the instance
      with no AWS-managed ssh key pair configured, creates a key pair to use
        successfully creates and tags the instance
      with AWS ssh key pair set, uses set key and does not create a key pair
        successfully creates and tags the instance

Finished in 0.0321 seconds (files took 2.81 seconds to load)
4 examples, 0 failures

cheeseplus · 2018-03-15T16:27:28Z

I'm not super opposed to false as it fits in with everything else in the ecosystem even if semantically it's not great.

ghost · 2018-03-15T16:30:04Z

At this point ... I am not going to look a gift horse in the mouth :) or ask you guys to do something contrary to the set of current coding standards

ghost · 2018-03-15T16:30:31Z

As the customer, I will be equally pleased with either approach so long as it is documented. Please and thanks!

robbkidd · 2018-03-15T16:51:31Z

Alternate proposal: config[:aws_ssh_key_id] = ⛔️

ghost · 2018-03-15T16:59:47Z

@robbkidd can that be done :)

ghost · 2018-03-15T17:01:35Z

also I hate to ask this, could this make its way into the next release of ChefDK ... I don't know how that process would work?

cheeseplus · 2018-03-15T17:03:17Z

The deps in ChefDK just need to get bumped - the catch being that the ChefDK release is on it's own monthly schedule so if you wanted it sooner you'd have to consume a build from the current channel once the Gemfile.lock has been bumped.

ghost · 2018-03-15T17:07:00Z

thanks for the info @cheeseplus!

coderanger · 2018-03-15T17:16:20Z

I think I've convinced myself that a magic string is fine but what if we do _disabled instead just to make it clear it's a special value? And re: to_sym, better to do .to_s == 'whatever' since to_sym involves permanent allocs ;-)

robbkidd · 2018-03-15T17:31:33Z

Updated for "_disabled".

coderanger · 2018-03-15T17:38:13Z

Just needs some text in the readme to document it :) I can do that post merge if you're low on cycles since you already put in a 💯 effort <3

robbkidd · 2018-03-15T17:40:06Z

Regarding ...

should ENV["AWS_SSH_KEY_ID"] also be allowed to be set to [_disabled]?

As things currently stand in this PR, if the environment variable is set to "_disabled", that value will carry in and apply in the same way as if you had set aws_ssh_key_id: _disabled in kitchen.yml.

robbkidd · 2018-03-15T17:40:45Z

@coderanger I'm working on the README update right now! Thanks for the offer, though.

Both ec2_spec and ec2/image_selection_spec use this class, so let's promote it to a proper shared test class under spec/support. Signed-off-by: Robb Kidd <robb@thekidds.org>

If :aws_ssh_key_id is nil, continue to perform the key auto-generation. If :aws_ssh_key_id is explicitly set to "_disabled", do not generate a key. This means that the kitchen configuration must be more explicit about how connections will be authenticated. For example, when an environment has disallowed AWS-managed keys, a key could be named in the driver's `user_data` and the private key specified in the transport. * moved the gnarly setup/expectations for #create_key to a separate describe block so that the expectations around key auto-generation or enable/disable are more clear Co-authored-by: Robb Kidd <robb@thekidds.org> Co-authored-by: Seth Thomas <sthomas@chef.io> Signed-off-by: Robb Kidd <robb@thekidds.org> Signed-off-by: Seth Thomas <sthomas@chef.io>

coderanger

Intended to call out the three states `aws_ssh_key_id` can be in and put the words appropriate for each state next to it. Signed-off-by: Robb Kidd <robb@thekidds.org>

robbkidd · 2018-03-15T18:26:18Z

Did a quick rearrangement within the doc section. Does that make things better or worse?

ghost · 2018-03-15T18:34:08Z

I personally feel it makes sense

cheeseplus · 2018-03-15T18:41:53Z

We've got two thumbs and since I started the PR I can't approve -

cheeseplus added the Developing label Mar 14, 2018

robbkidd reviewed Mar 14, 2018

View reviewed changes

robbkidd force-pushed the fix-391 branch from d1d5f21 to 6ce07f5 Compare March 15, 2018 15:37

robbkidd changed the title ~~Allow aws_ssh_key_id to be False~~ allow AWS-managed ssh key pairs to be disabled Mar 15, 2018

robbkidd force-pushed the fix-391 branch from 6ce07f5 to 2711fbf Compare March 15, 2018 16:05

robbkidd reviewed Mar 15, 2018

View reviewed changes

robbkidd force-pushed the fix-391 branch from 2711fbf to 0e890b9 Compare March 15, 2018 16:29

robbkidd force-pushed the fix-391 branch from 0e890b9 to 754e816 Compare March 15, 2018 17:14

robbkidd force-pushed the fix-391 branch from 754e816 to efad67f Compare March 15, 2018 17:30

robbkidd and others added 2 commits March 15, 2018 14:03

move FakeImage test class into a support file

c51ac21

Both ec2_spec and ec2/image_selection_spec use this class, so let's promote it to a proper shared test class under spec/support. Signed-off-by: Robb Kidd <robb@thekidds.org>

robbkidd force-pushed the fix-391 branch from efad67f to efca69a Compare March 15, 2018 18:06

coderanger approved these changes Mar 15, 2018

View reviewed changes

rearrange the AWS SSH documentation

6e755c6

Intended to call out the three states `aws_ssh_key_id` can be in and put the words appropriate for each state next to it. Signed-off-by: Robb Kidd <robb@thekidds.org>

robbkidd approved these changes Mar 15, 2018

View reviewed changes

coderanger merged commit 4adaa2a into master Mar 15, 2018

fnichol removed the Developing label Mar 15, 2018

robbkidd deleted the fix-391 branch March 15, 2018 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow AWS-managed ssh key pairs to be disabled #392

allow AWS-managed ssh key pairs to be disabled #392

cheeseplus commented Mar 14, 2018 •

edited by robbkidd

Loading

robbkidd Mar 14, 2018

cheeseplus Mar 14, 2018 •

edited

Loading

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

cheeseplus commented Mar 15, 2018 •

edited

Loading

robbkidd commented Mar 15, 2018

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

robbkidd Mar 15, 2018

coderanger commented Mar 15, 2018

robbkidd commented Mar 15, 2018 •

edited

Loading

cheeseplus commented Mar 15, 2018

ghost commented Mar 15, 2018

ghost commented Mar 15, 2018

robbkidd commented Mar 15, 2018

ghost commented Mar 15, 2018

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

ghost commented Mar 15, 2018

coderanger commented Mar 15, 2018

robbkidd commented Mar 15, 2018

coderanger commented Mar 15, 2018

robbkidd commented Mar 15, 2018

robbkidd commented Mar 15, 2018

coderanger left a comment

robbkidd commented Mar 15, 2018

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

allow AWS-managed ssh key pairs to be disabled #392

allow AWS-managed ssh key pairs to be disabled #392

Conversation

cheeseplus commented Mar 14, 2018 • edited by robbkidd Loading

TODO

robbkidd Mar 14, 2018

Choose a reason for hiding this comment

cheeseplus Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

cheeseplus commented Mar 15, 2018 • edited Loading

robbkidd commented Mar 15, 2018

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

robbkidd Mar 15, 2018

Choose a reason for hiding this comment

coderanger commented Mar 15, 2018

robbkidd commented Mar 15, 2018 • edited Loading

cheeseplus commented Mar 15, 2018

ghost commented Mar 15, 2018

ghost commented Mar 15, 2018

robbkidd commented Mar 15, 2018

ghost commented Mar 15, 2018

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

ghost commented Mar 15, 2018

coderanger commented Mar 15, 2018

robbkidd commented Mar 15, 2018

coderanger commented Mar 15, 2018

robbkidd commented Mar 15, 2018

robbkidd commented Mar 15, 2018

coderanger left a comment

Choose a reason for hiding this comment

robbkidd commented Mar 15, 2018

ghost commented Mar 15, 2018

cheeseplus commented Mar 15, 2018

cheeseplus commented Mar 14, 2018 •

edited by robbkidd

Loading

cheeseplus Mar 14, 2018 •

edited

Loading

cheeseplus commented Mar 15, 2018 •

edited

Loading

robbkidd commented Mar 15, 2018 •

edited

Loading