diff --git a/nova/cloudpipe/pipelib.py b/nova/cloudpipe/pipelib.py index db79d4d4230..73d6b92e6a8 100644 --- a/nova/cloudpipe/pipelib.py +++ b/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', diff --git a/nova/compute/api.py b/nova/compute/api.py index 48d404148da..3c76726dd98 100644 --- a/nova/compute/api.py +++ b/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: diff --git a/nova/db/api.py b/nova/db/api.py index bd519110ca2..ccbc6956e62 100644 --- a/nova/db/api.py +++ b/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) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 03dd439466f..5bbb21fe790 100644 --- a/nova/db/sqlalchemy/api.py +++ b/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): diff --git a/nova/db/sqlalchemy/migrate_repo/versions/190_add_security_group_uc.py b/nova/db/sqlalchemy/migrate_repo/versions/190_add_security_group_uc.py new file mode 100644 index 00000000000..72c26dd8abb --- /dev/null +++ b/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) diff --git a/nova/exception.py b/nova/exception.py index 350fb2f7786..52123159c36 100644 --- a/nova/exception.py +++ b/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") diff --git a/nova/tests/api/openstack/compute/contrib/test_cloudpipe.py b/nova/tests/api/openstack/compute/contrib/test_cloudpipe.py index 83dca7a95bf..e28e9aa7a64 100644 --- a/nova/tests/api/openstack/compute/contrib/test_cloudpipe.py +++ b/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): diff --git a/nova/tests/db/test_db_api.py b/nova/tests/db/test_db_api.py index 15ef5c3c146..85a31bc2ee9 100644 --- a/nova/tests/db/test_db_api.py +++ b/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): diff --git a/nova/tests/db/test_migrations.py b/nova/tests/db/test_migrations.py index 7e28fb1d572..f0c13f02995 100644 --- a/nova/tests/db/test_migrations.py +++ b/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."""