Prevent infinite loops when handling S3 region redirects #1419
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
joguSD
approved these changes
Mar 23, 2018
| { | ||
| "category": "``s3``", | ||
| "type": "bugfix", | ||
| "description": "Fix bug where invalidate head_object requests would cause an infinite loop (alternate fix to `#1400 <https://github.com/boto/botocore/issues/1400>`__)" |
Codecov Report
@@ Coverage Diff @@
## develop #1419 +/- ##
==========================================
+ Coverage 80.69% 80.7% +<.01%
==========================================
Files 87 87
Lines 12131 12135 +4
==========================================
+ Hits 9789 9793 +4
Misses 2342 2342
Continue to review full report at Codecov.
|
kyleknap
approved these changes
Mar 23, 2018
| @@ -339,6 +339,51 @@ def test_resign_request_with_region_when_needed(self): | |||
| '?encoding-type=url') | |||
| self.assertEqual(calls[1].url, fixed_url) | |||
|
|
|||
| def test_resign_request_in_us_east_1(self): | |||
There was a problem hiding this comment.
It might be good to have the other case when the initial bad_request_response should resolve to a 400 on the user side (i.e. bad SSE-C key case). It looks like there is a integration test for this, but it would be nice if there was a functional or unit test as well.
There was a problem hiding this comment.
Added a test for this case in 68b3997
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.
This adds an alternate fix for #1409 . I've pulled in @joguSD's patch and added additional unit/integration tests.
The fix is to set a flag when we redirect the first time and verify we haven't previously redirected on subsequent requests. I also added an integration test for the specific case mentioned in #1400.
As for the regression caused from #1409, we need buckets with DNS propagated in order to actually test this, but I did write a one off integration test to verify this works as expected. I don't think it makes sense to include in our main integration test suite, I think the unit/functional tests suffice. If you're curious though, here's the test: https://gist.github.com/jamesls/9e1f0eb8e6b632f62ea5cb38131de2ae