WIP: `sam build` Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions) by aahung · Pull Request #2732 · aws/aws-sam-cli

aahung · 2021-03-18T07:44:50Z

Which issue(s) does this change fix?

Attempt to fix #2724

Why is this change necessary?

When sam tries to resolve intrinsic functions in Parameters passed from parent, it simply does this replacement:

single `!Ref` used in parent

# parent.yaml
Type: AWS::Serverless::Application
Properties:
  Location: child.yaml
  Parameters:
    XXX: !Ref SomeLogicalID

# child.yaml
Parameters:
  XXX:
    Type: String

.....
-     Layers: 
-      - !Ref XXX
+      - SomeLogicalID

Compound intrinsic function like `!Join`

# parent.yaml
Type: AWS::Serverless::Application
Properties:
  Location: child.yaml
  Parameters:
    XXX: !Join [",", [!Ref SomeLogicalID, !Ref SomeLogicalID2]]

# child.yaml
Parameters:
  XXX:
    Type: CommaDelimitedList

.....
-     Layers: !Ref XXX
+     Layers: XXX

In either case, sam won't be able to link the resolved value to original resources passed from the parent

How does it address the issue?

Have special handling in get_translation().
Add a new special function "CrossStackRef" used by SamFunctionProvider to locate resource reference across different stacks.

Support these methods:

Directly passed down using `!Ref`

Resources:
  MyLayerVersion:
    Type: AWS::Serverless::LayerVersion
    Properties:
      LayerName: MyLayer
      Description: Layer description
      ContentUri: MyLayerVersion
      CompatibleRuntimes:
        - python3.8

  App:
    Type: AWS::Serverless::Application
    Properties:
      Location: ./child.yaml
      Parameters:
        Layer: !Ref MyLayerVersion

support multi-level passing?

Using `!Join [",", [..., ..., ...]]`

Resources:
  MyLayerVersion:
    Type: AWS::Serverless::LayerVersion
    Properties:
      LayerName: MyLayer
      Description: Layer description
      ContentUri: MyLayerVersion
      CompatibleRuntimes:
        - python3.8
  
  MyLayerVersion2:
    Type: AWS::Serverless::LayerVersion
    Properties:
      LayerName: MyLayer
      Description: Layer description
      ContentUri: MyLayerVersion
      CompatibleRuntimes:
        - python3.8

  App:
    Type: AWS::Serverless::Application
    Properties:
      Location: ./b_child.yaml
      Parameters:
        Layers: !Join [",", [!Ref MyLayerVersion, !Ref MyLayerVersion2]]
        ParentLayerVersion2: !Ref MyLayerVersion2

support multi-level passing?

What side effects does this change have?

Checklist

Add input/output type hints to new functions/methods
Write design document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

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

…ng arguments around

…t stacks

mndeveci

Can we add some tests to cover use cases with intrinsic functions? I didn't see any addition most of them was fixing the existing ones I believe.

mndeveci · 2021-03-18T22:32:19Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

+                # It is a parameter passed down from the parent
+                # here we use a special function "CrossStackRef" to refer a logicalID from other stacks
+                # ".." means the parent stack


nit: can we move the comment under if statement?

mndeveci · 2021-03-18T22:35:59Z

samcli/lib/providers/sam_stack_provider.py

        return os.path.normpath(os.path.join(os.path.dirname(stack_file_path), path))
+
+    @staticmethod
+    def resolve_stack(stacks: List[Stack], base_stack: Stack, relative_stack_path: str) -> Stack:


Please add some documentation to the method

mndeveci · 2021-03-18T22:38:52Z

samcli/lib/providers/sam_function_provider.py

+                # for example, {"Ref": "layerLogicalID"} refers to the "layerLogicalID" in the current stack.
+                # {"Ref": {"CrossStackRef": ["..", "layerLogicalID"]}
+                #   refers to the "layerLogicalID" in the parent stack.
+                # {"Ref": {"CrossStackRef": ["..", {"CrossStackRef": ["..", "layerLogicalID"]})}


We should carefully clear this CrossStackRef from the template.

And what happens if there are more than one level between the layer definition vs the function. Like RootStack (LayerDefinition) -> ChildStack -> GrandChildStack (Function which uses LayerDefinition from RootStack)?

We should carefully clear this CrossStackRef from the template.

Yes, but this does not affect the output template because it is only affected by builder.update_template()(only update codeuri and other path properties) and template.move_template() (only move the file).

And what happens if there are more than one level between the layer definition vs the function.

this is the last example. I tested this with

