firewall: T8282: Normalize invalid network-group names during migration

Replace unsupported characters (e.g. `,`, `+`) with `_` when migrating
from 1.3.x and handle name collisions safely to prevent commit failures.

3 files changed, 126 insertions, 14 deletions

diff --git a/smoketest/configs/assert/firewall-groups-name b/smoketest/configs/assert/firewall-groups-name
new file mode 100644
index 000000000..e84ad8fa1
--- /dev/null
+++ b/smoketest/configs/assert/firewall-groups-name
@@ -0,0 +1,18 @@
+set firewall group network-group TEST_GROUP_ network '192.168.1.0/24'
+set firewall group network-group TEST_GROUP__ network '192.168.0.0/24'
+set firewall ipv4 name TEST1 default-action 'drop'
+set firewall ipv4 name TEST1 rule 10 action 'return'
+set firewall ipv4 name TEST1 rule 10 source group network-group 'TEST_GROUP__'
+set firewall ipv4 name TEST1 rule 20 action 'drop'
+set firewall ipv4 name TEST1 rule 20 source group network-group '!TEST_GROUP__'
+set firewall ipv4 name TEST2 default-action 'drop'
+set firewall ipv4 name TEST2 rule 10 action 'return'
+set firewall ipv4 name TEST2 rule 10 source group network-group 'TEST_GROUP_'
+set firewall ipv4 name TEST2 rule 20 action 'drop'
+set firewall ipv4 name TEST2 rule 20 source group network-group '!TEST_GROUP_'
+set interfaces ethernet eth0
+set interfaces loopback lo
+set system console device ttyS0 speed '115200'
+set system host-name 'vyos'
+set system login user vyos authentication encrypted-password '$6$QxPS.uk6mfo$9QBSo8u1FkH16gMyAVhus6fU3LOzvLR9Z9.82m3tiHFAxTtIkhaZSWssSgzt4v4dGAL8rhVQxTg0oAG9/q11h/'
+set system login user vyos authentication plaintext-password ''
\ No newline at end of file
diff --git a/smoketest/configs/firewall-groups-name b/smoketest/configs/firewall-groups-name
new file mode 100644
index 000000000..56282133e
--- /dev/null
+++ b/smoketest/configs/firewall-groups-name
@@ -0,0 +1,75 @@
+firewall {
+    group {
+        network-group TEST_GROUP, {
+            network 192.168.0.0/24
+        }
+        network-group TEST_GROUP+ {
+            network 192.168.1.0/24
+        }
+    }
+    name TEST1 {
+        default-action drop
+        rule 10 {
+            action accept
+            source {
+                group {
+                    network-group TEST_GROUP,
+                }
+            }
+        }
+        rule 20 {
+            action drop
+            source {
+                group {
+                    network-group !TEST_GROUP,
+                }
+            }
+        }
+    }
+    name TEST2 {
+        default-action drop
+        rule 10 {
+            action accept
+            source {
+                group {
+                    network-group TEST_GROUP+
+                }
+            }
+        }
+        rule 20 {
+            action drop
+            source {
+                group {
+                    network-group !TEST_GROUP+
+                }
+            }
+        }
+    }
+}
+interfaces {
+    ethernet eth0 {
+    }
+    loopback lo {
+    }
+}
+system {
+    console {
+        device ttyS0 {
+            speed 115200
+        }
+    }
+    host-name vyos
+    login {
+        user vyos {
+            authentication {
+                encrypted-password $6$QxPS.uk6mfo$9QBSo8u1FkH16gMyAVhus6fU3LOzvLR9Z9.82m3tiHFAxTtIkhaZSWssSgzt4v4dGAL8rhVQxTg0oAG9/q11h/
+                plaintext-password ""
+            }
+        }
+    }
+}
+
+
+// Warning: Do not remove the following line.
+// vyos-config-version: "broadcast-relay@1:cluster@1:config-management@1:conntrack@3:conntrack-sync@2:container@1:dhcp-relay@2:dhcp-server@6:dhcpv6-server@1:dns-forwarding@3:firewall@5:https@2:interfaces@23:ipoe-server@1:ipsec@5:isis@1:l2tp@3:lldp@1:mdns@1:nat@5:ntp@1:pppoe-server@5:pptp@2:qos@1:quagga@8:rpki@1:salt@1:snmp@2:ssh@2:sstp@3:system@21:vrrp@2:vyos-accel-ppp@2:wanloadbalance@3:webproxy@2:zone-policy@1"
+// Release version: 1.3.8
\ No newline at end of file
diff --git a/src/migration-scripts/firewall/6-to-7 b/src/migration-scripts/firewall/6-to-7
index c914a71ee..db83497cd 100644
--- a/src/migration-scripts/firewall/6-to-7
+++ b/src/migration-scripts/firewall/6-to-7
@@ -92,6 +92,24 @@ icmpv6_translations = {
 v4_groups = ["address-group", "network-group", "port-group"]
 v6_groups = ["ipv6-address-group", "ipv6-network-group", "port-group"]
 
+valid_group_name_pattern = re.compile(r'^[A-Za-z0-9_.-]+$')
+invalid_group_name_pattern = re.compile(r'[^A-Za-z0-9_.-]')
+
+def is_valid_group_name(name: str) -> bool:
+    return bool(valid_group_name_pattern.match(name))
+
+def sanitize_group_name(name: str) -> str:
+    """Function to sanitize group name by replacing invalid characters"""
+    return invalid_group_name_pattern.sub('_', name)
+
+def get_unique_group_name(config: ConfigTree, path: list[str], name: str) -> str:
+    """Function to generate a unique group name if the proposed name already exists"""
+
+    new_name = sanitize_group_name(name)
+    while config.exists(path + [new_name]):
+        new_name = f'{new_name}_'
+    return new_name
+
 def migrate(config: ConfigTree) -> None:
     if not config.exists(base):
         # Nothing to do
@@ -108,16 +126,13 @@ def migrate(config: ConfigTree) -> None:
                 if config.exists(name_description):
                     tmp = config.return_value(name_description)
                     config.set(name_description, value=tmp[:max_len_description])
-                if '+' in group_name:
-                    replacement_string = "_"
+
+                if not is_valid_group_name(group_name):
                     if group_type in v4_groups and not v4_found:
                         v4_found = True
                     if group_type in v6_groups and not v6_found:
                         v6_found = True
-                    new_group_name = group_name.replace('+', replacement_string)
-                    while config.exists(base + ['group', group_type, new_group_name]):
-                        replacement_string = replacement_string + "_"
-                        new_group_name = group_name.replace('+', replacement_string)
+                    new_group_name = get_unique_group_name(config, base + ['group', group_type], group_name)
                     translated_dict[group_name] = new_group_name
                     config.copy(base + ['group', group_type, group_name], base + ['group', group_type, new_group_name])
                     config.delete(base + ['group', group_type, group_name])
@@ -185,11 +200,13 @@ def migrate(config: ConfigTree) -> None:
                         if config.exists(base + ['name', name, 'rule', rule, direction, 'group']) and v4_found:
                             for group_type in config.list_nodes(base + ['name', name, 'rule', rule, direction, 'group']):
                                 group_name = config.return_value(base + ['name', name, 'rule', rule, direction, 'group', group_type])
-                                if '+' in group_name:
-                                    if group_name[0] == "!":
-                                        new_group_name = "!" + translated_dict[group_name[1:]]
+
+                                translated_group_name = group_name[1:] if group_name.startswith('!') else group_name
+                                if translated_group_name in translated_dict:
+                                    if group_name.startswith('!'):
+                                        new_group_name = '!' + translated_dict[translated_group_name]
                                     else:
-                                        new_group_name = translated_dict[group_name]
+                                        new_group_name = translated_dict[translated_group_name]
                                     config.set(base + ['name', name, 'rule', rule, direction, 'group', group_type], value=new_group_name)
 
                         pg_base = base + ['name', name, 'rule', rule, direction, 'group', 'port-group']
@@ -288,11 +305,13 @@ def migrate(config: ConfigTree) -> None:
                         if config.exists(base + ['ipv6-name', name, 'rule', rule, direction, 'group']) and v6_found:
                             for group_type in config.list_nodes(base + ['ipv6-name', name, 'rule', rule, direction, 'group']):
                                 group_name = config.return_value(base + ['ipv6-name', name, 'rule', rule, direction, 'group', group_type])
-                                if '+' in group_name:
-                                    if group_name[0] == "!":
-                                        new_group_name = "!" + translated_dict[group_name[1:]]
+
+                                translated_group_name = group_name[1:] if group_name.startswith('!') else group_name
+                                if translated_group_name in translated_dict:
+                                    if group_name.startswith('!'):
+                                        new_group_name = '!' + translated_dict[translated_group_name]
                                     else:
-                                        new_group_name = translated_dict[group_name]
+                                        new_group_name = translated_dict[translated_group_name]
                                     config.set(base + ['ipv6-name', name, 'rule', rule, direction, 'group', group_type], value=new_group_name)
 
                         pg_base = base + ['ipv6-name', name, 'rule', rule, direction, 'group', 'port-group']