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 --- docs/vyos.rest.vyos_hostname_module.rst | 152 +++----------------------------- 1 file changed, 12 insertions(+), 140 deletions(-) (limited to 'docs') diff --git a/docs/vyos.rest.vyos_hostname_module.rst b/docs/vyos.rest.vyos_hostname_module.rst index c8ea1c5..7e4e37e 100644 --- a/docs/vyos.rest.vyos_hostname_module.rst +++ b/docs/vyos.rest.vyos_hostname_module.rst @@ -17,18 +17,10 @@ Version added: 1.0.0 Synopsis -------- -- Manages the ``set system host-name`` configuration on a VyOS device using the HTTPS REST API. -- Mirrors the behaviour of ``vyos.vyos.vyos_hostname`` but uses the HTTP API instead of SSH/network_cli. -- The states ``replaced``, ``overridden`` behave identically to ``merged`` for this single-value resource. +- Manages the ``system host-name`` configuration on a VyOS device using the HTTPS REST API. -Requirements ------------- -The below requirements are needed on the host that executes this module. - -- VyOS 1.3+ - Parameters ---------- @@ -41,21 +33,6 @@ Parameters Choices/Defaults Comments - - -
- api_key - -
- string -
- - - - -
API key configured on the device.
- -
@@ -89,53 +66,6 @@ Parameters - - -
- hostname - -
- string -
- - - - -
IP address or FQDN of the VyOS device (not needed with httpapi inventory).
- - - - -
- port - -
- integer -
- - - Default:
443
- - -
HTTPS port for the REST API.
- - - - -
- running_config - -
- string -
- - - - -
Used only with state parsed.
-
The value should be the output of show configuration commands | grep host-name from the device.
- -
@@ -152,53 +82,13 @@ Parameters
  • overridden
  • deleted
  • gathered
    • -
  • rendered
    • -
  • parsed
    • merged - Ensure the hostname is set to the value in config.
    -
    replaced - Identical to merged for this single-value resource.
    -
    overridden - Identical to merged for this single-value resource.
    -
    deleted - Remove the configured hostname (resets to default).
    -
    gathered - Read the current hostname from the device and return it in gathered without making changes.
    -
    rendered - Return the CLI commands for the given config without connecting to the device.
    -
    parsed - Parse the running_config string and return structured data without connecting to the device.
    - - - - -
    - timeout - -
    - integer -
    - - - Default:
    30
    - - -
    Request timeout in seconds.
    - - - - -
    - verify_ssl - -
    - boolean -
    - - - - - -
    Validate the device's TLS certificate.
    +
    replaced and overridden behave identically to merged for this single-value resource -- there is nothing else to distinctly replace or override when there is only one field.
    +
    deleted - Remove the configured hostname.
    +
    gathered - Read the current hostname from the device without making changes.
    @@ -226,32 +116,14 @@ Examples 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 - Return Values @@ -307,7 +179,7 @@ Common return values are documented `here always -
    REST API commands dispatched.
    +
    List of API command tuples sent to the device.

    @@ -329,30 +201,30 @@ Common return values are documented `here
    - parsed + response
    dictionary
    - when state is parsed + when changes are applied -
    Structured data parsed from running_config (state=parsed only).
    +
    Raw API response.

    - rendered + saved
    - list + boolean
    - when state is rendered + when changes are applied -
    CLI commands for the provided config (state=rendered only).
    +
    Whether the config was saved after changes.

    -- cgit v1.2.3 From 9fe93349225eb9b014e7a3e397da88fece40a0b2 Mon Sep 17 00:00:00 2001 From: omnom62 <75066712+omnom62@users.noreply.github.com> Date: Sat, 26 Sep 2026 01:42:14 +1000 Subject: T8989: vyos_static_routes dict_op refactor (#25) * T8989: vyos_static_routes dict_op refactor * T8989: Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../fragments/t8989_static_routes_dict_op.yml | 3 + docs/vyos.rest.vyos_static_routes_module.rst | 5 +- plugins/modules/vyos_static_routes.py | 64 ++++++++++++++++++++-- tests/unit/modules/test_vyos_static_routes.py | 50 +++++++++++++++++ 4 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 changelogs/fragments/t8989_static_routes_dict_op.yml (limited to 'docs') diff --git a/changelogs/fragments/t8989_static_routes_dict_op.yml b/changelogs/fragments/t8989_static_routes_dict_op.yml new file mode 100644 index 0000000..0f6be0d --- /dev/null +++ b/changelogs/fragments/t8989_static_routes_dict_op.yml @@ -0,0 +1,3 @@ +--- +minor_changes: + - vyos_static_routes - Refactor to convert to dict_op. diff --git a/docs/vyos.rest.vyos_static_routes_module.rst b/docs/vyos.rest.vyos_static_routes_module.rst index 353cf2d..78fbde9 100644 --- a/docs/vyos.rest.vyos_static_routes_module.rst +++ b/docs/vyos.rest.vyos_static_routes_module.rst @@ -193,11 +193,12 @@ Parameters
      Choices:
    • no
      • -
    • yes ←
      • +
    • yes
    Whether this next-hop is enabled.
    +
    Deliberately has no default: leaving it unset means "no opinion" and an existing device-side disabled state is left alone under merged. Explicitly setting true actively clears a previously disabled next-hop, which merged could otherwise never do (only replaced/overridden run a purge pass capable of noticing an omitted value).
    @@ -261,7 +262,7 @@ Parameters
    merged - Add routes without removing existing ones.
    replaced - Replace each named route (by afi + dest) exactly as specified.
    -
    overridden - Replace the entire static route table.
    +
    overridden - Replace the module-known parts of the static route table (see module scope note above; unmodeled attributes on unmodeled routes are not touched).
    deleted - Remove listed or all static routes.
    gathered - Read static routes from device without changes.
    diff --git a/plugins/modules/vyos_static_routes.py b/plugins/modules/vyos_static_routes.py index 3276588..11a636d 100644 --- a/plugins/modules/vyos_static_routes.py +++ b/plugins/modules/vyos_static_routes.py @@ -65,9 +65,17 @@ options: description: Administrative distance for this next-hop (1-255). type: int enabled: - description: Whether this next-hop is enabled. + description: + - Whether this next-hop is enabled. + - >- + Deliberately has no default: leaving it unset means + "no opinion" and an existing device-side disabled + state is left alone under C(merged). Explicitly + setting C(true) actively clears a previously + disabled next-hop, which C(merged) could otherwise + never do (only C(replaced)/C(overridden) run a + purge pass capable of noticing an omitted value). type: bool - default: true interface: description: Outgoing interface name. type: str @@ -75,7 +83,9 @@ options: description: - C(merged) - Add routes without removing existing ones. - C(replaced) - Replace each named route (by afi + dest) exactly as specified. - - C(overridden) - Replace the entire static route table. + - C(overridden) - Replace the module-known parts of the static route + table (see module scope note above; unmodeled attributes on + unmodeled routes are not touched). - C(deleted) - Remove listed or all static routes. - C(gathered) - Read static routes from device without changes. type: str @@ -316,6 +326,41 @@ def _device_to_argspec(raw): return result +def _clear_stale_disable_commands(config, norm_have): + """dict_op(op="set") only ever adds/updates keys present in want -- + it structurally cannot reach a "disable" leaf that's simply absent + from want, so under "merged" (which never runs a purge pass at + all) there was no way for the generic engine to clear a previously + disabled next-hop, confirmed as a real gap. This checks the + original argspec-shape config directly (not the device-shape + want, where explicit True and unset/None are indistinguishable by + construction) for an explicit enabled: true against a next-hop + that's currently disabled on the device, and emits an explicit + delete for its "disable" leaf when the two disagree. + + Not needed for replaced/overridden: their purge pass already + clears an omitted "disable" correctly regardless of why it's + omitted (purge compares the full want/have shape, not just this + one leaf), so calling this there would only be harmless, redundant + work -- it's scoped to merged specifically to avoid that. + """ + cmds = [] + for entry in config or []: + route_key = _ROUTE_KEY.get(entry.get("afi")) + if not route_key: + continue + for route in entry.get("routes") or []: + dest = route.get("dest") + if not dest: + continue + have_nhs = ((norm_have.get(route_key) or {}).get(dest) or {}).get("next-hop") or {} + for nh in route.get("next_hops") or []: + addr = nh.get("forward_router_address") + if addr and nh.get("enabled") is True and "disable" in (have_nhs.get(addr) or {}): + cmds.append(("delete", _BASE + [route_key, dest, "next-hop", addr, "disable"])) + return cmds + + def build_commands(config, raw_have, state): raw_have = raw_have or {} config = config or [] @@ -375,6 +420,8 @@ def build_commands(config, raw_have, state): _BASE + [route_key, dest], op="purge", ) + elif state == "merged": + commands += _clear_stale_disable_commands(config, norm_have) commands += dict_op(want, norm_have, _BASE, op="set") return commands @@ -382,7 +429,7 @@ def build_commands(config, raw_have, state): _NEXT_HOP_OPTIONS = dict( forward_router_address=dict(type="str", required=True), admin_distance=dict(type="int"), - enabled=dict(type="bool", default=True), + enabled=dict(type="bool"), interface=dict(type="str"), ) @@ -441,7 +488,14 @@ def main(): commands = build_commands(config, raw_have, state) if module.check_mode: - module.exit_json(changed=bool(commands), commands=commands, before=have) + # Predicting the exact post-apply shape without applying would + # mean re-implementing dict_op's effects in reverse. Rather + # than that, or silently omitting "after" (which RETURN + # documents as present "when changed" -- true here whenever + # commands is non-empty), return have as the best honestly + # available value: not a prediction, but at least a defined, + # non-crashing one for playbooks that read result.after. + module.exit_json(changed=bool(commands), commands=commands, before=have, after=have) if commands: response = vyos.apply_commands(commands) diff --git a/tests/unit/modules/test_vyos_static_routes.py b/tests/unit/modules/test_vyos_static_routes.py index 1a50da9..0ec74a5 100644 --- a/tests/unit/modules/test_vyos_static_routes.py +++ b/tests/unit/modules/test_vyos_static_routes.py @@ -259,6 +259,56 @@ class TestBuildCommands(VyOSModuleTestCase): expected = ("delete", _BASE + ["route", "192.0.2.0/24", "next-hop", "10.0.0.1", "distance"]) self.assertIn(expected, cmds) + def test_merged_explicit_enabled_true_clears_stale_disable(self): + """Confirmed real gap from review: dict_op(op="set") only ever + walks want's own keys, so it can never reach a "disable" leaf + that's simply absent from want -- under "merged" (which never + runs a purge pass at all) there was no way to clear a + previously disabled next-hop. Fixed via an explicit check + against the original argspec-shape config for enabled: true, + scoped to merged specifically since replaced/overridden + already handle this correctly through their existing purge + pass.""" + raw_have = {"route": {"192.0.2.0/24": {"next-hop": {"10.0.0.1": {"disable": {}}}}}} + config = [ + { + "afi": "ipv4", + "routes": [ + { + "dest": "192.0.2.0/24", + "next_hops": [ + {"forward_router_address": "10.0.0.1", "enabled": True}, + ], + }, + ], + }, + ] + cmds = build_commands(config, raw_have, "merged") + expected = ("delete", _BASE + ["route", "192.0.2.0/24", "next-hop", "10.0.0.1", "disable"]) + self.assertIn(expected, cmds) + + def test_merged_omitted_enabled_leaves_existing_disable_alone(self): + """enabled deliberately has no default (fixed alongside the + above): omitting it entirely means "no opinion", so an + unrelated merged update must not silently re-enable an + existing disabled next-hop.""" + raw_have = {"route": {"192.0.2.0/24": {"next-hop": {"10.0.0.1": {"disable": {}}}}}} + config = [ + { + "afi": "ipv4", + "routes": [ + { + "dest": "192.0.2.0/24", + "next_hops": [ + {"forward_router_address": "10.0.0.1", "admin_distance": 5}, + ], + }, + ], + }, + ] + cmds = build_commands(config, raw_have, "merged") + self.assertFalse(any("disable" in str(c) for c in cmds)) + def test_replaced_scoped_to_named_route_only(self): raw_have = { "route": { -- cgit v1.2.3