From 0d8f4a6fe07247d46294d5ecd3437ae8d198ed34 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 2 Jul 2026 20:03:01 +0200 Subject: [PATCH] Fix Redfish credential loading for 'redfish list' The `osism redfish list ...` command connected to the BMC with empty credentials. It builds a direct sushy connection via get_redfish_connection(), which is the only path that re-derives BMC credentials from the NetBox device "secrets" custom field through _get_conductor_redfish_credentials(). That helper silently returned None, None, so the connection fell back to anonymous auth and failed against any BMC that requires authentication -- with no exception raised and nothing logged. The configured Redfish address was similarly dropped, falling back to the default https://{hostname}. The Ironic sync path is unaffected: it calls _prepare_node_attributes() directly (ironic.py:823), already unpacks the tuple, and pushes the same secrets-derived credentials into Ironic. So BMC access that goes through Ironic (deploy, clean, power) kept working; only the standalone redfish list command, which re-derives the credentials on its own, was broken. The cause is in conductor/utils.py. _prepare_node_attributes() returns a (node_attributes, template_vars) 2-tuple, but _get_conductor_redfish_credentials() and _get_conductor_redfish_address() still bound the whole tuple to node_attributes and then tested `"driver_info" in node_attributes`. On a tuple that is a membership test against the two tuple elements, which is always False, so both helpers short-circuited before .get() and returned None, None / None regardless of the device configuration. The tuple return was introduced when ironic.py was refactored; these two callers were never updated and had no test coverage. Unpack the tuple at both call sites and add regression tests for both helpers covering the redfish happy path, the None-device guard, the non-redfish driver, missing driver_info, and swallowed exceptions. The happy-path tests fail against the pre-fix code, pinning the behaviour. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Roger Luethi --- osism/tasks/conductor/utils.py | 4 +- tests/unit/tasks/conductor/test_utils.py | 97 ++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/osism/tasks/conductor/utils.py b/osism/tasks/conductor/utils.py index 815a57539..1a81e394e 100644 --- a/osism/tasks/conductor/utils.py +++ b/osism/tasks/conductor/utils.py @@ -220,7 +220,7 @@ def get_ironic_parameters(): configuration = get_configuration() return configuration.get("ironic_parameters", {}) - node_attributes = _prepare_node_attributes(device, get_ironic_parameters) + node_attributes, _ = _prepare_node_attributes(device, get_ironic_parameters) # Extract Redfish credentials if available if ( @@ -252,7 +252,7 @@ def get_ironic_parameters(): configuration = get_configuration() return configuration.get("ironic_parameters", {}) - node_attributes = _prepare_node_attributes(device, get_ironic_parameters) + node_attributes, _ = _prepare_node_attributes(device, get_ironic_parameters) # Extract Redfish address if available if ( diff --git a/tests/unit/tasks/conductor/test_utils.py b/tests/unit/tasks/conductor/test_utils.py index c3186ac5e..c5260286e 100644 --- a/tests/unit/tasks/conductor/test_utils.py +++ b/tests/unit/tasks/conductor/test_utils.py @@ -8,6 +8,8 @@ from osism.tasks.conductor.utils import ( DELETE_SENTINEL, + _get_conductor_redfish_address, + _get_conductor_redfish_credentials, _is_secret_key, deep_compare, deep_decrypt, @@ -478,3 +480,98 @@ def test_load_yaml_file_does_not_call_get_vault_for_plain_file(tmp_path): assert result == {"foo": 1} get_vault.assert_not_called() + + +# --------------------------------------------------------------------------- +# _get_conductor_redfish_credentials / _get_conductor_redfish_address +# +# _prepare_node_attributes returns a (node_attributes, template_vars) tuple. +# Both helpers must unpack it before indexing into node_attributes; treating +# the tuple as a dict makes the "driver_info" membership test fall through and +# silently drops the conductor credential/address fallback. +# --------------------------------------------------------------------------- + + +def _patch_prepare(node_attributes, template_vars=None): + """Patch _prepare_node_attributes to return the tuple it produces.""" + return patch( + "osism.tasks.conductor.ironic._prepare_node_attributes", + return_value=(node_attributes, template_vars or {}), + ) + + +def test_get_conductor_redfish_credentials_returns_username_and_password(): + node_attributes = { + "driver": "redfish", + "driver_info": { + "redfish_username": "admin", + "redfish_password": "s3cret", + }, + } + + with _patch_prepare(node_attributes, {"ignored": "template_vars"}): + assert _get_conductor_redfish_credentials(object()) == ("admin", "s3cret") + + +def test_get_conductor_redfish_credentials_none_device_returns_none_pair(): + assert _get_conductor_redfish_credentials(None) == (None, None) + + +def test_get_conductor_redfish_credentials_non_redfish_driver_returns_none_pair(): + node_attributes = { + "driver": "ipmi", + "driver_info": {"ipmi_username": "root", "ipmi_password": "calvin"}, + } + + with _patch_prepare(node_attributes): + assert _get_conductor_redfish_credentials(object()) == (None, None) + + +def test_get_conductor_redfish_credentials_missing_driver_info_returns_none_pair(): + with _patch_prepare({"driver": "redfish"}): + assert _get_conductor_redfish_credentials(object()) == (None, None) + + +def test_get_conductor_redfish_credentials_swallows_exception(): + with patch( + "osism.tasks.conductor.ironic._prepare_node_attributes", + side_effect=RuntimeError("boom"), + ): + assert _get_conductor_redfish_credentials(object()) == (None, None) + + +def test_get_conductor_redfish_address_returns_address(): + node_attributes = { + "driver": "redfish", + "driver_info": {"redfish_address": "https://bmc.example"}, + } + + with _patch_prepare(node_attributes, {"ignored": "template_vars"}): + assert _get_conductor_redfish_address(object()) == "https://bmc.example" + + +def test_get_conductor_redfish_address_none_device_returns_none(): + assert _get_conductor_redfish_address(None) is None + + +def test_get_conductor_redfish_address_non_redfish_driver_returns_none(): + node_attributes = { + "driver": "ipmi", + "driver_info": {"ipmi_address": "1.2.3.4"}, + } + + with _patch_prepare(node_attributes): + assert _get_conductor_redfish_address(object()) is None + + +def test_get_conductor_redfish_address_missing_driver_info_returns_none(): + with _patch_prepare({"driver": "redfish"}): + assert _get_conductor_redfish_address(object()) is None + + +def test_get_conductor_redfish_address_swallows_exception(): + with patch( + "osism.tasks.conductor.ironic._prepare_node_attributes", + side_effect=RuntimeError("boom"), + ): + assert _get_conductor_redfish_address(object()) is None