kudu git commit: KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL
Repository: kudu Updated Branches: refs/heads/branch-1.7.x cd28c7553 -> b24f166e3 KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL See the associated JIRA for bug details. The fix follows a slightly different approach than suggested by Todd, namely initialization of the RPC tracker in the RPC context is delayed until after authorization. This allows the error path code to remain unchanged, and allows the one caller of the RpcContext constructor to be slightly cleaned up. Change-Id: I9717d36e438bbe68172fa2619c9829ad5fdc779d Reviewed-on: http://gerrit.cloudera.org:8080/11216 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins Reviewed-on: http://gerrit.cloudera.org:8080/11219 Reviewed-by: Adar Dembo Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/b24f166e Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b24f166e Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b24f166e Branch: refs/heads/branch-1.7.x Commit: b24f166e356d7f34f6cff2efa0dcb3ff3ebe6e0b Parents: cd28c75 Author: Dan Burkert Authored: Tue Aug 14 12:18:32 2018 -0700 Committer: Dan Burkert Committed: Tue Aug 14 22:55:34 2018 + -- src/kudu/rpc/rpc_context.cc | 11 +++ src/kudu/rpc/rpc_context.h| 10 -- src/kudu/rpc/rpc_stub-test.cc | 36 +--- src/kudu/rpc/service_if.cc| 12 +++- 4 files changed, 47 insertions(+), 22 deletions(-) -- http://git-wip-us.apache.org/repos/asf/kudu/blob/b24f166e/src/kudu/rpc/rpc_context.cc -- diff --git a/src/kudu/rpc/rpc_context.cc b/src/kudu/rpc/rpc_context.cc index 123d21f..fe6fb38 100644 --- a/src/kudu/rpc/rpc_context.cc +++ b/src/kudu/rpc/rpc_context.cc @@ -48,12 +48,10 @@ namespace rpc { RpcContext::RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - const scoped_refptr& result_tracker) + google::protobuf::Message *response_pb) : call_(CHECK_NOTNULL(call)), request_pb_(request_pb), -response_pb_(response_pb), -result_tracker_(result_tracker) { +response_pb_(response_pb) { VLOG(4) << call_->remote_method().service_name() << ": Received RPC request for " << call_->ToString() << ":" << std::endl << SecureDebugString(*request_pb_); TRACE_EVENT_ASYNC_BEGIN2("rpc_call", "RPC", this, @@ -64,6 +62,11 @@ RpcContext::RpcContext(InboundCall *call, RpcContext::~RpcContext() { } +void RpcContext::SetResultTracker(scoped_refptr result_tracker) { + DCHECK(!result_tracker_); + result_tracker_ = std::move(result_tracker); +} + void RpcContext::RespondSuccess() { if (AreResultsTracked()) { result_tracker_->RecordCompletionAndRespond(call_->header().request_id(), http://git-wip-us.apache.org/repos/asf/kudu/blob/b24f166e/src/kudu/rpc/rpc_context.h -- diff --git a/src/kudu/rpc/rpc_context.h b/src/kudu/rpc/rpc_context.h index a34c1a1..e27 100644 --- a/src/kudu/rpc/rpc_context.h +++ b/src/kudu/rpc/rpc_context.h @@ -69,11 +69,17 @@ class RpcContext { // and is not a public API. RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - const scoped_refptr& result_tracker); + google::protobuf::Message *response_pb); ~RpcContext(); + // Initialize a result tracker for the RPC. + // + // This is delayed until after the constructor in order to allow for RPCs to + // be validated and used prior to initializing the tracking (primarily for + // authorization). + void SetResultTracker(scoped_refptr result_tracker); + // Return the trace buffer for this call. Trace* trace(); http://git-wip-us.apache.org/repos/asf/kudu/blob/b24f166e/src/kudu/rpc/rpc_stub-test.cc -- diff --git a/src/kudu/rpc/rpc_stub-test.cc b/src/kudu/rpc/rpc_stub-test.cc index d5b85d1..7e46dc9 100644 --- a/src/kudu/rpc/rpc_stub-test.cc +++ b/src/kudu/rpc/rpc_stub-test.cc @@ -208,13 +208,35 @@ TEST_F(RpcStubTest, TestAuthorization) { p.set_user_credentials(creds); // Alice is disallowed by all RPCs. -RpcController controller; -WhoAmIRequestPB req; -WhoAmIResponsePB resp; -Status s = p.WhoAmI(req, , ); -ASSERT_FALSE(s.ok()); -ASSERT_EQ(s.ToString(), - "Remote error: Not authorized: alice is not allowed to call this method"); +{ + RpcController controller; + WhoAmIRequestPB req; + WhoAmIResponsePB resp; + Status s = p.WhoAmI(req, , ); +
kudu git commit: KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL
Repository: kudu Updated Branches: refs/heads/branch-1.6.x 6dfc10ac0 -> c50b7fe69 KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL See the associated JIRA for bug details. The fix follows a slightly different approach than suggested by Todd, namely initialization of the RPC tracker in the RPC context is delayed until after authorization. This allows the error path code to remain unchanged, and allows the one caller of the RpcContext constructor to be slightly cleaned up. Change-Id: I9717d36e438bbe68172fa2619c9829ad5fdc779d Reviewed-on: http://gerrit.cloudera.org:8080/11216 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins Reviewed-on: http://gerrit.cloudera.org:8080/11220 Reviewed-by: Adar Dembo Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c50b7fe6 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c50b7fe6 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c50b7fe6 Branch: refs/heads/branch-1.6.x Commit: c50b7fe6915febcb2af480e67a1ca43a708142c0 Parents: 6dfc10a Author: Dan Burkert Authored: Tue Aug 14 12:18:32 2018 -0700 Committer: Dan Burkert Committed: Tue Aug 14 22:55:37 2018 + -- src/kudu/rpc/rpc_context.cc | 11 +++ src/kudu/rpc/rpc_context.h| 10 -- src/kudu/rpc/rpc_stub-test.cc | 36 +--- src/kudu/rpc/service_if.cc| 12 +++- 4 files changed, 47 insertions(+), 22 deletions(-) -- http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/src/kudu/rpc/rpc_context.cc -- diff --git a/src/kudu/rpc/rpc_context.cc b/src/kudu/rpc/rpc_context.cc index 9c3d154..27011b5 100644 --- a/src/kudu/rpc/rpc_context.cc +++ b/src/kudu/rpc/rpc_context.cc @@ -47,12 +47,10 @@ namespace rpc { RpcContext::RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - const scoped_refptr& result_tracker) + google::protobuf::Message *response_pb) : call_(CHECK_NOTNULL(call)), request_pb_(request_pb), -response_pb_(response_pb), -result_tracker_(result_tracker) { +response_pb_(response_pb) { VLOG(4) << call_->remote_method().service_name() << ": Received RPC request for " << call_->ToString() << ":" << std::endl << SecureDebugString(*request_pb_); TRACE_EVENT_ASYNC_BEGIN2("rpc_call", "RPC", this, @@ -63,6 +61,11 @@ RpcContext::RpcContext(InboundCall *call, RpcContext::~RpcContext() { } +void RpcContext::SetResultTracker(scoped_refptr result_tracker) { + DCHECK(!result_tracker_); + result_tracker_ = std::move(result_tracker); +} + void RpcContext::RespondSuccess() { if (AreResultsTracked()) { result_tracker_->RecordCompletionAndRespond(call_->header().request_id(), http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/src/kudu/rpc/rpc_context.h -- diff --git a/src/kudu/rpc/rpc_context.h b/src/kudu/rpc/rpc_context.h index 42f096e..468e6d1 100644 --- a/src/kudu/rpc/rpc_context.h +++ b/src/kudu/rpc/rpc_context.h @@ -68,11 +68,17 @@ class RpcContext { // and is not a public API. RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - const scoped_refptr& result_tracker); + google::protobuf::Message *response_pb); ~RpcContext(); + // Initialize a result tracker for the RPC. + // + // This is delayed until after the constructor in order to allow for RPCs to + // be validated and used prior to initializing the tracking (primarily for + // authorization). + void SetResultTracker(scoped_refptr result_tracker); + // Return the trace buffer for this call. Trace* trace(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/src/kudu/rpc/rpc_stub-test.cc -- diff --git a/src/kudu/rpc/rpc_stub-test.cc b/src/kudu/rpc/rpc_stub-test.cc index 5fe2885..cb16c02 100644 --- a/src/kudu/rpc/rpc_stub-test.cc +++ b/src/kudu/rpc/rpc_stub-test.cc @@ -209,13 +209,35 @@ TEST_F(RpcStubTest, TestAuthorization) { p.set_user_credentials(creds); // Alice is disallowed by all RPCs. -RpcController controller; -WhoAmIRequestPB req; -WhoAmIResponsePB resp; -Status s = p.WhoAmI(req, , ); -ASSERT_FALSE(s.ok()); -ASSERT_EQ(s.ToString(), - "Remote error: Not authorized: alice is not allowed to call this method"); +{ + RpcController controller; + WhoAmIRequestPB req; + WhoAmIResponsePB resp; + Status s = p.WhoAmI(req, , ); +
kudu git commit: KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL
Repository: kudu Updated Branches: refs/heads/master 2d92370d9 -> e795d5a82 KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL See the associated JIRA for bug details. The fix follows a slightly different approach than suggested by Todd, namely initialization of the RPC tracker in the RPC context is delayed until after authorization. This allows the error path code to remain unchanged, and allows the one caller of the RpcContext constructor to be slightly cleaned up. Change-Id: I9717d36e438bbe68172fa2619c9829ad5fdc779d Reviewed-on: http://gerrit.cloudera.org:8080/11216 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e795d5a8 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e795d5a8 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e795d5a8 Branch: refs/heads/master Commit: e795d5a82c0c27c8a2ba1f82ac6695402aa42885 Parents: 2d92370 Author: Dan Burkert Authored: Tue Aug 14 12:18:32 2018 -0700 Committer: Dan Burkert Committed: Tue Aug 14 21:20:30 2018 + -- src/kudu/rpc/rpc_context.cc | 11 +++ src/kudu/rpc/rpc_context.h| 10 -- src/kudu/rpc/rpc_stub-test.cc | 36 +--- src/kudu/rpc/service_if.cc| 12 +++- 4 files changed, 47 insertions(+), 22 deletions(-) -- http://git-wip-us.apache.org/repos/asf/kudu/blob/e795d5a8/src/kudu/rpc/rpc_context.cc -- diff --git a/src/kudu/rpc/rpc_context.cc b/src/kudu/rpc/rpc_context.cc index b3072ca..9f95358 100644 --- a/src/kudu/rpc/rpc_context.cc +++ b/src/kudu/rpc/rpc_context.cc @@ -48,12 +48,10 @@ namespace rpc { RpcContext::RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - scoped_refptr result_tracker) + google::protobuf::Message *response_pb) : call_(CHECK_NOTNULL(call)), request_pb_(request_pb), -response_pb_(response_pb), -result_tracker_(std::move(result_tracker)) { +response_pb_(response_pb) { VLOG(4) << call_->remote_method().service_name() << ": Received RPC request for " << call_->ToString() << ":" << std::endl << SecureDebugString(*request_pb_); TRACE_EVENT_ASYNC_BEGIN2("rpc_call", "RPC", this, @@ -64,6 +62,11 @@ RpcContext::RpcContext(InboundCall *call, RpcContext::~RpcContext() { } +void RpcContext::SetResultTracker(scoped_refptr result_tracker) { + DCHECK(!result_tracker_); + result_tracker_ = std::move(result_tracker); +} + void RpcContext::RespondSuccess() { if (AreResultsTracked()) { result_tracker_->RecordCompletionAndRespond(call_->header().request_id(), http://git-wip-us.apache.org/repos/asf/kudu/blob/e795d5a8/src/kudu/rpc/rpc_context.h -- diff --git a/src/kudu/rpc/rpc_context.h b/src/kudu/rpc/rpc_context.h index 1b12023..38ce703 100644 --- a/src/kudu/rpc/rpc_context.h +++ b/src/kudu/rpc/rpc_context.h @@ -69,11 +69,17 @@ class RpcContext { // and is not a public API. RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - scoped_refptr result_tracker); + google::protobuf::Message *response_pb); ~RpcContext(); + // Initialize a result tracker for the RPC. + // + // This is delayed until after the constructor in order to allow for RPCs to + // be validated and used prior to initializing the tracking (primarily for + // authorization). + void SetResultTracker(scoped_refptr result_tracker); + // Return the trace buffer for this call. Trace* trace(); http://git-wip-us.apache.org/repos/asf/kudu/blob/e795d5a8/src/kudu/rpc/rpc_stub-test.cc -- diff --git a/src/kudu/rpc/rpc_stub-test.cc b/src/kudu/rpc/rpc_stub-test.cc index e626276..dc92f2e 100644 --- a/src/kudu/rpc/rpc_stub-test.cc +++ b/src/kudu/rpc/rpc_stub-test.cc @@ -209,13 +209,35 @@ TEST_F(RpcStubTest, TestAuthorization) { p.set_user_credentials(creds); // Alice is disallowed by all RPCs. -RpcController controller; -WhoAmIRequestPB req; -WhoAmIResponsePB resp; -Status s = p.WhoAmI(req, , ); -ASSERT_FALSE(s.ok()); -ASSERT_EQ(s.ToString(), - "Remote error: Not authorized: alice is not allowed to call this method"); +{ + RpcController controller; + WhoAmIRequestPB req; + WhoAmIResponsePB resp; + Status s = p.WhoAmI(req, , ); + ASSERT_FALSE(s.ok()); + ASSERT_EQ(s.ToString(), +"Remote error: Not