The Wayback Machine - https://web.archive.org/web/20220404160323/https://github.com/localstack/localstack/pull/5795
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

Add newline after XML preamble in S3 responses for Serverless compatibility #5795

Merged
merged 1 commit into from Apr 2, 2022

Conversation

whummer
Copy link
Member

@whummer whummer commented Apr 2, 2022

Add newline after XML preamble in S3 responses for Serverless compatibility.

Instead of this:

<?xml version="1.0" encoding="UTF-8"?><LocationConstraint ...

We need to return:

<?xml version="1.0" encoding="UTF-8"?>
<LocationConstraint ...

Tested against real AWS.

This is required because moto is generally collapsing all S3 XML responses: https://github.com/spulec/moto/blob/3718cde444b3e0117072c29b087237e1787c3a66/moto/core/responses.py#L102-L104 . Without this change, newer versions of Serverless are failing with:

Serverless: [AWS cloudformation 400 1.645s 0 retries] describeStacks({ StackName: 'sls-test-local' })
Serverless: Ensuring that deployment bucket exists
Serverless: [AWS s3 200 0.022s 0 retries] getBucketLocation({ Bucket: 'localstack-websockets-serverless' })

 Serverless Error ----------------------------------------

  ServerlessError: Deployment bucket is not in the same region as the lambda function

Debugging the Serverless logic a bit more, it turns out that the location constraint gets extracted as the (literal) string <LocationConstraint xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> instead of the location value like us-east-1 (seems like a bug in their XML parser).

@whummer whummer requested a review from thrau as a code owner Apr 2, 2022
@whummer whummer temporarily deployed to localstack-ext-tests Apr 2, 2022 Inactive
thrau
thrau approved these changes Apr 2, 2022
Copy link
Member

@thrau thrau left a comment

LGTM! we should probably also write an issue report for serverless. seems pretty silly to depend on newlines when parsing xml ...

@whummer
Copy link
Member Author

@whummer whummer commented Apr 2, 2022

Interesting! Seems to be caused by an issue in the AWS JS SDK itself.

This example:

const AWS = require("aws-sdk");
const client = new AWS.S3({endpoint: "http://s3.localhost.localstack.cloud:4566"});
(async () => {
  await client.createBucket({Bucket: "test"}).promise();
  const result = await client.getBucketLocation({Bucket: "test"}).promise();
  console.log(result);
})();

... results in:

$ node test.js
{
  LocationConstraint: '<LocationConstraint xmlns="http://s3.amazonaws.com/doc/2006-03-01/">'
}

(without the changes in this PR). Not really worth it creating a bug report there - too many of our reports/PRs have been ignored by AWS in the past already.. 😄

@coveralls
Copy link

@coveralls coveralls commented Apr 2, 2022

Coverage Status

Coverage decreased (-0.02%) to 91.56% when pulling 0b08f15 on s3-xml-newline into 0b9f90a on master.

@github-actions
Copy link

@github-actions github-actions bot commented Apr 2, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   56m 57s ⏱️ + 5m 35s
   952 tests ±0     912 ✔️ ±0  40 💤 ±0  0 ±0 
1 165 runs  ±0  1 097 ✔️ ±0  68 💤 ±0  0 ±0 

Results for commit 0b08f15. ± Comparison against base commit 0b9f90a.

@whummer whummer merged commit 8f73e14 into master Apr 2, 2022
16 checks passed
@whummer whummer deleted the s3-xml-newline branch Apr 2, 2022
@fabiob
Copy link

@fabiob fabiob commented Apr 4, 2022

Hi! Can this change be impacting GetObject responses from S3? My XML files are suddenly receiving this newline after the preamble.

@whummer
Copy link
Member Author

@whummer whummer commented Apr 4, 2022

Thanks for the pointer @fabiob - mostly likely that was an oversight, so it's quite possible. We'll look into it shortly.. 👍

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.

None yet

4 participants