From 2d7c0f1410fd6fe91c4af4152375b1dcd6d34511 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Wed, 27 Jan 2021 18:39:15 -0800 Subject: [PATCH 01/10] catching non-ASCII characters --- src/rpdk/core/data_loaders.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 8f10e794..e6480145 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -1,6 +1,7 @@ import json import logging import os +import re import shutil from io import TextIOWrapper from pathlib import Path @@ -141,6 +142,14 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 LOG.debug("Resource spec validation failed", exc_info=True) raise SpecValidationError(str(e)) from e + non_ascii_chars = re.findall( + r"[^ -~]", json.dumps(resource_spec, ensure_ascii=False) + ) + if non_ascii_chars: + LOG.warning( + "non-ASCII characters found in resource schema: %s", non_ascii_chars + ) + list_options = { "maxresults", "maxrecords", From 27f97acc1dfbe72d97cbf1f32b388b334f0cde32 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Wed, 27 Jan 2021 18:43:10 -0800 Subject: [PATCH 02/10] test resource schemas with common issue warnings --- .../data/schema/valid/invalid_non_ascii_chars.json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/data/schema/valid/invalid_non_ascii_chars.json diff --git a/tests/data/schema/valid/invalid_non_ascii_chars.json b/tests/data/schema/valid/invalid_non_ascii_chars.json new file mode 100644 index 00000000..109018aa --- /dev/null +++ b/tests/data/schema/valid/invalid_non_ascii_chars.json @@ -0,0 +1,14 @@ +{ + "typeName" : "AWS::Service::Type", + "description" : "🤠", + "additionalProperties" : false, + "properties" : { + "Property" : { + "type" : "string" + } + }, + "readOnlyProperties": [ + "/properties/Property" + ], + "primaryIdentifier" : [ "/properties/Property" ] +} From 8fb40f28fa4e7e82d5fd0e17a412092fd7304bd2 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Wed, 27 Jan 2021 20:13:22 -0800 Subject: [PATCH 03/10] patterns/enums --- setup.py | 1 + src/rpdk/core/data_loaders.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/setup.py b/setup.py index 14f94c51..0ac7e1c5 100644 --- a/setup.py +++ b/setup.py @@ -52,6 +52,7 @@ def find_version(*file_paths): "docker>=4.3.1", "ordered-set>=4.0.2", "cfn-lint>=0.43.0", + "nested-lookup" ], entry_points={ "console_scripts": ["cfn-cli = rpdk.core.cli:main", "cfn = rpdk.core.cli:main"] diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index e6480145..f5cfea54 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -10,6 +10,7 @@ import yaml from jsonschema import Draft7Validator, RefResolver from jsonschema.exceptions import RefResolutionError, ValidationError +from nested_lookup import nested_lookup from .exceptions import InternalError, SpecValidationError from .jsonutils.inliner import RefInliner @@ -142,6 +143,25 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 LOG.debug("Resource spec validation failed", exc_info=True) raise SpecValidationError(str(e)) from e + for pattern in nested_lookup("pattern", resource_spec): + if "arn:aws:" in pattern: + LOG.warning( + "Don't hardcode the aws partition in ARN patterns: %s", + pattern, + ) + try: + re.compile(pattern) + except re.error: + LOG.warning("Could not validate regular expression: %s", pattern) + + for enum in nested_lookup("enum", resource_spec): + if len(enum) > 15: + LOG.warning( + "Consider not manually maintaining large constantly evolving enums like ", + "instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly: %s", + enum, + ) + non_ascii_chars = re.findall( r"[^ -~]", json.dumps(resource_spec, ensure_ascii=False) ) From b9deeb599c5b9672aae6c91eaf5747b10c378abc Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Wed, 27 Jan 2021 20:44:03 -0800 Subject: [PATCH 04/10] test resource schemas with common issue warnings --- src/rpdk/core/data_loaders.py | 4 ++-- .../valid/invalid_property_constraints.json | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 tests/data/schema/valid/invalid_property_constraints.json diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index f5cfea54..52819382 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -157,8 +157,8 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 for enum in nested_lookup("enum", resource_spec): if len(enum) > 15: LOG.warning( - "Consider not manually maintaining large constantly evolving enums like ", - "instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly: %s", + "Consider not manually maintaining large constantly evolving enums like" + + "instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly: %s", enum, ) diff --git a/tests/data/schema/valid/invalid_property_constraints.json b/tests/data/schema/valid/invalid_property_constraints.json new file mode 100644 index 00000000..786e1681 --- /dev/null +++ b/tests/data/schema/valid/invalid_property_constraints.json @@ -0,0 +1,17 @@ +{ + "typeName" : "AWS::Service::Type", + "description" : "", + "additionalProperties" : false, + "properties" : { + "Property" : { + "type" : "string", + "pattern" : "arn:aws:[A-Za-z0-9]+{1,255}", + "enum" : [ "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z" ], + "minimum" : 0 + } + }, + "readOnlyProperties": [ + "/properties/Property" + ], + "primaryIdentifier" : [ "/properties/Property" ] +} From cee7bb7c1601850ce3480cb5919b2deca9afdbad Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Thu, 28 Jan 2021 15:10:39 -0800 Subject: [PATCH 05/10] lowercase properties and incorrect min/max constraints --- src/rpdk/core/data_loaders.py | 65 +++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 52819382..42676060 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -143,6 +143,71 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 LOG.debug("Resource spec validation failed", exc_info=True) raise SpecValidationError(str(e)) from e + min_max_keywords = { + "minimum", + "maximum", + "minLength", + "maxLength", + "minProperties", + "maxProperties", + "minItems", + "maxItems", + "exclusiveMinimum", + "exclusiveMaximum", + } + for property in resource_spec.get("properties", []): + if property[0].islower(): + LOG.warning( + "CloudFormation properties don't usually start with lowercase letters: %s", + property, + ) + try: + property_type = resource_spec.get("properties", [])[property]["type"] + property_keywords = resource_spec.get("properties", [])[property].keys() + for types, allowed_keywords in [ + ( + {"integer", "number"}, + { + "minimum", + "maximum", + "exclusiveMinimum", + "exclusiveMaximum", + }, + ), + ( + {"string"}, + { + "minLength", + "maxLength", + }, + ), + ( + {"object"}, + { + "minProperties", + "maxProperties", + }, + ), + ( + {"array"}, + { + "minItems", + "maxItems", + }, + ), + ]: + if ( + property_type in types + and min_max_keywords - allowed_keywords & property_keywords + ): + LOG.warning( + "Incorrect min/max JSON schema keywords for type: %s for property: %s", + property_type, + property, + ) + except: + pass + for pattern in nested_lookup("pattern", resource_spec): if "arn:aws:" in pattern: LOG.warning( From 7bf53832d02c8cb7aa2466e31eb90d9f0fc2a937 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Thu, 28 Jan 2021 15:11:11 -0800 Subject: [PATCH 06/10] test resource schemas with common issue warnings --- .../schema/valid/invalid_lowercase_property.json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/data/schema/valid/invalid_lowercase_property.json diff --git a/tests/data/schema/valid/invalid_lowercase_property.json b/tests/data/schema/valid/invalid_lowercase_property.json new file mode 100644 index 00000000..e2b018a4 --- /dev/null +++ b/tests/data/schema/valid/invalid_lowercase_property.json @@ -0,0 +1,14 @@ +{ + "typeName" : "AWS::Service::Type", + "description" : "", + "additionalProperties" : false, + "properties" : { + "property" : { + "type" : "string" + } + }, + "readOnlyProperties": [ + "/properties/property" + ], + "primaryIdentifier" : [ "/properties/property" ] +} From 425788eb72028d483a5b60435dbf886277234370 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Thu, 28 Jan 2021 17:35:13 -0800 Subject: [PATCH 07/10] pre-commit --- setup.cfg | 2 +- setup.py | 2 +- src/rpdk/core/data_loaders.py | 20 ++++++++++---------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/setup.cfg b/setup.cfg index a33bff55..f3de31c4 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,7 +29,7 @@ include_trailing_comma = true combine_as_imports = True force_grid_wrap = 0 known_first_party = rpdk -known_third_party = boto3,botocore,cfnlint,colorama,docker,hypothesis,jinja2,jsonschema,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml +known_third_party = boto3,botocore,cfnlint,colorama,docker,hypothesis,jinja2,jsonschema,nested_lookup,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml [tool:pytest] # can't do anything about 3rd part modules, so don't spam us diff --git a/setup.py b/setup.py index 0ac7e1c5..aad7ddd0 100644 --- a/setup.py +++ b/setup.py @@ -52,7 +52,7 @@ def find_version(*file_paths): "docker>=4.3.1", "ordered-set>=4.0.2", "cfn-lint>=0.43.0", - "nested-lookup" + "nested-lookup", ], entry_points={ "console_scripts": ["cfn-cli = rpdk.core.cli:main", "cfn = rpdk.core.cli:main"] diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 42676060..1e721361 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -125,7 +125,7 @@ def get_file_base_uri(file): return path.resolve().as_uri() -def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C901 +def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 """Load a resource provider definition from a file, and validate it.""" try: resource_spec = json.load(resource_spec_file) @@ -155,15 +155,15 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 "exclusiveMinimum", "exclusiveMaximum", } - for property in resource_spec.get("properties", []): - if property[0].islower(): + for prop in resource_spec.get("properties", []): + if prop[0].islower(): LOG.warning( "CloudFormation properties don't usually start with lowercase letters: %s", - property, + prop, ) try: - property_type = resource_spec.get("properties", [])[property]["type"] - property_keywords = resource_spec.get("properties", [])[property].keys() + property_type = resource_spec.get("properties", [])[prop]["type"] + property_keywords = resource_spec.get("properties", [])[prop].keys() for types, allowed_keywords in [ ( {"integer", "number"}, @@ -203,9 +203,9 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 LOG.warning( "Incorrect min/max JSON schema keywords for type: %s for property: %s", property_type, - property, + prop, ) - except: + except (KeyError, TypeError): pass for pattern in nested_lookup("pattern", resource_spec): @@ -222,8 +222,8 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 for enum in nested_lookup("enum", resource_spec): if len(enum) > 15: LOG.warning( - "Consider not manually maintaining large constantly evolving enums like" - + "instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly: %s", + "Consider not manually maintaining large constantly evolving enums like \ +instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly: %s", enum, ) From 4ca05288086758bf961237ea6eb9058a40ba2060 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Tue, 2 Feb 2021 21:58:12 -0800 Subject: [PATCH 08/10] iterating properties instead of just property names --- src/rpdk/core/data_loaders.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 1e721361..6a2585aa 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -155,15 +155,15 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 "exclusiveMinimum", "exclusiveMaximum", } - for prop in resource_spec.get("properties", []): - if prop[0].islower(): + for property_name, property_details in resource_spec.get("properties", {}).items(): + if property_name[0].islower(): LOG.warning( "CloudFormation properties don't usually start with lowercase letters: %s", - prop, + property_name, ) try: - property_type = resource_spec.get("properties", [])[prop]["type"] - property_keywords = resource_spec.get("properties", [])[prop].keys() + property_type = property_details["type"] + property_keywords = property_details.keys() for types, allowed_keywords in [ ( {"integer", "number"}, @@ -203,7 +203,7 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 LOG.warning( "Incorrect min/max JSON schema keywords for type: %s for property: %s", property_type, - prop, + property_name, ) except (KeyError, TypeError): pass From 99e7deca5622e230de09f97e78dc84f67587b407 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Tue, 2 Feb 2021 22:40:39 -0800 Subject: [PATCH 09/10] checking definitions in addition to properties --- src/rpdk/core/data_loaders.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 6a2585aa..6481e740 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -155,7 +155,8 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 "exclusiveMinimum", "exclusiveMaximum", } - for property_name, property_details in resource_spec.get("properties", {}).items(): + + def check_property(property_name, property_details): if property_name[0].islower(): LOG.warning( "CloudFormation properties don't usually start with lowercase letters: %s", @@ -208,6 +209,18 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 except (KeyError, TypeError): pass + for property_name, property_details in resource_spec.get("properties", {}).items(): + check_property(property_name, property_details) + for property_name, property_details in resource_spec.get("definitions", {}).items(): + check_property(property_name, property_details) + for definition in resource_spec.get("definitions", []): + for property_name, property_details in ( + resource_spec.get("definitions", [])[definition] + .get("properties", {}) + .items() + ): + check_property(property_name, property_details) + for pattern in nested_lookup("pattern", resource_spec): if "arn:aws:" in pattern: LOG.warning( From df38768e94748e9fb98d5d35618ad217aa5126e7 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Thu, 4 Feb 2021 14:54:17 -0800 Subject: [PATCH 10/10] using JsonSchemaFlattener --- src/rpdk/core/data_loaders.py | 117 ++++++++++++++++------------------ 1 file changed, 55 insertions(+), 62 deletions(-) diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 6481e740..12f8eff5 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -13,7 +13,9 @@ from nested_lookup import nested_lookup from .exceptions import InternalError, SpecValidationError +from .jsonutils.flattener import JsonSchemaFlattener from .jsonutils.inliner import RefInliner +from .jsonutils.utils import FlatteningError LOG = logging.getLogger(__name__) @@ -155,71 +157,62 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 "exclusiveMinimum", "exclusiveMaximum", } - - def check_property(property_name, property_details): - if property_name[0].islower(): - LOG.warning( - "CloudFormation properties don't usually start with lowercase letters: %s", - property_name, - ) - try: - property_type = property_details["type"] - property_keywords = property_details.keys() - for types, allowed_keywords in [ - ( - {"integer", "number"}, - { - "minimum", - "maximum", - "exclusiveMinimum", - "exclusiveMaximum", - }, - ), - ( - {"string"}, - { - "minLength", - "maxLength", - }, - ), - ( - {"object"}, - { - "minProperties", - "maxProperties", - }, - ), - ( - {"array"}, - { - "minItems", - "maxItems", - }, - ), - ]: - if ( - property_type in types - and min_max_keywords - allowed_keywords & property_keywords - ): + try: # pylint: disable=R + for _key, schema in JsonSchemaFlattener(resource_spec).flatten_schema().items(): + for property_name, property_details in schema.get("properties", {}).items(): + if property_name[0].islower(): LOG.warning( - "Incorrect min/max JSON schema keywords for type: %s for property: %s", - property_type, + "CloudFormation properties don't usually start with lowercase letters: %s", property_name, ) - except (KeyError, TypeError): - pass - - for property_name, property_details in resource_spec.get("properties", {}).items(): - check_property(property_name, property_details) - for property_name, property_details in resource_spec.get("definitions", {}).items(): - check_property(property_name, property_details) - for definition in resource_spec.get("definitions", []): - for property_name, property_details in ( - resource_spec.get("definitions", [])[definition] - .get("properties", {}) - .items() - ): - check_property(property_name, property_details) + try: + property_type = property_details["type"] + property_keywords = property_details.keys() + for types, allowed_keywords in [ + ( + {"integer", "number"}, + { + "minimum", + "maximum", + "exclusiveMinimum", + "exclusiveMaximum", + }, + ), + ( + {"string"}, + { + "minLength", + "maxLength", + }, + ), + ( + {"object"}, + { + "minProperties", + "maxProperties", + }, + ), + ( + {"array"}, + { + "minItems", + "maxItems", + }, + ), + ]: + if ( + property_type in types + and min_max_keywords - allowed_keywords & property_keywords + ): + LOG.warning( + "Incorrect min/max JSON schema keywords for type: %s for property: %s", + property_type, + property_name, + ) + except (KeyError, TypeError): + pass + except FlatteningError: + pass for pattern in nested_lookup("pattern", resource_spec): if "arn:aws:" in pattern: