Add unique constraint for security groups

- Add unique constraint (project_id, name, deleted)
  for security_groups
- Define new SecurityGroupExists exception
- Drop security_group_exists() in favor of catching
  SecurityGroupExists exception for security_group_create()

Fixes bug #1093458

Blueprint: db-enforce-unique-keys

Change-Id: Id9a1427587cc9250942d0abd9eafc69861352d9b

nova/cloudpipe/pipelib.py

@@ -142,13 +142,14 @@ def launch_vpn_instance(self, context):
 
     def setup_security_group(self, context):
         group_name = '%s%s' % (context.project_id, CONF.vpn_key_suffix)
-        if db.security_group_exists(context, context.project_id, group_name):
-            return group_name
         group = {'user_id': context.user_id,
                  'project_id': context.project_id,
                  'name': group_name,
                  'description': 'Group for vpn'}
-        group_ref = db.security_group_create(context, group)
+        try:
+            group_ref = db.security_group_create(context, group)
+        except exception.SecurityGroupExists:
+            return group_name
         rule = {'parent_group_id': group_ref['id'],
                 'cidr': '0.0.0.0/0',
                 'protocol': 'udp',

nova/compute/api.py

@@ -2952,16 +2952,15 @@ def create_security_group(self, context, name, description):
         try:
             self.ensure_default(context)
 
-            if self.db.security_group_exists(context,
-                                             context.project_id, name):
-                msg = _('Security group %s already exists') % name
-                self.raise_group_already_exists(msg)
-
             group = {'user_id': context.user_id,
                      'project_id': context.project_id,
                      'name': name,
                      'description': description}
-            group_ref = self.db.security_group_create(context, group)
+            try:
+                group_ref = self.db.security_group_create(context, group)
+            except exception.SecurityGroupExists:
+                msg = _('Security group %s already exists') % name
+                self.raise_group_already_exists(msg)
             # Commit the reservation
             QUOTAS.commit(context, reservations)
         except Exception:

nova/db/api.py

@@ -1172,11 +1172,6 @@ def security_group_get_by_instance(context, instance_uuid):
     return IMPL.security_group_get_by_instance(context, instance_uuid)
 
 
-def security_group_exists(context, project_id, group_name):
-    """Indicates if a group name exists in a project."""
-    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)

nova/db/sqlalchemy/api.py

@@ -3213,7 +3213,12 @@ def _security_group_create(context, values, session=None):
     # once save() is called.  This will get cleaned up in next orm pass.
     security_group_ref.rules
     security_group_ref.update(values)
-    security_group_ref.save(session=session)
+    try:
+        security_group_ref.save(session=session)
+    except db_exc.DBDuplicateEntry:
+        raise exception.SecurityGroupExists(
+                project_id=values['project_id'],
+                security_group_name=values['name'])
     return security_group_ref
 
 
@@ -3307,15 +3312,6 @@ def security_group_get_by_instance(context, instance_uuid):
                    all()
 
 
-@require_context
-def security_group_exists(context, project_id, group_name):
-    try:
-        group = security_group_get_by_name(context, project_id, group_name)
-        return group is not None
-    except exception.NotFound:
-        return False
-
-
 @require_context
 def security_group_in_use(context, group_id):
     session = get_session()
@@ -3356,7 +3352,15 @@ def security_group_update(context, security_group_id, values):
             raise exception.SecurityGroupNotFound(
                     security_group_id=security_group_id)
         security_group_ref.update(values)
-        return security_group_ref
+        name = security_group_ref['name']
+        project_id = security_group_ref['project_id']
+        try:
+            security_group_ref.save(session=session)
+        except db_exc.DBDuplicateEntry:
+            raise exception.SecurityGroupExists(
+                    project_id=project_id,
+                    security_group_name=name)
+    return security_group_ref
 
 
 def security_group_ensure_default(context):

nova/db/sqlalchemy/migrate_repo/versions/190_add_security_group_uc.py

