Merge "Check microversions before validations for allocations and deploy templates"

ironic/api/controllers/v1/allocation.py

@@ -16,6 +16,7 @@ from ironic_lib import metrics_utils
 from oslo_utils import uuidutils
 import pecan
 from six.moves import http_client
+from webob import exc as webob_exc
 import wsme
 from wsme import types as wtypes
 
@@ -200,6 +201,13 @@ class AllocationsController(pecan.rest.RestController):
 
     invalid_sort_key_list = ['extra', 'candidate_nodes', 'traits']
 
+    @pecan.expose()
+    def _route(self, args, request=None):
+        if not api_utils.allow_allocations():
+            raise webob_exc.HTTPNotFound(_(
+                "The API version does not allow allocations"))
+        return super(AllocationsController, self)._route(args, request)
+
     def _get_allocations_collection(self, node_ident=None, resource_class=None,
                                     state=None, marker=None, limit=None,
                                     sort_key='id', sort_dir='asc',
@@ -282,9 +290,6 @@ class AllocationsController(pecan.rest.RestController):
         :param fields: Optional, a list with a specified set of fields
                        of the resource to be returned.
         """
-        if not api_utils.allow_allocations():
-            raise exception.NotFound()
-
         cdict = pecan.request.context.to_policy_values()
         policy.authorize('baremetal:allocation:get', cdict, cdict)
 
@@ -302,9 +307,6 @@ class AllocationsController(pecan.rest.RestController):
         :param fields: Optional, a list with a specified set of fields
                        of the resource to be returned.
         """
-        if not api_utils.allow_allocations():
-            raise exception.NotFound()
-
         cdict = pecan.request.context.to_policy_values()
         policy.authorize('baremetal:allocation:get', cdict, cdict)
 
@@ -320,9 +322,6 @@ class AllocationsController(pecan.rest.RestController):
 
         :param allocation: an allocation within the request body.
         """
-        if not api_utils.allow_allocations():
-            raise exception.NotFound()
-
         context = pecan.request.context
         cdict = context.to_policy_values()
         policy.authorize('baremetal:allocation:create', cdict, cdict)
@@ -383,9 +382,6 @@ class AllocationsController(pecan.rest.RestController):
 
         :param allocation_ident: UUID or logical name of an allocation.
         """
-        if not api_utils.allow_allocations():
-            raise exception.NotFound()
-
         context = pecan.request.context
         cdict = context.to_policy_values()
         policy.authorize('baremetal:allocation:delete', cdict, cdict)
@@ -414,6 +410,13 @@ class NodeAllocationController(pecan.rest.RestController):
 
     invalid_sort_key_list = ['extra', 'candidate_nodes', 'traits']
 
+    @pecan.expose()
+    def _route(self, args, request=None):
+        if not api_utils.allow_allocations():
+            raise webob_exc.HTTPNotFound(_(
+                "The API version does not allow allocations"))
+        return super(NodeAllocationController, self)._route(args, request)
+
     def __init__(self, node_ident):
         super(NodeAllocationController, self).__init__()
         self.parent_node_ident = node_ident
@@ -422,9 +425,6 @@ class NodeAllocationController(pecan.rest.RestController):
     @METRICS.timer('NodeAllocationController.get_all')
     @expose.expose(Allocation, types.listtype)
     def get_all(self, fields=None):
-        if not api_utils.allow_allocations():
-            raise exception.NotFound()
-
         cdict = pecan.request.context.to_policy_values()
         policy.authorize('baremetal:allocation:get', cdict, cdict)
 
@@ -440,9 +440,6 @@ class NodeAllocationController(pecan.rest.RestController):
     @METRICS.timer('NodeAllocationController.delete')
     @expose.expose(None, status_code=http_client.NO_CONTENT)
     def delete(self):
-        if not api_utils.allow_allocations():
-            raise exception.NotFound()
-
         context = pecan.request.context
         cdict = context.to_policy_values()
         policy.authorize('baremetal:allocation:delete', cdict, cdict)

ironic/api/controllers/v1/deploy_template.py

@@ -20,6 +20,7 @@ from oslo_utils import uuidutils
 import pecan
 from pecan import rest
 from six.moves import http_client
+from webob import exc as webob_exc
 import wsme
 from wsme import types as wtypes
 
@@ -46,11 +47,6 @@ _DEPLOY_INTERFACE_TYPE = wtypes.Enum(
     wtypes.text, *conductor_utils.DEPLOYING_INTERFACE_PRIORITY)
 
 
-def _check_api_version():
-    if not api_utils.allow_deploy_templates():
-        raise exception.NotFound()
-
-
 class DeployStepType(wtypes.Base, base.AsDictMixin):
     """A type describing a deployment step."""
 
@@ -260,6 +256,13 @@ class DeployTemplatesController(rest.RestController):
 
     invalid_sort_key_list = ['extra', 'steps']
 
+    @pecan.expose()
+    def _route(self, args, request=None):
+        if not api_utils.allow_deploy_templates():
+            raise webob_exc.HTTPNotFound(_(
+                "The API version does not allow deploy templates"))
+        return super(DeployTemplatesController, self)._route(args, request)
+
     def _update_changed_fields(self, template, rpc_template):
         """Update rpc_template based on changed fields in a template."""
         for field in objects.DeployTemplate.fields:
@@ -295,7 +298,6 @@ class DeployTemplatesController(rest.RestController):
         :param detail: Optional, boolean to indicate whether retrieve a list
                        of deploy templates with detail.
         """
-        _check_api_version()
         api_utils.check_policy('baremetal:deploy_template:get')
 
         api_utils.check_allowed_fields(fields)
@@ -338,7 +340,6 @@ class DeployTemplatesController(rest.RestController):
         :param fields: Optional, a list with a specified set of fields
             of the resource to be returned.
         """
-        _check_api_version()
         api_utils.check_policy('baremetal:deploy_template:get')
 
         api_utils.check_allowed_fields(fields)
@@ -356,7 +357,6 @@ class DeployTemplatesController(rest.RestController):
 
         :param template: a deploy template within the request body.
         """
-        _check_api_version()
         api_utils.check_policy('baremetal:deploy_template:create')
 
         context = pecan.request.context
@@ -387,7 +387,6 @@ class DeployTemplatesController(rest.RestController):
         :param template_ident: UUID or logical name of a deploy template.
         :param patch: a json PATCH document to apply to this deploy template.
         """
-        _check_api_version()
         api_utils.check_policy('baremetal:deploy_template:update')
 
         context = pecan.request.context
@@ -434,7 +433,6 @@ class DeployTemplatesController(rest.RestController):
 
         :param template_ident: UUID or logical name of a deploy template.
         """
-        _check_api_version()
         api_utils.check_policy('baremetal:deploy_template:delete')
 
         context = pecan.request.context

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

@@ -152,6 +152,14 @@ class TestListAllocations(test_api_base.BaseApiTest):
             expect_errors=True)
         self.assertEqual(http_client.NOT_FOUND, response.status_int)
 
+    def test_get_one_invalid_api_version_without_check(self):
+        # Invalid name, but the check happens after the microversion check.
+        response = self.get_json(
+            '/allocations/ba!na!na!',
+            headers={api_base.Version.string: str(api_v1.min_version())},
+            expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_many(self):
         allocations = []
         for id_ in range(5):
@@ -326,6 +334,14 @@ class TestListAllocations(test_api_base.BaseApiTest):
         self.assertEqual({}, data["extra"])
         self.assertEqual(self.node.uuid, data["node_uuid"])
 
+    def test_get_by_node_resource_invalid_api_version(self):
+        obj_utils.create_test_allocation(self.context, node_id=self.node.id)
+        response = self.get_json(
+            '/nodes/%s/allocation' % self.node.uuid,
+            headers={api_base.Version.string: str(api_v1.min_version())},
+            expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_get_by_node_resource_with_fields(self):
         obj_utils.create_test_allocation(self.context, node_id=self.node.id)
         data = self.get_json('/nodes/%s/allocation?fields=name,extra' %
@@ -666,6 +682,14 @@ class TestDelete(test_api_base.BaseApiTest):
                                headers={api_base.Version.string: '1.14'})
         self.assertEqual(http_client.NOT_FOUND, response.status_int)
 
+    def test_delete_allocation_invalid_api_version_without_check(self,
+                                                                 mock_destroy):
+        # Invalid name, but the check happens after the microversion check.
+        response = self.delete('/allocations/ba!na!na1',
+                               expect_errors=True,
+                               headers={api_base.Version.string: '1.14'})
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_delete_allocation_by_name(self, mock_destroy):
         self.delete('/allocations/%s' % self.allocation.name,
                     headers=self.headers)
@@ -708,3 +732,12 @@ class TestDelete(test_api_base.BaseApiTest):
         res = self.delete('/nodes/%s/allocation' % uuidutils.generate_uuid(),
                           expect_errors=True, headers=self.headers)
         self.assertEqual(http_client.NOT_FOUND, res.status_code)
+
+    def test_delete_allocation_by_node_invalid_api_version(self, mock_destroy):
+        obj_utils.create_test_allocation(self.context, node_id=self.node.id)
+        response = self.delete(
+            '/nodes/%s/allocation' % self.node.uuid,
+            headers={api_base.Version.string: str(api_v1.min_version())},
+            expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+        self.assertFalse(mock_destroy.called)

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

@@ -417,6 +417,16 @@ class TestPatch(BaseDeployTemplatesAPITest):
         self.assertEqual(http_client.NOT_FOUND, response.status_int)
         self.assertFalse(mock_save.called)
 
+    def test_update_by_name_old_api_version(self, mock_save):
+        name = 'CUSTOM_DT2'
+        response = self.patch_json('/deploy_templates/%s' % self.template.name,
+                                   [{'path': '/name',
+                                     'value': name,
+                                     'op': 'add'}],
+                                   expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+        self.assertFalse(mock_save.called)
+
     def test_update_not_found(self, mock_save):
         name = 'CUSTOM_DT2'
         uuid = uuidutils.generate_uuid()
@@ -936,6 +946,13 @@ class TestDelete(BaseDeployTemplatesAPITest):
                                headers=self.invalid_version_headers)
         self.assertEqual(http_client.NOT_FOUND, response.status_int)
 
+    def test_delete_old_api_version(self, mock_dpt):
+        # Names like CUSTOM_1 were not valid in API 1.1, but the check should
+        # go after the microversion check.
+        response = self.delete('/deploy_templates/%s' % self.template.name,
+                               expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_delete_by_name_non_existent(self, mock_dpt):
         res = self.delete('/deploy_templates/%s' % 'blah', expect_errors=True,
                           headers=self.headers)