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/04/09 16:10:27 UTC
[kudu-CR] [ranger] Fix and refactor RangerClient
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15696
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
[ranger] Fix and refactor RangerClient
While working on integration tests we found a discrepancy handling
column-level privileges between Ranger and Sentry. This was caused by
RangerClient returning NotAuthorized when the requested action couldn't
be authorized on any of the resources. This commit moves all
authorization logic to the authorization provider instead of the client.
Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
---
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
3 files changed, 8 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/15696/1
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15696
to look at the new patch set (#4).
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
[ranger] Fix and refactor RangerClient
While working on integration tests we found a discrepancy handling
column-level privileges between Ranger and Sentry. This was caused by
RangerClient returning NotAuthorized when the requested action couldn't
be authorized on any of the resources. This commit moves all
authorization logic to the authorization provider instead of the client.
Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
---
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
4 files changed, 212 insertions(+), 159 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/15696/4
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/15696/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15696/1//COMMIT_MSG@12
PS1, Line 12: This commit moves all
: authorization logic to the authorization provider instead of the client.
> We are still returning NotAuthorized for AuthorizeAction case?
Done
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@92
PS1, Line 92: names' to the
> Does it make sense to document what this method returns if Ranger client or
Not sure if it's worth documenting it tbh, this can return many things, like IOError, EndOfFile and Corruption depending on what exactly goes wrong and where. Ranger client not being available is just one such a case, and Ranger server not being available is not even an error.
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@103
PS1, Line 103: std::unordered_set<ActionPB, ActionHash>* actions)
> update this too
Done
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc@420
PS1, Line 420:
> nit: consider moving this parsing into the authz provider too. Same below
hm I could do that here, but what about AuthorizeActionMultipleTables? Shouldn't we be consistent about it? We need to check for other errors anyway.
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc@445
PS1, Line 445: auto s = ParseRangerTableIdentifier(table_name, &db, &tbl);
> consider making a bool out parameter for this
Done
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 09 Apr 2020 21:00:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 18:03:54 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc@76
PS2, Line 76:
: return Status::OK();
: }
> micro-nit: might be nicer to read if the main scope is the common path, and
Done
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc@202
PS2, Line 202:
> Consider breaking this out in a RETURN_NOT_OK(). If we failed to make the r
Done
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc@220
PS2, Line 220: string db;
: string tbl;
> Should this be a RETURN_NOT_OK() too?
Done
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@92
PS1, Line 92:
> +1 SGTM, a comment in RangerClient class is a good idea.
Done
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc@420
PS1, Line 420: return Status::OK();
> Yeah, updating the interface to be a bunch of pair<string, string>s doesn't
I realized we don't even need to do that as this method doesn't return NotAuthorized if the table name can't be parsed, it only logs a warning and skips it. I decided to leave the current logic for this one as it saves us two extra loops.
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 14:13:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc@76
PS2, Line 76: if (PREDICT_TRUE(authorized)) {
: return Status::OK();
: }
micro-nit: might be nicer to read if the main scope is the common path, and these extra branches are PREDICT_FALSEs with on-error behavior. Same elsewehere.
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc@202
PS2, Line 202: (client_.AuthorizeAction(user, ActionPB::ALL, table_name, &authorized).ok()
Consider breaking this out in a RETURN_NOT_OK(). If we failed to make the request, that should be surfaced here, right? Something like:
bool has_all_privileges;
if (IsTrustedUser(user)) {
has_all_privileges = true;
} else {
RETURN_NOT_OK(AuthorizeAction(&has_all_privileges));
}
if (has_all_privileges) {
// fill in the pb
}
http://gerrit.cloudera.org:8080/#/c/15696/2/src/kudu/master/ranger_authz_provider.cc@220
PS2, Line 220: auto s = client_.AuthorizeActions(user, table_name, &actions);
: if (s.ok()) {
Should this be a RETURN_NOT_OK() too?
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@92
PS1, Line 92: names' to the
> Not sure if it's worth documenting it tbh, this can return many things, lik
It's probably worth adding a comment about this behavior in the RangerClient class comment. Ideally, a reader of this code should not have to read other parts of the codebase to understand the semantics the RangerClient. Maybe high level descriptions of what to expect, e.g. that if the underlying subprocess has failed, the methods will return errors, and that if Ranger is unavailable, the underlying Ranger subprocess relies on cached privileges or somesuch.
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc@420
PS1, Line 420:
> hm I could do that here, but what about AuthorizeActionMultipleTables? Shou
Yeah, updating the interface to be a bunch of pair<string, string>s doesn't seem unreasonable.
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 21:21:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
[ranger] Fix and refactor RangerClient
While working on integration tests we found a discrepancy handling
column-level privileges between Ranger and Sentry. This was caused by
RangerClient returning NotAuthorized when the requested action couldn't
be authorized on any of the resources. This commit moves all
authorization logic to the authorization provider instead of the client.
Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Reviewed-on: http://gerrit.cloudera.org:8080/15696
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Attila Bukor <ab...@apache.org>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
4 files changed, 219 insertions(+), 166 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, but someone else must approve
Attila Bukor: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@92
PS1, Line 92: NotAuthorized
Does it make sense to document what this method returns if Ranger client or Ranger itself is not available?
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:59:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15696
to look at the new patch set (#3).
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
[ranger] Fix and refactor RangerClient
While working on integration tests we found a discrepancy handling
column-level privileges between Ranger and Sentry. This was caused by
RangerClient returning NotAuthorized when the requested action couldn't
be authorized on any of the resources. This commit moves all
authorization logic to the authorization provider instead of the client.
Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
---
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
4 files changed, 206 insertions(+), 155 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/15696/3
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15696/4/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15696/4/src/kudu/master/ranger_authz_provider.cc@61
PS4, Line 61: auto s = ParseRangerTableIdentifier(table_name, db, &tbl);
> Nit: could make this not static and put it in the anonymous namespace
Done
http://gerrit.cloudera.org:8080/#/c/15696/4/src/kudu/master/ranger_authz_provider.cc@145
PS4, Line 145: LOG(WARNING) << Substitute("User $0 is not authorized to ALTER $1", user, old_table);
> Alter
Done
http://gerrit.cloudera.org:8080/#/c/15696/3/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/3/src/kudu/ranger/ranger_client.h@93
PS3, Line 93: Status AuthorizeActionMultipleTables(const std::string& user_name, const ActionPB& action,
> warning: function 'kudu::ranger::RangerClient::AuthorizeActionMultipleTable
Done
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 17:41:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15696/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15696/1//COMMIT_MSG@12
PS1, Line 12: This commit moves all
: authorization logic to the authorization provider instead of the client.
We are still returning NotAuthorized for AuthorizeAction case?
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 1
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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:07:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15696
to look at the new patch set (#2).
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
[ranger] Fix and refactor RangerClient
While working on integration tests we found a discrepancy handling
column-level privileges between Ranger and Sentry. This was caused by
RangerClient returning NotAuthorized when the requested action couldn't
be authorized on any of the resources. This commit moves all
authorization logic to the authorization provider instead of the client.
Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
---
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
4 files changed, 97 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/15696/2
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15696
to look at the new patch set (#5).
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
[ranger] Fix and refactor RangerClient
While working on integration tests we found a discrepancy handling
column-level privileges between Ranger and Sentry. This was caused by
RangerClient returning NotAuthorized when the requested action couldn't
be authorized on any of the resources. This commit moves all
authorization logic to the authorization provider instead of the client.
Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
---
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
4 files changed, 219 insertions(+), 166 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/15696/5
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 18:17:45 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@103
PS1, Line 103: // NotAuthorized otherwise.
update this too
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc@420
PS1, Line 420: return Status::NotAuthorized(kUnauthorizedAction);
nit: consider moving this parsing into the authz provider too. Same below
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.cc@445
PS1, Line 445: return Status::NotAuthorized(kUnauthorizedAction);
consider making a bool out parameter for this
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 1
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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 09 Apr 2020 17:27:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15696/4/src/kudu/master/ranger_authz_provider.cc
File src/kudu/master/ranger_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/15696/4/src/kudu/master/ranger_authz_provider.cc@61
PS4, Line 61: static Status ParseTableIdentifier(const string& table_name, string* db, string* table) {
Nit: could make this not static and put it in the anonymous namespace
http://gerrit.cloudera.org:8080/#/c/15696/4/src/kudu/master/ranger_authz_provider.cc@145
PS4, Line 145: LOG(WARNING) << Substitute("User $0 is not authorized to DROP $1", user, old_table);
Alter
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 17:28:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 5: Verified+1
unrelated flaky test failures
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 Apr 2020 18:12:51 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger] Fix and refactor RangerClient
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15696 )
Change subject: [ranger] Fix and refactor RangerClient
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15696/1/src/kudu/ranger/ranger_client.h@92
PS1, Line 92: NotAuthorized
> It's probably worth adding a comment about this behavior in the RangerClien
+1 SGTM, a comment in RangerClient class is a good idea.
--
To view, visit http://gerrit.cloudera.org:8080/15696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3fb7272601c9cb7ebe2e250bea728e76894d242a
Gerrit-Change-Number: 15696
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Apr 2020 23:20:12 +0000
Gerrit-HasComments: Yes