tls: move ssl connection info into SocketAddressProvider by soulxu · Pull Request #17334 · envoyproxy/envoy

soulxu · 2021-07-14T07:56:15Z

Commit Message: tls: move ssl connection info into SocketAddressProvider
Additional Description:
Moving the StreamInfo's SSL connection info into the SocketAddressProvider.
Risk Level: low
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Part of #17168

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

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

soulxu · 2021-07-14T07:56:29Z

cc @mattklein123

soulxu · 2021-07-14T07:59:28Z

envoy/network/connection.h

   */
  virtual const SocketAddressProvider& addressProvider() const PURE;
-  virtual SocketAddressProviderSharedPtr addressProviderSharedPtr() const PURE;
+  virtual SocketAddressSetterSharedPtr addressProviderSharedPtr() const PURE;


note: I have to change this due to allow the StreamInfo to set the upstream SSL connection into the AddressProvider.

soulxu · 2021-07-14T08:03:48Z

envoy/stream_info/stream_info.h

   * @param connection_info sets the upstream ssl connection.
   */
  virtual void
  setUpstreamSslConnection(const Ssl::ConnectionInfoConstSharedPtr& ssl_connection_info) PURE;


This was kept to avoid change the 'downstreamSslConnection()' interface as non const. I feel change the 'downstreamSslConnection' isn't right direction.

Also this is why I can change the place to set the upstream SSL connection

envoy/source/common/router/upstream_request.cc

Line 415 in 68fe53a

stream_info_.setUpstreamSslConnection(info.downstreamSslConnection());

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

soulxu · 2021-07-19T00:06:56Z

/retest

repokitteh-read-only · 2021-07-19T00:07:00Z

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

🐱

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

see: more, trace.

ggreenway

/wait

envoy/network/socket.h

ggreenway · 2021-07-21T18:12:15Z

envoy/network/socket.h

+   * @return the upstream SSL connection. This will be nullptr if the upstream
+   * connection does not use SSL.
+   */
+  virtual Ssl::ConnectionInfoConstSharedPtr upstreamSslConnection() const PURE;


I think this interface should only have the downstreamSslConnection(). All the other fields here are specific to the downstream connection, so including the upstream connection seems misleading.

Yes, I feel strange too. Although we can keep this in the StreamInfo also, but it still feel strange, since the StreamInfo is only mean to stream for one direction(upstream, or downstream). Also can see the downstream StreamInfo's upstream SSL info is copied from upstream StreamInfo. For example:

envoy/source/common/tcp_proxy/tcp_proxy.cc

Line 531 in a8033fa

getStreamInfo().setUpstreamSslConnection(ssl_info);

So in the end, the upstream SSL info is still mixed with downstream SSL info into downstream StreamInfo. Based on my guess the reason is the filter need the upstream StreamInfo, but other than tcpProxy or HttpRouter filter can't get the upstream StreamInfo, since the upstream StreamInfo is created the last filter (tcpProxy/http router filter).

The streaminfo is a bit odd, and has a mix of upstream/downstream. But SocketAddressProvider is only supposed to have information that should be copied to each request on the connection. The upstream ssl info should not be copied between requests, so I don't think it belongs here.

thanks, got it, let me revert the change to upstream SSL info.

envoy/network/socket.h

This reverts commit 2df315a. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

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

This reverts commit 3b56911. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

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

…ider" This reverts commit 2dffc9f. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

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

soulxu · 2021-07-26T01:35:07Z

/retest

repokitteh-read-only · 2021-07-26T01:35:10Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

soulxu · 2021-07-26T04:56:34Z

/retest

repokitteh-read-only · 2021-07-26T04:56:37Z

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

🐱

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

see: more, trace.

soulxu · 2021-07-26T13:12:06Z

emm...I can't reproduce the ci failure

ggreenway · 2021-07-27T15:57:18Z

Looks like the test timed out. Unsure why. Let's see if it's happier in CI today:

/retest

repokitteh-read-only · 2021-07-27T15:57:22Z

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

🐱

Caused by: a #17334 (comment) was created by @ggreenway.

see: more, trace.

ggreenway · 2021-07-27T17:05:13Z

The last two guarddog tests are taking over 1 minute each, so the test is timing out. Unsure why. Can you try merging main into the PR to see if that helps?

soulxu · 2021-07-29T02:44:35Z

The last two guarddog tests are taking over 1 minute each, so the test is timing out. Unsure why. Can you try merging main into the PR to see if that helps?

got it, thanks

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

ggreenway · 2021-08-02T20:30:03Z

@soulxu CI is fixed, but now's there a merge conflict; please resolve.

ggreenway · 2021-08-02T20:30:09Z

/wait

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

soulxu · 2021-08-03T09:24:29Z

@soulxu CI is fixed, but now's there a merge conflict; please resolve.

resolved, thanks again!

soulxu · 2021-08-03T14:24:28Z

emm....

https://dev.azure.com/cncf/envoy/_build/results?buildId=84261&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881&l=17863

Per-extension coverage failed:
Code coverage for source/common is lower than limit of 96.6 (96.5)
Code coverage for source/common/quic is lower than limit of 91.3 (91.2)

I will take a look at this tomorrow.

@ggreenway do you have any suggestions on fixing the coverage test, just add more tests?

ggreenway · 2021-08-03T17:54:11Z

Looks like this should address the coverage issue. Merge from main to get it. Sorry for all the CI trouble on this PR. a066b5e

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

