From 8a95c29f554d7feb3a0a01b94fe1b7c9fd752fb8 Mon Sep 17 00:00:00 2001 From: Andrey Kurilin Date: Fri, 2 Feb 2018 15:15:27 +0200 Subject: [PATCH] [jsonschema] Require specifying `additionalProperties` explicitly Validation of objects via jsonschema become meaningless if there is no `additionalProperties: False`. In case of complex schemas, it is easy to miss this property in some child property. Let's require always specifing additionalProperties to simplify the life of reviewers and do not miss anything. Change-Id: I49693a7968e820010751909a48f06f3f784c5721 --- .../openstack/context/ceilometer/samples.py | 3 ++- .../openstack/context/dataplane/heat.py | 3 +++ .../plugins/openstack/context/ec2/servers.py | 6 +++-- .../context/manila/manila_share_networks.py | 5 ++-- .../openstack/context/monasca/metrics.py | 6 +++-- .../openstack/context/network/routers.py | 6 +++-- .../openstack/context/neutron/lbaas.py | 3 ++- .../plugins/openstack/context/nova/servers.py | 23 +++++++++++++------ .../context/sahara/sahara_cluster.py | 6 +++-- .../openstack/context/senlin/profiles.py | 1 + .../openstack/context/vm/custom_image.py | 6 +++-- .../context/watcher/audit_templates.py | 7 ++++-- tests/unit/doc/test_jsonschemas.py | 15 ++++++++---- 13 files changed, 63 insertions(+), 27 deletions(-) diff --git a/rally/plugins/openstack/context/ceilometer/samples.py b/rally/plugins/openstack/context/ceilometer/samples.py index 7c63b2c4..0c9f26af 100644 --- a/rally/plugins/openstack/context/ceilometer/samples.py +++ b/rally/plugins/openstack/context/ceilometer/samples.py @@ -79,7 +79,8 @@ class CeilometerSampleGenerator(context.Context): "created_at": { "type": "string" } - } + }, + "additionalProperties": False } }, "batch_size": { diff --git a/rally/plugins/openstack/context/dataplane/heat.py b/rally/plugins/openstack/context/dataplane/heat.py index a78828df..392f1bf3 100644 --- a/rally/plugins/openstack/context/dataplane/heat.py +++ b/rally/plugins/openstack/context/dataplane/heat.py @@ -73,12 +73,15 @@ class HeatDataplane(context.Context): }, "files": { "type": "object", + "additionalProperties": True }, "parameters": { "type": "object", + "additionalProperties": True }, "context_parameters": { "type": "object", + "additionalProperties": True }, }, "additionalProperties": False diff --git a/rally/plugins/openstack/context/ec2/servers.py b/rally/plugins/openstack/context/ec2/servers.py index 6b23b5a7..454862d7 100644 --- a/rally/plugins/openstack/context/ec2/servers.py +++ b/rally/plugins/openstack/context/ec2/servers.py @@ -39,7 +39,8 @@ class EC2ServerGenerator(context.Context): "name": { "type": "string" } - } + }, + "additionalProperties": False }, "flavor": { "type": "object", @@ -47,7 +48,8 @@ class EC2ServerGenerator(context.Context): "name": { "type": "string" } - } + }, + "additionalProperties": False }, "servers_per_tenant": { "type": "integer", diff --git a/rally/plugins/openstack/context/manila/manila_share_networks.py b/rally/plugins/openstack/context/manila/manila_share_networks.py index b5913ea0..dafa5350 100644 --- a/rally/plugins/openstack/context/manila/manila_share_networks.py +++ b/rally/plugins/openstack/context/manila/manila_share_networks.py @@ -70,12 +70,13 @@ class ShareNetworks(context.Context): "properties": { "use_share_networks": { "type": "boolean", - "description": "specifies whether manila should use share " + "description": "Specifies whether manila should use share " "networks for share creation or not."}, "share_networks": { "type": "object", - "description": SHARE_NETWORKS_ARG_DESCR + "description": SHARE_NETWORKS_ARG_DESCR, + "additionalProperties": True }, }, "additionalProperties": False diff --git a/rally/plugins/openstack/context/monasca/metrics.py b/rally/plugins/openstack/context/monasca/metrics.py index 6fc8d22f..ec706124 100644 --- a/rally/plugins/openstack/context/monasca/metrics.py +++ b/rally/plugins/openstack/context/monasca/metrics.py @@ -48,7 +48,8 @@ class MonascaMetricGenerator(context.Context): "url": { "type": "string" } - } + }, + "additionalProperties": False }, "metrics_per_tenant": { "type": "integer", @@ -65,7 +66,8 @@ class MonascaMetricGenerator(context.Context): "value_meta_value": { "type": "string" } - } + }, + "additionalProperties": False } } }, diff --git a/rally/plugins/openstack/context/network/routers.py b/rally/plugins/openstack/context/network/routers.py index c2d3e8f9..d5ff6d70 100644 --- a/rally/plugins/openstack/context/network/routers.py +++ b/rally/plugins/openstack/context/network/routers.py @@ -48,7 +48,8 @@ class Router(context.Context): "properties": { "network_id": {"type": "string"}, "enable_snat": {"type": "boolean"} - } + }, + "additionalProperties": False }, "network_id": { "description": "Network ID", @@ -62,7 +63,8 @@ class Router(context.Context): "properties": { "ip_address": {"type": "string"}, "subnet_id": {"type": "string"} - } + }, + "additionalProperties": False, } }, "distributed": { diff --git a/rally/plugins/openstack/context/neutron/lbaas.py b/rally/plugins/openstack/context/neutron/lbaas.py index 31f9cf68..8ba5efc1 100644 --- a/rally/plugins/openstack/context/neutron/lbaas.py +++ b/rally/plugins/openstack/context/neutron/lbaas.py @@ -32,7 +32,8 @@ class Lbaas(context.Context): "$schema": consts.JSON_SCHEMA, "properties": { "pool": { - "type": "object" + "type": "object", + "additionalProperties": True }, "lbaas_version": { "type": "integer", diff --git a/rally/plugins/openstack/context/nova/servers.py b/rally/plugins/openstack/context/nova/servers.py index 590a8d0b..975ad1c4 100755 --- a/rally/plugins/openstack/context/nova/servers.py +++ b/rally/plugins/openstack/context/nova/servers.py @@ -38,14 +38,16 @@ class ServerGenerator(context.Context): "type": "object", "properties": { "name": {"type": "string"} - } + }, + "additionalProperties": False }, "flavor": { "description": "Name of flavor to boot server(s) with.", "type": "object", "properties": { "name": {"type": "string"} - } + }, + "additionalProperties": False }, "servers_per_tenant": { "description": "Number of servers to boot in each Tenant.", @@ -60,11 +62,18 @@ class ServerGenerator(context.Context): "type": "array", "description": "List of networks to attach to server.", "items": {"oneOf": [ - {"type": "object", - "properties": {"net-id": {"type": "string"}}, - "description": "Network ID in a format like OpenStack API" - " expects to see."}, - {"type": "string", "description": "Network ID."}]}, + { + "type": "object", + "properties": {"net-id": {"type": "string"}}, + "description": "Network ID in a format like OpenStack " + "API expects to see.", + "additionalProperties": False + }, + { + "type": "string", + "description": "Network ID." + } + ]}, "minItems": 1 } }, diff --git a/rally/plugins/openstack/context/sahara/sahara_cluster.py b/rally/plugins/openstack/context/sahara/sahara_cluster.py index 3295482a..f75ec3e0 100644 --- a/rally/plugins/openstack/context/sahara/sahara_cluster.py +++ b/rally/plugins/openstack/context/sahara/sahara_cluster.py @@ -77,10 +77,12 @@ class SaharaCluster(context.Context): } }, "node_configs": { - "type": "object" + "type": "object", + "additionalProperties": True }, "cluster_configs": { - "type": "object" + "type": "object", + "additionalProperties": True }, "enable_anti_affinity": { "type": "boolean" diff --git a/rally/plugins/openstack/context/senlin/profiles.py b/rally/plugins/openstack/context/senlin/profiles.py index 978fcdc3..c27c1802 100644 --- a/rally/plugins/openstack/context/senlin/profiles.py +++ b/rally/plugins/openstack/context/senlin/profiles.py @@ -34,6 +34,7 @@ class ProfilesGenerator(context.Context): }, "properties": { "type": "object", + "additionalProperties": True, } }, "additionalProperties": False, diff --git a/rally/plugins/openstack/context/vm/custom_image.py b/rally/plugins/openstack/context/vm/custom_image.py index 2eb06660..e36d39b8 100644 --- a/rally/plugins/openstack/context/vm/custom_image.py +++ b/rally/plugins/openstack/context/vm/custom_image.py @@ -55,7 +55,8 @@ class BaseCustomImageGenerator(context.Context): "name": { "type": "string" } - } + }, + "additionalProperties": False }, "flavor": { "type": "object", @@ -63,7 +64,8 @@ class BaseCustomImageGenerator(context.Context): "name": { "type": "string" } - } + }, + "additionalProperties": False }, "username": { "type": "string" diff --git a/rally/plugins/openstack/context/watcher/audit_templates.py b/rally/plugins/openstack/context/watcher/audit_templates.py index e53ee524..c3a7700a 100644 --- a/rally/plugins/openstack/context/watcher/audit_templates.py +++ b/rally/plugins/openstack/context/watcher/audit_templates.py @@ -49,7 +49,8 @@ class AuditTemplateGenerator(context.Context): "name": { "type": "string" } - } + }, + "additionalProperties": False }, "strategy": { "type": "object", @@ -57,9 +58,11 @@ class AuditTemplateGenerator(context.Context): "name": { "type": "string" } - } + }, + "additionalProperties": False }, }, + "additionalProperties": False, }, } }, diff --git a/tests/unit/doc/test_jsonschemas.py b/tests/unit/doc/test_jsonschemas.py index f8b6c82c..485cca6b 100644 --- a/tests/unit/doc/test_jsonschemas.py +++ b/tests/unit/doc/test_jsonschemas.py @@ -13,6 +13,7 @@ # under the License. import copy +import json from rally.common.plugin import plugin from rally import plugins @@ -35,9 +36,10 @@ class ConfigSchemasTestCase(test.TestCase): def fail(self, p, schema, msg): super(ConfigSchemasTestCase, self).fail( "Config schema of plugin '%s' (%s) is invalid. %s " - "(Schema: %s)" % (p.get_name(), + "Schema: \n%s" % (p.get_name(), "%s.%s" % (p.__module__, p.__name__), - msg, schema)) + msg, + json.dumps(schema, indent=3))) def _check_anyOf_or_oneOf(self, p, schema, definitions): if "anyOf" in schema or "oneOf" in schema: @@ -61,6 +63,11 @@ class ConfigSchemasTestCase(test.TestCase): if unexpected_keys: self.fail(p, schema, ("Found unexpected key(s) for object type: " "%s." % ", ".join(unexpected_keys))) + if "additionalProperties" not in schema: + self.fail(p, schema, + "'additionalProperties' is required field for objects. " + "Specify `'additionalProperties': True` explicitly to " + "accept not validated properties.") if "patternProperties" in schema: if "properties" in schema: @@ -147,7 +154,7 @@ class ConfigSchemasTestCase(test.TestCase): pass elif schema == {}: # NOTE(andreykurilin): an empty dict means that the user can - # transmit whatever he want in whatever he want format. It is + # transmit whatever he wants in whatever he wants format. It is # not the case which we want to support. self.fail(p, schema, "Empty schema is not allowed.") elif "$ref" in schema: @@ -161,7 +168,7 @@ class ConfigSchemasTestCase(test.TestCase): @plugins.ensure_plugins_are_loaded def test_schema_is_valid(self): for p in plugin.Plugin.get_all(): - if not hasattr(p, "CONFIG_SCHEMA"): + if not hasattr(p, "CONFIG_SCHEMA") or "tests.unit" in p.__module__: continue # allow only top level definitions definitions = p.CONFIG_SCHEMA.get("definitions", {})