You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/02/11 21:16:54 UTC

[kudu-CR] [WIP] Ranger client

Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15206


Change subject: [WIP] Ranger client
......................................................................

[WIP] Ranger client

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M CMakeLists.txt
A src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/ranger.proto
A src/kudu/ranger/ranger_action.cc
A src/kudu/ranger/ranger_action.h
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/client.h
M src/kudu/subprocess/server.h
10 files changed, 721 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h
File src/kudu/ranger/ranger_action.h:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h@29
PS12, Line 29:   INSERT,
             :   UPDATE,
             :   DELETE,
             :   ALTER,
             :   CREATE,
             :   DROP,
Drawing from CRUD for a minute, I'm curious why we chose to separate offer "create", "update", and "delete" actions for DDL and DML operations. Why not use the same set of actions for both, but require broader scoping for DDL (i.e. ON DATABASE vs. ON TABLE)?



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 26 Feb 2020 21:33:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 9:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client-test.cc@161
PS8, Line 161:   ASSERT_EQ(2, columns.size());
> Use ContainsKey() from gutil/map-util.h
Can't, it only works on collections that have a find method. Unfortunately vector doesn't have a find method, we need to pass the vectors begin & and pointers to std::find(). Changed it on the unordered_set above though.


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:

http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.h@46
PS8, Line 46: virtual Status Start() WARN_UNUSED_RESULT;
            : 
> Should also mention that this returns an error if the table name doesn't co
changed the behavior to not return errors in that case


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.h@70
PS8, Line 70:   Status SendRequest(RangerRequestListPB* req, RangerResponseL
> Could you add a TODO to make this a unique_ptr or a member variable when we
Done


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@48
PS8, Line 48: 
> warning: pass by value and use std::move [modernize-pass-by-value]
Done


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@112
PS8, Line 112:   string db;
> warning: loop variable is copied but only used as const reference; consider
Done


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@153
PS8, Line 153:     return Status::InvalidArgument("Empty set of tables");
> Should use const auto& here
Done


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@154
PS8, Line 154: 
> nit: prefer emplace_back() instead of push_back(), here and elsewhere.
Done


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@158
PS8, Line 158:   req_list.set_user(user_name);
> This means that if there are any tables at all that don't conform to the Ra
good point, maybe it's better to just skip authorizing them, rewrote it.


http://gerrit.cloudera.org:8080/#/c/15206/8/src/kudu/ranger/ranger_client.cc@171
PS8, Line 171: 
> We should probably validate that responses_size() == orig_table_names.size(
good idea, do you have a preference which one it should be?



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 25 Feb 2020 17:44:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 13:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/15206/12//COMMIT_MSG
Commit Message:

PS12: 
> Could you comment in the commit message regarding the change to ParseRanger
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h
File src/kudu/ranger/ranger_action.h:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h@27
PS12, Line 27: 
> Add a doc here?
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_action.h@29
PS12, Line 29: 
             : 
             : 
             : 
             : 
             : 
> Maybe I forgot that for Kudu's Ranger policy, there is no such entity scopi
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@55
PS12, Line 55:       column_name == rhs.column_name;
> This seems like it could be stack allocated.
Partially done, but I think I'm missing something. How would I inject the mock subprocess server to the real client if it's stack allocated in RangerClient?


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@124
PS12, Line 124:     auto packed_resp = new Any();
              :     packed_resp->PackFrom(resp_list);
              :     resp->set_allocated_response(packed_resp);
              : 
              :     r
> Rewrite as AddResponse(table == "default.foobar");
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@38
PS12, Line 38: //
> Why do these functions need to be virtual? Is it for future mocking? If so,
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@41
PS12, Line 41: 
> You can drop the kudu prefix and I think get by with one of the following:
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@54
PS12, Line 54: 
> perform an
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.h@56
PS12, Line 56: perform an action on, it 
> Why bother? Won't the NotAuthorized alone signal to the caller that the use
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@54
PS12, Line 54:   return server_->Init();
> Doesn't seem all that interesting; maybe omit it? Or downgrade to VLOG?
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@67
PS12, Line 67: 
> We should handle the return value in some way.
added a DCHECK, do you think that's enough?


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@75
PS12, Line 75:   Slice tbl;
             : 
             :   // ParseRangerTableIdentifier
> Let's move this down to where we actually need it (i.e. L86).
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@82
PS12, Line 82:   if (PREDICT_FALSE(!s.ok())) {
             :     return Status::OK();
             :   }
> At the very least this warrants a comment: why should we ignore a bad statu
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@94
PS12, Line 94:   req->set_table(tbl.ToString());
> Use DCHECK_EQ.
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@136
PS12, Line 136:     req->set_column(col);
> To adhere to the common calling convention used in Kudu, let's restructure 
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@170
PS12, Line 170: 
> Nit: why this empty line?
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.h
File src/kudu/subprocess/server.h:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.h@187
PS12, Line 187: //
> I'd supplement this doc with a quick note about why the public methods are 
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/subprocess/server.cc@141
PS12, Line 141:   if (!closing_.CountDown()) {
> This isn't super robust because it assumes a particular ordering in Init();
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Mar 2020 18:31:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 17:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger.proto
File src/kudu/ranger/ranger.proto:

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger.proto@27
PS16, Line 27: // The action type mapping is similar to the one in Sentry which was implemented
             : // before Ranger and the same privileges have to be enforced with both
             : // authorization providers.
             : //
             : // For more information on fine grained authz check docs/security.adoc
             : //
             : // SELECT - select row
> Agreed re: the usefulness of linking to existing docs, though we shouldn't 
Done


http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@55
PS12, Line 55: struct AuthorizedAction {
> Hmm, that may not work, but at least you could convert the shared ownership
hm SubprocessServer has an atomic member (next_id_) so it's not copy/move constructible. Do you think it's worth adding a move constructor to it so we can use a unique_ptr here? Or can you think of a better approach?


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc
File src/kudu/ranger/ranger_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@68
PS16, Line 68:   }
> Again, define a hasher rather than overloading std::hash.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@112
PS16, Line 112: 
> Could just as easily RETURN_NOT_OK.
RETURN_NOT_OK only works for methods that return a Status, this returns a boolean. I can return a runtime error or something, but this should always work, unless an incorrect protobuf message is returned which doesn't seem too likely and would indicate a serious error.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@128
PS16, Line 128: 
              :   void SetUp() override {
              :     server_->next_response_ = &next_response_;
> Could maybe do resp->mutable_response()->PackFrom(resp_list)?
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@38
PS16, Line 38: eturn 
> nit: Creates?
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@47
PS16, Line 47: 
> nit: indent.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@54
PS16, Line 54: d::strin
> Same here.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@59
PS16, Line 59: 
> It's not a vector.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@60
PS16, Line 60:  
> nit: is authorized to access if OK status is returned.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@62
PS16, Line 62: 
             : 
> Same here.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@69
PS16, Line 69:                
> nit: should it be named to AuthorizeActions given its functionality?
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@70
PS16, Line 70:        std::unordered_set<std::string>* column_names)
             :       WARN_UNUSED_RESULT;
> And here.
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@82
PS16, Line 82:   // Sends request to the subprocess and parses the response.
             :   Status SendRequest(RangerRequestListPB* req, RangerResponseListPB* resp) WARN_UNUSED_RESULT;
             : 
             :   std::shared_ptr<kudu::subprocess::SubprocessServer> server_;
             : };
             : 
             : } // namespace ranger
             : } 
