catch common resource schema issues in cfn validate #663

PatMyron · 2021-01-08T06:11:51Z

resource types with LIST API options like MaxResults, NextToken, Filters, etc as properties of the resource itself:
aws-cloudformation/aws-cloudformation-resource-providers-licensemanager#3
aws-cloudformation/aws-cloudformation-resource-providers-iotwireless#3
aws-cloudformation/aws-cloudformation-resource-providers-ssm#75

botocore $ grep -h 'input_token' botocore/data/*/*/paginators-1.json | tr '[:upper:]' '[:lower:]' | sort | uniq -ic | sort -nr | head
1147       "input_token": "nexttoken",
 328       "input_token": "marker",
  40       "input_token": "pagetoken",
  18       "input_token": "position",
  16       "input_token": "nextmarker",
   7       "input_token": "nextpagetoken",
   5       "input_token": "paginationtoken",

botocore $ grep -h 'output_token' botocore/data/*/*/paginators-1.json | tr '[:upper:]' '[:lower:]' | sort | uniq -ic | sort -nr | head
1147       "output_token": "nexttoken",
 199       "output_token": "marker",
  57       "output_token": "nextmarker",
  45       "output_token": "nextpagetoken",

botocore $ grep -h 'limit_key' botocore/data/*/*/paginators-1.json | tr '[:upper:]' '[:lower:]' | sort | uniq -ic | sort -nr | head
 931       "limit_key": "maxresults",
 159       "limit_key": "maxrecords",
 144       "limit_key": "limit",
 129       "limit_key": "maxitems",
  42       "limit_key": "pagesize",

readOnlyProperties overlapping with writeOnlyProperties or createOnlyProperties or required:
aws-cloudformation/aws-cloudformation-resource-providers-auditmanager#2
aws-cloudformation/aws-cloudformation-resource-providers-codeartifact#38
AWS::Events::Archive.ArchiveName
#668

wildcards in handler permissions:
CloudformationSchemas $ grep 'permissions.*\*' *
aws-cloudformation/aws-cloudformation-resource-providers-greengrassv2#4
aws-cloudformation/aws-cloudformation-resource-providers-imagebuilder@55fa9bf#r46035310
AWS::ApiGateway::DomainName
AWS::ServiceCatalog::CloudFormationProvisionedProduct

TODO in future PRs:

~~invalid regular expression patterns~~
~~non-ASCII resource schema characters~~
~~hardcoded aws partition~~
~~incorrect min/max constraints~~
~~inconsistent property casing~~
~~hardcoding constantly evolving enums like instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly~~
~~insertionOrder not specified correctly for array (default value for insertionOrder is true cloudformation-resource-schema#47 (comment))~~
~~taggable not specified correctly (Adding taggable support in schema cloudformation-resource-schema#105 (comment))~~
Add runtime schema validation for things that can't be checked by JSONSchema cloudformation-resource-schema#17
missing type of properties
both inline and referenced definitions: CloudformationSchemas $ grep -C 1 '$ref' * | grep '"type"'
nested property definitions that can't be properly backported to the Resource Specification yet
resource semantic syntax for nested property lists / definitions / empty lists
unrecognized IAM actions according to cfn-lint --include-experimental

how to run new validations on all existing resource provider schemas

Inspired by https://github.com/hashicorp/terraform-provider-aws/tree/main/providerlint

iann0036 · 2021-01-08T06:23:02Z

src/rpdk/core/data_loaders.py

+        "maxresults" in map(str.lower, resource_spec.get("properties", []))
+        or "nexttoken" in map(str.lower, resource_spec.get("properties", []))
+        or "nextmarker" in map(str.lower, resource_spec.get("properties", []))
+    ):


I've also seen Marker, ClientToken and StatusReason all as problematic properties that are clearly not assignable.

@iann0036 thanks for the heads up, I'll look into these tomorrow:
AWS::EFS::AccessPoint.ClientToken
AWS::LicenseManager::License.ClientToken
AWS::LicenseManager::Grant.ClientToken
AWS::LicenseManager::Grant.StatusReason

AWS::XRay::SamplingRule.SamplingRuleRecord.CreatedAt/ModifiedAt

was easy to track down LIST API option synonyms using paginators, but left those open while we try to find a way to track down all their synonyms too

resource types with LIST API options like MaxResults, NextToken, Filters, etc as properties of the resource itself: aws-cloudformation/aws-cloudformation-resource-providers-licensemanager#3 aws-cloudformation/aws-cloudformation-resource-providers-iotwireless#3 aws-cloudformation/aws-cloudformation-resource-providers-ssm#75 readOnlyProperties overlapping with writeOnlyProperties or createOnlyProperties: aws-cloudformation/aws-cloudformation-resource-providers-auditmanager#2 aws-cloudformation/aws-cloudformation-resource-providers-codeartifact#38

src/rpdk/core/data_loaders.py

aws-cloudformation/aws-cloudformation-resource-providers-greengrassv2#4 aws-cloudformation/aws-cloudformation-resource-providers-imagebuilder@55fa9bf#r46035310

omkhegde · 2021-01-19T17:45:09Z

src/rpdk/core/data_loaders.py

+    ):
+        LOG.warning(
+            "readOnlyProperties cannot be specified by customers and should not overlap with writeOnlyProperties or createOnlyProperties"
+        )


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?

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

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.

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

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.

