StreamInfo: Add downstreamDirectRemoteAddress by aunu53 · Pull Request #5064 · envoyproxy/envoy

aunu53 · 2018-11-16T16:23:01Z

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.

Signed-off-by: Auni Ahsan <auni@google.com>

mattklein123 · 2018-11-18T19:38:45Z

@dnoe PTAL

aunu53 · 2018-11-19T00:19:29Z

what is PTAL?

stale · 2018-11-26T00:23:39Z

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!

venilnoronha

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

include/envoy/stream_info/stream_info.h

dnoe · 2018-11-29T15:04:05Z

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.

venilnoronha · 2018-11-29T15:16:12Z

@dnoe will do.

venilnoronha · 2018-11-29T15:16:56Z

/assign @venilnoronha

Signed-off-by: Auni Ahsan <auni@google.com>

stale · 2018-12-06T16:05:31Z

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!

venilnoronha · 2018-12-06T17:26:49Z

Not stale.

aunu53 · 2018-12-09T16:19:24Z

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.

htuch · 2018-12-09T17:16:24Z

@auni53 there are build issues here under ASAN and other CI tests, can you take a look?

htuch · 2018-12-09T17:30:48Z

source/common/http/conn_manager_impl.cc

  }
  stream_info_.setDownstreamLocalAddress(
      connection_manager_.read_callbacks_->connection().localAddress());
