fix(intrinsics): only resolve the selected Fn::If branch - #9134
Conversation
Fn::If eagerly resolved both the true and false branches before checking the condition, so an unresolvable value in the branch NOT selected (e.g. Fn::GetAtt to FunctionUrl, which sam local invoke can't resolve) caused the whole intrinsic to be left unresolved even though the condition picked the other, resolvable branch. Fixes aws#4510, Fixes aws#6205
handle_fn_getatt called resolve_symbols without forwarding its own ignore_errors argument, so resolve_symbols always defaulted to False. Any Fn::GetAtt referencing an attribute the local resolver doesn't support (e.g. FunctionUrl) raised even when the caller explicitly asked to ignore errors, instead of falling back to a placeholder.
…le branch Reproduces the aws#4510 scenario end-to-end via sam local invoke: an environment variable that uses Fn::If where the selected branch is a resolvable Ref and the other branch is Fn::GetAtt to a FunctionUrl attribute, which the local resolver can't handle.
github-actions
Bot
added
pr/external
stage/needs-triage
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| verify_intrinsic_type_str(resource_type, IntrinsicResolver.FN_GET_ATT) | ||
|
|
||
| return self._symbol_resolver.resolve_symbols(logical_id, resource_type) | ||
| return self._symbol_resolver.resolve_symbols(logical_id, resource_type, ignore_errors) |
There was a problem hiding this comment.
[BUG] Forwarding ignore_errors into resolve_symbols changes behavior for every unresolvable Fn::GetAtt in the template, not just ones inside a selected Fn::If branch, and it regresses nested-stack layer handling.
SamBaseProvider.get_template / get_resolved_template_dict both call resolver.resolve_template(ignore_errors=True), so this is the only mode sam build and sam local ever run in. Previously an unresolvable GetAtt raised InvalidSymbolException, which the dict-recursion handler in intrinsic_property_resolver (and resolve_attribute) caught and used to leave the property as the raw intrinsic dict. Now it silently becomes the string "$LogicalId.Attribute", and downstream code that distinguishes "unresolved intrinsic dict" from "resolved string" takes the wrong path.
Concretely, for a layer referencing a nested stack output — the exact shape nested_stack_manager.py emits and that SamFunctionProvider._locate_layer_from_nested documents:
Layers:
- !GetAtt AwsSamAutoDependencyLayerNestedStack.Outputs.MyDepLayerresolve_symbols("AwsSamAutoDependencyLayerNestedStack", "Outputs.MyDepLayer") has no match in logical_id_translator, parameters, default_type_resolver, or common_attribute_resolver (which only handles Ref and Arn). Before this change the entry stayed a dict and _parse_layer_info fell into its else branch, logging layer "..." is not recognizable ... Skipping. After this change the entry is the string "$AwsSamAutoDependencyLayerNestedStack.Outputs.MyDepLayer", so isinstance(layer, str) is true, locate_layer_nested is False for build and local invoke, and it is passed to LayerVersion(layer, None, ...). _compute_layer_name / _compute_layer_version then fail to rsplit an ARN out of it and raise InvalidLayerVersionArn — a hard failure where the layer used to be skipped.
This change also isn't required for the Fn::If fix. If the selected branch contains an unsupported Fn::GetAtt, the exception already propagates out of handle_fn_if to the enclosing dict recursion, which catches it under ignore_errors=True and preserves the original value. Suggest reverting this line to keep the blast radius limited to handle_fn_if:
return self._symbol_resolver.resolve_symbols(logical_id, resource_type)If the placeholder degradation is genuinely wanted, it needs to be scoped so it can't turn an unresolved layer reference into something _parse_layer_info mistakes for a literal ARN, plus a test covering Layers: [!GetAtt Stack.Outputs.Layer]. Note that tests/unit/.../test_intrinsic_resolver.py:471 currently locks in the new placeholder behavior, so this would need updating too.
… resolution fix(intrinsics): stop forwarding ignore_errors into Fn::GetAtt symbol resolution handle_fn_getatt forwarded ignore_errors to resolve_symbols, but resolve_template is always called with ignore_errors=True in sam build and sam local invoke. This made every unresolvable Fn::GetAtt in the template degrade to a .Attribute placeholder string instead of raising, which used to be caught by the dict-recursion handler and left the raw intrinsic dict in place. This broke layers referencing nested stack outputs (the shape nested_stack_manager.py emits): the placeholder string satisfied isinstance(layer, str) in _parse_layer_info and was treated as a literal ARN, raising InvalidLayerVersionArn where the layer used to be safely skipped. The Fn::If fix doesn't need this: when the selected branch is unresolvable, the exception already propagates out of handle_fn_if and is caught by the enclosing dict recursion under ignore_errors=True. Reverts the ignore_errors forwarding from 16c70d0 and its placeholder-locking test; adds a regression test covering the nested stack layer scenario.
…PNOS/aws-sam-cli into fix/4510-lazy-fn-if-resolution
|
|
||
| return value_if_true if condition_evaluated else value_if_false | ||
| selected_value = intrinsic_value[1] if condition_evaluated else intrinsic_value[2] | ||
| return self.intrinsic_property_resolver(selected_value, ignore_errors, parent_function=IntrinsicResolver.FN_IF) |
There was a problem hiding this comment.
[BUG] handle_fn_if can now return None, which it could never do before, and at least one downstream consumer is not null-safe.
Before this change, a branch containing !Ref AWS::NoValue was pre-resolved to None by the outer intrinsic_property_resolver(intrinsic_value, ...) call, and the subsequent resolve(arguments[1|2]) hit the if intrinsic is None: raise InvalidIntrinsicException guard at the top of intrinsic_property_resolver. So Fn::If always raised, and with ignore_errors=True the enclosing dict loop left the property as the raw {"Fn::If": [...]} dict.
Now the selected branch is resolved directly, so !Ref AWS::NoValue reaches IntrinsicsSymbolTable.handle_pseudo_no_value() and None is returned and assigned as the property value (the generic dict branch does sanitized_dict[sanitized_key] = sanitized_val with no None filtering).
Concrete failure — the common "conditionally omit a property" idiom:
Properties:
Layers: !If [UseLayers, [!Ref MyLayer], !Ref "AWS::NoValue"]When UseLayers is false, Properties["Layers"] becomes None. In samcli/lib/providers/sam_function_provider.py:265, resource_properties.get("Layers", []) returns None (the default only applies when the key is absent), and _parse_layer_info then does for layer in list_of_layers → TypeError: 'NoneType' object is not iterable, an unhandled traceback instead of a domain error.
Note the element-level form Layers: [!If [Cond, !Ref MyLayer, !Ref "AWS::NoValue"]] is fine — it yields [None] and _parse_layer_info skips unrecognized entries. Only the whole-property form breaks, and that is exactly one of the scenarios this PR sets out to fix, so it is worth closing here rather than leaving it as a newly reachable crash.
Two options:
- Make the resolver match CloudFormation's
AWS::NoValuesemantics by dropping keys whose resolved value isNonein the generic dict branch ofintrinsic_property_resolver. This is the semantically correct fix but has wider blast radius, so it needs its own tests. - Harden the consumer, e.g.
resource_properties.get("Layers") or []in both call sites insam_function_provider.py.
Either way, please add a unit test covering an Fn::If whose selected branch is !Ref AWS::NoValue — the four new tests only cover unresolvable Fn::GetAtt in the unselected branch, so this path is currently untested.
Note on the previous review comment: the handle_fn_getatt change that forwarded ignore_errors into resolve_symbols is no longer in the diff, and test_template_ignore_errors_leaves_unresolvable_layer_getatt_as_dict was added as a guard for the nested-stack layer behavior. That finding is resolved and I did not re-raise it. The PR description still describes the handle_fn_getatt change, so it is now out of date.
1 participant
Which issue(s) does this change fix?
Fixes #4510, Fixes #6205
Why is this change necessary?
Fn::Ifeagerly resolved both the true and false branches before checkingthe condition. If the branch that the condition did NOT select contained a
value the local resolver can't handle (e.g.
Fn::GetAttto aFunctionUrlattribute, or
!Ref AWS::NoValuein aLayerslist), resolution of theentire
Fn::Iffailed even though the condition picked the other,perfectly resolvable branch.
sam deploy/sam buildwere unaffected sinceCloudFormation itself only evaluates the selected branch.
How does it address the issue?
handle_fn_ifnow evaluates the condition first and only recursivelyresolves the branch it selects, instead of resolving both branches
unconditionally.
handle_fn_getattnow forwards its ownignore_errorsargument toresolve_symbols, which it previously dropped. This matters once theFn::Iffix is in place: if the selected branch itself contains anunsupported
Fn::GetAtt,ignore_errors=Trueshould still let itdegrade to a placeholder instead of raising.
What side effects does this change have?
None expected. Both branches were already required to independently
type-check before this change; only the eager resolution of the unselected
branch's value is removed.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.