From 3fe4c767d57a8b9bf9713884a779482cddc810da Mon Sep 17 00:00:00 2001 From: Yumeng Bao Date: Wed, 16 Sep 2020 23:57:46 +0800 Subject: [PATCH] Add rc check for POST Device Profile At present, when creating a device profile, the function _validate_post_request only checks if the keys of groups matches that in ["resources:", "trait:", "accel:"] without checking the string following "trait:","resources:",this introduces risks. For example, the string followed by "trait:" default should be "CUSTOM_", what follows "resources:" should be a valid resource type, but the fact is that users may make typos such as adding extra spaces like "resources: FPGA", or other format errors. This kind of error cannot be detected by the current _validate_post_request, and it won't be exposed until nova parses the rc of cyborg. This is not in compliance with the process specification and potentially introduces greater risks. Therefore, it is necessary to implement a more strict inspection of rc in device profile POST API. This patch added the rc check. 1.Check the legality of rc name 2.Space removal in rc 3.rc value check Change-Id: Iabad6b1c3ad2b4f61e69249a2867bd68107d47d5 Story: 2008143 Task: 40883 --- cyborg/api/controllers/v2/device_profiles.py | 19 +++++++ cyborg/objects/extarq/ext_arq_job.py | 17 ++----- .../controllers/v2/test_device_profiles.py | 51 +++++++++++++++++++ cyborg/tests/unit/fake_device_profile.py | 4 +- cyborg/tests/unit/objects/test_ext_arq_job.py | 2 +- 5 files changed, 76 insertions(+), 17 deletions(-) diff --git a/cyborg/api/controllers/v2/device_profiles.py b/cyborg/api/controllers/v2/device_profiles.py index 7b30ba64..49d4aecb 100644 --- a/cyborg/api/controllers/v2/device_profiles.py +++ b/cyborg/api/controllers/v2/device_profiles.py @@ -28,6 +28,7 @@ from cyborg.api.controllers import link from cyborg.api.controllers import types from cyborg.api import expose from cyborg.common import authorize_wsgi +from cyborg.common import constants from cyborg.common import exception from cyborg import objects LOG = log.getLogger(__name__) @@ -160,6 +161,24 @@ class DeviceProfilesController(base.CyborgController, del group[key] standard_key = "trait:" + inner_trait group[standard_key] = value + # check rc name and it's value + if key.startswith("resources:"): + inner_origin_rc = ":".join(key.split(":")[1:]) + inner_rc = inner_origin_rc.strip(" ") + if inner_rc not in constants.SUPPORT_RESOURCES and \ + not inner_rc.startswith('CUSTOM_'): + raise exception.InvalidParameterValue( + err="Unsupported resource class %s" % inner_rc) + try: + int(value) + except ValueError: + raise exception.InvalidParameterValue( + err="Resources nummber %s is invalid" % value) + # strip " " and update old group key. + if inner_origin_rc != inner_rc: + del group[key] + standard_key = "resources:" + inner_rc + group[standard_key] = value def _get_device_profile_list(self, names=None, uuid=None): """Get a list of API objects representing device profiles.""" diff --git a/cyborg/objects/extarq/ext_arq_job.py b/cyborg/objects/extarq/ext_arq_job.py index 75736443..d0a1b829 100644 --- a/cyborg/objects/extarq/ext_arq_job.py +++ b/cyborg/objects/extarq/ext_arq_job.py @@ -49,7 +49,7 @@ class ExtARQJobMixin(object): def get_suitable_ext_arq(cls, context, uuid): """From the inherit subclass find the suitable ExtARQ.""" extarq = cls.get(context, uuid) - typ, _ = extarq.get_resources_from_device_profile_group() + typ = extarq.get_resources_from_device_profile_group() factory = cls.factory(typ) if factory != cls: return factory.get(context, uuid) @@ -222,19 +222,8 @@ class ExtARQJobMixin(object): if not resources: raise exception.InvalidParameterValue( 'No resources in device_profile_group: %s' % group) - res_type, res_num = resources[0] - # TODO(Sundar): this should be caught in ARQ create, not bind. - if res_type not in constants.SUPPORT_RESOURCES: - raise exception.InvalidParameterValue( - 'Unsupport resources %s from device_profile_group: %s' % - (res_type, group)) - try: - res_num = int(res_num) - except ValueError: - raise exception.InvalidParameterValue( - 'Resources nummber is a invalid in' - ' device_profile_group: %s' % group) - return res_type, res_num + res_type = resources[0][0] + return res_type @classmethod def apply_patch(cls, context, patch_list, valid_fields): diff --git a/cyborg/tests/unit/api/controllers/v2/test_device_profiles.py b/cyborg/tests/unit/api/controllers/v2/test_device_profiles.py index c1a2fe67..59cbded3 100644 --- a/cyborg/tests/unit/api/controllers/v2/test_device_profiles.py +++ b/cyborg/tests/unit/api/controllers/v2/test_device_profiles.py @@ -124,6 +124,57 @@ class TestDeviceProfileController(v2_test.APITestV2): # {'trait:CUSTOM_FPGA_INTEL_PAC_ARRIA10': 'required} self.assertTrue(out_dp['groups'] == self.fake_dp_objs[0]['groups']) + @mock.patch('cyborg.conductor.rpcapi.ConductorAPI.device_profile_create') + def test_create_with_extra_space_in_rc(self, mock_cond_dp): + test_unsupport_dp = self.fake_dps[0] + + # generate a requested dp which has extra space in rc + del test_unsupport_dp['groups'][0]['resources:FPGA'] + test_unsupport_dp['groups'][0]['resources: FPGA '] = '1' + dp = [test_unsupport_dp] + + mock_cond_dp.return_value = self.fake_dp_objs[0] + dp[0]['created_at'] = str(dp[0]['created_at']) + + response = self.post_json(self.DP_URL, dp, headers=self.headers) + out_dp = jsonutils.loads(response.controller_output) + + # check that the extra space in rc:{'resources: FPGA ': '1'} is + # successful stripped by the _validate_post_request function, and + # the created device_profile has no extra space in + # rc:{'resources:FPGA': '1'} + self.assertTrue(out_dp['groups'] == self.fake_dp_objs[0]['groups']) + + def test_create_with_unsupported_rc(self): + test_unsupport_dp = self.fake_dps[0] + # generate a special rc for test + del test_unsupport_dp['groups'][0]['resources:FPGA'] + test_unsupport_dp['groups'][0]["resources:FAKE_RC"] = '1' + + dp = [test_unsupport_dp] + dp[0]['created_at'] = str(dp[0]['created_at']) + self.assertRaisesRegex( + webtest.app.AppError, + ".*Unsupported resource class FAKE_RC.*", + self.post_json, + self.DP_URL, + dp, + headers=self.headers) + + def test_create_with_invalid_resource_value(self): + test_unsupport_dp = self.fake_dps[0] + del test_unsupport_dp['groups'][0]['resources:FPGA'] + test_unsupport_dp['groups'][0]["resources:CUSTOM_FAKE_RC"] = 'fake' + dp = [test_unsupport_dp] + dp[0]['created_at'] = str(dp[0]['created_at']) + self.assertRaisesRegex( + webtest.app.AppError, + ".*Resources nummber fake is invalid.*", + self.post_json, + self.DP_URL, + dp, + headers=self.headers) + @mock.patch('cyborg.conductor.rpcapi.ConductorAPI.device_profile_delete') @mock.patch('cyborg.objects.DeviceProfile.get_by_name') @mock.patch('cyborg.objects.DeviceProfile.get_by_uuid') diff --git a/cyborg/tests/unit/fake_device_profile.py b/cyborg/tests/unit/fake_device_profile.py index 982d6115..4e8e9123 100644 --- a/cyborg/tests/unit/fake_device_profile.py +++ b/cyborg/tests/unit/fake_device_profile.py @@ -41,7 +41,7 @@ def _get_device_profiles_as_dict(): "created_at": date1, "updated_at": None, "groups": [ - {"resources:ACCELERATOR_FPGA": "1", + {"resources:FPGA": "1", "trait:CUSTOM_FPGA_INTEL_PAC_ARRIA10": "required", "trait:CUSTOM_FUNCTION_ID_3AFB": "required", }, @@ -58,7 +58,7 @@ def _get_device_profiles_as_dict(): "updated_at": None, "description": "fake-daas_example_2-desc", "groups": [ - {"resources:ACCELERATOR_FPGA": "1", + {"resources:FPGA": "1", "trait:CUSTOM_REGION_ID_3ACD": "required", "accel:bitstream_id": "ea0d149c-8555-495b-bc79-608d7bab1260" } diff --git a/cyborg/tests/unit/objects/test_ext_arq_job.py b/cyborg/tests/unit/objects/test_ext_arq_job.py index 023b1835..e835e247 100644 --- a/cyborg/tests/unit/objects/test_ext_arq_job.py +++ b/cyborg/tests/unit/objects/test_ext_arq_job.py @@ -53,7 +53,7 @@ class TestExtARQJobMixin(base.DbTestCase): "device_profile_group"][constants.ACCEL_FUNCTION_ID] def test_get_resources_from_device_profile_group(self): - expect = [("GPU", 1)] + [("FPGA", 1)] * 4 + expect = [("GPU")] + [("FPGA")] * 4 actual = [v.get_resources_from_device_profile_group() for v in self.class_objects.values()] self.assertEqual(expect, actual)