From bd3f07140d4ebfa62f63d8ad170da5e43d7bbb74 Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Mon, 8 Aug 2016 11:09:24 -0500 Subject: [PATCH] Infer nova-net security groups better If a cloud does not have neutron, it's entirely possible to infer that security groups should go through nova. While it's currently possible to set secgroup_source: nova in clouds.yaml or in a vendor file, it's a little overkill for folks who otherwise do not need to express that level of complexity. Change-Id: I8e4dec2cf993a47c8a0d5cb55bbf80b0fdc4dd46 --- ...nfer-secgroup-source-58d840aaf1a1f485.yaml | 9 ++ shade/openstackcloud.py | 107 ++++++++++-------- shade/tests/unit/test_security_groups.py | 24 ++++ 3 files changed, 91 insertions(+), 49 deletions(-) create mode 100644 releasenotes/notes/infer-secgroup-source-58d840aaf1a1f485.yaml diff --git a/releasenotes/notes/infer-secgroup-source-58d840aaf1a1f485.yaml b/releasenotes/notes/infer-secgroup-source-58d840aaf1a1f485.yaml new file mode 100644 index 000000000..f3f35f480 --- /dev/null +++ b/releasenotes/notes/infer-secgroup-source-58d840aaf1a1f485.yaml @@ -0,0 +1,9 @@ +--- +features: + - If a cloud does not have a neutron service, it is now + assumed that Nova will be the source of security groups. + To handle clouds that have nova-network and do not have + the security group extension, setting secgroup_source to + None will prevent attempting to use them at all. If the + cloud has neutron but it is not a functional source of + security groups, set secgroup_source to nova. diff --git a/shade/openstackcloud.py b/shade/openstackcloud.py index 5bbb2fcc2..39a7465ac 100644 --- a/shade/openstackcloud.py +++ b/shade/openstackcloud.py @@ -1414,7 +1414,7 @@ class OpenStackCloud(object): """ # Don't even try if we're a cloud that doesn't have them - if self.secgroup_source not in ('nova', 'neutron'): + if not self._has_secgroups(): return [] with _utils.shade_exceptions(): @@ -1429,8 +1429,14 @@ class OpenStackCloud(object): :returns: A list of security group ``munch.Munch``. """ + # Security groups not supported + if not self._has_secgroups(): + raise OpenStackCloudUnavailableFeature( + "Unavailable feature: security groups" + ) + # Handle neutron security groups - if self.secgroup_source == 'neutron': + if self._use_neutron_secgroups(): # Neutron returns dicts, so no need to convert objects here. with _utils.neutron_exceptions( "Error fetching security group list"): @@ -1438,18 +1444,12 @@ class OpenStackCloud(object): _tasks.NeutronSecurityGroupList())['security_groups'] # Handle nova security groups - elif self.secgroup_source == 'nova': + else: with _utils.shade_exceptions("Error fetching security group list"): groups = self.manager.submitTask( _tasks.NovaSecurityGroupList()) return _utils.normalize_nova_secgroups(groups) - # Security groups not supported - else: - raise OpenStackCloudUnavailableFeature( - "Unavailable feature: security groups" - ) - def list_servers(self, detailed=False): """List all available servers. @@ -1776,6 +1776,16 @@ class OpenStackCloud(object): return (self.has_service('network') and self._floating_ip_source == 'neutron') + def _has_secgroups(self): + if not self.secgroup_source: + return False + else: + return self.secgroup_source.lower() in ('nova', 'neutron') + + def _use_neutron_secgroups(self): + return (self.has_service('network') + and self.secgroup_source == 'neutron') + def get_keypair(self, name_or_id, filters=None): """Get a keypair by name or ID. @@ -5244,7 +5254,14 @@ class OpenStackCloud(object): :raises: OpenStackCloudUnavailableFeature if security groups are not supported on this cloud. """ - if self.secgroup_source == 'neutron': + + # Security groups not supported + if not self._has_secgroups(): + raise OpenStackCloudUnavailableFeature( + "Unavailable feature: security groups" + ) + + if self._use_neutron_secgroups(): with _utils.neutron_exceptions( "Error creating security group {0}".format(name)): group = self.manager.submitTask( @@ -5255,7 +5272,7 @@ class OpenStackCloud(object): ) return group['security_group'] - elif self.secgroup_source == 'nova': + else: with _utils.shade_exceptions( "Failed to create security group '{name}'".format( name=name)): @@ -5266,12 +5283,6 @@ class OpenStackCloud(object): ) return _utils.normalize_nova_secgroups([group])[0] - # Security groups not supported - else: - raise OpenStackCloudUnavailableFeature( - "Unavailable feature: security groups" - ) - def delete_security_group(self, name_or_id): """Delete a security group @@ -5283,13 +5294,19 @@ class OpenStackCloud(object): :raises: OpenStackCloudUnavailableFeature if security groups are not supported on this cloud. """ + # Security groups not supported + if not self._has_secgroups(): + raise OpenStackCloudUnavailableFeature( + "Unavailable feature: security groups" + ) + secgroup = self.get_security_group(name_or_id) if secgroup is None: self.log.debug('Security group %s not found for deleting' % name_or_id) return False - if self.secgroup_source == 'neutron': + if self._use_neutron_secgroups(): with _utils.neutron_exceptions( "Error deleting security group {0}".format(name_or_id)): self.manager.submitTask( @@ -5299,7 +5316,7 @@ class OpenStackCloud(object): ) return True - elif self.secgroup_source == 'nova': + else: with _utils.shade_exceptions( "Failed to delete security group '{group}'".format( group=name_or_id)): @@ -5308,12 +5325,6 @@ class OpenStackCloud(object): ) return True - # Security groups not supported - else: - raise OpenStackCloudUnavailableFeature( - "Unavailable feature: security groups" - ) - @_utils.valid_kwargs('name', 'description') def update_security_group(self, name_or_id, **kwargs): """Update a security group @@ -5326,13 +5337,19 @@ class OpenStackCloud(object): :raises: OpenStackCloudException on operation error. """ + # Security groups not supported + if not self._has_secgroups(): + raise OpenStackCloudUnavailableFeature( + "Unavailable feature: security groups" + ) + secgroup = self.get_security_group(name_or_id) if secgroup is None: raise OpenStackCloudException( "Security group %s not found." % name_or_id) - if self.secgroup_source == 'neutron': + if self._use_neutron_secgroups(): with _utils.neutron_exceptions( "Error updating security group {0}".format(name_or_id)): group = self.manager.submitTask( @@ -5342,7 +5359,7 @@ class OpenStackCloud(object): ) return group['security_group'] - elif self.secgroup_source == 'nova': + else: with _utils.shade_exceptions( "Failed to update security group '{group}'".format( group=name_or_id)): @@ -5352,12 +5369,6 @@ class OpenStackCloud(object): ) return _utils.normalize_nova_secgroups([group])[0] - # Security groups not supported - else: - raise OpenStackCloudUnavailableFeature( - "Unavailable feature: security groups" - ) - def create_security_group_rule(self, secgroup_name_or_id, port_range_min=None, @@ -5409,13 +5420,18 @@ class OpenStackCloud(object): :raises: OpenStackCloudException on operation error. """ + # Security groups not supported + if not self._has_secgroups(): + raise OpenStackCloudUnavailableFeature( + "Unavailable feature: security groups" + ) secgroup = self.get_security_group(secgroup_name_or_id) if not secgroup: raise OpenStackCloudException( "Security group %s not found." % secgroup_name_or_id) - if self.secgroup_source == 'neutron': + if self._use_neutron_secgroups(): # NOTE: Nova accepts -1 port numbers, but Neutron accepts None # as the equivalent value. rule_def = { @@ -5439,7 +5455,7 @@ class OpenStackCloud(object): ) return rule['security_group_rule'] - elif self.secgroup_source == 'nova': + else: # NOTE: Neutron accepts None for protocol. Nova does not. if protocol is None: raise OpenStackCloudException('Protocol must be specified') @@ -5482,12 +5498,6 @@ class OpenStackCloud(object): ) return _utils.normalize_nova_secgroup_rules([rule])[0] - # Security groups not supported - else: - raise OpenStackCloudUnavailableFeature( - "Unavailable feature: security groups" - ) - def delete_security_group_rule(self, rule_id): """Delete a security group rule @@ -5499,8 +5509,13 @@ class OpenStackCloud(object): :raises: OpenStackCloudUnavailableFeature if security groups are not supported on this cloud. """ + # Security groups not supported + if not self._has_secgroups(): + raise OpenStackCloudUnavailableFeature( + "Unavailable feature: security groups" + ) - if self.secgroup_source == 'neutron': + if self._use_neutron_secgroups(): try: with _utils.neutron_exceptions( "Error deleting security group rule " @@ -5513,7 +5528,7 @@ class OpenStackCloud(object): return False return True - elif self.secgroup_source == 'nova': + else: try: self.manager.submitTask( _tasks.NovaSecurityGroupRuleDelete(rule=rule_id) @@ -5528,12 +5543,6 @@ class OpenStackCloud(object): id=rule_id, msg=str(e))) return True - # Security groups not supported - else: - raise OpenStackCloudUnavailableFeature( - "Unavailable feature: security groups" - ) - def list_zones(self): """List all available zones. diff --git a/shade/tests/unit/test_security_groups.py b/shade/tests/unit/test_security_groups.py index 4012c3206..9bfc1946c 100644 --- a/shade/tests/unit/test_security_groups.py +++ b/shade/tests/unit/test_security_groups.py @@ -55,6 +55,14 @@ nova_grp_dict = meta.obj_to_dict(nova_grp_obj) class TestSecurityGroups(base.TestCase): + def setUp(self): + super(TestSecurityGroups, self).setUp() + self.has_neutron = True + + def fake_has_service(*args, **kwargs): + return self.has_neutron + self.cloud.has_service = fake_has_service + @mock.patch.object(shade.OpenStackCloud, 'neutron_client') @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_list_security_groups_neutron(self, mock_nova, mock_neutron): @@ -67,6 +75,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_list_security_groups_nova(self, mock_nova, mock_neutron): self.cloud.secgroup_source = 'nova' + self.has_neutron = False self.cloud.list_security_groups() self.assertFalse(mock_neutron.list_security_groups.called) self.assertTrue(mock_nova.security_groups.list.called) @@ -75,6 +84,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_list_security_groups_none(self, mock_nova, mock_neutron): self.cloud.secgroup_source = None + self.has_neutron = False self.assertRaises(shade.OpenStackCloudUnavailableFeature, self.cloud.list_security_groups) self.assertFalse(mock_neutron.list_security_groups.called) @@ -93,6 +103,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_delete_security_group_nova(self, mock_nova): self.cloud.secgroup_source = 'nova' + self.has_neutron = False nova_return = [nova_grp_obj] mock_nova.security_groups.list.return_value = nova_return self.cloud.delete_security_group('2') @@ -111,6 +122,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_delete_security_group_nova_not_found(self, mock_nova): self.cloud.secgroup_source = 'nova' + self.has_neutron = False nova_return = [nova_grp_obj] mock_nova.security_groups.list.return_value = nova_return self.cloud.delete_security_group('doesNotExist') @@ -140,6 +152,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_create_security_group_nova(self, mock_nova): group_name = self.getUniqueString() + self.has_neutron = False group_desc = 'security group from test_create_security_group_neutron' new_group = fakes.FakeSecgroup(id='2', name=group_name, @@ -159,6 +172,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_create_security_group_none(self, mock_nova, mock_neutron): self.cloud.secgroup_source = None + self.has_neutron = False self.assertRaises(shade.OpenStackCloudUnavailableFeature, self.cloud.create_security_group, '', '') @@ -178,6 +192,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_update_security_group_nova(self, mock_nova): + self.has_neutron = False new_name = self.getUniqueString() self.cloud.secgroup_source = 'nova' nova_return = [nova_grp_obj] @@ -228,6 +243,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'get_security_group') @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_create_security_group_rule_nova(self, mock_nova, mock_get): + self.has_neutron = False self.cloud.secgroup_source = 'nova' new_rule = fakes.FakeNovaSecgroupRule( @@ -249,6 +265,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_create_security_group_rule_nova_no_ports(self, mock_nova, mock_get): + self.has_neutron = False self.cloud.secgroup_source = 'nova' new_rule = fakes.FakeNovaSecgroupRule( @@ -269,6 +286,7 @@ class TestSecurityGroups(base.TestCase): @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): + self.has_neutron = False self.cloud.secgroup_source = None self.assertRaises(shade.OpenStackCloudUnavailableFeature, self.cloud.create_security_group_rule, @@ -286,6 +304,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_delete_security_group_rule_nova(self, mock_nova): + self.has_neutron = False self.cloud.secgroup_source = 'nova' r = self.cloud.delete_security_group_rule('xyz') mock_nova.security_group_rules.delete.assert_called_once_with( @@ -295,6 +314,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'neutron_client') @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_delete_security_group_rule_none(self, mock_nova, mock_neutron): + self.has_neutron = False self.cloud.secgroup_source = None self.assertRaises(shade.OpenStackCloudUnavailableFeature, self.cloud.delete_security_group_rule, @@ -314,6 +334,7 @@ class TestSecurityGroups(base.TestCase): r = self.cloud.delete_security_group('doesNotExist') self.assertFalse(r) + self.has_neutron = False self.cloud.secgroup_source = 'nova' mock_neutron.security_group_rules.delete.side_effect = ( nova_exc.NotFound("uh oh") @@ -323,6 +344,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_nova_egress_security_group_rule(self, mock_nova): + self.has_neutron = False self.cloud.secgroup_source = 'nova' mock_nova.security_groups.list.return_value = [nova_grp_obj] self.assertRaises(shade.OpenStackCloudException, @@ -333,6 +355,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade._utils, 'normalize_nova_secgroups') @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_list_server_security_groups(self, mock_nova, mock_norm): + self.has_neutron = False server = dict(id='server_id') self.cloud.list_server_security_groups(server) mock_nova.servers.list_security_group.assert_called_once_with( @@ -342,6 +365,7 @@ class TestSecurityGroups(base.TestCase): @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_list_server_security_groups_bad_source(self, mock_nova): + self.has_neutron = False self.cloud.secgroup_source = 'invalid' server = dict(id='server_id') ret = self.cloud.list_server_security_groups(server)