From 42074a67259eb4bfe70631d087fa4fc4d509ab51 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Tue, 4 Apr 2017 17:37:27 -0400 Subject: [PATCH] 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 | 4 ++-- neutron/common/_constants.py | 4 ++++ neutron/db/securitygroups_db.py | 15 +++++++++++++-- .../tests/unit/extensions/test_securitygroup.py | 16 +++++++++++++--- ...ecurity-group-ipv6-icmp-221c59dcaf2caa3c.yaml | 14 ++++++++++++++ 5 files changed, 46 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/security-group-ipv6-icmp-221c59dcaf2caa3c.yaml diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index a10311a309f..1edc42a9d7e 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/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': - rule['protocol'] = 'ipv6-icmp' + if rule.get('protocol') in const.IPV6_ICMP_LEGACY_PROTO_LIST: + rule['protocol'] = constants.PROTO_NAME_IPV6_ICMP ipv6_sg_rules.append(rule) return ipv4_sg_rules, ipv6_sg_rules diff --git a/neutron/common/_constants.py b/neutron/common/_constants.py index 13056a51288..01fd3ebd124 100644 --- a/neutron/common/_constants.py +++ b/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 diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 9eb28dfd198..2f0b0f8d355 100644 --- a/neutron/db/securitygroups_db.py +++ b/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 = [ diff --git a/neutron/tests/unit/extensions/test_securitygroup.py b/neutron/tests/unit/extensions/test_securitygroup.py index 04ff3274e55..a735b15cbc1 100644 --- a/neutron/tests/unit/extensions/test_securitygroup.py +++ b/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" diff --git a/releasenotes/notes/security-group-ipv6-icmp-221c59dcaf2caa3c.yaml b/releasenotes/notes/security-group-ipv6-icmp-221c59dcaf2caa3c.yaml new file mode 100644 index 00000000000..ed58f09ac6e --- /dev/null +++ b/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 `_.