Set ansible_ssh_pass with ansible_password because of a testinfra issue

[lmesz-bitrise]

We are using delegated driver to connect to a recently created host on which ssh keys are not set yet, when we had tried to execute molecule test on that it stopped and asked for password in the verify phase even though the password was set in the create.yml.
For verification we use testinfra, under the hood it uses pytest with ansible backend.
After some investigation figured out the reason is that ansible_ssh_pass is not set and based on the doc it is mandatory if key is not set. This issue has been reported to testinfra, but until it is not fixed this PR implements a small workaround.

[ssbarnea]

Can you please provide some links that clarify the difference between ansible_ssh_pass and ansible_password.

If we fix it we better know what we are doing, maybe even including a clarifying comment in the source code.

• Do we need both?
• Do we have a case where one may be different than another?
• Are you aware that you can mention inventory variables directly in molecule file, bypassing any need to process them with molecule.

[ghost]

I can't find ansible_ssh_pass in the newer docs, but I believe it's the old version of ansible_password. I did find ansible_ssh_pass in the docs for version 2.3 and it seems to be a SSH specific password variable.

[greg-hellings]

I believe that ansible_ssh_pass is deprecated in favor of ansible_password, since there are alternatively connection types that might be supported.

[lmesz-bitrise]

Can you please provide some links that clarify the difference between ansible_ssh_pass and ansible_password.

If we fix it we better know what we are doing, maybe even including a clarifying comment in the source code.

• Do we need both?

If we only use that inventory for pytest than we don't need ansible_password ansible_ssh_pass is enough

• Do we have a case where one may be different than another?

Unfortunately, I don't know but if it is only for pytest then no.

• Are you aware that you can mention inventory variables directly in molecule file, bypassing any need to process them with molecule.

Ohh I wasn't aware of it, will check it. How can I do this?

Sorry I was not clear with the change. The parameter is not for ansible but for testinfra verifier. For test it uses pytest and pytest to establish the connection uses ansible backend. The documentation can be found here
In case of password and not key we need to use that variable since the connection for ansible is hardcoded here https://github.com/ansible-community/molecule/blob/master/lib/molecule/driver/base.py#L75

[ssbarnea]

Can you please provide some links that clarify the difference between ansible_ssh_pass and ansible_password.

If we fix it we better know what we are doing, maybe even including a clarifying comment in the source code.

• Do we need both?

If we only use that inventory for pytest than we don't need ansible_password ansible_ssh_pass is enough

• Do we have a case where one may be different than another?

Unfortunately, I don't know but if it is only for pytest then no.

• Are you aware that you can mention inventory variables directly in molecule file, bypassing any need to process them with molecule.

Ohh I wasn't aware of it, will check it. How can I do this?

Sorry I was not clear with the change. The parameter is not for ansible but for testinfra verifier. For test it uses pytest and pytest to establish the connection uses ansible backend. The documentation can be found here
In case of password and not key we need to use that variable since the connection for ansible is hardcoded here https://github.com/ansible-community/molecule/blob/master/lib/molecule/driver/base.py#L75

When in doubt, try to ask on #ansible-devel, other devs are more than willing to help with questions like this. Give them link to this PR. I suspect is an old version of the one you tried and in this case the correct fix is different.

[lmesz-bitrise]

Can you please provide some links that clarify the difference between ansible_ssh_pass and ansible_password.

If we fix it we better know what we are doing, maybe even including a clarifying comment in the source code.

• Do we need both?

If we only use that inventory for pytest than we don't need ansible_password ansible_ssh_pass is enough

• Do we have a case where one may be different than another?

Unfortunately, I don't know but if it is only for pytest then no.

• Are you aware that you can mention inventory variables directly in molecule file, bypassing any need to process them with molecule.

Ohh I wasn't aware of it, will check it. How can I do this?
Sorry I was not clear with the change. The parameter is not for ansible but for testinfra verifier. For test it uses pytest and pytest to establish the connection uses ansible backend. The documentation can be found here
In case of password and not key we need to use that variable since the connection for ansible is hardcoded here https://github.com/ansible-community/molecule/blob/master/lib/molecule/driver/base.py#L75

When in doubt, try to ask on #ansible-devel, other devs are more than willing to help with questions like this. Give them link to this PR. I suspect is an old version of the one you tried and in this case the correct fix is different.

Alright trying to connect. Old version of which component?

