Fix add_hosts when ansible_host_key_checking is passed to the new host

[philfry]

…in inventory

related to #1066

[moreati]

@philfry I wrote a test case, which uncovered an additional problem code path

PLAY [integration/connection_delegation/delegate_to_template.yml] **************

TASK [include_tasks _raw_params=../_mitogen_only.yml] **************************
Sunday 05 May 2024  09:32:37 +0000 (0:00:00.085)       0:01:08.596 ************ 
included: /Users/runner/work/1/s/tests/ansible/integration/_mitogen_only.yml for target

TASK [mitogen_get_stack ] ******************************************************
Sunday 05 May 2024  09:32:37 +0000 (0:00:00.104)       0:01:08.701 ************ 
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: TypeError: Can't instantiate abstract class MitogenViaSpec with abstract methods ansible_ssh_host_key_checking
fatal: [target -> cd-normal-alias]: FAILED! => 
  msg: Unexpected failure during module execution.
  stdout: ''

so I've built upon your PR in PR #1069.

1. Does this branch still fix your use case?
2. If you wish to rebase and incorporate my additions into this PR, then I think it'll be ready to merge Fix add_hosts when ansible_host_key_checking is passed to the new host #1067. Otherwise may I credit you as co-author, and close it through Test suite plays with hosts: localhost are not run #1069?
3. Would you like an entry in https://github.com/mitogen-hq/mitogen/blob/master/docs/contributors.rst?

[philfry]

1. Does this branch still fix your use case?

Yes, I just tested it successfully on my machines.

2. If you wish to rebase and incorporate my additions into this PR, then I think it'll be ready to merge [Fix add_hosts when ansible_host_key_checking is passed to the new host #1067](https://github.com/mitogen-hq/mitogen/pull/1067). Otherwise may I credit you as co-author, and close it through [Test suite plays with `hosts: localhost` are not run #1069](https://github.com/mitogen-hq/mitogen/issues/1069)?

I rebased my PR against the current master and added your commit 8a95c70. I hope that's what you meant.

3. Would you like an entry in https://github.com/mitogen-hq/mitogen/blob/master/docs/contributors.rst?

Gladly 😃

[moreati]

I've built upon your PR in PR #1069

I rebased my PR against the current master and added your commit 8a95c70. I hope that's what you meant.

D'oh! I meant #1070, which also includes the new test 207f4f2 and my adaptation of your fix be709c1. Sorry for the confusion.

3. Would you like an entry in https://github.com/mitogen-hq/mitogen/blob/master/docs/contributors.rst?

Turns out you're already in there :).

[philfry]

Hi Alex,

the reason for the (ugly) list/filter construct was because in my tests I was getting odd results otherwise.

Take a look at this:

#!/usr/bin/python3

# stripped down version of ansible's "boolean"
BOOLEANS_TRUE = frozenset(('y', 'yes', 'on', '1', 'true', 't', 1, 1.0, True))
BOOLEANS_FALSE = frozenset(('n', 'no', 'off', '0', 'false', 'f', 0, 0.0, False))
def boolean(value):
    if isinstance(value, bool):
        return value
    normalized_value = str(value).lower()
    if normalized_value in BOOLEANS_TRUE:
        return True
    elif normalized_value in BOOLEANS_FALSE:
        return False

# host_key_checking from pr1070
def pr1070(a, b, c):
    val = (a or b or c)
    return boolean(val)

# ansible_ssh_host_key_checking from pr1067
# not exactly the same because C.HOST_KEY_CHECKING is evaluated outside of this function,
# it's just to get the idea
def pr1067(a, b, c):
    tmp = [boolean(x) for x in list(
        filter(lambda x:x is not None, (a, b, c))
    )]
    if not tmp: return None
    return any(tmp)

# build a random matrix
matrix = []
for a in None, 'yes', 0:
    for b in None, 1, False:
        for c in None, True, False:
            matrix += [(a, b, c)]

# compare methods
for t in matrix:
    alex = pr1070(*t)
    phil = pr1067(*t)

    if alex != phil: print(f"{t}, alex: {alex}, phil: {phil}")
    else: print(t)

This results in

(None, None, None)
(None, None, True)
(None, None, False)
(None, 1, None)
(None, 1, True)
(None, 1, False)
(None, False, None), alex: None, phil: False
(None, False, True)
(None, False, False)
('yes', None, None)
('yes', None, True)
('yes', None, False)
('yes', 1, None)
('yes', 1, True)
('yes', 1, False)
('yes', False, None)
('yes', False, True)
('yes', False, False)
(0, None, None), alex: None, phil: False
(0, None, True)
(0, None, False)
(0, 1, None)
(0, 1, True)
(0, 1, False)
(0, False, None), alex: None, phil: False
(0, False, True)
(0, False, False)

