diff --git a/keystone/catalog/backends/sql.py b/keystone/catalog/backends/sql.py index 39ba3258a5..82dd00e971 100644 --- a/keystone/catalog/backends/sql.py +++ b/keystone/catalog/backends/sql.py @@ -54,9 +54,11 @@ class Region(sql.ModelBase, sql.DictBase): class Service(sql.ModelBase, sql.DictBase): __tablename__ = 'service' - attributes = ['id', 'type'] + attributes = ['id', 'type', 'enabled'] id = sql.Column(sql.String(64), primary_key=True) type = sql.Column(sql.String(255)) + enabled = sql.Column(sql.Boolean, nullable=False, default=True, + server_default='1') extra = sql.Column(sql.JsonBlob()) endpoints = sql.relationship("Endpoint", backref="service") diff --git a/keystone/catalog/controllers.py b/keystone/catalog/controllers.py index 500861d311..f3542043cf 100644 --- a/keystone/catalog/controllers.py +++ b/keystone/catalog/controllers.py @@ -201,8 +201,15 @@ class ServiceV3(controller.V3Controller): super(ServiceV3, self).__init__() self.get_member_from_driver = self.catalog_api.get_service + def _validate_service(self, service): + if 'enabled' in service and not isinstance(service['enabled'], bool): + msg = _('Enabled field must be a boolean') + raise exception.ValidationError(message=msg) + @controller.protected() def create_service(self, context, service): + self._validate_service(service) + ref = self._assign_unique_id(self._normalize_dict(service)) self._require_attribute(ref, 'type') @@ -223,6 +230,7 @@ class ServiceV3(controller.V3Controller): @controller.protected() def update_service(self, context, service_id, service): self._require_matching_id(service_id, service) + self._validate_service(service) ref = self.catalog_api.update_service(service_id, service) return ServiceV3.wrap_member(context, ref) diff --git a/keystone/catalog/core.py b/keystone/catalog/core.py index b4b3b8beda..6ff7967c75 100644 --- a/keystone/catalog/core.py +++ b/keystone/catalog/core.py @@ -87,6 +87,10 @@ class Manager(manager.Manager): except exception.NotFound: raise exception.RegionNotFound(region_id=region_id) + def create_service(self, service_id, service_ref): + service_ref.setdefault('enabled', True) + return self.driver.create_service(service_id, service_ref) + def get_service(self, service_id): try: return self.driver.get_service(service_id) diff --git a/keystone/common/sql/migrate_repo/versions/044_service_enabled.py b/keystone/common/sql/migrate_repo/versions/044_service_enabled.py new file mode 100644 index 0000000000..0196e84ae6 --- /dev/null +++ b/keystone/common/sql/migrate_repo/versions/044_service_enabled.py @@ -0,0 +1,147 @@ +# Copyright 2014 IBM Corp. +# +# 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. + + +"""Adds an `enabled` column to the `service` table. + +The enabled value for the `service` table was stored in the `extra` column +as part of a JSON string. + +To upgrade, the `enabled` column is added with a default value of ``true``, +then we check all the `extra` JSON for disabled and set the value to ``false`` +for those. + +Downgrade is essentially the opposite -- we update the JSON with +``"enabled": false`` for any services that are disabled and drop the `enabled` +column. + +""" + +import sqlalchemy as sql +from sqlalchemy.orm import sessionmaker + +from keystone.openstack.common import jsonutils +from keystone.openstack.common import strutils + + +def _migrate_enabled_from_extra(migrate_engine, service_table): + """Remove `enabled` from `extra`, put it in the `enabled` column.""" + + services = list(service_table.select().execute()) + + for service in services: + extra_dict = jsonutils.loads(service.extra) + + if 'enabled' not in extra_dict: + # `enabled` and `extra` are already as expected. + continue + + enabled = extra_dict.pop('enabled') + + if enabled is None: + enabled = True + else: + enabled = strutils.bool_from_string(enabled, default=True) + + new_values = { + 'enabled': enabled, + 'extra': jsonutils.dumps(extra_dict), + } + f = service_table.c.id == service.id + update = service_table.update().where(f).values(new_values) + migrate_engine.execute(update) + + +def _migrate_enabled_to_extra(migrate_engine, service_table): + """Get enabled value from 'enabled' column and put it in 'extra' JSON. + + Only put the enabled value to the 'extra' JSON if it's False, since the + default is True. + + """ + + services = list(service_table.select().execute()) + + for service in services: + + if service.enabled: + # Nothing to do since the service is enabled. + continue + + extra_dict = jsonutils.loads(service.extra) + extra_dict['enabled'] = False + + new_values = { + 'extra': jsonutils.dumps(extra_dict), + } + f = service_table.c.id == service.id + update = service_table.update().where(f).values(new_values) + migrate_engine.execute(update) + + +def upgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + service_table = sql.Table('service', meta, autoload=True) + enabled_column = sql.Column('enabled', sql.Boolean, nullable=False, + default=True, server_default='1') + enabled_column.create(service_table) + + _migrate_enabled_from_extra(migrate_engine, service_table) + + +def _downgrade_service_table_with_copy(meta, migrate_engine): + # Used with databases that don't support dropping a column (e.g., sqlite). + + maker = sessionmaker(bind=migrate_engine) + session = maker() + + session.execute('ALTER TABLE service RENAME TO orig_service;') + + service_table = sql.Table( + 'service', + meta, + sql.Column('id', sql.String(64), primary_key=True), + sql.Column('type', sql.String(255)), + sql.Column('extra', sql.Text())) + service_table.create(migrate_engine, checkfirst=True) + + orig_service_table = sql.Table('orig_service', meta, autoload=True) + for service in session.query(orig_service_table): + new_values = { + 'id': service.id, + 'type': service.type, + 'extra': service.extra, + } + session.execute('insert into service (id, type, extra) ' + 'values ( :id, :type, :extra);', + new_values) + session.execute('drop table orig_service;') + session.close() + + +def downgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + service_table = sql.Table('service', meta, autoload=True) + _migrate_enabled_to_extra(migrate_engine, service_table) + + if migrate_engine.name == 'sqlite': + meta.clear() + _downgrade_service_table_with_copy(meta, migrate_engine) + return + + service_table.c.enabled.drop() diff --git a/keystone/tests/test_backend.py b/keystone/tests/test_backend.py index da34ead5ee..4b16846b2c 100644 --- a/keystone/tests/test_backend.py +++ b/keystone/tests/test_backend.py @@ -3584,7 +3584,8 @@ class CatalogTests(object): res = self.catalog_api.create_service( service_id, new_service.copy()) - self.assertDictEqual(res, new_service) + new_service['enabled'] = True + self.assertDictEqual(new_service, res) # list services = self.catalog_api.list_services() diff --git a/keystone/tests/test_sql_upgrade.py b/keystone/tests/test_sql_upgrade.py index 99e227d588..ddcab070d0 100644 --- a/keystone/tests/test_sql_upgrade.py +++ b/keystone/tests/test_sql_upgrade.py @@ -2107,6 +2107,168 @@ class SqlUpgradeTests(SqlMigrateBase): unlimited_trust['id'], session.query(trust_table.columns.id).one()[0]) + def test_upgrade_service_enabled_cols(self): + """Migration 44 added `enabled` column to `service` table.""" + + self.upgrade(44) + + # Verify that there's an 'enabled' field. + exp_cols = ['id', 'type', 'extra', 'enabled'] + self.assertTableColumns('service', exp_cols) + + def test_downgrade_service_enabled_cols(self): + """Check columns when downgrade to migration 43. + + The downgrade from migration 44 removes the `enabled` column from the + `service` table. + + """ + + self.upgrade(44) + self.downgrade(43) + + exp_cols = ['id', 'type', 'extra'] + self.assertTableColumns('service', exp_cols) + + def test_upgrade_service_enabled_data(self): + """Migration 44 has to migrate data from `extra` to `enabled`.""" + + session = self.Session() + + def add_service(**extra_data): + service_id = uuid.uuid4().hex + + service = { + 'id': service_id, + 'type': uuid.uuid4().hex, + 'extra': json.dumps(extra_data), + } + + self.insert_dict(session, 'service', service) + + return service_id + + self.upgrade(43) + + # Different services with expected enabled and extra values, and a + # description. + random_attr_name = uuid.uuid4().hex + random_attr_value = uuid.uuid4().hex + random_attr = {random_attr_name: random_attr_value} + random_attr_str = "%s='%s'" % (random_attr_name, random_attr_value) + random_attr_enabled_false = {random_attr_name: random_attr_value, + 'enabled': False} + random_attr_enabled_false_str = 'enabled=False,%s' % random_attr_str + + services = [ + # Some values for True. + (add_service(), (True, {}), 'no enabled'), + (add_service(enabled=True), (True, {}), 'enabled=True'), + (add_service(enabled='true'), (True, {}), "enabled='true'"), + (add_service(**random_attr), + (True, random_attr), random_attr_str), + (add_service(enabled=None), (True, {}), 'enabled=None'), + + # Some values for False. + (add_service(enabled=False), (False, {}), 'enabled=False'), + (add_service(enabled='false'), (False, {}), "enabled='false'"), + (add_service(enabled='0'), (False, {}), "enabled='0'"), + (add_service(**random_attr_enabled_false), + (False, random_attr), random_attr_enabled_false_str), + ] + + self.upgrade(44) + + # Verify that the services have the expected values. + + self.metadata.clear() + service_table = sqlalchemy.Table('service', self.metadata, + autoload=True) + + def fetch_service(service_id): + cols = [service_table.c.enabled, service_table.c.extra] + f = service_table.c.id == service_id + s = sqlalchemy.select(cols).where(f) + ep = session.execute(s).fetchone() + return ep.enabled, json.loads(ep.extra) + + for service_id, exp, msg in services: + exp_enabled, exp_extra = exp + + enabled, extra = fetch_service(service_id) + + self.assertIs(exp_enabled, enabled, msg) + self.assertEqual(exp_extra, extra, msg) + + def test_downgrade_service_enabled_data(self): + """Downgrade from migration 44 migrates data. + + Downgrade from migration 44 migrates data from `enabled` to + `extra`. Any disabled services have 'enabled': False put into 'extra'. + + """ + + session = self.Session() + + def add_service(enabled=True, **extra_data): + service_id = uuid.uuid4().hex + + service = { + 'id': service_id, + 'type': uuid.uuid4().hex, + 'extra': json.dumps(extra_data), + 'enabled': enabled + } + + self.insert_dict(session, 'service', service) + + return service_id + + self.upgrade(44) + + # Insert some services using the new format. + + # We'll need a service entry since it's the foreign key for services. + service_id = add_service(True) + + new_service = (lambda enabled, **extra_data: + add_service(enabled, **extra_data)) + + # Different services with expected extra values, and a + # description. + services = [ + # True tests + (new_service(True), {}, 'enabled'), + (new_service(True, something='whatever'), + {'something': 'whatever'}, + "something='whatever'"), + + # False tests + (new_service(False), {'enabled': False}, 'enabled=False'), + (new_service(False, something='whatever'), + {'enabled': False, 'something': 'whatever'}, + "enabled=False, something='whatever'"), + ] + + self.downgrade(43) + + # Verify that the services have the expected values. + + self.metadata.clear() + service_table = sqlalchemy.Table('service', self.metadata, + autoload=True) + + def fetch_service(service_id): + cols = [service_table.c.extra] + f = service_table.c.id == service_id + s = sqlalchemy.select(cols).where(f) + ep = session.execute(s).fetchone() + return json.loads(ep.extra) + + for service_id, exp_extra, msg in services: + extra = fetch_service(service_id) + self.assertEqual(exp_extra, extra, msg) + def test_upgrade_endpoint_enabled_cols(self): """Migration 42 added `enabled` column to `endpoint` table.""" @@ -2125,7 +2287,7 @@ class SqlUpgradeTests(SqlMigrateBase): """ - self.upgrade(self.max_version) + self.upgrade(42) self.downgrade(41) exp_cols = ['id', 'legacy_endpoint_id', 'interface', 'region', @@ -2200,7 +2362,7 @@ class SqlUpgradeTests(SqlMigrateBase): (False, random_attr), random_attr_enabled_false_str), ] - self.upgrade(self.max_version) + self.upgrade(42) # Verify that the endpoints have the expected values. @@ -2260,7 +2422,7 @@ class SqlUpgradeTests(SqlMigrateBase): return endpoint_id - self.upgrade(self.max_version) + self.upgrade(42) # Insert some endpoints using the new format. @@ -2549,8 +2711,8 @@ class VersionTests(SqlMigrateBase): self.assertTrue(version > 0, "Version didn't change after migrated?") def test_unexpected_extension(self): - """When get the version for an extension that doesn't exist, raises - ImportError. + """The version for an extension that doesn't exist raises ImportError. + """ extension_name = uuid.uuid4().hex @@ -2559,8 +2721,8 @@ class VersionTests(SqlMigrateBase): extension=extension_name) def test_unversioned_extension(self): - """When get the version for an extension that doesn't provide - migrations, raises MigrationNotProvided. + """The version for extensions without migrations raise an exception. + """ self.assertRaises(exception.MigrationNotProvided, diff --git a/keystone/tests/test_v3.py b/keystone/tests/test_v3.py index c4b7fd7907..a6d5c54d27 100644 --- a/keystone/tests/test_v3.py +++ b/keystone/tests/test_v3.py @@ -660,6 +660,7 @@ class RestfulTestCase(tests.SQLDriverOverrides, rest.RestfulTestCase): def assertValidService(self, entity, ref=None): self.assertIsNotNone(entity.get('type')) + self.assertIsInstance(entity.get('enabled'), bool) if ref: self.assertEqual(ref['type'], entity['type']) return entity diff --git a/keystone/tests/test_v3_catalog.py b/keystone/tests/test_v3_catalog.py index 0aafbbd575..6f35a2411d 100644 --- a/keystone/tests/test_v3_catalog.py +++ b/keystone/tests/test_v3_catalog.py @@ -129,6 +129,55 @@ class CatalogTestCase(test_v3.RestfulTestCase): body={'service': ref}) self.assertValidServiceResponse(r, ref) + def test_create_service_no_enabled(self): + """Call ``POST /services``.""" + ref = self.new_service_ref() + del ref['enabled'] + r = self.post( + '/services', + body={'service': ref}) + ref['enabled'] = True + self.assertValidServiceResponse(r, ref) + self.assertIs(True, r.result['service']['enabled']) + + def test_create_service_enabled_false(self): + """Call ``POST /services``.""" + ref = self.new_service_ref() + ref['enabled'] = False + r = self.post( + '/services', + body={'service': ref}) + self.assertValidServiceResponse(r, ref) + self.assertIs(False, r.result['service']['enabled']) + + def test_create_service_enabled_true(self): + """Call ``POST /services``.""" + ref = self.new_service_ref() + ref['enabled'] = True + r = self.post( + '/services', + body={'service': ref}) + self.assertValidServiceResponse(r, ref) + self.assertIs(True, r.result['service']['enabled']) + + def test_create_service_enabled_str_true(self): + """Call ``POST /services``.""" + ref = self.new_service_ref() + ref['enabled'] = 'True' + self.post('/services', body={'service': ref}, expected_status=400) + + def test_create_service_enabled_str_false(self): + """Call ``POST /services``.""" + ref = self.new_service_ref() + ref['enabled'] = 'False' + self.post('/services', body={'service': ref}, expected_status=400) + + def test_create_service_enabled_str_random(self): + """Call ``POST /services``.""" + ref = self.new_service_ref() + ref['enabled'] = 'puppies' + self.post('/services', body={'service': ref}, expected_status=400) + def test_list_services(self): """Call ``GET /services``.""" r = self.get('/services')