Skip to content

fix: rename list bucket/object reponse class#7545

Merged
macnev2013 merged 3 commits intomasterfrom
fix/listbucket-reponse
Feb 6, 2023
Merged

fix: rename list bucket/object reponse class#7545
macnev2013 merged 3 commits intomasterfrom
fix/listbucket-reponse

Conversation

@macnev2013
Copy link
Contributor

@macnev2013 macnev2013 commented Jan 24, 2023 •
edited
Loading

Issue

Fix

  • Patched ListBucketsOutput to ListAllMyBucketsResult.
    • In boto spec definition of output from ListBuckets is ListBucketsOutput whereas AWS is returning ListAllMyBucketsResult.
  • list_objects contains ListBucketResult object whereas we are returning ListObjectsOutput

Commands to Reproduce

Start localstack instance with env `PROVIDER_OVERRIDE_S3: "asf"`

Comparing the response from this commands will po
list_bucket

awslocal s3api list-buckets --profile ls --debug

aws s3api list-buckets --profile ls --debug

list-objects

awslocal s3 mb s3://test-bucket --profile ls --region us-east-1 --debug
awslocal s3api list-objects --bucket test-bucket --profile ls --region us-east-1 --debug

aws s3 mb s3://test-bucket --profile ls --region us-east-1 --debug
aws s3api list-objects --bucket test-bucket --profile ls --region us-east-1 --debug

Tested Also with

https://github.com/Dowwie/localstack_s3_xml_issue

@macnev2013 macnev2013 temporarily deployed to localstack-ext-tests January 24, 2023 13:19 — with GitHub Actions Inactive
@macnev2013 macnev2013 mentioned this pull request Jan 24, 2023
Closed
@github-actions
Copy link

github-actions bot commented Jan 24, 2023 •
edited
Loading

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 28m 6s ⏱️ -14s
1 676 tests +1  1 344 ✔️ +1  332 💤 ±0  0 ❌ ±0 
2 388 runs  +1  1 718 ✔️ +1  670 💤 ±0  0 ❌ ±0 

Results for commit aa62903. ± Comparison against base commit 8c4de20.

♻️ This comment has been updated with latest results.

thrau
thrau approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM! we should double check if the tests pass with PROVIDER_OVERRIDE_S3=asf. and also i'll let @bentsku do the final sign-off

localstack/aws/spec-patches.json
Comment on lines +774 to +783
},
{
"op": "remove",
"path": "/shapes/ListBucketsOutput"
},
{
"op": "add",
"path": "/shapes/ListAllMyBucketsResult",
"value": {
"type": "structure",
"members": {
"Buckets": {
"shape": "Buckets",
"documentation": "<p>The list of buckets owned by the requester.</p>"
},
"Owner": {
"shape": "Owner",
"documentation": "<p>The owner of the buckets listed.</p>"
}
}
}
},
{
"op": "replace",
"path": "/operations/ListBuckets/output/shape",
"value": "ListAllMyBucketsResult"
Copy link
Member

@thrau thrau Jan 24, 2023

Choose a reason for hiding this comment

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

just asking (haven't checked): could this be simplified with the "move", or "replace" operation? https://jsonpatch.com/#move

Copy link
Contributor Author

@macnev2013 macnev2013 Jan 25, 2023

Choose a reason for hiding this comment

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

Hey @thrau, Move worked in this case. thanks for the suggestion :)

@macnev2013 macnev2013 temporarily deployed to localstack-ext-tests January 25, 2023 07:18 — with GitHub Actions Inactive
@macnev2013 macnev2013 requested a review from thrau January 25, 2023 07:25
@macnev2013
Copy link
Contributor Author

macnev2013 commented Jan 25, 2023 •
edited
Loading

LGTM! we should double check if the tests pass with PROVIDER_OVERRIDE_S3=asf. and also i'll let @bentsku do the final sign-off

@thrau, It was working with the older provider. The fix is mainly focused on PROVIDER_OVERRIDE_S3=asf. (I've double checked it though 👍🏻 )

@macnev2013 macnev2013 changed the title fix: rename list buckets reponse fix: rename list bucket/object reponse class Jan 25, 2023
@macnev2013 macnev2013 temporarily deployed to localstack-ext-tests January 25, 2023 13:51 — with GitHub Actions Inactive
@thrau thrau removed their request for review January 25, 2023 20:29
@alexrashed
Copy link
Member

alexrashed commented Jan 27, 2023 •
edited
Loading

LGTM! we should double check if the tests pass with PROVIDER_OVERRIDE_S3=asf. and also i'll let @bentsku do the final sign-off

@thrau We added the S3 ASF provider to the test pipeline some time ago, in order to make sure that there are no regressions introduced along the way. You can see the run for this PR here: https://app.circleci.com/pipelines/github/localstack/localstack/12048/workflows/9d86dafc-9ad2-4547-aa14-2cfec3162855/jobs/85316

However, these tests aren't reliable in this case, because they weren't failing before this bug was fixed.
This isn't really surprising, because the root elements of the XML responses are often ignored by clients / response parsers.
So I would actually be in favor of a specific test for this fix before merging it. It could be a simple request to the API which then accesses the response content, parses the XML response, and checks that the root tag is in fact the correct one.

bentsku
bentsku requested changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

I share Alex's opinion, we should write a simple test targeting the S3 endpoint for AWS and LS and test the content of the response (could use xmltodict), maybe using pre-signed URL for managing authentication of the request (making it easier to parse the response). Then we should be good to go! Thanks for tackling this and getting to know specs patches! 😄

@bentsku bentsku linked an issue Jan 27, 2023 that may be closed by this pull request
bug: AWS s3 is returning a different XML object than localstack does with the new Localstack s3 provider #7541
Closed
1 task
@macnev2013 macnev2013 temporarily deployed to localstack-ext-tests February 1, 2023 10:51 — with GitHub Actions Inactive
bentsku
bentsku approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 thanks for adding the test!

@macnev2013 macnev2013 force-pushed the fix/listbucket-reponse branch from f74a4da to aa62903 Compare February 1, 2023 15:57
@macnev2013 macnev2013 temporarily deployed to localstack-ext-tests February 1, 2023 15:57 — with GitHub Actions Inactive
@macnev2013 macnev2013 merged commit b164a71 into master Feb 6, 2023
@alexrashed alexrashed deleted the fix/listbucket-reponse branch February 6, 2023 10:42
@Dowwie
Copy link

Dowwie commented Feb 6, 2023 •
edited
Loading

Hey everyone. Has the Elixir example that I created to reproduce the issue no longer needed? Can I remove the repository? in re: https://github.com/Dowwie/localstack_s3_xml_issue

@Dowwie
Copy link

Dowwie commented Feb 6, 2023

@thrau

@macnev2013
Copy link
Contributor Author

macnev2013 commented Mar 3, 2023

Hi @Dowwie, Apologies for delayed response. You can remove the example. Thanks for your help 🚀

This was referenced Mar 27, 2023
Merged
Closed
Merged
@bentsku bentsku mentioned this pull request Apr 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thrau thrau thrau approved these changes

@bentsku bentsku bentsku approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: AWS s3 is returning a different XML object than localstack does with the new Localstack s3 provider

5 participants

@macnev2013 @alexrashed @Dowwie @thrau @bentsku