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

ssh_user, ssh_identity_file, saved keypair filename should be set by cluster except when they shouldn't #96

Open
mrflip opened this issue Jan 17, 2012 · 6 comments
Labels

Comments

@mrflip
Copy link
Member

mrflip commented Jan 17, 2012

Here are several perfectly reasonable automated choices for the ssh name and identity file:

  • User: flip, ~/.ssh/id_rsa -- my laptop's private key for me, the user account I'd like to log in as. Only interesting once I've got full-fledged NFS and user accounts
  • robot EC2: ubuntu, credentials/ec2_keys/{cluster}.pem -- EC2 machines out of the box come with a passwordless-sudo user named 'ubuntu'. This is the one that must work, the one you'll use if you're just getting started, and the one that's the fallback if the machine doesn't fully boot.
  • robot Other: root, credentials/??/{cluster}.pem -- is I assume more typical off EC2 cloud, and perhaps of RHEL boxen on EC2
  • Robot custom: {any}, {function(cluster name)} -- use a standard cloud private key, but put the file where you'd like it.

As you can see, this isn't a matter of just configuration. I want to knife cluster ssh in as the robot account one moment, as normal user the next -- either way cluster chef should find the right identity file.

Also: right now, ClusterChef::Ec2Keypair and knife cluster ssh have no awareness of each others' predilections. That's dumb.

Proposed:

First, we have to figure out what key to go retrieve.

  • our notion of ssh-user is either
    • :robot: gleaned from the cloud's image_name
    • otherwise we follow the default knife ssh --ssh-user rules
  • I can tell cluster chef to use :robot by default
  • On the commandline, I can say --ssh-user=:robot and it will proceed accordingly, even if my knife.rb said differently. (That is, I don't lose the magic when I'm at the commandline).

Next we have to get the identity file:

  • can do it explicitly using command line, of course
  • If it is not the :robot ssh-user, we just use the same default knife ssh would.
  • otherwise there is a simple unified lookup for the keyfile location.
    • by default, "#{Config.robot_ssh_key_path}/#{cluster.name}.pem"
  • it should either
    • be possible to customize this lookup using arbitrary code,
    • or have it try a couple patterns (id_foo, foo.pem) and choose the first filename that exists

... I gotta think for a little to figure out who owns choosing the filename: Cloud? PrivateKey? Server?

Changes:

  • Even if it is a concern of PrivateKey, this is no longer a concern of Ec2Keypair, or ec2-specific in any way -- an ec2 keypair is a special case of a robot (machine-or-cluster-specific) ssh key.
  • as such, the ec2_key_dir config variable becomes robot_ssh_key_path, and the default credentials ec2_keys directory is renamed to robot_ssh_keys,

Meanwhile there's also the question of customizing that lookup algorirthm. It's easy to say "o hey just specify Config[:robot_ssh_key_path] = lambda{|foo,bar| ... } and we will recognize it as a block and call it instead of using it as a string" but that leads to conditional logic in the wrong places and feels like a misallocation of magic.

I'd sooner just let you monkeypatch it, but have to figure out how to have that code lazily loaded (and not just lazily, but after cluster_chef has been sourced).

     # ... in cluster chef somewhere, the standard:
     def robot_ssh_identity_filename
       File.join(key_dir, "#{key_name}.pem")
     end

     # ... in a file of yours somewhere, a monkeypatch:
     def robot_ssh_identity_filename
       [ "#{key_name}_rsa", "id_#{key_name}", "#{key_name}.pem" ].
         map   {|fn| File.join(ENV['HOME'], '.ssh', fn) }.
         detect{|fn| File.exists?(fn) }
     end
@mrflip
Copy link
Member Author

mrflip commented Jan 17, 2012

... we should also fix the naming to precisely refer to the ssh username, an ssh identity file, and the amazon keypair object; that naming should if possible agree with what knife ssh chose.

@temujin9
Copy link
Contributor

See also PCN's pull request re: ssh keypair naming for additional needs in this arena.

@pcn
Copy link

pcn commented Feb 23, 2012

Is there any particular reason that the patch I submitted can't be included for now, and then support for the same conventions added via this request until your fix materializes?? Let me know if I should be signing off on a contributor's agreement or something. I'm hoping to be able to not have to maintain my fork for our internal use, but this doesn't seem to be the direction that things are going in at the moment.

@temujin9
Copy link
Contributor

We're reticent for a couple of reasons:

  1. It's not the API contract we'd be going with long term, and publishing and retracting a different one feels impolite to our larger audience.
  2. It's a lot of conditional logic (if statements), which we try to avoid internally. They can produce dormant code paths, which harbor bugs for longer than heavily used code.

I'm specing out something easy that allows both our requirements without monkey-patching, which I'll post once I've had a chance to double-check some assumptions. If you felt like taking a stab at the specs before one of the chimps finds time to, I'd definitely make working the results back in a priority.

@temujin9
Copy link
Contributor

So the idea is to rewrite ssh_identity_file to work by unshifting code blocks from a copy of settings[:ssh_identity_search], until it hits one that returns a valid filename, or empties the stack and errors out. To add your search targets, you push one or more blocks onto the queue from one of your knife.rb children (probably knife-org.rb); push the default behavior as a blocks at the head of the ssh_identity_file call, so they execute last. With that, we can add the default behaviors we want, without interrupting user's custom behaviors or adding a lot of conditional code.

Does that sound workable to everyone?

@pcn
Copy link

pcn commented Feb 26, 2012

So there'd be a default, and I could specify, e.g.

knife['ssh_identity_search'] = { return knife[:config][:name_of_id_file_I_want] }

if that's the case, then that sounds great.

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

No branches or pull requests

3 participants