Migrate from rusoto to aws-sdk-rust #849

josb · 2022-07-03T19:03:49Z

Fixes #575. Posting this for feedback.

I could not get a new Region struct to be created with v as its value, kept running into lifetime issues with value.

Perhaps something like this is needed to get HTTP status codes, as with S3WithCode(StatusCode, String).

codecov-commenter · 2022-07-03T19:09:58Z

Codecov Report

Base: 64.91% // Head: 64.80% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (d0c1d2a) compared to base (fb0734a).
Patch coverage: 34.22% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
- Coverage   64.91%   64.80%   -0.11%     
==========================================
  Files          68       68              
  Lines       11790    11755      -35     
==========================================
- Hits         7653     7618      -35     
  Misses       4137     4137

Impacted Files	Coverage Δ
.../symbolicator-service/src/services/download/gcs.rs	`42.36% <0.00%> (ø)`
...mbolicator-service/src/services/download/sentry.rs	`22.11% <0.00%> (ø)`
...s/symbolicator-service/src/services/download/s3.rs	`42.51% <30.23%> (+2.34%)`	⬆️
crates/symbolicator-sources/src/sources.rs	`88.20% <62.50%> (-3.82%)`	⬇️
...symbolicator-service/src/services/download/http.rs	`82.51% <100.00%> (ø)`
.../symbolicator-service/src/services/download/mod.rs	`87.03% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

flub

thanks for picking this up again!

flub · 2022-07-04T09:04:58Z

crates/symbolicator/src/services/download/mod.rs

-    #[error("S3 error code: {1} (http status: {0})")]
-    S3WithCode(StatusCode, String),
+    #[error("aws-sdk: failed to fetch data from S3")]
+    S3SDK(#[from] aws_smithy_http::byte_stream::Error),


I believe this should be S3Sdk? Doesn't clippy complain about this?

I don't see any complaints from Clippy. It's only used here. Do you want me to rename it?

yeah, let's rename this

crates/symbolicator/src/services/download/mod.rs

crates/symbolicator/src/services/download/s3.rs

flub · 2022-07-04T09:11:37Z

crates/symbolicator/src/services/download/s3.rs

-                    self.create_s3_client(provider, region)
+                    let provider = LazyCachingCredentialsProvider::builder()
+                        .load(provide_credentials_fn(|| async {
+                            aws_config::ecs::EcsCredentialsProvider::builder()


we should check with the folks who added the AutoRefreshingProvider and ask them to test this to see if it works equivalently. I recall at some point they had an outage around this. git blame should point in the right direction.

I'll investigate this. Once I have this branch working well enough, I can also test this myself to some extent.

This is still pending me looking into this.

I am keeping an eye on the seemingly-related awslabs/aws-sdk-rust#629.

flub · 2022-07-04T09:16:58Z

crates/symbolicator/src/services/download/s3.rs

+                tracing::debug!("Skipping response from s3://{}/{}: {}", bucket, &key, err0);
+                return match &err0 {
+                    ConstructionFailure(err) => {
+                        println!("ERROR: ConstructionFailure: {:?}", err);


looks like you're still debugging all this. But you could capture all in a single match arm and maybe capture it as a sentry error before throwing cancelled up the chain?

Sounds like a good idea but I am not sure how to go about that, any pointers? Doesn't it require access to a Sentry installation and corresponding API keys?

Looking for some guidance here as I know nothing about Sentry. SOme of these can probably grouped into a single response but the mapping from Rusoto errors to AWS SDK errors isn't clear to me (although granted I have not spent much time investigating this yet).

Also, the Rusoto code does some XML parsing of the AWS responce, I wonder how much of that is still required with the AWS SDK.

flub · 2022-07-04T09:20:03Z

crates/symbolicator/src/sources.rs

+            let pos = AWS_REGIONS.iter().position(|&x| x == value);
+            match pos {
+                Some(_) => Ok(Region::new(String::from(value))),
+                None => Err(E::custom(format!("unknown region: {}", value))),


IIUC this means we need to updated our code every time they add a region? That might not be ideal?

I updated this code to use the EC2 describe_regions function and cache the result. Things I don't like about my solution:

I'm calling tokio::runtime::Runtime::new() to grab a runtime so I can call block_on on it. This is needed because the describe_regions call uses an async send wrapper. Would it not be more appropriate to use one of the existing runtimes created in server.rs?

I'm creating a shared_config using aws_config::from_env().load().await; not even sure this will work, given that it should really use the credentials_provider (Container or Static) associated with the current source. Thoughts?

Also, does the use of the #[cached] macro look OK?

Ugh, this does indeed break the unit tests. Have to rethink this, because we need the region list during serde config parsing yet we won't have the credentials until after parsing. And there's no way to get the region list anonymously beforehand so we can pre-populate the list.

can you just accept any string in serde and just deal with the error once you try and use an invalid region?

Yes, I'll do that.

By the way, Rusoto currently also hardcodes the list of known regions, so if a new region is added, a recompile is needed anyway once Rusoto is updated to include it.

How about I add a simple test at startup to see if sts get-caller-identity works and if it fails, error out with an appropriate error message?

flub · 2022-07-04T09:21:05Z

crates/symbolicator/src/sources.rs


        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
-            formatter.write_str("string or tuple")
+            formatter.write_str("string")


is this type change going to be a problem for some configurations?

Yes; custom regions, which use tuples per the docs, don't currently work, as the AWS Rust SDK doesn't support them.

ouch, that's quite a regression. this custom region support was also contributed by someone who needed it so it this would probably break for them. there is no way at all to support this? the exact way of configuring doesn't necessarily have to stay the same, but somehow supporting it would be needed i think

I don't think this is in-scope for the AWS SDK, at any rate it doesn't support it today so if we want to offer this functionality we'll have to add it ourselves. :-/

Swatinem · 2022-11-21T11:08:07Z

I yes, we introduced moka for some in-memory LRU caches like the client_cache. It has a moka::future variant that has almost the same API that can accept futures.

Swatinem

This does look very good, thanks for sticking with it for so long!

The only missing thing would be to correctly map the errors. But I wouldn’t spend too much effort on that.
If we correctly flag connection timeout and notfound, we should be good. everything else can be an opaque S3Error.

crates/symbolicator-sources/src/sources.rs

Swatinem · 2022-11-21T14:29:21Z

crates/symbolicator-sources/src/sources.rs

                assert_eq!(cfg.source_key.access_key, "the-access-key");
                assert_eq!(cfg.source_key.secret_key, "the-secret-key");
            }
            _ => unreachable!(),
        }
    }

+    /*


we should probably continue supporting custom regions. I commented above

crates/symbolicator-sources/Cargo.toml

crates/symbolicator-service/src/services/download/s3.rs

Swatinem · 2022-11-21T15:24:13Z

crates/symbolicator-service/src/services/download/s3.rs

+            .bucket(bucket.clone())
+            .key(key.clone())
+            .send();
+
+        let source = RemoteDif::from(file_source.clone());


I believe if you clean up all the println!s below, you can avoid these clones as well.

Fixed by converting to clones to references. I'll deal with the println!s separately.

To get rid of the file_source.clone() I implemented a trait, please let me know if I should just keep the existing approach.

Swatinem · 2022-11-21T15:26:41Z

crates/symbolicator-service/src/services/download/s3.rs

@@ -264,6 +257,7 @@ impl S3Downloader {
    }
 }

+/*


I believe you can leave these tests in, they should be noops in case our credentials are not set.

I'll have to spend some time to convert the tests from rusoto to aws-sdk-rust, please stay tuned.

The tests now compile.

Swatinem · 2022-11-21T15:39:29Z

If the tests are giving troubles due to credentials, I can also pull this into a local branch manually and merge from there, that way it should have the right test credentials.

@Swatinem

…te a new string which is expensive Thanks @Swatinem

… directly

Rusoto hardcoded the available regions, aws-sdk-rust does not. But there's no way to obtain the current list of valid regions without making API calls, which means having working credentials and either defaulting to some region (like us-east-1) or having the user specify a valid region anyway.

josb · 2022-11-25T23:58:38Z

At this point, as far as I know the major issues are:

Custom region support; not sure how to go about this.
SdkError<GetObjectError> handling in download_source(); I might be able to clean this up a bit but was hoping for some better guidance from [guide section]: Error handling awslabs/aws-sdk-rust#191.
The failing no_permission integration test, related to the previous item.

Anything else you can think of?

Swatinem · 2022-11-28T10:30:20Z

Custom region support; not sure how to go about this.

I believe its okay to be as lenient as possible. People will get NotFound / NoPermission if they provide an invalid region.

josb · 2022-11-28T22:15:35Z

BTW by "custom region support" I mean this:

In order to use a custom region for an S3 compatible service such as Ceph or minio, specify a tuple: ["custom-region-name", "http://minio-address/"].

My patch took away support for this because unlike Rusoto, the Rust AWS SDK doesn't support a Custom Region variant. Perhaps this can be implemented by wrapping the AWS SDK Regionstruct in an enum with a variant for the regular Region and one for the custom one; not sure how much work that would be.

Swatinem · 2022-11-29T10:53:20Z

Hm, I don’t think we really support those. But I could be wrong. First time I read about it.

Unlike Rusoto, the Rust AWS SDK doesn't hardcode the currently known regions, and no validation is provided. API requests with invalid region names will fail.

josb · 2022-12-03T18:29:15Z

This discussion seems relevant; the answer given suggests that a custom endpoint resolver can be used, as I guess the region is ultimately used to synthesize the URI of the wanted service endpoint.

Swatinem · 2022-12-05T09:37:31Z

Ahhh, now I understand what this is about. I should have read the comment more closely (which needs updating btw :-)

This is a Visitor that forwards string types to rusoto_core::Region's FromStr impl and forwards tuples to rusoto_core::Region's Deserialize impl.

That enum variant indeed consists of name + endpoint:

https://docs.rs/rusoto_core/latest/rusoto_core/enum.Region.html#variant.Custom

I think this is where my confusion was coming from.

I believe in this case we can mimic that behavior and have a:

enum Region {
  Builtin(String),
  Custom {
    name: String,
    endpoint: String,
  }
}

Then on usage one would have to match to then create the appropriate endpoint.

Swatinem · 2022-12-28T12:39:38Z

Thanks @josb for the great work here!

I pulled in your changes and created a new PR: #954

That merges the latest changes, adds back support for custom regions and tries to clean up the error handling a bit, though the error handling from the upstream SDK is truely horrific :-(

Jos Backus added 16 commits June 6, 2022 10:58

WIP

92be891

Make this almost compile

fc598d3

Don't hold lock over await points

776d60c

Comment cleanup and rustfmt

29d6207

Rename, this holds a client

9d4ec9a

Package updates

d2c0592

Fix/work around some Region-related build errors

7648631

Use position() on a const array

a7a1db0

I could not get a new Region struct to be created with v as its value, kept running into lifetime issues with value.

Fix stream error type

a220fdd

Make compile, with 7 warnings about err which is unused

a9bd560

Fix warnings

bba5cc3

Remove http_client, use default shipping with aws-sdk-rust (reqwest)

78b05de

Perhaps something like this is needed to get HTTP status codes, as with S3WithCode(StatusCode, String).

Automatically refresh ECS container credentials

159c5fe

Remove obsolete commented-out code

c414417

Merge branch 'master' into task/aws-sdk-rust-migration

e3d89fa

Upgrade crates

b2e0adb

josb requested a review from a team July 3, 2022 19:03

josb marked this pull request as draft July 3, 2022 19:04

josb changed the title ~~WIP: Migrate from rusoto to aws-sdk-rust~~ Migrate from rusoto to aws-sdk-rust Jul 3, 2022

Add changelog entry

186b7a4

Jos Backus added 2 commits July 3, 2022 12:24

Clippy fixes

eafb09e

Remove superfluous qualifiers

58fe855

flub reviewed Jul 4, 2022

View reviewed changes

Jos Backus added 6 commits July 4, 2022 11:48

Dynamically retrieve and cache list of EC2 regions

008cb1b

Remove obsolete commented-out code

6212154

Group Failures and Errors together

c4a269e

Get rid of wildcard import

199903d

No need for aws_regions() to be async

afed528

Improve error message

9776fbd

Jos Backus added 2 commits November 18, 2022 16:53

cargo fmt

4d246a2

Remove rusoto crates, update lock file

ce77bff

Swatinem reviewed Nov 21, 2022

View reviewed changes

Jos Backus added 11 commits November 23, 2022 15:29

Merge branch 'master' into task/aws-sdk-rust-migration

ae98e6f

Accommodate the async AWS SDK credentials providers

42c9a31

aws_types::region::Region implements Hash directly, no need to alloca…

781ab7e

…te a new string which is expensive Thanks @Swatinem

Remove commented-out crate

8b2f82f

Remove outdated comment

e1b550e

aws_config::ecs::EcsCredentialsProvider implements ProvideCredentials…

b9fa696

… directly

No need for clone()s, just pass references

b695ebe

cargo fmt

b71e29c

Move clone() into trait

6eff303

Make tests build

6a72b7f

Jos Backus added 3 commits November 30, 2022 13:16

Put back accidentally-removed code

12cd8f5

Remove useless test

70205cc

Unlike Rusoto, the Rust AWS SDK doesn't hardcode the currently known regions, and no validation is provided. API requests with invalid region names will fail.

Merge branch 'master' into task/aws-sdk-rust-migration

d0c1d2a

Update axum-server to 0.4.4

b13e06a

josb mentioned this pull request Dec 19, 2022

RUSTSEC-2022-0071: Rusoto is unmaintained #944

Closed

Swatinem mentioned this pull request Dec 28, 2022

ref: Migrate from rusoto to aws-sdk-s3 #954

Merged

Swatinem closed this Dec 28, 2022

@@ @@ -264,6 +257,7 @@ impl S3Downloader { @@
                   }
               }
+              /*

Migrate from rusoto to aws-sdk-rust #849

Migrate from rusoto to aws-sdk-rust #849

Conversation

josb commented Jul 3, 2022

codecov-commenter commented Jul 3, 2022 • edited

Codecov Report

flub 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

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

Swatinem commented Nov 21, 2022

Swatinem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josb Nov 25, 2022 • edited

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

Swatinem commented Nov 21, 2022

josb commented Nov 25, 2022

Swatinem commented Nov 28, 2022

josb commented Nov 28, 2022 • edited

Swatinem commented Nov 29, 2022

josb commented Dec 3, 2022

Swatinem commented Dec 5, 2022

Swatinem commented Dec 28, 2022

codecov-commenter commented Jul 3, 2022 •

edited

josb Nov 25, 2022 •

edited

josb commented Nov 28, 2022 •

edited