Fn::FindInMap enhancements: default value and additional intrinsic function support

[mingxingong]

Language Enhancement Request For Comment

This is a request for comments about FindInMap enhancements (adding a default value and support for additional intrinsic functions. See #101 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.

[ohshazbot]

This RFC looks great. It provides defaulting (an ask of mine in #43 (comment)). And the added function support within the FindInMap call is some needed icing on the cake.

Lgtm

[benbridts]

I think we should be explicit about which functions are supported here. Currently one of the example uses Fn::Split, but that one is not in this list. This is made explicit at the end

A note about current and future functions provided by the transform could be useful too

[lejiati]

Good point, I think we should be more explicit about what functions are supported under what conditions. Maybe a dedicated section/table listing out all of them(functions and some exceptions such as the Fn::Select one) would help?

[benbridts]

Is the plan to both support the MappingPresent function and a new !FindInMap function signature?

From the details section it seem like only the !FindInMap way will be included (which is fine)

In either case, I think a definition/explanation of !FindInMap [Mapping, TopLevel, SecondLevel, Default] would be useful. This is included at the end

Another way to solve the same problem - with different trade-offs - is to have a special key in the mapping itself:

Mappings:
  DNS:
    us:
      dns: mypage.us.com
      ttl: '600'
    ca:
      dns: mypage.us.com
      ttl: '600'
    $default:
      dns: mypage.us.com
      ttl: '600'

# This automatically gets the default, so it's less explicit about what's happening
!FindInMap [DNS, !Ref country, dns]

# It could be made more explicit if needed (could be the only supported way too)
# This option could support multiple user-provided $-keys to get the best from both worlds
!FindInMap [DNS, !Ref country, dns, $default]  

For the described use case I think it's more useful to have only one place to define the default, otherwise, if you need to get the value more than one, the more maintainable way of writing it would be something like

Mappings:
  DNS:
    us:
      dns: mypage.us.com
      ttl: '600'
    ca:
      dns: mypage.us.com
      ttl: '600'
  Defaults:
    DNS:
      dns: mypage.default.com
      ttl: 300

!FindInMap [DNS, !Ref country, dns, !FindInMap [Defaults, DNS, dns]]
!FindInMap [DNS, !Ref country, ttl, !FindInMap [Defaults, DNS, ttl]]

or

Mappings:
  DNS:
    us:
      dns: mypage.us.com
      ttl: '600'
    ca:
      dns: mypage.us.com
      ttl: '600'
    default:
      dns: mypage.default.com
      ttl: 300

!FindInMap [DNS, !Ref country, dns, !FindInMap [DNS, default, dns]]
!FindInMap [DNS, !Ref country, ttl, !FindInMap [DNS, default, ttl]]

[lejiati]

That's an interesting idea.

Mappings:
  DNS:
    us:
      dns: mypage.us.com
      ttl: '600'
    ca:
      dns: mypage.us.com
      ttl: '600'
  Defaults:
    DNS:
      dns: mypage.default.com
      ttl: 300

Given the fact we would like to support more intrinsic functions in !FindInMap,

!FindInMap [DNS, !Ref country, dns, !FindInMap [Defaults, DNS, dns]]
!FindInMap [DNS, !Ref country, ttl, !FindInMap [Defaults, DNS, ttl]]

Should be a valid use case.

As for setting a special key in mapping as default key - I'm a bit concerned as it may break customers who already use it as a regular key.

Initially during our internal discussion, we thought adding an optional parameter makes it easy for customers to onboard this feature - you don't need to refactor your FindInMap function, just append the argument list then you are good to go. But now I'm thinking, maybe MappingPresent can be used without the context of FindInMap default value: it's a conditional function so it can alter the behavior of CFN engine, e.g provision a resource conditionally. Yet I don't have any concrete use cases on top of my head to support how useful this new MappingPresent function would be.

[benbridts]

I didn't check, but I seem to remember some values being not allowed in mapping keys, so depending on the list of values there can be a way to introduce special keys without breaking existing templates

Edit: the documentation isn't very clear. On the one hand it says

Within a mapping, each map is a key followed by another mapping. The key must be a map of name-value pairs and unique within the mapping. The name can contain only alphanumeric characters (A-Za-z0-9).

However in the examples us-east-1 is used too. There is a potential to find out the exact allowed pattern by calling aws cloudformation validate-template, but having the documentation be updated is probably better

[lejiati]

Hi Ben, sorry for the late reply.

Another way to solve the same problem - with different trade-offs - is to have a special key in the mapping itself

So I did some investigation on reserving a special key as default. But I decided to rule it out because:

1. IMHO, a reserved key is less flexible compare to adding an optional DefaultValue parameter. Users sometimes may need to define different default values for the same mapping under different circumstances. With $default key defined in mapping section, they are stuck with the pre-defined value.
2. It’s hard to define what exactly $default would contain: should it be a TopLevelKey or SecondLevelKey? To maintain extensibility, define $default as a SecondLevelKey without TopLevelKey seems make more sense but it would be confusing to users because $default does not maintain the same structure as other entries.
3. This enhancement is delivered via AWS::LanguageExtensions, so the response of our transform has to be a legit CloudFormation template. If we reserve a Key without special characters, we run into risk where users may have already define that key. If we do reserve a key with special characters, we need to remove $default section before returning template. This, imo is counter intuitive and does not provide the best customer experience.

However in the examples us-east-1 is used too. There is a potential to find out the exact allowed pattern by calling aws cloudformation validate-template, but having the documentation be updated is probably better

This is a great catch! - is allowed in key and I'll reach out to doc team to make an update.

[benbridts]

Sidenote: !StartsWith or !MatchRegex would be useful here (I couldn't immediately find the related issue, but I seem to remember something existing)

[benbridts]

#87

[jlhood]

Just a few minor comments to address but overall looks good!