Stop guessing mime types based on URLs

Currently we have a pecan feature enabled that strips extensions
from the end of the URL and treat it like requested content type.
E.g. /v1/nodes.json is treated as /v1/nodes with requested content
type Application/Json. However, this prevents certain node names:
e.g. /v1/nodes/small.1 is treated like /v1/nodes/small with content
type of a man page. It does not make any sense for ironic API,
as we only support Application/Json content type (and .json suffix).

This change disabled this pecan feature. To keep backward compability
a new middleware stips the .json prefix and saves a flag in the
environment about its presence. API accepting names try to find
their resource first without, then with .json suffix.

The following endpoints are special-cased to support names with .json:
* Node GET, PATCH and DELETE
* Ramdisk heartbeat
* Port group GET, PATCH and DELETE

VIF API is not updated, so VIF IDs still cannot have .json suffix.

Change-Id: I789ecfeac9b64a9c4105a20619f7bf5dfc133189
Closes-Bug: #1643995

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
 
 

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)

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)
 

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)

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.
 

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

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)

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)

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)

ironic/tests/unit/api/controllers/v1/test_ramdisk.py

@@ -188,6 +188,32 @@ class TestHeartbeat(test_api_base.BaseApiTest):
                                                node.uuid, 'url', None,
                                                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)

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.