Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

catch common resource schema issues in cfn validate #663

Merged
merged 4 commits into from
Jan 20, 2021
Merged

catch common resource schema issues in cfn validate #663

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7eead2
catch common resource schema issues
PatMyron Jan 8, 2021
32c9c0a
test resource schemas with common issue warnings
PatMyron Jan 14, 2021
893bd03
no wildcards in handler permissions
PatMyron Jan 17, 2021
921b60b
adding context for LIST API options warnings
PatMyron Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ good-names=e,ex,f,fp,i,j,k,n,_
[FORMAT]

indent-string=' '
max-line-length=88
max-line-length=160
2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ exclude =
.eggs,
.tox
max-complexity = 10
max-line-length = 80
max-line-length = 160
select = C,E,F,W,B,B950
# C812, C815, W503 clash with black
ignore = E501,C812,C816,C815,W503
Expand Down
50 changes: 36 additions & 14 deletions src/rpdk/core/data_loaders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,7 @@ def get_file_base_uri(file):
return path.resolve().as_uri()


def _is_in(schema, key):
def contains(value):
return value in schema.get(key, [])

return contains


def load_resource_spec(resource_spec_file): # noqa: C901
def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C901
"""Load a resource provider definition from a file, and validate it."""
try:
resource_spec = json.load(resource_spec_file)
Expand All @@ -148,6 +141,37 @@ def load_resource_spec(resource_spec_file): # noqa: C901
LOG.debug("Resource spec validation failed", exc_info=True)
raise SpecValidationError(str(e)) from e

if {
"maxresults",
"maxrecords",
"maxitems",
"nexttoken",
"nextmarker",
"nextpagetoken",
"pagetoken",
"paginationtoken",
} & set(map(str.lower, resource_spec.get("properties", []))):
LOG.warning(
"LIST API inputs like MaxResults, MaxRecords, MaxItems, NextToken, NextMarker, NextPageToken, PageToken, and Filters are not resource properties"
PatMyron marked this conversation as resolved.
Show resolved Hide resolved
)

if set(resource_spec.get("readOnlyProperties", [])) & set(
resource_spec.get("createOnlyProperties", [])
) or set(resource_spec.get("readOnlyProperties", [])) & set(
resource_spec.get("writeOnlyProperties", [])
):
LOG.warning(
"readOnlyProperties cannot be specified by customers and should not overlap with writeOnlyProperties or createOnlyProperties"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for users to successfully submit their resources when these warning are logged? If yes, should we throw an exception and stop them from submitting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for users to successfully submit their resources when these warning are logged?

As it's currently written, yes

If yes, should we throw an exception and stop them from submitting?

I've also been debating this. We should have enforced some of these from the start, but it's trickier now to suddenly introduce backwards incompatible breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do it, maybe with a major version bump and a release announcement that the changes are backward incompatible. Users can stick with a specific version until they are ready to upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is something we can introduce in the future, but i think these warnings are still valuable for customers to see before upgrading. usually with some backwards incompatible change there's a period of warning about an upcoming change in behavior, then a flip of some kind. we can use this as an opportunity to introduce the warnings then remove the functionality at a later date

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree NOT to introduce backward incompatible change as because there are some resources have it's special cases which it will break them.

Warning is good at this point to alert.


for handler in resource_spec.get("handlers", []):
for permission in resource_spec.get("handlers", [])[handler]["permissions"]:
if "*" in permission:
LOG.warning(
"Use specific handler permissions instead of using wildcards: %s",
permission,
)

try:
additional_properties_validator.validate(resource_spec)
except ValidationError as e:
Expand All @@ -158,13 +182,11 @@ def load_resource_spec(resource_spec_file): # noqa: C901
in it. Please fix the warnings: %s",
str(e),
)
in_readonly = _is_in(resource_spec, "readOnlyProperties")
in_createonly = _is_in(resource_spec, "createOnlyProperties")

primary_ids = resource_spec["primaryIdentifier"]

for primary_id in primary_ids:
if not in_readonly(primary_id) and not in_createonly(primary_id):
for primary_id in resource_spec["primaryIdentifier"]:
if primary_id not in resource_spec.get(
"readOnlyProperties", []
) and primary_id not in resource_spec.get("createOnlyProperties", []):
LOG.warning(
"Property 'primaryIdentifier' - %s must be specified \
as either readOnly or createOnly",
Expand Down
14 changes: 14 additions & 0 deletions tests/data/schema/valid/invalid_list_api_options_as_properties.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"typeName" : "AWS::Service::Type",
"description" : "",
"additionalProperties" : false,
"properties" : {
"NextToken" : {
"type" : "string"
}
},
"readOnlyProperties": [
"/properties/NextToken"
],
"primaryIdentifier" : [ "/properties/NextToken" ]
}
17 changes: 17 additions & 0 deletions tests/data/schema/valid/invalid_readOnlyProperties_overlap.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"typeName" : "AWS::Service::Type",
"description" : "",
"additionalProperties" : false,
"properties" : {
"Property" : {
"type" : "string"
}
},
"readOnlyProperties": [
"/properties/Property"
],
"createOnlyProperties": [
"/properties/Property"
],
"primaryIdentifier" : [ "/properties/Property" ]
}
21 changes: 21 additions & 0 deletions tests/data/schema/valid/invalid_wildcard_handler_permissions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"typeName" : "AWS::Service::Type",
"description" : "",
"additionalProperties" : false,
"properties" : {
"Property" : {
"type" : "string"
}
},
"readOnlyProperties": [
"/properties/Property"
],
"primaryIdentifier" : [ "/properties/Property" ],
"handlers": {
"create": {
"permissions": [
"service:create*"
]
}
}
}