-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: S3 on Outpost requests miss x-amz-content-sha256 headers #2811
Conversation
Codecov ReportBase: 93.45% // Head: 93.47% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2811 +/- ##
===========================================
+ Coverage 93.45% 93.47% +0.02%
===========================================
Files 63 63
Lines 13298 13298
===========================================
+ Hits 12427 12430 +3
+ Misses 871 868 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments on the tests, but otherwise looks good.
config = Config(signature_version="s3v4") | ||
self.client = self.session.create_client( | ||
"s3", self.region, config=config | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we intentionally set s3v4
or was this carryover from other tests? If it's not proving something specific, maybe we want to leave this unconfigured to make sure s3 is choosing the right signer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a carryover from the other tests. Note that this test is in a class named TestS3SigV4
.
At a high level:
- The only test that fails before the fix in this PR and passes afterwards is in
test_basic_outpost_arn
where this PR adds three lines (719-721). This is in commit 384f886. - All changes to the
TestS3SigV4
class are tangential and improve test coverage and add inline comments because those tests misled us during the investigation of this bug. These are in commit 2f2b1ac.
|
||
def test_content_sha256_set_s3_on_outpost(self): | ||
# S3 on Outpost bucket names should behave the same way. | ||
config = Config(signature_version="s3v4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note here, if we're setting the signature_version, I'm not sure this would have caught the original bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as above: Generally agree with you, but this is in a pre-existing test class named TestS3SigV4
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addresses an issue where requests for some S3 API operations that should include the
x-amz-content-sha256
header do not have this header for S3 on Outpost. This issue was introduced in version 1.28.0 of botocore.