So some combinations of None and False seem problematic.

Take this slightly modified playbook:

---
- hosts: localhost
  gather_facts: no
  tasks:
    - add_host:
        name: ansible-test-100
        ansible_host: 192.168.122.100
        ansible_ssh_private_key_file: /tmp/ansibletest-leases/100/id_ed25519
        ansible_host_key_checking: '{{chk}}'

- hosts: ansible-test-100
  gather_facts: no
  remote_user: ansible
  become: no
  tasks:
    - ping:

and run it with ansible-playbook test.yml -e chk='{{False}}' – imho this should result in having the host key checking disabled.
Here #1067 and #1070 show different behaviours.

Adding some debug lines to #1070:

--- i/ansible_mitogen/transport_config.py
+++ w/ansible_mitogen/transport_config.py
@@ -475,11 +475,17 @@ class PlayContextSpec(Spec):
             rediscover_python=rediscover_python)
 
     def host_key_checking(self):
+        print(f"""
+            ansible_ssh_host_key_checking: {self._connection.get_task_var('ansible_ssh_host_key_checking')}
+            ansible_host_key_checking: {self._connection.get_task_var('ansible_host_key_checking')}
+            C.HOST_KEY_CHECKING: {C.HOST_KEY_CHECKING}
+        """)
         val = (
             self._connection.get_task_var('ansible_ssh_host_key_checking')
             or self._connection.get_task_var('ansible_host_key_checking')
             or C.HOST_KEY_CHECKING
         )
+        print(f"val: {val}")
         return ansible.module_utils.parsing.convert_bool.boolean(val)
 
     def private_key_file(self):

→

            ansible_ssh_host_key_checking: None
            ansible_host_key_checking: False
            C.HOST_KEY_CHECKING: True
        
val: True

It's probably not very common passing "real" boolean arguments to Ansible, but older versions of Ansible had some bugs here that forced me to use them. FWIW: You can get a "real" None using -e chk='{{omit}}'.

What do you think, should we ignore this and take the #1070 code or could this become a problem in the future?

Another approach could be this:

    def host_key_checking(self):
        val = [ansible.module_utils.parsing.convert_bool.boolean(x) for x in list(
            filter(lambda x:x is not None, (
                self._connection.get_task_var('ansible_ssh_host_key_checking'),
                self._connection.get_task_var('ansible_host_key_checking'),
                C.HOST_KEY_CHECKING,
                True
            )))]
        return val[0] 

This preserves priorities and, for sanity, has a default value of True (not really needed, as C.HOST_KEY_CHECKING is always either True or False as defined by Ansible).

[philfry]

# ansible_ssh_host_key_checking from pr1067
# modified
def pr1067(a, b, c):
    tmp = [boolean(x) for x in list(
        filter(lambda x:x is not None, (a, b, c, True))
    )]
    return tmp[0]

→

(None, None, None), alex: None, phil: True
(None, None, True)
(None, None, False)
(None, 1, None)
(None, 1, True)
(None, 1, False)
(None, False, None), alex: None, phil: False
(None, False, True), alex: True, phil: False
(None, False, False)
('yes', None, None)
('yes', None, True)
('yes', None, False)
('yes', 1, None)
('yes', 1, True)
('yes', 1, False)
('yes', False, None)
('yes', False, True)
('yes', False, False)
(0, None, None), alex: None, phil: False
(0, None, True), alex: True, phil: False
(0, None, False)
(0, 1, None), alex: True, phil: False
(0, 1, True), alex: True, phil: False
(0, 1, False), alex: True, phil: False
(0, False, None), alex: None, phil: False
(0, False, True), alex: True, phil: False
(0, False, False)

[moreati]

I've updated #1070. I think it accounts for None, and I added test coverage. I'm trying to avoid eager expressions such as list comprehensions or Python 2.x filter(), so that the logic can still short circuit and avoid running unnecessary calls to e.g. self._host_vars.get().

mitogen/ansible_mitogen/transport_config.py

Lines 477 to 483 in 5749845

	def host_key_checking(self):
	def candidates():
	yield self._connection.get_task_var('ansible_ssh_host_key_checking')
	yield self._connection.get_task_var('ansible_host_key_checking')
	yield C.HOST_KEY_CHECKING
	val = next(v for v in candidates() if v is not None)
	return ansible.module_utils.parsing.convert_bool.boolean(val)

[philfry]

That looks pretty good!
I added a default value to the iteration, though, to avoid an exception if all values are None.

[moreati]

Just one cosmetic, then I'm happy. I'll rerun any flaky CI jobs (#1058) as needed.