RFC 0009: Add looping functionality in CFN Templates

[MalikAtalla-AWS]

trafficstars

Language Enhancement Request For Comment

This is a request for comments about a new intrinsic function called Fn::Map to support a looping functionality in CloudFormation templates. See the tracking issue (#9) for additional details.

---

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[benkehoe]

I have a number of different pieces of feedback.

Naming

In the issue I originally opened (#41), I proposed Fn::Map; I believe it's important for users to understand this as a functional concept, rather than an imperative one. They aren't writing a for loop, they are mapping a JSON template over a set of values. I'd settle for Fn::Foreach, though. I believe there's also value in a Fn::Reduce (for example, to create a list of ARNs from a collection of resources created with this function), which supports Fn::Map as a name.

Syntax

Index/value parameter names

I don't like the need to define the index and value parameter names. They're probably always going to be the same, it's just one more thing to type out and one more thing to go wrong. Could they be made optional, defaulting to some useful names? This brings me to my next point...

Function parameter ordering

There are so many parameters to this function. I understand that all other intrinsic functions use positional parameters, but maybe this is a place to use named parameters. For example:

!Foreach
  Input: !Ref AmiIds
  Value:
    Type: AWS::EC2::Instance
    Properties:
      InstanceType: "m1.small"
      ImageId: !Ref x
  LogicalId: !Sub Instance${i}
  ParameterNames: ["i", "x"]

This would allow for multiple independently-optional parameters, like LogicalId and ParameterNames. Speaking of LogicalId...

Use cases

I think there's actually four use cases here.

1. Inserting a list as a value
2. Inserting an object as a value
3. Merging an object into a parent
4. Creating a set of CloudFormation resources

Use cases 3 and 4 are both accomplished by the LogicalId parameter. Use case 2 is not supported.

Here's I'd propose:

• Instead of LogicalId and its current semantics, allow a Key parameter that, if set, means the function produces an object rather than a list. By default, this object is the value where the function is defined.
• Allow a boolean Merge parameter that requires the Key parameter also be set, that when true, merges the object defined by the function with its parent.
• Define a separate syntax for resources. Here's why...

Reifying resource mapping

If this intrinsic function simply allows users to create multiple resources with an intrinsic function that shuffles YAML around, we're missing a giant opportunity. We should be enabling CloudFormation to represent a collection of resources defined by mapping over some values as a first-class concept. Say that I want subnets in every AZ. That's an architectural intention that should be fully represented in the cloud. It should get a name, it should be able to inspected, etc.

This proposal doesn't serve that purpose. I think it can be a separate RFC, but imagine something like

Resources:
  Subnets:
    Type: CloudFormation::Map
    Input: [...]
    LogicalId: ... # actual logical id for the resource
    ResourceValue: ...
    # etc, similar syntax to the function

If we agree that such a thing should exist, it will help focus the syntax of this intrinsic function on use cases 1-3 (where use case 3 serves to allow users to create a non-reified set of resources).

[michaelbrewer]

Fn::Map or at worst Fn::Foreach make a-lot more sense then just Fn::For.

[cloudyparts]

Naming

---

Issue #9 describes Fn::For as;

Fn::For traverses a single given List of Strings from first to last, and over each iteration, it references each item under a given template snippet to be replicated.

This definition describes an iterator which implies functions like Map, Reduce, Filter, and Zip.

The term For may confuse developers since this term has a long history as A control-flow-statement.

I agree with @benkehow Fn::Map communicates the intent better than Fn::For.

[jlhood]

Really appreciate all the feedback and engagement on this RFC! After some internal discussions, we've determined it needs significant rework. We're going to iterate a bit internally and then post a new revision for more feedback. Stay tuned!

[MalikAtalla-AWS]

Hey all, I have posted a new revision addressing some of the feedback. This is a summary of the changes:

• We are changing the name of the function to Fn::Map
• We are making the parameters named, instead of positional. We figured that is warranted given the number of parameters, some of them being optional and how that impacts readability.
• The parameters for the index and value variables are now optional. We think customizing them is an important use case, but in most cases, users will be fine with the defaults.
• The LogicalId parameter is now simply called "key"
• We're introducing a new intrinsic function called Fn::Merge, which can be used to merge the result of Fn::Map into its parent object.

[MalikAtalla-AWS]

Really appreciate your feedback @benbridts and @benkehoe

[benkehoe]

re-posting this as a general comment so it doesn't get lost on an old revision:

from @benbridts

Things I'm wondering:

• Would it be clearer to use IndexParameter and ValueParameter for the keys (to make it clear that you're configuring something you will !Ref later)
• Would it be clearer to use Item instead of Value? This might be related to the languages I've been exposed to, but I'd use Key-Value or Index-Item, not using Value prevents confusion with the Key parameter

I think "key" and "value" if they are both used should be outputs, currently "fragment" is used for the output value. We've got four parameters here:

• input value from the input collection (currently "value")
• index of the input value (currently "index")
• output value (currently "fragment")
• optional output key (currently "key")

Maybe making them more explicit makes sense? InputIndex, InputValue, OutputValue, OutputKey? It would leave open the possibility that in the future the collection could be an object, which would would then involve an InputKey parameter as well.

[benkehoe]

Re-posting this as a general comment so it doesn't get lost on an old revision:

Both @benbridts and I commented that we think the parameters should be CamelCase to be consistent with CloudFormation conventions

[benkehoe]

@MalikAtalla-AWS FYI amending the commit and pushing means comments on previous commits are no longer visible in the diff, I'd recommend creating new commits and only squashing when it's ready for merging

[benbridts]

I think "key" and "value" if they are both used should be outputs, currently "fragment" is used for the output value. We've got four parameters here:

• input value from the input collection (currently "value")
• index of the input value (currently "index")
• output value (currently "fragment")
• optional output key (currently "key")

(and collection, but that one seems to be clear)

Maybe making them more explicit makes sense? InputIndex, InputValue, OutputValue, OutputKey? It would leave open the possibility that in the future the collection could be an object, which would would then involve an InputKey parameter as well.

the current value and index do not feel like inputs to me. They are more meta-inputs (used within the fragment) than inputs (used by the map function).

Since the current default for Value is X, I would propose something like element, item or variable.

I'm not the biggest fan of using "Output" in the names for fragment and key, but if you translates what happens to python, it makes sense to use Key and Value in the names. And even though output is a fitting name, they are also templates. (template already has a specific meaning in CloudFormation, but I don't think this has to be a problem)

def cfn_map(collection: List, key: str, fragment: str):
    return {key.format(i=i, x=x): fragment.format(i=i, x=x) for i, x in enumerate(collection)}
    
 # or with different names
 
 def cfn_map(collection: List, key: str, value: str):
    return {key.format(i=i, x=x): value.format(i=i, x=x) for i, x in enumerate(collection)}

This leads me to thinking something like

• IndexName (how you will refer to the index)
• VariableName, ItemName or ElementName (how you will refer to the variable)
• Collection (the actual collection you are mapping over)
• KeyTemplate or OutputKey (the key you will output, that can also reference variables)
• ValueTemplate or OutputValue (the value/fragment you will output, that can also reference variables)

[arthurboghossian]

Update: The AWS CloudFormation team really appreciates the interest and feedback we've received on this RFC! In the past months, our team did a deep dive to better understand specific customer pain points and brainstormed ideas with customers and community representatives. Based on this deep dive, we are updating this RFC with a proposal to add a single Fn::ForEach intrinsic function. Let us know your feedback and thoughts!

[benkehoe]

Needed in the FAQ, because people will try this immediately:

• Can I use an object for the collection instead of a list?
• Can I output a list instead of merge into an object?

[rhbecker]

Are there outstanding concerns preventing the assigned reviewers from approving this?

I'm looking forward to taking advantage of this capability, and am hoping it can soon move towards implementation.

[arthurboghossian]

Thanks for the callout @rhbecker!

Believe there aren't any outstanding concerns, so I'll re-request reviews and will merge the pull request if I receive the necessary approvals.

[rhbecker]

Sorry to be a pest, but if I'm reading the tracking issue #9 correctly, in order to keep things moving, I think someone needs to add an approved label to this PR, and a status/final-comments-period label to the tracking issue?

[MalikAtalla-AWS]

You're right @rhbecker. Appreciate the reminder. We'll take care of that.

[rhbecker]

🤔 Maybe instead of pestering you about labels (which still don't seem quite right on this or the tracking issue), I should instead ask whether implementation is fully unblocked.

My fear is that this issue is stuck in some queue, due to a "paperwork" blockage, when it could otherwise be ready for some hungry CFN engineer to start working on.

If it's already unblocked, or perhaps even already in progress, I can sit back, relax, and patiently (🤞) await completion.

[MalikAtalla-AWS]

Hey @rhbecker, thanks for following up. This issue is and has been unblocked. We have set the status of the RFC to "implementing" in the main readme of this repo and I (just now) added the approved-label to this PR. (Maybe we should consider removing that line about the approved label from our readme. The info in the main readme and the fact that the PR is merged is probably indication enough about the status.)