Store the first SSH private key in generated Ansible inventory #5765

Merged
merged 5 commits into from Jul 10, 2015

Conversation

Projects
None yet
3 participants
@gildegoma
Collaborator

gildegoma commented May 29, 2015

Pending before merge:

  • Confirmation from @mitchellh or @sethvargo that we add this feature, even if "Ansible Parallelism" is not officially supported (but so awesome and popular ;-)
  • Generated-Inventory QA Feedbacks (thanks to @conorsch)
  • Static-Inventory QA Feedbacks
  • Documentation Updates (df4b74e and 247fe72)
@gildegoma

This comment has been minimized.

Show comment
Hide comment
@gildegoma

gildegoma Jun 1, 2015

Collaborator

(force rebased after f7894d9, docs still missing ;-)

Collaborator

gildegoma commented Jun 1, 2015

(force rebased after f7894d9, docs still missing ;-)

@gildegoma gildegoma referenced this pull request Jun 1, 2015

Closed

[WIP] Add new provisioner: Ansible on the Guest VM #5340

0 of 9 tasks complete
@conorsch

This comment has been minimized.

Show comment
Hide comment
@conorsch

conorsch Jun 11, 2015

The changes included in this PR work great. Using the Ansible provisioner with a multi-machine setup now causes the default insecure key to be removed. Here's the multi-machine Vagrantfile:

Vagrant.configure(2) do |config|
  config.vm.box = "trusty64"
  config.ssh.insert_key = false

  config.vm.define :thing1 do |c|
  end

  config.vm.define :thing2 do |c|
    c.vm.provision :ansible do |ansible|
      ansible.playbook = "playbook.yml"
      ansible.limit = "all"
    end
  end
end

I've tested the following scenarios:

Running vagrant v1.7.2 unpatched, key insertion enabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: FAILS (on 'provision' step)

Running vagrant v1.7.2 unpatched, key insertion disabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: OK

Running patched vagrant, key insertion enabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: OK

Running patched vagrant, key insertion disabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: OK

One thing I haven't done is test with static inventory files. If someone's able to chime in there, that'd be a big help!

The changes included in this PR work great. Using the Ansible provisioner with a multi-machine setup now causes the default insecure key to be removed. Here's the multi-machine Vagrantfile:

Vagrant.configure(2) do |config|
  config.vm.box = "trusty64"
  config.ssh.insert_key = false

  config.vm.define :thing1 do |c|
  end

  config.vm.define :thing2 do |c|
    c.vm.provision :ansible do |ansible|
      ansible.playbook = "playbook.yml"
      ansible.limit = "all"
    end
  end
end

I've tested the following scenarios:

Running vagrant v1.7.2 unpatched, key insertion enabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: FAILS (on 'provision' step)

Running vagrant v1.7.2 unpatched, key insertion disabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: OK

Running patched vagrant, key insertion enabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: OK

Running patched vagrant, key insertion disabled

  • single trusty64 box: init, up, reload, ssh: OK
  • two trusty64 boxes: init, up, reload, provision, ssh: OK

One thing I haven't done is test with static inventory files. If someone's able to chime in there, that'd be a big help!

@conorsch

This comment has been minimized.

Show comment
Hide comment
@conorsch

conorsch Jul 7, 2015

Anything I can do to help here? I don't see this feature listed for inclusion in the pending changelog for 1.7.3, and would very much like to stop using the insecure default key with my ansible projects.

conorsch commented Jul 7, 2015

Anything I can do to help here? I don't see this feature listed for inclusion in the pending changelog for 1.7.3, and would very much like to stop using the insecure default key with my ansible projects.

gildegoma added a commit that referenced this pull request Jul 7, 2015

@gildegoma

This comment has been minimized.

Show comment
Hide comment
@gildegoma

gildegoma Jul 7, 2015

Collaborator

@conorsch thanks a lot for your help on testing.

@mitchellh @sethvargo I'd ❤️ to get your feedback on this change request. It is related to the Ansible Parallel Execution Trick explained in the "Tips and Tricks" section of the Ansible provisioner docs and discussed at considerable length on #1784 thread. It seams that many people use it and were impacted by the introduction of the "insecure key replacement" by default in Vagrant 1.7.

