Add local module support to the cloudformation package command

[ericzbeard]

This PR adds the ability to write local CloudFormation modules, which are YAML files that contain Parameters and Resources. These modules can be included in a parent template in a new Modules section. Modules are configured by a combination of formal Parameters (which coincide with the Properties defined in the parent) and an Overrides attribute, which allows users full control over the rendered output. This is a port from the functionality that exists in the Rain CLI. See the snippets below for a basic example.

Parent template

Modules:
  Content:
    Source: ./basic-module.yaml
    Properties:
      Name: foo
    Overrides:
      Bucket:
        Properties:
          OverrideMe: def

basic-module.yaml

Parameters:
  Name:
    Type: String
Resources:
  Bucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: !Ref Name
      OverrideMe: abc

Output from the package command

Resources:
  ContentBucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: foo
      OverrideMe: def

This PR is a proof of concept with a limited set of functionality.

Design Decisions

1. Modules can be imported into a template (or into a module itself) from a new Modules section. In Rain and in registry modules, this section does not exist. Instead, the imported content is configured in a resource and the source of the module is embedded in the Type attribute. This PR implements both ways of configuring an imported module, giving customers the option of whichever feels more intuitive. The benefit of using a Modules section is that it will be more intuitive if we decide to allow injecting other sections other than Resources, like Outputs.

2. Logical IDs are a concatenation of the module name and the logical id defined in the module. Unlike registry modules, users do not have the option of referring to the resources with a dot between the names.

3. The Overrides section is not considered an escape hatch of last resort. It is an encouraged way to configure modules without requiring the module author to anticipate every possible need. IaC abstractions are leaky by nature and we expect users to understand the internals of any module they import. Deciding how many actual Parameters to include in a module is highly opinionated and left up to authors. One caveat here is that Overrides are fragile, since they will break if the module author changes any of the logical IDs in the module. See item 5, Outputs, as a way to mitigate this.

4. This functionality has been added to the aws cloudformation package command. We could have created a new dedicated command for module packaging, like synth. This is an easily reversible decision. The code is ported from the Rain pkg command, which was originally written to mimic the behavior of aws cloudformation package, so it seems like the right place to put it. One annoyance is that if you are only doing local packaging, you still have to supply the s3-bucket argument, and you need to be connected to an AWS account.

5. Modules can define Outputs, which are key-value pairs that can be referenced by the parent. This gives module authors a formal way to pass values to the consumer without them needing to rely on Overrides, which will be fragile if the module author changes any of the names in the module.

6. Instead of using Fn::ForEach as a way of implementing loops, we added a Map attribute to a local module resource. The module output is repeated for each element in the map, substituting $MapValue and $MapIndex in module Properties. This can only be used with lists that can be resolved at build time.

What's not done in this PR

We need to decide what's necessary for an initial version and what can wait

[ ] - Many of the related Rain features like Metadata commands and Constants
[ ] - Conditionals in modules, to emit or omit resources based on Parameters

How to test this PR

Check out my branch and install the aws cli into a local Python environment.

git clone [email protected]:ericzbeard/aws-cli.git
git fetch origin cfn-modules
git checkout cfn-modules
python3 -m venv .env # Or whatever you use for python environments
source .env/bin/activate
pip install -e .
aws cloudformation package --s3-bucket [A_BUCKET_YOU_OWN] \
    --template-file tests/unit/customizations/cloudformation/modules/type-template.yaml

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 473 lines in your changes missing coverage. Please review.

Project coverage is 0.08%. Comparing base (cdea504) to head (e119576).
Report is 107 commits behind head on develop.

Files with missing lines	Patch %	Lines
awscli/customizations/cloudformation/modules.py	0.00%	377 Missing ⚠️
awscli/customizations/cloudformation/parse_sub.py	0.00%	74 Missing ⚠️
...customizations/cloudformation/artifact_exporter.py	0.00%	18 Missing ⚠️
awscli/customizations/cloudformation/exceptions.py	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #9124      +/-   ##
==========================================
- Coverage     0.08%   0.08%   -0.01%     
==========================================
  Files          210     212       +2     
  Lines        16955   17449     +494     
