Small fixes for S3 on Outposts #470
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
sauraank
reviewed
Aug 23, 2023
| fn add_header( | ||
| /// Add a header to this message. The header is added if necessary and any existing values for | ||
| /// this header are removed. | ||
| fn set_header( |
There was a problem hiding this comment.
Was there any use of adding a new header with different value rather than overwriting it, earlier?
There was a problem hiding this comment.
Nope, that's why I changed it. Multiple headers with the same name is a pretty niche HTTP thing that we don't need.
jamesbornholt
force-pushed
the
outposts-fixes
branch
from
199bef9 to
1e19915
Compare
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
force-pushed
the
outposts-fixes
branch
from
1e19915 to
1a81ffd
Compare
jamesbornholt
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
jamesbornholt
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
jamesbornholt
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
jamesbornholt
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
|
I can confirm that this also fixes the GetObject operation on the Seeweb provider; it previously failed with an invalid signature error. Thank you |
This fixes two issues that were preventing Mountpoint from working against Outposts buckets: 1. Outposts doesn't include the bucket name in ListObjectsV2 responses. We weren't actually using that output anyway, so I just removed it. 2. For GetObject requests, we were sending a HTTP header like `Accept: application/xml,*/*`. While technically valid HTTP, it's weird to accept */* as well as something else, and it was confusing Outposts' request signing. So I switched to overwriting the existing header, which is what the comment suggested the code was intended to do anyway. I also took this chance to make a little cleanup to parsing ListObjectsV2 responses: the `parse` functions shouldn't be defined on the generic `ListObjectsResult` structs, which are shared by all clients. Signed-off-by: James Bornholt <[email protected]>
jamesbornholt
force-pushed
the
outposts-fixes
branch
from
1a81ffd to
01ae4eb
Compare
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
jamesbornholt
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 10, 2024
Update the CRT libraries to the latest releases. In particular, include: * Support full object checksum and crc64nvme. ([awslabs/aws-c-s3#468](awslabs/aws-c-s3#468)) <details> <summary>Full CRT changelog:</summary> ``` Submodule mountpoint-s3-crt-sys/crt/aws-c-common be8ed873..fadfef49: > Support relative paths when prebuilding dependencies with CMake (#1174) > Switch CI to use roles (#1173) Submodule mountpoint-s3-crt-sys/crt/aws-c-s3 45894ed3..337155f6: > Support full object checksum (#468) > [meta request]: assign shutdown_callback inside critical region (#470) > Switch CI to use roles (#463) ``` </details> ### Does this change impact existing behavior? No. ### Does this change need a changelog entry? No. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the [Developer Certificate of Origin (DCO)](https://developercertificate.org/). Signed-off-by: Alessandro Passaro <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
This fixes two issues that were preventing Mountpoint from working against Outposts buckets:
Accept: application/xml,*/*. While technically valid HTTP, it's weird to accept*/*as well as something else, and it was confusing Outposts' request signing. So I switched to overwriting the existing header, which is what the comment suggested the code was intended to do anyway.I also took this chance to make a little cleanup to parsing ListObjectsV2 responses: the
parsefunctions shouldn't be defined on the genericListObjectsResultstructs, which are shared by all clients.This change doesn't make Mountpoint completely work on Outposts. The outstanding issue is that Outposts don't support S3 additional checksums, but we turn those on by default. We'll need a way to turn those off for Outposts buckets. But read-only usage should work.
Does this change impact existing behavior?
Yes, removing the bucket name is a breaking change to the client crate, and we now send a different
Acceptheader on GetObject requests. There shouldn't be any customer-visible effects.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).