Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow list-objects-v2 to run against an S3 Express bucket #3388

Merged
merged 35 commits into from
Feb 17, 2024

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Jan 29, 2024

Motivation and Context

Adds an implementation spike to allow list-objects-v2 (possibly others, haven't tested yet) to run against an S3 Express bucket.

Description

This PR implements two ingredients, S3ExpressIdentityProvider and S3ExpressSigner. S3ExpressIdentityProvider uses an internal S3 client to obtain an S3 Express session token that is passed to S3ExpressSigner. S3ExpressSigner then signs a request with that token, using effectively sigv4 but with session token omitted and an extra header added instead, x-amz-s3session-token.

In addition, this PR supports presigning for S3 Express. Similarly to signing headers, presigning for S3 Express excludes a query param X-Amz-Security-Token and instead uses X-Amz-S3session-Token for the signing query params. The following screeshot shows that a presigned URL from get_object works for an S3 Express bucket:

chain-provider-ext-timeout-2

Some implementation details:

  • Since S3ExpressIdentityProvider passes an S3 Express bucket name for S3's create_session API to obtain an S3 Express session token, it needs to obtain the bucket name from somewhere. S3ExpressIdentityProvider::ProvideCredentials I put previously did not have enough arguments for us to figure this out, so I switched to S3ExpressIdentityProvider::ResolveIdentity that takes enough arguments.
  • SigV4Signer::sign_http_request did not allow calling code to pass a configured SigningSettings; The signer needs to exclude a header x-amz-security-token and include x-amz-s3session-token. To make this happen, I made sigv4::extract_operation_config and sigv4::settings public APIs (previously private).
  • One area I haven't quite figured out yet is how to configure the inner S3 client to call create_session. The changes in this PR inherits runtime components & config bag from the "outer" S3 client, but customers may want to configure the inner S3 client in a more flexible manner (e.g. operation timeout).

Testing

To lock the behavior at this time, I added a connection recording test for list-objects-v2.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ysaito1001 and others added 9 commits January 24, 2024 20:45
This commit adds supporting types for S3 Express. They are provided via
the S3 customization and defining Rust types live in `aws-inlineable`.
This commit updates parts of the orchestrator so that when an S3 Express
bucket name is passed, control flow will be directed to placeholder types
added in the previous commit.
…/customize/s3/S3ExpressDecorator.kt

Co-authored-by: John DiSanti <[email protected]>
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@ysaito1001 ysaito1001 marked this pull request as ready for review January 30, 2024 15:58
@ysaito1001 ysaito1001 requested a review from a team as a code owner January 30, 2024 15:58
Copy link

github-actions bot commented Feb 8, 2024

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Base automatically changed from s3express-add-placeholders to ysaito/s3express February 10, 2024 04:42
@ysaito1001 ysaito1001 requested a review from a team as a code owner February 10, 2024 04:42
ysaito1001 and others added 4 commits February 9, 2024 23:09
…directed for S3 Express use case (#3386)

## Motivation and Context
This PR is the first in the series to support the S3 Express feature in
the Rust SDK. The work will be done in the feature branch, and once it
is code complete, the branch will be merged to main.

## Description
This PR adds placeholder types for S3 Express and enables control flow
to be redirected for S3 Express use case. For instance, if we run the
following example code against a generated SDK from this PR:
```rust
let shared_config = aws_config::from_env().region(aws_sdk_s3::config::Region::new("us-east-1")).load().await;
let client = aws_sdk_s3::Client::new(&shared_config);
client.list_objects_v2().bucket("testbucket--use1-az4--x-s3";).send().await.unwrap();
```
it will end up
```
thread 's3_express' panicked at 'not yet implemented', /Users/awsaito/src/smithy-rs/aws/sdk/build/aws-sdk/sdk/s3/src/s3_express.rs:104:13
```
which points to
```
impl ProvideCredentials for DefaultS3ExpressIdentityProvider {
    fn provide_credentials<'a>(&'a self) -> aws_credential_types::provider::future::ProvideCredentials<'a>
    where
        Self: 'a,
    {
        todo!() <---
    }
}
```

### Implementation decisions
- `DefaultS3ExpressIdentityProvider` has an accompanying identity cache.
That identity cache cannot be configured by customers so it makes sense
for the provider itself to internally own it. In that case, we do NOT
want to use the identity cache stored in `RuntimeComponents`, since it
interferes with the S3 Express's caching policy. To that end, I added an
enum `CacheLocation` to `SharedIdentityResolver` (it already had the
`cache_partition` field so it was kind of aware of caching).
- Two reasons why `CacheLocation` is added to `SharedIdentityResolver`,
but not to individual, concrete `IdentityResolver`s. One is
`SharedIdentityResolver` was already cache aware, as mentioned above.
The other is that it is more flexible that way; The cache location is
not tied to a type of identity resolver, but we can select it when
creating a `SharedIdentityResolver`.
- I considered but did not add a field `cacheable` to `Identity` since I
wanted to keep `Identity` as plain data, keeping the concept of
"caching" somewhere outside.
- I've added a separate `Config` method,
`set_express_credentials_provider`, to override credentials provider for
S3 Express. There are other SDKs (e.g.
[Ruby](https://www.rubydoc.info/gems/aws-sdk-s3/Aws/S3/Client)) that
follow this style and it makes it clear to the customers that this is
the method to use when overriding the express credentials provider. The
existing `set_credentials_provider`, given its input type, cannot tell
whether a passed-in credentials provider is for a regular `sigv4` or for
S3 Express.

## Testing
Only verified that control flow could be altered for an S3 Express use
case, as shown above. Further testing will be added in subsequent PRs.

## Checklist
I am planning to include in `CHANGELOG.next.toml` a user guide for S3
Express once the feature branch `ysaito/s3express` is ready to be merged
to main.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: AWS SDK Rust Bot <[email protected]>
Co-authored-by: AWS SDK Rust Bot <[email protected]>
Co-authored-by: Zelda Hessler <[email protected]>
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001 ysaito1001 requested a review from jdisanti February 12, 2024 18:00
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a little confused on the excluded headers/params special casing, as you'll find in my comments below. We should put some more though into how this exclusion is done.

Otherwise, just minor comments.

@ysaito1001 ysaito1001 requested a review from jdisanti February 15, 2024 22:36
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one concern.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 🚀

@ysaito1001 ysaito1001 merged commit 486b91d into ysaito/s3express Feb 17, 2024
41 checks passed
@ysaito1001 ysaito1001 deleted the s3express-allow-list-objects-v2-to-run branch February 17, 2024 02:33
github-merge-queue bot pushed a commit that referenced this pull request Mar 11, 2024
## Motivation and Context
Allows the Rust SDK to use [S3 Express One
Zone](https://aws.amazon.com/s3/storage-classes/express-one-zone/)

## Description
The PR adds the said S3-specific functionality to the Rust SDK. The code
changes have already been reviewed by previous sub PRs, but it's worth
going through them again as a whole:
- #3386
- #3388
- #3390
- #3432
- #3433
- #3459
- #3457
- #3462

In addition to the PRs above, commit eebe8af increases the canary
lambda's memory size to 512MB from 128MB (also makes it configurable
through a command line arg for `canary-runner`). By default, lambda's
allowed memory size is 128MB but with the addition of `canary-wasm` in
main, canary lambda's memory usage will be 152MB, causing the lambda to
be killed by a signal during runtime. The commit addresses that issue.

## Testing
- Unit tests in
[aws/rust-runtime/aws-inlineable/src/s3_express.rs](https://github.com/smithy-lang/smithy-rs/blob/7f8c28b7038372927ec6196eff88384452f908dd/aws/rust-runtime/aws-inlineable/src/s3_express.rs)
- Integration tests in
[aws/sdk/integration-tests/s3/tests/express.rs](https://github.com/smithy-lang/smithy-rs/blob/7f8c28b7038372927ec6196eff88384452f908dd/aws/sdk/integration-tests/s3/tests/express.rs)
- Canary in smithy-rs#3462

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: AWS SDK Rust Bot <[email protected]>
Co-authored-by: AWS SDK Rust Bot <[email protected]>
Co-authored-by: Zelda Hessler <[email protected]>
Co-authored-by: Russell Cohen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants