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?

[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

[ashemedai]

Alright, then afterwards I need to create a PR to fix the rule to do the right thing with regard to DeleteAfterDays and MoveToColdStorageAfterDays according to the documentation. The current logic I cannot verify based on the CloudFormation documentation.

[kddejong]

I think this rule is based on the docs. Let me know what you find out or submit a PR. I'll be happy to review and thanks for the feedback.