Fix ACLs config drift issue (#563)

* Fix ACLs config drift issue

Signed-off-by: rohitthakur2590 <[email protected]>

* update tests

Signed-off-by: rohitthakur2590 <[email protected]>

* fix sanity

Signed-off-by: rohitthakur2590 <[email protected]>

* fix sanity

Signed-off-by: rohitthakur2590 <[email protected]>

* fix sanity

Signed-off-by: rohitthakur2590 <[email protected]>

* fix sanity

Signed-off-by: rohitthakur2590 <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Signed-off-by: rohitthakur2590 <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

changelogs/fragments/fix_acls_config_drift.yaml

@@ -0,0 +1,5 @@
+---
+bugfixes:
+  - Fixed an issue in the `compare_configs` method where unnecessary negate commands were generated for ACL entries already present in both `have` and `want` configurations.
+  - Improved validation logic for ACL sequence numbers and content matching to ensure idempotency.
+  - Prevented redundant configuration updates for Access Control Lists.

plugins/module_utils/network/eos/config/acls/acls.py

@@ -140,37 +140,53 @@ def compare_configs(self, have, want):
         commands = []
         want = list(itertools.chain(*want))
         have = list(itertools.chain(*have))
+
+        # Flatten the configurations for comparison
         h_index = 0
-        config = list(want)
+        config = list(want)  # Start with a copy of `want`
+
         for w in want:
             access_list = re.findall(r"(ip.*) access-list (.*)", w)
             if access_list:
+                # Check if the whole ACL is already present
                 if w in have:
                     h_index = have.index(w)
             else:
+                # Check sequence-specific entries
                 for num, h in enumerate(have, start=h_index + 1):
                     if "access-list" not in h:
-                        seq_num = re.search(r"(\d+) (.*)", w)
-                        if seq_num:
-                            have_seq_num = re.search(r"(\d+) (.*)", h)
-                            if seq_num.group(1) == have_seq_num.group(
-                                1,
-                            ) and have_seq_num.group(
-                                2,
-                            ) != seq_num.group(2):
-                                negate_cmd = "no " + seq_num.group(1)
-                                config.insert(config.index(w), negate_cmd)
-                        if w in h:
-                            config.pop(config.index(w))
-                            break
+                        snum = re.search(r"(\d+) (.*)", w)
+                        if snum:
+                            have_snum = re.search(r"(\d+) (.*)", h)
+                            if have_snum:
+                                snum_group_2 = snum.group(2)
+                                have_snum_group_2 = have_snum.group(2)
+                                # Match sequence number and full content
+                                if (
+                                    snum.group(1) == have_snum.group(1)
+                                    and snum_group_2 == have_snum_group_2
+                                ):
+                                    # Entry already exists, skip
+                                    config.remove(w)
+                                    break
+                        else:
+                            have_snum = re.search(r"(\d+) (.*)", h)
+                            if have_snum:
+                                # Match sequence number and full content
+                                if w == have_snum.group(2):
+                                    config.remove(w)
+                                    break
+
+        # Generate commands for any remaining entries in `config`
         for c in config:
             access_list = re.findall(r"(ip.*) access-list (.*)", c)
             if access_list:
                 acl_index = config.index(c)
             else:
                 if config[acl_index] not in commands:
-                    commands.append(config[acl_index])
-                commands.append(c)
+                    commands.append(config[acl_index])  # Add ACL definition
+                commands.append(c)  # Add ACL entry
+
         return commands
 
     def set_state(self, want, have):

tests/integration/targets/eos_acls/tests/common/deleted.yaml

@@ -4,6 +4,8 @@
       Start eos_acls deleted integration tests ansible_connection={{ ansible_connection
       }}
 
+- ansible.builtin.include_tasks: _remove_config.yaml
+
 - ansible.builtin.include_tasks: _populate.yaml
 
 - ansible.builtin.set_fact:

tests/integration/targets/eos_acls/tests/common/merged.yaml

@@ -142,19 +142,12 @@
             acls:
               - name: test1
                 aces:
-                  - sequence: 35
-                    log: true
-                    ttl:
-                      eq: 33
-                    source:
-                      any: true
+                  - remark: "Update acl"
         state: merged
 
     - ansible.builtin.assert:
         that:
           - result.changed == true
-          - result.commands|length == 3
-          - "'no 35' in result.commands"
-          - "'35 deny tcp any any ttl eq 33 log'  in result.commands"
+          - result.commands|length == 2
   always:
     - ansible.builtin.include_tasks: _remove_config.yaml

tests/unit/modules/network/eos/test_eos_acls.py

@@ -141,6 +141,7 @@ def test_eos_acls_merged_idempotent(self):
                                         source=dict(any="true"),
                                         destination=dict(any="true"),
                                         protocol=6,
+                                        sequence=45,
                                     ),
                                 ],
                             ),
@@ -150,7 +151,7 @@ def test_eos_acls_merged_idempotent(self):
                 state="merged",
             ),
         )
-        self.execute_module(changed=False, commands=[])
+        result = self.execute_module(changed=False)
 
     def test_eos_acls_replaced(self):
         set_module_args(