With this change, it is no longer needed to set config.ssh.insert_key = false.
But since Ansible natively only handles a single private key per host, we still have to rely on OpenSSH parameters to pass additional private keys. For backwards compatibility reasons, we'll also have to pass the private keys via OpenSSH parameters when a static inventory is specified in the Vagrantfile.

I also added a "wip" version of the related documentation updates, so you can better figure out the impact of these changes.

I think that these code additions are still "not too complex/tricky" and are worth to be added for Vagrant 1.7.3, but I don't want to merge it without core team approval.

Collaborator

gildegoma commented Jul 7, 2015

@conorsch thanks a lot for your help on testing.

@mitchellh @sethvargo I'd ❤️ to get your feedback on this change request. It is related to the Ansible Parallel Execution Trick explained in the "Tips and Tricks" section of the Ansible provisioner docs and discussed at considerable length on #1784 thread. It seams that many people use it and were impacted by the introduction of the "insecure key replacement" by default in Vagrant 1.7.

With this change, it is no longer needed to set config.ssh.insert_key = false.
But since Ansible natively only handles a single private key per host, we still have to rely on OpenSSH parameters to pass additional private keys. For backwards compatibility reasons, we'll also have to pass the private keys via OpenSSH parameters when a static inventory is specified in the Vagrantfile.

I also added a "wip" version of the related documentation updates, so you can better figure out the impact of these changes.

I think that these code additions are still "not too complex/tricky" and are worth to be added for Vagrant 1.7.3, but I don't want to merge it without core team approval.

gildegoma added a commit that referenced this pull request Jul 8, 2015

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Jul 9, 2015

Member

LGTM so far @gildegoma, let me know when its ready!

Member

mitchellh commented Jul 9, 2015

LGTM so far @gildegoma, let me know when its ready!

@gildegoma

This comment has been minimized.

Show comment
Hide comment
@gildegoma

gildegoma Jul 9, 2015

Collaborator

@mitchellh excellent! Tomorrow, I'll do some more tests to verify again the backwards compatibility with static inventories and finalize the doc changes.

/cc @conorsch @lpabon @maspwr (if you have time to look at it)

Collaborator

gildegoma commented Jul 9, 2015

@mitchellh excellent! Tomorrow, I'll do some more tests to verify again the backwards compatibility with static inventories and finalize the doc changes.

/cc @conorsch @lpabon @maspwr (if you have time to look at it)

@conorsch

This comment has been minimized.

Show comment
Hide comment
@conorsch

conorsch Jul 9, 2015

@mitchellh calloo callay! @gildegoma I don't use static inventories much with Vagrant, but will generate a test case tonight and run it through the paces, just to make sure we've got our bases covered. The documentation updates look great.

conorsch commented Jul 9, 2015

@mitchellh calloo callay! @gildegoma I don't use static inventories much with Vagrant, but will generate a test case tonight and run it through the paces, just to make sure we've got our bases covered. The documentation updates look great.

@conorsch

This comment has been minimized.

Show comment
Hide comment
@conorsch

conorsch Jul 10, 2015

Using a static inventory file for Ansible and enabling dynamic key insertion as proposed in this PR causes the Ansible provisioner to fail, unless the new keys are added to the static inventory file. Running the Ansible provision with insert_key = false declared avoids the issue, as expected. Vagrant-only commands such as vagrant reload and vagrant ssh work both with and without key insertion enabled, as expected.

Overall this behavior is consistent with what the documentation states, but additional clarification would be helpful. In particular, this section of website/docs/source/v2/provisioning/ansible.html.md:

The SSH host addresses (and ports) must obviously be specified twice

should explicitly list the relevant settings, like so:

The SSH settings (host addresses, ports, and private key paths) must obviously be specified twice

If you allow Vagrant to swap out the key for hosts managed by a static inventory file, you just need to tack on the ansible_ssh_private_key_file=.vagrant/machines/MACHINE_NAME/virtualbox/private_key and it works perfectly.

Consider implementing the small edit to the docs, for newcomers. Otherwise, we're good to go! 👍

Using a static inventory file for Ansible and enabling dynamic key insertion as proposed in this PR causes the Ansible provisioner to fail, unless the new keys are added to the static inventory file. Running the Ansible provision with insert_key = false declared avoids the issue, as expected. Vagrant-only commands such as vagrant reload and vagrant ssh work both with and without key insertion enabled, as expected.