# template.yaml Resources: MyLayerVersion: Type: AWS::Serverless::LayerVersion Properties: LayerName: MyLayer Description: Layer description ContentUri: MyLayerVersion CompatibleRuntimes: - python3.8 App: Type: AWS::Serverless::Application Properties: Location: ./child.yaml Parameters: Layer: !Ref MyLayerVersion # child.yaml Parameters: Layer: Type: String Resources: Function: Type: AWS::Serverless::Function Properties: CodeUri: function Handler: app.lambda_handler Runtime: python3.8 Layers: - !Ref Layer App: Type: AWS::Serverless::Application Properties: Location: ./child2.yaml Parameters: Layer: !Ref Layer # child2.yaml Parameters: Layer: Type: String Resources: Function2: Type: AWS::Serverless::Function Properties: CodeUri: function Handler: app.lambda_handler Runtime: python3.8 Layers: - !Ref Layer

aahung · 2021-03-18T23:11:41Z

Can we add some tests to cover use cases with intrinsic functions? I didn't see any addition most of them was fixing the existing ones I believe.

@mndeveci Before I polish this PR too much, do you think this is the approach the one we want to implement? For each intrinsic function use case, we need to manually support it.

CoshUS · 2021-03-18T23:29:40Z

samcli/lib/providers/sam_function_provider.py

+                # Here layer refers to another resources.
+                # in a multi stack system, the layer might locate in the current stack or passed down from parents.
+                # for example, {"Ref": "layerLogicalID"} refers to the "layerLogicalID" in the current stack.
+                # {"Ref": {"CrossStackRef": ["..", "layerLogicalID"]}


Wouldn't adding a new SAM CLI specific intrinsic function break existing translation? The template will not be compatible with uploading to CFN directly.

The built template won't contain this special function.

The output template because it is only affected by builder.update_template()(only update codeuri and other path properties) and template.move_template() (only move the file).

aahung · 2021-03-23T19:00:41Z

close in favor of #2760

aahung added 4 commits March 17, 2021 23:47

refactor: Convert some staticmethods to normal methods to avoid passi…

6f4f69c

…ng arguments around

Update function provider unit test after refactoring

c8a44d4

Resolve layer ARN passed (directly using a single !Ref) from to paren…

a92d301

…t stacks

Handle !Join

f265ff6

aahung changed the title ~~Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions)~~ WIP: Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions) Mar 18, 2021

aahung changed the title ~~WIP: Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions)~~ WIP: sam build Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions) Mar 18, 2021

aahung requested review from CoshUS, jfuss, mndeveci, qingchm and sriram-mv March 18, 2021 07:47

mndeveci suggested changes Mar 18, 2021

View reviewed changes

CoshUS reviewed Mar 18, 2021

View reviewed changes

CoshUS added the pr/internal label Mar 22, 2021

aahung closed this Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: `sam build` Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions)#2732

WIP: `sam build` Resolve layer ARN passed from to parent stacks (not covering all intrinsic functions)#2732
aahung wants to merge 4 commits intoaws:developfrom
aahung:fix-intrinsic-nested

aahung commented Mar 18, 2021 •

edited

Loading

Uh oh!

mndeveci left a comment

Uh oh!

mndeveci Mar 18, 2021

Uh oh!

mndeveci Mar 18, 2021

Uh oh!

mndeveci Mar 18, 2021 •

edited

Loading

Uh oh!

aahung Mar 18, 2021 •

edited

Loading

Uh oh!

aahung commented Mar 18, 2021

Uh oh!

CoshUS Mar 18, 2021

Uh oh!

aahung Mar 18, 2021

Uh oh!

aahung commented Mar 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aahung commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) does this change fix?

Why is this change necessary?

single !Ref used in parent

Compound intrinsic function like !Join

How does it address the issue?

Directly passed down using !Ref

Using !Join [",", [..., ..., ...]]

What side effects does this change have?

Checklist

Uh oh!

mndeveci left a comment

Choose a reason for hiding this comment

Uh oh!

mndeveci Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

mndeveci Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

mndeveci Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aahung Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aahung commented Mar 18, 2021

Uh oh!

CoshUS Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

aahung Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

aahung commented Mar 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aahung commented Mar 18, 2021 •

edited

Loading

single `!Ref` used in parent

Compound intrinsic function like `!Join`

Directly passed down using `!Ref`

Using `!Join [",", [..., ..., ...]]`

mndeveci Mar 18, 2021 •

edited

Loading

aahung Mar 18, 2021 •

edited

Loading