tls: moving the server name into SocketAddressProvider by soulxu · Pull Request #16574 · envoyproxy/envoy

soulxu · 2021-05-19T14:08:40Z

Commit Message: tls: moving the server name into SocketAddressProvider

Additional Description:
Making the SocketAddressProvider is single source of server name.

Risk Level: low
Testing: n/a
Part of #17168
Fixes #14922

Signed-off-by: He Jie Xu hejie.xu@intel.com

Before the connection reach to TCPPorxy filter or HCM, the server name won't be populated into stream info. It leads to the access log get empty server name. This patch populate the server name in the path of closing socket. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

snowp

Thanks for working on a fix here! High level question on the approach

source/server/active_tcp_listener.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-05-24T06:35:24Z

/retest

repokitteh-read-only · 2021-05-24T06:35:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16574 (comment) was created by @soulxu.

see: more, trace.

snowp

Thanks this seems like a better direction to me, a few comments.

@ggreenway do you also want to review this?

test/server/active_tcp_listener_test.cc

source/server/active_tcp_listener.cc

ggreenway

This is looking good; I like this approach better.

source/common/http/conn_manager_impl.cc

ggreenway · 2021-05-24T16:07:31Z

source/server/active_tcp_listener.cc

 void ActiveTcpListener::newConnection(Network::ConnectionSocketPtr&& socket,
                                      std::unique_ptr<StreamInfo::StreamInfo> stream_info) {
+  // populate the SNI into stream info, the network filters needn't do that again.
+  if (!socket->requestedServerName().empty()) {


Can this be done unconditionally? I would expect that calling setRequestedServerName() with an empty string would have no effect.

It has a constructor even for empty string https://github.com/envoyproxy/envoy/blob/main/source/common/stream_info/stream_info_impl.h#L237. Or we can move the check inside the setRequestedServerName(), but that will block the case when want to set the server name back to empty. Although I'm not sure we have any case to set the server name back to empty, but it still feels bad for an interface.

I think it is functionally the same without this check; setting it to empty is the same as not setting it. Please remove the conditional to simplify the code. Same in unlink().

got it, let me remove it.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-05-31T09:01:29Z

/retest

repokitteh-read-only · 2021-05-31T09:01:33Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16574 (comment) was created by @soulxu.

see: more, trace.

ggreenway

Please add another test case for listener filter timeout that verifies the server name is set.

/wait

source/common/http/conn_manager_impl.cc

ggreenway · 2021-06-02T20:42:17Z

source/server/active_tcp_listener.cc

 void ActiveTcpListener::newConnection(Network::ConnectionSocketPtr&& socket,
                                      std::unique_ptr<StreamInfo::StreamInfo> stream_info) {
+  // populate the SNI into stream info, the network filters needn't do that again.
+  if (!socket->requestedServerName().empty()) {


I think it is functionally the same without this check; setting it to empty is the same as not setting it. Please remove the conditional to simplify the code. Same in unlink().

test/server/active_tcp_listener_test.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-06-03T09:13:04Z

thanks for the review, I will update soon!

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-06-03T22:40:50Z

/retest

repokitteh-read-only · 2021-06-03T22:42:31Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16574 (comment) was created by @soulxu.

see: more, trace.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway · 2021-06-03T23:47:56Z

Yes I think this is likely a bug in the same class of issues I fixed a while back with addresses. Optimally, we would do something similar here so we don't mess this up.

I didn't realize until now how similar it is. There's an accessor for the requestedServerName on the Connection, which I didn't realize. Maybe we can just delete that entirely, and say that the way to get it is always from the streamInfo, and make sure it's always populated on the streamInfo. Then it becomes a simpler problem than the addresses. It's only used in about ~10 places off of the Connection. Thoughts @mattklein123 ?

mattklein123 · 2021-06-04T15:41:54Z

I didn't realize until now how similar it is. There's an accessor for the requestedServerName on the Connection, which I didn't realize. Maybe we can just delete that entirely, and say that the way to get it is always from the streamInfo, and make sure it's always populated on the streamInfo. Then it becomes a simpler problem than the addresses. It's only used in about ~10 places off of the Connection. Thoughts @mattklein123 ?

I think this would be fine, but keep in mind that there are unfortunately multiple stacked stream infos (connection, stream, etc.) so we need to make sure to not actually break copying the data over.

Another option would be just adding this to SocketAddressProvider (and maybe renaming it) which is how I fixed all the address bugs. Basically I made it impossible to not have this data be carried everywhere.

soulxu · 2021-06-08T03:47:59Z

I didn't realize until now how similar it is. There's an accessor for the requestedServerName on the Connection, which I didn't realize. Maybe we can just delete that entirely, and say that the way to get it is always from the streamInfo, and make sure it's always populated on the streamInfo. Then it becomes a simpler problem than the addresses. It's only used in about ~10 places off of the Connection. Thoughts @mattklein123 ?

I think this would be fine, but keep in mind that there are unfortunately multiple stacked stream infos (connection, stream, etc.) so we need to make sure to not actually break copying the data over.

Another option would be just adding this to SocketAddressProvider (and maybe renaming it) which is how I fixed all the address bugs. Basically I made it impossible to not have this data be carried everywhere.

Hope I understand correctly. I search the socket address issue #14133, so the goal here is not to carry the info anywhere, avoid the bug we always forget to populate the info somewhere, just share the same info from the connection. So we can put the server_name into the socket address provider(need to rename it). Is it correct? @mattklein123

If it is correct, I found other than the 'server name', we also can put the 'downstream ssl info'

envoy/source/common/http/conn_manager_impl.cc

Line 651 in 233546f

filter_manager_.streamInfo().setDownstreamSslConnection(

and 'connection id'

envoy/source/common/http/conn_manager_impl.cc

Line 654 in 233546f

filter_manager_.streamInfo().setConnectionID(

into the socket address provider. Those are used for l4 stream info and http stream info and also populated from the connection.

Also, @ggreenway mentioned, we should clean up the way to get the stream info, it should be always from the stream info. for example:

envoy/source/extensions/filters/http/rbac/rbac_filter.cc

Line 49 in 233546f

callbacks_->connection()->requestedServerName(),

let me know whether this is the direction you guys looking for, I'm happy to work on it :)

mattklein123 · 2021-06-14T20:48:35Z

Yeah in general ^ makes sense to me. In general if we can consolidate information that should never change into a single structure to avoid bugs in copying it around that would be optimal.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-06-28T10:42:42Z

@mattklein123 @ggreenway I'm going to separate the whole thing into different PR since I found the PR will become huge and hard to review, this PR is just moving the server_name inside SocketAddressProvider. Then I will have other PRs for moving 'ssl info' and 'connection id'. Then a PR for ensure the server name only get from the address provider, the last PR will rename the SocketAddressProvider to proper name. But let me know if you have other perference.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-06-28T13:52:06Z

fyi, I create an issue to track the whole thing #17168

mattklein123 · 2021-06-28T15:58:25Z

Check CI?

/wait

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123

Thanks this looks like a great improvement. One small comment.

/wait

mattklein123 · 2021-06-29T19:10:19Z

envoy/network/socket.h

   */
  virtual const Address::InstanceConstSharedPtr& directRemoteAddress() const PURE;

+  /**


Just to leave an intermediate comment trail, can you add a TODO above here on what you plan to rename it to? Otherwise this will get confusing if someone looks at it now.

yea, good point. let me add a TODO

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123

Thanks!

* main: (51 commits) listener: add filter chain match support for direct source address (envoyproxy#17118) Increase common/common coverage (envoyproxy#17193) crash_dump: Added local_end_stream_ to crash dump for H2. (envoyproxy#17199) codeql: improve Ubuntu dependency installation (envoyproxy#16556) ci: Move tooling tests to tooling job (envoyproxy#17071) Fix issue with Windows container image (envoyproxy#17113) fix filter linking urls (envoyproxy#17185) bug fix: fix bug that check_format.py will check files which are ignored (envoyproxy#17195) tls: moving the server name into SocketAddressProvider (envoyproxy#16574) network: Use std::make_unique and std::make_shared in source/common/network instead of bare new() (envoyproxy#17177) Revert "alpha matching: support generic action factory context (envoyproxy#17025)" (envoyproxy#17191) ci: Dont clone filter example where not required (envoyproxy#17182) alpha matching: support generic action factory context (envoyproxy#17025) xds: Clarify comment for RouteMatch.case_sensitive field. (envoyproxy#17176) ci: Only publish the required docker image (envoyproxy#17080) coverage: fixing flake (envoyproxy#17190) api: add cluster_specifier_plugin to RouteAction (envoyproxy#16944) wasm: update V8 to v9.2.230.13. (envoyproxy#17183) wasm: update Proxy-Wasm C++ Host and SDK to latest (2021-06-24). (envoyproxy#17174) owners: add Piotr as senior extension maintainer (envoyproxy#17175) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

) Before the connection reach to TCPPorxy filter or HCM, the server name won't be populated into stream info. It leads to the access log get empty server name. This patch populate the server name in the path of closing socket. Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

) Before the connection reach to TCPPorxy filter or HCM, the server name won't be populated into stream info. It leads to the access log get empty server name. This patch populate the server name in the path of closing socket. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu changed the title ~~Populate the server name into stream info when the connection closed~~ tls:populate the server name into stream info when the connection closed May 19, 2021

soulxu changed the title ~~tls:populate the server name into stream info when the connection closed~~ tls: populate the server name into stream info when the connection closed May 19, 2021

add unittest

09913e1

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

alyssawilk assigned snowp May 20, 2021

snowp suggested changes May 20, 2021

View reviewed changes

source/server/active_tcp_listener.cc Outdated Show resolved Hide resolved

Populate the SNI value in ActiveTCPListener

9fbb477

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu changed the title ~~tls: populate the server name into stream info when the connection closed~~ tls: populate the server name into stream info in ActiveTCPListener May 21, 2021

Merge branch 'main' into populate_server_name_1

c9f9f07

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

snowp suggested changes May 24, 2021

View reviewed changes

test/server/active_tcp_listener_test.cc Outdated Show resolved Hide resolved

test/server/active_tcp_listener_test.cc Outdated Show resolved Hide resolved

source/server/active_tcp_listener.cc Outdated Show resolved Hide resolved

ggreenway reviewed May 24, 2021

View reviewed changes

soulxu added 2 commits May 26, 2021 16:05

address comments

8b500d8

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

populate sni when unlink

3dc3130

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

snowp assigned ggreenway Jun 2, 2021

ggreenway requested changes Jun 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 2, 2021

remove the empty check

936cd1b

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

address comment in test

66e34b3

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 3, 2021

mock balancer

b5f85c3

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123 added the waiting label Jun 4, 2021

soulxu added 3 commits June 23, 2021 12:23

Merge branch 'main' into populate_server_name_1

3a848a1

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Add requested server name into SocketAddressProvider

83ef195

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

a6fd3d4

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 28, 2021

soulxu mentioned this pull request Jun 28, 2021

Moving the connection info into a single provider #17168

Closed

soulxu changed the title ~~tls: populate the server name into stream info in ActiveTCPListener~~ tls: moving the server name into SocketAddressProvider Jun 28, 2021

soulxu added 3 commits June 28, 2021 13:02

fix lua test

bd8376a

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix test

41f019a

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

b2ffa64

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested a review from PiotrSikora as a code owner June 28, 2021 13:47

repokitteh-read-only bot added the waiting label Jun 28, 2021

fix test

09d7233

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2021

fix format

eb86924

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123 requested changes Jun 29, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 29, 2021

Add TODO comment

7afbbcc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2021

soulxu added 2 commits June 29, 2021 23:19

add TODO comment

91f798d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix typo

e2b9cb6

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123 approved these changes Jun 30, 2021

View reviewed changes

mattklein123 merged commit 8e3441c into envoyproxy:main Jun 30, 2021

Conversation

soulxu commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soulxu commented May 24, 2021

Uh oh!

repokitteh-read-only bot commented May 24, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway May 24, 2021

Choose a reason for hiding this comment

Uh oh!

soulxu May 25, 2021

Choose a reason for hiding this comment

Uh oh!

ggreenway Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

soulxu Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

soulxu commented May 31, 2021

Uh oh!

repokitteh-read-only bot commented May 31, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

soulxu commented Jun 3, 2021

Uh oh!

soulxu commented Jun 3, 2021

Uh oh!

repokitteh-read-only bot commented Jun 3, 2021

Uh oh!

ggreenway commented Jun 3, 2021

Uh oh!

mattklein123 commented Jun 4, 2021

Uh oh!

soulxu commented Jun 8, 2021

Uh oh!

mattklein123 commented Jun 14, 2021

Uh oh!

soulxu commented Jun 28, 2021

Uh oh!

soulxu commented Jun 28, 2021

Uh oh!

mattklein123 commented Jun 28, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

soulxu Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

soulxu commented May 19, 2021 •

edited

Loading