Handle S3 virtual host request type #2782

askoa · 2022-09-27T04:02:54Z

Which issue does this PR close?

Are there any user-facing changes?

Additional input parameter to determine if the virtual host request type should be use for aws S3.

tustvold · 2022-09-28T09:37:21Z

This could be my mistake, but running the integration tests with this change I get an error

called `Result::unwrap()` on an `Err` value: Generic { store: "S3", source: ListRequest { source: Error { retries: 0, message: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message><Key>raphael-test-bucket-e27e29e3</Key><RequestId>579ZMFJ7XFMM0CYV</RequestId><HostId>gdpmDPN2NW2o7FBJ6VG8xM4PTSd5xtuVWXlE8WT8LdznRBUAgTB3zlE7KM66YsU3r2ge2jRHCto=</HostId></Error>", source: reqwest::Error { kind: Status(404), url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("raphael-test-bucket-e27e29e3.s3.eu-west-2.amazonaws.com")), port: None, path: "/raphael-test-bucket-e27e29e3", query: Some("list-type=2"), fragment: None } } } } }

The bucket called raphael-test-bucket-e27e29e3 does exist in eu-west-2 so I'm not sure what is going on here?

askoa · 2022-09-28T12:48:28Z

This could be my mistake, but running the integration tests with this change I get an error

called `Result::unwrap()` on an `Err` value: Generic { store: "S3", source: ListRequest { source: Error { retries: 0, message: "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message><Key>raphael-test-bucket-e27e29e3</Key><RequestId>579ZMFJ7XFMM0CYV</RequestId><HostId>gdpmDPN2NW2o7FBJ6VG8xM4PTSd5xtuVWXlE8WT8LdznRBUAgTB3zlE7KM66YsU3r2ge2jRHCto=</HostId></Error>", source: reqwest::Error { kind: Status(404), url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("raphael-test-bucket-e27e29e3.s3.eu-west-2.amazonaws.com")), port: None, path: "/raphael-test-bucket-e27e29e3", query: Some("list-type=2"), fragment: None } } } } }

The bucket called raphael-test-bucket-e27e29e3 does exist in eu-west-2 so I'm not sure what is going on here?

The error message says NoSuchKey. From the error message I see that the string raphael-test-bucket-e27e29e3 is used both as bucket and key. Are you passing different argument for key? Does that key exist? The format is https://bucket-name.s3.region-code.amazonaws.com/key-name. If you are passing a different key argument and if that exist then let me know. There might be some problem with the code.

tustvold · 2022-09-28T12:50:59Z

I'm just running the standard integration tests, with the new environment variable set to "true"... Is it possible that the request URI is still including the bucket when it doesn't need to?

askoa · 2022-09-28T13:01:59Z

I'm just running the standard integration tests, with the new environment variable set to "true"... Is it possible that the request URI is still including the bucket when it doesn't need to?

Yes. It looks like I have to update the code in multiple places. I'll revise this PR.

askoa · 2022-09-28T15:50:00Z

@tustvold when an endpoint is given, should we assume it to comply with virtual_hosted_request_style? More precisely, if the endpoint is given and virtual_hosted_request_style is set to true, should we append bucket name at the end?

arrow-rs/object_store/src/aws/mod.rs

Lines 420 to 428 in 7639f28

    
               /// Sets the endpoint for communicating with AWS S3. Default value 
        
               /// is based on region. 
        
               /// 
        
               /// For example, this might be set to `"http://localhost:4566:` 
        
               /// for testing against a localstack instance. 
        
               pub fn with_endpoint(mut self, endpoint: impl Into<String>) -> Self { 
        
                   self.endpoint = Some(endpoint.into()); 
        
                   self 
        
               }

Edit: I am updating the PR assuming if the input endpoint is provided then we assume path style and append bucket at the end. Let me know if this needs to change.

object_store/src/aws/client.rs

tustvold · 2022-09-30T09:53:36Z

I am updating the PR assuming if the input endpoint is provided then we assume path style and append bucket at the end

I would expect the following behaviour:

If an endpoint is provided and virtual_hosted_request_style - assume bucket already present in provided endpoint
If no endpoint is provided and virtual_hosted_request_style - construct bucket_endpoint with bucket name prefix
If an endpoint is provided and not virtual_hosted_request_style - include bucket in path

askoa · 2022-09-30T15:42:24Z

I would expect the following behaviour:

If an endpoint is provided and virtual_hosted_request_style - assume bucket already present in provided endpoint

If no endpoint is provided and virtual_hosted_request_style - construct bucket_endpoint with bucket name prefix

If an endpoint is provided and not virtual_hosted_request_style - include bucket in path

@tustvold I updated the code. Can you please take a look?

tustvold

Thank you, just some minor nits

tustvold · 2022-10-01T11:50:25Z

object_store/src/aws/mod.rs

+        let bucket_endpoint: String;
+
+        //If `endpoint` is provided then its assumed to be consistent with
+        // `virutal_hosted_style_request`. i.e. if `virtual_hosted_request_style` is true then


Suggested change

// `virutal_hosted_style_request`. i.e. if `virtual_hosted_request_style` is true then

// `virtual_hosted_request_style`. i.e. if `virtual_hosted_request_style` is true then

virtual_hosted_style_request directly comes form AWS page defining it. See https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html. I am not inclined to make this change.

Edit: I see the problem now. I think I have to change the field name. I think it should be virtual_hosted_style_request in all places. Will make that change.

object_store/src/aws/mod.rs

ursabot · 2022-10-01T15:11:39Z

Benchmark runs are scheduled for baseline = 3999c77 and contender = 0052d25. 0052d25 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the object-store Object Store Interface label Sep 27, 2022

askoa marked this pull request as draft September 28, 2022 13:02

askoa marked this pull request as ready for review September 28, 2022 16:38

tustvold reviewed Sep 30, 2022

View reviewed changes

object_store/src/aws/client.rs Outdated Show resolved Hide resolved

tustvold reviewed Sep 30, 2022

View reviewed changes

object_store/src/aws/client.rs Show resolved Hide resolved

askoa added 7 commits September 30, 2022 10:12

include s2 virtual host request type

51d3041

formatting changes

9955040

fix issues highlighted in PR comments

bbed7d9

initialize bucket_endpoint

fdf317c

some imporments on endpoint initialization

9c4cb4c

fix issue in initalizing bucket_endpoint

88b7fb7

incorporating PR comments

3f49918

tustvold approved these changes Oct 1, 2022

View reviewed changes

askoa added 3 commits October 1, 2022 08:48

incorporate PR comments

93084c7

Merge branch 'master' of https://github.com/apache/arrow-rs into s3-url

512038f

fix typo in comment

25258f1

tustvold merged commit 0052d25 into apache:master Oct 1, 2022

askoa deleted the s3-url branch December 1, 2022 15:10

jiacai2050 mentioned this pull request Mar 9, 2023

Object Store S3 Alibaba Cloud OSS support #2777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle S3 virtual host request type #2782

Handle S3 virtual host request type #2782

askoa commented Sep 27, 2022

tustvold commented Sep 28, 2022

askoa commented Sep 28, 2022

tustvold commented Sep 28, 2022 •

edited

Loading

askoa commented Sep 28, 2022

askoa commented Sep 28, 2022 •

edited

Loading

tustvold commented Sep 30, 2022 •

edited

Loading

askoa commented Sep 30, 2022

tustvold left a comment •

edited

Loading

tustvold Oct 1, 2022 •

edited

Loading

askoa Oct 1, 2022 •

edited

Loading

ursabot commented Oct 1, 2022

	// `virutal_hosted_style_request`. i.e. if `virtual_hosted_request_style` is true then
	// `virtual_hosted_request_style`. i.e. if `virtual_hosted_request_style` is true then

Handle S3 virtual host request type #2782

Handle S3 virtual host request type #2782

Conversation

askoa commented Sep 27, 2022

Which issue does this PR close?

Are there any user-facing changes?

tustvold commented Sep 28, 2022

askoa commented Sep 28, 2022

tustvold commented Sep 28, 2022 • edited Loading

askoa commented Sep 28, 2022

askoa commented Sep 28, 2022 • edited Loading

tustvold commented Sep 30, 2022 • edited Loading

askoa commented Sep 30, 2022

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

tustvold Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

askoa Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

ursabot commented Oct 1, 2022

tustvold commented Sep 28, 2022 •

edited

Loading

askoa commented Sep 28, 2022 •

edited

Loading

tustvold commented Sep 30, 2022 •

edited

Loading

tustvold left a comment •

edited

Loading

tustvold Oct 1, 2022 •

edited

Loading

askoa Oct 1, 2022 •

edited

Loading