soulxu · 2021-08-04T01:48:55Z

/retest

repokitteh-read-only · 2021-08-04T01:48:58Z

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

🐱

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

see: more, trace.

ggreenway · 2021-08-04T15:41:13Z

/retest

repokitteh-read-only · 2021-08-04T15:41:16Z

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

🐱

Caused by: a #17334 (comment) was created by @ggreenway.

see: more, trace.

ggreenway · 2021-08-04T16:37:54Z

/retest

repokitteh-read-only · 2021-08-04T16:37:58Z

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

🐱

Caused by: a #17334 (comment) was created by @ggreenway.

see: more, trace.

…bridge-stream * upstream/main: (32 commits) tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334) conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577) api: LEDS api introduction (envoyproxy#17419) kafka: add support for api versions request in mesh-filter (envoyproxy#17475) ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531) tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505) xds: restructure CertificateProvider fields (envoyproxy#17201) Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530) listener: move active connection collection out of active tcp listener (envoyproxy#16947) tools: format checks for backticks (envoyproxy#17566) coverage: set lower limit for common/quic and common (envoyproxy#17573) v2: final source removal (envoyproxy#17565) test: bumping coverage (envoyproxy#17564) quic: enforcing header size and contents (envoyproxy#17520) Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137) listener: add a stat for transport socket connect timeout (envoyproxy#17458) listener: add listen() error handling (envoyproxy#17427) http: return per route config when direct response is set (envoyproxy#17449) removing most v2 references from source/ (envoyproxy#17415) bug fix: return bootstrap when validating config (envoyproxy#17499) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

…17334) Part of envoyproxy#17168 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu added 5 commits July 12, 2021 06:22

Add ssl info interface to SocketAddressProvider

cad3fa0

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

Setting the downstream ssl connection inside the ConnectionImpl

288b40a

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

Setting the upstream ssl connection through socketaddressprovider

2dffc9f

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

remove the StreamInfo::upstreamSslConnection

f9881a3

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

fix format

3b56911

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

soulxu requested a review from lizan as a code owner July 14, 2021 07:56

soulxu commented Jul 14, 2021

View reviewed changes

snowp assigned ggreenway Jul 14, 2021

soulxu added 3 commits July 15, 2021 05:37

silence the null pointer warning

d04acbd

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

Merge branch 'main' into move_ssl_info_2

f18d2af

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

fix format

2df315a

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

ggreenway requested changes Jul 21, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 21, 2021

soulxu added 5 commits July 24, 2021 12:18

Revert "fix format"

f4f5746

This reverts commit 2df315a. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Revert "silence the null pointer warning"

e4c648d

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

Revert "fix format"

4801259

This reverts commit 3b56911. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Revert "remove the StreamInfo::upstreamSslConnection"

69c9244

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

Revert "Setting the upstream ssl connection through socketaddressprov…

e05ab4e

…ider" This reverts commit 2dffc9f. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jul 24, 2021

soulxu added 5 commits July 25, 2021 00:22

fix format

03377f8

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

Merge branch 'main' into move_ssl_info_2

60b8ed5

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

rename downstreamSslConnection to sslConnection

7d1729c

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

fix format

93073ae

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

fix the lua test

27426b8

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

Merge branch 'main' into move_ssl_info_2

8238485

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

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

Merge branch 'main' into move_ssl_info_2

e996cc8

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

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

Merge branch 'main' into move_ssl_info_2

c14b661

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

ggreenway approved these changes Aug 4, 2021

View reviewed changes

ggreenway merged commit 371099f into envoyproxy:main Aug 4, 2021

soulxu mentioned this pull request Aug 15, 2021

Moving the connection info into a single provider #17168

Closed

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

tls: move ssl connection info into SocketAddressProvider (envoyproxy#…

18e7ce1

…17334) Part of envoyproxy#17168 Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Conversation

soulxu commented Jul 14, 2021

Uh oh!

soulxu commented Jul 14, 2021

Uh oh!

soulxu Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Jul 19, 2021

Uh oh!

repokitteh-read-only bot commented Jul 19, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

soulxu Jul 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soulxu commented Jul 26, 2021

Uh oh!

repokitteh-read-only bot commented Jul 26, 2021

Uh oh!

soulxu commented Jul 26, 2021

Uh oh!

repokitteh-read-only bot commented Jul 26, 2021

Uh oh!

soulxu commented Jul 26, 2021

Uh oh!

ggreenway commented Jul 27, 2021

Uh oh!

repokitteh-read-only bot commented Jul 27, 2021

Uh oh!

ggreenway commented Jul 27, 2021

Uh oh!

soulxu commented Jul 29, 2021

Uh oh!

ggreenway commented Aug 2, 2021

Uh oh!

ggreenway commented Aug 2, 2021

Uh oh!

soulxu commented Aug 3, 2021

Uh oh!

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

Uh oh!

Uh oh!

ggreenway commented Aug 3, 2021

Uh oh!

soulxu commented Aug 4, 2021

Uh oh!

repokitteh-read-only bot commented Aug 4, 2021

Uh oh!

ggreenway commented Aug 4, 2021

Uh oh!

repokitteh-read-only bot commented Aug 4, 2021

Uh oh!

ggreenway commented Aug 4, 2021

Uh oh!

repokitteh-read-only bot commented Aug 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

soulxu Jul 14, 2021 •

edited

Loading

soulxu Jul 22, 2021 •

edited

Loading

soulxu commented Aug 3, 2021 •

edited

Loading