Add connection poisoning to aws-smithy-client #2445

rcoh · 2023-03-09T18:44:11Z

Motivation and Context

[request]: S3 DNS re-lookup on retry awslabs/aws-sdk-rust#160

Description

Built on top of two changes to Hyper, this adds the concept of connection poisoning to Smithy client. To do this, we needed machinery to get access to the "connection metadata" used by the request. This is done in a connector agnostic way with the CaptureSmithyConnection bridge.

A separate layer considers the error classification then invokes poison as necessary. To support testing this, WireLeveMock was created, a mocking facility that binds to 0.0.0.0.

For AWS services, this behavior is enabled by default. For generic-smithy clients, this behavior is disabled by default.

Testing

integration test in S3
detailed tests of specific behavior in aws-smithy-client

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
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.

github-actions · 2023-03-09T19:21:55Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2023-03-09T20:40:53Z

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

A new doc preview is ready to view.

github-actions · 2023-03-09T21:58:51Z

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

A new doc preview is ready to view.

github-actions · 2023-03-13T19:57:21Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

jdisanti

Nice work on this!

jdisanti · 2023-03-13T20:21:05Z

rust-runtime/aws-smithy-client/src/hyper_ext.rs

@@ -108,7 +117,30 @@ use tower::{BoxError, Service};
 /// see [the module documentation](crate::hyper_ext).
 #[derive(Clone, Debug)]
 #[non_exhaustive]


Not needed anymore:

Suggested change

#[non_exhaustive]

jdisanti · 2023-03-13T20:39:23Z

rust-runtime/aws-smithy-types/src/retry.rs

+    ReconnectOnTransientError,
+
+    /// Disable reconnects
+    NoReconnect,


The name here is probably good enough, but I could see how it could potentially cause confusion with reconnecting on hang up or something.

Maybe NoReuseBadConnection? Usage is clearer from the tests / examples

I renamed NoReconnect to ReuseAllConnections

github-actions · 2023-03-13T20:50:26Z

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

A new doc preview is ready to view.

82marbag · 2023-03-14T09:54:45Z

CHANGELOG.next.toml

