Follow-up for flavor-extra-spec-validators series

Clarify the impact of this microversion in the release note and improve
some tests.

Part of blueprint flavor-extra-spec-validators

Change-Id: Idfccfa9831f1df6f261a247489154492a231d832
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

nova/api/openstack/compute/rest_api_version_history.rst

@@ -1130,7 +1130,7 @@ for the following APIs:
 * ``POST /flavors/{flavor_id}/os-extra_specs``
 * ``PUT /flavors/{flavor_id}/os-extra_specs/{id}``
 
-Validation is only used for recognized extra spec namespaces, namely:
+Validation is only used for recognized extra spec namespaces, currently:
 ``accel``, ``aggregate_instance_extra_specs``, ``capabilities``, ``hw``,
 ``hw_rng``, ``hw_video``, ``os``, ``pci_passthrough``, ``powervm``, ``quota``,
 ``resources``, ``trait``, and ``vmware``.

nova/api/validation/extra_specs/traits.py

@@ -38,10 +38,6 @@ for trait in os_traits.get_traits():
                     'name': 'group',
                     'pattern': r'(_[a-zA-z0-9_]*|\d+)?',
                 },
-                {
-                    'name': 'trait',
-                    'pattern': r'[a-zA-Z0-9_]+',
-                },
             ],
         )
     )

nova/api/validation/extra_specs/validators.py

@@ -47,8 +47,15 @@ def validate(name: str, value: str):
             validator.validate(name, value)
             return
 
-    namespace = name.split(':', 1)[0].split('{')[0] if ':' in name else None
+    # check if there's a namespace; if not, we've done all we can do
-    if not namespace or namespace not in NAMESPACES:  # unregistered namespace
+    if ':' not in name:  # no namespace
+        return
+
+    # if there is, check if it's one we recognize
+    for namespace in NAMESPACES:
+        if re.fullmatch(namespace, name.split(':', 1)[0]):
+            break
+    else:
         return
 
     raise exception.ValidationError(
@@ -72,8 +79,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:
+            if ':' in validator.name_regex:
-                NAMESPACES.add(validator.name.split(':', 1)[0].split('{')[0])
+                NAMESPACES.add(validator.name_regex.split(':', 1)[0])
 
 
 load_validators()

nova/tests/functional/test_flavor_extraspecs.py

@@ -14,8 +14,6 @@
 
 """Tests for os-extra_specs API."""
 
-import testtools
-
 from nova.tests.functional.api import client as api_client
 from nova.tests.functional import integrated_helpers
 
@@ -43,7 +41,7 @@ class FlavorExtraSpecsTest(integrated_helpers._IntegratedTestBase):
         This should pass because validation is not enabled in this API
         microversion.
         """
-        body = {'extra_specs': {'hw:numa_nodes': '1', 'foo': 'bar'}}
+        body = {'extra_specs': {'hw:numa_nodes': 'foo', 'foo': 'bar'}}
         self.admin_api.post_extra_spec(self.flavor_id, body)
         self.assertEqual(
             body['extra_specs'], self.admin_api.get_extra_specs(self.flavor_id)
@@ -64,8 +62,8 @@ class FlavorExtraSpecsTest(integrated_helpers._IntegratedTestBase):
         This should pass because validation is not enabled in this API
         microversion.
         """
-        spec_id = 'foo:bar'
+        spec_id = 'hw:foo'
-        body = {'foo:bar': 'baz'}
+        body = {'hw:foo': 'bar'}
         self.admin_api.put_extra_spec(self.flavor_id, spec_id, body)
         self.assertEqual(
             body, self.admin_api.get_extra_spec(self.flavor_id, spec_id)
@@ -82,11 +80,12 @@ class FlavorExtraSpecsV286Test(FlavorExtraSpecsTest):
 
         # this should fail because 'foo' is not a suitable value for
         # 'hw:numa_nodes'
-        with testtools.ExpectedException(
+        exc = self.assertRaises(
-            api_client.OpenStackApiException
+            api_client.OpenStackApiException,
-        ) as exc:
+            self.admin_api.post_extra_spec,
-            self.admin_api.post_extra_spec(self.flavor_id, body)
+            self.flavor_id, body,
-            self.assertEqual(400, exc.response.status_code)
+        )
+        self.assertEqual(400, exc.response.status_code)
 
         # ...and the extra specs should not be saved
         self.assertEqual({}, self.admin_api.get_extra_specs(self.flavor_id))
@@ -102,11 +101,12 @@ class FlavorExtraSpecsV286Test(FlavorExtraSpecsTest):
         body = {'extra_specs': {'hw:numa_nodes': '1', 'hw:foo': 'bar'}}
 
         # ...but this should fail because we do recognize the namespace
-        with testtools.ExpectedException(
+        exc = self.assertRaises(
-            api_client.OpenStackApiException
+            api_client.OpenStackApiException,
-        ) as exc:
+            self.admin_api.post_extra_spec,
-            self.admin_api.post_extra_spec(self.flavor_id, body)
+            self.flavor_id, body,
-            self.assertEqual(400, exc.response.status_code)
+        )
+        self.assertEqual(400, exc.response.status_code)
 
     def test_update_invalid_spec(self):
         """Test updating extra specs with invalid specs."""
@@ -114,21 +114,23 @@ class FlavorExtraSpecsV286Test(FlavorExtraSpecsTest):
         body = {'hw:foo': 'bar'}
 
         # this should fail because we don't recognize the extra spec
-        with testtools.ExpectedException(
+        exc = self.assertRaises(
-            api_client.OpenStackApiException
+            api_client.OpenStackApiException,
-        ) as exc:
+            self.admin_api.put_extra_spec,
-            self.admin_api.put_extra_spec(self.flavor_id, spec_id, body)
+            self.flavor_id, spec_id, body,
-            self.assertEqual(400, exc.response.status_code)
+        )
+        self.assertEqual(400, exc.response.status_code)
 
         spec_id = 'hw:numa_nodes'
         body = {'hw:numa_nodes': 'foo'}
 
         # ...while this should fail because the value is not valid
-        with testtools.ExpectedException(
+        exc = self.assertRaises(
-            api_client.OpenStackApiException
+            api_client.OpenStackApiException,
-        ) as exc:
+            self.admin_api.put_extra_spec,
-            self.admin_api.put_extra_spec(self.flavor_id, spec_id, body)
+            self.flavor_id, spec_id, body,
-            self.assertEqual(400, exc.response.status_code)
+        )
+        self.assertEqual(400, exc.response.status_code)
 
         # ...and neither extra spec should be saved
         self.assertEqual({}, self.admin_api.get_extra_specs(self.flavor_id))
@@ -141,3 +143,4 @@ class FlavorExtraSpecsV286Test(FlavorExtraSpecsTest):
         # this should pass because we don't recognize the extra spec but it's
         # not in a namespace we care about
         self.admin_api.put_extra_spec(self.flavor_id, spec_id, body)
+        self.assertEqual(body, self.admin_api.get_extra_specs(self.flavor_id))

nova/tests/unit/api/openstack/compute/test_flavors_extra_specs.py

@@ -272,6 +272,10 @@ class FlavorsExtraSpecsTestV21(test.TestCase):
             'hw:cpu_policy': 'sharrred',
             'hw:cpu_policyyyyyyy': 'shared',
             'hw:foo': 'bar',
+            'trait:STORAGE_DISK_SSD': 'forbiden',
+            'trait_foo:HW_CPU_X86_AVX2': 'foo',
+            'trait:bar': 'required',
+            'trait_foo:bar': 'required',
         }
         for key, value in invalid_specs.items():
             body = {'extra_specs': {key: value}}
@@ -303,6 +307,8 @@ class FlavorsExtraSpecsTestV21(test.TestCase):
             'hide_hypervisor_id': 'true',
             'hw:numa_nodes': '1',
             'hw:numa_cpus.0': '0-3,8-9,11,10',
+            'trait:STORAGE_DISK_SSD': 'forbidden',
+            'trait_foo:HW_CPU_X86_AVX2': 'required',
         }
         mock_flavor_extra_specs.side_effect = return_create_flavor_extra_specs
 

