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

fix LocationConstraint added for us-east-1 by the SDK #214

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 22, 2023

This PR adds a patch to the aws-sdk, using its own utilities to overwrite an S3 method.
It fixes localstack/localstack-demo#52

In aws-sdk/lib/services/s3.js, there this createBucket method:

  createBucket: function createBucket(params, callback) {
    // When creating a bucket *outside* the classic region, the location
    // constraint must be set for the bucket and it must match the endpoint.
    // This chunk of code will set the location constraint param based
    // on the region (when possible), but it will not override a passed-in
    // location constraint.
    if (typeof params === 'function' || !params) {
      callback = callback || params;
      params = {};
    }
    var hostname = this.endpoint.hostname;
    // copy params so that appending keys does not unintentioinallly
    // mutate params object argument passed in by user
    var copiedParams = AWS.util.copy(params);
    if (hostname !== this.api.globalEndpoint && !params.CreateBucketConfiguration) {
      copiedParams.CreateBucketConfiguration = { LocationConstraint: this.config.region };
    }
    return this.makeRequest('createBucket', copiedParams, callback);
  },

However, the if (hostname !== this.api.globalEndpoint && !params.CreateBucketConfiguration) check to add the LocationConstraint evaluate the endpoint (to see if it's a region specific one) against the global endpoint for us-east-1.

This patch changes the logic to simply check the region set in the config, and if different than us-east-1 and a configuration is not set, sets it.

I can now properly deploy the demo with the new S3 v2 provider which enforces the specific fact that you cannot set the LocationConstraint to us-east-1.

To reproduce, you can deploy https://github.com/localstack/localstack-demo/ with and without the fix, with PROVIDER_OVERRIDE_S3=v2 when starting LocalStack.

I however have no idea how to add a test for this.

@bentsku bentsku requested review from whummer and joe4dev March 22, 2023 20:13
actually use the patch
@bentsku bentsku force-pushed the fix-location-constraint branch from 6b43f85 to ae98669 Compare March 22, 2023 20:24
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

✅ I tested it against LocalStack on macOS and can confirm it fixes the location constraint issue.
(there are still other issues with the demo app such as a ClassDefNotFoundException in the stepfunctions jar)

I guess it should not affect deployment against AWS but a quick test might be helpful.

@@ -1,6 +1,6 @@
{
"name": "serverless-localstack",
"version": "1.0.4",
"version": "1.0.5",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: package.lock not updated

// copy params so that appending keys does not unintentioinallly
// mutate params object argument passed in by user
var copiedParams = AWS.util.copy(params);
if (this.config.region !== 'us-east-1' && !params.CreateBucketConfiguration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch 👍

Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@whummer whummer merged commit 360add9 into master Mar 23, 2023
@whummer whummer deleted the fix-location-constraint branch March 23, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment bucket error with new S3 provider
3 participants