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

CloudFormation Registry resource schema support (--registry-schemas) #1732

Merged
merged 26 commits into from
Dec 2, 2020

Conversation

PatMyron
Copy link
Contributor

@PatMyron PatMyron commented Oct 8, 2020

#1277

CloudFormation Registry announcement
Resource Schema repo


import boto3
for page in boto3.client('cloudformation').get_paginator('list_types').paginate():
    for resource_type in page['TypeSummaries']:
        with open(resource_type['TypeName'].replace("::", "-").lower() + '.json', 'w') as f:
            print(boto3.client('cloudformation').describe_type(Arn=resource_type['TypeArn'])['Schema'], file=f)

@PatMyron PatMyron marked this pull request as ready for review October 19, 2020 20:50

def match(self, cfn):
matches = []
for resource_schema in [os.path.basename(f) for f in glob('src/cfnlint/data/ResourceSchemas/*.json')]:

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?

Copy link
Contributor Author

@PatMyron PatMyron Oct 20, 2020

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

Copy link
Contributor

@kddejong kddejong left a 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
Copy link
Contributor

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):
Copy link
Contributor

@kddejong kddejong Nov 16, 2020

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.

@PatMyron PatMyron changed the title CloudFormation Registry resource schema support CloudFormation Registry resource schema support (--registry-schemas) Dec 1, 2020
README.md Show resolved Hide resolved
@@ -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):
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@PatMyron PatMyron Dec 1, 2020

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

Copy link
Contributor

@kddejong kddejong left a 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

@PatMyron PatMyron merged commit b04bd09 into master Dec 2, 2020
@PatMyron PatMyron deleted the schemas branch December 2, 2020 20:52
@PatMyron PatMyron linked an issue Dec 2, 2020 that may be closed by this pull request
@PatMyron
Copy link
Contributor Author

PatMyron commented Dec 5, 2020

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:.*'

Screen Shot 2020-12-04 at 2 54 46 PM

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}}'

@jasonmimick
Copy link

Need to add this new parameter to the config file json schema:
cfnlint/data/CfnLintCli/config/schema.json
needs

...
 "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     }

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.

Support registry resource schemas
5 participants