Generic Match/Project/Validate pattern rule

[kftsehk]

Issue #, if available:

Targeting the implementation of cross-checking, such as

#2296 #1059 #1439

Description of changes:

This is an attempt to make v0.x linter more capable of cross-validation that depends on multiple fields

• A generic Match/Project/Validate pattern rule
• YAML declared Match/Project/Validate parameters
• Implement [feature request] Validate Cloudfront::Distribution.DistributionConfig.DefaultCacheBehavior.TargetOriginId exists #2296
  • Assign rule ID (temporarily E2554)
  • Update changelog
• Implement AWS::CloudWatch::Alarm.Period >= 60 for metrics in the AWS/ namespace #1059
  • Assign rule ID (temporarily E2555 / E2556)
  • Update changelog
• Implement New rule for when ZipFile can be used based on Runtime #1439
  • Assign rule ID (temporarily E2553)
  • Update changelog

Some thoughts not implemented

• And/Or operator to Match/Validate is useful
• Good to have a way to show more useful message for different validation that failed, maybe with each Validations item should accept a string template with one %s
• Sometimes might want to group multiple rules under same error ID (E2555 / E2556 are quite similar in this case)
• Custom function fn = (checked_value) => bool needed, met a case need to check 10 | 30 | divisible by 60 cannot be enforced even with regex

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

[kddejong]

I think some of this makes sense. As we move to v1 and the registry provider schemas we are doing a lot more json schema validation as well. In that approach we are actually building template scenarios that remove conditions and focus on the actual values. We also modified how json schema validation works to handle the other intrinsic functions.

Why I say all of this is we can also write rules by extending the schema validation and writing json schemas.
fefa824

I don't think all the linked scenarios will work with this approach. I haven't written a mechanism for people to write their own json schemas at this point and this will not work across resources.

[kftsehk]

Surely will look into v1 preview. It is good to hear that these might be included in v1.

I much agree the v1 change that it is way better to focus on

• (A) possible scenarios generated by conditions
• (B) static value inference: !Ref AWS:NoValue (set it back to undefined!), !FindInMap, !Ref Parameter, !Ref AWS::PseudoParameters
• (C) static type inference: !Ref X, !GetAtt X.Att
  before rules are enforced.

We have used cfn for quite some years and definitely met some workarounds, issues that we want be able to configure the linters to check, and ended up arbitrary script that just load the yaml and check that.
Some use case and preliminary thoughts from experience using and contributing to cfn-lint v0.7 below in case it help

Please take it lightly in case it had been considered, since I have not dive into details of v1.

(A) Handling condition

My main concern is with linter performance if repeatedly evaluating parameter combinations on the whole template, which can be a pretty large if number of conditions are more than handful.

How condition can affect a template

1. By adding or removing resources
2. By choosing among different property values within a resources

It maybe possible to compute and aggregate <ConditionConsumed, [ResourceConsumedByRefOrGetAtt]> for each resources, and check against <Scenario, AvailableResource>, so 1. is solved, and 2. can be done locally only considering possible values of that particular condition.

(B) Static value inference

The current API seem to leave static value inference to rule implementation by check_find_in_map etc, it is definitely better to handle centrally by having concrete value at the rule

In this issue I've specifically added "Projection" for these use case I can think of

• [feature request] Validate Cloudfront::Distribution.DistributionConfig.DefaultCacheBehavior.TargetOriginId exists #2296, the list of origin id are arbitrarily named by user
• Checking AWS::StepFunctions::StateMachine.Definition #2619 Where StartAt Next etc must be one of the states which we name it arbitrarily

(C) Static type inference

cfn-lint v0.7 will miss on mismatched !Ref / !GetAtt, for example, !Ref LogicalName instead of !GetAtt LogicalName.Arn when an arn is actually required and !Ref returns only the logical name, prominently in IAM policy, role, SNS subscription etc.

The susceptibility to human error, for this Arn case, is probably due to the fact that it is quite arbitrary for each resource type that

Does !Ref return an ARN or logical name?: my guess is majority is for logical name, and substantial minority returns arn

• Arn in !Ref https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-secretsmanager-secret.html#aws-resource-secretsmanager-secret-return-values
• Logical name in !Ref https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#aws-resource-iam-role-return-values

Does !GetAtt X.Arn is available: most resources has, but one can always find exception

• No Arn attr.: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-secretsmanager-secret.html#aws-resource-secretsmanager-secret-return-values

When arn is required by some field not explicitly naming someArn

• AWS::Serverless::Function.Role: Role is (almost?) always an Arn, although it is not named RoleArn as in AWS::Lambda::Function
• AWS::SNS::Subscription.Endpoint: Is either email, url or arn, never a logical name
• AWS::ECS::TaskDefinition.ExecutionRoleArn/TaskRoleArn: Surprisingly, I am able to find one instance, where it is named Arn, but you can actually use !Ref https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-taskdefinition.html#cfn-ecs-taskdefinition-taskrolearn

Generally the returns will fall into several types, being one of url, ip, hostedZoneId, cidr, arn, string (usually logical names) etc. I am not sure if return values are available in cfn spec, if so, return value can then be checked against the requirement of that field to catch the above !Ref/GetAtt confusion

Some additional use cases

(D: suggestion) a stage for simple string substitution to dynamic references from downloaded values: !ImportValue, {{resolve::ssm::*}}, !Sub {{resolve:ssm:*}}

I guess there is no solution for a linter to check against external value, where everyone is happy and still lightweighted. I would suggest allowing to provide a string substitution file so we can pull samples from dev/uat to check against, there is only limited ability with string substitution, but it can be better than nothing.

(E: suggestion) includes non-Properties into check, Metadata, DeletionPolicy those sometimes matters

Met this when dealing with stack deployed with changeset, end up having to put arbitrary metadata around

• SSM Dynamic Reference Change not detected cloudformation-coverage-roadmap#844

It is also useful to enforce some deletion policy config on IaC source code level.

[kddejong]

@kftsehk merge conflict on this one. Do we still need this PR?

[kftsehk]

@kddejong any rule to assign rule id? I think I only miss that here. The conflict can be solved.

[kddejong]

Need to rework this to handle it in v1