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

v1 - Using Resource Provider Schemas #2606

Closed
36 of 37 tasks
kddejong opened this issue Mar 2, 2023 · 16 comments · Fixed by #2583
Closed
36 of 37 tasks

v1 - Using Resource Provider Schemas #2606

kddejong opened this issue Mar 2, 2023 · 16 comments · Fixed by #2583
Assignees
Labels
enhancement New feature or request v1 v1.X

Comments

@kddejong
Copy link
Contributor

kddejong commented Mar 2, 2023

Description

The effort to go to v1 will be driven by the goal to convert from CloudFormation specs to CloudFormation resource provider schemas. This will be a large change for how cfn-lint works and will result in rules having to be updated and changed. This issue will also serve to communicate the migration efforts.

Details

The CloudFormation resource provider schema is based on JSON Schema draft-07 but has modifications to handle the CloudFormation service These schemas allow us to do more straight JSON schema validation against resource properties. There are modifications to the schema and how the JSON schema validators work to handle CloudFormation specific capabilities. We find it important to provide the functionality and features that cfn-lint has had in v0 including the ability to disable and configure rules. As a result we will integrate in JSON schema validation into cfn-lint and cfn-lint will provide the functionality to massage the schemas and handle CloudFormation specific capabilities. Additionally there are checks in cfn-lint (best practices, etc.) that cannot be written into JSON schema validation.

Rule changes

All rules that have been modified or where the logic will change:

  • E1027 - DynamicReferenceSecureString - Depended on match_resource_sub_properties. Was re-written to use match
  • E1010 - GetAtt - Used the specs for valid GetAtt values
  • E1022 - Join - Used the specs to determine the type of a GetAtt
  • E1019 - Sub - Used the specs to determine the type of a GetAtt
  • E1029 - SubNeeded - Used the specs to determine valid GetAtt values
  • E6003 - Value (Outputs) - Used the specs to determine the type of a GetAtt
  • W2031 - AllowedPattern (Parameters) - Replaced by JSON Schema validation logic
  • W2030 - AllowedValue (Parameters) - Replaced by JSON Schema validation logic
  • E3001 - Configuration (Resources) - Replaced by JSON Schema validation. Logic is outside of the registry
  • E3000 - JsonSchema (Resources) - Expanded to handle registry resource schemas. Parent rule for all JSON schema validation rules
  • E3031 - AllowedPattern (Resource/Properties) - Replaced by JSON Schema validation logic
  • E3030 - AllowedValue (Resource/Properties) - Replaced by JSON Schema validation logic
  • E2522 - AtLeastOne (Resource/Properties) - Deleted?
  • E3017 - CfnSchema (Resource/Properties) (New Rule) - New rule to extend JSON Schema validation to handle scenarios not covered by the registry schema
  • E2520 - Exclusive (Resource/Properties) - Deleted. Handled by if/then/else logic in JSON schema validation
  • E2521 - Inclusive (Resource/Properties) - Deleted. Handled by if/then/else logic in JSON schema validation
  • E3502 - JsonSize (Resource/Properties) - Deleted. Converted to string maxLength and minLength validation
  • E3037 - ListDuplicates (Resource/Properties) - Replaced by JSON Schema validation logic
  • I3037 - ListDuplicatesAllowed (Resource/Properties) - Replaced by JSON Schema validation logic
  • E3032 - ListSize (Resource/Properties) - Replaced by JSON Schema validation logic
  • E3034 - NumberSize (Resource/Properties) - Replaced by JSON Schema validation logic
  • E2523 - OnlyOne (Resource/Properties) - Replaced by oneOf logic in JSON schema
  • E3002 - Properties (Resource/Properties) - Replaced by properties and additionalProperties in JSON schema
  • E3003 - Required (Resource/Properties) - Replaced by required in JSON schema
  • E3017 - RequiredBasedOnValue (Resource/Properties) - Replaced by if/then/else logic using cfnSchema and JSON schema
  • E3033 - StringSize (Resource/Properties) - Replaced by minLength and maxLength in JSON schema. May still need to add exceptions for dynamic references
  • E3018 - UnwantedBasedOnValue (Resource/Properties) - Replaced by if/then/else logic using cfnSchema and JSON schema
  • E3012 - ValuePrimitiveType (Resource/Properties) - Replaced by type in JSON schema
  • E3008 - ValueRefGetAtt (Resource/Properties) - WIP

