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

@@ -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

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)

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()

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.")

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, {})

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)

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'])
 

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):

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']))