nova/tests/unit/api/validation/extra_specs/test_validators.py

@@ -29,7 +29,8 @@ class TestValidators(test.NoDBTestCase):
         namespaces = {
             'accel', 'aggregate_instance_extra_specs', 'capabilities', 'hw',
             'hw_rng', 'hw_video', 'os', 'pci_passthrough', 'powervm', 'quota',
-            'resources', 'trait', 'vmware',
+            'resources(?P<group>(_[a-zA-z0-9_]*|\\d+)?)',
+            'trait(?P<group>(_[a-zA-z0-9_]*|\\d+)?)', 'vmware',
         }
         self.assertTrue(
             namespaces.issubset(validators.NAMESPACES),

releasenotes/notes/flavor-extra-spec-validators-76d1f2e52ba753db.yaml

@@ -3,4 +3,19 @@ features:
   - |
     The 2.86 microversion adds support for flavor extra spec validation when
     creating or updating flavor extra specs. Use of an unrecognized or invalid
-    flavor extra spec will result in a HTTP 400 response.
+    flavor extra spec in the following namespaces will result in a HTTP 400
+    response.
+
+    - ``accel``
+    - ``aggregate_instance_extra_specs``
+    - ``capabilities``
+    - ``hw``
+    - ``hw_rng``
+    - ``hw_video``
+    - ``os``
+    - ``pci_passthrough``
+    - ``powervm``
+    - ``quota``
+    - ``resources`` (including ``_{group}`` suffixes)
+    - ``trait`` (including ``_{group}`` suffixes)
+    - ``vmware``