From 5d7d6c0fbdcac75ea482e1fde3e128cd0c1646c1 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 14 Nov 2018 17:35:26 -0800 Subject: Add method to fail hijacked send messages --- .../end2end/client_interceptors_end2end_test.cc | 73 ++++++++++++++++++++++ 1 file changed, 73 insertions(+) (limited to 'test') diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 0b34ec93ae..81efd15452 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -269,6 +269,49 @@ class HijackingInterceptorMakesAnotherCallFactory } }; +class ClientStreamingRpcHijackingInterceptor + : public experimental::Interceptor { + public: + ClientStreamingRpcHijackingInterceptor(experimental::ClientRpcInfo* info) { + info_ = info; + } + virtual void Intercept(experimental::InterceptorBatchMethods* methods) { + bool hijack = false; + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { + hijack = true; + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)) { + if (++count_ > 10) { + methods->FailHijackedSendMessage(); + } + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_RECV_STATUS)) { + auto* status = methods->GetRecvStatus(); + *status = Status(StatusCode::UNAVAILABLE, "Done sending 10 messages"); + } + if (hijack) { + methods->Hijack(); + } else { + methods->Proceed(); + } + } + + private: + experimental::ClientRpcInfo* info_; + int count_ = 0; +}; +class ClientStreamingRpcHijackingInterceptorFactory + : public experimental::ClientInterceptorFactoryInterface { + public: + virtual experimental::Interceptor* CreateClientInterceptor( + experimental::ClientRpcInfo* info) override { + return new ClientStreamingRpcHijackingInterceptor(info); + } +}; + class LoggingInterceptor : public experimental::Interceptor { public: LoggingInterceptor(experimental::ClientRpcInfo* info) { info_ = info; } @@ -535,6 +578,36 @@ TEST_F(ClientInterceptorsStreamingEnd2endTest, ServerStreamingTest) { EXPECT_EQ(DummyInterceptor::GetNumTimesRun(), 20); } +TEST_F(ClientInterceptorsStreamingEnd2endTest, ClientStreamingHijackingTest) { + ChannelArguments args; + auto creators = std::unique_ptr>>( + new std::vector< + std::unique_ptr>()); + creators->push_back( + std::unique_ptr( + new ClientStreamingRpcHijackingInterceptorFactory())); + auto channel = experimental::CreateCustomChannelWithInterceptors( + server_address_, InsecureChannelCredentials(), args, std::move(creators)); + + auto stub = grpc::testing::EchoTestService::NewStub(channel); + ClientContext ctx; + EchoRequest req; + EchoResponse resp; + req.mutable_param()->set_echo_metadata(true); + req.set_message("Hello"); + string expected_resp = ""; + auto writer = stub->RequestStream(&ctx, &resp); + for (int i = 0; i < 10; i++) { + EXPECT_TRUE(writer->Write(req)); + expected_resp += "Hello"; + } + // Expect that the interceptor will reject the 11th message + EXPECT_FALSE(writer->Write(req)); + Status s = writer->Finish(); + EXPECT_EQ(s.ok(), false); +} + TEST_F(ClientInterceptorsStreamingEnd2endTest, BidiStreamingTest) { ChannelArguments args; DummyInterceptor::Reset(); -- cgit v1.2.3 From d4ebd30eb2eb94f77ac9b52c44880e3d70c6aef0 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 15 Nov 2018 15:57:43 -0800 Subject: Add method to get status of send message op on POST_SEND_MESSAGE --- include/grpcpp/impl/codegen/call_op_set.h | 18 ++++++++++++++++-- include/grpcpp/impl/codegen/interceptor.h | 6 +++++- include/grpcpp/impl/codegen/interceptor_common.h | 10 ++++++++++ test/cpp/end2end/client_interceptors_end2end_test.cc | 18 ++++++++++++++++-- 4 files changed, 47 insertions(+), 5 deletions(-) (limited to 'test') diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index 1f2b88e9e1..f330679ffc 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -315,9 +315,16 @@ class CallOpSendMessage { write_options_.Clear(); } void FinishOp(bool* status) { - send_buf_.Clear(); + if (!send_buf_.Valid()) { + return; + } if (hijacked_ && failed_send_) { + // Hijacking interceptor failed this Op *status = false; + } else if (!*status) { + // This Op was passed down to core and the Op failed + gpr_log(GPR_ERROR, "failure status"); + failed_send_ = true; } } @@ -330,7 +337,14 @@ class CallOpSendMessage { } void SetFinishInterceptionHookPoint( - InterceptorBatchMethodsImpl* interceptor_methods) {} + InterceptorBatchMethodsImpl* interceptor_methods) { + if (send_buf_.Valid()) { + interceptor_methods->AddInterceptionHookPoint( + experimental::InterceptionHookPoints::POST_SEND_MESSAGE); + // We had already registered failed_send_ earlier. No need to do it again. + } + send_buf_.Clear(); + } void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; diff --git a/include/grpcpp/impl/codegen/interceptor.h b/include/grpcpp/impl/codegen/interceptor.h index 47239332c8..154172dd81 100644 --- a/include/grpcpp/impl/codegen/interceptor.h +++ b/include/grpcpp/impl/codegen/interceptor.h @@ -41,9 +41,10 @@ class InterceptedMessage { }; enum class InterceptionHookPoints { - /* The first two in this list are for clients and servers */ + /* The first three in this list are for clients and servers */ PRE_SEND_INITIAL_METADATA, PRE_SEND_MESSAGE, + POST_SEND_MESSAGE, PRE_SEND_STATUS /* server only */, PRE_SEND_CLOSE /* client only */, /* The following three are for hijacked clients only and can only be @@ -85,6 +86,9 @@ class InterceptorBatchMethods { // sent virtual ByteBuffer* GetSendMessage() = 0; + // Checks whether the SEND MESSAGE op succeeded + virtual bool GetSendMessageStatus() = 0; + // Returns a modifiable multimap of the initial metadata to be sent virtual std::multimap* GetSendInitialMetadata() = 0; diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index 601a929afe..21326df73b 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -81,6 +81,8 @@ class InterceptorBatchMethodsImpl ByteBuffer* GetSendMessage() override { return send_message_; } + bool GetSendMessageStatus() override { return !*fail_send_message_; } + std::multimap* GetSendInitialMetadata() override { return send_initial_metadata_; } @@ -113,6 +115,7 @@ class InterceptorBatchMethodsImpl void FailHijackedSendMessage() override { GPR_CODEGEN_ASSERT(hooks_[static_cast( experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)]); + gpr_log(GPR_ERROR, "failing"); *fail_send_message_ = true; } @@ -396,6 +399,13 @@ class CancelInterceptorBatchMethods return nullptr; } + bool GetSendMessageStatus() override { + GPR_CODEGEN_ASSERT( + false && + "It is illegal to call GetSendMessageStatus on a method which " + "has a Cancel notification"); + } + std::multimap* GetSendInitialMetadata() override { GPR_CODEGEN_ASSERT(false && "It is illegal to call GetSendInitialMetadata on a " diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 81efd15452..97947e7393 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -287,6 +287,13 @@ class ClientStreamingRpcHijackingInterceptor methods->FailHijackedSendMessage(); } } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::POST_SEND_MESSAGE)) { + EXPECT_FALSE(got_failed_send_); + gpr_log(GPR_ERROR, "%d", got_failed_send_); + got_failed_send_ = !methods->GetSendMessageStatus(); + gpr_log(GPR_ERROR, "%d", got_failed_send_); + } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_RECV_STATUS)) { auto* status = methods->GetRecvStatus(); @@ -299,10 +306,16 @@ class ClientStreamingRpcHijackingInterceptor } } + static bool GotFailedSend() { return got_failed_send_; } + private: experimental::ClientRpcInfo* info_; int count_ = 0; + static bool got_failed_send_; }; + +bool ClientStreamingRpcHijackingInterceptor::got_failed_send_ = false; + class ClientStreamingRpcHijackingInterceptorFactory : public experimental::ClientInterceptorFactoryInterface { public: @@ -602,10 +615,11 @@ TEST_F(ClientInterceptorsStreamingEnd2endTest, ClientStreamingHijackingTest) { EXPECT_TRUE(writer->Write(req)); expected_resp += "Hello"; } - // Expect that the interceptor will reject the 11th message - EXPECT_FALSE(writer->Write(req)); + // The interceptor will reject the 11th message + writer->Write(req); Status s = writer->Finish(); EXPECT_EQ(s.ok(), false); + EXPECT_TRUE(ClientStreamingRpcHijackingInterceptor::GotFailedSend()); } TEST_F(ClientInterceptorsStreamingEnd2endTest, BidiStreamingTest) { -- cgit v1.2.3 From 00c9c40004d011f01c72d253a530edb3364992bf Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 15 Nov 2018 16:00:33 -0800 Subject: Remove extraneous logging statements --- include/grpcpp/impl/codegen/call_op_set.h | 1 - include/grpcpp/impl/codegen/interceptor_common.h | 1 - test/cpp/end2end/client_interceptors_end2end_test.cc | 2 -- 3 files changed, 4 deletions(-) (limited to 'test') diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index f330679ffc..ac3ba17bd9 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -323,7 +323,6 @@ class CallOpSendMessage { *status = false; } else if (!*status) { // This Op was passed down to core and the Op failed - gpr_log(GPR_ERROR, "failure status"); failed_send_ = true; } } diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index 21326df73b..321691236b 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -115,7 +115,6 @@ class InterceptorBatchMethodsImpl void FailHijackedSendMessage() override { GPR_CODEGEN_ASSERT(hooks_[static_cast( experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)]); - gpr_log(GPR_ERROR, "failing"); *fail_send_message_ = true; } diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 97947e7393..3708c11235 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -290,9 +290,7 @@ class ClientStreamingRpcHijackingInterceptor if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::POST_SEND_MESSAGE)) { EXPECT_FALSE(got_failed_send_); - gpr_log(GPR_ERROR, "%d", got_failed_send_); got_failed_send_ = !methods->GetSendMessageStatus(); - gpr_log(GPR_ERROR, "%d", got_failed_send_); } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_RECV_STATUS)) { -- cgit v1.2.3 From 31a775b425eac37bb43c301cfb25e1f6a4bde106 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 18 Dec 2018 12:52:14 -0800 Subject: Add missing argument --- include/grpcpp/impl/codegen/call_op_set.h | 3 +-- include/grpcpp/impl/codegen/interceptor_common.h | 1 + test/cpp/end2end/client_interceptors_end2end_test.cc | 8 +++----- 3 files changed, 5 insertions(+), 7 deletions(-) (limited to 'test') diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index 3db9f48bff..1c0ccbab52 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -340,12 +340,11 @@ class CallOpSendMessage { if (send_buf_.Valid()) { interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::POST_SEND_MESSAGE); - // We had already registered failed_send_ earlier. No need to do it again. } send_buf_.Clear(); // The contents of the SendMessage value that was previously set // has had its references stolen by core's operations - interceptor_methods->SetSendMessage(nullptr); + interceptor_methods->SetSendMessage(nullptr, &failed_send_); } void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index 321691236b..b01706af8d 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -403,6 +403,7 @@ class CancelInterceptorBatchMethods false && "It is illegal to call GetSendMessageStatus on a method which " "has a Cancel notification"); + return false; } std::multimap* GetSendInitialMetadata() override { diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 33773e3b3b..c55eaab4d6 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -580,11 +580,9 @@ TEST_F(ClientInterceptorsStreamingEnd2endTest, ServerStreamingTest) { TEST_F(ClientInterceptorsStreamingEnd2endTest, ClientStreamingHijackingTest) { ChannelArguments args; - auto creators = std::unique_ptr>>( - new std::vector< - std::unique_ptr>()); - creators->push_back( + std::vector> + creators; + creators.push_back( std::unique_ptr( new ClientStreamingRpcHijackingInterceptorFactory())); auto channel = experimental::CreateCustomChannelWithInterceptors( -- cgit v1.2.3 From aecc5f7285faedec634c99aff0b48eea86d3861a Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 28 Dec 2018 16:03:20 -0800 Subject: Add client interceptor test for bidi streaming hijacking interceptor --- .../end2end/client_interceptors_end2end_test.cc | 91 ++++++++++++++++++++++ test/cpp/end2end/interceptors_util.cc | 10 +++ test/cpp/end2end/interceptors_util.h | 3 + 3 files changed, 104 insertions(+) (limited to 'test') diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 8abf4eb3f4..ab387aa914 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -270,6 +270,84 @@ class HijackingInterceptorMakesAnotherCallFactory } }; +class BidiStreamingRpcHijackingInterceptor : public experimental::Interceptor { + public: + BidiStreamingRpcHijackingInterceptor(experimental::ClientRpcInfo* info) { + info_ = info; + } + + virtual void Intercept(experimental::InterceptorBatchMethods* methods) { + bool hijack = false; + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { + CheckMetadata(*methods->GetSendInitialMetadata(), "testkey", "testvalue"); + hijack = true; + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)) { + EchoRequest req; + auto* buffer = methods->GetSendMessage(); + auto copied_buffer = *buffer; + EXPECT_TRUE( + SerializationTraits::Deserialize(&copied_buffer, &req) + .ok()); + EXPECT_EQ(req.message().find("Hello"), 0); + msg = req.message(); + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_CLOSE)) { + // Got nothing to do here for now + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::POST_RECV_STATUS)) { + CheckMetadata(*methods->GetRecvTrailingMetadata(), "testkey", + "testvalue"); + auto* status = methods->GetRecvStatus(); + EXPECT_EQ(status->ok(), true); + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_RECV_MESSAGE)) { + EchoResponse* resp = + static_cast(methods->GetRecvMessage()); + resp->set_message(msg); + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::POST_RECV_MESSAGE)) { + EXPECT_EQ(static_cast(methods->GetRecvMessage()) + ->message() + .find("Hello"), + 0); + } + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_RECV_STATUS)) { + auto* map = methods->GetRecvTrailingMetadata(); + // insert the metadata that we want + EXPECT_EQ(map->size(), static_cast(0)); + map->insert(std::make_pair("testkey", "testvalue")); + auto* status = methods->GetRecvStatus(); + *status = Status(StatusCode::OK, ""); + } + if (hijack) { + methods->Hijack(); + } else { + methods->Proceed(); + } + } + + private: + experimental::ClientRpcInfo* info_; + grpc::string msg; +}; + +class BidiStreamingRpcHijackingInterceptorFactory + : public experimental::ClientInterceptorFactoryInterface { + public: + virtual experimental::Interceptor* CreateClientInterceptor( + experimental::ClientRpcInfo* info) override { + return new BidiStreamingRpcHijackingInterceptor(info); + } +}; + class LoggingInterceptor : public experimental::Interceptor { public: LoggingInterceptor(experimental::ClientRpcInfo* info) { info_ = info; } @@ -546,6 +624,19 @@ TEST_F(ClientInterceptorsStreamingEnd2endTest, ServerStreamingTest) { EXPECT_EQ(DummyInterceptor::GetNumTimesRun(), 20); } +TEST_F(ClientInterceptorsStreamingEnd2endTest, BidiStreamingHijackingTest) { + ChannelArguments args; + DummyInterceptor::Reset(); + std::vector> + creators; + creators.push_back( + std::unique_ptr( + new BidiStreamingRpcHijackingInterceptorFactory())); + auto channel = experimental::CreateCustomChannelWithInterceptors( + server_address_, InsecureChannelCredentials(), args, std::move(creators)); + MakeBidiStreamingCall(channel); +} + TEST_F(ClientInterceptorsStreamingEnd2endTest, BidiStreamingTest) { ChannelArguments args; DummyInterceptor::Reset(); diff --git a/test/cpp/end2end/interceptors_util.cc b/test/cpp/end2end/interceptors_util.cc index e0ad7d1526..900f02b5f3 100644 --- a/test/cpp/end2end/interceptors_util.cc +++ b/test/cpp/end2end/interceptors_util.cc @@ -132,6 +132,16 @@ bool CheckMetadata(const std::multimap& map, return false; } +bool CheckMetadata(const std::multimap& map, + const string& key, const string& value) { + for (const auto& pair : map) { + if (pair.first == key && pair.second == value) { + return true; + } + } + return false; +} + std::vector> CreateDummyClientInterceptors() { std::vector> diff --git a/test/cpp/end2end/interceptors_util.h b/test/cpp/end2end/interceptors_util.h index 659e613d2e..419845e5f6 100644 --- a/test/cpp/end2end/interceptors_util.h +++ b/test/cpp/end2end/interceptors_util.h @@ -165,6 +165,9 @@ void MakeCallbackCall(const std::shared_ptr& channel); bool CheckMetadata(const std::multimap& map, const string& key, const string& value); +bool CheckMetadata(const std::multimap& map, + const string& key, const string& value); + std::vector> CreateDummyClientInterceptors(); -- cgit v1.2.3 From a5ed3d245e448c1e0e0e28b93e5821aaa7a3e439 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 4 Jan 2019 11:17:35 -0800 Subject: Avoid unsigned signed comparison issues --- test/cpp/end2end/client_interceptors_end2end_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'test') diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index ab387aa914..fc75fdb290 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -291,7 +291,7 @@ class BidiStreamingRpcHijackingInterceptor : public experimental::Interceptor { EXPECT_TRUE( SerializationTraits::Deserialize(&copied_buffer, &req) .ok()); - EXPECT_EQ(req.message().find("Hello"), 0); + EXPECT_EQ(req.message().find("Hello"), 0u); msg = req.message(); } if (methods->QueryInterceptionHookPoint( @@ -316,7 +316,7 @@ class BidiStreamingRpcHijackingInterceptor : public experimental::Interceptor { EXPECT_EQ(static_cast(methods->GetRecvMessage()) ->message() .find("Hello"), - 0); + 0u); } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_RECV_STATUS)) { @@ -370,7 +370,7 @@ class LoggingInterceptor : public experimental::Interceptor { EXPECT_TRUE( SerializationTraits::Deserialize(&copied_buffer, &req) .ok()); - EXPECT_TRUE(req.message().find("Hello") == 0); + EXPECT_TRUE(req.message().find("Hello") == 0u); } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_CLOSE)) { @@ -386,7 +386,7 @@ class LoggingInterceptor : public experimental::Interceptor { experimental::InterceptionHookPoints::POST_RECV_MESSAGE)) { EchoResponse* resp = static_cast(methods->GetRecvMessage()); - EXPECT_TRUE(resp->message().find("Hello") == 0); + EXPECT_TRUE(resp->message().find("Hello") == 0u); } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::POST_RECV_STATUS)) { -- cgit v1.2.3