From f9a087e9122acc2913147a8b5a72dd81cc965e62 Mon Sep 17 00:00:00 2001 From: Eli Qiao Date: Thu, 4 Feb 2016 11:47:08 +0800 Subject: [PATCH] API: Move validate_properties to REST API layer We restrict that only node_count can be modified, this can be moved into REST API layer before we send request to conductor though rpc. Besides, remove some bad test case in REST API layer Change-Id: I5a73488d0c7529ea2e1416a9e41b4893f220a25a --- magnum/api/controllers/v1/bay.py | 5 ++ magnum/api/validation.py | 12 ++++ magnum/conductor/handlers/bay_conductor.py | 11 ---- .../tests/unit/api/controllers/v1/test_bay.py | 56 +++++++++++-------- magnum/tests/unit/api/test_validation.py | 7 +++ .../conductor/handlers/test_bay_conductor.py | 19 ------- 6 files changed, 58 insertions(+), 52 deletions(-) diff --git a/magnum/api/controllers/v1/bay.py b/magnum/api/controllers/v1/bay.py index 7a40e36454..84d1965b41 100644 --- a/magnum/api/controllers/v1/bay.py +++ b/magnum/api/controllers/v1/bay.py @@ -26,6 +26,7 @@ from magnum.api.controllers.v1 import collection from magnum.api.controllers.v1 import types from magnum.api import expose from magnum.api import utils as api_utils +from magnum.api.validation import validate_bay_properties from magnum.common import exception from magnum.common import policy from magnum import objects @@ -325,6 +326,10 @@ class BaysController(rest.RestController): if rpc_bay[field] != patch_val: rpc_bay[field] = patch_val + delta = rpc_bay.obj_what_changed() + + validate_bay_properties(delta) + res_bay = pecan.request.rpcapi.bay_update(rpc_bay) return Bay.convert_with_links(res_bay) diff --git a/magnum/api/validation.py b/magnum/api/validation.py index 224e2cbe00..9aecba3715 100644 --- a/magnum/api/validation.py +++ b/magnum/api/validation.py @@ -56,6 +56,9 @@ baymodel_opts = [ cfg.CONF.register_opts(baymodel_opts, group='baymodel') +bay_update_allowed_properties = set(['node_count']) + + def enforce_bay_types(*bay_types): """Enforce that bay_type is in supported list.""" @decorator.decorator @@ -154,6 +157,15 @@ def _enforce_volume_driver_types(baymodel): validator.validate_volume_driver(baymodel['volume_driver']) +def validate_bay_properties(delta): + + update_disallowed_properties = delta - bay_update_allowed_properties + if update_disallowed_properties: + err = (_("cannot change bay property(ies) %s.") % + ", ".join(update_disallowed_properties)) + raise exception.InvalidParameterValue(err=err) + + class Validator(object): validators = {} diff --git a/magnum/conductor/handlers/bay_conductor.py b/magnum/conductor/handlers/bay_conductor.py index 73f9038a8a..a1311eba29 100644 --- a/magnum/conductor/handlers/bay_conductor.py +++ b/magnum/conductor/handlers/bay_conductor.py @@ -124,8 +124,6 @@ def _update_stack(context, osc, bay, scale_manager=None): class Handler(object): - _update_allowed_properties = set(['node_count']) - def __init__(self): super(Handler, self).__init__() @@ -163,13 +161,6 @@ class Handler(object): return bay - def _validate_properties(self, delta): - update_disallowed_properties = delta - self._update_allowed_properties - if update_disallowed_properties: - err = (_("cannot change bay property(ies) %s.") % - ", ".join(update_disallowed_properties)) - raise exception.InvalidParameterValue(err=err) - def bay_update(self, context, bay): LOG.debug('bay_heat bay_update') @@ -194,8 +185,6 @@ class Handler(object): if not delta: return bay - self._validate_properties(delta) - manager = scale_manager.ScaleManager(context, osc, bay) _update_stack(context, osc, bay, manager) diff --git a/magnum/tests/unit/api/controllers/v1/test_bay.py b/magnum/tests/unit/api/controllers/v1/test_bay.py index 5dec271260..ce1f070cc1 100644 --- a/magnum/tests/unit/api/controllers/v1/test_bay.py +++ b/magnum/tests/unit/api/controllers/v1/test_bay.py @@ -200,47 +200,47 @@ class TestPatch(api_base.FunctionalTest): @mock.patch('oslo_utils.timeutils.utcnow') def test_replace_ok(self, mock_utcnow): - name = 'bay_example_B' + new_node_count = 4 test_time = datetime.datetime(2000, 1, 1, 0, 0) mock_utcnow.return_value = test_time response = self.patch_json('/bays/%s' % self.bay.uuid, - [{'path': '/name', 'value': name, + [{'path': '/node_count', + 'value': new_node_count, 'op': 'replace'}]) self.assertEqual('application/json', response.content_type) self.assertEqual(200, response.status_code) response = self.get_json('/bays/%s' % self.bay.uuid) - self.assertEqual(name, response['name']) + self.assertEqual(new_node_count, response['node_count']) return_updated_at = timeutils.parse_isotime( response['updated_at']).replace(tzinfo=None) self.assertEqual(test_time, return_updated_at) # Assert nothing else was changed self.assertEqual(self.bay.uuid, response['uuid']) self.assertEqual(self.bay.baymodel_id, response['baymodel_id']) - self.assertEqual(self.bay.node_count, response['node_count']) @mock.patch('oslo_utils.timeutils.utcnow') def test_replace_ok_by_name(self, mock_utcnow): - name = 'bay_example_B' + new_node_count = 4 test_time = datetime.datetime(2000, 1, 1, 0, 0) mock_utcnow.return_value = test_time response = self.patch_json('/bays/%s' % self.bay.name, - [{'path': '/name', 'value': name, + [{'path': '/node_count', + 'value': new_node_count, 'op': 'replace'}]) self.assertEqual('application/json', response.content_type) self.assertEqual(200, response.status_code) response = self.get_json('/bays/%s' % self.bay.uuid) - self.assertEqual(name, response['name']) + self.assertEqual(new_node_count, response['node_count']) return_updated_at = timeutils.parse_isotime( response['updated_at']).replace(tzinfo=None) self.assertEqual(test_time, return_updated_at) # Assert nothing else was changed self.assertEqual(self.bay.uuid, response['uuid']) self.assertEqual(self.bay.baymodel_id, response['baymodel_id']) - self.assertEqual(self.bay.node_count, response['node_count']) @mock.patch('oslo_utils.timeutils.utcnow') def test_replace_ok_by_name_not_found(self, mock_utcnow): @@ -255,6 +255,18 @@ class TestPatch(api_base.FunctionalTest): self.assertEqual('application/json', response.content_type) self.assertEqual(404, response.status_code) + def test_replace_baymodel_id_failed(self): + baymodel = obj_utils.create_test_baymodel(self.context, + uuid=utils.generate_uuid()) + response = self.patch_json('/bays/%s' % self.bay.uuid, + [{'path': '/baymodel_id', + 'value': baymodel.uuid, + 'op': 'replace'}], + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(400, response.status_code) + self.assertTrue(response.json['error_message']) + @mock.patch('oslo_utils.timeutils.utcnow') def test_replace_ok_by_name_multiple_bay(self, mock_utcnow): test_time = datetime.datetime(2000, 1, 1, 0, 0) @@ -272,17 +284,6 @@ class TestPatch(api_base.FunctionalTest): self.assertEqual('application/json', response.content_type) self.assertEqual(409, response.status_code) - def test_replace_baymodel_id(self): - baymodel = obj_utils.create_test_baymodel(self.context, - uuid=utils.generate_uuid()) - response = self.patch_json('/bays/%s' % self.bay.uuid, - [{'path': '/baymodel_id', - 'value': baymodel.uuid, - 'op': 'replace'}], - expect_errors=True) - self.assertEqual('application/json', response.content_type) - self.assertEqual(200, response.status_code) - def test_replace_non_existent_baymodel_id(self): response = self.patch_json('/bays/%s' % self.bay.uuid, [{'path': '/baymodel_id', @@ -312,6 +313,16 @@ class TestPatch(api_base.FunctionalTest): self.assertEqual('application/json', response.content_type) self.assertTrue(response.json['error_message']) + def test_replace_bay_name_failed(self): + response = self.patch_json('/bays/%s' % self.bay.uuid, + [{'path': '/name', + 'value': 'bay_example_B', + 'op': 'replace'}], + expect_errors=True) + self.assertEqual(400, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['error_message']) + def test_add_non_existent_property(self): response = self.patch_json( '/bays/%s' % self.bay.uuid, @@ -326,16 +337,17 @@ class TestPatch(api_base.FunctionalTest): self.assertIsNotNone(response['name']) response = self.patch_json('/bays/%s' % self.bay.uuid, - [{'path': '/name', 'op': 'remove'}]) + [{'path': '/node_count', 'op': 'remove'}]) self.assertEqual('application/json', response.content_type) self.assertEqual(200, response.status_code) response = self.get_json('/bays/%s' % self.bay.uuid) - self.assertIsNone(response['name']) + # only allow node_count for bay, and default value is 1 + self.assertEqual(1, response['node_count']) # Assert nothing else was changed self.assertEqual(self.bay.uuid, response['uuid']) self.assertEqual(self.bay.baymodel_id, response['baymodel_id']) - self.assertEqual(self.bay.node_count, response['node_count']) + self.assertEqual(self.bay.name, response['name']) self.assertEqual(self.bay.master_count, response['master_count']) def test_remove_uuid(self): diff --git a/magnum/tests/unit/api/test_validation.py b/magnum/tests/unit/api/test_validation.py index 239ce462ef..c945f537de 100644 --- a/magnum/tests/unit/api/test_validation.py +++ b/magnum/tests/unit/api/test_validation.py @@ -411,3 +411,10 @@ class TestValidation(base.BaseTestCase): self._test_enforce_volume_driver_types_update( volume_driver_type='cinder', op='remove') + + def test_validate_bay_properties_pass(self): + v.validate_bay_properties(set(['node_count'])) + + def test_validate_bay_properties_failed(self): + self.assertRaises(exception.InvalidParameterValue, + v.validate_bay_properties, set(['name'])) diff --git a/magnum/tests/unit/conductor/handlers/test_bay_conductor.py b/magnum/tests/unit/conductor/handlers/test_bay_conductor.py index e5bceafdce..fd82e50253 100644 --- a/magnum/tests/unit/conductor/handlers/test_bay_conductor.py +++ b/magnum/tests/unit/conductor/handlers/test_bay_conductor.py @@ -146,25 +146,6 @@ class TestHandler(db_base.DbTestCase): def test_update_bay_status_adopt_compelete(self): self._test_update_bay_status_complete(bay_status.ADOPT_COMPLETE) - @patch('magnum.common.clients.OpenStackClients') - def test_update_bay_with_invalid_params( - self, mock_openstack_client_class): - mock_heat_stack = mock.MagicMock() - mock_heat_stack.stack_status = bay_status.CREATE_COMPLETE - mock_heat_client = mock.MagicMock() - mock_heat_client.stacks.get.return_value = mock_heat_stack - mock_openstack_client = mock_openstack_client_class.return_value - mock_openstack_client.heat.return_value = mock_heat_client - - self.bay.node_count = 2 - self.bay.api_address = '7.7.7.7' - self.assertRaises(exception.InvalidParameterValue, - self.handler.bay_update, - self.context, - self.bay) - bay = objects.Bay.get(self.context, self.bay.uuid) - self.assertEqual(1, bay.node_count) - @patch('magnum.conductor.handlers.bay_conductor.HeatPoller') @patch('magnum.conductor.handlers.bay_conductor.cert_manager') @patch('magnum.conductor.handlers.bay_conductor._create_stack')