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

Add placeholder types for S3 Express and enable control flow to be redirected for S3 Express use case #3386

Merged
merged 22 commits into from
Feb 10, 2024

Conversation

ysaito1001
Copy link
Contributor

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:

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 IdentityResolvers. 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) 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.

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.
@ysaito1001 ysaito1001 requested a review from a team as a code owner January 25, 2024 02:47
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 and others added 2 commits January 29, 2024 10:53
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 and others added 6 commits February 7, 2024 10:11
…/customize/s3/S3ExpressDecorator.kt

Co-authored-by: John DiSanti <[email protected]>
_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@ysaito1001 ysaito1001 requested a review from jdisanti February 8, 2024 21:58
aws-sdk-rust-ci and others added 2 commits February 8, 2024 14:05
_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
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.

This CI step will check for subtle semver hazards with the `ConfigBag`
during release. Once all the runtime crates are independent, then this
check can be moved into normal CI.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Velfi and others added 2 commits February 9, 2024 17:19
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
#3322

## Description
<!--- Describe your changes in detail -->
I gave classifiers priorities but never actually sorted them. This PR
fixes that.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
I wrote a test

## 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]>
## Description
Via code inspection, we have identified that there is a potential bug in
`DEFAULT_BUFFER_TIME_JITTER_FRACTION`. Specifically, if the fraction
happens to be set to 1.0, we end up not respecting the buffer time for
cache refresh (see diagrams in #2335). This PR will cap the max fraction
value to 0.5 to avoid the problem.

## 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
smithy-rs codegen or runtime crates
- [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]>
@ysaito1001 ysaito1001 requested a review from a team as a code owner February 10, 2024 04:17
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001 ysaito1001 merged commit 39be6ce into ysaito/s3express Feb 10, 2024
41 checks passed
@ysaito1001 ysaito1001 deleted the s3express-add-placeholders branch February 10, 2024 04:42
ysaito1001 added a commit that referenced this pull request Feb 10, 2024
…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]>
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.

4 participants