kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL
Date Tue, 14 Aug 2018 22:57:28 GMT
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 <aserbin@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-on: http://gerrit.cloudera.org:8080/11219
Reviewed-by: Adar Dembo <adar@cloudera.com>


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 <danburkert@apache.org>
Authored: Tue Aug 14 12:18:32 2018 -0700
Committer: Dan Burkert <danburkert@apache.org>
Committed: Tue Aug 14 22:55:34 2018 +0000

----------------------------------------------------------------------
 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<ResultTracker>& 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<ResultTracker> 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..e255557 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<ResultTracker>& 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<ResultTracker> 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, &resp, &controller);
-    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, &resp, &controller);
+      ASSERT_FALSE(s.ok());
+      ASSERT_EQ(s.ToString(),
+                "Remote error: Not authorized: alice is not allowed to call this method");
+    }
+
+    // KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL
+    {
+      RpcController controller;
+
+      unique_ptr<RequestIdPB> request_id(new RequestIdPB);
+      request_id->set_client_id("client-id");
+      request_id->set_attempt_no(0);
+      request_id->set_seq_no(0);
+      request_id->set_first_incomplete_seq_no(-1);
+      controller.SetRequestIdPB(std::move(request_id));
+
+      ExactlyOnceRequestPB req;
+      req.set_value_to_add(1);
+      ExactlyOnceResponsePB resp;
+      Status s = p.AddExactlyOnce(req, &resp, &controller);
+      ASSERT_FALSE(s.ok());
+      ASSERT_EQ(s.ToString(),
+                "Remote error: Not authorized: alice is not allowed to call this method");
+    }
   }
 
   // Try some calls as "bob".

http://git-wip-us.apache.org/repos/asf/kudu/blob/b24f166e/src/kudu/rpc/service_if.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/service_if.cc b/src/kudu/rpc/service_if.cc
index 008c478..af53452 100644
--- a/src/kudu/rpc/service_if.cc
+++ b/src/kudu/rpc/service_if.cc
@@ -110,20 +110,14 @@ void GeneratedServiceIf::Handle(InboundCall *call) {
   }
   Message* resp = method_info->resp_prototype->New();
 
-  bool track_result = call->header().has_request_id()
-                      && method_info->track_result
-                      && FLAGS_enable_exactly_once;
-  RpcContext* ctx = new RpcContext(call,
-                                   req.release(),
-                                   resp,
-                                   track_result ? result_tracker_ : nullptr);
+  RpcContext* ctx = new RpcContext(call, req.release(), resp);
   if (!method_info->authz_method(ctx->request_pb(), resp, ctx)) {
     // The authz_method itself should have responded to the RPC.
     return;
   }
 
-  if (track_result) {
-    RequestIdPB request_id(call->header().request_id());
+  if (call->header().has_request_id() && method_info->track_result &&
FLAGS_enable_exactly_once) {
+    ctx->SetResultTracker(result_tracker_);
     ResultTracker::RpcState state = ctx->result_tracker()->TrackRpc(
         call->header().request_id(),
         resp,


Mime
View raw message