Use the orchestrator client for ECS and IMDS credentials in aws-config #2997

jdisanti · 2023-09-23T00:52:20Z

This ports the direct uses of the aws_smithy_client::Client in aws_config to the orchestrator.

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

This ports the direct uses of the `aws_smithy_client::Client` in aws_config to the orchestrator.

github-actions · 2023-09-23T01:21:13Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

…irect-invoke

github-actions · 2023-09-25T17:51:04Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

rcoh · 2023-09-26T14:12:02Z

the direct changes look good. My main questions / feedback are around the explicit SharedRuntimePlugin API change.

aws/rust-runtime/aws-config/Cargo.toml

aws/rust-runtime/aws-config/src/imds/client/error.rs

aws/rust-runtime/aws-config/src/imds/client/token.rs

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsPresigningDecorator.kt

...are/amazon/smithy/rust/codegen/client/smithy/customizations/HttpChecksumRequiredGenerator.kt

...oftware/amazon/smithy/rust/codegen/client/smithy/customizations/IdempotencyTokenGenerator.kt

rust-runtime/aws-smithy-runtime-api/src/client/runtime_plugin.rs

…irect-invoke

github-actions · 2023-09-27T01:47:53Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

rcoh

I like IntoShared! We should probably ensure that you get decent type hints / docs in various editors at some point

rcoh · 2023-09-27T15:11:09Z

rust-runtime/aws-smithy-runtime-api/src/shared.rs

+pub trait IntoShared<Shared> {
+    /// Creates a shared type from an unshared type.
+    fn into_shared(self) -> Shared;
+}


I think this should be an associated type? It doesn't make sense for one type to be convertible into multiple different shared types right?

I basically made these traits equivalent to From/Into. I guess it makes sense to restrict it to one shared type, but typing IntoShared<Shared = MySharedType> everywhere kind of sucks. I wish we had trait aliases.

rust-runtime/aws-smithy-runtime-api/src/shared.rs

rcoh · 2023-09-27T15:12:47Z

rust-runtime/aws-smithy-runtime-api/src/shared.rs

+/// ```rust,no_run
+/// use aws_smithy_runtime_api::impl_shared_conversions;
+/// use std::sync::Arc;
+///
+/// trait Thing {}
+///
+/// struct Thingamajig;
+/// impl Thing for Thingamajig {}
+///
+/// struct SharedThing(Arc<dyn Thing>);
+/// impl Thing for SharedThing {}
+/// impl SharedThing {
+///     fn new(thing: impl Thing + 'static) -> Self {
+///         Self(Arc::new(thing))
+///     }
+/// }
+/// impl_shared_conversions!(convert SharedThing from Thing using SharedThing::new);
+/// ```


thanks for adding an example!

rcoh · 2023-09-27T15:13:25Z

rust-runtime/aws-smithy-runtime-api/src/shared.rs

+    #[test]
+    fn test() {
+        let thing = Thingamajig;
+        assert_eq!("Thingamajig", format!("{thing:?}"), "precondition");
+
+        let shared_thing: SharedThing = thing.into_shared();
+        assert_eq!(
+            "SharedThing(Thingamajig)",
+            format!("{shared_thing:?}"),
+            "precondition"
+        );
+
+        let very_shared_thing: SharedThing = shared_thing.into_shared();
+        assert_eq!(
+            "SharedThing(Thingamajig)",
+            format!("{very_shared_thing:?}"),
+            "it should not nest the shared thing in another shared thing"
+        );
+    }


should we assert via downcasting that an already shared type is not double boxed?

This seems to be very non-trivial.

rcoh · 2023-09-27T15:14:29Z

rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs

+            .with_endpoint_resolver(Some(FakeEndpointResolver))
+            .with_http_connector(Some(FakeConnector))


should these methods accept T instead of Option<T>? I might imagine a set_ that accepted an option 💭

It already has the the set_ variants. I can't remember why I did it this way, but it looks like nothing is calling them with None. So I think this is a sane change to make.

Thinking this should be done in a follow-up PR.

rcoh · 2023-09-27T15:15:17Z

rust-runtime/aws-smithy-runtime-api/src/client/interceptors.rs

