From 551d09ce4d249138a8616709dcb36dc1a0963a47 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 6 Aug 2026 10:10:39 +0200 Subject: [PATCH 1/2] inventory: add a host-context expression resolver Add resolve_in_host_context(), which templates a Jinja2 expression in a host's Ansible variable context and returns the result. Nothing calls it yet; the callers follow. "ansible-inventory --host" returns variables as defined, so anything Jinja-valued comes back as the raw "{{ ... }}". osism/defaults all/README.md ("Consuming these values from code") tells external tooling how such a value has to be resolved instead: through templating in the host context, never by re-implementing the expression in the consumer. This helper is that operation, in one place, so callers do not each grow their own approximation of Jinja. It runs "ansible -m copy" with the expression as the content and a file in a temporary directory as the destination, then reads the file: - "-c local" keeps the module on the controller. No connection is made to the host, so a node that is down or unreachable still resolves, and ANSIBLE_GATHERING=explicit states that no fact gathering happens. - The value is read back from a file rather than parsed out of Ansible's output. The package runs under more than one ansible-core (2.19.11 in its own image, 2.18.x in the osism-ansible, kolla-ansible and ceph-ansible images), and their callbacks differ in wording and in which stream they use. "--tree" would give structured output but is deprecated for removal in ansible-core 2.23. - Module arguments are passed as JSON, so a value containing spaces survives. - Facts are supplied as extra vars rather than through a fact-cache plugin. The redis cache plugin lives in community.general, which is not installed here, and a jsonfile cache is honoured by an ad-hoc lookup on 2.18 but not on 2.19. Extra vars behave identically on both, and the precedence is right for facts, which already outrank host_vars. - Failure is decided by the return code alone. The undefined-variable sentinel differs between the two versions ("VARIABLE IS NOT DEFINED!" against "<>"), so matching on it would work on one and not the other. On failure the error text Ansible produced is passed through unparsed, because it names the variable that could not be resolved. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/utils/inventory.py | 116 ++++++++++++++++++++++++ tests/unit/utils/test_inventory.py | 141 ++++++++++++++++++++++++++++- 2 files changed, 256 insertions(+), 1 deletion(-) diff --git a/osism/utils/inventory.py b/osism/utils/inventory.py index dd90adc72..ea5a4c14f 100644 --- a/osism/utils/inventory.py +++ b/osism/utils/inventory.py @@ -1,10 +1,17 @@ # SPDX-License-Identifier: Apache-2.0 +import json import os +import subprocess +import tempfile from loguru import logger +class HostContextResolutionError(Exception): + """Raised when an expression cannot be templated in a host's context.""" + + def get_inventory_path(base_path: str, prefer_minified: bool = True) -> str: """Return the best available inventory path. @@ -57,3 +64,112 @@ def get_hosts_from_inventory(data: dict) -> list: if isinstance(value, dict) and "hosts" in value: hosts.update(value["hosts"]) return sorted(hosts) + + +def resolve_in_host_context( + host: str, + expression: str, + inventory_path: str, + facts: dict | None = None, + timeout: int = 60, +) -> str: + """Template a Jinja2 expression in a host's Ansible variable context. + + ``ansible-inventory --host`` returns variables *as defined*, so anything + Jinja-valued comes back as the raw ``{{ ... }}``. Resolving such a value + requires Ansible's own templating, in the host's variable context -- see + ``osism/defaults`` ``all/README.md``, section "Consuming these values from + code". Re-implementing the templating in the consumer is not an option: it + only ever covers the shapes that were thought of. + + The value is produced by having Ansible ``copy`` the templated expression + into a file, run with ``-c local`` so the module executes on the controller. + No connection is made to the host, so this works for hosts that are down or + unreachable, and the value is read back byte-exact instead of being parsed + out of human-readable output. That matters because the callback formats + differ between the ansible-core versions this package runs under, and + because ``--tree``, the other way to get structured output, is deprecated + for removal in ansible-core 2.23. + + Facts are passed as extra vars rather than through a fact-cache plugin. + That keeps the call independent of which cache plugin is configured (the + ``redis`` plugin lives in ``community.general``, which is not installed + here) and of the Ansible version: ansible-core 2.18 exposes cached facts to + an ad-hoc ``debug`` while 2.19 does not, whereas extra vars behave + identically on both. Extra vars outrank everything, which is what we want + for facts -- they already outrank host_vars in normal precedence. + + Args: + host: Inventory hostname to evaluate the expression for. + expression: Jinja2 expression *without* the surrounding braces. + inventory_path: Inventory to resolve the host and its variables from. + facts: Ansible facts for the host, as stored in the fact cache. + timeout: Seconds to wait for Ansible. + + Returns: + The templated value, as a string. + + Raises: + HostContextResolutionError: If Ansible could not evaluate the + expression. The message carries Ansible's own explanation, which + names the undefined variable or attribute. + """ + env = os.environ.copy() + # The module runs locally, but be explicit: never gather facts, so a host + # that is down cannot turn a lookup into an SSH timeout. + env["ANSIBLE_GATHERING"] = "explicit" + env["ANSIBLE_RETRY_FILES_ENABLED"] = "False" + env["ANSIBLE_NOCOLOR"] = "1" + + with tempfile.TemporaryDirectory(prefix="osism-resolve-") as workdir: + value_path = os.path.join(workdir, "value") + # JSON module args rather than key=value, so a value containing spaces + # survives. + module_args = json.dumps( + {"content": "{{ %s }}" % expression, "dest": value_path} + ) + command = [ + "ansible", + host, + "-i", + inventory_path, + "-c", + "local", + "-m", + "copy", + "-a", + module_args, + ] + if facts: + facts_path = os.path.join(workdir, "facts.json") + with open(facts_path, "w") as fp: + json.dump(facts, fp) + command += ["-e", f"@{facts_path}"] + + try: + result = subprocess.run( + command, + capture_output=True, + text=True, + timeout=timeout, + env=env, + ) + except subprocess.TimeoutExpired as exc: + raise HostContextResolutionError( + f"ansible timed out after {timeout}s" + ) from exc + + if result.returncode != 0: + # Ansible names the undefined variable, which is the most useful + # thing to pass on. Its wording and stream differ between versions, + # so take whichever of the two is non-empty and do not parse it. + detail = (result.stdout or "").strip() or (result.stderr or "").strip() + raise HostContextResolutionError( + detail or f"ansible exited {result.returncode}" + ) + + try: + with open(value_path) as fp: + return fp.read() + except OSError as exc: + raise HostContextResolutionError("ansible wrote no value") from exc diff --git a/tests/unit/utils/test_inventory.py b/tests/unit/utils/test_inventory.py index f69738476..b46e9f3d9 100644 --- a/tests/unit/utils/test_inventory.py +++ b/tests/unit/utils/test_inventory.py @@ -1,6 +1,18 @@ # SPDX-License-Identifier: Apache-2.0 -from osism.utils.inventory import get_hosts_from_inventory, get_inventory_path +import json +import os +import subprocess +from types import SimpleNamespace + +import pytest + +from osism.utils.inventory import ( + HostContextResolutionError, + get_hosts_from_inventory, + get_inventory_path, + resolve_in_host_context, +) def _make_base(tmp_path): @@ -140,3 +152,130 @@ def test_get_hosts_from_inventory_group_with_hosts_and_children(): } assert get_hosts_from_inventory(data) == ["host-a"] + + +class TestResolveInHostContext: + """``resolve_in_host_context`` delegates templating to Ansible. + + The subprocess is mocked: what matters here is the command that gets built, + that facts travel as an extra-vars file, and that the value is read back from + the file Ansible writes rather than parsed out of its output -- the callback + format differs between the ansible-core versions this package runs under. + """ + + @staticmethod + def _run(mocker, returncode=0, value=None, stdout="", stderr="", capture=None): + """Patch subprocess.run, optionally writing the value file.""" + + def fake_run(command, **kwargs): + args = json.loads(command[command.index("-a") + 1]) + if capture is not None: + # The temp dir is gone by the time the test inspects anything, + # so read the extra-vars file here. + extra_vars = None + if "-e" in command: + with open(command[command.index("-e") + 1][1:]) as fp: + extra_vars = json.load(fp) + capture.append((command, kwargs, args, extra_vars)) + if value is not None: + with open(args["dest"], "w") as fp: + fp.write(value) + return SimpleNamespace(returncode=returncode, stdout=stdout, stderr=stderr) + + return mocker.patch( + "osism.utils.inventory.subprocess.run", side_effect=fake_run + ) + + def test_returns_templated_value(self, mocker): + self._run(mocker, value="10.0.0.5") + + assert resolve_in_host_context("host1", "some_expression", "/inv") == "10.0.0.5" + + def test_value_is_returned_byte_exact(self, mocker): + # No stripping: the helper is generic, and Ansible's copy writes the + # content without adding a trailing newline. + self._run(mocker, value="a b c") + + assert resolve_in_host_context("host1", "expr", "/inv") == "a b c" + + def test_command_wraps_expression_and_passes_inventory(self, mocker): + calls = [] + self._run(mocker, value="10.0.0.5", capture=calls) + + resolve_in_host_context("host1", "internal_interface", "/inv/hosts.yml") + + command, kwargs, args, _ = calls[0] + assert command[:2] == ["ansible", "host1"] + assert command[command.index("-i") + 1] == "/inv/hosts.yml" + assert command[command.index("-m") + 1] == "copy" + # -c local keeps the module on the controller, so a host that is down + # still resolves and no SSH is attempted. + assert command[command.index("-c") + 1] == "local" + assert args["content"] == "{{ internal_interface }}" + assert kwargs["env"]["ANSIBLE_GATHERING"] == "explicit" + + def test_module_args_are_json_not_key_value(self, mocker): + # key=value args would split a value containing spaces. + calls = [] + self._run(mocker, value="x", capture=calls) + + resolve_in_host_context("host1", "expr", "/inv") + + raw = calls[0][0][calls[0][0].index("-a") + 1] + assert json.loads(raw) # parses as JSON + + def test_facts_are_passed_as_extra_vars_file(self, mocker): + calls = [] + self._run(mocker, value="10.0.0.5", capture=calls) + facts = {"ansible_local": {"testbed_network_devices": {"management": "eth3"}}} + + resolve_in_host_context("host1", "expr", "/inv", facts=facts) + + command, _, _, extra_vars = calls[0] + assert command[command.index("-e") + 1].startswith("@") + assert extra_vars == facts + + def test_no_extra_vars_argument_without_facts(self, mocker): + calls = [] + self._run(mocker, value="10.0.0.5", capture=calls) + + resolve_in_host_context("host1", "expr", "/inv") + + assert "-e" not in calls[0][0] + + def test_nonzero_exit_raises_with_ansible_message(self, mocker): + # Ansible names the undefined attribute; that is what the operator needs. + self._run(mocker, returncode=2, stdout="has no attribute 'ansible_vlan999'") + + with pytest.raises(HostContextResolutionError, match="ansible_vlan999"): + resolve_in_host_context("host1", "expr", "/inv") + + def test_nonzero_exit_falls_back_to_stderr(self, mocker): + # 2.18 and 2.19 do not agree on which stream carries the error. + self._run(mocker, returncode=4, stderr="could not match supplied host pattern") + + with pytest.raises(HostContextResolutionError, match="host pattern"): + resolve_in_host_context("host1", "expr", "/inv") + + def test_success_without_value_file_raises(self, mocker): + self._run(mocker, returncode=0) + + with pytest.raises(HostContextResolutionError, match="no value"): + resolve_in_host_context("host1", "expr", "/inv") + + def test_timeout_raises(self, mocker): + mocker.patch( + "osism.utils.inventory.subprocess.run", + side_effect=subprocess.TimeoutExpired("ansible", 60), + ) + + with pytest.raises(HostContextResolutionError, match="timed out"): + resolve_in_host_context("host1", "expr", "/inv") + + def test_temporary_directory_is_cleaned_up(self, mocker): + calls = [] + self._run(mocker, value="10.0.0.5", capture=calls) + + resolve_in_host_context("host1", "expr", "/inv", facts={"a": 1}) + + assert not os.path.exists(os.path.dirname(calls[0][2]["dest"])) From 386e9767517aff6dbfd888e92fcea12629b33268 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 6 Aug 2026 10:10:41 +0200 Subject: [PATCH 2/2] rabbitmq: resolve internal_interface through Ansible get_rabbitmq_node_addresses() read internal_interface with "ansible-inventory --host", which returns variables as defined, so a Jinja-valued internal_interface came back as the raw "{{ ... }}". It then resolved that by hand: a regex captured the contents of the first "{{ ... }}" and the dotted path was walked through the ansible facts. That covers exactly one shape -- a single expression whose dotted path is rooted in facts, i.e. the testbed's internal_interface: "{{ ansible_local.testbed_network_devices.management }}" and nothing else: - An internal_interface pointing at an inventory variable fails, because the walk only ever looks in the facts: "Could not resolve template '{{ some_var }}' from facts for ". - A mixed literal and template such as "vlan{{ vlan_id }}" is passed through verbatim, because re.match requires "{{" at offset 0, and the lookup then asks for a fact named ansible_vlan{{ vlan_id }}. - The regex is unanchored, so "{{ base }}.100" matches only the leading expression and silently discards the ".100" tail, resolving to the wrong interface -- a wrong answer rather than an error. - Filters, hostvars lookups and defaults are not evaluated at all. Use resolve_in_host_context() instead, so Ansible does the templating in the host's own variable context. This is what osism/defaults all/README.md ("Consuming these values from code") prescribes for external consumers, and it makes the supported set "whatever Jinja2 supports" rather than a list of anticipated shapes. The whole resolver goes away, along with the subsequent walk from interface name to ansible_ to ipv4.address, since the expression covers all of it. The resolved value is validated as an IPv4 address before use. The return code already reports a templating failure, but a value that comes back looking nothing like an address must not be passed on as one either. The integration case for an internal_interface that points at an inventory variable was marked xfail(strict) when it was added, because the resolver could not resolve it. It passes now, so the marker goes: with strict set, leaving it would fail the suite on the unexpected pass. That is the demonstration this change needed -- the shapes that already worked are still covered by the same tests, and the one that did not now passes against real Ansible rather than against a mock. The tests for the deleted resolver go with it: Jinja2 traversal, the non-string and non-dict cases, the interface-name to fact-key mapping and the ipv4 extraction all tested behaviour that is now Ansible's. What replaces them asserts the contract that remains -- that the expression and the cached facts are handed over unchanged, that a resolution failure surfaces Ansible's own message, and that a non-address result is refused. Verified against ansible-core 2.18.9 and 2.19.11 with the reported variable shape ("{{ vlan_var }}" where vlan_var is itself "vlan{{ id }}"), a dotted interface name, a dashed one, and the fact-derived shape the old resolver supported, plus a missing fact. One behaviour change worth noting for review: a missing internal_interface is now reported through Ansible's undefined-variable message rather than a dedicated one. osism status rabbitmq shares the helper and is fixed with it. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/utils/rabbitmq.py | 96 ++++------- tests/integration/test_rabbitmq_addresses.py | 12 +- tests/unit/utils/test_rabbitmq.py | 158 +++++++++++-------- 3 files changed, 131 insertions(+), 135 deletions(-) diff --git a/osism/utils/rabbitmq.py b/osism/utils/rabbitmq.py index e11f1f21b..53a6c85ea 100644 --- a/osism/utils/rabbitmq.py +++ b/osism/utils/rabbitmq.py @@ -1,13 +1,27 @@ # SPDX-License-Identifier: Apache-2.0 +import ipaddress import json import os -import re import subprocess from loguru import logger -from osism.utils.inventory import get_hosts_from_inventory, get_inventory_path +from osism.utils.inventory import ( + HostContextResolutionError, + get_hosts_from_inventory, + get_inventory_path, + resolve_in_host_context, +) + +# The node's internal address. Ansible names interface facts with "-" replaced +# by "_" and dots left alone (PrefixFactNamespace._underscore), so "br-ex" is +# ansible_br_ex while "bond0.100" is ansible_bond0.100. +INTERNAL_ADDRESS_EXPRESSION = ( + "hostvars[inventory_hostname]" + "['ansible_' + (internal_interface | replace('-', '_'))]" + "['ipv4']['address']" +) def get_rabbitmq_node_addresses(): @@ -51,73 +65,31 @@ def get_rabbitmq_node_addresses(): facts = json.loads(facts_data) - # Get hostvars for this host to find internal_interface + # Resolve internal_interface and the address it carries in one + # templated lookup, so that any Jinja2 shape works -- not just + # the ones a hand-written resolver anticipated. hostvar_inventory_path = get_inventory_path( "/ansible/inventory/hosts.yml", prefer_minified=False ) - result = subprocess.check_output( - f"ansible-inventory -i {hostvar_inventory_path} --host {host}", - shell=True, - stderr=subprocess.DEVNULL, - ) - hostvars = json.loads(result) - - internal_interface_raw = hostvars.get("internal_interface") - if not internal_interface_raw: - logger.error(f"internal_interface not found in hostvars for {host}") - continue - - # Resolve Jinja2 template if present (e.g., "{{ ansible_local.testbed_network_devices.management }}") - internal_interface = internal_interface_raw - template_match = re.match( - r"\{\{\s*(.+?)\s*\}\}", internal_interface_raw - ) - if template_match: - path = template_match.group(1).strip() - parts = path.split(".") - value = facts - for part in parts: - if isinstance(value, dict): - value = value.get(part) - else: - value = None - break - if value and isinstance(value, str): - internal_interface = value - else: - logger.error( - f"Could not resolve template '{internal_interface_raw}' from facts for {host}" - ) - continue - - logger.debug(f"Internal interface for {host}: {internal_interface}") - - # Look for the interface in ansible facts. Ansible replaces "-" - # with "_" in fact names and leaves dots alone - # (PrefixFactNamespace._underscore), so "br-ex" is - # ansible_br_ex while "bond0.100" is ansible_bond0.100. - normalized_interface = internal_interface.replace("-", "_") - interface_key = f"ansible_{normalized_interface}" - - interface_facts = facts.get(interface_key) - if not interface_facts: - logger.error( - f"Interface {internal_interface} ({interface_key}) not found in ansible facts for {host}" - ) - continue - - # Get IPv4 address - ipv4_info = interface_facts.get("ipv4") - if not ipv4_info: - logger.error( - f"No IPv4 address found for interface {internal_interface} on {host}" + try: + ipv4_address = resolve_in_host_context( + host, + INTERNAL_ADDRESS_EXPRESSION, + hostvar_inventory_path, + facts=facts, ) + except HostContextResolutionError as exc: + logger.error(f"Could not resolve address for {host}: {exc}") continue - ipv4_address = ipv4_info.get("address") - if not ipv4_address: + # A templating failure is reported by the return code, but a + # module that returns a non-address string must not be trusted + # either -- validate rather than pass it on as an address. + try: + ipaddress.IPv4Address(ipv4_address) + except ValueError: logger.error( - f"No IPv4 address found for interface {internal_interface} on {host}" + f"Resolved address for {host} is not an IPv4 address: {ipv4_address!r}" ) continue diff --git a/tests/integration/test_rabbitmq_addresses.py b/tests/integration/test_rabbitmq_addresses.py index 3ecc84525..8fb89da6b 100644 --- a/tests/integration/test_rabbitmq_addresses.py +++ b/tests/integration/test_rabbitmq_addresses.py @@ -134,15 +134,11 @@ def test_dashed_interface_name(scenario): assert rabbitmq.get_rabbitmq_node_addresses() == [("10.74.34.13", host)] -@pytest.mark.xfail( - strict=True, - reason="internal_interface pointing at an inventory variable is not resolved; " - "the resolver only walks dotted paths through the facts (osism/issues#1425)", -) def test_interface_from_inventory_variable(scenario): - # The shape reported by a client: internal_interface refers to an inventory - # variable, which is itself a literal plus a template. Nothing here is a - # fact, so a facts-only walk cannot resolve it. + # The shape reported in osism/issues#1425: internal_interface refers to an + # inventory variable, which is itself a literal plus a template. Nothing + # here is a fact, which is why resolving it needs Ansible's templating + # rather than a walk through the facts. Marked xfail until that landed. host = scenario( "ctl5", { diff --git a/tests/unit/utils/test_rabbitmq.py b/tests/unit/utils/test_rabbitmq.py index 990cc43bb..64a28f1f5 100644 --- a/tests/unit/utils/test_rabbitmq.py +++ b/tests/unit/utils/test_rabbitmq.py @@ -3,8 +3,8 @@ """Unit tests for ``osism.utils.rabbitmq``. Tests are grouped into one class per function: ``TestGetRabbitmqNodeAddresses`` -(inventory + host discovery, per-host interface resolution including Jinja2 -template traversal, and result aggregation) and ``TestLoadRabbitmqPassword`` +(inventory + host discovery, per-host address resolution, and result +aggregation) and ``TestLoadRabbitmqPassword`` (secrets-file loading and normalization), plus the ``RABBITMQ_USER`` module constant. The collaborators of each function are wired up by the ``setup_addresses`` / ``setup_password`` factory fixtures. @@ -26,7 +26,7 @@ import pytest import osism.utils as utils_pkg -from osism.utils import rabbitmq +from osism.utils import inventory, rabbitmq # A valid group-listing payload for the first ``ansible-inventory`` call. The # content is irrelevant because ``get_hosts_from_inventory`` is mocked; only the @@ -39,11 +39,6 @@ def _encode(payload): return json.dumps(payload).encode() -def _hostvars(interface): - """Build a ``--host`` hostvars payload carrying ``internal_interface``.""" - return _encode({"internal_interface": interface}) - - def _facts(interface_key, address): """Build an ansible-facts payload exposing one interface with an IPv4.""" return _encode({interface_key: {"ipv4": {"address": address}}}) @@ -68,7 +63,7 @@ def setup_addresses(mocker): keys and command lines they were invoked with. """ - def _setup(*, hosts, redis_side_effect, check_output): + def _setup(*, hosts, redis_side_effect, check_output, resolve=None): fake_redis = mocker.MagicMock() fake_redis.get.side_effect = redis_side_effect # Seed the lazy attribute on the package so ``utils.redis`` resolves to @@ -83,10 +78,17 @@ def _setup(*, hosts, redis_side_effect, check_output): check_output_mock = mocker.patch( "osism.utils.rabbitmq.subprocess.check_output", side_effect=check_output ) + # Templating is Ansible's job; the unit under test only has to hand it + # the right expression and facts and validate what comes back. + resolve_mock = mocker.patch( + "osism.utils.rabbitmq.resolve_in_host_context", + side_effect=list(resolve) if resolve is not None else [], + ) return SimpleNamespace( redis=fake_redis, get_inventory_path=get_inventory_path, check_output=check_output_mock, + resolve=resolve_mock, ) return _setup @@ -139,7 +141,8 @@ def test_two_hosts_returned_in_alphabetical_order( _facts("ansible_eth0", "10.0.0.5"), _facts("ansible_eth0", "10.0.0.6"), ], - check_output=[_GROUP_LISTING, _hostvars("eth0"), _hostvars("eth0")], + check_output=[_GROUP_LISTING], + resolve=["10.0.0.5", "10.0.0.6"], ) result = rabbitmq.get_rabbitmq_node_addresses() @@ -159,7 +162,8 @@ def test_inventory_queries_use_expected_arguments( mocks = setup_addresses( hosts=["host1"], redis_side_effect=[_facts("ansible_eth0", "10.0.0.5")], - check_output=[_GROUP_LISTING, _hostvars("eth0")], + check_output=[_GROUP_LISTING], + resolve=["10.0.0.5"], ) rabbitmq.get_rabbitmq_node_addresses() @@ -167,7 +171,6 @@ def test_inventory_queries_use_expected_arguments( # ``--limit rabbitmq`` is the only thing scoping the listing to the # rabbitmq group; ``get_hosts_from_inventory`` does no group filtering. assert "--limit rabbitmq" in mocks.check_output.call_args_list[0].args[0] - assert "--host host1" in mocks.check_output.call_args_list[1].args[0] # The hostvars lookup must not use the minified inventory, which omits # hostvars such as ``internal_interface``. assert mocks.get_inventory_path.call_args_list == [ @@ -220,24 +223,74 @@ def test_outer_generic_exception_returns_none(self, setup_addresses, loguru_logs # -- per-host resolution ------------------------------------------------- + def test_resolver_called_with_expression_inventory_and_facts( + self, setup_addresses, loguru_logs + ): + facts = {"ansible_eth0": {"ipv4": {"address": "10.0.0.5"}}} + setup_addresses( + hosts=["host1"], + redis_side_effect=[_encode(facts)], + check_output=[_GROUP_LISTING], + resolve=["10.0.0.5"], + ) + + mocks = rabbitmq.get_rabbitmq_node_addresses() + + assert mocks == [("10.0.0.5", "host1")] + + def test_resolver_receives_cached_facts_verbatim( + self, setup_addresses, loguru_logs + ): + # The facts read from the cache must reach Ansible unchanged: they are + # what makes fact-derived values such as + # ``{{ ansible_local.testbed_network_devices.management }}`` resolvable. + facts = {"ansible_local": {"testbed_network_devices": {"management": "eth3"}}} + mocks = setup_addresses( + hosts=["host1"], + redis_side_effect=[_encode(facts)], + check_output=[_GROUP_LISTING], + resolve=["10.0.0.5"], + ) + + rabbitmq.get_rabbitmq_node_addresses() + + assert mocks.resolve.call_args_list == [ + call( + "host1", + rabbitmq.INTERNAL_ADDRESS_EXPRESSION, + "/inv", + facts=facts, + ) + ] + + def test_expression_normalizes_dashes_but_not_dots(self): + # Ansible names interface facts with "-" replaced by "_" and leaves + # dots alone (PrefixFactNamespace._underscore), so the expression must + # do exactly that -- an earlier implementation also replaced dots and + # therefore never found ``ansible_bond0.100``. + assert "replace('-', '_')" in rabbitmq.INTERNAL_ADDRESS_EXPRESSION + assert "'.'" not in rabbitmq.INTERNAL_ADDRESS_EXPRESSION + def test_missing_facts_in_cache_skips_host_and_continues( self, setup_addresses, loguru_logs ): setup_addresses( hosts=["host1", "host2"], redis_side_effect=[None, _facts("ansible_eth0", "10.0.0.6")], - check_output=[_GROUP_LISTING, _hostvars("eth0")], + check_output=[_GROUP_LISTING], + resolve=["10.0.0.6"], ) assert rabbitmq.get_rabbitmq_node_addresses() == [("10.0.0.6", "host2")] _assert_error_logged(loguru_logs, "No ansible facts found in cache for host1") @pytest.mark.parametrize( - "redis_side_effect,check_output", + "redis_side_effect,resolve,expected_error", [ pytest.param( [_facts("ansible_eth0", "10.0.0.5"), b"{corrupt facts"], - [_GROUP_LISTING, _hostvars("eth0")], + ["10.0.0.5"], + "Failed to resolve address for host2", id="corrupt_cached_facts", ), pytest.param( @@ -246,85 +299,62 @@ def test_missing_facts_in_cache_skips_host_and_continues( _facts("ansible_eth0", "10.0.0.6"), ], [ - _GROUP_LISTING, - _hostvars("eth0"), - subprocess.CalledProcessError(1, "ansible-inventory"), + "10.0.0.5", + inventory.HostContextResolutionError("'foo' is undefined"), ], - id="hostvars_query_fails", + "Could not resolve address for host2", + id="templating_failed", ), pytest.param( [ _facts("ansible_eth0", "10.0.0.5"), _facts("ansible_eth0", "10.0.0.6"), ], - [_GROUP_LISTING, _hostvars("eth0"), b"{not valid json"], - id="corrupt_hostvars_json", + ["10.0.0.5", "VARIABLE IS NOT DEFINED!"], + "is not an IPv4 address", + id="non_address_value", ), pytest.param( [ _facts("ansible_eth0", "10.0.0.5"), _facts("ansible_eth0", "10.0.0.6"), ], - [_GROUP_LISTING, _hostvars("eth0"), _hostvars(["eth0"])], - id="non_string_internal_interface", + ["10.0.0.5", "fe80::1"], + "is not an IPv4 address", + id="ipv6_value", ), ], ) def test_per_host_failure_keeps_addresses_of_other_hosts( - self, setup_addresses, loguru_logs, redis_side_effect, check_output + self, setup_addresses, loguru_logs, redis_side_effect, resolve, expected_error ): # host1 resolves before host2 fails; the failure must only drop host2. setup_addresses( hosts=["host1", "host2"], redis_side_effect=redis_side_effect, - check_output=check_output, + check_output=[_GROUP_LISTING], + resolve=resolve, ) assert rabbitmq.get_rabbitmq_node_addresses() == [("10.0.0.5", "host1")] - _assert_error_logged(loguru_logs, "Failed to resolve address for host2") - - @pytest.mark.parametrize( - "management_value", - [None, {"nested": "x"}, 42], - ids=["none", "dict", "int"], - ) - def test_template_resolving_to_non_string_skips_host( - self, setup_addresses, loguru_logs, management_value - ): - facts = { - "ansible_local": { - "testbed_network_devices": {"management": management_value} - } - } - setup_addresses( - hosts=["host1"], - redis_side_effect=[_encode(facts)], - check_output=[ - _GROUP_LISTING, - _hostvars("{{ ansible_local.testbed_network_devices.management }}"), - ], - ) - - assert rabbitmq.get_rabbitmq_node_addresses() is None - _assert_error_logged(loguru_logs, "Could not resolve template") + _assert_error_logged(loguru_logs, expected_error) - def test_template_traversal_hits_non_dict_skips_host( - self, setup_addresses, loguru_logs - ): - # ``ansible_local`` is a string, so walking ``.testbed_network_devices`` - # leaves the dict path and the template resolves to None. - facts = {"ansible_local": "not-a-dict"} + def test_resolution_error_message_is_surfaced(self, setup_addresses, loguru_logs): + # Ansible's own wording names the undefined variable, which is the most + # useful thing an operator can be told; it must not be swallowed. setup_addresses( hosts=["host1"], - redis_side_effect=[_encode(facts)], - check_output=[ - _GROUP_LISTING, - _hostvars("{{ ansible_local.testbed_network_devices.management }}"), + redis_side_effect=[_facts("ansible_eth0", "10.0.0.5")], + check_output=[_GROUP_LISTING], + resolve=[ + inventory.HostContextResolutionError( + "has no attribute 'ansible_vlan999'" + ) ], ) assert rabbitmq.get_rabbitmq_node_addresses() is None - _assert_error_logged(loguru_logs, "Could not resolve template") + _assert_error_logged(loguru_logs, "has no attribute 'ansible_vlan999'") # -- aggregate results ---------------------------------------------------- @@ -336,9 +366,7 @@ def test_all_hosts_skipped_returns_none(self, setup_addresses, loguru_logs): ) assert rabbitmq.get_rabbitmq_node_addresses() is None - _assert_error_logged( - loguru_logs, "Could not retrieve address for any RabbitMQ node" - ) + _assert_error_logged(loguru_logs, "Could not retrieve address for any RabbitMQ") class TestLoadRabbitmqPassword: