-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
stream info: add interface name of the local side of the upstream connection #19336
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
…voy into interface-name-stream-info Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
This reverts commit 8c2da05. Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
…voy into interface-name-stream-info
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and there are some minor comments about the new API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks this looks reasonable to me, just a couple things on top of @wbpcode's review
@@ -923,6 +924,11 @@ void ClientConnectionImpl::connect() { | |||
|
|||
void ClientConnectionImpl::onConnected() { | |||
stream_info_.upstreamInfo()->upstreamTiming().onUpstreamConnectComplete(dispatcher_.timeSource()); | |||
// There are no meaningful socket source address semantics for non-IP sockets, so skip. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would interfaceName()
just be empty in that case? I wonder if it would be simpler semantically to gate on just that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can make the set*
API take a optional arg absl::optional<absl::string_view>
. Then we only need do simple check in the set*
API implementation. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would interfaceName() just be empty in that case? I wonder if it would be simpler semantically to gate on just that
No, unfortunately this goes all the way to what unix provides us in getsockname
: a successful syscall but an non-zero bytes "empty" string. So, as it stands calling localAddress()
on a UDS throws. I can fix that. But it seemed independent of this change.
Signed-off-by: Jose Nino <jnino@lyft.com>
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM and cc @snowp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
…nection (envoyproxy#19336) Commit Message: This PR wires the interface name of the local side of the upstream connection to stream info. Additional Description: The underlying logic uses syscalls, but only once per connection, so the addition to stream info on every stream is not expensive. Note that UDS has no meaningful source address information. Risk Level: low - the socket functionality is already existing. Testing: added tests Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: Josh Perry <josh.perry@mx.com>
Commit Message: This PR wires the interface name of the local side of the upstream connection to stream info.
Additional Description: The underlying logic uses syscalls, but only once per connection, so the addition to stream info on every stream is not expensive. Note that UDS has no meaningful source address information.
Risk Level: low - the socket functionality is already existing.
Testing: added tests