-impl AsRef<dyn Interceptor> for SharedInterceptor {
-    fn as_ref(&self) -> &(dyn Interceptor + 'static) {
-        self.interceptor.as_ref()


any reason to remove this? seems like a reasonable way to handle it

I can't use the IntoShared/FromUnshared traits with it if it is done this way. We might just want to code generate the Interceptor and SharedInterceptor types, come to think of it. They're just not fun to work with in Rust due to the insane number of similar looking hooks with a ton of arguments.

rcoh · 2023-09-27T15:16:21Z

rust-runtime/aws-smithy-runtime-api/src/client/auth.rs

+impl_shared_conversions!(
+    convert SharedAuthSchemeOptionResolver
+    from AuthSchemeOptionResolver
+    using SharedAuthSchemeOptionResolver::new
+);


I'd probably consider adding : in the macro to remove any parsing hazard but this seems to be working fine

convert: SharedAuthSchemeOptionResolver from: Auth....

aws/rust-runtime/aws-config/src/imds/client/token.rs

…irect-invoke

github-actions · 2023-09-27T18:19:28Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

…irect-invoke

github-actions · 2023-09-29T01:01:35Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
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.

Use the orchestrator client for ECS and IMDS credentials in aws-config

3b4be07

This ports the direct uses of the `aws_smithy_client::Client` in aws_config to the orchestrator.

jdisanti force-pushed the jdisanti-aws-config-direct-invoke branch from 3281229 to 3b4be07 Compare September 23, 2023 00:53

jdisanti added 2 commits September 25, 2023 10:19

Fix aws-config imports

be6908c

Merge remote-tracking branch 'origin/main' into jdisanti-aws-config-d…

3e1162f

…irect-invoke

jdisanti added the breaking-change This will require a breaking change label Sep 25, 2023

jdisanti marked this pull request as ready for review September 25, 2023 18:54

jdisanti requested review from a team as code owners September 25, 2023 18:54

jdisanti added the needs-sdk-review label Sep 25, 2023

rcoh reviewed Sep 26, 2023

View reviewed changes

jdisanti added 6 commits September 26, 2023 13:58

Mark IMDS token header value as sensitive and fix typo

fc8bffc

Add changelog entry for IMDS client builder

3acb901

Rename OpInput

eb507a4

Reduce manual SharedRuntimePlugin instantiations

01720a9

More broadly apply IntoShared

c97ad37

Merge remote-tracking branch 'origin/main' into jdisanti-aws-config-d…

a6de8b9

…irect-invoke

rcoh approved these changes Sep 27, 2023

View reviewed changes

jdisanti added 3 commits September 27, 2023 10:32

Improve shared type docs

baad175

Remove unnecessary shared type instantiations

1d68fdf

Merge remote-tracking branch 'origin/main' into jdisanti-aws-config-d…

bc36fe4

…irect-invoke

Merge remote-tracking branch 'origin/main' into jdisanti-aws-config-d…

80c87c2

…irect-invoke

jdisanti removed the needs-sdk-review label Sep 29, 2023

jdisanti enabled auto-merge September 29, 2023 00:45

jdisanti added this pull request to the merge queue Sep 29, 2023

Merged via the queue into main with commit d800d33 Sep 29, 2023
40 of 41 checks passed

jdisanti deleted the jdisanti-aws-config-direct-invoke branch September 29, 2023 01:31

NickDarvey mentioned this pull request Jun 25, 2024

chore: upgrade aws-config for aarch64-pc-windows support lancedb/lance#2510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the orchestrator client for ECS and IMDS credentials in aws-config #2997

Use the orchestrator client for ECS and IMDS credentials in aws-config #2997

jdisanti commented Sep 23, 2023

github-actions bot commented Sep 23, 2023

github-actions bot commented Sep 25, 2023

rcoh commented Sep 26, 2023

github-actions bot commented Sep 27, 2023

rcoh left a comment

rcoh Sep 27, 2023

jdisanti Sep 27, 2023

rcoh Sep 27, 2023

rcoh Sep 27, 2023

jdisanti Sep 27, 2023

rcoh Sep 27, 2023

jdisanti Sep 27, 2023

jdisanti Sep 27, 2023

rcoh Sep 27, 2023

jdisanti Sep 27, 2023

rcoh Sep 27, 2023

rcoh Sep 27, 2023

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 29, 2023

		.with_endpoint_resolver(Some(FakeEndpointResolver))
		.with_http_connector(Some(FakeConnector))

Use the orchestrator client for ECS and IMDS credentials in aws-config #2997

Use the orchestrator client for ECS and IMDS credentials in aws-config #2997

Conversation

jdisanti commented Sep 23, 2023

github-actions bot commented Sep 23, 2023

github-actions bot commented Sep 25, 2023

rcoh commented Sep 26, 2023

github-actions bot commented Sep 27, 2023

rcoh 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

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 29, 2023