From 384f15ab6eb68610d04b1149e1341564f428e2e4 Mon Sep 17 00:00:00 2001 From: yang-g Date: Fri, 24 May 2019 14:46:12 -0700 Subject: [PATCH 1/5] Delay calling plugin_creds callback --- .../credentials/plugin/plugin_credentials.cc | 62 +++++-------- .../credentials/plugin/plugin_credentials.h | 7 +- .../security/transport/client_auth_filter.cc | 2 - test/cpp/end2end/end2end_test.cc | 88 +++++++++++++++---- 4 files changed, 94 insertions(+), 65 deletions(-) diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.cc b/src/core/lib/security/credentials/plugin/plugin_credentials.cc index 333366d0f0a..8e926de59da 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.cc +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.cc @@ -54,15 +54,10 @@ void grpc_plugin_credentials::pending_request_remove_locked( } } -// Checks if the request has been cancelled. -// If not, removes it from the pending list, so that it cannot be -// cancelled out from under us. -// When this returns, r->cancelled indicates whether the request was -// cancelled before completion. void grpc_plugin_credentials::pending_request_complete(pending_request* r) { GPR_DEBUG_ASSERT(r->creds == this); gpr_mu_lock(&mu_); - if (!r->cancelled) pending_request_remove_locked(r); + pending_request_remove_locked(r); gpr_mu_unlock(&mu_); // Ref to credentials not needed anymore. Unref(); @@ -76,7 +71,8 @@ static grpc_error* process_plugin_result( char* msg; gpr_asprintf(&msg, "Getting metadata from plugin failed with error: %s", error_details); - error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); + error = grpc_error_set_int(GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg), + GRPC_ERROR_INT_GRPC_STATUS, status); gpr_free(msg); } else { bool seen_illegal_header = false; @@ -95,7 +91,9 @@ static grpc_error* process_plugin_result( } } if (seen_illegal_header) { - error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal metadata"); + error = grpc_error_set_int( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal metadata."), + GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_INTERNAL); } else { for (size_t i = 0; i < num_md; ++i) { grpc_mdelem mdelem = @@ -125,19 +123,18 @@ static void plugin_md_request_metadata_ready(void* request, "asynchronously", r->creds, r); } - // Remove request from pending list if not previously cancelled. + // Remove request from pending list r->creds->pending_request_complete(r); // If it has not been cancelled, process it. - if (!r->cancelled) { - grpc_error* error = - process_plugin_result(r, md, num_md, status, error_details); - GRPC_CLOSURE_SCHED(r->on_request_metadata, error); + if (r->error == GRPC_ERROR_NONE) { + r->error = process_plugin_result(r, md, num_md, status, error_details); } else if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { gpr_log(GPR_INFO, "plugin_credentials[%p]: request %p: plugin was previously " "cancelled", r->creds, r); } + GRPC_CLOSURE_SCHED(r->on_request_metadata, r->error); gpr_free(r); } @@ -145,11 +142,11 @@ bool grpc_plugin_credentials::get_request_metadata( grpc_polling_entity* pollent, grpc_auth_metadata_context context, grpc_credentials_mdelem_array* md_array, grpc_closure* on_request_metadata, grpc_error** error) { - bool retval = true; // Synchronous return. if (plugin_.get_metadata != nullptr) { // Create pending_request object. pending_request* request = static_cast(gpr_zalloc(sizeof(*request))); + request->error = GRPC_ERROR_NONE; request->creds = this; request->md_array = md_array; request->on_request_metadata = on_request_metadata; @@ -183,29 +180,16 @@ bool grpc_plugin_credentials::get_request_metadata( return false; // Asynchronous return. } // Returned synchronously. - // Remove request from pending list if not previously cancelled. + // Remove request from pending list. request->creds->pending_request_complete(request); - // If the request was cancelled, the error will have been returned - // asynchronously by plugin_cancel_get_request_metadata(), so return - // false. Otherwise, process the result. - if (request->cancelled) { - if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { - gpr_log(GPR_INFO, - "plugin_credentials[%p]: request %p was cancelled, error " - "will be returned asynchronously", - this, request); - } - retval = false; - } else { - if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { - gpr_log(GPR_INFO, - "plugin_credentials[%p]: request %p: plugin returned " - "synchronously", - this, request); - } - *error = process_plugin_result(request, creds_md, num_creds_md, status, - error_details); + if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { + gpr_log(GPR_INFO, + "plugin_credentials[%p]: request %p: plugin returned " + "synchronously", + this, request); } + *error = process_plugin_result(request, creds_md, num_creds_md, status, + error_details); // Clean up. for (size_t i = 0; i < num_creds_md; ++i) { grpc_slice_unref_internal(creds_md[i].key); @@ -214,7 +198,7 @@ bool grpc_plugin_credentials::get_request_metadata( gpr_free((void*)error_details); gpr_free(request); } - return retval; + return true; // Synchronous return. } void grpc_plugin_credentials::cancel_get_request_metadata( @@ -227,15 +211,11 @@ void grpc_plugin_credentials::cancel_get_request_metadata( gpr_log(GPR_INFO, "plugin_credentials[%p]: cancelling request %p", this, pending_request); } - pending_request->cancelled = true; - GRPC_CLOSURE_SCHED(pending_request->on_request_metadata, - GRPC_ERROR_REF(error)); - pending_request_remove_locked(pending_request); + pending_request->error = error; break; } } gpr_mu_unlock(&mu_); - GRPC_ERROR_UNREF(error); } grpc_plugin_credentials::grpc_plugin_credentials( diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.h b/src/core/lib/security/credentials/plugin/plugin_credentials.h index 77a957e5137..7350b37f8a5 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.h +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.h @@ -31,7 +31,7 @@ extern grpc_core::TraceFlag grpc_plugin_credentials_trace; struct grpc_plugin_credentials final : public grpc_call_credentials { public: struct pending_request { - bool cancelled; + grpc_error* error; struct grpc_plugin_credentials* creds; grpc_credentials_mdelem_array* md_array; grpc_closure* on_request_metadata; @@ -51,11 +51,6 @@ struct grpc_plugin_credentials final : public grpc_call_credentials { void cancel_get_request_metadata(grpc_credentials_mdelem_array* md_array, grpc_error* error) override; - // Checks if the request has been cancelled. - // If not, removes it from the pending list, so that it cannot be - // cancelled out from under us. - // When this returns, r->cancelled indicates whether the request was - // cancelled before completion. void pending_request_complete(pending_request* r); private: diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index 0c40dd7ff1e..40393de2b13 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -160,8 +160,6 @@ static void on_credentials_metadata(void* arg, grpc_error* input_error) { if (error == GRPC_ERROR_NONE) { grpc_call_next_op(elem, batch); } else { - error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, - GRPC_STATUS_UNAVAILABLE); grpc_transport_stream_op_batch_finish_with_failure(batch, error, calld->call_combiner); } diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index c027e5954ec..a505d1a633f 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -90,11 +90,13 @@ class TestMetadataCredentialsPlugin : public MetadataCredentialsPlugin { TestMetadataCredentialsPlugin(const grpc::string_ref& metadata_key, const grpc::string_ref& metadata_value, - bool is_blocking, bool is_successful) + bool is_blocking, bool is_successful, + int delay_ms) : metadata_key_(metadata_key.data(), metadata_key.length()), metadata_value_(metadata_value.data(), metadata_value.length()), is_blocking_(is_blocking), - is_successful_(is_successful) {} + is_successful_(is_successful), + delay_ms_(delay_ms) {} bool IsBlocking() const override { return is_blocking_; } @@ -102,6 +104,11 @@ class TestMetadataCredentialsPlugin : public MetadataCredentialsPlugin { grpc::string_ref service_url, grpc::string_ref method_name, const grpc::AuthContext& channel_auth_context, std::multimap* metadata) override { + if (delay_ms_ != 0) { + gpr_sleep_until( + gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), + gpr_time_from_millis(delay_ms_, GPR_TIMESPAN))); + } EXPECT_GT(service_url.length(), 0UL); EXPECT_GT(method_name.length(), 0UL); EXPECT_TRUE(channel_auth_context.IsPeerAuthenticated()); @@ -119,6 +126,7 @@ class TestMetadataCredentialsPlugin : public MetadataCredentialsPlugin { grpc::string metadata_value_; bool is_blocking_; bool is_successful_; + int delay_ms_; }; const char TestMetadataCredentialsPlugin::kBadMetadataKey[] = @@ -137,7 +145,7 @@ class TestAuthMetadataProcessor : public AuthMetadataProcessor { std::unique_ptr( new TestMetadataCredentialsPlugin( TestMetadataCredentialsPlugin::kGoodMetadataKey, kGoodGuy, - is_blocking_, true))); + is_blocking_, true, 0))); } std::shared_ptr GetIncompatibleClientCreds() { @@ -145,7 +153,7 @@ class TestAuthMetadataProcessor : public AuthMetadataProcessor { std::unique_ptr( new TestMetadataCredentialsPlugin( TestMetadataCredentialsPlugin::kGoodMetadataKey, "Mr Hyde", - is_blocking_, true))); + is_blocking_, true, 0))); } // Interface implementation @@ -1835,12 +1843,13 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginKeyFailure) { std::unique_ptr( new TestMetadataCredentialsPlugin( TestMetadataCredentialsPlugin::kBadMetadataKey, - "Does not matter, will fail the key is invalid.", false, true)))); + "Does not matter, will fail the key is invalid.", false, true, + 0)))); request.set_message("Hello"); Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); + EXPECT_EQ(s.error_code(), StatusCode::INTERNAL); } TEST_P(SecureEnd2endTest, AuthMetadataPluginValueFailure) { @@ -1853,12 +1862,59 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginValueFailure) { std::unique_ptr( new TestMetadataCredentialsPlugin( TestMetadataCredentialsPlugin::kGoodMetadataKey, - "With illegal \n value.", false, true)))); + "With illegal \n value.", false, true, 0)))); request.set_message("Hello"); Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); + EXPECT_EQ(s.error_code(), StatusCode::INTERNAL); +} + +TEST_P(SecureEnd2endTest, AuthMetadataPluginWithDeadline) { + MAYBE_SKIP_TEST; + ResetStub(); + EchoRequest request; + EchoResponse response; + ClientContext context; + const int delay = 100; + std::chrono::system_clock::time_point deadline = + std::chrono::system_clock::now() + std::chrono::milliseconds(delay); + context.set_deadline(deadline); + context.set_credentials(grpc::MetadataCredentialsFromPlugin( + std::unique_ptr( + new TestMetadataCredentialsPlugin("meta_key", "Does not matter", true, + true, delay)))); + request.set_message("Hello"); + + Status s = stub_->Echo(&context, request, &response); + if (!s.ok()) { + EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, s.error_code()); + } +} + +TEST_P(SecureEnd2endTest, AuthMetadataPluginWithCancel) { + MAYBE_SKIP_TEST; + ResetStub(); + EchoRequest request; + EchoResponse response; + ClientContext context; + const int delay = 100; + context.set_credentials(grpc::MetadataCredentialsFromPlugin( + std::unique_ptr( + new TestMetadataCredentialsPlugin("meta_key", "Does not matter", true, + true, delay)))); + request.set_message("Hello"); + + std::thread cancel_thread([&context] { + gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), + gpr_time_from_millis(delay, GPR_TIMESPAN))); + context.TryCancel(); + }); + Status s = stub_->Echo(&context, request, &response); + if (!s.ok()) { + EXPECT_EQ(StatusCode::CANCELLED, s.error_code()); + } + cancel_thread.join(); } TEST_P(SecureEnd2endTest, NonBlockingAuthMetadataPluginFailure) { @@ -1871,13 +1927,13 @@ TEST_P(SecureEnd2endTest, NonBlockingAuthMetadataPluginFailure) { std::unique_ptr( new TestMetadataCredentialsPlugin( TestMetadataCredentialsPlugin::kGoodMetadataKey, - "Does not matter, will fail anyway (see 3rd param)", false, - false)))); + "Does not matter, will fail anyway (see 3rd param)", false, false, + 0)))); request.set_message("Hello"); Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); + EXPECT_EQ(s.error_code(), StatusCode::NOT_FOUND); EXPECT_EQ(s.error_message(), grpc::string("Getting metadata from plugin failed with error: ") + kTestCredsPluginErrorMsg); @@ -1935,13 +1991,13 @@ TEST_P(SecureEnd2endTest, BlockingAuthMetadataPluginFailure) { std::unique_ptr( new TestMetadataCredentialsPlugin( TestMetadataCredentialsPlugin::kGoodMetadataKey, - "Does not matter, will fail anyway (see 3rd param)", true, - false)))); + "Does not matter, will fail anyway (see 3rd param)", true, false, + 0)))); request.set_message("Hello"); Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); + EXPECT_EQ(s.error_code(), StatusCode::NOT_FOUND); EXPECT_EQ(s.error_message(), grpc::string("Getting metadata from plugin failed with error: ") + kTestCredsPluginErrorMsg); @@ -1962,11 +2018,11 @@ TEST_P(SecureEnd2endTest, CompositeCallCreds) { grpc::MetadataCredentialsFromPlugin( std::unique_ptr( new TestMetadataCredentialsPlugin(kMetadataKey1, kMetadataVal1, - true, true))), + true, true, 0))), grpc::MetadataCredentialsFromPlugin( std::unique_ptr( new TestMetadataCredentialsPlugin(kMetadataKey2, kMetadataVal2, - true, true))))); + true, true, 0))))); request.set_message("Hello"); request.mutable_param()->set_echo_metadata(true); From 47dbf1dd263e0bf7aba7dbd9651b2bcb570d6889 Mon Sep 17 00:00:00 2001 From: yang-g Date: Tue, 28 May 2019 23:47:29 -0700 Subject: [PATCH 2/5] Second approach --- .../credentials/plugin/plugin_credentials.cc | 62 ++++++++++++------- .../credentials/plugin/plugin_credentials.h | 7 ++- .../lib/security/transport/auth_filters.h | 3 + .../security/transport/client_auth_filter.cc | 16 +++++ src/cpp/client/secure_credentials.cc | 8 ++- test/cpp/end2end/end2end_test.cc | 12 ++-- 6 files changed, 78 insertions(+), 30 deletions(-) diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.cc b/src/core/lib/security/credentials/plugin/plugin_credentials.cc index 8e926de59da..333366d0f0a 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.cc +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.cc @@ -54,10 +54,15 @@ void grpc_plugin_credentials::pending_request_remove_locked( } } +// Checks if the request has been cancelled. +// If not, removes it from the pending list, so that it cannot be +// cancelled out from under us. +// When this returns, r->cancelled indicates whether the request was +// cancelled before completion. void grpc_plugin_credentials::pending_request_complete(pending_request* r) { GPR_DEBUG_ASSERT(r->creds == this); gpr_mu_lock(&mu_); - pending_request_remove_locked(r); + if (!r->cancelled) pending_request_remove_locked(r); gpr_mu_unlock(&mu_); // Ref to credentials not needed anymore. Unref(); @@ -71,8 +76,7 @@ static grpc_error* process_plugin_result( char* msg; gpr_asprintf(&msg, "Getting metadata from plugin failed with error: %s", error_details); - error = grpc_error_set_int(GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg), - GRPC_ERROR_INT_GRPC_STATUS, status); + error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); gpr_free(msg); } else { bool seen_illegal_header = false; @@ -91,9 +95,7 @@ static grpc_error* process_plugin_result( } } if (seen_illegal_header) { - error = grpc_error_set_int( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal metadata."), - GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_INTERNAL); + error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal metadata"); } else { for (size_t i = 0; i < num_md; ++i) { grpc_mdelem mdelem = @@ -123,18 +125,19 @@ static void plugin_md_request_metadata_ready(void* request, "asynchronously", r->creds, r); } - // Remove request from pending list + // Remove request from pending list if not previously cancelled. r->creds->pending_request_complete(r); // If it has not been cancelled, process it. - if (r->error == GRPC_ERROR_NONE) { - r->error = process_plugin_result(r, md, num_md, status, error_details); + if (!r->cancelled) { + grpc_error* error = + process_plugin_result(r, md, num_md, status, error_details); + GRPC_CLOSURE_SCHED(r->on_request_metadata, error); } else if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { gpr_log(GPR_INFO, "plugin_credentials[%p]: request %p: plugin was previously " "cancelled", r->creds, r); } - GRPC_CLOSURE_SCHED(r->on_request_metadata, r->error); gpr_free(r); } @@ -142,11 +145,11 @@ bool grpc_plugin_credentials::get_request_metadata( grpc_polling_entity* pollent, grpc_auth_metadata_context context, grpc_credentials_mdelem_array* md_array, grpc_closure* on_request_metadata, grpc_error** error) { + bool retval = true; // Synchronous return. if (plugin_.get_metadata != nullptr) { // Create pending_request object. pending_request* request = static_cast(gpr_zalloc(sizeof(*request))); - request->error = GRPC_ERROR_NONE; request->creds = this; request->md_array = md_array; request->on_request_metadata = on_request_metadata; @@ -180,16 +183,29 @@ bool grpc_plugin_credentials::get_request_metadata( return false; // Asynchronous return. } // Returned synchronously. - // Remove request from pending list. + // Remove request from pending list if not previously cancelled. request->creds->pending_request_complete(request); - if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { - gpr_log(GPR_INFO, - "plugin_credentials[%p]: request %p: plugin returned " - "synchronously", - this, request); + // If the request was cancelled, the error will have been returned + // asynchronously by plugin_cancel_get_request_metadata(), so return + // false. Otherwise, process the result. + if (request->cancelled) { + if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { + gpr_log(GPR_INFO, + "plugin_credentials[%p]: request %p was cancelled, error " + "will be returned asynchronously", + this, request); + } + retval = false; + } else { + if (GRPC_TRACE_FLAG_ENABLED(grpc_plugin_credentials_trace)) { + gpr_log(GPR_INFO, + "plugin_credentials[%p]: request %p: plugin returned " + "synchronously", + this, request); + } + *error = process_plugin_result(request, creds_md, num_creds_md, status, + error_details); } - *error = process_plugin_result(request, creds_md, num_creds_md, status, - error_details); // Clean up. for (size_t i = 0; i < num_creds_md; ++i) { grpc_slice_unref_internal(creds_md[i].key); @@ -198,7 +214,7 @@ bool grpc_plugin_credentials::get_request_metadata( gpr_free((void*)error_details); gpr_free(request); } - return true; // Synchronous return. + return retval; } void grpc_plugin_credentials::cancel_get_request_metadata( @@ -211,11 +227,15 @@ void grpc_plugin_credentials::cancel_get_request_metadata( gpr_log(GPR_INFO, "plugin_credentials[%p]: cancelling request %p", this, pending_request); } - pending_request->error = error; + pending_request->cancelled = true; + GRPC_CLOSURE_SCHED(pending_request->on_request_metadata, + GRPC_ERROR_REF(error)); + pending_request_remove_locked(pending_request); break; } } gpr_mu_unlock(&mu_); + GRPC_ERROR_UNREF(error); } grpc_plugin_credentials::grpc_plugin_credentials( diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.h b/src/core/lib/security/credentials/plugin/plugin_credentials.h index 7350b37f8a5..77a957e5137 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.h +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.h @@ -31,7 +31,7 @@ extern grpc_core::TraceFlag grpc_plugin_credentials_trace; struct grpc_plugin_credentials final : public grpc_call_credentials { public: struct pending_request { - grpc_error* error; + bool cancelled; struct grpc_plugin_credentials* creds; grpc_credentials_mdelem_array* md_array; grpc_closure* on_request_metadata; @@ -51,6 +51,11 @@ struct grpc_plugin_credentials final : public grpc_call_credentials { void cancel_get_request_metadata(grpc_credentials_mdelem_array* md_array, grpc_error* error) override; + // Checks if the request has been cancelled. + // If not, removes it from the pending list, so that it cannot be + // cancelled out from under us. + // When this returns, r->cancelled indicates whether the request was + // cancelled before completion. void pending_request_complete(pending_request* r); private: diff --git a/src/core/lib/security/transport/auth_filters.h b/src/core/lib/security/transport/auth_filters.h index 16a8e58ed9a..5e73c21cc44 100644 --- a/src/core/lib/security/transport/auth_filters.h +++ b/src/core/lib/security/transport/auth_filters.h @@ -32,6 +32,9 @@ void grpc_auth_metadata_context_build( const grpc_slice& call_method, grpc_auth_context* auth_context, grpc_auth_metadata_context* auth_md_context); +void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from, + grpc_auth_metadata_context* to); + void grpc_auth_metadata_context_reset(grpc_auth_metadata_context* context); #endif /* GRPC_CORE_LIB_SECURITY_TRANSPORT_AUTH_FILTERS_H */ diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index 40393de2b13..dbe288237d4 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -112,6 +112,20 @@ struct call_data { } // namespace +void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from, + grpc_auth_metadata_context* to) { + grpc_auth_metadata_context_reset(to); + to->channel_auth_context = from->channel_auth_context; + if (to->channel_auth_context != nullptr) { + const_cast(to->channel_auth_context) + ->Ref(DEBUG_LOCATION, "grpc_auth_metadata_context_copy") + .release(); + } + to->service_url = + (from->service_url == nullptr) ? nullptr : strdup(from->service_url); + to->method_name = + (from->method_name == nullptr) ? nullptr : strdup(from->method_name); +} void grpc_auth_metadata_context_reset( grpc_auth_metadata_context* auth_md_context) { if (auth_md_context->service_url != nullptr) { @@ -160,6 +174,8 @@ static void on_credentials_metadata(void* arg, grpc_error* input_error) { if (error == GRPC_ERROR_NONE) { grpc_call_next_op(elem, batch); } else { + error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, + GRPC_STATUS_UNAVAILABLE); grpc_transport_stream_op_batch_finish_with_failure(batch, error, calld->call_combiner); } diff --git a/src/cpp/client/secure_credentials.cc b/src/cpp/client/secure_credentials.cc index 724a43a9709..2d041a28664 100644 --- a/src/cpp/client/secure_credentials.cc +++ b/src/cpp/client/secure_credentials.cc @@ -22,6 +22,7 @@ #include #include #include +#include "src/core/lib/security/transport/auth_filters.h" #include "src/cpp/client/create_channel_internal.h" #include "src/cpp/common/secure_auth_context.h" @@ -247,10 +248,13 @@ int MetadataCredentialsPluginWrapper::GetMetadata( return true; } if (w->plugin_->IsBlocking()) { + grpc_auth_metadata_context context_copy = grpc_auth_metadata_context(); + grpc_auth_metadata_context_copy(&context, &context_copy); // Asynchronous return. - w->thread_pool_->Add([w, context, cb, user_data] { + w->thread_pool_->Add([w, context_copy, cb, user_data]() mutable { w->MetadataCredentialsPluginWrapper::InvokePlugin( - context, cb, user_data, nullptr, nullptr, nullptr, nullptr); + context_copy, cb, user_data, nullptr, nullptr, nullptr, nullptr); + grpc_auth_metadata_context_reset(&context_copy); }); return 0; } else { diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index a505d1a633f..1d699e97b5d 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -1849,7 +1849,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginKeyFailure) { Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::INTERNAL); + EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); } TEST_P(SecureEnd2endTest, AuthMetadataPluginValueFailure) { @@ -1867,7 +1867,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginValueFailure) { Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::INTERNAL); + EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); } TEST_P(SecureEnd2endTest, AuthMetadataPluginWithDeadline) { @@ -1888,7 +1888,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginWithDeadline) { Status s = stub_->Echo(&context, request, &response); if (!s.ok()) { - EXPECT_EQ(StatusCode::DEADLINE_EXCEEDED, s.error_code()); + EXPECT_EQ(StatusCode::UNAVAILABLE, s.error_code()); } } @@ -1912,7 +1912,7 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginWithCancel) { }); Status s = stub_->Echo(&context, request, &response); if (!s.ok()) { - EXPECT_EQ(StatusCode::CANCELLED, s.error_code()); + EXPECT_EQ(StatusCode::UNAVAILABLE, s.error_code()); } cancel_thread.join(); } @@ -1933,7 +1933,7 @@ TEST_P(SecureEnd2endTest, NonBlockingAuthMetadataPluginFailure) { Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::NOT_FOUND); + EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); EXPECT_EQ(s.error_message(), grpc::string("Getting metadata from plugin failed with error: ") + kTestCredsPluginErrorMsg); @@ -1997,7 +1997,7 @@ TEST_P(SecureEnd2endTest, BlockingAuthMetadataPluginFailure) { Status s = stub_->Echo(&context, request, &response); EXPECT_FALSE(s.ok()); - EXPECT_EQ(s.error_code(), StatusCode::NOT_FOUND); + EXPECT_EQ(s.error_code(), StatusCode::UNAVAILABLE); EXPECT_EQ(s.error_message(), grpc::string("Getting metadata from plugin failed with error: ") + kTestCredsPluginErrorMsg); From 9d3288e4082f9be89d75a4d221161443a5feccb8 Mon Sep 17 00:00:00 2001 From: yang-g Date: Wed, 29 May 2019 09:22:17 -0700 Subject: [PATCH 3/5] Fix test bugs --- test/cpp/end2end/end2end_test.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 1d699e97b5d..983714c044a 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1888,7 +1889,8 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginWithDeadline) { Status s = stub_->Echo(&context, request, &response); if (!s.ok()) { - EXPECT_EQ(StatusCode::UNAVAILABLE, s.error_code()); + EXPECT_TRUE(s.error_code() == StatusCode::DEADLINE_EXCEEDED || + s.error_code() == StatusCode::UNAVAILABLE); } } @@ -1905,14 +1907,15 @@ TEST_P(SecureEnd2endTest, AuthMetadataPluginWithCancel) { true, delay)))); request.set_message("Hello"); - std::thread cancel_thread([&context] { + std::thread cancel_thread([&] { gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), gpr_time_from_millis(delay, GPR_TIMESPAN))); context.TryCancel(); }); Status s = stub_->Echo(&context, request, &response); if (!s.ok()) { - EXPECT_EQ(StatusCode::UNAVAILABLE, s.error_code()); + EXPECT_TRUE(s.error_code() == StatusCode::CANCELLED || + s.error_code() == StatusCode::UNAVAILABLE); } cancel_thread.join(); } From bd97b1361df0a2d7ac0bbd02a2a7e618fab299cc Mon Sep 17 00:00:00 2001 From: yang-g Date: Wed, 29 May 2019 12:04:40 -0700 Subject: [PATCH 4/5] Delete wrapper in executor thread to avoid self-joining deadlock --- src/cpp/client/secure_credentials.cc | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/cpp/client/secure_credentials.cc b/src/cpp/client/secure_credentials.cc index 2d041a28664..3d2a123180b 100644 --- a/src/cpp/client/secure_credentials.cc +++ b/src/cpp/client/secure_credentials.cc @@ -22,6 +22,7 @@ #include #include #include +#include "src/core/lib/iomgr/executor.h" #include "src/core/lib/security/transport/auth_filters.h" #include "src/cpp/client/create_channel_internal.h" #include "src/cpp/common/secure_auth_context.h" @@ -224,13 +225,23 @@ std::shared_ptr MetadataCredentialsFromPlugin( } // namespace grpc_impl namespace grpc { - -void MetadataCredentialsPluginWrapper::Destroy(void* wrapper) { - if (wrapper == nullptr) return; +namespace { +void DeleteWrapper(void* wrapper, grpc_error* ignored) { MetadataCredentialsPluginWrapper* w = static_cast(wrapper); delete w; } +} // namespace + +void MetadataCredentialsPluginWrapper::Destroy(void* wrapper) { + if (wrapper == nullptr) return; + grpc_core::ApplicationCallbackExecCtx callback_exec_ctx; + grpc_core::ExecCtx exec_ctx; + GRPC_CLOSURE_RUN(GRPC_CLOSURE_CREATE(DeleteWrapper, wrapper, + grpc_core::Executor::Scheduler( + grpc_core::ExecutorJobType::SHORT)), + GRPC_ERROR_NONE); +} int MetadataCredentialsPluginWrapper::GetMetadata( void* wrapper, grpc_auth_metadata_context context, From a874fd8bbbcb3bbc729cc98857e5417a9b523ed1 Mon Sep 17 00:00:00 2001 From: yang-g Date: Thu, 30 May 2019 15:20:10 -0700 Subject: [PATCH 5/5] Resolve review comments --- src/core/lib/security/transport/client_auth_filter.cc | 7 +++---- src/cpp/client/secure_credentials.cc | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index dbe288237d4..1062fc2f4fa 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -121,11 +121,10 @@ void grpc_auth_metadata_context_copy(grpc_auth_metadata_context* from, ->Ref(DEBUG_LOCATION, "grpc_auth_metadata_context_copy") .release(); } - to->service_url = - (from->service_url == nullptr) ? nullptr : strdup(from->service_url); - to->method_name = - (from->method_name == nullptr) ? nullptr : strdup(from->method_name); + to->service_url = gpr_strdup(from->service_url); + to->method_name = gpr_strdup(from->method_name); } + void grpc_auth_metadata_context_reset( grpc_auth_metadata_context* auth_md_context) { if (auth_md_context->service_url != nullptr) { diff --git a/src/cpp/client/secure_credentials.cc b/src/cpp/client/secure_credentials.cc index 3d2a123180b..197112d4bb7 100644 --- a/src/cpp/client/secure_credentials.cc +++ b/src/cpp/client/secure_credentials.cc @@ -259,6 +259,8 @@ int MetadataCredentialsPluginWrapper::GetMetadata( return true; } if (w->plugin_->IsBlocking()) { + // The internals of context may be destroyed if GetMetadata is cancelled. + // Make a copy for InvokePlugin. grpc_auth_metadata_context context_copy = grpc_auth_metadata_context(); grpc_auth_metadata_context_copy(&context, &context_copy); // Asynchronous return.