+  stream_info_.setDownstreamDirectlyConnectedAddress(


I think I would prefer downstreamDirectRemoteAddress() for consistency with existing naming, or something like that.

I'm fine with that, changing

htuch · 2018-12-09T17:33:32Z

test/common/http/conn_manager_impl_test.cc

+  constexpr char local_address[] = "0.0.0.0";
+  constexpr char xff_address[] = "1.2.3.4";
+
+  // stream_info.directRemoteAddress() will populate from xff


directRemoteAddress? How does this populate from XFF?

whoops, made a typo. cleaned up the comment, is this fine?

htuch · 2018-12-09T17:35:26Z

test/common/http/conn_manager_impl_test.cc

 }

 TEST_F(HttpConnectionManagerImplTest, TestAccessLog) {
+  constexpr char local_address[] = "0.0.0.0";


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.

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>

aunu53 · 2018-12-11T12:20:22Z

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>

venilnoronha

Thanks for cleaning up the PR.

docs/root/intro/version_history.rst

include/envoy/stream_info/stream_info.h

source/common/stream_info/stream_info_impl.h

test/common/stream_info/test_util.h

test/mocks/stream_info/mocks.h

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2018-12-11T15:36:41Z

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.

venilnoronha

LGTM. Thanks!

venilnoronha · 2018-12-11T17:08:46Z

@auni53 you can ignore the clang_tidy error. It's just a timeout.

ci/build_setup.sh: line 115:   199 Hangup                  ./run_clang_tidy.sh
Too long with no output (exceeded 10m0s)

@htuch over to you for another pass.

htuch

Great, thanks!

aunu53 · 2018-12-12T17:21:20Z

@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 ./ci/run_envoy_docker.sh ci/do_ci.sh bazel.clang_tidy; it compiled the DB and had a bunch of "x warnings generated" messages without any errors. The beginning of the run included this error/warning repeated a few times:

WARNING: Duplicate rc file: /source/tools/bazel.rc is read multiple times, most recently imported from /source/ci/.bazelrc
WARNING: Processed legacy workspace file /source/ci/tools/bazel.rc. This file will not be processed in the next release of Bazel. Please read https://github.com/bazelbuild/bazel/issues/6319 for further information, including how to upgrade.

lizan · 2018-12-13T00:59:42Z

@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>

aunu53 · 2018-12-13T14:20:41Z

doneeee

repokitteh-read-only · 2018-12-13T14:21:27Z

🙀 Error while processing event:

evaluation error
error: context deadline exceeded

🐱

Caused by: a #5064 (comment) was created by @auni53.

see: more, trace.

htuch · 2018-12-14T00:39:12Z

@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 · 2018-12-14T02:35:27Z

@htuch I opened #5296 to debug CI, if you want to merge this PR sooner, then I think it is OK to force merge.

htuch · 2018-12-14T02:52:31Z

@lizan ok, will force merge when a reasonable number of these presubmits pass.

venilnoronha · 2018-12-14T03:04:19Z

/retest

repokitteh-read-only · 2018-12-14T03:04:23Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5064 (comment) was created by @venilnoronha.

see: more, trace.

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>

aunu53 force-pushed the direct branch from f78be6b to 6c1eeaf Compare November 16, 2018 16:25

aunu53 mentioned this pull request Nov 16, 2018

StreamInfo: Preserving the source address on downstreamRemoteAddress #4996

Closed

aunu53 force-pushed the direct branch 2 times, most recently from 2530eb7 to 75a4096 Compare November 16, 2018 16:34

Add downstreamDirectlyConnectedAddress to StreamInfo

54cd8b8

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 force-pushed the direct branch from 75a4096 to 54cd8b8 Compare November 16, 2018 16:42

aunu53 changed the title ~~Add downstreamDirectlyConnectedAddress to StreamInfo~~ StreamInfo: Add downstreamDirectlyConnectedAddress Nov 16, 2018

mattklein123 assigned dnoe Nov 18, 2018

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 26, 2018

venilnoronha reviewed Nov 28, 2018

View reviewed changes

include/envoy/stream_info/stream_info.h Outdated Show resolved Hide resolved

stale bot removed stale stalebot believes this issue/PR has not been touched recently labels Nov 28, 2018

repokitteh-read-only bot assigned venilnoronha Nov 29, 2018

Merge remote-tracking branch 'envoy/master' into direct

78e0bf1

Signed-off-by: Auni Ahsan <auni@google.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 6, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 6, 2018

aunu53 force-pushed the direct branch from 0eb8169 to b058c1c Compare December 9, 2018 16:18

aunu53 force-pushed the direct branch 2 times, most recently from 8acb682 to 726ffb5 Compare December 9, 2018 16:26

aunu53 requested review from htuch and lizan as code owners December 9, 2018 16:26

htuch suggested changes Dec 9, 2018

View reviewed changes

aunu53 added 3 commits December 10, 2018 11:51

Refactor new stream_info field to downstreamDirectRemoteAddress

cc8f2cf

Signed-off-by: Auni Ahsan <auni@google.com>

Merge remote-tracking branch 'envoy/master' into direct

f8d8be4

Signed-off-by: Auni Ahsan <auni@google.com>

fix format

9c49bff

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 changed the title ~~StreamInfo: Add downstreamDirectlyConnectedAddress~~ StreamInfo: Add downstreamDirectRemoteAddress Dec 11, 2018

Attempt to resolve clang-tidy errors

de3efaa

Signed-off-by: Auni Ahsan <auni@google.com>

Empty commit to kick CI

edfb5bb

Signed-off-by: Auni Ahsan <auni@google.com>

venilnoronha reviewed Dec 11, 2018

View reviewed changes

Fix naming and release

5b3b377

Signed-off-by: Auni Ahsan <auni@google.com>

This comment has been minimized.

Sign in to view

venilnoronha approved these changes Dec 11, 2018

View reviewed changes

htuch approved these changes Dec 12, 2018

View reviewed changes

Merge remote-tracking branch 'envoy/master' into direct

c86b7b4

Signed-off-by: Auni Ahsan <auni@google.com>

htuch merged commit 13d6396 into envoyproxy:master Dec 14, 2018

aunu53 deleted the direct branch December 14, 2018 13:22

ggreenway mentioned this pull request Mar 11, 2020

Access log specifier %DOWNSTREAM_DIRECT_REMOTE_ADDRESS% does not work as documented with proxy-protocol #10328

Closed

Conversation

aunu53 commented Nov 16, 2018

Uh oh!

mattklein123 commented Nov 18, 2018

Uh oh!

aunu53 commented Nov 19, 2018

Uh oh!

stale bot commented Nov 26, 2018

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnoe commented Nov 29, 2018

Uh oh!

venilnoronha commented Nov 29, 2018

Uh oh!

venilnoronha commented Nov 29, 2018

Uh oh!

stale bot commented Dec 6, 2018

Uh oh!

venilnoronha commented Dec 6, 2018

Uh oh!

aunu53 commented Dec 9, 2018

Uh oh!

htuch commented Dec 9, 2018

Uh oh!

htuch Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

aunu53 Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

htuch Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

aunu53 Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

htuch Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

aunu53 Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

aunu53 commented Dec 11, 2018

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aunu53 commented Dec 11, 2018

Uh oh!

This comment has been minimized.

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

venilnoronha commented Dec 11, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

aunu53 commented Dec 12, 2018

Uh oh!

lizan commented Dec 13, 2018

Uh oh!

aunu53 commented Dec 13, 2018

Uh oh!

repokitteh-read-only bot commented Dec 13, 2018

Uh oh!

htuch commented Dec 14, 2018

Uh oh!

lizan commented Dec 14, 2018

Uh oh!

htuch commented Dec 14, 2018

Uh oh!