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);