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.
… layers
resource_properties.get("Layers", []) only falls back to [] when the
key is absent. With the lazy Fn::If resolution, Layers: !If [Cond,
[...], !Ref "AWS::NoValue"] now resolves the property to None instead
of leaving the raw intrinsic dict in place, so _parse_layer_info
received None and crashed with TypeError: 'NoneType' object is not
iterable instead of raising a domain-specific error.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
||
| 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, and the or [] patches in sam_function_provider.py only cover two of the affected call sites.
When the selected branch is {"Ref": "AWS::NoValue"}, resolve_symbols routes to IntrinsicsSymbolTable.handle_pseudo_no_value(), which returns Python None. Before this change that None hit the if intrinsic is None: raise InvalidIntrinsicException guard, so handle_fn_if always raised and the enclosing dict branch (with ignore_errors=True) left the raw {"Fn::If": [...]} in place. Now None is returned and stored as the property value:
# intrinsic_property_resolver, dict branch
sanitized_val = self.intrinsic_property_resolver(val, ignore_errors, parent_function=parent_function)
...
sanitized_dict[sanitized_key] = sanitized_val # key kept, value is NoneThe key is retained rather than dropped, which does not match CloudFormation's AWS::NoValue semantics (the property is removed). A concrete crash path is SamApiProvider, which reads Events off the intrinsic-resolved stack.resources:
Events:
ApiEvent: !If [UseApi, {Type: Api, Properties: {Path: /, Method: get}}, !Ref "AWS::NoValue"]With UseApi false this resolves to {"ApiEvent": None}, and sam local start-api then hits samcli/lib/providers/sam_api_provider.py:469:
for , event in serverlessfunction_events.items():
event_type = event.get(self._EVENT_TYPE) # AttributeError: 'NoneType' object has no attribute 'get'Previously that entry stayed as the raw Fn::If dict and was silently skipped. The same shape applies to properties.get("Events", {}) / properties.get(AUTHORIZER_TYPE, "").lower() style lookups elsewhere — a .get(key, default) default never fires when the key is present with a None value, so patching individual consumers with or [] will keep surfacing new failures.
Normalizing at the resolver would fix the whole class at once, and there is already precedent for exactly this in the codebase — CfnLanguageExtensionsApi._remove_no_value in samcli/lib/cfn_language_extensions/api.py:482, whose docstring notes "When Fn::If returns AWS::NoValue, the resolver returns None" and which skips such keys and list items (while deliberately preserving AWS::NoValue inside intrinsic-function arguments).
With lazy Fn::If branch resolution, a selected branch of !Ref AWS::NoValue now resolves directly to None instead of raising, so the generic dict-recursion branch kept the key with a None value. This does not match CloudFormation's AWS::NoValue semantics, where the property is removed entirely, and broke consumers that treat a present-but-None value differently from an absent key (e.g. SamApiProvider indexing into a None event dict, or the earlier Layers fix that only patched two call sites of a wider problem). Skip the key when its resolved value is None. List branches are left untouched since they also resolve intrinsic function argument lists (Fn::Join, Fn::GetAtt, ...), where filtering None could silently shrink a positional argument list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| sanitized_dict[sanitized_key] = sanitized_val | ||
| # A resolved value of None means the property was Fn::If-ed to AWS::NoValue. | ||
| # CloudFormation drops such properties entirely rather than keeping a null value. | ||
| if sanitized_val is not None: |
There was a problem hiding this comment.
[BUG] The AWS::NoValue handling is only applied in the dictionary branch. The list branch of intrinsic_property_resolver (around line 196) is unchanged:
if isinstance(intrinsic, list):
return [self.intrinsic_property_resolver(item, ignore_errors) for item in intrinsic]So a per-element Fn::If that selects AWS::NoValue — a common CloudFormation pattern — now resolves to a list containing a literal None:
Layers:
- !Ref BaseLayer
- !If [UseExtraLayer, !Ref ExtraLayer, !Ref "AWS::NoValue"]Before this PR the Fn::If raised while resolving the unselected AWS::NoValue branch, so with ignore_errors=True the whole Layers property was left raw. After this PR it resolves to [{"Ref": "BaseLayer"}, None]. CloudFormation removes the element from the list, so the resolved value should be a single-element list.
Concrete consequences of the None element:
- SamFunctionProvider._parse_layer_info falls through to the final else and logs
layer "None" is not recognizable, it might be using intrinsic functions that we don't support yet. Skipping.— misleading, since nothing unsupported was used. - List properties consumed positionally get None. For
Architectures: [!If [UseArm, arm64, !Ref "AWS::NoValue"]], Function.architectures becomes[None], and validate_architecture_runtime (samcli/lib/utils/architecture.py:71) raisesUnsupportedRuntimeArchitectureError: Runtime ... is not supported on 'None' architecture. _get_function_architecture (samcli/lib/build/utils.py:51) returns None instead of defaulting to X86_64.
Applying the same rule the new comment states ("CloudFormation drops such properties entirely") to lists keeps the two branches consistent:
if isinstance(intrinsic, list):
resolved_items = [self.intrinsic_property_resolver(item, ignore_errors) for item in intrinsic]
# An item that resolves to None was Fn::If-ed to AWS::NoValue; CloudFormation
# removes the item from the list rather than keeping a null entry.
return [item for item in resolved_items if item is not None]This is safe: a literal null element in a template never reaches the filter, because intrinsic_property_resolver raises InvalidIntrinsicException on None input and the list comprehension has no try/except, so None elements can only originate from AWS::NoValue.
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.