Conditionally adding metadata to translated resources #2224

andrew-glenn · 2021-11-15T23:07:28Z

Exposing an additional translated_resource_mapping property in the Translator class allows downstream consumers to apply logic to translated resources.

An example use case: copying cfn-lint metadata to new resources so that rule exceptions are honored.

See: aws-cloudformation/cfn-lint#2173

moelasmar

could you please add some unit testing cases for this new change?

jfuss · 2021-11-16T21:21:23Z

@andrew-glenn It isn't clear what the purpose of this is for. Is it only for local tooling support? Why do you want to mutate underlying resources instead of the SAM Resources itself?

jfuss

"Request changes" to indicate not to merge until the use-case is more understood.

andrew-glenn · 2021-11-16T21:35:38Z

@jfuss As an example, cfn-lint supports metadata per-resource to define specific configuration options. A byproduct of the SAM Translator is that a single resource is split off into multiple, without the original metadata being copied to the new resources.

Rather than mess with the translator logic - which I'm not familiar with, and has a massive blast radius - I opted to surface a mapping of original SAM resource names --> Generated CFN Resources.

Providing this mapping allows me (downstream) to iterate over the map and perform whatever actions I wish based on this data.

Admittedly, this property wouldn't be used directly via the SAM CLI, but rather those that import SAM as a python library.

moelasmar · 2021-11-17T05:26:38Z

@jfuss .. my understanding is that change will not affect the transformation logic, but will add some value for the sam translator library.

jfuss · 2021-11-17T17:01:19Z

@moelasmar

@andrew-glenn and I discussed over slack yesterday afternoon. It is true this wouldn't impact the service but does mean we have to support this "overriding" locally. In the ideal world, I would like cfn-lint to lint the SAM Resources not the underlying ones but that requires a spec from SAM (this also decouples cfn-lint and SAM, which is an added benefit). I want to understand more of the intention behind the change and why we should support this. Not trying to say this isn't something we should do but there might be other ways to do the same thing. For example, maybe SAM should be taking the Metadata and applying it to all the underlying resources. Then we don't have to this local only "patching" mechanism, making support easier on us.

I scoped some time for myself on Thursday to think a little deeper and reach back out to @andrew-glenn to understand what he is trying to achieve with this and cfn-lint. That should give me a better understanding on what we may want to do here. My initially thinking is either 1) apply Metadata to generated resources or 2) Apply Metadata to generated resources so but make it a flag in the transform call.

Open to other ideas here, just general cautious of features that only apply locally.

andrew-glenn · 2021-11-17T17:02:56Z

For what it's worth to the studio audience, I'm now convinced having SAM copy the metadata is a better approach. I'll sync up with @jfuss tomorrow to determine the details

andrew-glenn · 2021-11-19T22:58:45Z

@jfuss @moelasmar

I've updated the PR to reflect the new approach (adding metadata in translation). I've set it to default disabled for backwards compatibility.

codecov-commenter · 2021-11-19T23:40:42Z

Codecov Report

Merging #2224 (bca27e4) into develop (e7a1496) will increase coverage by 0.81%.
The diff coverage is 96.85%.

@@             Coverage Diff             @@
##           develop    #2224      +/-   ##
===========================================
+ Coverage    93.58%   94.40%   +0.81%     
===========================================
  Files           90       97       +7     
  Lines         6124     7092     +968     
  Branches      1260     1436     +176     
===========================================
+ Hits          5731     6695     +964     
+ Misses         183      181       -2     
- Partials       210      216       +6

