firewall_rules: Fix incorrect behavior when IPv4 and IPv6 rule sets have the same name (#236)

firewall_rules: Fix incorrect behavior when IPv4 and IPv6 rule sets have the same name

SUMMARY
VyOS supports IPv4 and IPv6 rule sets having the same name, but there are a couple places in the Ansible module that don't handle this situation.

The fact gathering for ansible_network_resources.firewall_rules has been updated to look for name <name> or ipv6-name <name> instead of just <name>.
The vyos_firewall_rules module has been updated to take the afi into consideration when comparing the have and want states.
V4-EGRESS and V6-EGRESS have been renamed to just EGRESS in the tests. The existing tests seem to be complete enough to test this same-name situation. (V4-INGRESS and V6-INGRESS were not renamed.)

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
vyos_facts and vyos_firewall_rules
ADDITIONAL INFORMATION
An example of a configuration that was originally causing an issue:

(Click to expand):
name wan-lan {
    default-action drop
    rule 1 {
        action accept
        state {
            established enable
            related enable
        }
    }
    rule 2 {
        action drop
        log enable
        state {
            invalid enable
        }
    }
}

ipv6-name wan-lan {
    default-action drop
    rule 1 {
        action accept
        state {
            established enable
            related enable
        }
    }
    rule 2 {
        action drop
        log enable
        state {
            invalid enable
        }
    }
    rule 10 {
        action accept
        protocol icmpv6
    }
}


With this configuration, ansible_network_resources.firewall_rules would show the icmpv6 rule under both ipv4 and ipv6:

(Click to expand):
[
    {
        "afi": "ipv4",
        "rule_sets": [
            {
                "default_action": "drop",
                "name": "wan-lan",
                "rules": [
                    {
                        "action": "accept",
                        "number": 1,
                        "state": {
                            "established": true,
                            "related": true
                        }
                    },
                    {
                        "action": "drop",
                        "number": 2,
                        "state": {
                            "invalid": true
                        }
                    },
                    {
                        "action": "accept",
                        "number": 10,
                        "protocol": "icmpv6"
                    }
                ]
            },
        ]
    },
    {
        "afi": "ipv6",
        "rule_sets": [
            {
                "default_action": "drop",
                "name": "wan-lan",
                "rules": [
                    {
                        "action": "accept",
                        "number": 1,
                        "state": {
                            "established": true,
                            "related": true
                        }
                    },
                    {
                        "action": "drop",
                        "number": 2,
                        "state": {
                            "invalid": true
                        }
                    },
                    {
                        "action": "accept",
                        "number": 10,
                        "protocol": "icmpv6"
                    }
                ]
            },
        ]
    }
]

A similar issue would happen when using vyos_firewall_rules as well, where it would attempt to change rules for the wrong afi.

Reviewed-by: GomathiselviS <None>
Reviewed-by: None <None>

5 files changed, 37 insertions, 25 deletions

diff --git a/changelogs/fragments/rule_set_same_name.yaml b/changelogs/fragments/rule_set_same_name.yaml
new file mode 100644
index 0000000..48bf565
--- /dev/null
+++ b/changelogs/fragments/rule_set_same_name.yaml
@@ -0,0 +1,5 @@
+---
+minor_changes:
+  - Update vyos_facts to support IPv4 and IPv6 rule sets having the same name
+  - Update vyos_firewall_rules to support IPv4 and IPv6 rule sets having the same name
+  - Rename V4-EGRESS/V6-EGRESS to EGRESS in the tests to test the same-name situation
diff --git a/plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py b/plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py
index 1f1536c..22973bd 100644
--- a/plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py
+++ b/plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py
@@ -176,7 +176,7 @@ class Firewall_rules(ConfigBase):
                     # already have (to be replaced by our desired
                     # configuration's rule set).
                     wanted_rule_set = self.search_r_sets_in_have(
-                        want, rs["name"], "r_list"
+                        want, rs["name"], "r_list", h["afi"]
                     )
                     if wanted_rule_set is not None:
                         # Remove the rules that we already have if the wanted
@@ -205,7 +205,9 @@ class Firewall_rules(ConfigBase):
             for h in have:
                 r_sets = self._get_r_sets(h)
                 for rs in r_sets:
-                    w = self.search_r_sets_in_have(want, rs["name"], "r_list")
+                    w = self.search_r_sets_in_have(
+                        want, rs["name"], "r_list", h["afi"]
+                    )
                     if not w:
                         commands.append(
                             self._compute_command(
@@ -230,7 +232,9 @@ class Firewall_rules(ConfigBase):
         for w in want:
             r_sets = self._get_r_sets(w)
             for rs in r_sets:
-                h = self.search_r_sets_in_have(have, rs["name"], "r_list")
+                h = self.search_r_sets_in_have(
+                    have, rs["name"], "r_list", w["afi"]
+                )
                 commands.extend(self._add_r_sets(w["afi"], rs, h))
         return commands
 
@@ -248,7 +252,7 @@ class Firewall_rules(ConfigBase):
                 if r_sets:
                     for rs in r_sets:
                         h = self.search_r_sets_in_have(
-                            have, rs["name"], "r_list"
+                            have, rs["name"], "r_list", w["afi"]
                         )
                         if h:
                             commands.append(
@@ -842,12 +846,13 @@ class Firewall_rules(ConfigBase):
                             )
         return commands
 
-    def search_r_sets_in_have(self, have, w_name, type="rule_sets"):
+    def search_r_sets_in_have(self, have, w_name, type="rule_sets", afi=None):
         """
         This function  returns the rule-set/rule if it is present in target config.
         :param have: target config.
         :param w_name: rule-set name.
         :param type: rule_sets/rule/r_list.
+        :param afi: address family (when type is r_list).
         :return: rule-set/rule.
         """
         if have:
@@ -859,10 +864,11 @@ class Firewall_rules(ConfigBase):
                         return r
             elif type == "r_list":
                 for h in have:
-                    r_sets = self._get_r_sets(h)
-                    for rs in r_sets:
-                        if rs[key] == w_name:
-                            return rs
+                    if h["afi"] == afi:
+                        r_sets = self._get_r_sets(h)
+                        for rs in r_sets:
+                            if rs[key] == w_name:
+                                return rs
             else:
                 for rs in have:
                     if rs[key] == w_name:
diff --git a/plugins/module_utils/network/vyos/facts/firewall_rules/firewall_rules.py b/plugins/module_utils/network/vyos/facts/firewall_rules/firewall_rules.py
index 8e29dbd..f1c080f 100644
--- a/plugins/module_utils/network/vyos/facts/firewall_rules/firewall_rules.py
+++ b/plugins/module_utils/network/vyos/facts/firewall_rules/firewall_rules.py
@@ -100,7 +100,8 @@ class Firewall_rulesFacts(object):
         r_v4 = []
         r_v6 = []
         for r in set(rules):
-            rule_regex = r" %s .+$" % r.strip("'")
+            name_key = "ipv6-name" if type == "ipv6" else "name"
+            rule_regex = r" %s %s .+$" % (name_key, r.strip("'"))
             cfg = findall(rule_regex, data, M)
             fr = self.render_config(cfg, r.strip("'"))
             fr["name"] = r.strip("'")
diff --git a/tests/unit/modules/network/vyos/fixtures/vyos_firewall_rules_config.cfg b/tests/unit/modules/network/vyos/fixtures/vyos_firewall_rules_config.cfg
index 8726301..32d4294 100644
--- a/tests/unit/modules/network/vyos/fixtures/vyos_firewall_rules_config.cfg
+++ b/tests/unit/modules/network/vyos/fixtures/vyos_firewall_rules_config.cfg
@@ -9,7 +9,7 @@ set firewall name V4-INGRESS rule 101
 set firewall name V4-INGRESS rule 101 disabled
 set firewall name V4-INGRESS rule 101 action 'accept'
 set firewall name V4-INGRESS rule 101 ipsec 'match-ipsec'
-set firewall name V4-EGRESS default-action 'reject'
-set firewall ipv6-name V6-EGRESS default-action 'reject'
-set firewall ipv6-name V6-EGRESS rule 20
-set firewall ipv6-name V6-EGRESS rule 20 icmpv6 type 'echo-request'
\ No newline at end of file
+set firewall name EGRESS default-action 'reject'
+set firewall ipv6-name EGRESS default-action 'reject'
+set firewall ipv6-name EGRESS rule 20
+set firewall ipv6-name EGRESS rule 20 icmpv6 type 'echo-request'
diff --git a/tests/unit/modules/network/vyos/test_vyos_firewall_rules.py b/tests/unit/modules/network/vyos/test_vyos_firewall_rules.py
index f80157c..4be8ec9 100644
--- a/tests/unit/modules/network/vyos/test_vyos_firewall_rules.py
+++ b/tests/unit/modules/network/vyos/test_vyos_firewall_rules.py
@@ -772,7 +772,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 description="This rule-set is configured by Ansible RM",
                             ),
                             dict(
-                                name="V6-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                                 description="This rule-set is configured by Ansible RM",
                                 rules=[
@@ -800,7 +800,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
             "set firewall name V4-INGRESS rule 102 description 'Rule 102 is configured by Ansible RM'",
             "set firewall name V4-INGRESS rule 102",
             "set firewall ipv6-name V6-INGRESS description 'This rule-set is configured by Ansible RM'",
-            "set firewall ipv6-name V6-EGRESS description 'This rule-set is configured by Ansible RM'",
+            "set firewall ipv6-name EGRESS description 'This rule-set is configured by Ansible RM'",
         ]
         self.execute_module(changed=True, commands=commands)
 
@@ -838,7 +838,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 default_action="accept",
                             ),
                             dict(
-                                name="V6-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                                 rules=[
                                     dict(
@@ -883,7 +883,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 ],
                             ),
                             dict(
-                                name="V4-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                             ),
                         ],
@@ -896,7 +896,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 default_action="accept",
                             ),
                             dict(
-                                name="V6-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                                 rules=[
                                     dict(
@@ -970,7 +970,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 ],
                             ),
                             dict(
-                                name="V4-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                             ),
                         ],
@@ -983,7 +983,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 default_action="accept",
                             ),
                             dict(
-                                name="V6-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                                 rules=[
                                     dict(
@@ -1062,9 +1062,9 @@ class TestVyosFirewallRulesModule(TestVyosModule):
         )
         commands = [
             "delete firewall ipv6-name V6-INGRESS",
-            "delete firewall ipv6-name V6-EGRESS",
+            "delete firewall ipv6-name EGRESS",
             "delete firewall name V4-INGRESS",
-            "delete firewall name V4-EGRESS",
+            "delete firewall name EGRESS",
             "set firewall name V4-IN default-action 'accept'",
             "set firewall name V4-IN description 'This is IPv4 INGRESS rule set'",
             "set firewall name V4-IN enable-default-log",
@@ -1115,7 +1115,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 ],
                             ),
                             dict(
-                                name="V4-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                             ),
                         ],
@@ -1128,7 +1128,7 @@ class TestVyosFirewallRulesModule(TestVyosModule):
                                 default_action="accept",
                             ),
                             dict(
-                                name="V6-EGRESS",
+                                name="EGRESS",
                                 default_action="reject",
                                 rules=[
                                     dict(