==========================================
  Hits            14      14              
- Misses       16941   17435     +494     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kddejong]

Solid start. I have some corrections and some questions.

[kddejong]

I feel like there are better flows here. You pass in k,v but also the dict and parent k that holds k, v? From what I can tell the big reason for that is to switch an object using an intrinsic function back into a value. You are taking advantage of the fact that dict/list are sent by reference. It feels like an approach that does a return instead of relying on a reference would read better.

[ericzbeard]

You mean something like d[n] = self.resolve(k, v)? Maybe that is better. But I am hesitant to change it, this was the trickiest piece of code to get right.

[kddejong]

What about a string since GetAtt supports both

[ericzbeard]

Do you mean treating GetAtt like a Ref? Just !GetAtt LogicalId?

[kddejong]

Yea, if I'm reading this correct if GetAtt is !GetAtt Foo.Bar it wouldn't be a list and we would raise an error unless the yaml parser is converting it to a list. If that's the case its also worth noting Fn::GetAtt: "Foo.Bar" is also valid.

this is a valid template:

Resources:
  Bucket:
    Type: AWS::S3::Bucket
Outputs:
  BucketArn:
    Value:
      Fn::GetAtt: "Bucket.Arn"

[kddejong]

Also worth noting I forgot to look what yaml parser we are using here so maybe this is covered.

[kddejong]

aws-cli/awscli/customizations/cloudformation/yamlhelper.py

Lines 36 to 40 in 804662b

	if tag == "GetAtt" and isinstance(node.value, str):
	# ShortHand notation for !GetAtt accepts Resource.Attribute format
	# while the standard notation is to use an array
	# [Resource, Attribute]. Convert shorthand to standard format
	value = node.value.split(".", 1)

While I agree with flipping !GetAtt into a list this will miss the conversion of Fn::GetAtt: "foo.bar" and those will remain as strings.

[ericzbeard]

Unless the quotes were stripped off already?

[benbridts]

I was thinking about how to refer to values from a template and noticed that that is a place where the leaky abstraction might be a downside (most of it is true for the overrides too, but it feels more expected there):

If you want to reference a resource in a module you would use e.g. !GetAtt ContentBucket.Arn, which means:

• the module author can't change any resource name in the template
• (not a problem now, but possibly when extending to allow conditions) there is no way to have to different resources cover the same generic module. Imagine e.g a ApiGateway module that creates an HTTP or REST api, depending on a parameter/condition.
• Even if users are not using any overrides, they would need to know the resources (and names) created by the template - I thing the same argument as why this is not a problem with overrides can be applied here, but if you see overrides as a more advanced use case, it might not.

Having an "outputs" or "outputs" equivalent could solve this.

e.g

Parent template

Modules:
  Content:
    Source: ./basic-module.yaml
    Properties:
      Name: foo
Outputs:
  ExampleOutput:
    Value: !GetAtt Content.BucketArn

basic-module.yaml

Parameters:
  Name:
    Type: String
Resources:
  Bucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: !Ref Name
Outputs:
  BucketArn: !GetAtt Bucket.Arn

Output from the package command

Resources:
  ContentBucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: foo
Outputs:
  ExampleOutput:
    Value: !GetAtt ContentBucket.Arn

[kddejong]

Agreed with Ben on this one. You could still allow for an override capability that allows for a missing output that may be needed but having to understand the module resource names can get frustrating and prevents flexible in changing them. There are going to be limitations in this approach that are similar to modules. Adding mappings/conditions into a module will limit its usage because it cannot rely on another resource from the parent template or other module.

[ericzbeard]

Having an "outputs" or "outputs" equivalent could solve this.

I don't see any reason not to add Outputs. It's definitely a more predictable way to reference resources in the module. I still want the Overrides section and the ability to reference things in the module directly if you know the name, but that can be considered a more advanced use case. If you're consuming a 3rd party module, the risk is higher than if you're referencing your own local modules.

[ericzbeard]

Having an "outputs" or "outputs" equivalent could solve this.

Implemented here: 160be7c