Impacted Files	Coverage Δ
samtranslator/model/cognito.py	`85.71% <ø> (ø)`
samtranslator/model/api/http_api_generator.py	`91.21% <75.00%> (+0.02%)`	⬆️
samtranslator/model/api/api_generator.py	`93.70% <89.28%> (-0.66%)`	⬇️
samtranslator/model/apigateway.py	`96.98% <96.15%> (-0.18%)`	⬇️
samtranslator/__init__.py	`100.00% <100.00%> (ø)`
samtranslator/feature_toggle/dialup.py	`100.00% <100.00%> (ø)`
samtranslator/feature_toggle/feature_toggle.py	`100.00% <100.00%> (+12.16%)`	⬆️
samtranslator/intrinsics/actions.py	`98.79% <100.00%> (+0.07%)`	⬆️
samtranslator/metrics/method_decorator.py	`100.00% <100.00%> (ø)`
samtranslator/metrics/metrics.py	`100.00% <100.00%> (ø)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b446dd...bca27e4. Read the comment docs.

jfuss · 2021-11-23T15:34:14Z

tests/translator/output/translate_convert_metadata.json

+{
+  "Resources": {
+    "Images": {
+      "Type": "AWS::S3::Bucket",


The bucket is defined in the template above, I don't think we want SAM to mutate that resource. I know we do today, but that is in very specific use-cases (adding events triggers).

I'm not 100% seeing how this resource is being mutated, since it exists previously. What am I overlooking?

Note, to save back and forth:

If we want to exclude certain resource types from the metadata passthrough, that's fine. I'll need a list of resource types that would apply here. I'm a little bit in the dark.

Edit: I see the issue here.

jfuss · 2021-11-23T15:37:42Z

samtranslator/translator/transform.py

@@ -2,7 +2,7 @@
 from samtranslator.parser.parser import Parser


-def transform(input_fragment, parameter_values, managed_policy_loader, feature_toggle=None):
+def transform(input_fragment, parameter_values, managed_policy_loader, feature_toggle=None, convert_metadata=False):


Nit: We aren't really converting anything for metadata here. Maybe something like add_metadata or passthrough_metadata is more clear for naming?

Sure, no problem.

jfuss · 2021-11-23T15:40:40Z

samtranslator/translator/translator.py

@@ -152,7 +153,10 @@ def translate(self, sam_template, parameter_values, feature_toggle=None):
                del template["Resources"][logical_id]
                for resource in translated:
                    if verify_unique_logical_id(resource, sam_template["Resources"]):
-                        template["Resources"].update(resource.to_dict())
+                        _r = resource.to_dict()


Why convert the resources to dict? Should we just model Metadata with the resource pojo's we have?

The current implementation (Line 155) converts it to a dictionary. I assumed this was done for a Very Good(tm) reason, so I simply followed the existing pattern.

jfuss · 2021-11-23T15:50:06Z

samtranslator/translator/translator.py

@@ -152,7 +153,10 @@ def translate(self, sam_template, parameter_values, feature_toggle=None):
                del template["Resources"][logical_id]
                for resource in translated:
                    if verify_unique_logical_id(resource, sam_template["Resources"]):
-                        template["Resources"].update(resource.to_dict())
+                        _r = resource.to_dict()
+                        if resource_dict.get("Metadata") and convert_metadata:


I think a comment here could go a long away in understanding what is happening. It took me a while but I think what is happening is the following:
For each SAM resource in the source template, we translate to the underlying CloudFormation Resources. After we translate, we append any metadata in the SAM Resource to the translated CloudFormation Resources.

They generally works but I think there might be a couple edge cases outstanding:

S3 buckets are updated (I made a comment below about this)

I am not sure how generated resources work in this case. If you have an API Event on an AWS::Serverless::Function.Event what happens? Does the Metadata get pass through correctly? That is the Metadata on AWS::Serverless::Function gets added to all the API Gateway resources? This would be a good test case to add regardless of it it works in the current implementation or not.

see above

Yes. A single SAM resource translates to multiple CFN resources. The metadata on the single SAM resource is passed through to the generated CFN resources, regardless of what the resource type is.

jfuss · 2022-01-24T22:42:45Z

@moelasmar Can you re-review this? Things have changed since you last review

need to re-review it again, PR changed since last time I reviewed it.

github-actions bot added the pr/external label Nov 15, 2021

andrew-glenn mentioned this pull request Nov 15, 2021

Extending linter to fully support translated SAM templates aws-cloudformation/cfn-lint#2173

Closed

moelasmar requested changes Nov 16, 2021

View reviewed changes

moelasmar requested a review from jfuss November 16, 2021 00:17

andrew-glenn force-pushed the develop branch 3 times, most recently from 7ae6500 to f29153d Compare November 16, 2021 18:00

moelasmar previously approved these changes Nov 16, 2021

View reviewed changes

jfuss requested changes Nov 16, 2021

View reviewed changes

andrew-glenn force-pushed the develop branch from f29153d to ecabca2 Compare November 19, 2021 22:57

andrew-glenn changed the title ~~Adding Translator.translated_resource_mapping property~~ Conditionally adding metadata to translated resources Nov 19, 2021

jfuss self-assigned this Nov 23, 2021

jfuss requested changes Nov 23, 2021

View reviewed changes

aws-sam-cli-bot added the maintainer/need-followup label Dec 6, 2021

andrew-glenn force-pushed the develop branch from ecabca2 to c66de91 Compare December 6, 2021 18:43

Conditionally exposing metadata to translated resources.

7279821

andrew-glenn force-pushed the develop branch from c66de91 to 7279821 Compare December 6, 2021 18:59

Merge branch 'develop' into develop

bca27e4

jfuss approved these changes Jan 24, 2022

View reviewed changes

moelasmar self-requested a review January 24, 2022 22:55

aws-sam-cli-bot removed the maintainer/need-followup label Jan 25, 2022

moelasmar approved these changes Jan 25, 2022

View reviewed changes

moelasmar merged commit c575b54 into aws:develop Jan 25, 2022

andrew-glenn mentioned this pull request Feb 3, 2022

Develop aws-quickstart/quickstart-cribl-logstream#4

Merged

mew1033 mentioned this pull request Jan 22, 2024

Use samtranslate passthrough_metadata to support ignoring checks on Serverless resources aws-cloudformation/cfn-lint#3028

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally adding metadata to translated resources #2224

Conditionally adding metadata to translated resources #2224

andrew-glenn commented Nov 15, 2021 •

edited

moelasmar left a comment

jfuss commented Nov 16, 2021

jfuss left a comment

andrew-glenn commented Nov 16, 2021

moelasmar commented Nov 17, 2021

jfuss commented Nov 17, 2021

andrew-glenn commented Nov 17, 2021

andrew-glenn commented Nov 19, 2021

codecov-commenter commented Nov 19, 2021 •

edited

jfuss Nov 23, 2021

andrew-glenn Dec 3, 2021

andrew-glenn Dec 3, 2021 •

edited

jfuss Nov 23, 2021

andrew-glenn Dec 3, 2021

jfuss Nov 23, 2021

andrew-glenn Dec 3, 2021

jfuss Nov 23, 2021

andrew-glenn Dec 3, 2021

jfuss commented Jan 24, 2022

Conditionally adding metadata to translated resources #2224

Conditionally adding metadata to translated resources #2224

Conversation

andrew-glenn commented Nov 15, 2021 • edited

moelasmar left a comment

Choose a reason for hiding this comment

jfuss commented Nov 16, 2021

jfuss left a comment

Choose a reason for hiding this comment

andrew-glenn commented Nov 16, 2021

moelasmar commented Nov 17, 2021

jfuss commented Nov 17, 2021

andrew-glenn commented Nov 17, 2021

andrew-glenn commented Nov 19, 2021

codecov-commenter commented Nov 19, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-glenn Dec 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss commented Jan 24, 2022

andrew-glenn commented Nov 15, 2021 •

edited

codecov-commenter commented Nov 19, 2021 •

edited

andrew-glenn Dec 3, 2021 •

edited