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>

4 files changed, 115 insertions, 7 deletions

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
                 <td>
                         <ul style="margin: 0; padding: 0"><b>Choices:</b>
                                     <li>no</li>
-                                    <li><div style="color: blue"><b>yes</b>&nbsp;&larr;</div></li>
+                                    <li>yes</li>
                         </ul>
                 </td>
                 <td>
                         <div>Whether this next-hop is enabled.</div>
+                        <div>Deliberately has no default: leaving it unset means &quot;no opinion&quot; and an existing device-side disabled state is left alone under <code>merged</code>. Explicitly setting <code>true</code> actively clears a previously disabled next-hop, which <code>merged</code> could otherwise never do (only <code>replaced</code>/<code>overridden</code> run a purge pass capable of noticing an omitted value).</div>
                 </td>
             </tr>
             <tr>
@@ -261,7 +262,7 @@ Parameters
                 <td>
                         <div><code>merged</code> - Add routes without removing existing ones.</div>
                         <div><code>replaced</code> - Replace each named route (by afi + dest) exactly as specified.</div>
-                        <div><code>overridden</code> - Replace the entire static route table.</div>
+                        <div><code>overridden</code> - Replace the module-known parts of the static route table (see module scope note above; unmodeled attributes on unmodeled routes are not touched).</div>
                         <div><code>deleted</code> - Remove listed or all static routes.</div>
                         <div><code>gathered</code> - Read static routes from device without changes.</div>
                 </td>
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": {