diff options
author | Zuul <zuul@review.openstack.org> | 2018-02-08 01:04:02 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2018-02-08 01:04:02 +0000 |
commit | 9ab04d0962b5d3ccf9b616efdb0c5113be84041c (patch) | |
tree | 7fc79841ec7d5463bb04d4e48e7d59392b7e0a9a | |
parent | 39431a6ccfeab69f7d4ec05e65113ccee7bb0774 (diff) | |
parent | cfc167eadfc373b262d92d8ce4a00a9fd38d23d7 (diff) | |
download | ironic-9ab04d0962b5d3ccf9b616efdb0c5113be84041c.tar.gz |
Merge "Stop guessing mime types based on URLs"
-rw-r--r-- | ironic/api/app.py | 8 | ||||
-rw-r--r-- | ironic/api/controllers/v1/node.py | 6 | ||||
-rw-r--r-- | ironic/api/controllers/v1/portgroup.py | 9 | ||||
-rw-r--r-- | ironic/api/controllers/v1/ramdisk.py | 2 | ||||
-rw-r--r-- | ironic/api/controllers/v1/utils.py | 46 | ||||
-rw-r--r-- | ironic/api/middleware/__init__.py | 5 | ||||
-rw-r--r-- | ironic/api/middleware/json_ext.py | 43 | ||||
-rw-r--r-- | ironic/tests/unit/api/controllers/v1/test_node.py | 69 | ||||
-rw-r--r-- | ironic/tests/unit/api/controllers/v1/test_portgroup.py | 43 | ||||
-rw-r--r-- | ironic/tests/unit/api/controllers/v1/test_ramdisk.py | 26 | ||||
-rw-r--r-- | releasenotes/notes/name-suffix-47aea2d265fa75ae.yaml | 19 |
11 files changed, 266 insertions, 10 deletions
diff --git a/ironic/api/app.py b/ironic/api/app.py index 9cd9036b7..7a55def79 100644 --- a/ironic/api/app.py +++ b/ironic/api/app.py @@ -26,6 +26,7 @@ from ironic.api.controllers import base from ironic.api import hooks from ironic.api import middleware from ironic.api.middleware import auth_token +from ironic.api.middleware import json_ext from ironic.common import exception from ironic.conf import CONF @@ -73,6 +74,11 @@ def setup_app(pecan_config=None, extra_hooks=None): force_canonical=getattr(pecan_config.app, 'force_canonical', True), hooks=app_hooks, wrap_app=middleware.ParsableErrorMiddleware, + # NOTE(dtantsur): enabling this causes weird issues with nodes named + # as if they had a known mime extension, e.g. "mynode.1". We do + # simulate the same behaviour for .json extensions for backward + # compatibility through JsonExtensionMiddleware. + guess_content_type_from_ext=False, ) if CONF.audit.enabled: @@ -106,6 +112,8 @@ def setup_app(pecan_config=None, extra_hooks=None): base.Version.string] ) + app = json_ext.JsonExtensionMiddleware(app) + return app diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 48281fdaa..a7da93a6c 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1788,7 +1788,7 @@ class NodesController(rest.RestController): api_utils.check_allow_specify_fields(fields) api_utils.check_allowed_fields(fields) - rpc_node = api_utils.get_rpc_node(node_ident) + rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) return Node.convert_with_links(rpc_node, fields=fields) @METRICS.timer('NodesController.post') @@ -1913,7 +1913,7 @@ class NodesController(rest.RestController): if r_interface and not api_utils.allow_rescue_interface(): raise exception.NotAcceptable() - rpc_node = api_utils.get_rpc_node(node_ident) + rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) remove_inst_uuid_patch = [{'op': 'remove', 'path': '/instance_uuid'}] if rpc_node.maintenance and patch == remove_inst_uuid_patch: @@ -1988,7 +1988,7 @@ class NodesController(rest.RestController): if self.from_chassis: raise exception.OperationNotPermitted() - rpc_node = api_utils.get_rpc_node(node_ident) + rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) chassis_uuid = _get_chassis_uuid(rpc_node) notify.emit_start_notification(context, rpc_node, 'delete', chassis_uuid=chassis_uuid) diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 74c4e3b71..9ced692b9 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -422,7 +422,8 @@ class PortgroupsController(pecan.rest.RestController): api_utils.check_allowed_portgroup_fields(fields) - rpc_portgroup = api_utils.get_rpc_portgroup(portgroup_ident) + rpc_portgroup = api_utils.get_rpc_portgroup_with_suffix( + portgroup_ident) return Portgroup.convert_with_links(rpc_portgroup, fields=fields) @METRICS.timer('PortgroupsController.post') @@ -502,7 +503,8 @@ class PortgroupsController(pecan.rest.RestController): api_utils.is_path_updated(patch, '/properties'))): raise exception.NotAcceptable() - rpc_portgroup = api_utils.get_rpc_portgroup(portgroup_ident) + rpc_portgroup = api_utils.get_rpc_portgroup_with_suffix( + portgroup_ident) names = api_utils.get_patch_values(patch, '/name') for name in names: @@ -573,7 +575,8 @@ class PortgroupsController(pecan.rest.RestController): if self.parent_node_ident: raise exception.OperationNotPermitted() - rpc_portgroup = api_utils.get_rpc_portgroup(portgroup_ident) + rpc_portgroup = api_utils.get_rpc_portgroup_with_suffix( + portgroup_ident) rpc_node = objects.Node.get_by_id(pecan.request.context, rpc_portgroup.node_id) diff --git a/ironic/api/controllers/v1/ramdisk.py b/ironic/api/controllers/v1/ramdisk.py index 9d0c5e82b..90d5408ea 100644 --- a/ironic/api/controllers/v1/ramdisk.py +++ b/ironic/api/controllers/v1/ramdisk.py @@ -180,7 +180,7 @@ class HeartbeatController(rest.RestController): cdict = pecan.request.context.to_policy_values() policy.authorize('baremetal:node:ipa_heartbeat', cdict, cdict) - rpc_node = api_utils.get_rpc_node(node_ident) + rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) try: topic = pecan.request.rpcapi.get_topic_for(rpc_node) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index cc5ba1220..3fb5e10c1 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -164,6 +164,20 @@ def allow_node_logical_names(): return pecan.request.version.minor >= versions.MINOR_5_NODE_NAME +def _get_with_suffix(get_func, ident, exc_class): + """Helper to get a resource taking into account API .json suffix.""" + try: + return get_func(ident) + except exc_class: + if not pecan.request.environ['HAS_JSON_SUFFIX']: + raise + + # NOTE(dtantsur): strip .json prefix to maintain compatibility + # with the guess_content_type_from_ext feature. Try to return it + # back if the resulting resource was not found. + return get_func(ident + '.json') + + def get_rpc_node(node_ident): """Get the RPC node from the node uuid or logical name. @@ -188,6 +202,21 @@ def get_rpc_node(node_ident): raise exception.NodeNotFound(node=node_ident) +def get_rpc_node_with_suffix(node_ident): + """Get the RPC node from the node uuid or logical name. + + If HAS_JSON_SUFFIX flag is set in the pecan environment, try also looking + for node_ident with '.json' suffix. Otherwise identical to get_rpc_node. + + :param node_ident: the UUID or logical name of a node. + + :returns: The RPC Node. + :raises: InvalidUuidOrName if the name or uuid provided is not valid. + :raises: NodeNotFound if the node is not found. + """ + return _get_with_suffix(get_rpc_node, node_ident, exception.NodeNotFound) + + def get_rpc_portgroup(portgroup_ident): """Get the RPC portgroup from the portgroup UUID or logical name. @@ -210,6 +239,23 @@ def get_rpc_portgroup(portgroup_ident): raise exception.InvalidUuidOrName(name=portgroup_ident) +def get_rpc_portgroup_with_suffix(portgroup_ident): + """Get the RPC portgroup from the portgroup UUID or logical name. + + If HAS_JSON_SUFFIX flag is set in the pecan environment, try also looking + for portgroup_ident with '.json' suffix. Otherwise identical + to get_rpc_portgroup. + + :param portgroup_ident: the UUID or logical name of a portgroup. + + :returns: The RPC portgroup. + :raises: InvalidUuidOrName if the name or uuid provided is not valid. + :raises: PortgroupNotFound if the portgroup is not found. + """ + return _get_with_suffix(get_rpc_portgroup, portgroup_ident, + exception.PortgroupNotFound) + + def is_valid_node_name(name): """Determine if the provided name is a valid node name. diff --git a/ironic/api/middleware/__init__.py b/ironic/api/middleware/__init__.py index 385547d70..81bda5d3b 100644 --- a/ironic/api/middleware/__init__.py +++ b/ironic/api/middleware/__init__.py @@ -13,11 +13,14 @@ # under the License. from ironic.api.middleware import auth_token +from ironic.api.middleware import json_ext from ironic.api.middleware import parsable_error ParsableErrorMiddleware = parsable_error.ParsableErrorMiddleware AuthTokenMiddleware = auth_token.AuthTokenMiddleware +JsonExtensionMiddleware = json_ext.JsonExtensionMiddleware __all__ = ('ParsableErrorMiddleware', - 'AuthTokenMiddleware') + 'AuthTokenMiddleware', + 'JsonExtensionMiddleware') diff --git a/ironic/api/middleware/json_ext.py b/ironic/api/middleware/json_ext.py new file mode 100644 index 000000000..c8e89a5de --- /dev/null +++ b/ironic/api/middleware/json_ext.py @@ -0,0 +1,43 @@ +# 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 oslo_log import log + +from ironic.common import utils + + +LOG = log.getLogger(__name__) + + +class JsonExtensionMiddleware(object): + """Simplified processing of .json extension. + + Previously Ironic API used the "guess_content_type_from_ext" feature. + It was never needed, as we never allowed non-JSON content types anyway. + Now that it is removed, this middleware strips .json extension for + backward compatibility. + + """ + def __init__(self, app): + self.app = app + + def __call__(self, env, start_response): + path = utils.safe_rstrip(env.get('PATH_INFO'), '/') + if path and path.endswith('.json'): + LOG.debug('Stripping .json prefix from %s for compatibility ' + 'with pecan', path) + env['PATH_INFO'] = path[:-5] + env['HAS_JSON_SUFFIX'] = True + else: + env['HAS_JSON_SUFFIX'] = False + + return self.app(env, start_response) diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 2b2102817..43de1d8a2 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -157,6 +157,45 @@ class TestListNodes(test_api_base.BaseApiTest): # never expose the chassis_id self.assertNotIn('chassis_id', data) + def test_get_one_with_json(self): + # Test backward compatibility with guess_content_type_from_ext + node = obj_utils.create_test_node(self.context, + chassis_id=self.chassis.id) + data = self.get_json( + '/nodes/%s.json' % node.uuid, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(node.uuid, data['uuid']) + + def test_get_one_with_json_in_name(self): + # Test that it is possible to name a node ending with .json + node = obj_utils.create_test_node(self.context, + name='node.json', + chassis_id=self.chassis.id) + data = self.get_json( + '/nodes/%s' % node.name, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(node.uuid, data['uuid']) + + def test_get_one_with_suffix(self): + # This tests that we don't mess with mime-like suffixes + node = obj_utils.create_test_node(self.context, + name='test.1', + chassis_id=self.chassis.id) + data = self.get_json( + '/nodes/%s' % node.name, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(node.uuid, data['uuid']) + + def test_get_one_with_double_json(self): + # Check that .json is only stripped once + node = obj_utils.create_test_node(self.context, + name='node.json', + chassis_id=self.chassis.id) + data = self.get_json( + '/nodes/%s.json' % node.name, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(node.uuid, data['uuid']) + def test_node_states_field_hidden_in_lower_version(self): node = obj_utils.create_test_node(self.context, chassis_id=self.chassis.id) @@ -1381,7 +1420,7 @@ class TestPatch(test_api_base.BaseApiTest): def setUp(self): super(TestPatch, self).setUp() self.chassis = obj_utils.create_test_chassis(self.context) - self.node = obj_utils.create_test_node(self.context, name='node-57', + self.node = obj_utils.create_test_node(self.context, name='node-57.1', chassis_id=self.chassis.id) self.node_no_name = obj_utils.create_test_node( self.context, uuid='deadbeef-0000-1111-2222-333333333333', @@ -1458,6 +1497,25 @@ class TestPatch(test_api_base.BaseApiTest): self.mock_update_node.assert_called_once_with( mock.ANY, mock.ANY, 'test-topic') + def test_update_ok_by_name_with_json(self): + self.mock_update_node.return_value = self.node + (self + .mock_update_node + .return_value + .updated_at) = "2013-12-03T06:20:41.184720+00:00" + response = self.patch_json( + '/nodes/%s.json' % self.node.name, + [{'path': '/instance_uuid', + 'value': 'aaaaaaaa-1111-bbbb-2222-cccccccccccc', + 'op': 'replace'}], + headers={api_base.Version.string: "1.5"}) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual(self.mock_update_node.return_value.updated_at, + timeutils.parse_isotime(response.json['updated_at'])) + self.mock_update_node.assert_called_once_with( + mock.ANY, mock.ANY, 'test-topic') + def test_update_state(self): response = self.patch_json('/nodes/%s' % self.node.uuid, [{'power_state': 'new state'}], @@ -2780,11 +2838,18 @@ class TestDelete(test_api_base.BaseApiTest): @mock.patch.object(rpcapi.ConductorAPI, 'destroy_node') def test_delete_node_by_name(self, mock_dn): - node = obj_utils.create_test_node(self.context, name='foo') + node = obj_utils.create_test_node(self.context, name='foo.1') self.delete('/nodes/%s' % node.name, headers={api_base.Version.string: "1.5"}) mock_dn.assert_called_once_with(mock.ANY, node.uuid, 'test-topic') + @mock.patch.object(rpcapi.ConductorAPI, 'destroy_node') + def test_delete_node_by_name_with_json(self, mock_dn): + node = obj_utils.create_test_node(self.context, name='foo') + self.delete('/nodes/%s.json' % node.name, + headers={api_base.Version.string: "1.5"}) + mock_dn.assert_called_once_with(mock.ANY, node.uuid, 'test-topic') + @mock.patch.object(objects.Node, 'get_by_uuid') def test_delete_node_not_found(self, mock_gbu): node = obj_utils.get_test_node(self.context) diff --git a/ironic/tests/unit/api/controllers/v1/test_portgroup.py b/ironic/tests/unit/api/controllers/v1/test_portgroup.py index 48b301f55..fb047bfa0 100644 --- a/ironic/tests/unit/api/controllers/v1/test_portgroup.py +++ b/ironic/tests/unit/api/controllers/v1/test_portgroup.py @@ -86,6 +86,29 @@ class TestListPortgroups(test_api_base.BaseApiTest): # never expose the node_id self.assertNotIn('node_id', data) + def test_get_one_with_json(self): + portgroup = obj_utils.create_test_portgroup(self.context, + node_id=self.node.id) + data = self.get_json('/portgroups/%s.json' % portgroup.uuid, + headers=self.headers) + self.assertEqual(portgroup.uuid, data['uuid']) + + def test_get_one_with_json_in_name(self): + portgroup = obj_utils.create_test_portgroup(self.context, + name='pg.json', + node_id=self.node.id) + data = self.get_json('/portgroups/%s' % portgroup.uuid, + headers=self.headers) + self.assertEqual(portgroup.uuid, data['uuid']) + + def test_get_one_with_suffix(self): + portgroup = obj_utils.create_test_portgroup(self.context, + name='pg.1', + node_id=self.node.id) + data = self.get_json('/portgroups/%s' % portgroup.uuid, + headers=self.headers) + self.assertEqual(portgroup.uuid, data['uuid']) + def test_get_one_custom_fields(self): portgroup = obj_utils.create_test_portgroup(self.context, node_id=self.node.id) @@ -477,6 +500,7 @@ class TestPatch(test_api_base.BaseApiTest): super(TestPatch, self).setUp() self.node = obj_utils.create_test_node(self.context) self.portgroup = obj_utils.create_test_portgroup(self.context, + name='pg.1', node_id=self.node.id) p = mock.patch.object(rpcapi.ConductorAPI, 'get_topic_for') @@ -522,6 +546,19 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual(http_client.OK, response.status_code) self.assertEqual(extra, response.json['extra']) + def test_update_byname_with_json(self, mock_upd): + extra = {'foo': 'bar'} + mock_upd.return_value = self.portgroup + mock_upd.return_value.extra = extra + response = self.patch_json('/portgroups/%s.json' % self.portgroup.name, + [{'path': '/extra/foo', + 'value': 'bar', + 'op': 'add'}], + headers=self.headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual(extra, response.json['extra']) + def test_update_invalid_name(self, mock_upd): mock_upd.return_value = self.portgroup response = self.patch_json('/portgroups/%s' % self.portgroup.name, @@ -1211,6 +1248,7 @@ class TestDelete(test_api_base.BaseApiTest): super(TestDelete, self).setUp() self.node = obj_utils.create_test_node(self.context) self.portgroup = obj_utils.create_test_portgroup(self.context, + name='pg.1', node_id=self.node.id) gtf = mock.patch.object(rpcapi.ConductorAPI, 'get_topic_for') @@ -1269,6 +1307,11 @@ class TestDelete(test_api_base.BaseApiTest): headers=self.headers) self.assertTrue(mock_dpt.called) + def test_delete_portgroup_byname_with_json(self, mock_dpt): + self.delete('/portgroups/%s.json' % self.portgroup.name, + headers=self.headers) + self.assertTrue(mock_dpt.called) + def test_delete_portgroup_byname_not_existed(self, mock_dpt): res = self.delete('/portgroups/%s' % 'blah', expect_errors=True, headers=self.headers) diff --git a/ironic/tests/unit/api/controllers/v1/test_ramdisk.py b/ironic/tests/unit/api/controllers/v1/test_ramdisk.py index 0c9e77419..ec667634f 100644 --- a/ironic/tests/unit/api/controllers/v1/test_ramdisk.py +++ b/ironic/tests/unit/api/controllers/v1/test_ramdisk.py @@ -189,6 +189,32 @@ class TestHeartbeat(test_api_base.BaseApiTest): topic='test-topic') @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) + def test_ok_with_json(self, mock_heartbeat): + node = obj_utils.create_test_node(self.context) + response = self.post_json( + '/heartbeat/%s.json' % node.uuid, + {'callback_url': 'url'}, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(http_client.ACCEPTED, response.status_int) + self.assertEqual(b'', response.body) + mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, + node.uuid, 'url', None, + topic='test-topic') + + @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) + def test_ok_by_name(self, mock_heartbeat): + node = obj_utils.create_test_node(self.context, name='test.1') + response = self.post_json( + '/heartbeat/%s' % node.name, + {'callback_url': 'url'}, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(http_client.ACCEPTED, response.status_int) + self.assertEqual(b'', response.body) + mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, + node.uuid, 'url', None, + topic='test-topic') + + @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) def test_ok_agent_version(self, mock_heartbeat): node = obj_utils.create_test_node(self.context) response = self.post_json( diff --git a/releasenotes/notes/name-suffix-47aea2d265fa75ae.yaml b/releasenotes/notes/name-suffix-47aea2d265fa75ae.yaml new file mode 100644 index 000000000..ed8c4dc22 --- /dev/null +++ b/releasenotes/notes/name-suffix-47aea2d265fa75ae.yaml @@ -0,0 +1,19 @@ +--- +fixes: + - | + Nodes and port groups with names ending with known file extensions are now + correctly handled by the API. See `bug 1643995 + <https://bugs.launchpad.net/bugs/1643995>`_ for more details. +issues: + - | + If you have two nodes or port groups with names that only differ in + a ``.json`` suffix (for example, ``test`` and ``test.json``) you won't be + able to get, update or delete the one with the suffix via the + ``/v1/nodes/<node>`` endpoint (``/v1/portgroups/<portgroup>`` for port + groups). Similarly, the ``/v1/heartbeat/<node>`` endpoint won't work + for the node with the suffix. + + To work around it, add one more ``.json`` suffix (for example, use + ``/v1/nodes/test`` for node ``test`` and ``/v1/nodes/test.json.json`` + for ``test.json``). This issue will be addressed in one of the future + API revisions. |