Bug: `Check whether Ansible can connect as <user>` – passes when SSH connection fails

strarsis commented 1 year ago

Terms

[X] I have read the guidelines for Contributing to Roots Projects
[X] This request is not a duplicate of an existing issue
[X] I have read the docs and followed them (if applicable)
[X] I have seached the Roots Discourse for answers and followed them (if applicable)
[X] This is not a personal support request that should be posted on the Roots Discourse community

Description

What's wrong?

The Check whether Ansible can connect as [...] task succeeds although the SSH connection actually fails (like an auth error). This means that SSH connection problems during the the provision and deploy playbooks will not be caught immediately.

What have you tried?

For testing I ensured that SSH connections will fail to the target system (disabled the SSH agent). Manual SSH connections then indeed fail. Ansible also now fails with connecting, but only further down the line, here at the Gathering facts step. The Check whether Ansible can connect as [...] task still passes. In ansible playbook verbose mode the stdout is shown that contains the SSH authentication errors and the final error that the SSH connection indeed failed: (See log section).

Minimal test case (playbook) (e.g. test-connection.yml):

- hosts: staging
  gather_facts: no
  tasks:
    - name: Check whether Ansible can connect as web
      command: |
        ansible staging -m raw -a whoami -u web -vvvv
      delegate_to: localhost
      failed_when: false
      changed_when: false
      check_mode: no

ansible-playbook test-connection.yml -e env=staging (Make it very verbose (-vvvv) so the values of stdout and stderr can be inspected).

What insights have you gained?

Although the SSH command actually fails and while the stderr is empty, the stdout contains the SSH errors, the current task passes, as failed_when: false apparently doesn't apply as intended when the ansible raw module fails to connect in this test command.

The exit code for a failing raw ansible SSH test command is non-zero, for a successful SSH connection it is zero (0):

ansible staging -m raw -a whoami -u web -vvvv
echo $?
4 # or 127

The current implementation in Trellis uses the stdout of that command to check for SSH host key issues, but it doesn't catch SSH connection errors.

Possible solutions

Make the Check whether Ansible can connect as [...] task fail for non-zero exit codes (that indicates an error) (failed_when: connection_status.rc != 0 instead of failed_when: false):

- name: Check whether Ansible can connect as {{ dynamic_user | default(true) | ternary('root', web_user) }}
  command: |
    ansible {{ inventory_hostname }} -m raw -a whoami
    -u {{ dynamic_user | default(true) | ternary('root', web_user) }} {{ cli_options | default('') }} -vvvv
  delegate_to: localhost
  environment:
    ANSIBLE_SSH_ARGS: "{{ ssh_args_default }} {{ ansible_ssh_extra_args | default('') }}"
  failed_when: connection_status.rc != 0
  changed_when: false
  check_mode: no
  register: connection_status
  tags: [connection-tests]

Temporary workarounds

As the playbook will fail later in case of a SSH connection error, no workaround is necessary. But the task that has the name Check whether Ansible can connect as [...] should ensure that the SSH connection is actually possible and catch this issue before proceeding any further.

Steps To Reproduce

Ensure the SSH connection to target system fails.
Run the playbook (e.g. for provision or deploy), either by using the trellis CLI or invoking ansible-playbook directly. Note that the Check whether Ansible can connect as [...] task will pass, and only in the subsequent steps the playbook fails due to a SSH connection error.

Expected Behavior

The Check whether Ansible can connect as [...] task and hence the playbook at that point should fail as the SSH connection failed.

Actual Behavior

The Check whether Ansible can connect as [...] task passes (OK`), the playbook will fail later, after that.

Relevant Log Output

"stdout": "\u001b[0;34mansible [core 2.12.6]\u001b[0m\n\u001b[0;34m  config file = [...]/trellis/ansible.cfg\u001b[0m\n\u001b[0;34m  [...] \ndebug3: no such identity: /home/build/.ssh/id_xmss: No such file or directory\\r\\ndebug2: we did not send a packet, disable method\\r\\ndebug1: No more authentication methods to try.\\r\\nweb@staging-2.localdomain: Permission denied (publickey).\",\u001b[0m\n\u001b[1;31m    \"unreachable\": true\u001b[0m\n\u001b[1;31m}\u001b[0m"

Versions

Add built-in fail2ban filters (#1375)

swalkinshaw commented 1 year ago

Thanks for all the details. I can see why that's confusing but I'm sort of wondering if this was done on purpose... though I wouldn't know the reason. The intention behind that check isn't strictly "can you connect via SSH" but the more nuanced "can the root user connect in order to determine which admin_user Trellis tries to use after".

One goal of this current design might have been to not try and override Ansible's default behaviour for SSH failures. Your solution would be a normal failure, not strictly a connection failure. So if we did that, we might want to write out a nicer custom failure message?

strarsis commented 1 year ago

Yes, more nuanced error messages would be great. When there is an SSH issue (KeePass not opened (for KeeAgent)) this task passes, but then it fails later. As this check happens directly at the start, it would be nice to know immediately if something is wrong.

roots / trellis