Fixed force_virtual_addressing problem #1923
Conversation
| @@ -408,7 +408,7 @@ def _initialize_oss_fs(self) -> FileSystem: | |||
| "access_key": get_first_property_value(self.properties, S3_ACCESS_KEY_ID, AWS_ACCESS_KEY_ID), | |||
| "secret_key": get_first_property_value(self.properties, S3_SECRET_ACCESS_KEY, AWS_SECRET_ACCESS_KEY), | |||
| "session_token": get_first_property_value(self.properties, S3_SESSION_TOKEN, AWS_SESSION_TOKEN), | |||
| "region": get_first_property_value(self.properties, S3_REGION, AWS_REGION), | |||
| "force_virtual_addressing": property_as_bool(self.properties, S3_FORCE_VIRTUAL_ADDRESSING, True), | |||
There was a problem hiding this comment.
I don't think we can remove region?
There was a problem hiding this comment.
Looks like it breaks the tests:
# For oss scheme, user provided region is used instead
> assert pyarrow_file_io.new_input(f"oss://{bucket_region[0]}/path/to/file")._filesystem.region == user_provided_region
E AssertionError: assert 'us-east-1' == 'ap-southeast-1'
E - ap-southeast-1
E + us-east-1
There was a problem hiding this comment.
I don't think we can remove
region?
For OSS authentication, it's not really necessary since they only need endpoint, but perhaps you're right that it's nicer to keep it.
There was a problem hiding this comment.
@helmiazizm Thoughts on changing the default?
Also, below we have False the default:
if self.properties.get(S3_FORCE_VIRTUAL_ADDRESSING) is not None:
client_kwargs["force_virtual_addressing"] = property_as_bool(self.properties, S3_FORCE_VIRTUAL_ADDRESSING, False)There was a problem hiding this comment.
Unlike S3 authentication, virtual addressing is mandatory in OSS. Not having it set to True will always throw access denied error, hence why setting it to True by default would make authentication easier for users in case if they ever forget to set s3.force-virtual-addressing parameter to True. I also don't think this setting for OSS will go away anytime soon as other S3-compatible object storages are also using the same method, so this change should be safe.
I still keep the property_as_bool function to make the authentication feel as graceful in case if users insist on setting it to False.
There was a problem hiding this comment.
@helmiazizm Thanks for the explanation, as I don't have any hands-on experience with Alibaba Cloud. Lets move this forward 👍
…ssing is set to True by default for OSS authentication
Rationale for this change
This fix addressed issue #1922 and changed the behavior for both
_oss_fsand_s3_fsto be able to parses3.force-virtual-addressingcorrectly.Are these changes tested?
No unit tests are given to this change yet
Are there any user-facing changes?
No