Skip to content

[grpc]: fix ex_authz grpc client race condition #17619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yanavlasov merged 21 commits into from
Aug 13, 2021
Merged

[grpc]: fix ex_authz grpc client race condition #17619

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
cec90fa
initial
Aug 6, 2021
68c1ab3
fix format
Aug 7, 2021
b1f6e1f
add test
Aug 7, 2021
d7ca14f
clean up test
Aug 7, 2021
4e1da1c
add comment
Aug 7, 2021
a8d9e2d
format
Aug 7, 2021
f940e29
add thread consistency check
Aug 8, 2021
c48535c
add thread consistency test
Aug 9, 2021
aea819b
add test
Aug 9, 2021
4873dc9
add comment
Aug 9, 2021
691119a
clean up test
Aug 9, 2021
9f5b439
fix death test
Aug 10, 2021
b761e4c
fix death test
Aug 10, 2021
9a5bef9
fix format
Aug 10, 2021
f15e520
fix death test
Aug 10, 2021
b6126d0
fix death test
Aug 10, 2021
9e5e4d5
fix death test
Aug 10, 2021
e11520a
fix death test
Aug 10, 2021
21c9094
fix death test
Aug 10, 2021
acd1d3d
execute factory callback from a seperate thread
Aug 11, 2021
86577ef
remove v2 test
Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions source/extensions/filters/http/ext_authz/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped(
const auto filter_config = std::make_shared<FilterConfig>(
proto_config, context.scope(), context.runtime(), context.httpContext(), stats_prefix,
context.getServerFactoryContext().bootstrap());
// The callback is created in main thread and executed in worker thread, variables except factory
// context must be captured by value into the callback.
Http::FilterFactoryCb callback;

if (proto_config.has_http_service()) {
Expand All @@ -42,7 +44,6 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped(
};
} else if (proto_config.grpc_service().has_google_grpc()) {
// Google gRPC client.

const uint32_t timeout_ms =
PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, DefaultTimeout);

Expand All @@ -57,15 +58,14 @@ Http::FilterFactoryCb ExtAuthzFilterConfig::createFilterFactoryFromProtoTyped(
};
} else {
// Envoy gRPC client.

Grpc::RawAsyncClientSharedPtr raw_client =
context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClient(
proto_config.grpc_service(), context.scope(), true, Grpc::CacheOption::AlwaysCache);
const uint32_t timeout_ms =
PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, DefaultTimeout);
callback = [raw_client, filter_config, timeout_ms,
callback = [grpc_service = proto_config.grpc_service(), &context, filter_config, timeout_ms,
Comment thread
chaoqin-li1123 marked this conversation as resolved.
transport_api_version = Config::Utility::getAndCheckTransportVersion(proto_config)](
Http::FilterChainFactoryCallbacks& callbacks) {
Grpc::RawAsyncClientSharedPtr raw_client =
context.clusterManager().grpcAsyncClientManager().getOrCreateRawAsyncClient(
grpc_service, context.scope(), true, Grpc::CacheOption::AlwaysCache);
auto client = std::make_unique<Filters::Common::ExtAuthz::GrpcClientImpl>(
raw_client, std::chrono::milliseconds(timeout_ms), transport_api_version);
callbacks.addStreamFilter(std::make_shared<Filter>(filter_config, std::move(client)));
Expand Down
60 changes: 41 additions & 19 deletions test/extensions/filters/http/ext_authz/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,18 @@ namespace HttpFilters {
namespace ExtAuthz {
namespace {

void expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion api_version) {
void expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion api_version,
std::string const& grpc_service_yaml) {
std::unique_ptr<TestDeprecatedV2Api> _deprecated_v2_api;
if (api_version != envoy::config::core::v3::ApiVersion::V3) {
_deprecated_v2_api = std::make_unique<TestDeprecatedV2Api>();
}
std::string yaml = R"EOF(
transport_api_version: V3
grpc_service:
google_grpc:
target_uri: ext_authz_server
stat_prefix: google
failure_mode_allow: false
transport_api_version: {}
)EOF";

ExtAuthzFilterConfig factory;
ProtobufTypes::MessagePtr proto_config = factory.createEmptyConfigProto();
TestUtility::loadFromYaml(
fmt::format(yaml, TestUtility::getVersionStringFromApiVersion(api_version)), *proto_config);
fmt::format(grpc_service_yaml, TestUtility::getVersionStringFromApiVersion(api_version)),
*proto_config);

testing::StrictMock<Server::Configuration::MockFactoryContext> context;
testing::StrictMock<Server::Configuration::MockServerFactoryContext> server_context;
Expand All @@ -49,26 +42,55 @@ void expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion api_version) {
EXPECT_CALL(context, clusterManager());
EXPECT_CALL(context, runtime());
EXPECT_CALL(context, scope()).Times(2);

Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
// Expect the raw async client to be created inside the callback.
// The creation of the filter callback is in main thread while the execution of callback is in
// worker thread. Because of the thread local cache of async client, it must be created in worker
// thread inside the callback.
EXPECT_CALL(context.cluster_manager_.async_client_manager_, getOrCreateRawAsyncClient(_, _, _, _))
.WillOnce(Invoke(
[](const envoy::config::core::v3::GrpcService&, Stats::Scope&, bool, Grpc::CacheOption) {
return std::make_unique<NiceMock<Grpc::MockAsyncClient>>();
}));

Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(*proto_config, "stats", context);
Http::MockFilterChainFactoryCallbacks filter_callback;
EXPECT_CALL(filter_callback, addStreamFilter(_));
cb(filter_callback);

@yanavlasov yanavlasov Aug 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be useful to add test that calls cb on a separate thread and verifies that getOrCreateRawAsyncClient is called on that separate thread.

@chaoqin-li1123 chaoqin-li1123 Aug 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, test added.

}

} // namespace

TEST(HttpExtAuthzConfigTest, CorrectProtoGrpc) {
TEST(HttpExtAuthzConfigTest, CorrectProtoGoogleGrpc) {
std::string google_grpc_service_yaml = R"EOF(
transport_api_version: V3
grpc_service:
google_grpc:
target_uri: ext_authz_server
stat_prefix: google
failure_mode_allow: false
transport_api_version: {}
)EOF";
#ifndef ENVOY_DISABLE_DEPRECATED_FEATURES
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::AUTO, google_grpc_service_yaml);
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::V2, google_grpc_service_yaml);

@htuch htuch Aug 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: not worth covering V2, suggest removing that, and we might move AUTO to be v3 by default, so maybe just leave as a TODO.

#endif
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::V3, google_grpc_service_yaml);
}

TEST(HttpExtAuthzConfigTest, CorrectProtoEnvoyGrpc) {
std::string envoy_grpc_service_yaml = R"EOF(
transport_api_version: V3
grpc_service:
envoy_grpc:
cluster_name: ext_authz_server
failure_mode_allow: false
transport_api_version: {}
)EOF";
#ifndef ENVOY_DISABLE_DEPRECATED_FEATURES
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::AUTO);
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::V2);
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::AUTO, envoy_grpc_service_yaml);
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::V2, envoy_grpc_service_yaml);
#endif
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::V3);
expectCorrectProtoGrpc(envoy::config::core::v3::ApiVersion::V3, envoy_grpc_service_yaml);
}

TEST(HttpExtAuthzConfigTest, CorrectProtoHttp) {
Expand Down