fix: wrong cast of !Ref when using CommaDelimitedList #6768

EloB · 2024-03-01T18:47:03Z

Which issue(s) does this change fix?

Why is this change necessary?

When you pass a parameter with the type CommaDelimitedList and use it with !Ref it should be transformed to an array of strings. Right now it's impossible to set AllowOrigins in local development with !Ref. You always get "CORS Property AllowOrigins was not fully resolved. Will proceed as if the Property was not defined." when using sam local start-api.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html#parameters-section-structure-properties

CommaDelimitedList
An array of literal strings that are separated by commas. The total number of strings should be one more than the total number of commas. Also, each member string is space trimmed.

For example, users could specify "test,dev,prod", and a Ref would result in ["test","dev","prod"].

How does it address the issue?

Resolves the correct type.

What side effects does this change have?

Don't know. First time I touched python. I'm happy that I got this far and fixed the issue. Review with care. Gladly take any feedback on how todo this the proper "python way". Best would probably if you guys take over this. 😅

I've tried this using samdev local start-api and was able to enable CORS using this fix.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

Add input/output type hints to new functions/methods
Write design document if needed (Do I need to write a design document?)
Write/update unit tests
Write/update integration tests
Write/update functional tests if needed
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.

EloB · 2024-03-01T19:11:28Z

Good information from #4026 (comment)

sam --version
SAM CLI, version 1.110.0

It's pretty easy to reproduce current issue.

# Simple "sam init" 
# Output: https://app.warp.dev/block/j5SwgAAQCg2bv9GN7uJAxP
> git clone [email protected]:EloB/aws-sam-cli-command.git
> cd aws-sam-cli-command/
> sam local start-api

Initializing the lambda functions containers.
Local image is up-to-date
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.

Mounting /Users/ollebroms/Documents/sam-app/hello-world as /var/task:ro,delegated, inside runtime container
Containers Initialization is done.
CORS Property AllowOrigins was not fully resolved. Will proceed as if the Property was not defined.
# ...

Attempt 1, failed using parameters

> sam local start-api --parameter-overrides CorsAllowedOrigins="https://www.google.com"

Initializing the lambda functions containers.
Local image is up-to-date
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.

Mounting /Users/ollebroms/Documents/sam-app/hello-world as /var/task:ro,delegated, inside runtime container
Containers Initialization is done.
CORS Property AllowOrigins was not fully resolved. Will proceed as if the Property was not defined.
# ...

Attempt 2, failed using default value as string

Goto template.yaml file and uncomment Default: "https://www.google.com" or run provided git patch.

> git apply <<EOF
diff --git a/template.yaml b/template.yaml
index aeaa32a..ea43120 100644
--- a/template.yaml
+++ b/template.yaml
@@ -9,7 +9,7 @@ Parameters:
   CorsAllowedOrigins:
     Type: CommaDelimitedList
     # Uncomment the following to set the default value. Then rerun sam local start-api.
-    # Default: "https://www.google.com"
+    Default: "https://www.google.com"
     # Default: ["https://www.google.com"]
     Description: The list of allowed origins for CORS


EOF

> sam local start-api

Initializing the lambda functions containers.
Local image is up-to-date
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.

Mounting /Users/ollebroms/Documents/sam-app/hello-world as /var/task:ro,delegated, inside runtime container
Containers Initialization is done.
CORS Property AllowOrigins was not fully resolved. Will proceed as if the Property was not defined.
# ...

Attempt 3, works when providing default as array of strings

Goto template.yaml file and uncomment Default: ["https://www.google.com"] or run provided script.

I think this indicates that sam local start-api isn't transforming that !Ref correctly. According to the json schema for Cloudformation this shouldn't be allowed to enter but it makes the CORS to work.

> git checkout -- .
> git apply <<EOF
diff --git a/template.yaml b/template.yaml
index aeaa32a..af8d0b8 100644
--- a/template.yaml
+++ b/template.yaml
@@ -10,7 +10,7 @@ Parameters:
     Type: CommaDelimitedList
     # Uncomment the following to set the default value. Then rerun sam local start-api.
     # Default: "https://www.google.com"
-    # Default: ["https://www.google.com"]
+    Default: ["https://www.google.com"]
     Description: The list of allowed origins for CORS

 # More info about Globals: https://github.com/awslabs/serverless-application-model/blob/master/docs/globals.rst

EOF