Breaking changes

This will serve as a list of changes that will occur in the migration from v0 to v1

  • match_resource_sub_properties will be deprecated. This function was based on the specs and is not easily converted into the resource provider schema approach

Issues

  • Some resource schemas have readOnlyProperties that are still write-able. Since we are removing them for validation we need to know the list of exceptions
  • Packaging doesn't like linked files so they are dropped. Need a different approach to handling marking resources as cached

Action Items

  • Convert custom or 3rd party registry schema validation to the new rule E3000
  • Don't run property checks multiple times if the specs across regions are cached
  • Convert enum values taken from boto
  • Convert other manually created allowedvalues, patterns, etc.
  • Write tests to validate JSON Schema docs and extensions
  • Add in AWS::CDK::Metadata schema
@kddejong kddejong added the enhancement New feature or request label Mar 2, 2023
@kddejong kddejong self-assigned this Mar 2, 2023
@kddejong kddejong added this to To do in 1.0.0 Release via automation Mar 2, 2023
@kddejong kddejong moved this from To do to In progress in 1.0.0 Release Mar 2, 2023
@kddejong kddejong pinned this issue Mar 2, 2023
@kddejong kddejong removed a link to a pull request Mar 2, 2023
@PatMyron
Copy link
Contributor

PatMyron commented Mar 4, 2023

  • match_resource_sub_properties will be deprecated

undocumented and no other public repos show up in sourcegraph search using that, which bodes well

@kddejong
Copy link
Contributor Author

kddejong commented Mar 9, 2023

When dealing with oneOf, allOf, anyOf, if/then/else logic JSON schema will fail with a rolled up error and the context will include all the errors determined from the sub schemas. While this works it hides some of the information for the user to determine what actually caused their issues and how to fix it.

Do not raise any generic errors for these types
allOf create multiple errors for all sub errors
anyOf create multiple errors for all sub errors
oneOf return the best error
if/then/else return the best error

@whoDoneItAgain
Copy link
Contributor

i saw the rc1 released yesterday. i ran it against one my pipelines CF stacks. It returned a many findings, though basically the same 3-5 findings repeatedly. Where would you like the feedback?

@kddejong
Copy link
Contributor Author

kddejong commented May 2, 2023

I would love the feedback. I've been trying to find as many templates as I can to run against behind the scenes but I'm running out of additional issues to chase. Thanks!

@whoDoneItAgain
Copy link
Contributor

whoDoneItAgain commented May 2, 2023

E3001 and E3030 are the most seen errors. Below are the unique errors and a snippet example.

E3030 - {'Fn::FindInMap': ['StaticParams', 'S3EmptierLambda', 'LambdaOSArchitechture']} is not one of ['x86_64', 'arm64']

