Add option to force s3 virtual host addressing #655
Conversation
|
Looking at this now I have some initial high level feedback before digging into this: |
|
That sounds good. It would provide a precedence to prevent the Config file from becoming to bloated. Maybe name it to something more explicit like |
|
|
||
| # Determine if the user specified the use of virtual host addressing | ||
| # style for s3. | ||
| s3_addressing_style = 'default' |
There was a problem hiding this comment.
Can you move this out to a separate method? This method is getting pretty long.
|
Looks good, just had some small feedback. |
|
@kyleknap I would prefer s3 instead of s3_configuration. I think the fact that it's in the |
|
Ok that's fair. |
You can force virtual style addressing through the config file:
s3 =
addresing_style = virtual_host
You can also specify it in the Config object passed in when a client
is instantiated. The virtual host addressing no longer defaults to the
s3 global endpoint. Instead, it will use the endpoint url determined from
the region or if the endpoint url that is specified upon instantaition of
a client.
You now specify it as a dictionary where 'addressing_style' is a key and the value is a string representing the desired style.
Moved fix_s3_host to the client instantiation to make logic easier to build on.
kyleknap
force-pushed
the
dns-style-host
branch
from
5172308 to
53381e2
Compare
|
@jamesls |
kyleknap
added
pr/needs-review
| s3_configuration = scoped_config.get('s3') | ||
|
|
||
| # Next the client config value takes precedence | ||
| if client_config is not None: |
There was a problem hiding this comment.
That's an interesting point. Should we maybe be merging the configs if a user has both scoped config and client config? As we add more s3 options, it would mean that a customer would typically have to grab the s3 config from the session, merge their options on top, and then create a Config object.
|
Changes look good. Just wondering what we should be doing about merging config. It makes it slightly more complicated for the user once they have various S3 options in their config and they want to override only specific options, but I suppose it's not that hard for them to manually merge the s3 config dicts before creating a client. What do you think? |
Values in client config take precedence over value in scoped config
|
@jamesls |
|
|
Add option to force s3 virtual host addressing
|
This broke some CLI integration tests reverting. |
You can force virtual style addressing through the config file:
You can also specify it in the Config object passed in when a client is instantiated. The virtual host addressing no longer defaults to the s3 global endpoint. Instead, it will use the endpoint url determined from the region or if the endpoint url that is specified upon instantiation of a client:
If you force the virtual style addressing and the bucket name on an operation you call is not DNS compatible it will throw an error. Another important note to emphasize is that if you force the virtual style addressing, it will use the endpoint of the region you specified so you may hit PermanentRedirect errors. Otherwise if it is not forced, the requests will still be sent to the the global s3 endpoint.
As for testing, the s3 integration tests pass for both botocore and the CLI. I even tried running the integration tests with virtual style forced and the botocore tests passed and most of the CLI test passed (their were a few tests that randomly generated '.' in their name which would cause an error to be thrown as expected because the name is not DNS compatible).
Also, thanks to testing it in this fashion I fixed some bugs where if virtual style is forced and it is sigv4, we would get signature errors. We did not run into these bugs before because we would turn off virtual style if sigv4 is being used.
cc @jamesls @mtdowling @rayluo @JordonPhillips