-
Notifications
You must be signed in to change notification settings - Fork 48
feat(stream_io): Improve object storage config handling #189
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
cca2079
fix(stream_io): Use correct addressing style for CAIOS endpoints
Eta0 7a762df
docs: Update the changelog to mention the addressing style logic change
Eta0 9690542
feat(stream_io): Support Boto3's default credential resolution
Eta0 d007355
feat(stream_io): Support the `use_https` flag in `.s3cfg`
Eta0 6ecf0ab
docs: Update the changelog to note the new `use_https` config support
Eta0 86a9e3f
feat(stream_io): Respect endpoint env vars and Boto3 endpoint configs
Eta0 c6a5a20
fix(serialization): Fix large writes on some non-Linux platforms
Eta0 09acd08
fix(serialization): Check buffer size correctly in `_pwrite_compat`
Eta0 235a1e0
fix(stream_io): Use a subclass instead of method wrappers for uploads
Eta0 8dfb67d
feat(stream_io): Change HTTP(S) handling for Boto3-configured endpoints
Eta0 c92ccaa
docs: Update the changelog to note the Python 3.12+ `open_stream` fix
Eta0 376adcc
tests: Fix conflict between `moto` mocks and `boto3` parsed configs
Eta0 be337a0
tests: Disable Boto3's config and credential file parsing during tests
Eta0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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.
Really not a fan of us hardcoding these like this (or the ord1 defaults we have had elsewhere for some time). But can be convinced this is ok.
For reference to those other hardcoded values that should be either A) dropped, or B) changed to our current object storage offering.
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.
I'm pretty sure in
boto3's source tree there is a large data segment that has these sorts of hardcoded compatibility checks for a huge set of different AWS endpoints, which is how Boto3's "magically pick the right one" feature works. Unless/until we upstream a change to support our object storage endpoints correctly, which I'm not sure they'd accept, this seems to me like a fair place to put a workaround.The other hardcoded values are necessary for the default
s3://tensorizedbucket to "just work" since they're in that location. I'd like to change it to the newer endpoint in release 3.0.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.
If this is a pattern already used in boto3 then probably fine... although we are not automatically changing the
force_httpbased on if we are usingcwlota.comor not which would fall into the same pattern as what we are doing here. Same points apply on both sides there as well as if a user configures it correctly it will work, but by default with a minimal amount of configuring it will not.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.
I am nervous about defaulting
force_http=Truefor thecwlota.comendpoint on the basis that it could transmit a presigned URL as part of a request over an unknown network in cleartext if it is used somewhere wherecwlota.comdoesn't resolve to a local network (so not a CW datacenter, maybe from someone's own machine where they're trying to test something), although I think the presigned URL would only be valid for use by someone who can accesscwlota.comendpoints. I'd rather leave that one specifically as something that needs to be declared explicitly in the 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.
As long as it works correctly when
http://cwlota.comis in the config or environment variables as the endpoint then I don't strongly think it needs to have something explicitly forcing it here.