quic: test fast-fail if secrets not loaded when create quic connection by YaoZengzeng · Pull Request #18705 · envoyproxy/envoy

YaoZengzeng · 2021-10-21T08:14:01Z

Commit Message: quic: test fast-fail if secrets not loaded when create quic connection
Additional Description: N/A
Risk Level: Low
Testing: N/A
Docs Changes: N/A

RyanTheOptimist · 2021-10-21T18:27:12Z

Do you have any context for this change?
Please make sure we have a new unit/integration test.
It looks like there's a build problem at the moment which should have been fixed by #18708. I recommend syncing and trying again?

Also, please fill out the required fields in the PR description. For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

/wait

RyanTheOptimist · 2021-10-21T18:27:17Z

/wait

YaoZengzeng · 2021-10-22T01:30:19Z

@RyanTheOptimist because the function createQuicNetworkConnection() (https://github.com/envoyproxy/envoy/blob/main/source/common/quic/client_connection_factory_impl.cc#L65) would return nullptr, so need this fix to avoid invalid reference.

danzh2010 · 2021-10-25T16:05:56Z

@RyanTheOptimist because the function createQuicNetworkConnection() (https://github.com/envoyproxy/envoy/blob/main/source/common/quic/client_connection_factory_impl.cc#L65) would return nullptr, so need this fix to avoid invalid reference.

It looks like quic upstream would reference nullptr if using SDS without this fix. Could you please add a regression test for this fix?

YaoZengzeng · 2021-10-26T01:14:36Z

@RyanTheOptimist because the function createQuicNetworkConnection() (https://github.com/envoyproxy/envoy/blob/main/source/common/quic/client_connection_factory_impl.cc#L65) would return nullptr, so need this fix to avoid invalid reference.

It looks like quic upstream would reference nullptr if using SDS without this fix. Could you please add a regression test for this fix?

I'll do it :)

RyanTheOptimist · 2021-10-26T16:40:30Z

/wait

YaoZengzeng · 2021-10-28T06:20:09Z

It seems there are fast-fail to check if secret is loaded (https://github.com/envoyproxy/envoy/blob/main/source/common/http/http3/conn_pool.cc#L88), so if the secrets will not change to empty again, and the createQuicNetworkConnection will not return nullptr.

I wonder if it's necessary to double check in createQuicNetworkConnection. If do it, it is necessary to confirm the return value of createQuicNetworkConnection out of defense.

Although it's necessary to add a test for fast-fail if secrets not loaded, and I'd like to do it:)

RyanTheOptimist

Thanks for the test!

RyanTheOptimist · 2021-10-28T14:00:34Z

test/common/http/http3/conn_pool_test.cc

Does this object leak, or does the dispatcher end up returning it at some point? If the latter, please add a short comment which says that.

RyanTheOptimist · 2021-10-28T14:02:14Z

test/common/http/http3/conn_pool_test.cc

nit: Please use ConnectionPool::InstancePtr instead of auto (assuming that's the right type).

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng · 2021-10-29T07:52:29Z

@RyanTheOptimist updated. Errors in CI don't appear to be related to this PR.

RyanTheOptimist · 2021-10-29T13:53:04Z

It seems there are fast-fail to check if secret is loaded (https://github.com/envoyproxy/envoy/blob/main/source/common/http/http3/conn_pool.cc#L88), so if the secrets will not change to empty again, and the createQuicNetworkConnection will not return nullptr.

I wonder if it's necessary to double check in createQuicNetworkConnection. If do it, it is necessary to confirm the return value of createQuicNetworkConnection out of defense.

Although it's necessary to add a test for fast-fail if secrets not loaded, and I'd like to do it:)

So to clarify, is the new test testing this existing return nullptr (on line 95) or the new code you added? If it's testing the existing code, can we get a test to cover the new code?

RyanTheOptimist · 2021-10-29T13:54:08Z

@RyanTheOptimist updated. Errors in CI don't appear to be related to this PR.

I had two comments from yesterday that don't seem to be address. Have they been address?

A note for the future: force pushes breaks the reviewing flow, so please avoid doing so while iterating on the PR. We end up squashing the commits on merge anyways, so it leaving the commits up doesn't matter.

RyanTheOptimist · 2021-10-29T13:57:05Z

/retest

repokitteh-read-only · 2021-10-29T13:57:09Z

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

🐱

Caused by: a #18705 (comment) was created by @RyanTheOptimist.

see: more, trace.

YaoZengzeng · 2021-10-29T14:11:29Z

@RyanTheOptimist updated. Errors in CI don't appear to be related to this PR.

I had two comments from yesterday that don't seem to be address. Have they been address?

A note for the future: force pushes breaks the reviewing flow, so please avoid doing so while iterating on the PR. We end up squashing the commits on merge anyways, so it leaving the commits up doesn't matter.

Both have been fixed, add comment for suspected leak and change auto to specific type.

I used to keep the commit log clean :) But thanks for the advice, I'll follow it next time.

YaoZengzeng · 2021-10-29T14:15:44Z

It seems there are fast-fail to check if secret is loaded (https://github.com/envoyproxy/envoy/blob/main/source/common/http/http3/conn_pool.cc#L88), so if the secrets will not change to empty again, and the createQuicNetworkConnection will not return nullptr.
I wonder if it's necessary to double check in createQuicNetworkConnection. If do it, it is necessary to confirm the return value of createQuicNetworkConnection out of defense.
Although it's necessary to add a test for fast-fail if secrets not loaded, and I'd like to do it:)

So to clarify, is the new test testing this existing return nullptr (on line 95) or the new code you added? If it's testing the existing code, can we get a test to cover the new code?

New testing code is for the existing return nullptr. And I think the new code which I added is just out of defence, I don't think if the secrets have been loaded before createQuicNetworkConnection, it will become empty in that function.

RyanTheOptimist · 2021-10-29T23:47:00Z

It seems there are fast-fail to check if secret is loaded (https://github.com/envoyproxy/envoy/blob/main/source/common/http/http3/conn_pool.cc#L88), so if the secrets will not change to empty again, and the createQuicNetworkConnection will not return nullptr.
I wonder if it's necessary to double check in createQuicNetworkConnection. If do it, it is necessary to confirm the return value of createQuicNetworkConnection out of defense.
Although it's necessary to add a test for fast-fail if secrets not loaded, and I'd like to do it:)

So to clarify, is the new test testing this existing return nullptr (on line 95) or the new code you added? If it's testing the existing code, can we get a test to cover the new code?

New testing code is for the existing return nullptr. And I think the new code which I added is just out of defence, I don't think if the secrets have been loaded before createQuicNetworkConnection, it will become empty in that function.

Interesting. I wonder what would happen if you duplicated your new tests, but changed:
EXPECT_CALL(*config, isReady()).WillRepeatedly(Return(false));
So that the first call returns true (and hence the old return nullptr is not triggered) but second call returns false and hence hits the new return nullptr. What do you think

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng · 2021-10-30T09:39:26Z

@RyanTheOptimist Updated. Actually we can't manipulate isReady(), because it's triggered by secrets change. But we can't precisely make the secret change after fast-fail but before createQuicNetworkConnection.

So I create a mock QuicClientTransportSocketFactory and to achive the goal by changing the return value of method ssl_context :)

YaoZengzeng · 2021-10-30T14:33:51Z

/retest

repokitteh-read-only · 2021-10-30T14:33:55Z

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

🐱

Caused by: a #18705 (comment) was created by @YaoZengzeng.

see: more, trace.

RyanTheOptimist

This looks good to me. @danzh2010 what do you think?

RyanTheOptimist · 2021-11-01T21:52:36Z

/approve-from envoyproxy/senior-maintainers

RyanTheOptimist · 2021-11-01T21:52:53Z

/assign-from envoyproxy/senior-maintainers

repokitteh-read-only · 2021-11-01T21:52:57Z

envoyproxy/senior-maintainers assignee is @ggreenway

🐱

Caused by: a #18705 (comment) was created by @RyanTheOptimist.

see: more, trace.

ggreenway

/wait-any

ggreenway · 2021-11-02T17:05:31Z

source/common/http/http3/conn_pool.cc

                                              source_address, quic_stat_names, scope);
+        if (data.connection_ == nullptr) {
+          ENVOY_LOG_TO_LOGGER(
+              Envoy::Logger::Registry::getLog(Envoy::Logger::Id::pool), warn,


Is warn going to be too much output in the log? I presume if this happens once, it'll probably happen for every connection attempt. Maybe use ENVOY_LOG_PERIODIC_TO_LOGGER or ENVOY_LOG_EVERY_POW_2_TO_LOGGER?

Is there any failure metric that increases in this case?

@ggreenway updated to ENVOY_LOG_EVERY_POW_2_TO_LOGGER.

As for failure metric, there seems to be no metric to indicate the failure of quic connection creation. From my research, even for HTTP 1/2, there is only a metric bind_errors_ which is used to indicate the bind error when failed to create client network connection.

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng force-pushed the null branch from 39a80c6 to 03f96f3 Compare October 21, 2021 12:05

htuch assigned RyanTheOptimist Oct 21, 2021

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

YaoZengzeng force-pushed the null branch from 03f96f3 to d490f42 Compare October 22, 2021 01:15

repokitteh-read-only bot removed the waiting label Oct 22, 2021

repokitteh-read-only bot added the waiting label Oct 26, 2021

YaoZengzeng force-pushed the null branch from d490f42 to cd15ead Compare October 28, 2021 06:47

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

YaoZengzeng changed the title ~~quic: return nullptr if create quic network connection failed~~ quic: test fast-fail if secrets not loaded when create quic connection Oct 28, 2021

YaoZengzeng force-pushed the null branch 4 times, most recently from e148a6a to 3f4bf3a Compare October 28, 2021 10:38

RyanTheOptimist reviewed Oct 28, 2021

View reviewed changes

YaoZengzeng force-pushed the null branch 3 times, most recently from b2d2a25 to 73d3b2f Compare October 29, 2021 03:16

quic: test fast-fail if secrets not loaded when create quic connection

0ea6649

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng force-pushed the null branch from 73d3b2f to 0ea6649 Compare October 29, 2021 05:53

YaoZengzeng added 2 commits October 30, 2021 17:11

MockQuicClientTransportFactory

32a8cc2

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

output log if create connection failed

b4d5695

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

RyanTheOptimist previously approved these changes Nov 1, 2021

View reviewed changes

danzh2010 previously approved these changes Nov 1, 2021

View reviewed changes

repokitteh-read-only bot assigned ggreenway Nov 1, 2021

ggreenway requested changes Nov 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Nov 2, 2021

ENVOY_LOG_TO_LOGGER to ENVOY_LOG_EVERY_POW_2_TO_LOGGER

22feb33

Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>

YaoZengzeng dismissed stale reviews from danzh2010 and RyanTheOptimist via 22feb33 November 3, 2021 07:24

repokitteh-read-only bot removed the waiting:any label Nov 3, 2021

ggreenway approved these changes Nov 4, 2021

View reviewed changes

ggreenway merged commit 361fd53 into envoyproxy:main Nov 4, 2021

Conversation

YaoZengzeng commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanTheOptimist commented Oct 21, 2021

Uh oh!

RyanTheOptimist commented Oct 21, 2021

Uh oh!

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

Uh oh!

Uh oh!

danzh2010 commented Oct 25, 2021

Uh oh!

YaoZengzeng commented Oct 26, 2021

Uh oh!

RyanTheOptimist commented Oct 26, 2021

Uh oh!

YaoZengzeng commented Oct 28, 2021

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng commented Oct 29, 2021

Uh oh!

RyanTheOptimist commented Oct 29, 2021

Uh oh!

RyanTheOptimist commented Oct 29, 2021

Uh oh!

RyanTheOptimist commented Oct 29, 2021

Uh oh!

repokitteh-read-only bot commented Oct 29, 2021

Uh oh!

YaoZengzeng commented Oct 29, 2021

Uh oh!

YaoZengzeng commented Oct 29, 2021

Uh oh!

RyanTheOptimist commented Oct 29, 2021

Uh oh!

YaoZengzeng commented Oct 30, 2021

Uh oh!

YaoZengzeng commented Oct 30, 2021

Uh oh!

repokitteh-read-only bot commented Oct 30, 2021

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Nov 1, 2021

Uh oh!

RyanTheOptimist commented Nov 1, 2021

Uh oh!

repokitteh-read-only bot commented Nov 1, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

YaoZengzeng commented Oct 21, 2021 •

edited

Loading

YaoZengzeng commented Oct 22, 2021 •

edited

Loading