Prevent URL collisions with sub-controllers: nodes/ports

This patch removes the route v1/nodes/ports by removing the ports
controllers from being a class attribute of the nodes controller, and
starts using the pecan _lookup function to process sub-resources.

Co-Authored-By: Vladyslav Drok <vdrok@mirantis.com>
Partial-Bug: #1580997
Change-Id: I1f76a5f2ea45629a000e61a215b2b32fdd52fb37

ironic/api/controllers/v1/node.py

@@ -1023,19 +1023,12 @@ class NodesController(rest.RestController):
     """A resource used for vendors to expose a custom functionality in
     the API"""
 
-    ports = port.PortsController()
-    """Expose ports as a sub-element of nodes"""
-
     management = NodeManagementController()
     """Expose management as a sub-element of nodes"""
 
     maintenance = NodeMaintenanceController()
     """Expose maintenance as a sub-element of nodes"""
 
-    # Set the flag to indicate that the requests to this resource are
-    # coming from a top-level resource
-    ports.from_nodes = True
-
     from_chassis = False
     """A flag to indicate if the requests to this controller are coming
     from the top-level resource Chassis"""
@@ -1049,6 +1042,17 @@ class NodesController(rest.RestController):
                              'instance_info', 'driver_internal_info',
                              'clean_step', 'raid_config', 'target_raid_config']
 
+    _subcontroller_map = {
+        'ports': port.PortsController
+    }
+
+    @pecan.expose()
+    def _lookup(self, ident, subres, *remainder):
+        ident = types.uuid_or_name.validate(ident)
+        subcontroller = self._subcontroller_map.get(subres)
+        if subcontroller:
+            return subcontroller(node_ident=ident), remainder
+
     def _get_nodes_collection(self, chassis_uuid, instance_uuid, associated,
                               maintenance, provision_state, marker, limit,
                               sort_key, sort_dir, driver=None,

ironic/api/controllers/v1/port.py

@@ -215,10 +215,6 @@ class PortCollection(collection.Collection):
 class PortsController(rest.RestController):
     """REST controller for Ports."""
 
-    from_nodes = False
-    """A flag to indicate if the requests to this controller are coming
-    from the top-level resource Nodes."""
-
     _custom_actions = {
         'detail': ['GET'],
     }
@@ -227,12 +223,13 @@ class PortsController(rest.RestController):
 
     advanced_net_fields = ['pxe_enabled', 'local_link_connection']
 
+    def __init__(self, node_ident=None):
+        super(PortsController, self).__init__()
+        self.parent_node_ident = node_ident
+
     def _get_ports_collection(self, node_ident, address, marker, limit,
                               sort_key, sort_dir, resource_url=None,
                               fields=None):
-        if self.from_nodes and not node_ident:
-            raise exception.MissingParameterValue(
-                _("Node identifier not specified."))
 
         limit = api_utils.validate_limit(limit)
         sort_dir = api_utils.validate_sort_dir(sort_dir)
@@ -247,6 +244,7 @@ class PortsController(rest.RestController):
                 _("The sort_key value %(key)s is an invalid field for "
                   "sorting") % {'key': sort_key})
 
+        node_ident = self.parent_node_ident or node_ident
         if node_ident:
             # FIXME(comstud): Since all we need is the node ID, we can
             #                 make this more efficient by only querying
@@ -395,7 +393,7 @@ class PortsController(rest.RestController):
         cdict = pecan.request.context.to_dict()
         policy.authorize('baremetal:port:get', cdict, cdict)
 
-        if self.from_nodes:
+        if self.parent_node_ident:
             raise exception.OperationNotPermitted()
 
         api_utils.check_allow_specify_fields(fields)
@@ -414,7 +412,7 @@ class PortsController(rest.RestController):
         cdict = pecan.request.context.to_dict()
         policy.authorize('baremetal:port:create', cdict, cdict)
 
-        if self.from_nodes:
+        if self.parent_node_ident:
             raise exception.OperationNotPermitted()
 
         pdict = port.as_dict()
@@ -443,7 +441,7 @@ class PortsController(rest.RestController):
         cdict = pecan.request.context.to_dict()
         policy.authorize('baremetal:port:update', cdict, cdict)
 
-        if self.from_nodes:
+        if self.parent_node_ident:
             raise exception.OperationNotPermitted()
         if not api_utils.allow_port_advanced_net_fields():
             for field in self.advanced_net_fields:
@@ -495,7 +493,7 @@ class PortsController(rest.RestController):
         cdict = pecan.request.context.to_dict()
         policy.authorize('baremetal:port:delete', cdict, cdict)
 
