Skip to content

Commit

Permalink
Allow Ref AWS::NoValue in FindInMap parameters (#3399)
Browse files Browse the repository at this point in the history
  • Loading branch information
kddejong authored Jun 25, 2024
1 parent 5d5e6c8 commit ed39a19
Show file tree
Hide file tree
Showing 6 changed files with 142 additions and 40 deletions.
22 changes: 14 additions & 8 deletions src/cfnlint/jsonschema/_filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,18 @@ class FunctionFilter:
init=False,
default_factory=lambda: [
"dependencies",
"if",
"then",
"else",
"required",
"requiredXor",
"requiredAtLeastOne",
"dependentExcluded",
"dependentRequired",
"minItems",
"minProperties",
"else",
"if",
"maxItems",
"maxProperties",
"minItems",
"minProperties",
"required",
"requiredAtLeastOne",
"requiredXor",
"then",
"uniqueItems",
],
)
Expand Down Expand Up @@ -116,6 +116,12 @@ def filter(self, validator: Any, instance: Any, schema: Any):
return
return

# if there are no functions then we don't need to worry
# about ref AWS::NoValue or If conditions
if not validator.context.functions:
yield instance, schema
return

# dependencies, required, minProperties, maxProperties
# need to have filtered properties to validate
# because of Ref: AWS::NoValue
Expand Down
42 changes: 40 additions & 2 deletions src/cfnlint/rules/functions/FindInMap.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,24 +62,62 @@ def schema(self, validator: Validator, instance: Any) -> dict[str, Any]:
scalar_schema,
scalar_schema,
{
"functions": [],
"schema": {
"findinmap_parameters": True,
"type": ["object"],
"properties": {
"DefaultValue": {
"type": ("array",) + singular_types,
"default_value": True,
}
},
"additionalProperties": False,
"required": ["DefaultValue"],
}
},
},
]

return schema

def _default_value(
self, validator: Validator, s: Any, instance: Any, schema: Any
) -> ValidationResult:
validator = validator.evolve(
context=validator.context.evolve(
functions=["Ref"],
),
)
yield from validator.descend(
instance,
{
"type": ("array",) + singular_types,
},
)

def fn_findinmap(
self, validator: Validator, s: Any, instance: Any, schema: Any
) -> ValidationResult:

if validator.context.transforms.has_language_extensions_transform():
# we have to use a special validator for this
# as we don't want DefaultValue: !Ref AWS::NoValue
# is valid
mapping_validator = validator.extend(
validators={
"default_value": self._default_value,
},
)(validator.schema)
validator = mapping_validator.evolve(
context=validator.context.evolve(
resources={},
),
cfn=validator.cfn,
function_filter=validator.function_filter,
resolver=validator.resolver,
)
yield from super().validate(validator, s, instance, schema)
return

