cfn-lint v1

[kddejong]

We are close to releasing v1. This discussion thread is to handle any questions that may come up related to that conversion. If you have an issue with v1 you can submit an issue.

[ashemedai]

Hey @kddejong,

I haven't kept up to date with this, but I see some validators have methods that are not called validate and this can cause exceptions. Is it the goal to rename those method names from whatever to validate (based on a quick look at other files)? Or is this thinking too simplistic?

[kddejong]

Its all good... its been changing a bit. Are you writing your own rule or is this a rule on my side? The names can be technically be named whatever. I've been centralizing on just using validate to make it easier for me to follow. For our more dynamic cfn-lint rules it was just easier to commonize on validate.

Here is a decent break down:

• The base json schema validators are all defined here
• The base validator for the entire template is defined here. Since we are associating rules to validators you will see that we basically populate the self.validators with those rules. Those rules can either be used to assign a validator to a rule so we can have a rule ID or they can override the validator and provide a function of the name of the validator. Example see rule E3012
• Then additional we add a keyword (cfnLint) based on the path in the template. This allows to write rules like E3615. We can provide a generic path in the keywords and provide an additional schema for doing validation. This allows us to extend the base schemas without patching them. You can extend by using a schema file or by using your own validation logic like rule E2531
• The last group is the intrinsic function rules which modifies the schema as we find them. These are validated using the dynamic names with naming standard fn_sub. See rule E1019

[kddejong]

Additionally I've been working on adding tests to lower the cfn-lint risk of making this mistake as I've made it a few times.

[ashemedai]

In this particular case I was testing out v1 against a template I had written a week ago and it failed in the existing rule BackupPlanLifecycleRule.py with an exception of not being able to find validate().

Can be reproduced with the template from here and then you will see:

E0002 Unknown exception while processing rule E1001: "'BackupPlanLifecycleRule' object has no attribute 'validate'"
backup.yaml:1:1

So my naive assumption as this point was to change that file to something like this (happy to make it a PR once I know I am on the right track):

"""
Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
SPDX-License-Identifier: MIT-0
"""

from collections import deque
from typing import Any

from cfnlint.jsonschema import ValidationError, Validator
from cfnlint.rules.jsonschema.CfnLintKeyword import CfnLintKeyword


class BackupPlanLifecycleRule(CfnLintKeyword):
    """
    Check Backup Plan rules with lifecycle has greater
    period for delete than cold
    """

    id = "E3504"
    shortdesc = "Check greater delete period than cold is met for BackupPlan"
    description = (
        "Check that Backup plans with lifecycle rules have a greater delete"
        " period than cold"
    )
    source_url = "https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-backup-backupplan-lifecycleresourcetype.html"
    tags = ["properties", "backup", "plan", "lifecycle"]

    def __init__(self) -> None:
        super().__init__(
            [
                "Resources/AWS::Backup::BackupPlan/Properties/BackupPlan/BackupPlanRule/*/Lifecycle"
            ]
        )

    # pylint: disable=unused-argument
    def validate(self, validator: Validator, uI: Any, instance: Any, schema: Any):
        delete_after_days = instance.get("DeleteAfterDays")
        move_to_cold_storage_after_days = instance.get("MoveToColdStorageAfterDays")

        if not validator.is_type(delete_after_days, "integer"):
            return
        if not validator.is_type(move_to_cold_storage_after_days, "integer"):
            return
        if delete_after_days <= move_to_cold_storage_after_days:
            yield ValidationError(
                (
                    f"DeleteAfterDays ({delete_after_days}) must be greater "
                    "than the value of MoveToColdStorageAfterDays "
                    f"({move_to_cold_storage_after_days})"
                ),
                path=deque(["DeleteAfterDays"]),
                rule=self,
            )

That at least brings me to:

% cfn-lint backup.yaml
W1020 'Fn::Sub' isn't needed because there are no variables
backup.yaml:33:9

W3005 Obsolete DependsOn on resource (BackupPlanWithThinBackups), dependency already enforced by a "Ref" at Resources/TagBasedBackupSelection/Properties/BackupPlanId/Ref
backup.yaml:40:5

This also changes the logic for the bottom test to reflect what the documentation states about DeleteAfterDays and MoveToColdStorageAfterDays.

However, this doesn't trigger for Lifecycle underneath CopyActions, which I thought the * would achieve as part of the super().__init__(). Try with this change to the template, for example:

        BackupPlanRule:
        - RuleName: "RuleForDailyBackups"
          TargetBackupVault: Default
          ScheduleExpression: "cron(25 11 ? * * *)"
          Lifecycle:
            DeleteAfterDays: a
            MoveToColdStorageAfterDays: b
          CopyActions:
          - DestinationBackupVaultArn: arn:aws:backup:us-west-2:111222333444:backup-vault:Default
            Lifecycle:
              DeleteAfterDays: 14
              MoveToColdStorageAfterDays: 40
        - RuleName: "RuleForDailyBackupsMoving"
          TargetBackupVault: Default
          ScheduleExpression: "cron(25 11 ? * * *)"
          Lifecycle:
            DeleteAfterDays: 7
            MoveToColdStorageAfterDays: 45

And you will get:

% cfn-lint backup.yaml
E3012 'a' is not of type 'number'
backup.yaml:19:13

E3012 'b' is not of type 'number'
backup.yaml:20:13

E3504 DeleteAfterDays (7) must be greater than the value of MoveToColdStorageAfterDays (45)
backup.yaml:30:13

Nowhere are we getting results for the CopyActions/Lifecycle.

[kddejong]

This would be the change. I'm going to work on tests for this issue too.

def __init__(self) -> None:
        super().__init__(
            [
                "Resources/AWS::Backup::BackupPlan/Properties/BackupPlan/BackupPlanRule/*/Lifecycle",
                "Resources/AWS::Backup::BackupPlan/Properties/BackupPlan/BackupPlanRule/*/CopyActions/*/Lifecycle",
            ]
        )

    def validate(self, validator, uI, instance, schema):

[kddejong]

This PR should add testing and fix what was the only error that it found. #3266