Always include 'enabled' field in service response
NOTE: this is basically a mirror of the following patch, but for
services instead of endpoints:
https://review.openstack.org/#/c/75727/
The 'enabled' field wasn't always returned in a service response.
The 'enabled' attribute for services was stored in the 'extra'
column as part of a JSON string. Now the 'enabled' attribute is in
its own column. It will also be easier to filter out disabled
services now that enabled is a separate column.
With the 'enabled' field being a Boolean column in the database,
the server also needs to validate that the value is a Boolean (if
it's present). This is done in the same way that the server checks
the type of the 'enabled' value when creating users.
Change-Id: I73f0bb58cd31d80b3c1b6c6834e9e8b38adc86a0
Co-Authored-By: Brant Knudson <bknudson@us.ibm.com>
Partial-Bug: 1273867
This commit is contained in:
Dolph Mathews
parent
0fb0dfdf41
commit
45cb6ea93f
|
|
@ -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")
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
|
|
|
|||
Loading…
Reference in New Issue