# Also build if you want to be able to fetch. Don't forget to remove folder between attempts.

> sam local start-api

Initializing the lambda functions containers.
Local image is up-to-date
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.

Mounting /Users/ollebroms/Documents/sam-app/hello-world as /var/task:ro,delegated, inside runtime container
Containers Initialization is done.
Mounting HelloWorldFunction at http://127.0.0.1:3000/hello [GET, OPTIONS]
# ...

hnnasit · 2024-03-08T06:26:24Z

Hi @EloB, thanks for your contributions and the detailed information 👍. We will try to validate the changes to make sure everything is working as expected.

sidhujus · 2024-03-11T21:30:15Z

Hi @EloB thanks for your contribution! It looks great. Could you add some unit tests here to cover the new logic? Thanks

sidhujus · 2024-03-11T21:30:37Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

@@ -324,6 +324,9 @@ def get_translation(self, logical_id, resource_attributes=IntrinsicResolver.REF)
        if any(isinstance(logical_id_item, object_type) for object_type in [str, list, bool, int]):
            if resource_attributes not in (IntrinsicResolver.REF, ""):
                return None
+            parameter_info = self._parameters.get(logical_id)
+            if parameter_info and parameter_info.get("Type") == "CommaDelimitedList":


Can we move the "CommaDelimitedList" to a constant

EloB · 2024-03-14T14:57:52Z

@sidhujus @hnnasit I think it's better if you guys can help finalizing this issue. I don't have the experience to be able to write unit test and best practice syntax. The only knowledge I have in Python right now was to enable the debugger and step through the code than I let ChatGPT write the fix.

If this was JavaScript or TypeScript. I would have no problem doing anything above.

Best regards :)

EloB · 2024-03-20T21:57:33Z

Would be good if this becomes merged because it fixes a bug/broken functionality. Right now it blocks local development if you have CORS functionality.

Maybe you guys could add the test later on?

mildaniel · 2024-03-26T16:11:57Z

Thank you for the contribution @EloB!

fix: wrong cast of !Ref when using CommaDelimitedList

963f4c8

EloB requested a review from a team as a code owner March 1, 2024 18:47

EloB requested review from mndeveci and sidhujus March 1, 2024 18:47

github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Mar 1, 2024

sidhujus reviewed Mar 11, 2024

View reviewed changes

mildaniel added 2 commits March 25, 2024 14:25

Add unit tests, keep supporting using an array

9e5d2fe

Format files

42fc5f4

sidhujus approved these changes Mar 25, 2024

View reviewed changes

hnnasit approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'develop' into fix/ref-commadelimitedlist

8a23046

mildaniel added this pull request to the merge queue Mar 26, 2024

Merged via the queue into aws:develop with commit 9ecb3b1 Mar 26, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wrong cast of !Ref when using CommaDelimitedList #6768

fix: wrong cast of !Ref when using CommaDelimitedList #6768

EloB commented Mar 1, 2024 •

edited

Loading

EloB commented Mar 1, 2024 •

edited

Loading

hnnasit commented Mar 8, 2024

sidhujus commented Mar 11, 2024 •

edited

Loading

sidhujus Mar 11, 2024 •

edited

Loading

EloB commented Mar 14, 2024 •

edited

Loading

EloB commented Mar 20, 2024 •

edited

Loading

mildaniel commented Mar 26, 2024

fix: wrong cast of !Ref when using CommaDelimitedList #6768

fix: wrong cast of !Ref when using CommaDelimitedList #6768

Conversation

EloB commented Mar 1, 2024 • edited Loading

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

EloB commented Mar 1, 2024 • edited Loading

Good information from #4026 (comment)

Attempt 1, failed using parameters

Attempt 2, failed using default value as string

Attempt 3, works when providing default as array of strings

hnnasit commented Mar 8, 2024

sidhujus commented Mar 11, 2024 • edited Loading

sidhujus Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

EloB commented Mar 14, 2024 • edited Loading

EloB commented Mar 20, 2024 • edited Loading

mildaniel commented Mar 26, 2024

EloB commented Mar 1, 2024 •

edited

Loading

EloB commented Mar 1, 2024 •

edited

Loading

sidhujus commented Mar 11, 2024 •

edited

Loading

sidhujus Mar 11, 2024 •

edited

Loading

EloB commented Mar 14, 2024 •

edited

Loading

EloB commented Mar 20, 2024 •

edited

Loading