LambdaLogGroup:
Type: AWS::Logs::LogGroup
DeletionPolicy: Delete
UpdateReplacePolicy: Delete
Properties:
LogGroupName: !Sub '/aws/lambda/${Region}-${VPCName}-${AppName}-${LambdaPurpose}'
RetentionInDays: !FindInMap [GlobalSettings, GlobalSettings, LogRetention]`

E3001 - {'Fn::If': ['ConditionDev', 'Delete', 'Retain']} is not one of ['Delete', 'Retain', 'Snapshot']

E3001 - {'Fn::If': ['ConditionDev', 'Delete', 'Retain']} is not of type 'string'

ALBLogsBucket:
Type: AWS::S3::Bucket
DeletionPolicy: !If
- ConditionDev
- Delete
- Retain
UpdateReplacePolicy: !If
- ConditionDev
- Delete
- Retain
Properties:

E0002 - Unknown exception while processing rule E6003: Resource type 'Custom::R53EndpointIp' is not found in 'us-east-1'

E1010 - 'EndpointIds' is not one of ['FirewallId', 'ResourceArn'] (This is an interesting one. Erroring on a custom resource)

NetworkFirewallVpceAz1:
Type: Custom::FirewallVpceFinder
Condition: ConditionCreatePublicSubnets
Properties:
ServiceToken:
Fn::ImportValue: !Sub '${Region}-${VpcName}-lambda-firewall-vpce-finder-fn-arn'
EndpointIds: !GetAtt NetworkFirewall.EndpointIds
AvailabilityZone: !GetAtt FirewallSubnet1.AvailabilityZone

E3032 - [] is too short

(Its barking about the Regions section)

ClientVpnAuthStackSet:
Type: AWS::CloudFormation::StackSet
Condition: ConditionCreateCrossRegionAuthRules
Properties:
...
StackInstancesGroup:
- DeploymentTargets:
Accounts:
- !Ref AWS::AccountId
Regions:
- !If
- ConditionCreateAuthorizationRulesUse1
- us-east-1
- !Ref AWS::NoValue
- !If
- ConditionCreateAuthorizationRulesUse2
- us-east-2
- !Ref AWS::NoValue
- !If
- ConditionCreateAuthorizationRulesUsw2
- us-west-2
- !Ref AWS::NoValue
- !If
- ConditionCreateAuthorizationRulesAps1
- ap-south-1
- !Ref AWS::NoValue

E3012 - Specify only 'SubnetMappings' or 'Subnets'

ApplicationLoadBalancer:
Type: AWS::ElasticLoadBalancingV2::LoadBalancer
Condition: ConditionCreateLambda
Properties:
Type: application
Name: !Sub '${Region}-cross-vpc-testing-alb'
Scheme: internal
SecurityGroups:
- !Ref ALBSecurityGroup
Subnets:
- Fn::ImportValue: !Sub '${Region}-${VPCName}-private-subnet-1-id'
- Fn::ImportValue: !Sub '${Region}-${VPCName}-private-subnet-2-id'
- !If
- ConditionAzCount3
- Fn::ImportValue: !Sub '${Region}-${VPCName}-private-subnet-3-id'
- !Ref AWS::NoValue

@kddejong
Copy link
Contributor Author

kddejong commented May 2, 2023

Action items:

  • Language extension schema changes. This will handle E3001 conditions in the DeletionPolicy issue. Docs
  • Update enum, pattern, type to handle functions
  • Fix an issue with GetAtts where we dropped arrays that have a ref in the items portion of the schema
  • E3012 - handle intrinsic functions in cfnSchema validation
  • Update conditions logic to include resource or output level Condition scenarios

I'll add more details as I investigate and figure out the solutions.

@kddejong kddejong mentioned this issue May 5, 2023
@kddejong
Copy link
Contributor Author

kddejong commented May 7, 2023

for E3032 - [] is too short can you tell me what Condition: ConditionCreateCrossRegionAuthRules is configured like?

I am testing this and I'm not able to repeat this error. I used the following condition logic.

  ConditionCreateAuthorizationRulesUse1: !Equals [!Ref AWS::Region, "us-east-1"]
  ConditionCreateAuthorizationRulesUse2: !Equals [!Ref AWS::Region, "us-east-2"]
  ConditionCreateAuthorizationRulesUsw2: !Equals [!Ref AWS::Region, "us-west-2"]
  ConditionCreateAuthorizationRulesAps1: !Equals [!Ref AWS::Region, "ap-south-1"]
  ConditionCreateCrossRegionAuthRules: !Or [
    !Condition ConditionCreateAuthorizationRulesUse1,
    !Condition ConditionCreateAuthorizationRulesUse2,
    !Condition ConditionCreateAuthorizationRulesUsw2,
    !Condition ConditionCreateAuthorizationRulesAps1 ]

@whoDoneItAgain
Copy link
Contributor

Mappings:
ClientVpn:
Fn::Transform:
Name: AWS::Include
Parameters:
Location: !Sub 's3://${Region}-${Environment}-infra-pipeline-bucket-${AWS::AccountId}/resource-sync/transforms/cvpn/mapping-cvpn-${ClientVpnPurpose}.yaml'

Conditions:
ConditionCreateCrossRegionAuthRules: !Or
- !Condition ConditionCreateAuthorizationRulesUse1
- !Condition ConditionCreateAuthorizationRulesUse2
- !Condition ConditionCreateAuthorizationRulesUsw2
- !Condition ConditionCreateAuthorizationRulesAps1

ConditionCreateAuthorizationRulesUse1:
!Equals [!FindInMap [ClientVpn, Enabled, use1], true]

ConditionCreateAuthorizationRulesUse2:
!Equals [!FindInMap [ClientVpn, Enabled, use2], true]

ConditionCreateAuthorizationRulesUsw2:
!Equals [!FindInMap [ClientVpn, Enabled, usw2], true]

ConditionCreateAuthorizationRulesAps1:
!Equals [!FindInMap [ClientVpn, Enabled, aps1], true]

Conents of transform in mapping:

Acm:
DomainName: domainName obscured
ZoneId: zoneId obscured
Enabled:
aps1: false
use1: true
use2: false
usw2: false
Subnet:
Assignment: 2

@kddejong
Copy link
Contributor Author

kddejong commented May 8, 2023

Thank you. Let me look into it. I think this is a problem in v0 but there was an expanded check here in v1 that is highlighting the issue.

@whoDoneItAgain
Copy link
Contributor

whoDoneItAgain commented May 10, 2023

down to a single error code now. E1010

E1010 'EndpointIds' is not one of ['FirewallId', 'ResourceArn']
templates/common/vpc/vpc.yaml:854:7

  NetworkFirewallVpceAz3:
    Type: Custom::FirewallVpceFinder
    Condition: ConditionCreatePublicSubnetsAz3
    Properties:
      ServiceToken:
        Fn::ImportValue: !Sub '${Region}-${VpcName}-lambda-firewall-vpce-finder-fn-arn'
      EndpointIds: !GetAtt NetworkFirewall.EndpointIds
      AvailabilityZone: !GetAtt FirewallSubnet3.AvailabilityZone
  NetworkFirewall:
    Type: AWS::NetworkFirewall::Firewall
    Condition: ConditionCreatePublicSubnets
    Properties:
      FirewallName: !Sub '${Region}-${VpcName}-network-firewall'
      FirewallPolicyArn: !Ref NetworkFirewallPolicy
      VpcId: !Ref VPC
      SubnetMappings:
        - SubnetId: !Ref FirewallSubnet1
        - SubnetId: !Ref FirewallSubnet2
        - !If
          - ConditionAzCount3
          - SubnetId: !Ref FirewallSubnet3
          - !Ref AWS::NoValue
      Description: !Sub '${Region}-${VpcName}-network-firewall'
      Tags:
        - Key: Name
          Value: !Sub '${Region}-${VpcName}-network-firewall'

@kddejong
Copy link
Contributor Author

got that one fixed now. Thanks for all the testing @whoDoneItAgain

@whoDoneItAgain
Copy link
Contributor

yep. no problem. I'll run it against some of my other repos too and see what else I get

@whoDoneItAgain
Copy link
Contributor

whoDoneItAgain commented May 11, 2023

Do you want me to continue putting these here or open new issues for each?

E3510 datetime.date(2012, 10, 17) is not one of ['2008-10-17', '2012-10-17'] (easy enough to fix on my end by quoting the date but v0 doesnt error on this)

  GluePolicy:
    Metadata:
      cfn_nag:
        rules_to_suppress:
          - id: F4
            reason: Scoping will occur before promotion to Qa
    Type: AWS::IAM::Policy
    Condition: ConditionNotDev
    Properties:
      PolicyName: !Sub '${GluePurpose}-policy'
      PolicyDocument:
        Version: 2012-10-17
        Statement:
          - Effect: Allow
            Action:
              - glue:InvokeGlue

E1010 'Value' is not one of ['Id'] - Erroring on both FromPort and ToPort (I suspect this is fixed based on the last one i sent but since that was a custom resource I'm adding this)

  SourceSubnetEgressRule3:
    Type: AWS::EC2::SecurityGroupEgress
    Condition: ConditionDmsSgEgressRule3
    Properties:
      Description: !Sub "DMS Task '${TaskName}' - Source DB Access"
      IpProtocol: tcp
      FromPort: !If
        - ConditionImportedSource
        - !GetAtt ImportedSourceDatabasePortSSM.Value
        - !Ref LocalSourceDbPort
      ToPort: !If
        - ConditionImportedSource
        - !GetAtt ImportedSourceDatabasePortSSM.Value
        - !Ref LocalSourceDbPort
      CidrIp: !Ref SourceDbSubnet3
      GroupId:
        Fn::ImportValue: !Sub '${Region}-${BusinessUnit}-${Environment}-${AppName}-sg-dms-${DMSInstanceNumber}'

E3003 'NoncurrentDays' is a required property

          - Id: non-current-versions
            NoncurrentVersionExpiration:
              NoncurrentDays: !If
                - ConditionExpireNoncurrentVersions
                - !Ref NoncurrentVersionExpiration
                - !Ref AWS::NoValue
            NoncurrentVersionTransitions:
              - StorageClass: INTELLIGENT_TIERING
                TransitionInDays: 0
            Status: Enabled

E2523 Either CIDR Block or IPv4 IPAM Pool and IPv4 Netmask Length must be provided

Parameters:
  VPCCIDR:
    Description: VPC CIDR
    Type: String
    AllowedPattern: ^(?:10\.(19[2-9]|2[0-4]\d|25[0-5])\.([048]|(([1]?)([02468][048]|[13579][26]))|(([2]?)([024][048]|[13][26]))|252)\.0\/22)|(?:auto-22)$
    Default: auto-22

Conditions:
  ConditionAutoAssignVPCCidr: !Equals
    - !Select [0, !Split ['-', !Ref VPCCIDR]]
    - auto

Resources:
  VPC:
    Type: AWS::EC2::VPC
    Properties:
      EnableDnsSupport: true
      EnableDnsHostnames: true
      CidrBlock: !If
        - ConditionAutoAssignVPCCidr
        - !Ref AWS::NoValue
        - !Ref VPCCIDR
      Ipv4IpamPoolId: !If
        - ConditionAutoAssignVPCCidr
        - !FindInMap [IpamPoolsMap, !Ref 'AWS::Region', !Ref BusinessUnit]
        - !Ref AWS::NoValue
      Ipv4NetmaskLength: !If
        - ConditionAutoAssignVPCCidr
        - !Select [1, !Split ['-', !Ref VPCCIDR]]
        - !Ref AWS::NoValue
      Tags:
        - Key: Name
          Value: !Sub '${Region}-${BusinessUnit}-${Environment}-vpc'

@andrew-glenn
Copy link
Contributor

andrew-glenn commented May 30, 2023

I realize it's early - however will there be migration guidance for end-users whom have written custom rules ?

Edited to add: If that hasn't been really considered much yet, I'm happy to collaborate offline

@kddejong
Copy link
Contributor Author

@whoDoneItAgain I think I got most of these items resolved but I did re-work some of how the json schema validation was handled so hopefully I didn't cause any more.

@kddejong
Copy link
Contributor Author

@andrew-glenn absolutely. Not a lot has changed but there are a few things. There are probably some more clever ways to write rules though so should work on documenting that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1 v1.X
Projects
No open projects
1.0.0 Release
  
In progress
Development

Successfully merging a pull request may close this issue.

4 participants