Support S3 Express One Zone #5268

tustvold · 2024-01-01T22:58:51Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2024-01-01T23:01:05Z

object_store/src/aws/credential.rs

 use std::collections::BTreeMap;
 use std::sync::Arc;
 use std::time::{Duration, Instant};
 use tracing::warn;
 use url::Url;

+#[derive(Debug, Snafu)]
+#[allow(clippy::enum_variant_names)]


Otherwise clippy complains about the same prefix for the variants, this is future proofing for when we add further variants for the other error propagation in this module that is currently very ad-hoc

tustvold · 2024-01-01T23:01:54Z

object_store/src/aws/client.rs

+                .with_sign_payload(self.config.sign_payload);
+
+        if self.session_token {
+            let token = HeaderName::from_static("x-amz-s3session-token");


This is not clearly documented anywhere I could find, I reverse engineered it from the boto library - https://github.com/boto/botocore/blob/develop/botocore/auth.py#L547

object_store/src/aws/builder.rs

tustvold · 2024-01-03T13:25:31Z

@Xuanwo perhaps you might be able to give this one a once over?

cozmo · 2024-01-05T07:11:38Z

Hey, I wanted to play around with some "real world" use cases of S3 Express 1Z serving Parquet, and this seemed like a good place to start. When I got a test project up and running with this branch, I noticed that the same datafusion queries executed against the same files in a normal bucket and a express1z bucket were taking roughly 2x as long with the express1z bucket.

When I started to dive into why this was, I noticed that (in my code at least), the SessionProvider fetch_token method was being executed for every call of GetClient get_request. I observed this by adding logs to both methods, and seeing them logged 1:1.

I'm new to this project, and I know this code is WIP, so it's very possible I was holding it wrong (does the caller need to set up credential caching somehow when initializing the object store?), and also very possible that I misunderstand how this code is supposed to be working. That said, I wanted to flag here in case it's unexpected that fetch_token is called that much.

Xuanwo

Overall, LGTM, but there are a few decision details I'd like to discuss.

Which naming to use in our project?

AWS S3 Express is the product name of this feature, but all it's docs are using directory bucket.

I feel that directory bucket is more easy to understand and much close to it's feature set: directory based, / as delimiter only, no start-after, ...

Should we expose `s3 express` to users directly?

AWS S3 Express is now exclusive to AWS. Its unique designs, such as the --x-s3 endpoint modifier, ensure that other vendors will not adopt a similar approach.

Should we offer s3 express as a configurable option to users, or simply provide additional configurations along with documentation to assist users in setting up a directory bucket?

We can internally determine if is_s3_directory_bucket by checking for --x-s3 in the endpoint and managing credential-related matters.

Xuanwo · 2024-01-05T07:26:50Z

object_store/src/aws/credential.rs

+static DATE_HEADER: HeaderName = HeaderName::from_static("x-amz-date");
+static HASH_HEADER: HeaderName = HeaderName::from_static("x-amz-content-sha256");
+static TOKEN_HEADER: HeaderName = HeaderName::from_static("x-amz-security-token");
+static AUTH_HEADER: HeaderName = HeaderName::from_static("authorization");


nit: Maybe we can use http::header::AUTHORIZATION?

Xuanwo · 2024-01-05T07:27:34Z

object_store/src/aws/credential.rs

@@ -555,20 +581,20 @@ struct AssumeRoleResponse {
 #[derive(Debug, Deserialize)]
 #[serde(rename_all = "PascalCase")]
 struct AssumeRoleResult {
-    credentials: AssumeRoleCredentials,
+    credentials: SessionCredentials,


Nice change, since AssumeRoleWithWebIdentity returns the same struct.

Xuanwo · 2024-01-05T07:29:21Z

object_store/src/aws/credential.rs

+        Ok(TemporaryToken {
+            token: Arc::new(creds.into()),
+            // Credentials last 5 minutes
+            expiry: Some(Instant::now() + Duration::from_secs(5 * 60)),


I'm a bit confused. Is there another consideration preventing us from using something like SessionCredentials.expiration - Duration::from_secs(600)? Default of the expiration could last 3600 seconds.

Temporary security credentials are scoped to the bucket and expire after 5 minutes.

https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateSession.html

Lesson learnt!

Xuanwo · 2024-01-05T07:32:48Z

object_store/src/aws/mod.rs

@@ -255,6 +255,16 @@ impl ObjectStore for AmazonS3 {
        prefix: Option<&Path>,
        offset: &Path,
    ) -> BoxStream<'_, Result<ObjectMeta>> {
+        if self.client.config.session_provider.is_some() {


The relationship between session_provider and S3 Express is not immediately clear at first glance. I'm not sure if it worth to add an is_s3_express_bucket, or more formally, is_s3_directory_bucket.

I'll add a private member function to make this more clear

tustvold · 2024-01-05T08:36:22Z

Naming is a good point, it is certainly unhelpful how AWS have chosen to document this. IMO it seems wrong to call something a directory bucket that doesn't actually have a concept of directories... That aside, I think it important to make it clear it is an AWS specific thing, given the number of S3-compatible stores. This is also the reason I'm hesitant to automatically match based on the bucket name, in case you aren't talking to S3.

Edit: although you raise a good point about the --x-s3 suffix...

tustvold · 2024-01-05T08:38:31Z

does the caller need to set up credential caching somehow when initializing the object store

Credentials are cached for 5 minutes and reused, is it possible you are creating a new object store instance per request, or making requests less frequently than this?

Edit: In fact you are totally correct, the timeout the TokenProvider is using to decide when to get new credentials is the same 5 minutes that the credentials are valid for... - nice spot 👍

Xuanwo · 2024-01-05T08:39:08Z

Naming is a good point, it is certainly unhelpful how AWS have chosen to document this. IMO it seems wrong to call something a directory bucket that doesn't actually have a concept of directories... That aside, I think it important to make it clear it is an AWS specific thing, given the number of S3-compatible stores. This is also the reason I'm hesitant to automatically match based on the bucket name, in case you aren't taking to S3.

Makes sense, LGTM to me now.

github-actions bot added the object-store Object Store Interface label Jan 1, 2024

Support S3 Express One Zone

b76f8e7

tustvold force-pushed the s3-express-support branch from da0603b to b76f8e7 Compare January 1, 2024 23:00

tustvold commented Jan 1, 2024

View reviewed changes

Fix endpoint

68088d0

tustvold commented Jan 2, 2024

View reviewed changes

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

Update object_store/src/aws/builder.rs

fe315fc

Xuanwo reviewed Jan 5, 2024

View reviewed changes

tustvold added 3 commits January 5, 2024 10:35

Fix credential caching

60d3305

Review feedback

7ee13b5

Clippy

cb3503b

tustvold merged commit f7101ec into apache:master Jan 5, 2024
13 checks passed

tustvold mentioned this pull request Jan 5, 2024

Support S3 Express One Zone #5140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support S3 Express One Zone #5268

Support S3 Express One Zone #5268

tustvold commented Jan 1, 2024

tustvold Jan 1, 2024

tustvold Jan 1, 2024

tustvold commented Jan 3, 2024

cozmo commented Jan 5, 2024 •

edited

Loading

Xuanwo left a comment

Xuanwo Jan 5, 2024

Xuanwo Jan 5, 2024

Xuanwo Jan 5, 2024

tustvold Jan 5, 2024

Xuanwo Jan 5, 2024

Xuanwo Jan 5, 2024

tustvold Jan 5, 2024

tustvold commented Jan 5, 2024 •

edited

Loading

tustvold commented Jan 5, 2024 •

edited

Loading

Xuanwo commented Jan 5, 2024

Support S3 Express One Zone #5268

Support S3 Express One Zone #5268

Conversation

tustvold commented Jan 1, 2024

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

tustvold commented Jan 3, 2024

cozmo commented Jan 5, 2024 • edited Loading

Xuanwo left a comment

Choose a reason for hiding this comment

Which naming to use in our project?

Should we expose s3 express to users directly?

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 Jan 5, 2024 • edited Loading

tustvold commented Jan 5, 2024 • edited Loading

Xuanwo commented Jan 5, 2024

cozmo commented Jan 5, 2024 •

edited

Loading

Should we expose `s3 express` to users directly?

tustvold commented Jan 5, 2024 •

edited

Loading

tustvold commented Jan 5, 2024 •

edited

Loading