Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support S3 URI format for package artifact #9403

Closed
zachwhaley opened this issue Apr 29, 2021 · 6 comments · Fixed by #9411
Closed

Support S3 URI format for package artifact #9403

zachwhaley opened this issue Apr 29, 2021 · 6 comments · Fixed by #9411

Comments

@zachwhaley
Copy link
Contributor

Use case description

Serverless supports downloading and using a package artifact from an S3 bucket, but it only allows the outdated global legacy path style S3 URL format (e.g. s3.amazonaws.com/bucket/path) which can be confusing when the AWS web console will give a URL like bucket.s3.us-west-2.amazonaws.com/path.

There are other S3 bucket URI formats.

https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-bucket-intro.html

Proposed solution

Under the hood, Serverless is just parsing this string for the bucket and path, and then making a call to AWS JS SDK S3.getObject()

I propose Serverless should add support for the more basic and easier to parse S3 URI format: S3://bucket-name/key-name and docs should suggest using this format instead of the legacy format.

@pgrzesik
Copy link
Contributor

Hello @zachwhaley - I believe this was just addressed by: #9380

Could you confirm that's the case?

@zachwhaley
Copy link
Contributor Author

zachwhaley commented Apr 30, 2021

Oh wow, merged 4 hours ago 😄

The changes do not seem to account for Region in the old URL style, but that’s a not big deal since the new S3 URI is preferred.

@pgrzesik
Copy link
Contributor

Thanks for bringing that up @zachwhaley - we should definitely support and properly handler old-style URLs with region in mind. Would you be interested in submitting a PR for that?

@zachwhaley
Copy link
Contributor Author

Sure thing! I’ll give it a go :)

@bishtawi
Copy link
Contributor

bishtawi commented Apr 30, 2021

It should be fairly easy to update the regexes to support region urls.

In lib/plugins/aws/utils/parse-s3-uri.js:
The regex ([^/]+)\\.s3\\.amazonaws\\.com/(.+) becomes ([^/]+)\\.s3(?:[.-].+)?\\.amazonaws\\.com/(.+)
And s3\\.amazonaws\\.com/([^/]+)/(.+) becomes s3(?:[.-].+)?\\.amazonaws\\.com/([^/]+)/(.+)

@bishtawi
Copy link
Contributor

bishtawi commented Apr 30, 2021

You can even replace the (?:[.-].+)? with just .* to keep the regex a little more simple I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants