From 01aeceeae988262c2c5c2de7041ff9483fe5a855 Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Tue, 30 Jun 2015 16:35:55 -0400 Subject: [PATCH] Modify secgroup rule processing It turns out that Neutron can accept None for a rule protocol to represent all protocols. Nova has no similar concept for a single rule, so let's throw an exception. Also, Neutron relaxes the rules on accepting None for both port values to represent the full port range for TCP and UDP. For ICMP, it is the same as -1. Change-Id: Icc408586804e2a538399a2aa3f740b97bb4c5246 --- shade/__init__.py | 28 +++++++++++++++++++----- shade/tests/unit/test_security_groups.py | 27 ++++++++++++++++++++--- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/shade/__init__.py b/shade/__init__.py index 41c4c5d45..5d4510531 100644 --- a/shade/__init__.py +++ b/shade/__init__.py @@ -3100,12 +3100,28 @@ class OpenStackCloud(object): return rule['security_group_rule'] elif self.secgroup_source == 'nova': - # NOTE: Neutron accepts None for ports, but Nova accepts -1 - # as the equivalent value. - if port_range_min is None: - port_range_min = -1 - if port_range_max is None: - port_range_max = -1 + # NOTE: Neutron accepts None for protocol. Nova does not. + if protocol is None: + raise OpenStackCloudException('Protocol must be specified') + + # NOTE: Neutron accepts None for ports, but Nova requires -1 + # as the equivalent value for ICMP. + # + # For TCP/UDP, if both are None, Neutron allows this and Nova + # represents this as all ports (1-65535). Nova does not accept + # None values, so to hide this difference, we will automatically + # convert to the full port range. If only a single port value is + # specified, it will error as normal. + if protocol == 'icmp': + if port_range_min is None: + port_range_min = -1 + if port_range_max is None: + port_range_max = -1 + elif protocol in ['tcp', 'udp']: + if port_range_min is None and port_range_max is None: + port_range_min = 1 + port_range_max = 65535 + try: rule = meta.obj_to_dict( self.manager.submitTask( diff --git a/shade/tests/unit/test_security_groups.py b/shade/tests/unit/test_security_groups.py index 33ea7090b..37179a6d5 100644 --- a/shade/tests/unit/test_security_groups.py +++ b/shade/tests/unit/test_security_groups.py @@ -235,20 +235,41 @@ class TestSecurityGroups(base.TestCase): self.cloud.secgroup_source = 'nova' new_rule = fakes.FakeNovaSecgroupRule( - id='xyz', from_port=-1, to_port=2000, ip_protocol='tcp', + id='xyz', from_port=1, to_port=2000, ip_protocol='tcp', cidr='1.2.3.4/32') mock_nova.security_group_rules.create.return_value = new_rule mock_get.return_value = {'id': 'abc'} self.cloud.create_security_group_rule( - 'abc', port_range_max=2000, protocol='tcp', + 'abc', port_range_min=1, port_range_max=2000, protocol='tcp', remote_ip_prefix='1.2.3.4/32', remote_group_id='123') mock_nova.security_group_rules.create.assert_called_once_with( - parent_group_id='abc', ip_protocol='tcp', from_port=-1, + parent_group_id='abc', ip_protocol='tcp', from_port=1, to_port=2000, cidr='1.2.3.4/32', group_id='123' ) + @mock.patch.object(shade.OpenStackCloud, 'get_security_group') + @mock.patch.object(shade.OpenStackCloud, 'nova_client') + def test_create_security_group_rule_nova_no_ports(self, + mock_nova, mock_get): + self.cloud.secgroup_source = 'nova' + + new_rule = fakes.FakeNovaSecgroupRule( + id='xyz', from_port=1, to_port=65535, ip_protocol='tcp', + cidr='1.2.3.4/32') + mock_nova.security_group_rules.create.return_value = new_rule + mock_get.return_value = {'id': 'abc'} + + self.cloud.create_security_group_rule( + 'abc', protocol='tcp', + remote_ip_prefix='1.2.3.4/32', remote_group_id='123') + + mock_nova.security_group_rules.create.assert_called_once_with( + parent_group_id='abc', ip_protocol='tcp', from_port=1, + to_port=65535, cidr='1.2.3.4/32', group_id='123' + ) + @mock.patch.object(shade.OpenStackCloud, 'neutron_client') @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_create_security_group_rule_none(self, mock_nova, mock_neutron):