From c87b75e0086f65bbdebb07218fd39c57a8d99ed3 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 6 Apr 2020 16:41:30 +0100 Subject: [PATCH] Drop concept of '?validation' parameter We had planned to introduce a 'validation' parameter to the various extra spec APIs in change If67f0d924ea372746a6dc440ea7bdc655e4f0bea to configure a policy for extra spec validation. Following reviews, we've decided to instead go with a strict policy for all known namespaces and no validation for any unknown namespaces. Update the tests we had in place for this. Part of blueprint flavor-extra-spec-validators Change-Id: If30990ec1c43177b7d13bd8ee1c5dc481265e47b Signed-off-by: Stephen Finucane --- .../openstack/compute/flavors_extraspecs.py | 3 +- nova/api/validation/extra_specs/validators.py | 12 +- .../compute/test_flavors_extra_specs.py | 138 +++--------------- .../validation/extra_specs/test_validators.py | 77 +++++----- 4 files changed, 68 insertions(+), 162 deletions(-) diff --git a/nova/api/openstack/compute/flavors_extraspecs.py b/nova/api/openstack/compute/flavors_extraspecs.py index 481a16efedcc..cabae5c6e38d 100644 --- a/nova/api/openstack/compute/flavors_extraspecs.py +++ b/nova/api/openstack/compute/flavors_extraspecs.py @@ -37,7 +37,6 @@ class FlavorExtraSpecsController(wsgi.Controller): def _check_extra_specs_value(self, req, specs): # TODO(stephenfin): Wire this up to check the API microversion validation_supported = False - validation_mode = 'strict' for name, value in specs.items(): # NOTE(gmann): Max length for numeric value is being checked @@ -53,7 +52,7 @@ class FlavorExtraSpecsController(wsgi.Controller): explanation=error.format_message()) if validation_supported: - validators.validate(name, value, validation_mode) + validators.validate(name, value) @wsgi.expected_errors(404) def index(self, req, flavor_id): diff --git a/nova/api/validation/extra_specs/validators.py b/nova/api/validation/extra_specs/validators.py index 18b80b2b4d5d..a1e3d9d50e5f 100644 --- a/nova/api/validation/extra_specs/validators.py +++ b/nova/api/validation/extra_specs/validators.py @@ -26,19 +26,16 @@ from nova import exception LOG = logging.getLogger(__name__) VALIDATORS: ty.Dict[str, base.ExtraSpecValidator] = {} +NAMESPACES: ty.Set[str] = set() -def validate(name: str, value: str, mode: str): +def validate(name: str, value: str): """Validate a given extra spec. :param name: Extra spec name. :param value: Extra spec value. - :param mode: Validation mode; one of: strict, permissive, disabled :raises: exception.ValidationError if validation fails. """ - if mode == 'disabled': - return - # attempt a basic lookup for extra specs without embedded parameters if name in VALIDATORS: VALIDATORS[name].validate(name, value) @@ -50,7 +47,8 @@ def validate(name: str, value: str, mode: str): validator.validate(name, value) return - if mode == 'permissive': # unregistered extra spec, ignore + namespace = name.split(':', 1)[0].split('{')[0] if ':' in name else None + if not namespace or namespace not in NAMESPACES: # unregistered namespace return raise exception.ValidationError( @@ -74,6 +72,8 @@ def load_validators(): # TODO(stephenfin): Make 'register' return a dict rather than a list? for validator in ext.plugin.register(): VALIDATORS[validator.name] = validator + if ':' in validator.name: + NAMESPACES.add(validator.name.split(':', 1)[0].split('{')[0]) load_validators() diff --git a/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py b/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py index 141c1c09ec8f..e9386211cb87 100644 --- a/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py +++ b/nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py @@ -268,13 +268,13 @@ class FlavorsExtraSpecsTestV21(test.TestCase): # TODO(stephenfin): Wire the microversion up @unittest.expectedFailure - def test_create_invalid_specs_strict(self): - """Test behavior of strict validator.""" + def test_create_invalid_known_namespace(self): + """Test behavior of validator with specs from known namespace.""" invalid_specs = { 'hw:numa_nodes': 'foo', 'hw:cpu_policy': 'sharrred', - 'foo': 'bar', 'hw:cpu_policyyyyyyy': 'shared', + 'hw:foo': 'bar', } for key, value in invalid_specs.items(): body = {'extra_specs': {key: value}} @@ -286,68 +286,20 @@ class FlavorsExtraSpecsTestV21(test.TestCase): ): self.controller.create(req, 1, body=body) - # TODO(stephenfin): Wire the microversion up - @unittest.expectedFailure - def test_create_invalid_specs_permissive(self): - """Test behavior of permissive validator.""" - invalid_specs = { - 'hw:numa_nodes': 'foo', - 'hw:cpu_policy': 'sharrred', - } - for key, value in invalid_specs.items(): - body = {'extra_specs': {key: value}} - req = self._get_request( - '1/os-extra_specs?validation=permissive', - use_admin_context=True, version='2.82', - ) - with testtools.ExpectedException( - self.bad_request, 'Validation failed; .*', - ): - self.controller.create(req, 1, body=body) - - valid_specs = { + def test_create_invalid_unknown_namespace(self): + """Test behavior of validator with specs from unknown namespace.""" + unknown_specs = { 'foo': 'bar', - 'hw:cpu_policyyyyyyy': 'shared', + 'foo:bar': 'baz', + 'hww:cpu_policy': 'sharrred', } - for key, value in valid_specs.items(): + for key, value in unknown_specs.items(): body = {'extra_specs': {key: value}} req = self._get_request( - '1/os-extra_specs?validation=permissive', - use_admin_context=True, version='2.82', + '1/os-extra_specs', use_admin_context=True, version='2.82', ) self.controller.create(req, 1, body=body) - def test_create_invalid_specs_disabled(self): - """Test behavior of permissive validator.""" - valid_specs = { - 'hw:numa_nodes': 'foo', - 'hw:cpu_policy': 'sharrred', - 'foo': 'bar', - 'hw:cpu_policyyyyyyy': 'shared', - } - for key, value in valid_specs.items(): - body = {'extra_specs': {key: value}} - req = self._get_request( - '1/os-extra_specs?validation=disabled', - use_admin_context=True, version='2.82', - ) - self.controller.create(req, 1, body=body) - - # TODO(stephenfin): Wire the microversion up - @unittest.expectedFailure - def test_create_invalid_validator_mode(self): - """Test behavior with an invalid validator mode.""" - body = {'extra_specs': {'hw:numa_nodes': '1'}} - req = self._get_request( - '1/os-extra_specs?validation=foo', - use_admin_context=True, version='2.82', - ) - with testtools.ExpectedException( - self.bad_request, - 'Invalid input for query parameters validation.*', - ): - self.controller.create(req, 1, body=body) - @mock.patch('nova.objects.flavor._flavor_extra_specs_add') def test_create_valid_specs(self, mock_flavor_extra_specs): valid_specs = { @@ -453,18 +405,18 @@ class FlavorsExtraSpecsTestV21(test.TestCase): # TODO(stephenfin): Wire the microversion up @unittest.expectedFailure - def test_update_invalid_specs_strict(self): - """Test behavior of strict validator.""" + def test_update_invalid_specs_known_namespace(self): + """Test behavior of validator with specs from known namespace.""" invalid_specs = { 'hw:numa_nodes': 'foo', 'hw:cpu_policy': 'sharrred', - 'foo': 'bar', 'hw:cpu_policyyyyyyy': 'shared', + 'hw:foo': 'bar', } for key, value in invalid_specs.items(): body = {key: value} req = self._get_request( - '1/os-extra_specs/{key}?validation=strict', + '1/os-extra_specs/{key}', use_admin_context=True, version='2.82', ) with testtools.ExpectedException( @@ -472,69 +424,21 @@ class FlavorsExtraSpecsTestV21(test.TestCase): ): self.controller.update(req, 1, key, body=body) - # TODO(stephenfin): Wire the microversion up - @unittest.expectedFailure - def test_update_invalid_specs_permissive(self): - """Test behavior of permissive validator.""" - invalid_specs = { - 'hw:numa_nodes': 'foo', - 'hw:cpu_policy': 'sharrred', - } - for key, value in invalid_specs.items(): - body = {key: value} - req = self._get_request( - f'1/os-extra_specs/{key}?validation=permissive', - use_admin_context=True, version='2.82', - ) - with testtools.ExpectedException( - self.bad_request, 'Validation failed; .*', - ): - self.controller.update(req, 1, key, body=body) - - valid_specs = { + def test_update_invalid_specs_unknown_namespace(self): + """Test behavior of validator with specs from unknown namespace.""" + unknown_specs = { 'foo': 'bar', - 'hw:cpu_policyyyyyyy': 'shared', + 'foo:bar': 'baz', + 'hww:cpu_policy': 'sharrred', } - for key, value in valid_specs.items(): + for key, value in unknown_specs.items(): body = {key: value} req = self._get_request( - f'1/os-extra_specs/{key}?validation=permissive', + f'1/os-extra_specs/{key}', use_admin_context=True, version='2.82', ) self.controller.update(req, 1, key, body=body) - def test_update_invalid_specs_disabled(self): - """Test behavior of permissive validator.""" - valid_specs = { - 'hw:numa_nodes': 'foo', - 'hw:cpu_policy': 'sharrred', - 'foo': 'bar', - 'hw:cpu_policyyyyyyy': 'shared', - } - for key, value in valid_specs.items(): - body = {key: value} - req = self._get_request( - f'1/os-extra_specs/{key}?validation=disabled', - use_admin_context=True, version='2.82', - ) - self.controller.update(req, 1, key, body=body) - - # TODO(stephenfin): Wire the microversion up - @unittest.expectedFailure - def test_update_invalid_validator_mode(self): - """Test behavior with an invalid validator mode.""" - key = 'hw:numa_nodes' - body = {'hw:numa_nodes': '1'} - req = self._get_request( - '1/os-extra_specs/{key}?validation=foo', - use_admin_context=True, version='2.82', - ) - with testtools.ExpectedException( - self.bad_request, - 'Invalid input for query parameters validation.*', - ): - self.controller.update(req, 1, key, body=body) - @mock.patch('nova.objects.flavor._flavor_extra_specs_add') def test_update_valid_specs(self, mock_flavor_extra_specs): valid_specs = { diff --git a/nova/tests/unit/api/validation/extra_specs/test_validators.py b/nova/tests/unit/api/validation/extra_specs/test_validators.py index 8aa51ff30955..2ec5a4304017 100644 --- a/nova/tests/unit/api/validation/extra_specs/test_validators.py +++ b/nova/tests/unit/api/validation/extra_specs/test_validators.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import ddt import testtools from nova.api.validation.extra_specs import validators @@ -20,28 +19,43 @@ from nova import exception from nova import test -@ddt.ddt class TestValidators(test.NoDBTestCase): - @ddt.data('strict', 'permissive', 'disabled') - def test_spec(self, policy): - invalid_specs = ( - ('hw:cpu_realtime_maskk', '^0'), + def test_namespaces(self): + """Ensure we see at least the in-tree namespaces. + + If we add new namespaces, they should be added to this list. + """ + namespaces = { + 'accel', 'aggregate_instance_extra_specs', 'capabilities', 'hw', + 'hw_rng', 'hw_video', 'os', 'pci_passthrough', 'powervm', 'quota', + 'resources', 'trait', 'vmware', + } + self.assertTrue( + namespaces.issubset(validators.NAMESPACES), + f'{namespaces} is not a subset of {validators.NAMESPACES}', + ) + + def test_spec(self): + unknown_namespaces = ( ('hhw:cpu_realtime_mask', '^0'), ('w:cpu_realtime_mask', '^0'), - ('hw:cpu_realtime_mas', '^0'), ('hw_cpu_realtime_mask', '^0'), ('foo', 'bar'), ) - for key, value in invalid_specs: - if policy == 'strict': - with testtools.ExpectedException(exception.ValidationError): - validators.validate(key, value, policy) - else: - validators.validate(key, value, policy) + for key, value in unknown_namespaces: + validators.validate(key, value) - @ddt.data('strict', 'permissive', 'disabled') - def test_value__str(self, policy): + known_invalid_namespaces = ( + ('hw:cpu_realtime_maskk', '^0'), + ('hw:cpu_realtime_mas', '^0'), + ('hw:foo', 'bar'), + ) + for key, value in known_invalid_namespaces: + with testtools.ExpectedException(exception.ValidationError): + validators.validate(key, value) + + def test_value__str(self): valid_specs = ( # patterns ('hw:cpu_realtime_mask', '^0'), @@ -55,7 +69,7 @@ class TestValidators(test.NoDBTestCase): ('hw:pci_numa_affinity_policy', 'legacy'), ) for key, value in valid_specs: - validators.validate(key, value, policy) + validators.validate(key, value) invalid_specs = ( # patterns @@ -70,14 +84,10 @@ class TestValidators(test.NoDBTestCase): ('hw:pci_numa_affinity_policy', 'lgacy'), ) for key, value in invalid_specs: - if policy in ('strict', 'permissive'): - with testtools.ExpectedException(exception.ValidationError): - validators.validate(key, value, policy) - else: - validators.validate(key, value, policy) + with testtools.ExpectedException(exception.ValidationError): + validators.validate(key, value) - @ddt.data('strict', 'permissive', 'disabled') - def test_value__int(self, policy): + def test_value__int(self): valid_specs = ( ('hw:numa_nodes', '1'), ('os:monitors', '1'), @@ -86,7 +96,7 @@ class TestValidators(test.NoDBTestCase): ('powervm:shared_weight', '255'), ) for key, value in valid_specs: - validators.validate(key, value, 'strict') + validators.validate(key, value) invalid_specs = ( ('hw:serial_port_count', 'five'), # NaN @@ -98,14 +108,10 @@ class TestValidators(test.NoDBTestCase): ('powervm:shared_weight', '256'), # has max ) for key, value in invalid_specs: - if policy in ('strict', 'permissive'): - with testtools.ExpectedException(exception.ValidationError): - validators.validate(key, value, policy) - else: - validators.validate(key, value, policy) + with testtools.ExpectedException(exception.ValidationError): + validators.validate(key, value) - @ddt.data('strict', 'permissive', 'disabled') - def test_value__bool(self, policy): + def test_value__bool(self): valid_specs = ( ('hw:cpu_realtime', '1'), ('hw:cpu_realtime', '0'), @@ -113,7 +119,7 @@ class TestValidators(test.NoDBTestCase): ('hw:boot_menu', 'y'), ) for key, value in valid_specs: - validators.validate(key, value, 'strict') + validators.validate(key, value) invalid_specs = ( ('hw:cpu_realtime', '2'), @@ -122,8 +128,5 @@ class TestValidators(test.NoDBTestCase): ('hw:boot_menu', 'yah'), ) for key, value in invalid_specs: - if policy in ('strict', 'permissive'): - with testtools.ExpectedException(exception.ValidationError): - validators.validate(key, value, policy) - else: - validators.validate(key, value, policy) + with testtools.ExpectedException(exception.ValidationError): + validators.validate(key, value)