From ef3ded9ace9d7c650e15104698acdf629c57f797 Mon Sep 17 00:00:00 2001 From: omnom62 Date: Sun, 23 Aug 2026 08:39:18 +1000 Subject: T8989: vyos_hostname rework --- plugins/modules/vyos_hostname.py | 294 ++++++++++++++------------------------- 1 file changed, 106 insertions(+), 188 deletions(-) (limited to 'plugins/modules/vyos_hostname.py') diff --git a/plugins/modules/vyos_hostname.py b/plugins/modules/vyos_hostname.py index 86444ad..3218333 100644 --- a/plugins/modules/vyos_hostname.py +++ b/plugins/modules/vyos_hostname.py @@ -12,83 +12,51 @@ DOCUMENTATION = r""" module: vyos_hostname short_description: Manage the system hostname on a VyOS device via the REST API. description: - - Manages the C(set system host-name) configuration on a VyOS device - using the HTTPS REST API. - - Mirrors the behaviour of C(vyos.vyos.vyos_hostname) but uses the HTTP - API instead of SSH/network_cli. - - The states C(replaced), C(overridden) behave identically to C(merged) - for this single-value resource. + - Manages the C(system host-name) configuration on a VyOS device using the HTTPS REST API. version_added: "1.0.0" author: - VyOS Community (@vyos) options: config: - description: - - Hostname configuration. + description: Hostname configuration. type: dict suboptions: hostname: - description: - - System hostname (max 63 characters, no underscores). + description: System hostname (max 63 characters, no underscores). type: str required: true - running_config: - description: - - Used only with state C(parsed). - - The value should be the output of - B(show configuration commands | grep host-name) from the device. - type: str state: description: - C(merged) - Ensure the hostname is set to the value in I(config). - - C(replaced) - Identical to C(merged) for this single-value resource. - - C(overridden) - Identical to C(merged) for this single-value resource. - - C(deleted) - Remove the configured hostname (resets to default). - - C(gathered) - Read the current hostname from the device and return it - in I(gathered) without making changes. - - C(rendered) - Return the CLI commands for the given config without - connecting to the device. - - C(parsed) - Parse the C(running_config) string and return structured - data without connecting to the device. + - >- + C(replaced) and C(overridden) behave identically to C(merged) for + this single-value resource -- there is nothing else to distinctly + replace or override when there is only one field. + - C(deleted) - Remove the configured hostname. + - C(gathered) - Read the current hostname from the device without making changes. type: str - choices: - - merged - - replaced - - overridden - - deleted - - gathered - - rendered - - parsed + choices: [merged, replaced, overridden, deleted, gathered] default: merged - hostname: - description: - - IP address or FQDN of the VyOS device (not needed with httpapi inventory). - type: str - port: - description: - - HTTPS port for the REST API. - type: int - default: 443 - api_key: - description: - - API key configured on the device. - type: str - timeout: - description: - - Request timeout in seconds. - type: int - default: 30 - verify_ssl: - description: - - Validate the device's TLS certificate. - type: bool - default: false -requirements: - - VyOS 1.3+ seealso: - module: vyos.vyos.vyos_hostname """ +EXAMPLES = r""" +- name: Set hostname + vyos.rest.vyos_hostname: + config: + hostname: vyos-core-01 + state: merged + +- name: Gather current hostname + vyos.rest.vyos_hostname: + state: gathered + +- name: Delete hostname configuration + vyos.rest.vyos_hostname: + state: deleted +""" + RETURN = r""" before: description: Configuration on the device before the module ran. @@ -102,173 +70,123 @@ gathered: description: Hostname read from the device (state=gathered only). returned: when state is gathered type: dict -rendered: - description: CLI commands for the provided config (state=rendered only). - returned: when state is rendered - type: list -parsed: - description: Structured data parsed from running_config (state=parsed only). - returned: when state is parsed - type: dict commands: - description: REST API commands dispatched. + description: List of API command tuples sent to the device. returned: always type: list +saved: + description: Whether the config was saved after changes. + returned: when changes are applied + type: bool +response: + description: Raw API response. + returned: when changes are applied + type: dict """ -EXAMPLES = r""" -- name: Set hostname - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-01 - state: merged - -- name: Replace hostname - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-02 - state: replaced - -- name: Gather current hostname - vyos.rest.vyos_hostname: - state: gathered - register: result - -- name: Delete hostname configuration - vyos.rest.vyos_hostname: - state: deleted - -- name: Render commands without connecting - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-01 - state: rendered - -- name: Parse running config - vyos.rest.vyos_hostname: - running_config: "set system host-name 'vyos'" - state: parsed -""" - -import re - from ansible.module_utils.basic import AnsibleModule -from ansible_collections.vyos.rest.plugins.module_utils.vyos_rest import ( - VYOS_REST_CONNECTION_ARGSPEC, - VyOSRestClient, +from ansible_collections.vyos.rest.plugins.module_utils.vyos import ( + VyOSModule, VyOSRestError, ) -_PATH = ["system", "host-name"] +_BASE = ["system", "host-name"] -def _get_hostname(client): - try: - result = client.retrieve_return_value(_PATH) - return result.get("data", "") - except VyOSRestError: - return "" +def get_running_config(vyos): + """ "system host-name" is a plain leafNode, not a container -- a + single scalar value, not a config subtree. get_value (VyOS's + "returnValue" retrieve operation) is the correct fetch for this, + distinct from get_config's "showConfig" operation used everywhere + else in this collection for genuine nested config sections. + """ + return vyos.get_value(_BASE) -def _parse_hostname(running_config): - """Parse hostname from 'show configuration commands | grep host-name' output.""" - match = re.search(r"host-name\s+['\"]?(\S+?)['\"]?\s*$", running_config, re.M) - return match.group(1) if match else "" +def build_commands(config, current, state): + if state == "deleted": + return [("delete", _BASE)] if current else [] + desired = (config or {}).get("hostname") + if not desired or desired == current: + return [] + return [("set", _BASE + [desired])] -def main(): - argument_spec = dict( - config=dict( - type="dict", - options=dict( - hostname=dict(type="str", required=True), - ), - ), - running_config=dict(type="str"), - state=dict( - type="str", - default="merged", - choices=[ - "merged", - "replaced", - "overridden", - "deleted", - "gathered", - "rendered", - "parsed", - ], + +ARGUMENT_SPEC = dict( + config=dict( + type="dict", + options=dict( + hostname=dict(type="str", required=True), ), - ) - argument_spec.update(VYOS_REST_CONNECTION_ARGSPEC) + ), + state=dict( + type="str", + default="merged", + choices=["merged", "replaced", "overridden", "deleted", "gathered"], + ), +) + +def main(): module = AnsibleModule( - argument_spec=argument_spec, - mutually_exclusive=[["config", "running_config"]], + ARGUMENT_SPEC, required_if=[ ("state", "merged", ["config"]), ("state", "replaced", ["config"]), ("state", "overridden", ["config"]), - ("state", "rendered", ["config"]), - ("state", "parsed", ["running_config"]), ], supports_check_mode=True, ) + vyos = VyOSModule(module) + # Collapsed states: replaced/overridden are identical to merged for + # this single-value resource -- there is nothing else to distinctly + # replace or override when there's only one field. state = module.params["state"] - - # rendered — offline, no device connection needed - if state == "rendered": - hostname = module.params["config"]["hostname"] - module.exit_json( - rendered=["set system host-name '{h}'".format(h=hostname)], - commands=[], - ) - - # parsed — offline, no device connection needed - if state == "parsed": - hostname = _parse_hostname(module.params["running_config"] or "") - module.exit_json( - parsed={"hostname": hostname}, - commands=[], - ) - - # collapsed states — replaced and overridden are identical to merged if state in ("replaced", "overridden"): state = "merged" - client = VyOSRestClient(module) - commands = [] - changed = False - - current = _get_hostname(client) - before = {"hostname": current} + config = module.params.get("config") or {} - if state == "gathered": - module.exit_json(changed=False, gathered=before, before=before, commands=[]) + try: + current = get_running_config(vyos) + except VyOSRestError as exc: + module.fail_json(msg="failed to read current hostname: {e}".format(e=str(exc))) - if module.check_mode: - module.exit_json(changed=True, before=before, commands=["(check mode)"]) + have = {"hostname": current} - try: - if state == "merged": - desired = module.params["config"]["hostname"] - if current != desired: - client.configure_set(_PATH, desired) - commands.append("set system host-name '{h}'".format(h=desired)) - changed = True + if state == "gathered": + module.exit_json(changed=False, gathered=have, commands=[]) - elif state == "deleted": - if current: - client.configure_delete(_PATH) - commands.append("delete system host-name") - changed = True + commands = build_commands(config, current, state) - except VyOSRestError as exc: - module.fail_json(msg=str(exc)) + if module.check_mode: + module.exit_json(changed=bool(commands), commands=commands, before=have, after=have) + + if commands: + response = vyos.apply_commands(commands) + saved = vyos.save_config() + try: + after_current = get_running_config(vyos) + except VyOSRestError as exc: + module.fail_json( + msg="hostname change applied but failed to read back result: {e}".format( + e=str(exc), + ), + ) + after = {"hostname": after_current} + module.exit_json( + changed=True, + before=have, + after=after, + commands=commands, + saved=saved, + response=response, + ) - after = {"hostname": _get_hostname(client)} if changed else before - module.exit_json(changed=changed, before=before, after=after, commands=commands) + module.exit_json(changed=False, before=have, after=have, commands=[]) if __name__ == "__main__": -- cgit v1.2.3 From da624a86b8067ca730985d77c99922d08c6a8aa9 Mon Sep 17 00:00:00 2001 From: omnom62 Date: Mon, 14 Sep 2026 19:03:19 +1000 Subject: T8989: vyos_hostname AI comments fixed --- plugins/modules/vyos_hostname.py | 7 +- tests/unit/modules/test_vyos_hostname.py | 138 ++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 5 deletions(-) (limited to 'plugins/modules/vyos_hostname.py') diff --git a/plugins/modules/vyos_hostname.py b/plugins/modules/vyos_hostname.py index 3218333..b7bdd50 100644 --- a/plugins/modules/vyos_hostname.py +++ b/plugins/modules/vyos_hostname.py @@ -149,6 +149,8 @@ def main(): state = "merged" config = module.params.get("config") or {} + if state == "merged" and not config.get("hostname"): + module.fail_json(msg="config.hostname must be a non-empty string") try: current = get_running_config(vyos) @@ -163,7 +165,10 @@ def main(): commands = build_commands(config, current, state) if module.check_mode: - module.exit_json(changed=bool(commands), commands=commands, before=have, after=have) + # Matches the established convention across the rest of the + # collection: omit "after" entirely in check mode, rather than + # reporting have as if it were the post-change state. + module.exit_json(changed=bool(commands), commands=commands, before=have) if commands: response = vyos.apply_commands(commands) diff --git a/tests/unit/modules/test_vyos_hostname.py b/tests/unit/modules/test_vyos_hostname.py index f5f40b3..713c0ff 100644 --- a/tests/unit/modules/test_vyos_hostname.py +++ b/tests/unit/modules/test_vyos_hostname.py @@ -79,14 +79,18 @@ class TestHostnameKeyAlwaysPresent(unittest.TestCase): from unittest.mock import patch - sys.argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})] + import ansible.module_utils.basic as basic + + argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})] captured = {} def fake_exit_json(self_mod, **kwargs): captured.update(kwargs) raise SystemExit(0) - with patch( + basic._ANSIBLE_ARGS = None + basic._PARSED_MODULE_ARGS = None + with patch.object(sys, "argv", argv), patch( "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient", ) as mock_client, patch( "ansible.module_utils.basic.AnsibleModule.exit_json", @@ -114,7 +118,9 @@ class TestGatheredReturnsCommands(unittest.TestCase): from unittest.mock import patch - sys.argv = [ + import ansible.module_utils.basic as basic + + argv = [ "x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}}), ] @@ -124,7 +130,9 @@ class TestGatheredReturnsCommands(unittest.TestCase): captured.update(kwargs) raise SystemExit(0) - with patch( + basic._ANSIBLE_ARGS = None + basic._PARSED_MODULE_ARGS = None + with patch.object(sys, "argv", argv), patch( "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient", ) as mock_client, patch( "ansible.module_utils.basic.AnsibleModule.exit_json", @@ -175,5 +183,127 @@ class TestArgumentSpecNoConnectionParams(unittest.TestCase): self.assertNotIn(key, ARGUMENT_SPEC) +class TestEmptyHostnameFailsExplicitly(unittest.TestCase): + """Confirmed real bug (Copilot): with state=merged, an empty + string for config.hostname previously produced a silent no-op -- + build_commands' falsy check treated "" the same as "not + specified" -- even though hostname is declared required=True. + Ansible's own argspec validation only checks presence, not + non-emptiness, so this reached build_commands unnoticed. Confirmed + by direct reproduction before fixing; now fails explicitly instead. + + In-process pattern (patch.object for sys.argv, matching the + established tests elsewhere in this file) rather than subprocess + isolation: confirmed the subprocess approach breaks under + ansible-test --docker's custom collection-loading machinery, which + a freshly spawned subprocess does not inherit. + """ + + def _run_main(self, config, state="merged"): + import json + import sys + + from unittest.mock import patch + + import ansible.module_utils.basic as basic + + args = {"state": state} + if config is not None: + args["config"] = config + argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": args})] + captured = {} + + def fake_exit_json(self_mod, **kwargs): + captured.update(kwargs) + raise SystemExit(0) + + def fake_fail_json(self_mod, **kwargs): + captured.update(kwargs) + captured["failed"] = True + raise SystemExit(1) + + basic._ANSIBLE_ARGS = None + basic._PARSED_MODULE_ARGS = None + with patch.object(sys, "argv", argv), patch( + "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient", + ) as mock_client, patch( + "ansible.module_utils.basic.AnsibleModule.exit_json", + fake_exit_json, + ), patch( + "ansible.module_utils.basic.AnsibleModule.fail_json", + fake_fail_json, + ): + mock_client.return_value.retrieve_return_value.return_value = { + "data": "existing-host", + } + from ansible_collections.vyos.rest.plugins.modules import vyos_hostname + + with self.assertRaises(SystemExit): + vyos_hostname.main() + return captured + + def test_empty_hostname_fails_explicitly(self): + result = self._run_main({"hostname": ""}) + self.assertTrue(result.get("failed")) + self.assertIn("non-empty", result.get("msg", "")) + + def test_non_empty_hostname_still_works(self): + result = self._run_main({"hostname": "newhost"}) + self.assertNotIn("failed", result) + + +class TestCheckModeOmitsAfter(unittest.TestCase): + """Confirmed real bug (Copilot): check_mode returned after=have + even when commands was non-empty, misrepresenting the pre-change + state as if it were the post-change result. Now matches the + established convention across the rest of the collection + (confirmed against vyos_nat, vyos_ha, vyos_snmp_server, + vyos_ntp_global): omit "after" entirely in check mode. + """ + + def test_check_mode_omits_after(self): + import json + import sys + + from unittest.mock import patch + + import ansible.module_utils.basic as basic + + argv = [ + "x", + json.dumps( + { + "ANSIBLE_MODULE_ARGS": { + "config": {"hostname": "newhost"}, + "state": "merged", + "_ansible_check_mode": True, + }, + }, + ), + ] + captured = {} + + def fake_exit_json(self_mod, **kwargs): + captured.update(kwargs) + raise SystemExit(0) + + basic._ANSIBLE_ARGS = None + basic._PARSED_MODULE_ARGS = None + with patch.object(sys, "argv", argv), patch( + "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient", + ) as mock_client, patch( + "ansible.module_utils.basic.AnsibleModule.exit_json", + fake_exit_json, + ): + mock_client.return_value.retrieve_return_value.return_value = {"data": "oldhost"} + from ansible_collections.vyos.rest.plugins.modules import vyos_hostname + + with self.assertRaises(SystemExit): + vyos_hostname.main() + + self.assertTrue(captured.get("changed")) + self.assertNotIn("after", captured) + + if __name__ == "__main__": unittest.main() -- cgit v1.2.3