@@ -0,0 +1,40 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2013 Boris Pavlovic (boris@pavlovic.me).
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from migrate.changeset import UniqueConstraint
+from sqlalchemy import MetaData, Table
+
+from nova.db.sqlalchemy import utils
+
+
+UC_NAME = "uniq_security_groups0project_id0name0deleted"
+COLUMNS = ('project_id', 'name', 'deleted')
+TABLE_NAME = 'security_groups'
+
+
+def upgrade(migrate_engine):
+    meta = MetaData(bind=migrate_engine)
+    t = Table(TABLE_NAME, meta, autoload=True)
+
+    utils.drop_old_duplicate_entries_from_table(migrate_engine, TABLE_NAME,
+                                                True, *COLUMNS)
+    uc = UniqueConstraint(*COLUMNS, table=t, name=UC_NAME)
+    uc.create()
+
+
+def downgrade(migrate_engine):
+    utils.drop_unique_constraint(migrate_engine, TABLE_NAME, UC_NAME, *COLUMNS)

nova/exception.py

@@ -741,6 +741,11 @@ class SecurityGroupNotFoundForRule(SecurityGroupNotFound):
     message = _("Security group with rule %(rule_id)s not found.")
 
 
+class SecurityGroupExists(Invalid):
+    message = _("Security group %(security_group_name)s already exists "
+                "for project %(project_id)s.")
+
+
 class SecurityGroupExistsForInstance(Invalid):
     message = _("Security group %(security_group_id)s is already associated"
                 " with the instance %(instance_id)s")

nova/tests/api/openstack/compute/contrib/test_cloudpipe.py

@@ -19,7 +19,6 @@
 from nova.api.openstack.compute.contrib import cloudpipe
 from nova.api.openstack import wsgi
 from nova.compute import utils as compute_utils
-from nova import db
 from nova.openstack.common import timeutils
 from nova import test
 from nova.tests.api.openstack import fakes
@@ -47,11 +46,6 @@ def compute_api_get_all(context, search_opts=None):
         return [fake_vpn_instance()]
 
 
-def db_security_group_exists(context, project_id, group_name):
-    # used in pipelib
-    return True
-
-
 def utils_vpn_ping(addr, port, timoeout=0.05, session_id=None):
     return True
 
@@ -63,8 +57,6 @@ def setUp(self):
         self.controller = cloudpipe.CloudpipeController()
         self.stubs.Set(self.controller.compute_api, "get_all",
                        compute_api_get_all_empty)
-        self.stubs.Set(db, "security_group_exists",
-                       db_security_group_exists)
         self.stubs.Set(utils, 'vpn_ping', utils_vpn_ping)
 
     def test_cloudpipe_list_no_network(self):

nova/tests/db/test_db_api.py

@@ -1117,8 +1117,8 @@ def test_security_group_rule_get_by_security_group_grantee(self):
         self.assertEqual(rules[0]['id'], security_group_rule['id'])
 
     def test_security_group_rule_destroy(self):
-        security_group1 = self._create_security_group({})
-        security_group2 = self._create_security_group({})
+        security_group1 = self._create_security_group({'name': 'fake1'})
+        security_group2 = self._create_security_group({'name': 'fake2'})
         security_group_rule1 = self._create_security_group_rule({})
         security_group_rule2 = self._create_security_group_rule({})
         db.security_group_rule_destroy(self.ctxt, security_group_rule1['id'])
@@ -1147,8 +1147,8 @@ def test_security_group_rule_get_not_found_exception(self):
                           db.security_group_rule_get, self.ctxt, 100500)
 
     def test_security_group_rule_count_by_group(self):
-        sg1 = self._create_security_group({})
-        sg2 = self._create_security_group({})
+        sg1 = self._create_security_group({'name': 'fake1'})
+        sg2 = self._create_security_group({'name': 'fake2'})
         rules_by_group = {sg1: [], sg2: []}
         for group in rules_by_group:
             rules = rules_by_group[group]
@@ -1299,19 +1299,6 @@ def test_security_group_get_all(self):
         self._assertEqualListsOfObjects(security_groups, real,
                                         ignored_keys=['instances'])
 