ansible 2.10.1
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/Users/lmeszaros/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.8/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.8.4 (default, Jul 14 2020, 02:58:48) [Clang 11.0.3 (clang-1103.0.32.62)]
molecule 3.2.0a0
    ansible:2.10.1 python:3.8
    delegated:3.2.0a0 from molecule

I mean I doubt a little cause the documentation exactly says that to use that variable.
If ssh identity file is not provided via –ssh-identity-file flag, testinfra will try to use ansible_ssh_private_key_file, ansible_private_key_file and, finally, ansible_user with ansible_ssh_pass variables, both should be specified.

[ssbarnea]

@tadeboro Gave a very useful link: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/ssh_connection.html#parameter-password -- which indicates that there are 3 variables around password, not two.

I suspect that the bug may be a testinfra related bug? I see you already ruled out an outdated Ansible issue.

[lmesz-bitrise]

@tadeboro Gave a very useful link: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/ssh_connection.html#parameter-password -- which indicates that there are 3 variables around password, not two.

Do you suggest setting all three variables?

I suspect that the bug may be a testinfra related bug? I see you already ruled out an outdated Ansible issue.

Sorry I don't understand that part. What kind of bug?

[tadeboro]

As far as Ansible is concerned, setting ansible_password or ansible_ssh_pass is exactly the same thing. So I guess something strange happens during the verification step. Unfortunatelly, there is no output attached to this PR, so it is hard to know what exactly went wrong.

[lmesz-bitrise]

Nothing strange happened. Based on testinfra code it is intended.
As I posted earlier from the doc it says ...and, finally, ansible_user with ansible_ssh_pass variables, both should be specified.
I took a look and it is because they explicitly grab that parameter relevant code

[lmesz-bitrise]

@ssbarnea you were right, your suggestion also works:

---
dependency:
  name: galaxy
driver:
  name: delegated
platforms:
  - name: instance
provisioner:
  name: ansible
  lint:
    name: ansible-lint
  inventory:
    host_vars:
      instance:
        ansible_ssh_pass: vagrant
verifier:
  name: testinfra
  additional_files_or_dirs:
    - ../../../../node/tests/*

[tadeboro]

So, this seems like a testinfra bug, not a molecule bug, and should be fixed there. But for the sake of end-user convenience, I guess this workaround should be added to Molecule also. I would just add a comment over the added line that describes the why behind the change. Something like "testinfra expects older alias for SSH password here" just to make sure we know why this line of code is there in a year or two.

And I guess this same information should go into commit message also because right now, it is next to impossible to determine why this change is needed (the commit message says nothing about testinfra at all).

[ssbarnea]

So, this seems like a testinfra bug, not a molecule bug, and should be fixed there. But for the sake of end-user convenience, I guess this workaround should be added to Molecule also. I would just add a comment over the added line that describes the why behind the change. Something like "testinfra expects older alias for SSH password here" just to make sure we know why this line of code is there in a year or two.

And I guess this same information should go into commit message also because right now, it is next to impossible to determine why this change is needed (the commit message says nothing about testinfra at all).

I have maintainer rights on testinfra so I could help use merging a fix there. Here are few action points:

• Create upstream (testinfra) bug with details
• Add source code comment this PR that points to the testinfra bug, so next time we look at the code we know why it was added and we will also know when we can remove the workaround
• Update PR title/description with new information, remove unnecessary details.

Once done, I will approve the workaround.

[lmesz-bitrise]

So, this seems like a testinfra bug, not a molecule bug, and should be fixed there. But for the sake of end-user convenience, I guess this workaround should be added to Molecule also. I would just add a comment over the added line that describes the why behind the change. Something like "testinfra expects older alias for SSH password here" just to make sure we know why this line of code is there in a year or two.
And I guess this same information should go into commit message also because right now, it is next to impossible to determine why this change is needed (the commit message says nothing about testinfra at all).

I have maintainer rights on testinfra so I could help use merging a fix there. Here are few action points:

• Create upstream (testinfra) bug with details

I created a testinfra bug, I hope added all the necessary information.

• Add source code comment this PR that points to the testinfra bug, so next time we look at the code we know why it was added and we will also know when we can remove the workaround

Added with the bug in it.

• Update PR title/description with new information, remove unnecessary details.

Updated accordingly.

Once done, I will approve the workaround.

Thank you for the help with the whole PR.