Canonicalize IPv6 ICMP protocol name in security groups

Currently, 'icmp', 'ipv6-icmp' and 'icmpv6' can be
specified as an IPv6 ICMP protocol value.  This can
lead to duplicate entries in the DB for doing exactly
the same thing.

Change to always be 'ipv6-icmp' so this doesn't happen.

Existing rules using one of the old values will now be
returned with 'ipv6-icmp' as the protocol value.

Depends-on: https://review.opendev.org/660206
Depends-on: https://review.opendev.org/660387

Change-Id: I7cd146691dce1a690e1d2c309dfd54b4a0032f76
Partial-Bug: #1582500

neutron/agent/linux/iptables_firewall.py

@@ -400,8 +400,8 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
             if rule.get('ethertype') == constants.IPv4:
                 ipv4_sg_rules.append(rule)
             elif rule.get('ethertype') == constants.IPv6:
-                if rule.get('protocol') == 'icmp':
+                if rule.get('protocol') in const.IPV6_ICMP_LEGACY_PROTO_LIST:
-                    rule['protocol'] = 'ipv6-icmp'
+                    rule['protocol'] = constants.PROTO_NAME_IPV6_ICMP
                 ipv6_sg_rules.append(rule)
         return ipv4_sg_rules, ipv6_sg_rules
 

neutron/common/_constants.py

@@ -41,6 +41,10 @@ IPTABLES_MULTIPORT_ONLY_PROTOCOLS = [
     constants.PROTO_NAME_UDPLITE
 ]
 
+# Legacy IPv6 ICMP protocol list
+IPV6_ICMP_LEGACY_PROTO_LIST = [constants.PROTO_NAME_ICMP,
+                               constants.PROTO_NAME_IPV6_ICMP_LEGACY]
+
 # Number of resources for neutron agent side functions to deal
 # with large sets.
 # Setting this value does not count on special conditions, it is just a human

neutron/db/securitygroups_db.py

@@ -445,10 +445,14 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
             protocol = constants.IP_PROTOCOL_NAME_ALIASES[protocol]
         return int(constants.IP_PROTOCOL_MAP.get(protocol, protocol))
 
-    def _get_ip_proto_name_and_num(self, protocol):
+    def _get_ip_proto_name_and_num(self, protocol, ethertype=None):
         if protocol is None:
             return
         protocol = str(protocol)
+        # Force all legacy IPv6 ICMP protocol names to be 'ipv6-icmp'
+        if (ethertype == constants.IPv6 and
+                protocol in const.IPV6_ICMP_LEGACY_PROTO_LIST):
+            protocol = constants.PROTO_NAME_IPV6_ICMP
         if protocol in constants.IP_PROTOCOL_MAP:
             return [protocol, str(constants.IP_PROTOCOL_MAP.get(protocol))]
         elif protocol in constants.IP_PROTOCOL_NUM_TO_NAME_MAP:
@@ -540,8 +544,14 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
                 raise ext_sg.SecurityGroupRulesNotSingleTenant()
         return sg_groups.pop()
 
+    def _make_canonical_ipv6_icmp_protocol(self, rule):
+        if (rule.get('ethertype') == constants.IPv6 and
+                rule.get('protocol') in const.IPV6_ICMP_LEGACY_PROTO_LIST):
+            rule['protocol'] = constants.PROTO_NAME_IPV6_ICMP
+
     def _validate_security_group_rule(self, context, security_group_rule):
         rule = security_group_rule['security_group_rule']
+        self._make_canonical_ipv6_icmp_protocol(rule)
         self._validate_port_range(rule)
         self._validate_ip_prefix(rule)
         self._validate_ethertype_and_protocol(rule)
@@ -615,7 +625,8 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
             elif value is None:
                 return none_char
             elif key == 'protocol':
-                return str(self._get_ip_proto_name_and_num(value))
+                return str(self._get_ip_proto_name_and_num(
+                               value, ethertype=rule.get('ethertype')))
             return str(value)
 
         comparison_keys = [

neutron/tests/unit/extensions/test_securitygroup.py

@@ -519,7 +519,7 @@ class TestSecurityGroups(SecurityGroupDBTestCase):
         with self.security_group(name, description) as sg:
             security_group_id = sg['security_group']['id']
             rule = self._build_security_group_rule(
-                security_group_id, 'ingress', const.PROTO_NAME_IPV6_ICMP)
+                security_group_id, 'ingress', const.PROTO_NAME_IPV6_FRAG)
             res = self._create_security_group_rule(self.fmt, rule)
             self.deserialize(self.fmt, res)
             self.assertEqual(webob.exc.HTTPBadRequest.code, res.status_int)
@@ -1049,7 +1049,7 @@ class TestSecurityGroups(SecurityGroupDBTestCase):
                 for k, v, in keys:
                     self.assertEqual(rule['security_group_rule'][k], v)
 
-    def test_create_security_group_rule_icmpv6_legacy_protocol_name(self):
+    def _test_create_security_group_rule_legacy_protocol_name(self, protocol):
         name = 'webservers'
         description = 'my webservers'
         with self.security_group(name, description) as sg:
@@ -1057,7 +1057,6 @@ class TestSecurityGroups(SecurityGroupDBTestCase):
             direction = "ingress"
             ethertype = const.IPv6
             remote_ip_prefix = "2001::f401:56ff:fefe:d3dc/128"
-            protocol = const.PROTO_NAME_IPV6_ICMP_LEGACY
             keys = [('remote_ip_prefix', remote_ip_prefix),
                     ('security_group_id', security_group_id),
                     ('direction', direction),
@@ -1069,8 +1068,19 @@ class TestSecurityGroups(SecurityGroupDBTestCase):
                                           None, None,
                                           ethertype) as rule:
                 for k, v, in keys:
+                    # IPv6 ICMP protocol will always be 'ipv6-icmp'
+                    if k == 'protocol':
+                        v = const.PROTO_NAME_IPV6_ICMP
                     self.assertEqual(rule['security_group_rule'][k], v)
 
+    def test_create_security_group_rule_ipv6_icmp_legacy_protocol_name(self):
+        protocol = const.PROTO_NAME_ICMP
+        self._test_create_security_group_rule_legacy_protocol_name(protocol)
+
+    def test_create_security_group_rule_icmpv6_legacy_protocol_name(self):
+        protocol = const.PROTO_NAME_IPV6_ICMP_LEGACY
+        self._test_create_security_group_rule_legacy_protocol_name(protocol)
+
     def test_create_security_group_source_group_ip_and_ip_prefix(self):
         security_group_id = "4cd70774-cc67-4a87-9b39-7d1db38eb087"
         direction = "ingress"

releasenotes/notes/security-group-ipv6-icmp-221c59dcaf2caa3c.yaml

@@ -0,0 +1,14 @@
+---
+upgrade:
+  - |
+    Existing IPv6 ICMP security group rules created by using legacy protocol
+    names ``icmpv6`` and ``icmp`` will now be returned as ``ipv6-icmp`` in
+    an API GET call.
+fixes:
+  - |
+    Security group rule code has been changed to better detect duplicate
+    rules by standardizing on ``ipv6-icmp`` as the protocol field value
+    for IPv6 ICMP rules.  The legacy names ``icmpv6`` and ``icmp`` can still
+    be used in API POST calls, but API GET calls will return ``ipv6-icmp``.
+    Partial fix for bug
+    `1582500 <https://bugs.launchpad.net/neutron/+bug/1582500>`_.