openstack/nova
Code Issues Proposed changes

Add unique constraint for security groups

Browse Source 
- 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
This commit is contained in:
Roman Bogorodskiy
2013-06-13 07:41:51 -04:00
parent 88531ada39
commit 231c97007c
9 changed files with 125 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
nova/cloudpipe/pipelib.py
View File

@@ -142,13 +142,14 @@ class CloudPipe(object):
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',

11
nova/compute/api.py
View File

@@ -2952,16 +2952,15 @@ class SecurityGroupAPI(base.Base, security_group_base.SecurityGroupBase):
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:

5
nova/db/api.py
View File

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

26
nova/db/sqlalchemy/api.py
View File

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

40
nova/db/sqlalchemy/migrate_repo/versions/190_add_security_group_uc.py Normal file
View File

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

5
nova/exception.py
View File

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

8
nova/tests/api/openstack/compute/contrib/test_cloudpipe.py
View File

@@ -19,7 +19,6 @@ from oslo.config import cfg
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 @@ class CloudpipeTest(test.TestCase):
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):

50
nova/tests/db/test_db_api.py
View File

@@ -1117,8 +1117,8 @@ class SecurityGroupRuleTestCase(test.TestCase, ModelsObjectComparatorMixin):
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 @@ class SecurityGroupRuleTestCase(test.TestCase, ModelsObjectComparatorMixin):
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 @@ class SecurityGroupTestCase(test.TestCase, ModelsObjectComparatorMixin):
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 @@ class SecurityGroupTestCase(test.TestCase, ModelsObjectComparatorMixin):
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 @@ class SecurityGroupTestCase(test.TestCase, ModelsObjectComparatorMixin):
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):

30
nova/tests/db/test_migrations.py
View File

@@ -1669,6 +1669,36 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn):
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."""