Fix ImportValue handling in existing S3 buckets #6416

[quassnoi]

What did you implement:

Closes #6416

How did you implement it:

Currently, bucket ARN for the permissions is being generated using string concatenation, even if the bucket name is not a string.

I changed the policy generating code to use Fn::Join if the bucket name is not a string but a CloudFormation function (or anything other than a string).

If the bucket name is a string, then the ARN is still being generated using string concatenation on Serverless side (for backwards compatibility and to keep the template simpler)

How can we verify it:

• Create an S3 bucket in a CloudFormation stack, exporting its name
• Use this template:

service: test

provider:
  name: aws
  runtime: nodejs10.x

functions:
  hello:
    handler: handler.hello
    events:
      - s3:
          bucket: !ImportValue test
          events:
            - s3:ObjectCreated:*
          existing: true

, substituting the exported value from the previously created stack

• Make sure the new stack compiles and deploys.

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

• Write tests and confirm existing functionality is not broken.
  Validate via npm test
• Write documentation
• Ensure there are no lint errors.
  Validate via npm run lint-updated
  Note: Some reported issues can be automatically fixed by running npm run lint:fix
• Ensure introduced changes match Prettier formatting.
  Validate via npm run prettier-check-updated
  Note: All reported issues can be automatically fixed by running npm run prettify-updated
• Make sure code coverage hasn't dropped
• Provide verification config / commands / resources
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[pmuens]

Thanks again for working on this @quassnoi 👍

We just merged our bigger refactorings into master. As expected the merge introduced some conflicts with your PR (sorry for that).

Would it be possible to introduce support for multiple different AWS reference types (Arn, GetAtt, Ref, etc.)? We could even create a global helper function which automatically parses such usages of AWS magic (see #3212 and the SQS / SNS event source). What do you think?

[quassnoi]

@pmuens I've fixed the PR.

As for the #3212 you've mentioned, I think that issue is not quite the same.

The one that I'm addressing here deals with the ARN not being generated correctly from the parameter. You have !ImportValue some-export in your template and the code needs to generate the equivalent of arn:aws:s3:::${the-value-of-some-export} out of it. This one is pretty straightforward: just a throw an Fn::Join in the CloudFormation template instead of JS string concatenation, simple as that. It would work as long as there's something CloudFormation can parse in the bucket parameter of the template.

#3212 is the other way around: you have the ARN on input, from a template parameter (possibly a string, possibly a CloudFormation function, possibly something else), and you want to extract the last part of this ARN.

This would require implementing a full-fledged CloudFormation template parser, and ideally the real access to CloudFormation.

This is somewhat out of scope of what I was trying to fix here (honestly, I just wanted to make my existing bucket events work!), but let's see what we can do with that.

What if (just theorizing here) someone passes something like Fn::Join: ["", [Fn::GetAtt: SomeExport, "MyFunction"]] and SomeExport would actually contain arn:aws:lambda:${region}:${account}:function as a hardcoded string value? CloudFormation would not have any problem with this, but to build the ARN on the serverless side you'd have to import SomeExport first which is not a trivial task and probably not something we would want to do in the first place.

So why not let CloudFormation deal with that on the AWS side? Instead of trying to reimplement the CloudFormation logic, we could just use something like Fn::Select : [ 5, Fn::Split: [":", ${arn}]], substituting whatever crazy definition the $arn has, as is, right into the template, and use if wherever we need to use the function name.

I'm not really familiar with whatever exactly serverless needs this name for, but unless serverless absolutely needs it as a string on the client side, there is no reason CloudFormation can't deal with it on the AWS side.

[pmuens]

Hey @quassnoi thanks for updating this PR 👍

I really like the simplicity! Tested it today and it works just fine

Also a big "thank you" for taking a look into the Global Arn parser idea. Apparently I missed something when reading through the PR description / title here.

---

What if (just theorizing here) someone passes something like Fn::Join: ["", [Fn::GetAtt: SomeExport, "MyFunction"]] and SomeExport would actually contain arn:aws:lambda:${region}:${account}:function as a hardcoded string value? CloudFormation would not have any problem with this, but to build the ARN on the serverless side you'd have to import SomeExport first which is not a trivial task and probably not something we would want to do in the first place.

So why not let CloudFormation deal with that on the AWS side? Instead of trying to reimplement the CloudFormation logic, we could just use something like Fn::Select : [ 5, Fn::Split: [":", ${arn}]], substituting whatever crazy definition the $arn has, as is, right into the template, and use if wherever we need to use the function name.

I'm not really familiar with whatever exactly serverless needs this name for, but unless serverless absolutely needs it as a string on the client side, there is no reason CloudFormation can't deal with it on the AWS side.

Yes, I agree with you. It would be way more beneficial to let AWS do the heavy lifting. Thanks for leaving some pointers here on how we could tackle that problem (I'll cross-reference your comment in the issue thread)!