+be reused.
+"""
+references = ["aws-sdk-rust#160", "smithy-rs#2445"]
+meta = { "breaking" = false, "tada" = false, "bug" = false }


Missing target

ah—we changed the default to all at some point which is why this isn't an error

github-actions · 2023-03-14T15:59:41Z

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

A new doc preview is ready to view.

ysaito1001

WireLevelTestConnection is pretty cool!

github-actions · 2023-03-14T18:39:24Z

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

A new doc preview is ready to view.

github-actions · 2023-03-14T20:05:21Z

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

A new doc preview is ready to view.

crisidev · 2023-03-20T14:11:48Z

rust-runtime/aws-smithy-client/src/hyper_ext.rs

+    let capture_conn = capture_conn.clone();
+    if let Some(conn) = capture_conn.clone().connection_metadata().as_ref() {


Do you really need the double clone here?

crisidev · 2023-03-20T14:12:47Z

rust-runtime/aws-smithy-client/src/hyper_ext.rs

+            http_info.map(|info| info.remote_addr()),
+            move || match capture_conn.connection_metadata().as_ref() {
+                Some(conn) => conn.poison(),
+                None => tracing::trace!("no connection existed to poison"),


Maybe rephrase this error message like no connection found that should be poisoned ?

crisidev · 2023-03-20T14:15:00Z

rust-runtime/aws-smithy-client/src/test_connection.rs

+                            }
+                            tracing::debug!("replying with {:?}", next_event);
+                            let event = generate_response_event(next_event).await;
+                            dbg!(event)


Is this a leftover or do we want to debug every time we spinup a new mock?

crisidev · 2023-03-20T14:16:02Z

rust-runtime/aws-smithy-client/src/test_connection.rs

+            let sock_addr = self.socket_addr;
+            let log = self.log.clone();
+            Box::pin(async move {
+                println!("looking up {:?}, replying with {:?}", req, sock_addr);


Same as above: is this a leftover?

rcoh force-pushed the poison-connections branch from 9b0603e to e6416ae Compare March 9, 2023 18:54

rcoh changed the title ~~tests passing!~~ Add connection poisoning to aws-smithy-client Mar 9, 2023

rcoh force-pushed the poison-connections branch from e6416ae to 3d9184d Compare March 9, 2023 20:14

rcoh force-pushed the poison-connections branch from 3d9184d to cb0ef09 Compare March 9, 2023 21:38

rcoh force-pushed the poison-connections branch from cb0ef09 to ac7987f Compare March 13, 2023 19:36

rcoh force-pushed the poison-connections branch from ac7987f to 722635c Compare March 13, 2023 20:07

rcoh marked this pull request as ready for review March 13, 2023 20:09

rcoh requested review from a team as code owners March 13, 2023 20:09

rcoh requested review from crisidev and david-perez March 13, 2023 20:09

rcoh force-pushed the poison-connections branch from 722635c to 672bb6f Compare March 13, 2023 20:18

jdisanti approved these changes Mar 13, 2023

View reviewed changes

82marbag reviewed Mar 14, 2023

View reviewed changes

82marbag approved these changes Mar 14, 2023

View reviewed changes

82marbag added needs-client-review Generic client and removed needs-client-review Generic client labels Mar 14, 2023

rcoh added 4 commits March 14, 2023 11:24

Add Connection Poisoning to aws-smithy-client

b4674e0

Fix doc links

c67f649

Remove required tokio dependency from aws-smithy-client

b306277

Remove external type exposed

3edbc5e

rcoh force-pushed the poison-connections branch from 19a739b to 3edbc5e Compare March 14, 2023 15:24

Rename, re-add tokio dependency

688e0dd

ysaito1001 approved these changes Mar 14, 2023

View reviewed changes

rcoh added 2 commits March 14, 2023 14:07

Change IP to 127.0.0.1 to attempt to fix windows

fe7398e

Add dns::Name to external types

24418cd

rcoh added 2 commits March 14, 2023 15:41

Remove non_exhaustive not needed

63f9f2e

Add client target to changelog

610e4f5

rcoh enabled auto-merge March 14, 2023 19:51

rcoh added this pull request to the merge queue Mar 14, 2023

Merged via the queue into main with commit 61934da Mar 14, 2023

rcoh deleted the poison-connections branch March 14, 2023 20:33

crisidev reviewed Mar 20, 2023

View reviewed changes

benesch mentioned this pull request Jun 14, 2023

AWS SDK seems to have failed to poison S3 connections after today's outage awslabs/aws-sdk-rust#827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection poisoning to aws-smithy-client #2445

Add connection poisoning to aws-smithy-client #2445

rcoh commented Mar 9, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 13, 2023

jdisanti left a comment

jdisanti Mar 13, 2023

jdisanti Mar 13, 2023

82marbag Mar 14, 2023 •

edited by Velfi

Loading

rcoh Mar 14, 2023

github-actions bot commented Mar 13, 2023

82marbag Mar 14, 2023

rcoh Mar 14, 2023

github-actions bot commented Mar 14, 2023

ysaito1001 left a comment

github-actions bot commented Mar 14, 2023

github-actions bot commented Mar 14, 2023

crisidev Mar 20, 2023

crisidev Mar 20, 2023

crisidev Mar 20, 2023

crisidev Mar 20, 2023

		let capture_conn = capture_conn.clone();
		if let Some(conn) = capture_conn.clone().connection_metadata().as_ref() {

Add connection poisoning to aws-smithy-client #2445

Add connection poisoning to aws-smithy-client #2445

Conversation

rcoh commented Mar 9, 2023 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 13, 2023

jdisanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

82marbag Mar 14, 2023 • edited by Velfi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2023

ysaito1001 left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2023

github-actions bot commented Mar 14, 2023

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

rcoh commented Mar 9, 2023 •

edited

Loading

82marbag Mar 14, 2023 •

edited by Velfi

Loading