-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
StreamInfo: Add downstreamDirectRemoteAddress #5064
Conversation
2530eb7
to
75a4096
Compare
Signed-off-by: Auni Ahsan <auni@google.com>
@dnoe PTAL |
what is PTAL? |
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
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.
Looks good overall. Can you add a test to verify that the downstreamDirectlyConnectedAddress
does return the actual physical address even when the downstreamRemoteAddress
is mutated via proto or headers? Also, it'd be good to note this new API in the release notes (version_history.rst
file).
I'm going to have to punt this off on someone else since I have a critical issue on my plate this week. Thanks for taking an initial look @venilnoronha - let me know when it is ready from your standpoint and I will move it forward. |
@dnoe will do. |
/assign @venilnoronha |
Signed-off-by: Auni Ahsan <auni@google.com>
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Not stale. |
Sorry for the delay, was out sick last week. There's no test ascertaining the changing state for downstreamRemoteAddress(), so I tacked on w xff to a conn manager impl test. |
8acb682
to
726ffb5
Compare
@auni53 there are build issues here under ASAN and other CI tests, can you take a look? |
@@ -369,6 +369,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect | |||
} | |||
stream_info_.setDownstreamLocalAddress( | |||
connection_manager_.read_callbacks_->connection().localAddress()); | |||
stream_info_.setDownstreamDirectlyConnectedAddress( |
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.
I think I would prefer downstreamDirectRemoteAddress()
for consistency with existing naming, or something like 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.
I'm fine with that, changing
constexpr char local_address[] = "0.0.0.0"; | ||
constexpr char xff_address[] = "1.2.3.4"; | ||
|
||
// stream_info.directRemoteAddress() will populate from xff |
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.
directRemoteAddress
? How does this populate from XFF?
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.
whoops, made a typo. cleaned up the comment, is this fine?
@@ -932,6 +932,11 @@ TEST_F(HttpConnectionManagerImplTest, | |||
} | |||
|
|||
TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { | |||
constexpr char local_address[] = "0.0.0.0"; |
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.
Ideally we also have a more specific unit test on ConnectionManagerImpl::ActiveStream::ActiveStream
, i.e. HCM, that shows this behavior. If there's a reasonable way to add this without too many gymnastics, that could be great.
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.
Looking into this, but I'm trying to find out whether ActiveStream is usually tested in isolation without an http conn manager. In TestAccessLog (l 970) the ActiveStream is already directly instantiated as decoder
and decodeHeaders is called on it.
Signed-off-by: Auni Ahsan <auni@google.com>
Signed-off-by: Auni Ahsan <auni@google.com>
Signed-off-by: Auni Ahsan <auni@google.com>
Signed-off-by: Auni Ahsan <auni@google.com>
Kinda bizarre...the build failures are actually just clang-tidy failures, but they only throw on asan/tsan/mac runs. (Still wish there was an easier way to validate these errors than waiting a long time.) |
Signed-off-by: Auni Ahsan <auni@google.com>
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 for cleaning up the PR.
Signed-off-by: Auni Ahsan <auni@google.com>
Oopsy, thanks for catching that. Fixed that error + the nit. clang-tidy errors are outstanding, but it won't tell me what's wrong so running it locally. |
This comment has been minimized.
This comment has been minimized.
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!
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.
Great, thanks!
@lizan could you take a look at this? The clang_tidy tests on circleCI time out, and my local run isn't showing me anything useful. Local run details: I ran
|
@auni53 can you merge master to see how it goes? I add an echo for clang-tidy to identify the problem. |
Signed-off-by: Auni Ahsan <auni@google.com>
doneeee |
🙀 Error while processing event:
|
@lizan still not seeing any useful debug here. Do we just want to force merge this PR or is this a useful reproducer to keep investigating with? |
@lizan ok, will force merge when a reasonable number of these presubmits pass. |
/retest |
🔨 rebuilding |
The downstreamRemoteAddress field is modified when the remote address is inferred from proxy information or headers. This new field guarantees to hold the directly connected host's physical address. Risk Level: Low Testing: Searched around for references to downstreamRemoteAddress() and filled in similar tests/mocks. Fixes envoyproxy#4996. Signed-off-by: Auni Ahsan <auni@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Auni Ahsan auni@google.com
Description: The downstreamRemoteAddress field is modified when the remote address is inferred from proxy information or headers. This new field guarantees to hold the directly connected host's physical address.
Risk Level: Low
Testing: Searched around for references to downstreamRemoteAddress() and filled in similar tests/mocks.
Fixes #4996.