AsyncClientFactory: use getThreadLocalCluster for thread safety by wozz · Pull Request #15227 · envoyproxy/envoy

wozz · 2021-02-28T05:18:19Z

Commit Message: AsyncClientFactory: use getThreadLocalCluster for thread safety
Additional Description: due to #14204, AsyncClientFactory should use ClusterManager::getThreadLocalCluster to ensure that checking the cluster status can be performed on main or worker threads.
Risk Level: low
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

due to envoyproxy#14204, doing static cluster checks for AsyncClientFactory can only be done on the main thread. adds a ENVOY_BUG check to ensure that cluster checks are skipped when not running on the main thread. Signed-off-by: Michael Wozniak <wozz@koh.ms> Signed-off-by: wozz <wozz@users.noreply.github.com>

Signed-off-by: Michael Wozniak <wozz@koh.ms>

mattklein123

Thanks for adding this check. One question.

/wait

source/common/grpc/async_client_manager_impl.cc

Signed-off-by: Michael Wozniak <wozz@koh.ms>

mattklein123 · 2021-03-01T18:32:47Z

source/common/grpc/async_client_manager_impl.cc

+  ASSERT(Thread::MainThread::isMainThread(),
+         "async client factory cluster checks should only be performed on the main thread");


I'm really sorry, I should have looked at this in more detail, but instead of using the clusters() api should we just be using the get() API which is thread local? This code seems broken. If we did that could we remove the skip clusters check param entirely?

I guess we can use the get api, but the skip_clusters_check would become require_static_cluster? I'm not sure what the implications are of a cluster being statically defined vs provided by API or if there are cases that actually require statically defined clusters or if that requirement is due to some legacy behavior that's no longer necessary.

actually, I think I can use this: https://github.com/envoyproxy/envoy/blob/main/source/common/config/utility.h#L116..L128

Yes ^ exactly, thanks.

/wait

the utility function has the same issue, relying on ClusterManager::clusters(), so I've just updated this one instance to use getThreadLocalCluster and maintain the same semantics as previously for the skip_cluster_checks param.

The async client factory should only ever be used on the main thread. I think it might be worth asserting that, since you probably will get other kinds of broken behavior if you try use it on a worker thread.

This is an example where it's currently being used on a worker thread: https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/ext_authz/config.cc#L80..L89

Does that need to change?

Hmm, good point actually, I think what you have makes sense then. CC @chaoqin-li1123 for visibility into this PR.

Signed-off-by: Michael Wozniak <wozz@koh.ms>

mattklein123 · 2021-03-02T19:09:28Z

include/envoy/grpc/async_client_manager.h


 using AsyncClientFactoryPtr = std::unique_ptr<AsyncClientFactory>;

+enum class AsyncClientFactoryClusterChecks { Skip, ValidateStatic, ValidateStaticDuringBootstrap };


Where is ValidateStaticDuringBootstrap used in prod code? I don't see it?

it's effectively used here: https://github.com/envoyproxy/envoy/blob/main/source/server/server.cc#L620..L621 in factoryForGrpcApiConfigSource

This is the place where getThreadLocalCluster doesn't work.

OK got it. It really seems like we should be able to do something better here but I'm struggling to think of what that better thing is so I agree we can go with this right now. I will take a look once passing CI.

Signed-off-by: Michael Wozniak <wozz@koh.ms>

mattklein123

Thanks!

