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

9 files changed, 113 insertions, 1 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index 560a2d0dd..50732d086 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 bd8057532..69fe589c1 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 ed9286eff..fe94894df 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -2817,6 +2817,41 @@ def security_group_exists(context, project_id, group_name):
 
 
 @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()
     # FIXME(devcamcar): Unless I do this, rules fails with lazy load exception
diff --git a/nova/exception.py b/nova/exception.py
index dfa20dd25..a6941549c 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 168088150..e9d21d7fd 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 f7548ceb4..a6bc737dd 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 66a8db2b5..b6135bc8f 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 31d82b20b..db28b6f5a 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 d097d232a..6a0dc48ec 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']))