Overall this behavior is consistent with what the documentation states, but additional clarification would be helpful. In particular, this section of website/docs/source/v2/provisioning/ansible.html.md:

The SSH host addresses (and ports) must obviously be specified twice

should explicitly list the relevant settings, like so:

The SSH settings (host addresses, ports, and private key paths) must obviously be specified twice

If you allow Vagrant to swap out the key for hosts managed by a static inventory file, you just need to tack on the ansible_ssh_private_key_file=.vagrant/machines/MACHINE_NAME/virtualbox/private_key and it works perfectly.

Consider implementing the small edit to the docs, for newcomers. Otherwise, we're good to go! 👍

@gildegoma

This comment has been minimized.

Show comment
Hide comment
@gildegoma

gildegoma Jul 10, 2015

Collaborator

Thanks again @conorsch for your contributions!
Just to clarify, in "normal" (sequential) mode, there is no need to specifiy the private key (that's the backwards compatibility critical point). Only users that want to make parallel provisioning (running the Ansible provisioner only once for the last VM booted), will have to be careful if they use their own static repository.

As an illustration for the records, let's take following static inventory:

machine1 ansible_ssh_host=192.168.77.21
machine2 ansible_ssh_host=192.168.77.22
machine3 ansible_ssh_host=192.168.77.23

Everything runs perfectly with following sequential Vagrantfile:

Vagrant.configure(2) do |config|

  config.vm.box = "ubuntu/trusty64"

  N = 3
  (1..N).each do |machine_id|
    config.vm.define "machine#{machine_id}" do |machine|
      machine.vm.hostname = "machine#{machine_id}"
      machine.vm.network "private_network", ip: "192.168.77.#{20+machine_id}"
      machine.vm.provision :ansible do |ansible|
        ansible.playbook = "example.yml"
        ansible.inventory_path = "static_inventory"
      end
    end
  end
end

And for the Ansible Parallelization Trick, the following approach works fine without modifying the static inventory:

  N = 3

  VAGRANT_VM_PROVIDER = "virtualbox"
  ANSIBLE_RAW_SSH_ARGS = []

  (1..N-1).each do |machine_id|
    ANSIBLE_RAW_SSH_ARGS << "-o IdentityFile=#{ENV["VAGRANT_DOTFILE_PATH"]}/machines/machine#{machine_id}/#{VAGRANT_VM_PROVIDER}/private_key"
  end

  (1..N).each do |machine_id|
    config.vm.define "machine#{machine_id}" do |machine|
      machine.vm.hostname = "machine#{machine_id}"
      machine.vm.network "private_network", ip: "192.168.77.#{20+machine_id}"
      if machine_id == N
        machine.vm.provision :ansible do |ansible|
          ansible.playbook = "example.yml"
          ansible.limit = 'all'
          ansible.inventory_path = "static_inventory"
          ansible.raw_ssh_args = ANSIBLE_RAW_SSH_ARGS
        end
      end

    end
  end

end
Collaborator

gildegoma commented Jul 10, 2015

Thanks again @conorsch for your contributions!
Just to clarify, in "normal" (sequential) mode, there is no need to specifiy the private key (that's the backwards compatibility critical point). Only users that want to make parallel provisioning (running the Ansible provisioner only once for the last VM booted), will have to be careful if they use their own static repository.

As an illustration for the records, let's take following static inventory:

machine1 ansible_ssh_host=192.168.77.21
machine2 ansible_ssh_host=192.168.77.22
machine3 ansible_ssh_host=192.168.77.23

Everything runs perfectly with following sequential Vagrantfile:

Vagrant.configure(2) do |config|

  config.vm.box = "ubuntu/trusty64"

  N = 3
  (1..N).each do |machine_id|
    config.vm.define "machine#{machine_id}" do |machine|
      machine.vm.hostname = "machine#{machine_id}"
      machine.vm.network "private_network", ip: "192.168.77.#{20+machine_id}"
      machine.vm.provision :ansible do |ansible|
        ansible.playbook = "example.yml"
        ansible.inventory_path = "static_inventory"
      end
    end
  end
end

And for the Ansible Parallelization Trick, the following approach works fine without modifying the static inventory:

  N = 3

  VAGRANT_VM_PROVIDER = "virtualbox"
  ANSIBLE_RAW_SSH_ARGS = []

  (1..N-1).each do |machine_id|
    ANSIBLE_RAW_SSH_ARGS << "-o IdentityFile=#{ENV["VAGRANT_DOTFILE_PATH"]}/machines/machine#{machine_id}/#{VAGRANT_VM_PROVIDER}/private_key"
  end

  (1..N).each do |machine_id|
    config.vm.define "machine#{machine_id}" do |machine|
      machine.vm.hostname = "machine#{machine_id}"
      machine.vm.network "private_network", ip: "192.168.77.#{20+machine_id}"
      if machine_id == N
        machine.vm.provision :ansible do |ansible|
          ansible.playbook = "example.yml"
          ansible.limit = 'all'
          ansible.inventory_path = "static_inventory"
          ansible.raw_ssh_args = ANSIBLE_RAW_SSH_ARGS
        end
      end

    end
  end

end
@gildegoma

This comment has been minimized.

Show comment
Hide comment
@gildegoma

gildegoma Jul 10, 2015

Collaborator

@mitchellh I'll rebase it and merge right now to be sure to get it into 1.7.3 and will push the final documentation additions asap (@conorsch you're more than welcome to verify my english
intelligibility ;-)

Collaborator

gildegoma commented Jul 10, 2015

@mitchellh I'll rebase it and merge right now to be sure to get it into 1.7.3 and will push the final documentation additions asap (@conorsch you're more than welcome to verify my english
intelligibility ;-)

