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.

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

Get value constraints from AWS registry types #1867

Merged

Conversation

kddejong
Copy link
Contributor

@kddejong kddejong commented Jan 12, 2021

Issue #, if available: #1277

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kddejong kddejong force-pushed the feature/valuecontraintsfromregistry branch from d453491 to 22a86e1 Compare January 12, 2021 21:35
""" main function """
configure_logging()

types = client.list_types(
Copy link
Contributor

@PatMyron PatMyron Jan 12, 2021

Choose a reason for hiding this comment

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

comment to remember pagination if calling list_types() (+1 to boto/botocore#2177)

Copy link
Contributor

Choose a reason for hiding this comment

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

These hosted resource provider schemas might be even better to avoid credential requirements and pagination

@@ -0,0 +1,180 @@
#!/usr/bin/env python
Copy link
Contributor

@PatMyron PatMyron Jan 12, 2021

Choose a reason for hiding this comment

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

let's make sure this is run in #1792 as well

Copy link
Contributor

@PatMyron PatMyron Jan 12, 2021

Choose a reason for hiding this comment

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

thoughts on just putting this in cfn-lint --update-specs like #1682 if we use the hosted resource provider schemas to avoid new credential requirements? Would be great if updates could be easily picked up without us needing to release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea good question. Any thoughts on how this would scale and the speed of it? We could download the zips and read them instead of using the APIs (honestly didn't know that existed). Plus it would help with my rate limiting issues.

@@ -0,0 +1,1883 @@
[
Copy link
Contributor

@PatMyron PatMyron Jan 12, 2021

Choose a reason for hiding this comment

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

thoughts on ditching these files + writing directly to CloudSpecs if we make this part of cfn-lint --update-specs?

think it's confusing that ExtendedSpecs/all is partially manually maintained and partially autogenerated

might be useful to include some metadata that the information is coming from the Registry though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. The automation portions can get confusing. Let me see what I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current values that would need to get corrected in AWS::DataBrew::Job.DatasetName

DatasetName" : {
      "description" : "Dataset name",
      "type" : "string",
      "pattern" : "[ -퟿-�𐀀-\t]*",
      "minLength" : 1,
      "maxLength" : 255
    },

Copy link
Contributor

@PatMyron PatMyron Jan 21, 2021

Choose a reason for hiding this comment

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

found all non-ASCII characters in resource schemas (only pattern ones related to this PR):

$ brew install coreutils
$ ggrep --color='auto' -P -n "[^\x00-\x7F]" *
aws-appflow-connectorprofile.json:158:          "description" : "A unique alphanumeric identifier used to authenticate a user, developer, or calling program to your API.",
aws-appflow-connectorprofile.json:171:          "description" : "A unique alphanumeric identifier used to authenticate a user, developer, or calling program to your API.",
aws-appflow-connectorprofile.json:175:          "description" : "Application keys, in conjunction with your API key, give you full access to Datadog’s programmatic API. Application keys are associated with the user account that created them. The application key is used to log all requests made to the API.",
aws-appflow-connectorprofile.json:215:          "description" : "The identifier for the desired client.",
aws-appflow-connectorprofile.json:245:          "description" : "The identifier for the user.",
aws-appflow-connectorprofile.json:273:          "description" : "The identifier for the desired client.",
aws-appflow-connectorprofile.json:327:          "description" : "The object key for the destination bucket in which Amazon AppFlow will place the files.",
aws-appflow-connectorprofile.json:398:          "description" : "A unique alphanumeric identifier used to authenticate a user, developer, or calling program to your API.",
aws-appflow-connectorprofile.json:408:          "description" : "The identifier for the desired client.",
aws-appflow-connectorprofile.json:454:          "description" : "The name of the Snowflake warehouse.",
aws-appflow-connectorprofile.json:458:          "description" : "The name of the Amazon S3 stage that was created while setting up an Amazon S3 stage in the\nSnowflake account. This is written in the following format: < Database>< Schema><Stage Name>.",
aws-appflow-connectorprofile.json:462:          "description" : "The name of the Amazon S3 bucket associated with Snowflake.",
aws-appflow-connectorprofile.json:466:          "description" : "The bucket prefix that refers to the Amazon S3 bucket associated with Snowflake.",
aws-appflow-connectorprofile.json:470:          "description" : "The Snowflake Private Link service name to be used for private data transfers.",
aws-appflow-connectorprofile.json:478:          "description" : "The region of the Snowflake account.",
aws-appflow-connectorprofile.json:522:          "description" : "The identifier for the desired client.",
aws-databrew-job.json:9:      "pattern" : "[ -퟿-�𐀀-�\t]*",
aws-databrew-job.json:27:      "pattern" : "[ -퟿-�𐀀-�\t]*",
aws-databrew-job.json:72:      "pattern" : "[ -퟿-�𐀀-�\t]*",
aws-databrew-recipe.json:8:      "pattern" : "[ -퟿-�𐀀-�\t]*",
aws-databrew-recipe.json:16:      "pattern" : "^[ -.0-^`-퟿-�]*$",
aws-databrew-schedule.json:37:      "pattern" : "[ -퟿-�𐀀-�\t]*",
aws-gamelift-gameservergroup.json:244:      "pattern" : "[ -퟿-�𐀀-�\r\n\t]*",
aws-kinesis-stream.json:26:      "description" : "An arbitrary set of tags (key–value pairs) to associate with the Kinesis stream.",
aws-kinesis-stream.json:73:      "description" : "An arbitrary set of tags (key–value pairs) to associate with the Kinesis stream.",
aws-mediapackage-packagingconfiguration.json:100:          "description" : "This setting controls how ad markers are included in the packaged OriginEndpoint. “NONE” will omit all SCTE-35 ad markers from the output. “PASSTHROUGH” causes the manifest to contain a copy of the SCTE-35 ad markers (comments) taken directly from the input HTTP Live Streaming (HLS) manifest. “SCTE35_ENHANCED” generates ad markers and blackout tags based on SCTE-35 messages in the input source.",
aws-mediapackage-packagingconfiguration.json:141:          "description" : "The Dynamic Adaptive Streaming over HTTP (DASH) profile type. When set to “HBBTV_1_5”, HbbTV 1.5 compliant output is enabled.",
aws-mediapackage-packagingconfiguration.json:217:          "description" : "A list of triggers that controls when the outgoing Dynamic Adaptive Streaming over HTTP (DASH) Media Presentation Description (MPD) will be partitioned into multiple periods. If empty, the content will not be partitioned into more than one period. If the list contains “ADS”, new periods will be created where the Asset contains SCTE-35 ad markers.",

https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-appflow/pull/6#discussion_r562217193
aws-cloudformation/aws-cloudformation-resource-providers-databrew#2 (comment)
https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-gamelift/pull/13#discussion_r562220060

https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-kinesis/pull/6#discussion_r562226794
https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-mediapackage/pull/3#discussion_r562228692
(in TODO list in aws-cloudformation/cloudformation-cli#663 / aws-cloudformation/cloudformation-cli#459)

@PatMyron
Copy link
Contributor

PatMyron commented Jan 12, 2021

Noting constraints applied last overwrite any already existing constraints (seems okay, just noteworthy)

@PatMyron
Copy link
Contributor

PatMyron commented Jan 12, 2021

Do you know how this handles combined JSON schemas? Fine to not handle them yet since there are so few resource provider schemas using them so far, just want to make sure they're at least not handled incorrectly:

grep -l -E -i '(oneOf|anyOf|allOf)' *
aws-applicationinsights-application.json
aws-cloudformation-moduledefaultversion.json
aws-cloudformation-stackset.json
aws-cloudwatch-metricstream.json
aws-databrew-dataset.json
aws-databrew-recipe.json
aws-gamelift-alias.json
aws-iotwireless-wirelessdevice.json
aws-kendra-datasource.json
aws-rds-globalcluster.json
aws-s3-storagelens.json
aws-sagemaker-pipeline.json
aws-synthetics-canary.json

@kddejong kddejong force-pushed the feature/valuecontraintsfromregistry branch from 22a86e1 to 86a6cf0 Compare January 12, 2021 22:26
@PatMyron
Copy link
Contributor

PatMyron commented Jan 12, 2021

This is awesome btw, this alone is pulling in significantly more constraints:

AllowedValues: 315 new
NumberMin/NumberMax: 49 new
AllowedPatternRegex: 773 new
StringMin/StringMax: 662 new

than we had before

"op": "add",
"path": "/ValueTypes/AWS::SageMaker::ModelBiasJobDefinition.EndpointInput.EndTimeOffset",
"value": {
"Allowed
Copy link
Contributor

@PatMyron PatMyron Jan 13, 2021

Choose a reason for hiding this comment

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

Similar to aws-cloudformation/cloudformation-cli#459, we need to think about how to verify regular expressions are valid in Java and Python
(some don't seem to be valid in either though 🤔)

Copy link
Contributor Author

@kddejong kddejong Jan 13, 2021

Choose a reason for hiding this comment

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

Switched to the regex package that supports unicode. That helps bring Python closer to Java but there are still some expressions that are not compliant. Example: ^[a-zA-Z0-9-]+{1,255}$

Copy link
Contributor

Choose a reason for hiding this comment

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

@PatMyron PatMyron changed the title Get value constraints from registry types Get value constraints from AWS registry types Jan 13, 2021
@kddejong kddejong force-pushed the feature/valuecontraintsfromregistry branch 2 times, most recently from 1d8726a to 265a207 Compare January 14, 2021 18:04
@kddejong kddejong force-pushed the feature/valuecontraintsfromregistry branch from fe4d594 to 1a3e7f3 Compare February 11, 2021 21:25
@kddejong
Copy link
Contributor Author

This is going to add a little time to build update-specs process because the amount of jsonpatches. I think the better long term solution is to separate out our spec update process into another repo that we can build and release them.

@jlongtine
Copy link
Contributor

This is going to add a little time to build update-specs process because the amount of jsonpatches. I think the better long term solution is to separate out our spec update process into another repo that we can build and release them.

I like this idea. Separating this from the python codebase would help me with cfn-cue.

@kddejong kddejong force-pushed the feature/valuecontraintsfromregistry branch from 1a3e7f3 to 776f7fb Compare February 11, 2021 22:22
@kddejong kddejong merged commit df9b214 into aws-cloudformation:master Feb 11, 2021
@kddejong kddejong deleted the feature/valuecontraintsfromregistry branch February 11, 2021 22:43
@PatMyron PatMyron mentioned this pull request Feb 15, 2021
direvus pushed a commit to direvus/cfn-python-lint that referenced this pull request Apr 12, 2021
* Get value constraints from registry types
* Eliminate unicode strings from being allowed for patterns
Copy link
Contributor

@PatMyron PatMyron left a comment

Choose a reason for hiding this comment

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

minItems/maxItems could be translated to ListMin/ListMax as well


currently 468 usages of those 2 keywords in AWS resource schemas:

CloudformationSchema $ grep -Eho '"m\w\wItems"' * | sort | uniq -c | sort -nr
 257 "maxItems"
 211 "minItems"

results['.'.join(names + [propname])] = {}
if propdetails.get('pattern'):
p = propdetails.get('pattern')
if '.'.join(names + [propname]) == 'AWS::OpsWorksCM::Server.CustomPrivateKey':
Copy link
Contributor

@PatMyron PatMyron May 13, 2021

Choose a reason for hiding this comment

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

Comment on lines +339 to +342
if propdetails.get('enum'):
results['.'.join(names + [propname])].update({
'AllowedValues': propdetails.get('enum')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +349 to +350
'NumberMin': propdetails.get('minimum'),
'NumberMax': propdetails.get('maximum'),
Copy link
Contributor

@PatMyron PatMyron May 13, 2021

Choose a reason for hiding this comment

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

Could use exclusiveMinimum/exclusiveMaximum as well


Although no AWS resource schemas use these keywords yet anyway


Could even just set NumberMin/NumberMax for these for now even though those are inclusive since that'd still be better than not supporting it at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants