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

Record TCP connection local socket address in metadata #3286

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Record TCP connection local socket address in metadata #3286

merged 1 commit into from
Dec 6, 2023

Conversation

declanvk
Copy link
Contributor

@declanvk declanvk commented Dec 6, 2023

Motivation and Context

I want to use this field to uniquely identify TCP connection based on their local_addr + remote_addr.

See awslabs/aws-sdk-rust#990 for additional motivation for this change.

Description

  • Add a new optional local_addr field in the ConnectionMetadata struct.
  • Transfer the local_addr SocketAddress from the hyper::HttpInfo to the ConnectionMetadata field.
  • Add to the trace-serialize example program so that it will print out the capture connection values.

Testing

cargo test in rust-runtime/aws-smithy-runtime-api and aws-smithy-runtime.

Also ran:

thedeck@c889f3b04fb0 examples % cargo run --example trace-serialize
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `/Users/thedeck/repos/github/declanvk/smithy-rs/target/debug/examples/trace-serialize`
2023-12-06T00:13:15.605555Z  INFO lazy_load_identity: aws_smithy_runtime::client::identity::cache::lazy: identity cache miss occurred; added new identity (took Ok(296µs))
2023-12-06T00:13:15.608344Z  INFO trace_serialize: Response received: response=Response { status: StatusCode(200), headers: Headers { headers: {"content-type": HeaderValue { _private: "application/json" }, "content-length": HeaderValue { _private: "17" }, "date": HeaderValue { _private: "Wed, 06 Dec 2023 00:13:15 GMT" }} }, body: SdkBody { inner: BoxBody, retryable: false }, extensions: Extensions }
2023-12-06T00:13:15.608388Z  INFO trace_serialize: Captured connection info remote_addr=Some(127.0.0.1:13734) local_addr=Some(127.0.0.1:50199)
2023-12-06T00:13:15.608511Z  INFO trace_serialize: Response received POKEMON_SERVICE_URL=http://localhost:13734 response=GetServerStatisticsOutput { calls_count: 0 }

You can see the log line with "Captured connection info" contains the remote_addr and the local_addr fields.

Checklist

  • 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.

@declanvk declanvk requested review from a team as code owners December 6, 2023 00:18
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.

Mostly looks good. Just one note on backwards compatibility.

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.

This looks great! I appreciate you making the builder consistent with all the others 😄

Just a couple minor comments that need to be addressed, and then we'll get this merged in. Thank you.

@declanvk
Copy link
Contributor Author

declanvk commented Dec 6, 2023

Oops I pushed too quickly, sorry! Probably will get a build failure for the is_proxied to proxied builder method name change. Fixing that now

**Description**
 - Add a new optional `local_addr` field in the `ConnectionMetadata`
   struct.
 - Transfer the `local_addr` `SocketAddress` from the
   `hyper::HttpInfo` to the `ConnectionMetadata` field.
 - Add to the `trace-serialize` example program so that it will print
   out the capture connection values.
 - Add a new builder type to construct the `ConnectionMetadata` type

**Motivation**
I want to use this field to uniquely identify TCP connection based
on their `local_addr` + `remote_addr`.

See awslabs/aws-sdk-rust#990 for additional
motivation for this change.

Needed a new builder type so that adding new fields is backwards
compatible.

**Testing Done**
`cargo test` in `rust-runtime/aws-smithy-runtime-api` and
`aws-smithy-runtime`.

Also ran:
```
thedeck@c889f3b04fb0 examples % cargo run --example trace-serialize
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `/Users/thedeck/repos/github/declanvk/smithy-rs/target/debug/examples/trace-serialize`
2023-12-06T00:13:15.605555Z  INFO lazy_load_identity: aws_smithy_runtime::client::identity::cache::lazy: identity cache miss occurred; added new identity (took Ok(296µs))
2023-12-06T00:13:15.608344Z  INFO trace_serialize: Response received: response=Response { status: StatusCode(200), headers: Headers { headers: {"content-type": HeaderValue { _private: "application/json" }, "content-length": HeaderValue { _private: "17" }, "date": HeaderValue { _private: "Wed, 06 Dec 2023 00:13:15 GMT" }} }, body: SdkBody { inner: BoxBody, retryable: false }, extensions: Extensions }
2023-12-06T00:13:15.608388Z  INFO trace_serialize: Captured connection info remote_addr=Some(127.0.0.1:13734) local_addr=Some(127.0.0.1:50199)
2023-12-06T00:13:15.608511Z  INFO trace_serialize: Response received POKEMON_SERVICE_URL=http://localhost:13734 response=GetServerStatisticsOutput { calls_count: 0 }
```

You can see the log line with "Captured connection info" contains the
`remote_addr` and the `local_addr` fields.
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! I will go ahead and merge this. Thank you for the contribution ❤️

@jdisanti jdisanti added this pull request to the merge queue Dec 6, 2023
Merged via the queue into smithy-lang:main with commit b78367c Dec 6, 2023
39 checks passed
@declanvk declanvk deleted the connection-local-addr branch December 6, 2023 19:38
github-merge-queue bot pushed a commit that referenced this pull request Dec 7, 2023
## Motivation and Context
Enables `cargo semver-checks` in CI for PRs created by external
contributors

## Description
For instance, we skipped a run of `cargo semver-checks` in
#3286 and failed to detect
[a breaking
change](#3286 (comment))
programmatically.

With this PR, the workflow will run a job `semver-checks` even if the
preceding jobs `save-docker-login-token` or `acquire-base-image` are
skipped. Those jobs are relevant when the PR made changes to build
tools, which is less likely for PRs created by external contributors, so
it's reasonable to skip them and still run the `semver-checks` job.

Furthermore, this PR enables `semver-checks` to run against all crates
in `tmp-codegen-diff/aws-sdk/sdk/`, not just those limited by
`list(os.listdir())[:10]`.

## Testing
Tested the change against [a dummy
PR](#3288) I created from
my fork of `smithy-rs`. Specifically, `semver-checks` [caught the
aforementioned breaking
change](https://github.com/smithy-lang/smithy-rs/actions/runs/7121830175/job/19391798131#step:4:681)
in CI.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
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.

None yet

2 participants