Replace rusoto with custom implementation for AWS (#2176) #2352

tustvold · 2022-08-07T13:50:05Z

Which issue does this PR close?

Closes #2350
Part of #2176

Rationale for this change

See #2176

What changes are included in this PR?

Replaces rusoto with a custom implementation based on reqwest

Are there any user-facing changes?

Not that I'm aware of, but there is a possibility

tustvold · 2022-08-07T13:50:43Z

object_store/src/aws/credential.rs

+    &[DATE_HEADER, HASH_HEADER, TOKEN_HEADER, AUTH_HEADER];
+
+impl<'a> RequestSigner<'a> {
+    fn sign(&self, request: &mut Request) {


Everything about this process is ludicrous... TLS exists... Why oh why would you do this 😭

Big cooperation firewall/VPN stuff with TLS stripping 🙈

Previous comment about this being ludicrous still stands 😆

object_store/src/aws/credential.rs

tustvold · 2022-08-07T13:53:01Z

object_store/src/multipart.rs

-///
-/// Note: this does not use AsyncTrait as the lifetimes are difficult to manage
-pub(crate) trait CloudMultiPartUploadImpl {
+#[async_trait]


I took the opportunity to clean up this interface a bit, this isn't public and so this isn't a breaking change. I may split this out into another PR

object_store/src/multipart.rs

tustvold · 2022-08-07T15:05:46Z

Looks like I found a bug in the azure implementation 😅

Edit: fix in #2354

tustvold · 2022-08-08T14:31:31Z

object_store/src/aws/credential.rs

+}
+
+/// <https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-technical-overview.html>
+async fn web_identity(


I don't have a good way to integration test this, but I have tested it with some real EKS tokens and it seems to work

crepererum

Finished the first round. Good job! I think this will definitely pay off.

crepererum · 2022-08-09T07:48:58Z

object_store/src/client/pagination.rs

+use futures::Stream;
+use std::future::Future;
+
+/// Performs a paginated operation


This docstring says close to nothing 😁

Maybe add a few words on how a stream is constructed out of a state and an async function and what this provides over unfold and poll_fn (which also exists for streams). Esp. that the output is paginated as well, it is NOT flattened.

Furthermore, I would suggest renaming list to list_fn or something, because initially I thought list is A list -- not a function to list -- that somehow gets unrolled here.

crepererum · 2022-08-09T07:52:54Z

object_store/src/aws/mod.rs

+        Self::Generic {
+            store: "S3",
+            source: Box::new(err),
+        }


Shouldn't there be at least a NotFound error (and the generic test suite should probably assert that fetching an unknown object actually results in an "not found" error, not some generic one).

The NotFound variant is produced by the client

crepererum · 2022-08-09T07:57:02Z

object_store/src/aws/client.rs

+use bytes::{Buf, Bytes};
+use chrono::{DateTime, Utc};
+use percent_encoding::{utf8_percent_encode, AsciiSet, PercentEncode, NON_ALPHANUMERIC};
+use reqwest::{Client, Method, Response, StatusCode};


Suggested change

use reqwest::{Client, Method, Response, StatusCode};

use reqwest::{Client as ReqwestClient, Method, Response, StatusCode};

crepererum · 2022-08-09T07:57:55Z

object_store/src/aws/client.rs

+#[derive(Debug)]
+pub(crate) struct S3Client {
+    config: S3Config,
+    client: Client,


Suggested change

client: Client,

reqwest_client: ReqwestClient,

I was wondering what the client in a client in a client was.

crepererum · 2022-08-09T07:59:30Z

object_store/src/aws/credential.rs

+}
+
+impl CredentialExt for RequestBuilder {
+    /// Sign a request <https://docs.aws.amazon.com/general/latest/gr/sigv4_signing.html>


This doc should go the the trait, not the impl (or both if the the impl is special).

crepererum · 2022-08-09T08:01:48Z

object_store/src/aws/credential.rs

+    fn sign(self, credential: &AwsCredential, region: &str, service: &str) -> Self;
+}
+
+impl CredentialExt for RequestBuilder {


I'm always a bit torn when it comes to extension traits for foreign types. Sure the make the code more elegant (I can clearly see that that in client.rs, which now feels like rather normal reqwest client stuff with very few parameters) but looking at code that uses is often makes me wonder "what's that new/fancy method on this type I've worked with before?!". So you can leave it as it is, just don't overdo it 😉

Also: they are technically prone to break under semantic versioning, because upstream may be free to add new methods (not a breaking change) which then collide with this method name. Since your "use side" doesn't use a fully qualified name, the code will stop compiling due to ambiguity.

One thing to avoid both (my confusion and the potential breakage) would be to choose a more specific method name like aws_sign or s3_sign.

crepererum · 2022-08-09T08:07:46Z

object_store/src/aws/mod.rs

+/// Interface for [Amazon S3](https://aws.amazon.com/s3/).
+#[derive(Debug)]
+pub struct AmazonS3 {
+    client: Arc<S3Client>,


What's the reason that the S3Client is not just inlined into the store object? Feels a bit like a layer that isn't really necessary.

It helps with encapsulation, especially as many methods on ObjectStore map to some combination of actual upstream requests - e.g. the various types of Put request. Also made it easier to split into its own module

crepererum · 2022-08-09T08:09:52Z

object_store/src/aws/credential.rs

@@ -0,0 +1,526 @@
+// Licensed to the Apache Software Foundation (ASF) under one


This module makes me a bit unhappy. It's such a PITA that AWS requires all that stuff. However I see you clearly did your homework and it's documented and the tests work, so I just assume it does the right thing.

If you want me to double-check the code with the official docs, I can do that but I think it's not really required.

crepererum · 2022-08-09T08:10:58Z

object_store/src/aws/credential.rs

+
+/// Canonicalizes headers into the AWS Canonical Form.
+///
+/// <http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>


Suggested change

/// <http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>

/// <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>

.github/workflows/object_store.yml

tustvold · 2022-08-09T10:40:02Z

https://github.com/influxdata/influxdb_iox/pull/5342 has been deployed and appears to be working correctly, testing both the Instance Metadata credentials and the WebIdentity

alamb · 2022-08-09T20:48:11Z

I would like to request holding off merging this PR until we have done the official 0.4 object store release from this repo, tracked by ~~#2338~~ #2261

My rationale is that there are enough moving parts and we are (effectively) still testing some of this code in production so I don't want to accidentally introduce a regression that would require a new release quickly after the previous one

alamb · 2022-08-11T11:43:09Z

Sounds like the IOx 'testing in production' has gone very well ❤️

So to move this PR forward:

It seems like like there are a few more comments from @crepererum to address.
The 0.4.0 release is going along nicely,
I think we are on track to merge this over the weekend or early next week

tustvold · 2022-08-15T10:21:21Z

I intend to get this in this afternoon unless anybody would like longer to review

alamb · 2022-08-15T10:54:25Z

I don't have any objections, though I haven't reviewed this PR in detail either. I will confirm we deployed these changes in our IOx environment and they seem to be working great

cc @roeap

alamb · 2022-08-15T10:54:27Z

I don't have any objections, though I haven't reviewed this PR in detail either. I will confirm we deployed these changes in our IOx environment and they seem to be working great

cc @roeap

ursabot · 2022-08-15T11:32:21Z

Benchmark runs are scheduled for baseline = 47a2c21 and contender = 3f0e12d. 3f0e12d 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 Aug 7, 2022

tustvold commented Aug 7, 2022

View reviewed changes

object_store/src/multipart.rs Outdated Show resolved Hide resolved

tustvold mentioned this pull request Aug 7, 2022

Fix Copy from percent-encoded path (#2353) #2354

Merged

Replace rusoto (apache#2176)

c14db7f

tustvold force-pushed the replace-rusoto branch from 41872b2 to c14db7f Compare August 8, 2022 10:23

tustvold added 3 commits August 8, 2022 14:00

Add integration test for metadata endpoint

3c70707

Fix WebIdentity

7b23659

Fix doc

d1e694b

tustvold marked this pull request as ready for review August 8, 2022 14:20

tustvold commented Aug 8, 2022

View reviewed changes

Fix handling of multipart errors

cd4b25f

crepererum reviewed Aug 9, 2022

View reviewed changes

Use separate client for credentials

10ffea5

tustvold force-pushed the replace-rusoto branch from ed2704f to 10ffea5 Compare August 9, 2022 12:27

tustvold added 2 commits August 9, 2022 14:22

Include port in Host header canonical request

0f9d902

Fix doc link

f688002

roeap mentioned this pull request Aug 10, 2022

feat: add token provider authorization to azure store #2374

Merged

alamb changed the title ~~Replace rusoto with custom implementation (#2176)~~ Replace rusoto with custom implementation for AWS (#2176) Aug 11, 2022

tustvold added 2 commits August 15, 2022 09:34

Review feedback

33e570a

Merge remote-tracking branch 'upstream/master' into replace-rusoto

9863961

crepererum approved these changes Aug 15, 2022

View reviewed changes

tustvold merged commit 3f0e12d into apache:master Aug 15, 2022

alamb mentioned this pull request Aug 31, 2022

Release object_store 0.5.0 #2620

Closed

7 tasks

tustvold added the api-change Changes to the arrow API label Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace rusoto with custom implementation for AWS (#2176) #2352

Replace rusoto with custom implementation for AWS (#2176) #2352

tustvold commented Aug 7, 2022 •

edited

Loading

tustvold Aug 7, 2022

crepererum Aug 8, 2022

tustvold Aug 8, 2022

tustvold Aug 7, 2022

tustvold commented Aug 7, 2022 •

edited

Loading

tustvold Aug 8, 2022

crepererum left a comment

crepererum Aug 9, 2022

crepererum Aug 9, 2022

tustvold Aug 9, 2022

crepererum Aug 9, 2022

crepererum Aug 9, 2022

alamb Aug 11, 2022

crepererum Aug 9, 2022

crepererum Aug 9, 2022

crepererum Aug 9, 2022

tustvold Aug 9, 2022

crepererum Aug 9, 2022

crepererum Aug 9, 2022

tustvold commented Aug 9, 2022

alamb commented Aug 9, 2022 •

edited

Loading

alamb commented Aug 11, 2022

tustvold commented Aug 15, 2022

alamb commented Aug 15, 2022

alamb commented Aug 15, 2022

ursabot commented Aug 15, 2022

	use reqwest::{Client, Method, Response, StatusCode};
	use reqwest::{Client as ReqwestClient, Method, Response, StatusCode};

		@@ -0,0 +1,526 @@
		// Licensed to the Apache Software Foundation (ASF) under one

	/// <http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>
	/// <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>

Replace rusoto with custom implementation for AWS (#2176) #2352

Replace rusoto with custom implementation for AWS (#2176) #2352

Conversation

tustvold commented Aug 7, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Aug 7, 2022 • edited Loading

Choose a reason for hiding this comment

crepererum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Aug 9, 2022

alamb commented Aug 9, 2022 • edited Loading

alamb commented Aug 11, 2022

tustvold commented Aug 15, 2022

alamb commented Aug 15, 2022

alamb commented Aug 15, 2022

ursabot commented Aug 15, 2022

tustvold commented Aug 7, 2022 •

edited

Loading

tustvold commented Aug 7, 2022 •

edited

Loading

alamb commented Aug 9, 2022 •

edited

Loading