From 47dbf1dd263e0bf7aba7dbd9651b2bcb570d6889 Mon Sep 17 00:00:00 2001 From: yang-g Date: Tue, 28 May 2019 23:47:29 -0700 Subject: [PATCH] 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);