johnttompkins

any unit tests for these checks? i see the example schemas was curious if they are used by our test suite

PatMyron · 2021-01-19T19:31:56Z

any unit tests for these checks? i see the example schemas was curious if they are used by our test suite

Yup, these new code branches hit by adding those schemas:

cloudformation-cli/.coveragerc

Line 6 in c542ea9

fail_under = 100

johnttompkins

Would like to see some additional logging to aid customers in finding issues in schema rather than catch all log messages. Would be good to log which properties trip the checks

wbingli · 2021-01-20T00:31:12Z

src/rpdk/core/data_loaders.py

+    ):
+        LOG.warning(
+            "readOnlyProperties cannot be specified by customers and should not overlap with writeOnlyProperties or createOnlyProperties"
+        )


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.

continuing #663 readOnlyProperties overlapping with required: AWS::CodeArtifact::Repository.DomainName AWS::MWAA::Environment.Name

iann0036 · 2021-01-23T01:52:06Z

Saw CallerReference recently, which could be good for the list

resource types with LIST API options like MaxResults, NextToken, Filters, etc as properties of the resource itself: aws-cloudformation/aws-cloudformation-resource-providers-licensemanager#3 aws-cloudformation/aws-cloudformation-resource-providers-iotwireless#3 aws-cloudformation/aws-cloudformation-resource-providers-ssm#75 readOnlyProperties overlapping with writeOnlyProperties or createOnlyProperties: aws-cloudformation/aws-cloudformation-resource-providers-auditmanager#2 aws-cloudformation/aws-cloudformation-resource-providers-codeartifact#38 no wildcards in handler permissions: aws-cloudformation/aws-cloudformation-resource-providers-greengrassv2#4 aws-cloudformation/aws-cloudformation-resource-providers-imagebuilder@55fa9bf#r46035310

continuing aws-cloudformation#663 readOnlyProperties overlapping with required: AWS::CodeArtifact::Repository.DomainName AWS::MWAA::Environment.Name

resource types with LIST API options like MaxResults, NextToken, Filters, etc as properties of the resource itself: aws-cloudformation/aws-cloudformation-resource-providers-licensemanager#3 aws-cloudformation/aws-cloudformation-resource-providers-iotwireless#3 aws-cloudformation/aws-cloudformation-resource-providers-ssm#75 readOnlyProperties overlapping with writeOnlyProperties or createOnlyProperties: aws-cloudformation/aws-cloudformation-resource-providers-auditmanager#2 aws-cloudformation/aws-cloudformation-resource-providers-codeartifact#38 no wildcards in handler permissions: aws-cloudformation/aws-cloudformation-resource-providers-greengrassv2#4 aws-cloudformation/aws-cloudformation-resource-providers-imagebuilder@55fa9bf#r46035310

continuing aws-cloudformation#663 readOnlyProperties overlapping with required: AWS::CodeArtifact::Repository.DomainName AWS::MWAA::Environment.Name

continuing aws-cloudformation/cloudformation-cli#663 readOnlyProperties overlapping with required: AWS::CodeArtifact::Repository.DomainName AWS::MWAA::Environment.Name

PatMyron force-pushed the catch-schema-issues branch 2 times, most recently from c2c7cbb to 4d1e326 Compare January 8, 2021 06:24

iann0036 reviewed Jan 8, 2021

View reviewed changes

PatMyron force-pushed the catch-schema-issues branch 3 times, most recently from 394a0d4 to 3828495 Compare January 13, 2021 23:00

PatMyron added the schema Related to the provider meta schema label Jan 14, 2021

PatMyron mentioned this pull request Jan 14, 2021

Operations/methods missing paginators boto/botocore#1462

Closed

PatMyron force-pushed the catch-schema-issues branch 2 times, most recently from 01b3066 to 4d9bf63 Compare January 14, 2021 02:25

PatMyron mentioned this pull request Jan 14, 2021

added primary id validation #482

Merged

test resource schemas with common issue warnings

32c9c0a

PatMyron force-pushed the catch-schema-issues branch from 4d9bf63 to 32c9c0a Compare January 14, 2021 02:42

PatMyron marked this pull request as ready for review January 14, 2021 02:51

PatMyron requested review from johnttompkins, omkhegde, kddejong, miparnisari and srujithpoondla03 January 14, 2021 03:02

johnttompkins reviewed Jan 14, 2021

View reviewed changes

src/rpdk/core/data_loaders.py Outdated Show resolved Hide resolved

PatMyron force-pushed the catch-schema-issues branch from dbfdb72 to 452021b Compare January 17, 2021 04:22

