From 236df71da58a8f7e34f0d997f213a0816d16a974 Mon Sep 17 00:00:00 2001 From: Kevin DeJong Date: Fri, 7 Apr 2023 10:54:33 -0700 Subject: [PATCH] V1 - cleaning up more rules (#2670) * Add back in logic for rule W2031 * Validate conditions when checking resource types for a region --- .../resource/configuration.json | 2 +- .../rules/parameters/AllowedPattern.py | 39 ++ src/cfnlint/rules/resources/Configuration.py | 31 +- .../resources/properties/AllowedPattern.py | 10 + src/cfnlint/schema/_constants.py | 33 -- src/cfnlint/schema/getatts.py | 35 +- .../maintenance/test_update_resource_specs.py | 374 ------------------ 7 files changed, 105 insertions(+), 419 deletions(-) delete mode 100644 src/cfnlint/schema/_constants.py delete mode 100644 test/unit/module/maintenance/test_update_resource_specs.py diff --git a/src/cfnlint/data/AdditionalSchemas/resource/configuration.json b/src/cfnlint/data/AdditionalSchemas/resource/configuration.json index a9a907dd27..7dd3bd9b03 100644 --- a/src/cfnlint/data/AdditionalSchemas/resource/configuration.json +++ b/src/cfnlint/data/AdditionalSchemas/resource/configuration.json @@ -1,5 +1,6 @@ { "additionalProperties": false, + "awsType": true, "else": { "not": { "required": [ @@ -55,7 +56,6 @@ "type": "object" }, "Type": { - "awsType": true, "type": "string" }, "UpdatePolicy": { diff --git a/src/cfnlint/rules/parameters/AllowedPattern.py b/src/cfnlint/rules/parameters/AllowedPattern.py index 8fd3809b74..cf54fb198c 100644 --- a/src/cfnlint/rules/parameters/AllowedPattern.py +++ b/src/cfnlint/rules/parameters/AllowedPattern.py @@ -2,8 +2,13 @@ Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. SPDX-License-Identifier: MIT-0 """ +from typing import Generator, List, Union +import regex as re + +from cfnlint.jsonschema import ValidationError from cfnlint.rules import CloudFormationLintRule +from cfnlint.template import Template class AllowedPattern(CloudFormationLintRule): @@ -14,3 +19,37 @@ class AllowedPattern(CloudFormationLintRule): description = "Check if parameters have a valid value in a pattern. The Parameter's allowed pattern is based on the usages in property (Ref)" source_url = "https://github.com/aws-cloudformation/cfn-python-lint/blob/main/docs/cfn-resource-specification.md#allowedpattern" tags = ["parameters", "resources", "property", "allowed pattern"] + + def __init__(self): + super().__init__() + self.parameters = {} + + def initialize(self, cfn: Template): + """Initialize the rule""" + self.parameters = cfn.get_parameters() + + def _pattern( + self, instance: str, patrn: str, path: List[Union[str, int]] + ) -> Generator[ValidationError, None, None]: + if not re.search(patrn, instance): + yield ValidationError( + f"{instance!r} does not match {patrn!r}", + rule=self, + path_override=path, + ) + + def validate(self, ref: str, patrn: str): + p = self.parameters.get(ref, {}) + if isinstance(p, dict): + p_default = p.get("Default", None) + if isinstance(p_default, (str)): + yield from self._pattern( + p_default, patrn, ["Parameters", ref, "Default"] + ) + + p_avs = p.get("AllowedValues", []) + if isinstance(p_avs, list): + for p_av_index, p_av in enumerate(p_avs): + yield from self._pattern( + p_av, patrn, ["Parameters", ref, "AllowedValues", p_av_index] + ) diff --git a/src/cfnlint/rules/resources/Configuration.py b/src/cfnlint/rules/resources/Configuration.py index 20a4624968..7f9d8cc54a 100644 --- a/src/cfnlint/rules/resources/Configuration.py +++ b/src/cfnlint/rules/resources/Configuration.py @@ -32,27 +32,40 @@ def __init__(self): cfn=None, rules=None, )(schema=schema) + self.cfn = None def initialize(self, cfn): super().initialize(cfn) self.regions = cfn.regions + self.cfn = cfn # pylint: disable=unused-argument def _awsType(self, validator, iT, instance, schema): - if not validator.is_type(instance, "string"): + resource_type = instance.get("Type") + if not validator.is_type(resource_type, "string"): return + + resource_condition = instance.get("Condition") + for region in self.regions: - if instance in PROVIDER_SCHEMA_MANAGER.get_resource_types(region=region): - return - if not instance.startswith( + if validator.is_type(resource_condition, "string"): + if False in self.cfn.conditions.build_scenerios_on_region( + resource_condition, region + ): + continue + if resource_type in PROVIDER_SCHEMA_MANAGER.get_resource_types( + region=region + ): + continue + if not resource_type.startswith( ("Custom::", "AWS::Serverless::") - ) and not instance.endswith("::MODULE"): + ) and not resource_type.endswith("::MODULE"): yield ValidationError( - f"Resource type `{instance}` does not exist in '{region}'" + f"Resource type `{resource_type}` does not exist in '{region}'" ) # pylint: disable=unused-argument - def _check_resource(self, cfn, resource_name, resource_values): + def _check_resource(self, resource_name, resource_values): """Check Resource""" matches = [] @@ -97,8 +110,6 @@ def match(self, cfn): self.logger.debug( "Validating resource %s base configuration", resource_name ) - matches.extend( - self._check_resource(cfn, resource_name, resource_values) - ) + matches.extend(self._check_resource(resource_name, resource_values)) return matches diff --git a/src/cfnlint/rules/resources/properties/AllowedPattern.py b/src/cfnlint/rules/resources/properties/AllowedPattern.py index e51961f1e6..a9c73faf70 100644 --- a/src/cfnlint/rules/resources/properties/AllowedPattern.py +++ b/src/cfnlint/rules/resources/properties/AllowedPattern.py @@ -17,9 +17,19 @@ class AllowedPattern(CloudFormationLintRule): description = "Check if properties have a valid value in case of a pattern (Regular Expression)" source_url = "https://github.com/awslabs/cfn-python-lint/blob/main/docs/cfn-resource-specification.md#allowedpattern" tags = ["resources", "property", "allowed pattern", "regex"] + child_rules = { + "W2031": None, + } # pylint: disable=unused-argument def pattern(self, validator, patrn, instance, schema): + if isinstance(instance, dict): + if len(instance) == 1: + for k, v in instance.items(): + if k == "Ref": + if self.child_rules.get("W2031"): + yield from self.child_rules["W2031"].validate(v, patrn) + return if validator.is_type(instance, "string"): # skip any dynamic reference strings if REGEX_DYN_REF.findall(instance): diff --git a/src/cfnlint/schema/_constants.py b/src/cfnlint/schema/_constants.py deleted file mode 100644 index b67c66194d..0000000000 --- a/src/cfnlint/schema/_constants.py +++ /dev/null @@ -1,33 +0,0 @@ -_all_property_types = [ - "AWS::Amplify::Branch", - "AWS::Amplify::Domain", - "AWS::AppSync::DomainName", - "AWS::Backup::BackupSelection", - "AWS::Backup::BackupVault", - "AWS::CodeArtifact::Domain", - "AWS::CodeArtifact::Repository", - "AWS::EC2::CapacityReservation", - "AWS::EC2::Subnet", - "AWS::EC2::VPC", - "AWS::EFS::MountTarget", - "AWS::EKS::Nodegroup", - "AWS::ImageBuilder::Component", - "AWS::ImageBuilder::ContainerRecipe", - "AWS::ImageBuilder::DistributionConfiguration", - "AWS::ImageBuilder::ImagePipeline", - "AWS::ImageBuilder::ImageRecipe", - "AWS::ImageBuilder::InfrastructureConfiguration", - "AWS::RDS::DBParameterGroup", - "AWS::RoboMaker::RobotApplication", - "AWS::RoboMaker::SimulationApplication", - "AWS::Route53Resolver::ResolverRule", - "AWS::Route53Resolver::ResolverRuleAssociation", - "AWS::SNS::Topic", - "AWS::SQS::Queue", - "AWS::SageMaker::DataQualityJobDefinition", - "AWS::SageMaker::ModelBiasJobDefinition", - "AWS::SageMaker::ModelExplainabilityJobDefinition", - "AWS::SageMaker::ModelQualityJobDefinition", - "AWS::SageMaker::MonitoringSchedule", - "AWS::StepFunctions::Activity", -] diff --git a/src/cfnlint/schema/getatts.py b/src/cfnlint/schema/getatts.py index 16ac3d083b..59eca38f11 100644 --- a/src/cfnlint/schema/getatts.py +++ b/src/cfnlint/schema/getatts.py @@ -5,9 +5,42 @@ import enum from typing import Any, Dict, Optional -from cfnlint.schema._constants import _all_property_types from cfnlint.schema._pointer import resolve_pointer +_all_property_types = [ + "AWS::Amplify::Branch", + "AWS::Amplify::Domain", + "AWS::AppSync::DomainName", + "AWS::Backup::BackupSelection", + "AWS::Backup::BackupVault", + "AWS::CodeArtifact::Domain", + "AWS::CodeArtifact::Repository", + "AWS::EC2::CapacityReservation", + "AWS::EC2::Subnet", + "AWS::EC2::VPC", + "AWS::EFS::MountTarget", + "AWS::EKS::Nodegroup", + "AWS::ImageBuilder::Component", + "AWS::ImageBuilder::ContainerRecipe", + "AWS::ImageBuilder::DistributionConfiguration", + "AWS::ImageBuilder::ImagePipeline", + "AWS::ImageBuilder::ImageRecipe", + "AWS::ImageBuilder::InfrastructureConfiguration", + "AWS::RDS::DBParameterGroup", + "AWS::RoboMaker::RobotApplication", + "AWS::RoboMaker::SimulationApplication", + "AWS::Route53Resolver::ResolverRule", + "AWS::Route53Resolver::ResolverRuleAssociation", + "AWS::SNS::Topic", + "AWS::SQS::Queue", + "AWS::SageMaker::DataQualityJobDefinition", + "AWS::SageMaker::ModelBiasJobDefinition", + "AWS::SageMaker::ModelExplainabilityJobDefinition", + "AWS::SageMaker::ModelQualityJobDefinition", + "AWS::SageMaker::MonitoringSchedule", + "AWS::StepFunctions::Activity", +] + class GetAttType(enum.Enum): ReadOnly = 1 diff --git a/test/unit/module/maintenance/test_update_resource_specs.py b/test/unit/module/maintenance/test_update_resource_specs.py deleted file mode 100644 index 1f2b8177e2..0000000000 --- a/test/unit/module/maintenance/test_update_resource_specs.py +++ /dev/null @@ -1,374 +0,0 @@ -""" -Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -SPDX-License-Identifier: MIT-0 -""" -import logging -import sys -from test.testlib.testcase import BaseTestCase -from unittest.mock import ANY, MagicMock, Mock, call, patch - -import cfnlint.maintenance - -LOGGER = logging.getLogger("cfnlint.maintenance") -LOGGER.addHandler(logging.NullHandler()) - - -def patch_spec_sideffect(content, region, patch_types="ExtendedSpecs"): - if region == "all" and patch_types == "ExtendedSpecs": - return { - "PropertyTypes": { - "AWS::Lambda::CodeSigningConfig.AllowedPublishers": { - "Properties": { - "SigningProfileVersionArns": {}, - } - }, - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies": { - "Properties": { - "UntrustedArtifactOnDeployment": {}, - } - }, - }, - "ResourceTypes": { - "AWS::Lambda::CodeSigningConfig": { - "Attributes": { - "CodeSigningConfigArn": { - "PrimitiveType": "String", - }, - "CodeSigningConfigId": { - "PrimitiveType": "String", - }, - }, - "Properties": { - "AllowedPublishers": {}, - "CodeSigningPolicies": {}, - "Description": {}, - }, - } - }, - "ValueTypes": {}, - } - if region in ["us-east-1", "us-west-2"] and patch_types == "ExtendedSpecs": - return { - "PropertyTypes": { - "AWS::Lambda::CodeSigningConfig.AllowedPublishers": { - "Properties": { - "SigningProfileVersionArns": {}, - } - }, - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies": { - "Properties": { - "UntrustedArtifactOnDeployment": {}, - } - }, - }, - "ResourceTypes": { - "AWS::Lambda::CodeSigningConfig": { - "Attributes": { - "CodeSigningConfigArn": { - "PrimitiveType": "String", - }, - "CodeSigningConfigId": { - "PrimitiveType": "String", - }, - }, - "Properties": { - "AllowedPublishers": {}, - "CodeSigningPolicies": {}, - "Description": {}, - }, - } - }, - "ValueTypes": { - "AWS::EC2::Instance.Types": ["m2.medium"], - }, - } - - return content - - -class TestUpdateResourceSpecs(BaseTestCase): - """Used for Testing Resource Specs""" - - @patch("cfnlint.maintenance.url_has_newer_version") - @patch("cfnlint.maintenance.get_url_content") - @patch("cfnlint.maintenance.json.dump") - @patch("cfnlint.maintenance.patch_spec") - @patch("cfnlint.maintenance.SPEC_REGIONS", {"us-east-1": "http://foo.badurl"}) - @patch("cfnlint.maintenance.urlopen") - def test_update_resource_spec( - self, - mock_urlopen, - mock_patch_spec, - mock_json_dump, - mock_content, - mock_url_newer_version, - ): - """Success update resource spec""" - - mock_url_newer_version.return_value = True - mock_content.return_value = '{"PropertyTypes": {}, "ResourceTypes": {}}' - mock_patch_spec.side_effect = patch_spec_sideffect - - cm = MagicMock() - cm.getcode.return_value = 200 - cm.__enter__.return_value = cm - - with open("test/fixtures/registry/schema.zip", "rb") as f: - byte = f.read() - cm.read.return_value = byte - - mock_urlopen.return_value = cm - schema_cache = cfnlint.maintenance.get_schema_value_types() - - builtin_module_name = "builtins" - - with patch("{}.open".format(builtin_module_name)) as mock_builtin_open: - cfnlint.maintenance.update_resource_spec( - "us-east-1", "http://foo.badurl", schema_cache - ) - mock_json_dump.assert_called_with( - { - "PropertyTypes": { - "AWS::Lambda::CodeSigningConfig.AllowedPublishers": { - "Properties": { - "SigningProfileVersionArns": { - "Value": { - "ValueType": "AWS::Lambda::CodeSigningConfig.AllowedPublishers.SigningProfileVersionArns", - } - }, - } - }, - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies": { - "Properties": { - "UntrustedArtifactOnDeployment": { - "Value": { - "ValueType": "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies.UntrustedArtifactOnDeployment", - } - }, - } - }, - }, - "ResourceTypes": { - "AWS::Lambda::CodeSigningConfig": { - "Attributes": { - "CodeSigningConfigArn": { - "PrimitiveType": "String", - }, - "CodeSigningConfigId": { - "PrimitiveType": "String", - }, - }, - "Properties": { - "AllowedPublishers": {}, - "CodeSigningPolicies": {}, - "Description": {}, - }, - } - }, - "ValueTypes": { - "AWS::EC2::Instance.Types": ["m2.medium"], - "AWS::Lambda::CodeSigningConfig.AllowedPublishers.SigningProfileVersionArns": { - "AllowedPatternRegex": "arn:(aws[a-zA-Z0-9-]*):([a-zA-Z0-9\\-])+:([a-z]{2}(-gov)?-[a-z]+-\\d{1})?:(\\d{12})?:(.*)", - "StringMin": 12, - "StringMax": 1024, - }, - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies.UntrustedArtifactOnDeployment": { - "AllowedValues": ["Warn", "Enforce"] - }, - }, - }, - mock_builtin_open.return_value.__enter__.return_value, - indent=1, - separators=(",", ": "), - sort_keys=True, - ) - - @patch("cfnlint.maintenance.url_has_newer_version") - @patch("cfnlint.maintenance.get_url_content") - @patch("cfnlint.maintenance.json.dump") - @patch("cfnlint.maintenance.patch_spec") - @patch("cfnlint.maintenance.SPEC_REGIONS", {"us-east-1": "http://foo.badurl"}) - @patch("cfnlint.maintenance.urlopen") - @patch("cfnlint.maintenance.cfnlint.helpers.load_resource") - def test_update_resource_spec_cache( - self, - mock_load_resource, - mock_urlopen, - mock_patch_spec, - mock_json_dump, - mock_content, - mock_url_newer_version, - ): - """Success update resource spec with cache""" - - mock_url_newer_version.return_value = True - mock_content.return_value = '{"PropertyTypes": {}, "ResourceTypes": {}}' - mock_patch_spec.side_effect = patch_spec_sideffect - - cm = MagicMock() - cm.getcode.return_value = 200 - cm.__enter__.return_value = cm - - with open("test/fixtures/registry/schema.zip", "rb") as f: - byte = f.read() - cm.read.return_value = byte - - mock_urlopen.return_value = cm - schema_cache = cfnlint.maintenance.get_schema_value_types() - - builtin_module_name = "builtins" - - mock_load_resource.return_value = { - "PropertyTypes": { - "AWS::Lambda::CodeSigningConfig.AllowedPublishers": { - "Properties": { - "SigningProfileVersionArns": { - "Value": { - "ValueType": "AWS::Lambda::CodeSigningConfig.AllowedPublishers.SigningProfileVersionArns", - } - }, - } - }, - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies": { - "Properties": { - "UntrustedArtifactOnDeployment": { - "Value": { - "ValueType": "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies.UntrustedArtifactOnDeployment", - } - }, - } - }, - }, - "ResourceTypes": { - "AWS::Lambda::CodeSigningConfig": { - "Attributes": { - "CodeSigningConfigArn": { - "PrimitiveType": "String", - }, - "CodeSigningConfigId": { - "PrimitiveType": "String", - }, - }, - "Properties": { - "AllowedPublishers": {}, - "CodeSigningPolicies": {}, - "Description": {}, - }, - } - }, - "ValueTypes": { - "AWS::EC2::Instance.Types": [ - "m2.medium", - "m2.large", - ], - "AWS::Lambda::CodeSigningConfig.AllowedPublishers.SigningProfileVersionArns": { - "AllowedPatternRegex": "arn:(aws[a-zA-Z0-9-]*):([a-zA-Z0-9\\-])+:([a-z]{2}(-gov)?-[a-z]+-\\d{1})?:(\\d{12})?:(.*)", - "StringMin": 12, - "StringMax": 1024, - }, - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies.UntrustedArtifactOnDeployment": { - "AllowedValues": ["Warn", "Enforce"] - }, - }, - } - - with patch("{}.open".format(builtin_module_name)) as mock_builtin_open: - cfnlint.maintenance.update_resource_spec( - "us-west-2", "http://foo.badurl", schema_cache - ) - mock_json_dump.assert_called_with( - { - "PropertyTypes": { - "AWS::Lambda::CodeSigningConfig.AllowedPublishers": "CACHED", - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies": "CACHED", - }, - "ResourceTypes": { - "AWS::Lambda::CodeSigningConfig": "CACHED", - }, - "ValueTypes": { - # does not match above - "AWS::EC2::Instance.Types": [ - "m2.medium", - ], - "AWS::Lambda::CodeSigningConfig.AllowedPublishers.SigningProfileVersionArns": "CACHED", - "AWS::Lambda::CodeSigningConfig.CodeSigningPolicies.UntrustedArtifactOnDeployment": "CACHED", - }, - }, - mock_builtin_open.return_value.__enter__.return_value, - indent=1, - separators=(",", ": "), - sort_keys=True, - ) - - @patch("cfnlint.maintenance.url_has_newer_version") - @patch("cfnlint.maintenance.get_url_content") - @patch("cfnlint.maintenance.json.dump") - @patch("cfnlint.maintenance.patch_spec") - @patch("cfnlint.maintenance.SPEC_REGIONS", {"us-east-1": "http://foo.badurl"}) - def test_do_not_update_resource_spec( - self, mock_patch_spec, mock_json_dump, mock_content, mock_url_newer_version - ): - """Success update resource spec""" - - mock_url_newer_version.return_value = False - - result = cfnlint.maintenance.update_resource_spec( - "us-east-1", "http://foo.badurl", ANY - ) - self.assertIsNone(result) - mock_content.assert_not_called() - mock_patch_spec.assert_not_called() - mock_json_dump.assert_not_called() - - @patch("cfnlint.maintenance.url_has_newer_version") - @patch("cfnlint.maintenance.get_url_content") - @patch("cfnlint.maintenance.json.dump") - @patch("cfnlint.maintenance.patch_spec") - @patch("cfnlint.maintenance.SPEC_REGIONS", {"us-east-1": "http://foo.badurl"}) - @patch("cfnlint.maintenance.urlopen") - def test_update_resource_spec_force( - self, - mock_urlopen, - mock_patch_spec, - mock_json_dump, - mock_content, - mock_url_newer_version, - ): - """Success update resource spec""" - - mock_url_newer_version.return_value = False - mock_content.return_value = '{"PropertyTypes": {}, "ResourceTypes": {}}' - mock_patch_spec.side_effect = patch_spec_sideffect - - mock_urlresponse = Mock() - cm = MagicMock() - cm.getcode.return_value = 200 - cm.__enter__.return_value = cm - - with open("test/fixtures/registry/schema.zip", "rb") as f: - byte = f.read() - cm.read.return_value = byte - - mock_urlopen.return_value = cm - schema_cache = cfnlint.maintenance.get_schema_value_types() - - builtin_module_name = "builtins" - - with patch("{}.open".format(builtin_module_name)) as mock_builtin_open: - cfnlint.maintenance.update_resource_spec( - "us-east-1", "http://foo.badurl", schema_cache, True - ) - mock_content.assert_called_once() - mock_patch_spec.assert_called() - mock_json_dump.assert_called_once() - - @patch("cfnlint.maintenance.multiprocessing.Pool") - @patch("cfnlint.maintenance.update_resource_spec") - @patch("cfnlint.maintenance.SPEC_REGIONS", {"us-east-1": "http://foo.badurl"}) - def test_update_resource_specs_python_3(self, mock_update_resource_spec, mock_pool): - fake_pool = MagicMock() - mock_pool.return_value.__enter__.return_value = fake_pool - - cfnlint.maintenance.update_resource_specs() - - fake_pool.starmap.assert_called_once()