-    def test_security_group_exists(self):
-        security_group = self._create_security_group(
-                {'name': 'fake1', 'project_id': 'fake_proj1'})
-
-        real = (db.security_group_exists(self.ctxt,
-                                         security_group['project_id'],
-                                         security_group['name']),
-                db.security_group_exists(self.ctxt,
-                                         security_group['project_id'],
-                                         'fake_sec_group'))
-
-        self.assertEqual((True, False), real)
-
     def test_security_group_count_by_project(self):
         values = [
             {'name': 'fake1', 'project_id': 'fake_proj1'},
@@ -1331,7 +1318,8 @@ def test_security_group_count_by_project(self):
     def test_security_group_in_use(self):
         instance = db.instance_create(self.ctxt, dict(host='foo'))
         values = [
-            {'instances': [instance]},
+            {'instances': [instance],
+             'name': 'fake_in_use'},
             {'instances': []},
         ]
 
@@ -1348,15 +1336,29 @@ def test_security_group_in_use(self):
         self.assertEquals(expected, real)
 
     def test_security_group_ensure_default(self):
-        self.assertFalse(db.security_group_exists(self.ctxt,
-                                                  self.ctxt.project_id,
-                                                  'default'))
+        self.assertEquals(0, len(db.security_group_get_by_project(
+                                    self.ctxt,
+                                    self.ctxt.project_id)))
 
         db.security_group_ensure_default(self.ctxt)
 
-        self.assertTrue(db.security_group_exists(self.ctxt,
-                                                 self.ctxt.project_id,
-                                                 'default'))
+        security_groups = db.security_group_get_by_project(
+                            self.ctxt,
+                            self.ctxt.project_id)
+
+        self.assertEquals(1, len(security_groups))
+        self.assertEquals("default", security_groups[0]["name"])
+
+    def test_security_group_update_to_duplicate(self):
+        security_group1 = self._create_security_group(
+                {'name': 'fake1', 'project_id': 'fake_proj1'})
+        security_group2 = self._create_security_group(
+                {'name': 'fake1', 'project_id': 'fake_proj2'})
+
+        self.assertRaises(exception.SecurityGroupExists,
+                          db.security_group_update,
+                          self.ctxt, security_group2['id'],
+                          {'project_id': 'fake_proj1'})
 
 
 class InstanceTestCase(DbTestCase):

nova/tests/db/test_migrations.py

@@ -1669,6 +1669,36 @@ def get_(name, deleted):
                           cells.insert().execute,
                           {'name': 'name_123', 'deleted': 0})
 
+    def _pre_upgrade_190(self, engine):
+        security_groups = db_utils.get_table(engine, 'security_groups')
+        data = [
+            {'name': 'group1', 'project_id': 'fake', 'deleted': 0},
+            {'name': 'group1', 'project_id': 'fake', 'deleted': 0},
+            {'name': 'group2', 'project_id': 'fake', 'deleted': 0},
+        ]
+
+        for item in data:
+            security_groups.insert().values(item).execute()
+
+    def _check_190(self, engine, data):
+        security_groups = db_utils.get_table(engine, 'security_groups')
+
+        def get_(name, project_id, deleted):
+            deleted_value = 0 if not deleted else security_groups.c.id
+            return security_groups.select().\
+                        where(security_groups.c.name == name).\
+                        where(security_groups.c.project_id == project_id).\
+                        where(security_groups.c.deleted == deleted_value).\
+                        execute().\
+                        fetchall()
+
+        self.assertEqual(1, len(get_('group1', 'fake', False)))
+        self.assertEqual(1, len(get_('group1', 'fake', True)))
+        self.assertEqual(1, len(get_('group2', 'fake', False)))
+        self.assertRaises(sqlalchemy.exc.IntegrityError,
+                          security_groups.insert().execute,
+                          dict(name='group2', project_id='fake', deleted=0))
+
 
 class TestBaremetalMigrations(BaseMigrationTestCase, CommonTestsMixIn):
     """Test sqlalchemy-migrate migrations."""