validator = validator.evolve(
context=validator.context.evolve(
resources={},
Expand Down
45 changes: 42 additions & 3 deletions test/unit/module/jsonschema/test_filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,68 @@ def filter():


@pytest.mark.parametrize(
"name,instance,schema,path,expected",
"name,instance,schema,path,functions,expected",
[
(
"Don't validate dynamic references inside of function",
"{{resolve:ssm:${AWS::AccountId}/${AWS::Region}/ac}}",
{"enum": "Foo"},
deque(["Foo", "Test", "Fn::Sub"]),
[],
[],
),
(
"Validate dynamic references",
"{{resolve:ssm:secret}}",
{"enum": "Foo"},
deque(["Foo", "Test"]),
[],
[
("{{resolve:ssm:secret}}", {"dynamicReference": {"enum": "Foo"}}),
],
),
(
"Lack of functions returns the schema and instance",
{
"Foo": {"Ref": "AWS::NoValue"},
},
{"required": ["Foo"]},
deque([]),
[],
[
(
{
"Foo": {"Ref": "AWS::NoValue"},
},
{"required": ["Foo"]},
),
],
),
(
"Filtered schemas",
{
"Foo": {"Ref": "AWS::NoValue"},
},
{"required": ["Foo"]},
deque([]),
["Ref", "Fn::If"],
[
(
{},
{"required": ["Foo"]},
),
({"Foo": {"Ref": "AWS::NoValue"}}, {"cfnLint": [""]}),
],
),
],
)
def test_filter(name, instance, schema, path, expected, filter):
def test_filter(name, instance, schema, path, functions, expected, filter):
validator = CfnTemplateValidator(
context=Context(regions=["us-east-1"], path=Path(path)),
context=Context(
regions=["us-east-1"],
path=Path(path),
functions=functions,
),
schema=schema,
)
results = list(filter.filter(validator, instance, schema))
Expand Down
37 changes: 28 additions & 9 deletions test/unit/rules/functions/test_find_in_map.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ def context(cfn):
{"Fn::FindInMap": ["A", "B", "C"]},
{"type": "string"},
{},
[],
None,
[],
),
(
"Invalid Fn::FindInMap too long",
{"Fn::FindInMap": ["foo", "bar", "key", "key2"]},
{"type": "string"},
{},
[],
None,
[
ValidationError(
"['foo', 'bar', 'key', 'key2'] is too long (3)",
Expand All @@ -69,7 +69,7 @@ def context(cfn):
{"Fn::FindInMap": {"foo": "bar"}},
{"type": "string"},
{},
[],
None,
[
ValidationError(
"{'foo': 'bar'} is not of type 'array'",
Expand All @@ -84,7 +84,7 @@ def context(cfn):
{"Fn::FindInMap": [{"Fn::GetAtt": "MyResource.Arn"}, "foo", "bar"]},
{"type": "string"},
{},
[],
None,
[
ValidationError(
"{'Fn::GetAtt': 'MyResource.Arn'} is not of type 'string'",
Expand All @@ -99,15 +99,15 @@ def context(cfn):
{"Fn::FindInMap": ["A", "B", "C", {"DefaultValue": "D"}]},
{"type": "string"},
{"transforms": Transforms(["AWS::LanguageExtensions"])},
[],
None,
[],
),
(
"Invalid Fn::FindInMap options not of type object",
{"Fn::FindInMap": ["A", "B", "C", []]},
{"type": "string"},
{"transforms": Transforms(["AWS::LanguageExtensions"])},
[],
None,
[
ValidationError(
"[] is not of type 'object'",
Expand All @@ -122,7 +122,7 @@ def context(cfn):
{"Fn::FindInMap": ["A", "B", "C", {}]},
{"type": "string"},
{"transforms": Transforms(["AWS::LanguageExtensions"])},
[],
None,
[
ValidationError(
"'DefaultValue' is a required property",
Expand All @@ -147,6 +147,21 @@ def context(cfn):
),
],
),
(
"Valid Fn::FindInMap with a Ref to AWS::NoValue",
{
"Fn::FindInMap": [
"A",
"B",
"C",
{"DefaultValue": {"Ref": "AWS::NoValue"}},
]
},
{"type": "string"},
{"transforms": Transforms(["AWS::LanguageExtensions"])},
[],
[],
),
],
)
def test_validate(
Expand All @@ -162,10 +177,14 @@ def test_validate(
):
context = context.evolve(**context_evolve)
ref_mock = MagicMock()
ref_mock.return_value = iter(ref_mock_values)
ref_mock.return_value = iter(ref_mock_values or [])
validator = CfnTemplateValidator({}).extend(validators={"ref": ref_mock})(
context=context, cfn=cfn
)
errs = list(rule.fn_findinmap(validator, schema, instance, {}))
assert ref_mock.call_count == len(ref_mock_values)

if ref_mock_values is None:
ref_mock.assert_not_called()
else:
assert ref_mock.call_count == len(ref_mock_values) or 1
assert errs == expected, f"Test {name!r} got {errs!r}"
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ def rule():
"At least one essential container is required",
rule=TaskDefinitionEssentialContainer(),
path=deque([]),
validator="minItems",
schema_path=deque(["minItems"]),
validator="contains",
schema_path=deque(["contains"]),
)
],
),
Expand Down
32 changes: 16 additions & 16 deletions test/unit/rules/resources/route53/test_recordsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,22 +341,6 @@ def rule():
],
},
[
ValidationError(
"['cname1.example.com', 'foo√bar'] is too long (1)",
rule=RecordSet(),
path=deque(["ResourceRecords"]),
validator="maxItems",
schema_path=deque(
[
"allOf",
3,
"then",
"properties",
"ResourceRecords",
"maxItems",
]
),
),
ValidationError(
"'foo√bar' is not valid under any of the given schemas",
rule=RecordSet(),
Expand Down Expand Up @@ -398,6 +382,22 @@ def rule():
]
),
),
ValidationError(
"['cname1.example.com', 'foo√bar'] is too long (1)",
rule=RecordSet(),
path=deque(["ResourceRecords"]),
validator="maxItems",
schema_path=deque(
[
"allOf",
3,
"then",
"properties",
"ResourceRecords",
"maxItems",
]
),
),
],
),
(
Expand Down

0 comments on commit ed39a19

Please sign in to comment.