…ty (envoyproxy#15227)" This reverts commit 296d106. Signed-off-by: Michael Wozniak <wozz@koh.ms>

…ty (#15227)" (#15449) This reverts commit 296d106. Signed-off-by: Michael Wozniak <wozz@koh.ms>

…ty (envoyproxy#15227)" (envoyproxy#15449) This reverts commit 296d106. Signed-off-by: Michael Wozniak <wozz@koh.ms> Signed-off-by: Auni Ahsan <auni@google.com>

wozz force-pushed the patch-1 branch from 8a2fc9c to c0ad021 Compare February 28, 2021 06:09

skip cluster checks for grpc access logger

899fd3a

Signed-off-by: Michael Wozniak <wozz@koh.ms>

wozz requested a review from zuercher as a code owner February 28, 2021 22:53

update corresponding tests

22f92f7

Signed-off-by: Michael Wozniak <wozz@koh.ms>

wozz requested review from htuch and yanavlasov as code owners February 28, 2021 23:54

alyssawilk assigned mattklein123 Mar 1, 2021

mattklein123 requested changes Mar 1, 2021

View reviewed changes

source/common/grpc/async_client_manager_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 1, 2021

switch to ASSERT

7cb3ba2

Signed-off-by: Michael Wozniak <wozz@koh.ms>

repokitteh-read-only bot removed the waiting label Mar 1, 2021

wozz changed the title ~~AsyncClientFactory: add bug check for main thread~~ AsyncClientFactory: add assert for main thread Mar 1, 2021

mattklein123 reviewed Mar 1, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 1, 2021

fix call to clusters()

cae753f

Signed-off-by: Michael Wozniak <wozz@koh.ms>

repokitteh-read-only bot removed the waiting label Mar 1, 2021

wozz changed the title ~~AsyncClientFactory: add assert for main thread~~ AsyncClientFactory: use getThreadLocalCluster for thread safety Mar 1, 2021

use enum to distinguish bootstrap vs non-bootstrap

a56b2d7

Signed-off-by: Michael Wozniak <wozz@koh.ms>

wozz requested review from PiotrSikora, Shikugawa, dio, jmarantz and lizan as code owners March 2, 2021 05:13

wozz added 4 commits March 1, 2021 23:20

fix

312aabf

Signed-off-by: Michael Wozniak <wozz@koh.ms>

update namespace

69f1974

Signed-off-by: Michael Wozniak <wozz@koh.ms>

more namespaces missing

10cc9b9

Signed-off-by: Michael Wozniak <wozz@koh.ms>

more updates

a86f543

Signed-off-by: Michael Wozniak <wozz@koh.ms>

mattklein123 reviewed Mar 2, 2021

View reviewed changes

updates

919d35b

Signed-off-by: Michael Wozniak <wozz@koh.ms>

tracing during bootstrap

2b55fca

Signed-off-by: Michael Wozniak <wozz@koh.ms>

mattklein123 approved these changes Mar 4, 2021

View reviewed changes

mattklein123 merged commit 296d106 into envoyproxy:main Mar 4, 2021

wozz added a commit to wozz/envoy that referenced this pull request Mar 11, 2021

Revert "AsyncClientFactory: use getThreadLocalCluster for thread safe…

4fd2d6a

…ty (envoyproxy#15227)" This reverts commit 296d106. Signed-off-by: Michael Wozniak <wozz@koh.ms>

wozz mentioned this pull request Mar 11, 2021

Revert "AsyncClientFactory: use getThreadLocalCluster for thread safe… #15449

Merged

mattklein123 pushed a commit that referenced this pull request Mar 12, 2021

Revert "AsyncClientFactory: use getThreadLocalCluster for thread safe…

042a3df

…ty (#15227)" (#15449) This reverts commit 296d106. Signed-off-by: Michael Wozniak <wozz@koh.ms>

wozz mentioned this pull request Mar 12, 2021

AsyncClientFactory potentially calls ClusterManager::clusters() outside main thread #15458

Open

		ASSERT(Thread::MainThread::isMainThread(),
		"async client factory cluster checks should only be performed on the main thread");


		using AsyncClientFactoryPtr = std::unique_ptr<AsyncClientFactory>;

		enum class AsyncClientFactoryClusterChecks { Skip, ValidateStatic, ValidateStaticDuringBootstrap };

Conversation

wozz commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wozz commented Feb 28, 2021 •

edited

Loading