Luis Pabón and others added some commits Dec 21, 2014

provisioners/ansible: Put ssh key in the inventory
Vagrant 1.7.1 creates and injects new ssh keys for each virtual machine.
When it started ansible with the "parallel provisioning trick",
it would only send the ssh key of the targeted virtual machine.
With this change, vagrant now stores the ssh key for each virtual
machines directly in the generated ansible inventory, and thus allow
ansible parallelism.

Note that this change is not sufficient, as it would break vagrant
configuration based on a custom inventory (file or script). This issue
will be addressed in a next commit.

Signed-off-by: Luis Pabón <lpabon@redhat.com>
provisioners/ansible: Update unit tests
Signed-off-by: Luis Pabón <lpabon@redhat.com>
provisioners/ansible: provide ssh identities via ANSIBLE_SSH_ARGS (wh…
…en necessary)

When provisioning multiple machines in sequence (the default vagrant
behaviour), it doesn't make sense to require to provide the private ssh
key(s) via the custom ansible inventory script/file.

To align with the handling of multiple ssh keys per machine, we won't
rely any longer on `--private-key` command line argument, but only pass
the keys via `ANSIBLE_SSH_ARGS` environment variable.

Note that when vagrant generates the ansible inventory and that only one
key is associated to a VM, this step would be redundant, and therefore
won't be applied.

This change fixes the breaking change introduced by 3d62a91.
@gildegoma

This comment has been minimized.

Show comment
Hide comment
@gildegoma

gildegoma Jul 10, 2015

Collaborator

Gotcha, my 4969a75 workaround is crappy, as 3 other unit tests (outside of Ansible provisioner scope) are failing now: https://travis-ci.org/mitchellh/vagrant/builds/70339230#L206-L217

At least it demonstrate that nothing is missing in the Ansible provisioner parts, so I merge (no time to tackle the RSpec regression now).

Collaborator

gildegoma commented Jul 10, 2015

Gotcha, my 4969a75 workaround is crappy, as 3 other unit tests (outside of Ansible provisioner scope) are failing now: https://travis-ci.org/mitchellh/vagrant/builds/70339230#L206-L217

At least it demonstrate that nothing is missing in the Ansible provisioner parts, so I merge (no time to tackle the RSpec regression now).

gildegoma added a commit that referenced this pull request Jul 10, 2015

Merge pull request #5765 from mitchellh/gildegoma/key-in-ansible-inve…
…ntory

Store the first SSH private key in generated Ansible inventory

@gildegoma gildegoma merged commit 8f95129 into master Jul 10, 2015

@gildegoma gildegoma deleted the gildegoma/key-in-ansible-inventory branch Jul 10, 2015

gildegoma added a commit that referenced this pull request Jul 10, 2015

@sergii-tkachenko sergii-tkachenko referenced this pull request in DoSomething/tower Oct 20, 2015

Open

Improve Vagrant login #168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment