T8989: interfaces AI comment fixedT8989_interfaces_dict_op_refactor

2 files changed, 31 insertions, 2 deletions

diff --git a/plugins/modules/vyos_interfaces.py b/plugins/modules/vyos_interfaces.py
index 9c32356..f5733ac 100644
--- a/plugins/modules/vyos_interfaces.py
+++ b/plugins/modules/vyos_interfaces.py
@@ -161,6 +161,7 @@ from ansible_collections.vyos.rest.plugins.module_utils.vyos import (
 
 _BASE = ["interfaces"]
 
+
 _IFACE_TYPE_PREFIX = {
     "eth": "ethernet",
     "bond": "bonding",
@@ -203,6 +204,7 @@ _DEVICE_RENAMES = {
     "vifs": "vif",
 }
 
+
 _ENABLED_FIELD = "enabled"
 _DISABLE_DEVICE_KEY = "disable"
 
@@ -391,7 +393,18 @@ def _scoped_purge_commands(name, have_entry, raw_have):
     return _purge_commands({}, have_device, base)
 
 
-def _enabled_leaves(device):
+def _enabled_leaves(device, allowed_vlan_ids=None):
+    """allowed_vlan_ids restricts the VIF portion to VLAN IDs the task
+    actually listed. The interface itself needs no such restriction --
+    it's always "listed" by virtue of appearing in want at all -- but
+    an unlisted VIF was never mentioned by the task, and merged must
+    never touch it. Confirmed real bug otherwise: a VIF the task
+    doesn't reference at all (or a listed interface whose vifs simply
+    omits it) would still have its stale "disable" leaf removed,
+    silently re-enabling a VLAN the administrator deliberately
+    disabled. Pass None (the want side, where every VIF present
+    already is one the task listed) to skip this restriction.
+    """
     result = {}
     if _DISABLE_DEVICE_KEY in device:
         result[_DISABLE_DEVICE_KEY] = device[_DISABLE_DEVICE_KEY]
@@ -401,6 +414,7 @@ def _enabled_leaves(device):
             vlan_id: {_DISABLE_DEVICE_KEY: v[_DISABLE_DEVICE_KEY]}
             for vlan_id, v in vif.items()
             if _DISABLE_DEVICE_KEY in v
+            and (allowed_vlan_ids is None or vlan_id in allowed_vlan_ids)
         }
         if vif_result:
             result["vif"] = vif_result
@@ -441,7 +455,10 @@ def build_commands(config, raw_have, state):
             commands += _purge_commands(want_device, have_device, base)
         else:
             want_enabled = _enabled_leaves(want_device)
-            have_enabled = _enabled_leaves(have_device)
+            have_enabled = _enabled_leaves(
+                have_device,
+                allowed_vlan_ids=(want_device.get("vif") or {}).keys(),
+            )
             commands += _purge_commands(want_enabled, have_enabled, base)
         commands += dict_op(want_device, have_device, base, op="set")
 
diff --git a/tests/unit/modules/test_vyos_interfaces.py b/tests/unit/modules/test_vyos_interfaces.py
index af0687b..56683df 100644
--- a/tests/unit/modules/test_vyos_interfaces.py
+++ b/tests/unit/modules/test_vyos_interfaces.py
@@ -601,6 +601,18 @@ class TestBuildCommands(VyOSModuleTestCase):
             cmds,
         )
 
+    def test_merged_never_reenables_an_unlisted_vif(self):
+        """CodeRabbit regression: merged's re-enable logic previously
+        scanned every disabled VIF on the device, not just ones the
+        task actually lists -- so a VLAN the administrator
+        deliberately disabled would be silently re-enabled by a task
+        that never even mentions "vifs" at all. merged must leave an
+        unlisted VIF completely untouched, same as any other unlisted
+        field."""
+        raw_have = _FIXTURES["vif_disabled_on_device"]
+        cmds = build_commands([{"name": "eth0", "description": "x"}], raw_have, "merged")
+        self.assertFalse(any("200" in c[1] for c in cmds))
+
     def test_merged_new_interface_with_vrf_and_vif(self):
         config = [
             {