no wildcards in handler permissions

893bd03

aws-cloudformation/aws-cloudformation-resource-providers-greengrassv2#4 aws-cloudformation/aws-cloudformation-resource-providers-imagebuilder@55fa9bf#r46035310

PatMyron force-pushed the catch-schema-issues branch from 452021b to 893bd03 Compare January 17, 2021 04:26

omkhegde reviewed Jan 19, 2021

View reviewed changes

johnttompkins reviewed Jan 19, 2021

View reviewed changes

johnttompkins approved these changes Jan 19, 2021

View reviewed changes

omkhegde approved these changes Jan 19, 2021

View reviewed changes

wbingli approved these changes Jan 20, 2021

View reviewed changes

adding context for LIST API options warnings

921b60b

PatMyron merged commit 197b3b1 into master Jan 20, 2021

PatMyron deleted the catch-schema-issues branch January 20, 2021 01:45

PatMyron added a commit that referenced this pull request Jan 20, 2021

catch common resource schema issues

3604cdf

continuing #663 readOnlyProperties overlapping with required: AWS::CodeArtifact::Repository.DomainName AWS::MWAA::Environment.Name

PatMyron mentioned this pull request Jan 20, 2021

catch common resource schema issues in cfn validate #668

Merged

PatMyron added a commit that referenced this pull request Jan 21, 2021

catch common resource schema issues (#668)

13dc44a

continuing #663 readOnlyProperties overlapping with required: AWS::CodeArtifact::Repository.DomainName AWS::MWAA::Environment.Name

PatMyron mentioned this pull request Jan 21, 2021

Get value constraints from AWS registry types aws-cloudformation/cfn-lint#1867

Merged

PatMyron changed the title ~~catch common resource schema issues~~ catch common resource schema issues in cfn validate Jan 27, 2021

PatMyron mentioned this pull request Jan 28, 2021

catch common resource schema issues in cfn validate #675

Merged

PatMyron mentioned this pull request Feb 5, 2021

Validation: Resource property cannot set readOnly properties aws-cloudformation/cloudformation-resource-schema#102

Open

PatMyron mentioned this pull request Mar 18, 2021

cfn validate iann0036/cfn-rps-lint#9

Open

PatMyron mentioned this pull request Apr 13, 2021

catch common resource schema issues in cfn validate #729

Merged

PatMyron added the schema processing label May 5, 2021

PatMyron mentioned this pull request Jun 10, 2021

Add runtime schema validation for things that can't be checked by JSONSchema aws-cloudformation/cloudformation-resource-schema#17

Open

6 tasks

PatMyron mentioned this pull request Jun 22, 2021

catch common resource schema issues in cfn validate #785

Merged

PatMyron mentioned this pull request Jul 3, 2021

Implement AWS::Events:Connection, AWS::Events::ApiDestination resourc… aws-cloudformation/aws-cloudformation-resource-providers-events#2

Merged

PatMyron mentioned this pull request Oct 22, 2021

Adding resource type for VS Code Remote Development aws-cloudformation/aws-cloudformation-samples#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

catch common resource schema issues in cfn validate #663

catch common resource schema issues in cfn validate #663

PatMyron commented Jan 8, 2021 •

edited

Loading

iann0036 Jan 8, 2021

PatMyron Jan 8, 2021 •

edited

Loading

PatMyron Jan 20, 2021 •

edited

Loading

omkhegde Jan 19, 2021

PatMyron Jan 19, 2021

omkhegde Jan 19, 2021

johnttompkins Jan 19, 2021

wbingli Jan 20, 2021

johnttompkins left a comment

PatMyron commented Jan 19, 2021

johnttompkins left a comment

wbingli Jan 20, 2021

iann0036 commented Jan 23, 2021 •

edited by PatMyron

Loading

catch common resource schema issues in cfn validate #663

catch common resource schema issues in cfn validate #663

Conversation

PatMyron commented Jan 8, 2021 • edited Loading

iann0036 Jan 8, 2021

Choose a reason for hiding this comment

PatMyron Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

PatMyron Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

omkhegde Jan 19, 2021

Choose a reason for hiding this comment

PatMyron Jan 19, 2021

Choose a reason for hiding this comment

omkhegde Jan 19, 2021

Choose a reason for hiding this comment

johnttompkins Jan 19, 2021

Choose a reason for hiding this comment

wbingli Jan 20, 2021

Choose a reason for hiding this comment

johnttompkins left a comment

Choose a reason for hiding this comment

PatMyron commented Jan 19, 2021

johnttompkins left a comment

Choose a reason for hiding this comment

wbingli Jan 20, 2021

Choose a reason for hiding this comment

iann0036 commented Jan 23, 2021 • edited by PatMyron Loading

PatMyron commented Jan 8, 2021 •

edited

Loading

PatMyron Jan 8, 2021 •

edited

Loading

PatMyron Jan 20, 2021 •

edited

Loading

iann0036 commented Jan 23, 2021 •

edited by PatMyron

Loading