> This is a no-no in the Google Style Guide: https://google.github.io/stylegu
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@55
PS16, Line 55: atus RangerClient::Star
> Add a TODO here if you are going to have a follow up patch to actually call
this will only call init, ranger authz provider sets up the server, do you think that's a good approach? if it is, I don't think it's necessary to add a todo here.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@67
PS16, Line 67: RETURN_NOT_OK(server_->Execute(&sreq, 
> Maybe use DCHECK instead?
that was the original approach, but it got optimized out and it segfaulted in release mode. As this needs to run anyway, I think between storing the result in a variable and DCHECK on that and simply CHECK I think it's better to CHECK, but let me know if you disagree.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@79
PS16, Line 79: 
             :   auto s = ParseRangerTableIdentifier(table_name, &db, &tbl);
             :   if (PREDICT_FALSE(!s.ok())) {
> I tend to agree. Like with Sentry (though that was due to HMS integration),
Even with Sentry only conflicting tables had to be renamed. Tables with names that are invalid in Sentry are unmodified. OTOH those tables are denied in Sentry too so I'm changing it to deny non-Ranger tables and log a warning.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@156
PS16, Line 156:   return Status::OK();
> BTW, the Sentry provider doesn't provide any detail besides "unauthorized a
Done


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/subprocess/server.cc@155
PS16, Line 155:   if (write_thread_) {
> Nit: it's C++, so you can simplify to:
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 13:18:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#8).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/ranger_action.cc
A src/kudu/ranger/ranger_action.h
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
13 files changed, 639 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/8
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#21).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Removes default_database out parameter from ParseRangerTableIdentifier
in table_util as we decided to take a slightly different path and this
information is not needed anymore.

Current test coverage consists of only unit tests with a mock subprocess
server. Real integration tests will be added in a follow-up patch once a
working MiniRanger is added to facilitate this testing.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/ranger/ranger.proto
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/subprocess/subprocess_proxy.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
13 files changed, 843 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/21
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 16:

Also it seems there is a IWYU failure for anger_client-test.cc.


-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Mar 2020 23:46:30 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#24).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Removes default_database out parameter from ParseRangerTableIdentifier
in table_util as we decided to take a slightly different path and this
information is not needed anymore.

Current test coverage consists of only unit tests with a mock subprocess
server. Real integration tests will be added in a follow-up patch once a
working MiniRanger is added to facilitate this testing.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/ranger/ranger.proto
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/subprocess/subprocess_proxy.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
13 files changed, 831 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/24
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 24
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#17).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Removes default_database out parameter from ParseRangerTableIdentifier
in table_util as we decided to take a slightly different path and this
information is not needed anymore.

Current test coverage consists of only unit tests with a mock subprocess
server. Real integration tests will be added in a follow-up patch once a
working MiniRanger is added to facilitate this testing.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/ranger/ranger.proto
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
12 files changed, 751 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/17
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.

Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 19: Code-Review+1

(5 comments)

Overall looks good to me, thanks a lot for working on the patch Attila!

http://gerrit.cloudera.org:8080/#/c/15206/19/src/kudu/ranger/ranger.proto
File src/kudu/ranger/ranger.proto:

http://gerrit.cloudera.org:8080/#/c/15206/19/src/kudu/ranger/ranger.proto@33
PS19, Line 33: // SELECT - select rows in a table
             : // INSERT - insert to a table
             : // UPDATE - update existing rows in a table
             : // DELETE - delete rows in a table
             : // ALTER - alter schema
             : // CREATE - creating a table
             : // DROP - drop a table
nit: I think this can be removed now, and it is not quite accurate.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@55
PS16, Line 55: atus RangerClient::Star
> this will only call init, ranger authz provider sets up the server, do you 
A little surprised to see it is set in ranger authz provider as Ranger client should be able to successfully connect to Ranger  subprocess on its own. However it is fine for now as mini Ranger is not in place.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@67
PS16, Line 67: RETURN_NOT_OK(server_->Execute(&sreq, 
> that was the original approach, but it got optimized out and it segfaulted 
Ack, LGTM.


http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@79
PS16, Line 79: 
             :   auto s = ParseRangerTableIdentifier(table_name, &db, &tbl);
             :   if (PREDICT_FALSE(!s.ok())) {
> Even with Sentry only conflicting tables had to be renamed. Tables with nam
Ack


http://gerrit.cloudera.org:8080/#/c/15206/19/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/19/src/kudu/ranger/ranger_client.cc@82
PS19, Line 82: Denying action on table with invalid name
Maybe also advise the user to update the table name via table rename tool?



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 19
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 19:15:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 20:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:

http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@46
PS20, Line 46: 
             :   // Authorizes an action on the table. Returns OK if 'user_name' is authorized
             :   // to perform 'action' on 'table_name', NotAuthorized otherwise.
             :   Status AuthorizeAction(const std::string& user_name, const ActionPB& action,
             :                          const std::string& table_name) WARN_UNUSED_RESULT;
Is this needed if we have AuthorizeActions()?


http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@69
PS20, Line 69:  a table-level action 
nit: "multiple table-level actions"


http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.h@74
PS20, Line 74:                           std::unordered_set<ActionPB, ActionHash>* actions,
nit: per the GSG, output parameters should be last.


http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@73
PS20, Line 73: 
             : Status RangerClient::SendRequest(RangerRequestListPB* req, RangerResponseListPB* resp) {
             :   SubprocessRequestPB sreq;
             :   SubprocessResponsePB sresp;
             : 
             :   auto packed_req = new Any();
             :   packed_req->PackFrom(*req);
             :   sreq.set_allocated_request(packed_req);
             :   RETURN_NOT_OK(server_->Execute(&sreq, &sresp));
             : 
             :   CHECK(sresp.response().UnpackTo(resp));
             : 
             :   return Status::OK();
             : }
Depending on the order of what lands, may want to rebase on this and typedef some RangerSubprocess:
https://gerrit.cloudera.org/c/15344/

Though we can always address that later too.


http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@112
PS20, Line 112:   CHECK_EQ(resp_list.responses_size(), 1);
nit: flip the order of the arguments so the error message is more correct (expected value first). Same elsewhere.


http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/ranger/ranger_client.cc@276
PS20, Line 276: string RangerClient::GetJavaClasspath() {
              :   Env* env = Env::Default();
              :   string exe;
              :   CHECK_OK(env->GetExecutablePath(&exe));
              :   const string bin_dir = DirName(exe);
              :   return Substitute("$0/kudu-ranger.jar:$1", bin_dir, FLAGS_ranger_config_path);
              : }
Will the JARs always be located in the binary directory? I was under the impression that many deployments have their own separate directory for JARs (e.g. Kudu backup JARs in CDH are located in a JARs directory). If that's the case, would it make sense to have the JAR path be configurable?


http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15206/20/src/kudu/subprocess/server.cc@147
PS20, Line 147:   if (process_->IsStarted()) {
              :     WARN_NOT_OK(process_->KillAndWait(SIGTERM), "failed to stop subprocess");
              :   }
              :   inbound_response_queue_.Shutdown();
              :   outbound_call_queue_.Shutdown();
              : 
              :   // We should be able to clean up our threads; they'll see that we're closing,
              :   // the pipe has been closed, or the queues have been shut down.
              :   if (write_thread_) {
              :     write_thread_->Join();
              :   }
              :   if (read_thread_) {
              :     read_thread_->Join();
              :   }
              :   if (deadline_checker_) {
              :     deadline_checker_->Join();
              :   }
nit: could you mention in a comment that these are all expected to always be there, except for in the mock?



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 20
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 23:00:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#16).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Removes default_database out parameter from ParseRangerTableIdentifier
in table_util as we decided to take a slightly different path and this
information is not needed anymore.

Current test coverage consists of only unit tests with a mock subprocess
server. Real integration tests will be added in a follow-up patch once a
working MiniRanger is added to facilitate this testing.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/ranger/ranger.proto
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
12 files changed, 790 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/16
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#13).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Removes default_database out parameter from ParseRangerTableIdentifier
in table_util as we decided to take a slightly different path and this
information is not needed anymore.

Current test coverage consists of only unit tests with a mock subprocess
server. Real integration tests will be added in a follow-up patch once a
working MiniRanger is added to facilitate this testing.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/ranger/ranger.proto
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
12 files changed, 791 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/13
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )

Change subject: KUDU-2972 Add Ranger client
......................................................................


Patch Set 10:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/common/table_util.h
File src/kudu/common/table_util.h:

http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/common/table_util.h@46
PS10, Line 46: Status
> nit: can you comment on when the returned Status will not be ok?
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.h
File src/kudu/ranger/ranger_action.h:

http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.h@39
PS10, Line 39: ActionPB ActionToActionPB(const Action& action);
> nit: Can you add comments for the methods in this class.
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc
File src/kudu/ranger/ranger_action.cc:

http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc@46
PS10, Line 46:       return ActionPB::METADATA;
> LOG(FATAL) for unknown action? Similar to https://github.com/apache/kudu/bl
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc@70
PS10, Line 70:   }
> Same here.
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:

http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.h@45
PS10, Line 45:  and client
> nit: remove?
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:

http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@54
PS10, Line 54:   return server_->Init();
> Add a logging?
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@93
PS10, Line 93: if (resp_list.responses()
> DCHECK the response size?
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@97
PS10, Line 97: Substitute("User %s is not authorized to "
             :                                           "perform %s on %s",
> Should we add a debug logging if the user is not authorized?
not sure if it's worth spamming the logs with it, a future audit log might be a better place for it. What do you think?


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@134
PS10, Line 134: req_list
> Should we check the req_list size is the same as resp_list?
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@162
PS10, Line 162: non_ranger_tables
> nit: maybe name it to invalid_ranger_tables?
Done


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@178
PS10, Line 178: non_ranger_tables.emplace_back(table);
> Add a log here for invalid ranger tables?
if we choose the approach I explained below I don't think it's necessary, maybe vlog?


http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@193
PS10, Line 193:   for (auto i = 0; i < non_ranger_tables.size(); ++i) {
              :     table_names->emplace(non_ranger_tables[i]);
              :   }
> why we place the invalid ranger table back to table_names? Shouldn't we den
as they can't be managed by Ranger anyway I thought it would be better to retain existing functionality which is allow everything for authenticated users.



-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 26 Feb 2020 11:51:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2972 Add Ranger client

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15206

to look at the new patch set (#9).

Change subject: KUDU-2972 Add Ranger client
......................................................................

KUDU-2972 Add Ranger client

Adds a Ranger client that communicates with the Java subprocess that
wraps the Ranger plugin. It can be used by an authorization provider to
translate between Kudu and Ranger.

Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
---
M src/kudu/common/table_util-test.cc
M src/kudu/common/table_util.cc
M src/kudu/common/table_util.h
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/ranger_action.cc
A src/kudu/ranger/ranger_action.h
A src/kudu/ranger/ranger_client-test.cc
A src/kudu/ranger/ranger_client.cc
A src/kudu/ranger/ranger_client.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
13 files changed, 662 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15206/9
-- 
To view, visit http://gerrit.cloudera.org:8080/15206
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4
Gerrit-Change-Number: 15206
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)