From 1c2e4639901cd259d8073d8b367dc9dfb9dc1d0c Mon Sep 17 00:00:00 2001 From: Kevin DeJong Date: Thu, 18 Jul 2024 09:48:30 -0700 Subject: [PATCH] Update condition implies fn --- src/cfnlint/conditions/conditions.py | 20 ++-- src/cfnlint/rules/__init__.py | 14 +-- .../rules/jsonschema/CfnLintRelationship.py | 105 ++++++++++-------- src/cfnlint/template/template.py | 2 +- test/fixtures/templates/good/generic.yaml.dot | 0 .../jsonschema/test_cfn_lint_relationship.py | 33 +++--- 6 files changed, 89 insertions(+), 85 deletions(-) delete mode 100644 test/fixtures/templates/good/generic.yaml.dot diff --git a/src/cfnlint/conditions/conditions.py b/src/cfnlint/conditions/conditions.py index 6afef36441..7dfe25f138 100644 --- a/src/cfnlint/conditions/conditions.py +++ b/src/cfnlint/conditions/conditions.py @@ -277,20 +277,16 @@ def implies(self, scenarios: dict[str, bool], implies: str) -> bool: and_condition = And(*conditions) - # if the implies condition has to be true already then we don't - # need to imply it - if not scenarios.get(implies): - implies_condition = self._conditions[implies].build_true_cnf( - self._solver_params - ) - cnf.add_prop(Not(Implies(and_condition, implies_condition))) - else: - cnf.add_prop(and_condition) + implies_condition = self._conditions[implies].build_true_cnf( + self._solver_params + ) + cnf.add_prop(Not(Implies(and_condition, implies_condition))) - if satisfiable(cnf): - return True + results = satisfiable(cnf) + if results: + return False - return False + return True except KeyError: # KeyError is because the listed condition doesn't exist because of bad # formatting or just the wrong condition name diff --git a/src/cfnlint/rules/__init__.py b/src/cfnlint/rules/__init__.py index d8cc5f4336..5eb87f653c 100644 --- a/src/cfnlint/rules/__init__.py +++ b/src/cfnlint/rules/__init__.py @@ -5,12 +5,8 @@ from cfnlint.rules._rule import CloudFormationLintRule, Match, RuleMatch from cfnlint.rules._rules import Rules, RulesCollection - -# type: ignore -from cfnlint.rules.jsonschema import ( - CfnLintJsonSchema, - CfnLintJsonSchemaRegional, - CfnLintKeyword, - CfnLintRelationship, - SchemaDetails, -) +from cfnlint.rules.jsonschema import CfnLintJsonSchema # type: ignore +from cfnlint.rules.jsonschema import CfnLintJsonSchemaRegional # type: ignore +from cfnlint.rules.jsonschema import CfnLintKeyword # type: ignore +from cfnlint.rules.jsonschema import CfnLintRelationship # type: ignore +from cfnlint.rules.jsonschema import SchemaDetails diff --git a/src/cfnlint/rules/jsonschema/CfnLintRelationship.py b/src/cfnlint/rules/jsonschema/CfnLintRelationship.py index 7b393687f7..ee28ad0887 100644 --- a/src/cfnlint/rules/jsonschema/CfnLintRelationship.py +++ b/src/cfnlint/rules/jsonschema/CfnLintRelationship.py @@ -22,42 +22,59 @@ def __init__( self.relationship = relationship or "" self.parent_rules = ["E1101"] - def _get_relationship( - self, validator: Validator, instance: Any, path: deque[str | int] + def _get_relationship_fn_if( + self, validator: Validator, key: Any, value: Any, path: deque[str | int] ) -> Iterator[tuple[Any, Validator]]: - fn_k, fn_v = is_function(instance) - if fn_k == "Fn::If": - if not isinstance(fn_v, list) or len(fn_v) != 3: - return - condition = fn_v[0] - # True path - status = { - k: set([v]) for k, v in validator.context.conditions.status.items() - } - if condition not in status: - status[condition] = set([True, False]) - for scenario in validator.cfn.conditions.build_scenarios(status): - if_validator = validator.evolve( - context=validator.context.evolve( - conditions=validator.context.conditions.evolve( - status=scenario, - ) + if not isinstance(value, list) or len(value) != 3: + return + condition = value[0] + # True path + status = {k: set([v]) for k, v in validator.context.conditions.status.items()} + if condition not in status: + status[condition] = set([True, False]) + for scenario in validator.cfn.conditions.build_scenarios(status): + if_validator = validator.evolve( + context=validator.context.evolve( + conditions=validator.context.conditions.evolve( + status=scenario, ) ) + ) + + i = 1 if scenario[condition] else 2 + for r, v in self._get_relationship( + validator.evolve( + context=if_validator.context.evolve( + path=if_validator.context.path.descend(path=key).descend(path=i) + ) + ), + value[i], + path.copy(), + ): + yield r, v - i = 1 if scenario[condition] else 2 + def _get_relationship_list( + self, validator: Validator, key: Any, instance: Any, path: deque[str | int] + ) -> Iterator[tuple[Any, Validator]]: + if key == "*": + for i, v in enumerate(instance): for r, v in self._get_relationship( validator.evolve( - context=if_validator.context.evolve( - path=if_validator.context.path.descend(path=fn_k).descend( - path=i - ) - ) + context=validator.context.evolve( + path=validator.context.path.descend(path=i) + ), ), - fn_v[i], + v, path.copy(), ): yield r, v + + def _get_relationship( + self, validator: Validator, instance: Any, path: deque[str | int] + ) -> Iterator[tuple[Any, Validator]]: + fn_k, fn_v = is_function(instance) + if fn_k == "Fn::If": + yield from self._get_relationship_fn_if(validator, fn_k, fn_v, path) return if fn_k == "Ref" and fn_v == "AWS::NoValue": @@ -74,18 +91,7 @@ def _get_relationship( key = path.popleft() if isinstance(instance, list): - if key == "*": - for i, v in enumerate(instance): - for r, v in self._get_relationship( - validator.evolve( - context=validator.context.evolve( - path=validator.context.path.descend(path=i) - ), - ), - v, - path.copy(), - ): - yield r, v + yield from self._get_relationship_list(validator, key, instance, path) return if not isinstance(instance, dict): @@ -102,22 +108,31 @@ def _get_relationship( ): yield r, v - def get_relationship(self, validator: Validator) -> Iterator[tuple[Any, Validator]]: + def _validate_get_relationship_parameters(self, validator: Validator) -> bool: if len(validator.context.path.path) < 2: - return + return False if "Resources" != validator.context.path.path[0]: + return False + + if not validator.cfn.graph: + return False + + if len(self.relationship.split("/")) < 2: + return False + + return True + + def get_relationship(self, validator: Validator) -> Iterator[tuple[Any, Validator]]: + if not self._validate_get_relationship_parameters(validator): return resource_name = validator.context.path.path[1] path = self.relationship.split("/") - if len(path) < 2: - return + # get related resources by ref/getatt to the source # and relationship types - if not validator.cfn.graph: - return - for edge in validator.cfn.graph.graph.out_edges(resource_name): + for edge in validator.cfn.graph.graph.out_edges(resource_name): # type: ignore other_resource = validator.cfn.template.get(path[0], {}).get(edge[1], {}) if other_resource.get("Type") != path[1]: diff --git a/src/cfnlint/template/template.py b/src/cfnlint/template/template.py index 138a87cddb..3fa1a73433 100644 --- a/src/cfnlint/template/template.py +++ b/src/cfnlint/template/template.py @@ -824,7 +824,7 @@ def is_resource_available(self, path: Path, resource: str) -> list[dict[str, boo return results scenario[condition_name] = list(condition_bool)[0] - if self.conditions.implies(scenario, resource_condition): + if not self.conditions.implies(scenario, resource_condition): return [{**{resource_condition: False}, **scenario}] # if resource condition isn't available then the resource is available diff --git a/test/fixtures/templates/good/generic.yaml.dot b/test/fixtures/templates/good/generic.yaml.dot deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/test/unit/rules/jsonschema/test_cfn_lint_relationship.py b/test/unit/rules/jsonschema/test_cfn_lint_relationship.py index fcbda4cf6d..6606bbf10d 100644 --- a/test/unit/rules/jsonschema/test_cfn_lint_relationship.py +++ b/test/unit/rules/jsonschema/test_cfn_lint_relationship.py @@ -66,11 +66,11 @@ def template(): }, "Three": { "Type": "AWS::EC2::Instance", - "Condition": "IsUsEast1", + "Condition": "IsImageIdSpecified", "Properties": { "ImageId": { "Fn::If": [ - "IsImageIdSpecified", + "IsUsEast1", {"Ref": "ImageId"}, {"Ref": "AWS::NoValue"}, ], @@ -102,7 +102,6 @@ def template(): }, "Five": { "Type": "AWS::EC2::Instance", - "Condition": "IsUsEast1", "Properties": { "ImageId": { "Fn::If": [ @@ -114,17 +113,14 @@ def template(): }, "ParentFive": { "Type": "AWS::EC2::Instance", - "Condition": "IsImageIdSpecified", "Properties": {"ImageId": {"Fn::GetAtt": ["Five", "ImageId"]}}, }, "Six": { "Type": "AWS::EC2::Instance", - "Condition": "IsUsEast1", "Properties": "Foo", }, "ParentSix": { "Type": "AWS::EC2::Instance", - "Condition": "IsImageIdSpecified", "Properties": {"ImageId": {"Fn::GetAtt": ["Six", "ImageId"]}}, }, "Seven": { @@ -168,7 +164,7 @@ def template(): "name,path,status,expected", [ ( - "One", + "Condition for value", deque(["Resources", "ParentOne", "Properties", "ImageId"]), {}, [ @@ -177,48 +173,49 @@ def template(): ], ), ( - "Two", + "Standard", deque(["Resources", "ParentTwo", "Properties", "ImageId"]), {}, [({"Ref": "ImageId"}, {})], ), ( - "Three", + "Lots of conditions along the way", deque(["Resources", "ParentThree", "Properties", "ImageId"]), { "IsImageIdSpecified": True, }, - [({"Ref": "ImageId"}, {"IsUsEast1": True, "IsImageIdSpecified": True})], + [ + ({"Ref": "ImageId"}, {"IsUsEast1": True, "IsImageIdSpecified": True}), + (None, {"IsUsEast1": False, "IsImageIdSpecified": True}), + ], ), ( - "Four", + "Unrelated conditions", deque(["Resources", "ParentFour", "Properties", "ImageId"]), { "IsImageIdSpecified": True, }, - [ - ({"Ref": "ImageId"}, {"IsUsEast1": True, "IsImageIdSpecified": True}), - ], + [], ), ( - "Five", + "Destiniation Fn::If isn't properly formatted", deque(["Resources", "ParentFive", "Properties", "ImageId"]), {}, [], ), ( - "Six", + "Properties isn't an object", deque(["Resources", "ParentSix", "Properties", "ImageId"]), {}, [], ), ( - "Seven", + "Condition's don't align", deque(["Resources", "ParentSeven", "Properties", "ImageId"]), { "IsUsEast1": True, }, - [()], + [], ), ( "Short Path",