-        if self.from_nodes:
+        if self.parent_node_ident:
             raise exception.OperationNotPermitted()
         rpc_port = objects.Port.get_by_uuid(pecan.request.context,
                                             port_uuid)

ironic/tests/unit/api/v1/test_nodes.py

@@ -477,7 +477,7 @@ class TestListNodes(test_api_base.BaseApiTest):
         obj_utils.create_test_port(self.context, node_id=node.id)
         # No node id specified
         response = self.get_json('/nodes/ports', expect_errors=True)
-        self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
 
     def test_ports_subresource_node_not_found(self):
         non_existent_uuid = 'eeeeeeee-cccc-aaaa-bbbb-cccccccccccc'
@@ -1184,10 +1184,18 @@ class TestPatch(test_api_base.BaseApiTest):
         self.mock_update_node.assert_called_once_with(
             mock.ANY, mock.ANY, 'test-topic')
 
-    def test_patch_ports_subresource(self):
+    def test_patch_ports_subresource_no_port_id(self):
         response = self.patch_json('/nodes/%s/ports' % self.node.uuid,
                                    [{'path': '/extra/foo', 'value': 'bar',
                                      'op': 'add'}], expect_errors=True)
+        self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+
+    def test_patch_ports_subresource(self):
+        response = self.patch_json(
+            '/nodes/%s/ports/9bb50f13-0b8d-4ade-ad2d-d91fefdef9cc' %
+            self.node.uuid,
+            [{'path': '/extra/foo', 'value': 'bar',
+              'op': 'add'}], expect_errors=True)
         self.assertEqual(http_client.FORBIDDEN, response.status_int)
 
     def test_remove_uuid(self):
@@ -1876,12 +1884,20 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual(http_client.BAD_REQUEST, response.status_code)
         self.assertTrue(response.json['error_message'])
 
-    def test_post_ports_subresource(self):
+    def test_post_ports_subresource_no_node_id(self):
         node = obj_utils.create_test_node(self.context)
         pdict = test_api_utils.port_post_data(node_id=None)
         pdict['node_uuid'] = node.uuid
         response = self.post_json('/nodes/ports', pdict,
                                   expect_errors=True)
+        self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+
+    def test_post_ports_subresource(self):
+        node = obj_utils.create_test_node(self.context)
+        pdict = test_api_utils.port_post_data(node_id=None)
+        pdict['node_uuid'] = node.uuid
+        response = self.post_json('/nodes/%s/ports' % node.uuid, pdict,
+                                  expect_errors=True)
         self.assertEqual(http_client.FORBIDDEN, response.status_int)
 
     def test_create_node_no_mandatory_field_driver(self):
@@ -2074,10 +2090,19 @@ class TestDelete(test_api_base.BaseApiTest):
         self.assertTrue(response.json['error_message'])
         mock_gbn.assert_called_once_with(mock.ANY, node.name)
 
-    def test_delete_ports_subresource(self):
+    def test_delete_ports_subresource_no_port_id(self):
         node = obj_utils.create_test_node(self.context)
         response = self.delete('/nodes/%s/ports' % node.uuid,
                                expect_errors=True)
+        self.assertEqual(http_client.BAD_REQUEST, response.status_int)
+
+    def test_delete_ports_subresource(self):
+        node = obj_utils.create_test_node(self.context)
+        port = obj_utils.create_test_port(self.context, node_id=node.id)
+        response = self.delete(
+            '/nodes/%(node_uuid)s/ports/%(port_uuid)s' %
+            {'node_uuid': node.uuid, 'port_uuid': port.uuid},
+            expect_errors=True)
         self.assertEqual(http_client.FORBIDDEN, response.status_int)
 
     @mock.patch.object(rpcapi.ConductorAPI, 'destroy_node')

ironic/tests/unit/api/v1/test_utils.py

@@ -462,7 +462,7 @@ class TestVendorPassthru(base.TestCase):
         self._test_vendor_passthru_attach(b'\x00\x01', b'\x00\x01')
 
     def test_get_controller_reserved_names(self):
-        expected = ['maintenance', 'management', 'ports', 'states',
+        expected = ['maintenance', 'management', 'states',
                     'vendor_passthru', 'validate', 'detail']
         self.assertEqual(sorted(expected),
                          sorted(utils.get_controller_reserved_names(

releasenotes/notes/fix-url-collisions-43abfc8364ca34e7.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - Removed invalid accidental API URL v1/nodes/ports
+    See https://bugs.launchpad.net/ironic/+bug/1580997