Update security group rule if port range is all ports

A security group rule where port_range_min:port_range_max
is 1:65535 is specifying all ports, but it is not optimal
for backends to try and implement this potentially large
rule.

Since it is essentially the entire port range, change
min:max to be None, making the rule specify the entire
protocol instead.

Change-Id: Iff22e2fc84d679e20a5a04b8516750c6ea949078
Closes-bug: #1848213

neutron/db/securitygroups_db.py

@@ -517,6 +517,19 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
                 raise ext_sg.SecurityGroupInvalidProtocolForPort(
                     protocol=ip_proto, valid_port_protocols=port_protocols)
 
+    def _make_canonical_port_range(self, rule):
+        if (rule['port_range_min'] == constants.PORT_RANGE_MIN and
+                rule['port_range_max'] == constants.PORT_RANGE_MAX):
+            LOG.info('Project %(project)s added a security group rule '
+                     'specifying the entire port range (%(min)s - '
+                     '%(max)s). It was automatically converted to not '
+                     'have a range to better optimize it for the backend '
+                     'security group implementation(s).',
+                     {'project': rule['tenant_id'],
+                      'min': rule['port_range_min'],
+                      'max': rule['port_range_max']})
+            rule['port_range_min'] = rule['port_range_max'] = None
+
     def _validate_ethertype_and_protocol(self, rule):
         """Check if given ethertype and  protocol are valid or not"""
         if rule['protocol'] in [constants.PROTO_NAME_IPV6_ENCAP,
@@ -576,6 +589,7 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase,
     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._make_canonical_port_range(rule)
         self._validate_port_range(rule)
         self._validate_ip_prefix(rule)
         self._validate_ethertype_and_protocol(rule)

neutron/tests/fullstack/test_securitygroup.py

@@ -113,13 +113,14 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
              2. connection from allowed security group is permitted
              3. traffic not explicitly allowed (eg. ICMP) is blocked,
              4. a security group update takes effect,
-             5. a remote security group member addition works, and
+             5. a security group update for entire port range works
-             6. an established connection stops by deleting a SG rule.
+             6. a remote security group member addition works, and
-             7. multiple overlapping remote rules work,
+             7. an established connection stops by deleting a SG rule.
-             8. test other protocol functionality by using SCTP protocol
+             8. multiple overlapping remote rules work,
-             9. test two vms with same mac on the same host in different
+             9. test other protocol functionality by using SCTP protocol
-                networks
+             10. test two vms with same mac on the same host in different
-             10. test using multiple security groups
+                 networks
+             11. test using multiple security groups
         """
 
         tenant_uuid = uuidutils.generate_uuid()
@@ -172,7 +173,7 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
             vms[1].namespace, vms[0].namespace, vms[0].ip, 3344,
             net_helpers.NetcatTester.TCP)
 
-        self.safe_client.create_security_group_rule(
+        rule1 = self.safe_client.create_security_group_rule(
             tenant_uuid, sgs[0]['id'],
             remote_group_id=sgs[0]['id'], direction='ingress',
             ethertype=constants.IPv4,
@@ -183,7 +184,35 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
             vms[1].namespace, vms[0].namespace, vms[0].ip, 3344,
             net_helpers.NetcatTester.TCP)
 
-        # 5. check if a remote security group member addition works
+        # 5. check if a security group update for entire port range works
+        self.client.delete_security_group_rule(rule1['id'])
+
+        self.assert_no_connection(
+            vms[1].namespace, vms[0].namespace, vms[0].ip, 3344,
+            net_helpers.NetcatTester.TCP)
+
+        self.assert_no_connection(
+            vms[1].namespace, vms[0].namespace, vms[0].ip, 3366,
+            net_helpers.NetcatTester.TCP)
+
+        rule1 = self.safe_client.create_security_group_rule(
+            tenant_uuid, sgs[0]['id'],
+            remote_group_id=sgs[0]['id'], direction='ingress',
+            ethertype=constants.IPv4,
+            protocol=constants.PROTO_NAME_TCP,
+            port_range_min=1, port_range_max=65535)
+
+        self.assert_connection(
+            vms[1].namespace, vms[0].namespace, vms[0].ip, 3344,
+            net_helpers.NetcatTester.TCP)
+
+        self.assert_connection(
+            vms[1].namespace, vms[0].namespace, vms[0].ip, 3366,
+            net_helpers.NetcatTester.TCP)
+
+        self.client.delete_security_group_rule(rule1['id'])
+
+        # 6. check if a remote security group member addition works
         rule2 = self.safe_client.create_security_group_rule(
             tenant_uuid, sgs[0]['id'],
             remote_group_id=sgs[1]['id'], direction='ingress',
@@ -195,7 +224,7 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
             vms[2].namespace, vms[0].namespace, vms[0].ip, 3355,
             net_helpers.NetcatTester.TCP)
 
-        # 6. check if an established connection stops by deleting
+        # 7. check if an established connection stops by deleting
         #    the supporting SG rule.
         index_to_host.append(index_to_host[2])
         self.index_to_sg.append(1)
@@ -230,7 +259,7 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
                                      sleep=8)
         netcat.stop_processes()
 
-        # 7. check if multiple overlapping remote rules work
+        # 8. check if multiple overlapping remote rules work
         self.safe_client.create_security_group_rule(
             tenant_uuid, sgs[0]['id'],
             remote_group_id=sgs[1]['id'], direction='ingress',
@@ -253,7 +282,7 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
                 vms[3].namespace, vms[0].namespace, vms[0].ip, 8080,
                 net_helpers.NetcatTester.TCP)
 
-        # 8. check SCTP is supported by security group
+        # 9. check SCTP is supported by security group
         self.assert_no_connection(
             vms[1].namespace, vms[0].namespace, vms[0].ip, 3366,
             net_helpers.NetcatTester.SCTP)
@@ -269,10 +298,10 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest):
             vms[1].namespace, vms[0].namespace, vms[0].ip, 3366,
             net_helpers.NetcatTester.SCTP)
 
-        # 9. test two vms with same mac on the same host in different networks
+        # 10. test two vms with same mac on the same host in different networks
         self._test_overlapping_mac_addresses()
 
-        # 10. Check using multiple security groups
+        # 11. Check using multiple security groups
         self._test_using_multiple_security_groups()
 
     def _test_using_multiple_security_groups(self):

neutron/tests/unit/extensions/test_securitygroup.py

@@ -967,6 +967,27 @@ class TestSecurityGroups(SecurityGroupDBTestCase):
                     for k, v, in keys:
                         self.assertEqual(v, rule['security_group_rule'][k])
 
+    def test_create_security_group_rule_port_range_min_max_limits(self):
+        name = 'webservers'
+        description = 'my webservers'
+        with self.security_group(name, description) as sg:
+            security_group_id = sg['security_group']['id']
+            direction = "ingress"
+            protocol = const.PROTO_NAME_TCP
+            port_range_min = const.PORT_RANGE_MIN
+            port_range_max = const.PORT_RANGE_MAX
+            # The returned rule should have port range min/max as None
+            keys = [('security_group_id', security_group_id),
+                    ('direction', direction),
+                    ('protocol', protocol),
+                    ('port_range_min', None),
+                    ('port_range_max', None)]
+            with self.security_group_rule(security_group_id, direction,
+                                          protocol, port_range_min,
+                                          port_range_max) as rule:
+                for k, v, in keys:
+                    self.assertEqual(v, rule['security_group_rule'][k])
+
     def test_create_security_group_rule_icmp_with_type_and_code(self):
         name = 'webservers'
         description = 'my webservers'

releasenotes/notes/security-group-rule-all-ports-update-2857d80e5742ebc5.yaml

@@ -0,0 +1,10 @@
+---
+upgrade:
+  - |
+    A security group rule added for the entire port range, for example,
+    TCP ports 1-65535, is not optimal for backends that implement the
+    rule. Rules like this will now automatically be converted to apply
+    to the procotol itself, in other words, all TCP - the port ranges
+    will be ignored.
+    See bug `1848213 <https://bugs.launchpad.net/neutron/+bug/1848213>`_
+    for more details.