-
Notifications
You must be signed in to change notification settings - Fork 598
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
CloudFormation Registry resource schema support (--registry-schemas) #1732
Conversation
|
||
def match(self, cfn): | ||
matches = [] | ||
for resource_schema in [os.path.basename(f) for f in glob('src/cfnlint/data/ResourceSchemas/*.json')]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this would assume all the potential schemas are downloaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, wrote list-types
/ describe-type
script to make it easy, but don't think it makes sense to call every runtime due to
- credentials and networking requirements
- ability to provide additional resource schemas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure the standard jsonschema packages are going to fit the CloudFormation functionality. We can expand the schema from the registry to support the intrinsic functions when we read them but the possibilities are massive.
import os | ||
import re | ||
from glob import glob | ||
from jsonschema import validate, ValidationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the big problems we are going to have with using this package is handling Fn::If
or other intrinsic functions. Either way we are going to have to handle it outside of the schema validation which we have logic to handle or we are going to have to rewrite these validation functions that this package provides to handle these things differently.
resource_type = load_resource(ResourceSchemas, resource_schema)['typeName'] | ||
for resource_name, resource_values in cfn.get_resources([resource_type]).items(): | ||
properties = resource_values.get('Properties', {}) | ||
if not re.match(REGEX_DYN_REF, str(properties)) and not any(x in str(properties) for x in PSEUDOPARAMS + UNCONVERTED_SUFFIXES) and FN_PREFIX not in str(properties): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this says ignore intrinsic function errors but doesn't handle more complex Fn::If scenarios.
ResourceName:
Type: AWS::CloudFormation::StackSet
Properties:
Fn::If:
- ConditionName
- PropertiesScenarioTrue
- PropertiesScenarioFalse
We basically don't get a failure even though there could be one in the provided scenario.
@@ -39,7 +40,7 @@ class UnexpectedRuleException(CfnLintExitException): | |||
"""When processing a rule fails in an unexpected way""" | |||
|
|||
|
|||
def run_cli(filename, template, rules, regions, override_spec, build_graph, mandatory_rules=None): | |||
def run_cli(filename, template, rules, regions, override_spec, build_graph, registry_schemas, mandatory_rules=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outside packages shouldn't be using run_cli by any concerns with the ordering here? I've been adding parameters and setting a default value to keep some backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only found this from searching: https://github.com/Cloudzero/samwise/blob/793463e0774f9d1c702f4252377f19fa3408a558/samwise/__main__.py#L129 which looks already broken from #1411
cc @silvexis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have generally been debating compatibility for reliance on undocumented internal functions/variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few changes but looks good to me
Example messages: [cfn-lint] E3000 'RequiredEnumProperty' is a required property
[cfn-lint] E3000 Additional properties are not allowed ('InvalidProperty' was unexpected)
[cfn-lint] E3000 'string' is not of type 'integer'
[cfn-lint] E3000 200 is greater than the maximum of 100
[cfn-lint] E3000 -1 is less than the minimum of 0
[cfn-lint] E3000 'd' is not one of ['a', 'b', 'c']
[cfn-lint] E3000 'name' does not match 'arn:.*' schema/template{
"typeName" : "Third::Party::Type",
"description" : "",
"additionalProperties" : false,
"properties" : {
"RequiredEnumProperty" : {
"type" : "string",
"enum": [
"a",
"b",
"c"
]
},
"PatternProperty" : {
"type" : "string",
"pattern": "arn:.*"
},
"RangeProperty" : {
"type": "integer",
"minimum": 0,
"maximum": 100
}
},
"required" : ["RequiredEnumProperty"],
"primaryIdentifier" : [ "/properties/RequiredEnumProperty" ]
} Resources:
ResourceMissingRequiredProperty:
Type: Third::Party::Type
ResourceMissingRequiredProperty2:
Type: Third::Party::Type
Properties:
RangeProperty: 50
ResourceWithInvalidProperty:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: required
RangeProperty: 50
InvalidProperty: invalid
ResourceWithWrongPrimitiveType:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: a
RangeProperty: string
ResourceWithValueOutsideMinMaxRange:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: a
RangeProperty: 200
ResourceWithValueOutsideMinMaxRange2:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: a
RangeProperty: -1
ResourceWithInvalidValue:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: d
RangeProperty: 50
ResourceWithInvalidPattern:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: a
PatternProperty: name
ValidResource:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: a
RangeProperty: 50
ResourceWithUnresolvableSyntax:
Type: Third::Party::Type
Properties:
RequiredEnumProperty: !Ref ValidResource
RangeProperty: '{{resolve:secretsmanager:secret:secret:secret}}' |
Need to add this new parameter to the config file json schema: ...
"ignore_templates": {
79 "description": "Templates to ignore",
80 "items": {
81 "type": "string"
82 },
83 "type": "array"
84 },
85 "registry_schemas": {
86 "description": "one or more directories of CloudFormation Registry Resource Schemas",
87 "items": {
88 "type": "string"
89 },
90 "type": "array"
91 }
|
#1277
CloudFormation Registry announcement
Resource Schema repo
RuleMatch
format (only returns one error per resource + exact context isn't great, especially for combined JSON schemas)GetAtt
referenced elsewhere: CloudFormation Registry resource support - GetAtt rules broken #2622E3001
to include these resource types:https://github.com/aws-cloudformation/cfn-python-lint/blob/93659fba5cf86ee34177c55b28df0aa9140b7dc9/src/cfnlint/rules/resources/Configuration.py#L105list-types
/describe-type
script to make it easier: