feat: Add CloudFormation Language Extensions support (Fn::ForEach)#8637
feat: Add CloudFormation Language Extensions support (Fn::ForEach)#8637
Conversation
0be94d0 to
5d6cbf3
Compare
|
Integration test: https://github.com/aws/aws-sam-cli/actions/runs/21808626015 |
e68efa0 to
4ed8396
Compare
5324dad to
707baad
Compare
8caac65 to
a42f5f0
Compare
a42f5f0 to
64665ff
Compare
5aa92bc to
c83b1a3
Compare
Implement a local CloudFormation Language Extensions processor supporting: - Fn::ForEach loop expansion in Resources, Conditions, and Outputs - Fn::Length, Fn::ToJsonString intrinsic functions - Fn::FindInMap with DefaultValue support - Conditional DeletionPolicy/UpdateReplacePolicy - Nested ForEach depth validation (max 5 levels) - Partial resolution mode preserving unresolvable references Pipeline architecture: TemplateParsingProcessor -> ForEachProcessor -> IntrinsicResolverProcessor -> DeletionPolicyProcessor -> UpdateReplacePolicyProcessor Includes comprehensive unit tests and CloudFormation compatibility suite.
Wire the language extensions library into SAM CLI with two-phase architecture: - Phase 1: expand_language_extensions() -> LanguageExtensionResult - Phase 2: SamTranslatorWrapper.run_plugins() (SAM transform only) Key components: - expand_language_extensions() canonical entry point - SamTranslatorWrapper receives pre-expanded template (Phase 2 only) - SamLocalStackProvider.get_stacks() calls expand_language_extensions() - SamTemplateValidator calls expand_language_extensions() - DynamicArtifactProperty dataclass for Mappings transformation - Fn::ForEach guards in artifact_exporter, normalizer, cdk/utils
- _get_template_for_output() preserves Fn::ForEach in build output - _update_foreach_artifact_paths() generates Mappings for dynamic artifact properties with per-function build paths - Recursive nested Fn::ForEach support - ForEach-aware path resolution skips Docker image URIs Test templates: static CodeUri, dynamic CodeUri, parameter collections, nested stacks, nested ForEach, dynamic ImageUri, depth validation.
Package:
- _export() calls expand_language_extensions() for Phase 1
- Preserves Fn::ForEach in packaged template with S3 URIs
- Generates Mappings for dynamic artifact properties
- _find_artifact_uri_for_resource() handles all export formats:
string, {S3Bucket,S3Key}, {Bucket,Key}, {ImageUri}
- Recursive nested Fn::ForEach support
- Warning for parameter-based collections
Deploy:
- Uploads original unexpanded template to CloudFormation
- Clear error for missing Mapping keys
Integration tests for CodeUri, ContentUri, DefinitionUri, ImageUri,
BodyS3Location across all packageable resource types.
dd84085 to
a1100e9
Compare
- sam validate: valid ForEach, invalid syntax, cloud-dependent collections, dynamic CodeUri, nested depth validation (5 valid, 6 invalid) - sam local invoke: expanded function names from ForEach - sam local start-api: ForEach-generated API endpoints
a1100e9 to
6b4513b
Compare
Track CFNLanguageExtensions as a UsedFeature event when templates with AWS::LanguageExtensions transform are expanded. Emitted once per expansion in expand_language_extensions().
reedham-aws
left a comment
There was a problem hiding this comment.
This is my review of the first commit that adds all the language extension support in its own library. I didn't review the tests, but I really don't think that's feasible to review due to the sheer size.
| Note: The actual loop expansion logic is implemented in task 12.2. | ||
| This task (12.1) focuses on detection, validation, and collection resolution. | ||
|
|
||
| Requirements: | ||
| - 6.7: WHEN Fn::ForEach has invalid layout (wrong number of arguments, | ||
| invalid types), THEN THE Resolver SHALL raise an Invalid_Template_Exception | ||
| - 6.8: WHEN Fn::ForEach collection contains a Ref to a parameter, | ||
| THEN THE Resolver SHALL resolve the parameter value before iteration | ||
| - 18.2: WHEN a template contains 5 or fewer levels of nested Fn::ForEach loops, | ||
| THE SAM_CLI SHALL process the template successfully | ||
| - 18.3: WHEN a template contains more than 5 levels of nested Fn::ForEach loops, | ||
| THE SAM_CLI SHALL raise an error before processing |
There was a problem hiding this comment.
I'm going to leave one comment here but as a general issue I don't think we should be having task identifiers in the code like this. They're not particularly useful, especially because those docs aren't even committed.
| Example: | ||
| >>> processor = ForEachProcessor() | ||
| >>> context = TemplateProcessingContext( | ||
| ... fragment={ | ||
| ... "Resources": { | ||
| ... "Fn::ForEach::Topics": [ | ||
| ... "TopicName", | ||
| ... ["Alerts", "Notifications"], | ||
| ... {"Topic${TopicName}": {"Type": "AWS::SNS::Topic"}} | ||
| ... ] | ||
| ... } | ||
| ... } | ||
| ... ) | ||
| >>> processor.process_template(context) |
There was a problem hiding this comment.
I guess I'm fine having these doctest strings, but we don't actually use them for anything so they're a little clunky. We don't do this anywhere else in the code.
| @staticmethod | ||
| def _merge_expanded( |
There was a problem hiding this comment.
Style nit: I think having this as @staticmethod conflicts with the name starting with _.
|
|
||
| return result | ||
|
|
||
| def _process_section(self, section: Any, context: TemplateProcessingContext, section_name: str) -> Any: |
There was a problem hiding this comment.
It seems to me like _process_section and _process_properties are exactly the same. The section_name parameter isn't even used here, so I feel like we could merge the methods and start using it to differentiate.
| # Resolve and validate identifier | ||
| identifiers = self._resolve_identifiers(identifier, context) | ||
| if not identifiers: | ||
| raise InvalidTemplateException(f"{key} layout is incorrect") |
There was a problem hiding this comment.
We use the same hardcoded string several times in this method, probably better to have it in one place and use a variable.
| # Format error message like Kotlin: "Found circular condition dependency between X and Y" | ||
| if len(cycle) >= 2: | ||
| raise InvalidTemplateException( | ||
| f"Found circular condition dependency between {cycle[0]} and {cycle[1]}" |
There was a problem hiding this comment.
Do we not show anything for something in the list greater than index 1?
| try: | ||
| self._resolver.resolve_value({"Condition": condition_name}) | ||
| except InvalidTemplateException: | ||
| raise |
There was a problem hiding this comment.
try/except not necessary if you're going to raise the exception with no processing.
|
|
||
| # Check each condition for cycles | ||
| for condition_name in conditions: | ||
| visited.clear() |
There was a problem hiding this comment.
I don't think visited needs to be cleared here, would we not end up rechecking the same nodes?
| Example: | ||
| >>> template = {"Resources": {"MyQueue": {"Type": "AWS::SQS::Queue"}}} | ||
| >>> json_str = serialize_to_json(template) | ||
| >>> print(json_str) | ||
| { | ||
| "Resources": { | ||
| "MyQueue": { | ||
| "Type": "AWS::SQS::Queue" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Requirements: | ||
| - 13.1: Provide function to serialize to JSON format | ||
| - 13.3: Produce valid JSON that can be parsed back |
There was a problem hiding this comment.
Don't need all this documentation to wrap json.dumps
There was a problem hiding this comment.
Probably same for yaml down below too.
| def is_foreach_key(key: str) -> bool: | ||
| """Check if a resource key is a Fn::ForEach block.""" | ||
| return isinstance(key, str) and key.startswith(FOREACH_PREFIX) |
There was a problem hiding this comment.
This method is duplicate to one in processors/foreach.py
reedham-aws
left a comment
There was a problem hiding this comment.
My review of the second commit; once again, I did not review any tests.
| return template_dict | ||
|
|
||
|
|
||
| def _update_foreach_relative_paths(foreach_value, original_root, new_root): |
There was a problem hiding this comment.
Most of the code in this method is a direct copy of _update_relative_paths. I get that we need to do some preprocessing, but let's simplify so we're not copying code like this.
| resource_type = resource_def.get("Type") | ||
| if resource_type in packageable_resources.keys(): | ||
| # Flatten list of locations per resource type | ||
| locations = list(itertools.chain(*packageable_resources.get(resource_type))) | ||
| for location in locations: | ||
| properties = resource_def.get("Properties", {}) | ||
| # Search for package-able location within resource properties | ||
| if jmespath.search(location, properties): | ||
| artifacts.append(properties.get("PackageType", ZIP)) |
There was a problem hiding this comment.
This block is copied from get_template_artifacts_format. Lets break it into a helper.
| # Return the ForEach key as a placeholder - the actual function IDs | ||
| # will be determined after language extensions are processed | ||
| _function_resource_ids.append(resource_id) |
There was a problem hiding this comment.
I'm sure it does, but without looking further when does this happen? I want to make sure we're not forgetting this.
|
|
||
| from samcli.commands._utils.experimental import ExperimentalFlag, is_experimental_enabled | ||
| from samcli.commands.package import exceptions | ||
| from samcli.lib.cfn_language_extensions.utils import iter_resources |
There was a problem hiding this comment.
I think this is a good method to use, but it seems like it was kind of unevenly applied. For example, in samcli/lib/package/artifact_exporter.py, it's not really used at all when it could be. There might be other places where it can be used that I haven't seen yet.
I also think the name is not descriptive enough to show that it's skipping foreach blocks.
| return None | ||
|
|
||
|
|
||
| def contains_loop_variable(value: Any, loop_variable: str) -> bool: |
There was a problem hiding this comment.
nit: I was confused by loop_variable here. In other places in the library (eg. the _expand_foreach method in foreach.py) this is referred to as the identifier. I think they both make sense, and I don't think it's worth changing, just wanted to call that out.
| from samcli.lib.cfn_language_extensions.sam_integration import expand_language_extensions | ||
| from samcli.lib.intrinsic_resolver.intrinsics_symbol_table import IntrinsicsSymbolTable |
There was a problem hiding this comment.
Should be at top level
| DynamicArtifactProperty, | ||
| ) | ||
|
|
||
| foreach_required_elements = 3 |
There was a problem hiding this comment.
We have this as a global in samcli/commands/_utils/template.py. I think it makes more sense to be defined as a global in this file.
| @@ -62,6 +63,9 @@ def normalize(template_dict, normalize_parameters=False): | |||
| resources = template_dict.get(RESOURCES_KEY, {}) | |||
|
|
|||
| for logical_id, resource in resources.items(): | |||
There was a problem hiding this comment.
I won't comment on more, but here is a time we could use iter_resources.
|
|
||
| @staticmethod | ||
| def _check_using_language_extension(template: Dict) -> bool: | ||
| def _check_using_language_extension(template: Optional[Dict]) -> bool: |
There was a problem hiding this comment.
Instead of changing this method to wrap something from the library, lets just use that method instead.
| Update all other member that also depends on the stacks. | ||
| This should be called whenever there is a change to the template. | ||
| """ | ||
| from samcli.lib.cfn_language_extensions.sam_integration import clear_expansion_cache |
There was a problem hiding this comment.
I don't know what the exact amount of integration tests that would be good for this feature would be, but I feel like this is too many. I think our testing strategy should be more about testing the library itself; if that works, then builds should work too. A basic case wouldn't hurt, but I think that we risk really bloating our test suites here. Very much open to the opinions of others.
|
I've also noticed there's not a lot of logging in this PR. I think that would be good to have in some places, but it would be difficult to add all at once. |
Description
This PR adds support for CloudFormation Language Extensions in SAM CLI, addressing GitHub issue #5647.
Features
Fn::Ifin resource policiesKey Design Decisions
Fn::ForEachblocks with dynamic artifact properties (e.g.,CodeUri: ./src/${Name}) are supported via a Mappings transformationFn::ForEachcollections must be resolvable locally; cloud-dependent values (Fn::GetAtt,Fn::ImportValue) are not supported with clear error messagesSupported Commands
sam build- Builds all expanded functions, preserves original templatesam package- PreservesFn::ForEachstructure with S3 URIssam deploy- Uploads original template for CloudFormation to processsam validate- Validates language extension syntaxsam local invoke- Invokes expanded functions by namesam local start-api- Serves ForEach-generated API endpointssam local start-lambda- Serves all expanded functionsExample
Resolves #5647
Testing
Checklist