diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py index 560a2d0ddaf4..50732d08681a 100644 --- a/nova/api/ec2/cloud.py +++ b/nova/api/ec2/cloud.py @@ -763,6 +763,8 @@ class CloudController(object): security_group = db.security_group_get(context, group_id) if not security_group: raise notfound(security_group_id=group_id) + if db.security_group_in_use(context, security_group.id): + raise exception.InvalidGroup(reason="In Use") LOG.audit(_("Delete security group %s"), group_name, context=context) db.security_group_destroy(context, security_group.id) return True diff --git a/nova/db/api.py b/nova/db/api.py index bd80575325f8..69fe589c1762 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -1168,6 +1168,11 @@ def security_group_exists(context, project_id, group_name): return IMPL.security_group_exists(context, project_id, group_name) +def security_group_in_use(context, group_id): + """Indicates if a security group is currently in use.""" + return IMPL.security_group_in_use(context, group_id) + + def security_group_create(context, values): """Create a new security group.""" return IMPL.security_group_create(context, values) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index ed9286eff4ab..fe94894df098 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -2816,6 +2816,41 @@ def security_group_exists(context, project_id, group_name): return False +@require_context +def security_group_in_use(context, group_id): + session = get_session() + with session.begin(): + # Are there any other groups that haven't been deleted + # that include this group in their rules? + rules = session.query(models.SecurityGroupIngressRule).\ + filter_by(group_id=group_id).\ + filter_by(deleted=False).\ + all() + for r in rules: + num_groups = session.query(models.SecurityGroup).\ + filter_by(deleted=False).\ + filter_by(id=r.parent_group_id).\ + count() + if num_groups: + return True + + # Are there any instances that haven't been deleted + # that include this group? + inst_assoc = session.query(models.SecurityGroupInstanceAssociation).\ + filter_by(security_group_id=group_id).\ + filter_by(deleted=False).\ + all() + for ia in inst_assoc: + num_instances = session.query(models.Instance).\ + filter_by(deleted=False).\ + filter_by(id=ia.instance_id).\ + count() + if num_instances: + return True + + return False + + @require_context def security_group_create(context, values): security_group_ref = models.SecurityGroup() diff --git a/nova/exception.py b/nova/exception.py index dfa20dd2546f..a6941549c7d9 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -294,6 +294,10 @@ class InvalidAggregateAction(Invalid): "%(aggregate_id)s. Reason: %(reason)s.") +class InvalidGroup(Invalid): + message = _("Group not valid. Reason: %(reason)s") + + class InstanceInvalidState(Invalid): message = _("Instance %(instance_uuid)s in %(attr)s %(state)s. Cannot " "%(method)s while the instance is in this state.") diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py index 1680881509c3..e9d21d7fd18a 100644 --- a/nova/tests/api/ec2/test_cloud.py +++ b/nova/tests/api/ec2/test_cloud.py @@ -437,6 +437,49 @@ class CloudTestCase(test.TestCase): self.assertRaises(exception.EC2APIError, revoke, self.context, **kwargs) + def test_delete_security_group_in_use_by_group(self): + group1 = self.cloud.create_security_group(self.context, 'testgrp1', + "test group 1") + group2 = self.cloud.create_security_group(self.context, 'testgrp2', + "test group 2") + kwargs = {'groups': {'1': {'user_id': u'%s' % self.context.user_id, + 'group_name': u'testgrp2'}}, + } + self.cloud.authorize_security_group_ingress(self.context, + group_name='testgrp1', **kwargs) + + self.assertRaises(exception.InvalidGroup, + self.cloud.delete_security_group, + self.context, 'testgrp2') + self.cloud.delete_security_group(self.context, 'testgrp1') + self.cloud.delete_security_group(self.context, 'testgrp2') + + def test_delete_security_group_in_use_by_instance(self): + """Ensure that a group can not be deleted if in use by an instance.""" + image_uuid = 'cedef40a-ed67-4d10-800e-17455edce175' + args = {'reservation_id': 'a', + 'image_ref': image_uuid, + 'instance_type_id': 1, + 'host': 'host1', + 'vm_state': 'active'} + inst = db.instance_create(self.context, args) + + args = {'user_id': self.context.user_id, + 'project_id': self.context.project_id, + 'name': 'testgrp', + 'description': 'Test group'} + group = db.security_group_create(self.context, args) + + db.instance_add_security_group(self.context, inst.uuid, group.id) + + self.assertRaises(exception.InvalidGroup, + self.cloud.delete_security_group, + self.context, 'testgrp') + + db.instance_destroy(self.context, inst.id) + + self.cloud.delete_security_group(self.context, 'testgrp') + def test_describe_volumes(self): """Makes sure describe_volumes works and filters results.""" vol1 = db.volume_create(self.context, {}) diff --git a/nova/tests/test_api.py b/nova/tests/test_api.py index f7548ceb48c9..a6bc737ddedf 100644 --- a/nova/tests/test_api.py +++ b/nova/tests/test_api.py @@ -570,6 +570,15 @@ class ApiEc2TestCase(test.TestCase): self.expect_http() self.mox.ReplayAll() + # Can not delete the group while it is still used by + # another group. + self.assertRaises(EC2ResponseError, + self.ec2.delete_security_group, + other_security_group_name) + + self.expect_http() + self.mox.ReplayAll() + rv = self.ec2.get_all_security_groups() for group in rv: @@ -583,3 +592,4 @@ class ApiEc2TestCase(test.TestCase): self.mox.ReplayAll() self.ec2.delete_security_group(security_group_name) + self.ec2.delete_security_group(other_security_group_name) diff --git a/nova/tests/test_libvirt.py b/nova/tests/test_libvirt.py index 66a8db2b5598..b6135bc8fa99 100644 --- a/nova/tests/test_libvirt.py +++ b/nova/tests/test_libvirt.py @@ -1753,6 +1753,8 @@ class NWFilterTestCase(test.TestCase): self.fw.prepare_instance_filter(instance, network_info) self.fw.apply_instance_filter(instance, network_info) _ensure_all_called(mac) + db.instance_remove_security_group(self.context, inst_uuid, + self.security_group.id) self.teardown_security_group() db.instance_destroy(context.get_admin_context(), instance_ref['id']) diff --git a/smoketests/base.py b/smoketests/base.py index 31d82b20baab..db28b6f5a671 100644 --- a/smoketests/base.py +++ b/smoketests/base.py @@ -72,6 +72,16 @@ class SmokeTestCase(unittest.TestCase): else: return False + def wait_for_not_running(self, instance, tries=60, wait=1): + """Wait for instance to not be running""" + for x in xrange(tries): + instance.update() + if not instance.state.startswith('running'): + return True + time.sleep(wait) + else: + return False + def wait_for_ping(self, ip, command="ping", tries=120): """Wait for ip to be pingable""" for x in xrange(tries): diff --git a/smoketests/test_netadmin.py b/smoketests/test_netadmin.py index d097d232a7c7..6a0dc48ec5cb 100644 --- a/smoketests/test_netadmin.py +++ b/smoketests/test_netadmin.py @@ -195,8 +195,9 @@ class SecurityGroupTests(base.UserSmokeTestCase): def test_999_tearDown(self): self.conn.disassociate_address(self.data['public_ip']) self.conn.delete_key_pair(TEST_KEY) + self.conn.terminate_instances([self.data['instance'].id]) + self.wait_for_not_running(self.data['instance']) self.conn.delete_security_group(TEST_GROUP) groups = self.conn.get_all_security_groups() self.assertFalse(TEST_GROUP in [group.name for group in groups]) - self.conn.terminate_instances([self.data['instance'].id]) self.assertTrue(self.conn.release_address(self.data['public_ip']))