From 3dc539bcb0d9031f81076ac2e1870918400150ed Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 10 Feb 2012 19:01:10 -0500 Subject: [PATCH] Don't allow EC2 removal of security group in use. Fix bug 817872. This patch modifies the behavior of removing security groups via the EC2 API to better match the EC2 API spec. The EC2 documentation says that a group that is still in use can not be removed. A new function has been added to the db API to find out whether a particular security group is still in use. "In use" is defined as applied to an active instance, or applied to another group that has not been deleted. Unit tests have been updated to ensure that an error is raised when these conditions are hit. Change-Id: I5b3fdf1da213b04084fe266c1a6ed92e01cf1e19 --- nova/api/ec2/cloud.py | 2 ++ nova/db/api.py | 5 ++++ nova/db/sqlalchemy/api.py | 35 ++++++++++++++++++++++++++ nova/exception.py | 4 +++ nova/tests/api/ec2/test_cloud.py | 43 ++++++++++++++++++++++++++++++++ nova/tests/test_api.py | 10 ++++++++ nova/tests/test_libvirt.py | 2 ++ smoketests/base.py | 10 ++++++++ smoketests/test_netadmin.py | 3 ++- 9 files changed, 113 insertions(+), 1 deletion(-) 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']))