You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2018/10/11 07:18:24 UTC
[kudu-CR] wip: [sentry] add SentryAuthzProvider
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11659
Change subject: wip: [sentry] add SentryAuthzProvider
......................................................................
wip: [sentry] add SentryAuthzProvider
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_authz_provider-test.cc
A src/kudu/sentry/sentry_authz_provider.cc
A src/kudu/sentry/sentry_authz_provider.h
4 files changed, 375 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/1
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: 1
> nit: does the default setting of 1 seem good enough for the production use?
I think one time retry is general enough to avoid to end the connection too early. Or you have other suggestions?
I do not quite follow the second question. Retry count and timeout settings are related but it doesn't mean the former is ignored.
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117
PS10, Line 117: TAG_FLAG(sentry_service_max_message_size_bytes, advanced);
: TAG_FLAG(sentry_service_max_message_size_bytes, experimental);
: TAG_FLAG(sentry_service_max_message_size_bytes, runtime);
> These only get set once when the AuthzProvider starts up; should the be non
Makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 00:33:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: u
> I mean look how Kudu RPC works -- you don't specify number of retries, you
Hmm, I think the scenario is bit different where here is the flag for Kudu as a Sentry client where the retry with exponential backoff (https://github.com/apache/kudu/blob/master/src/kudu/thrift/client.h#L236). And the timeouts are per retry.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 18:57:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 14: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: u
> Hmm, I think the scenario is bit different where here is the flag for Kudu
I took a look into the thrift client code. Apparently, the approach used there is different than in Kudu client and looks simpler. That's fine, especially if those default values for the parameters proved to be reasonable after some testing.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 05 Nov 2018 17:15:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#10).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
14 files changed, 1,003 insertions(+), 116 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/10
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() = 0;
:
: // Stops the AuthzProvider instance.
: virtual void Stop() =
> The same reason as for having the AuthorizeXxx() method as pure virtual in
It is hard to say it is not important for Start()/Stop() in that sense, so updated it.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147
PS7, Line 147:
> Oh, I guess 'Authorize' in this sense means 'check the for authorization'.
I will update it to avoid confusion. Thanks!
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@238
PS7, Line 238: ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata("db.table", kTestUser));
> What happens if Sentry server is not responsive e.g., due network errors or
Yeah, in L249 it is testing what error we are getting when Sentry service is not responsive. I will add a simulation of busy server too though.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@51
PS7, Line 51: TABLE,
> nit: here and below drop 'virtual' since that's a derived class and the 'ov
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
> Ah, OK. Thank you for clarification.
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@235
PS7, Line 235: RETURN_NOT_OK(Authorize(table_authorizable, table_action, user));
> I don't think this needs to be a part of the SentryAuthzProvider class. May
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 20:04:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 13: Code-Review+2
LGTM as long as Alexey's content
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 20:59:59 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:13:55 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11659/2/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/2/src/kudu/sentry/sentry_authz_provider.cc@34
PS2, Line 34: #include "kudu/util/flag_tags.h"
> warning: using decl 'TSentryPrivilege' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/11659/2/src/kudu/sentry/sentry_authz_provider.cc@38
PS2, Line 38: using sentry::TListSentryPrivilegesRequest;
> warning: using decl 'Substitute' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/11659/2/src/kudu/sentry/sentry_authz_provider.cc@134
PS2, Line 134: bool* authorized) {
> warning: parameter 'new_table' is unused [misc-unused-parameters]
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Oct 2018 18:41:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.
Change subject: [sentry] SentryAuthzProvider
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] wip: [sentry] add SentryAuthzProvider
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: wip: [sentry] add SentryAuthzProvider
......................................................................
Patch Set 1:
(2 comments)
high level looks good, I didn't check the actual authorization methods, since I think they will change based on our conversations yesterday
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.h
File src/kudu/sentry/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.h@32
PS1, Line 32: // A high-level API above the HMS which handles converting to and from
update doc
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.cc@60
PS1, Line 60: Sentery
Sentry
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 12 Oct 2018 17:19:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#7).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
13 files changed, 907 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/7
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117
PS10, Line 117:
: namespace kudu {
:
> Makes sense.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 00:42:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] wip: [sentry] add SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#2).
Change subject: wip: [sentry] add SentryAuthzProvider
......................................................................
wip: [sentry] add SentryAuthzProvider
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_authz_provider-test.cc
A src/kudu/sentry/sentry_authz_provider.cc
A src/kudu/sentry/sentry_authz_provider.h
M src/kudu/sentry/sentry_client-test.cc
5 files changed, 615 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/2
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 6:
(27 comments)
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@13
PS5, Line 13: is placed in th
> is placed in the
Done
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14
PS5, Line 14: the fu
> nit: ...in the future, other...
Done
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14
PS5, Line 14: in the fu
> in the future
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h
File src/kudu/master/default_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@32
PS4, Line 32: virtual Status AuthorizeCreateTable(const std::string& /*table_name*/,
> The parameter is required, but the preferred way to add arguments that aren
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@78
PS5, Line 78:
: Status StopSentry() {
: RETURN_NOT_OK(sentry_client_->Stop());
:
> If that's all this does, you don't need it.
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@122
PS5, Line 122: const TSentryGrantOption::type& grant_option) {
: TSentryPrivilege privilege;
: privilege.__set_privilegeScope("DATABASE");
: privilege.__set_serverName(FLAGS_server_name);
> nit: Any reason to not have this return the TSentryPrivilege directly? Same
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@95
PS5, Line 95: RETURN_NOT_OK(sentry_client_->CreateRole(role_req));
:
: TSentryGroup group;
: group.groupName = group_name;
: set<TSentryGroup> groups;
: groups.insert(group);
: TAlterSentryRoleAddGroupsRequest group_request;
: TAlterSentryRoleAddGroupsResponse group_response;
: group_request.__set_requestorUserName(kAdminUser);
: group_request.__set_roleName(role_name);
: group_request.__set_groups(groups);
: return sentry_client_->AlterRoleAddGroups(group_request, &group_response);
: }
:
: Status AlterRoleGrantPrivilege(const string& role_name, const TSentryPrivilege& privilege) {
: TAlterSentryRoleGrantPrivilegeRequest privilege_request;
: TAlterSentryRoleGrantPrivilegeResponse privilege_response;
: privilege_request.__set_requestorUserName(kAdminUser);
: privilege_request.__set_roleName(role_name);
: privilege_request.__set_privilege(privilege);
: return sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response);
: }
:
: // Returns a database level TSentryPrivilege with the given database name, action
: // and grant option.
: TSentryPrivilege GetDatabasePrivilege(const string& db_name,
: const string& action,
: const TSentryGrantOption::type& grant_option) {
: TSentryPrivilege privilege;
: privilege.__set_privilegeScope("DATABASE");
: privilege.__set_serverName(FLAGS_server_name);
: privilege.__set_dbName(db_name);
: privilege.__set_action(action);
: privilege.__set_grantOption(TSentryGrantOption::TRUE);
: return privilege;
: }
:
: // Returns a table level TSentryPrivilege with the given table name, database name,
: // action and grant option.
: TSentryPrivilege GetTablePrivilege(const string& db_name,
: const stri
> nit: docs for these?
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@140
PS5, Line 140: return privilege;
: }
:
: protected:
: unique_ptr<SentryAuthzProvider> sentry_authz_provider_;
: };
:
: INSTA
> nit: maybe
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@177
PS5, Line 177: , "AL
> nit: for boolean params, it's generally easier to read coming back to it as
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@181
PS5, Line 181:
> nit: comma not needed
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270
PS5, Line 270:
> nit: spacing
I thought wrapped function has 4 space indent?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@288
PS5, Line 288:
> Here and below, the expected value should come first.
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@295
PS5, Line 295:
> nit: spacing
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@57
PS5, Line 57: // user in Sentry, or Sentry fai
> Probably worth doc'ing this, if its behavior is specific to Sentry
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@62
PS5, Line 62: const std::string& user,
: const std::string& owner) override WARN_UNUSED_RESULT;
:
> Rather than repeating this in every comment, can you put this in the block
Right, it corresponds to the config 'sentry.service.admin.group' in Sentry Server. After a second though I think it might be good to expose it as a gflag as well.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@96
PS5, Line 96: const std::string& user,
> Could this use ValidateAddresses instead? How useful are the Statuses provi
Remove this to https://gerrit.cloudera.org/#/c/11843/.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@111
PS4, Line 111:
:
> I'm not Dan but I wanted to make sure this was chased down. Is the API you'
Currently, connection to Sentry server is guarded by 'sentry.service.allow.connect' config, that only the trusted service users can connect to the Sentry service. Therefore, I fail to see the reasons this is less safe than other Sentry APIs.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187
PS4, Line 187:
> I'm inclined to agree with Hao: reuse the bulk of ParseStrings() to share c
Put it to https://gerrit.cloudera.org/#/c/11843/.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@59
PS5, Line 59: Server2 configuration, and the value of the --server_name "
: "in Impala configuration.");
: TAG_FLAG(server_name, experimental);
> How will this fail if these statements aren't true?
Different server name implies different privilege is checked. That means the authorization might fail or the user might be able to access databases/tables belong to other server that she/he doesn't have privileges. Though I don't see a way Kudu can detect that without connecting to HiveServer2 and Impala.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@149
PS5, Line 149: 2. 'CREATE ON DATABASE
> Does capitalization matter at this point?
Kudu does enforce case sensitivity for table names. Even though with HMS integration enabled Kudu follows HMS table naming rule, the catalog manager has logic to normalize the table name. So I think it makes sense to do a case sensitive comparison.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259
PS5, Line 259: return Status::OK();
: }
:
> Does the user's request have more information than what's presented in this
For example, if user Bob has ALL privilege on table B and he wants to check if table A exists. He can alter table B to A. If he gets the error message with 'alter to table A is not permitted' instead of AlreadyPresent, then he knows that table A already does not exist.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@262
PS5, Line 262: e master
: } // namespace kudu
> nit: use Substitute
I don't see there is a method in auto-gen to convert TSentryAuthorizable to string?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@264
PS5, Line 264:
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@275
PS5, Line 275:
> nit: maybe don't set these until we know we'll return OK? Is there a move c
Yeah, I don't think here it matters. Since ParseHiveTableIdentifier in L279 should not be called again the call in L274 succeeds.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry-test-base.h@76
PS5, Line 76: kerberos_enabled
> nit: maybe end with a _?
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc@56
PS5, Line 56:
: TEST_P(SentryClientTest, TestMiniSentryLifecycle) {
: // Create an HA Sentry client and ensure it automatically reconnects after service interruption.
: thrift::HaClient<SentryClient> client;
: thrift::ClientOptions sentry_client_opts;
: if (kerberos_enabled_) {
:
> Don't need these since they're both the default.
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc@56
PS5, Line 56:
: TEST_P(SentryClientTest, TestMiniSentryLifecycle) {
: // Create an HA Sentry client and ensure it automatically reconnects after service interruption.
: thrift::HaClient<SentryClient> client;
: thrift::ClientOptions sentry_client_opts;
: if (kerberos_enabled_) {
:
> Here too; if all this is doing is calling the superclass, it can be omitted
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 03:39:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(7 comments)
Mostly just a few nits at this point.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270
PS5, Line 270:
> I thought wrapped function has 4 space indent?
That's what we do for wrapping long lines.
From the GSG (https://google.github.io/styleguide/cppguide.html#Formatting_Lambda_Expressions):
"Format parameters and bodies as for any other function"
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@55
PS7, Line 55: This following methods will fail if the operation is not authorized, or the
: // Sentry service is unreachable, or 'kudu' (Kudu server name) is a non-admin
: // user in Sentry, or Sentry fails to resolve the group mapping of
: // the user.
How about we split these out so it's a bit easier to read.
The following authorizing methods will fail if:
- the operation is not authorized
- the Sentry service is unreachable
- the specified '--kudu_service_name' is a non-admin user in Sentry
- Sentry fails to resolve the group mapping of the user
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@90
PS7, Line 90: //
: // This method will fail if the Sentry service is unreachable, or 'kudu'
: // (Kudu server name) is a non admin user in Sentry, or Sentry fail to
: // resolve the group mapping of the user.
nit: not needed given the comment up top?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259
PS5, Line 259: return Status::OK();
: }
:
> For example, if user Bob has ALL privilege on table B and he wants to check
Ah, thanks for the explanation. So based on where the Authorize() fails, a malicious user might be able to discover something they shouldn't?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@115
PS7, Line 115: on
super nit: elsewhere we seem to favor capitalizing the whole of "ALL ON DATABASE"
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@209
PS7, Line 209:
nit: spacing here too
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@235
PS7, Line 235: Status SentryAuthzProvider::GetAuthorizable(const string& table_name,
I don't think this needs to be a part of the SentryAuthzProvider class. Maybe stick it in an anonymous namespace?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 06:04:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147
PS7, Line 147: Authorize
> Here and below for negative cases: did you actually mean 'Don't authorize'
Oh, I guess 'Authorize' in this sense means 'check the for authorization'. If that's a sort of lingo used, that means it was just my misunderstanding: feel free to ignore my comment then.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 18:29:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add SentryAuthzProvider
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add SentryAuthzProvider
......................................................................
Patch Set 4:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@10
PS4, Line 10: retries, reconnections, HA, error
: handling, and concurrent requests
this is all handled by the existing thrift::HaClient.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@67
PS4, Line 67: AuthorizeGetTableMetadata
Where is this intended to be used?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider-test.cc@67
PS4, Line 67: ="
whitespace here and below
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@57
PS4, Line 57: OVERRIDE
'override' here and below
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@88
PS4, Line 88: virtual Status AuthorizeGetTableMetadata(const std::string& table_name,
Can you remind me what master/catalog manager ops need to call this?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@111
PS4, Line 111: or 'kudu'
: // (Kudu server name) is a non admin user in Sentry
Is there any way we can recognize this failure scenario as part of Start()? I think the earlier we can catch this, the easier it will be to surface a good error message, and consequently debug.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@110
PS4, Line 110: // then the creating user must have 'ALL on DATABASE' with grant.
Interesting - is there a link or somewhere you can reference in Sentry that explains why this is?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@140
PS4, Line 140: // For table alteration (with table rename) requires
Same thing here, I think there's a SENTRY JIRA that explains this distinction, would be good to link for future reference.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187
PS4, Line 187: // Note: the RPC addresses of the Sentry service(s) can be with/without
We already have HostPort::ParseStrings which does almost the exact same thing as this, however it doesn't handle schemes. I'd like to see this use that method instead of implementing largely the same logic again, which means either we need to not handle addresses with schemes, or add scheme support to ParseStrings. I'm leaning toward the former since I don't think schemes are used much for Sentry, but I don't really have data to back that up. I'm curious what others think.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@244
PS4, Line 244: (privilege.grantOption == TSentryGrantOption::UNSET ||
: privilege.grantOption == TSentryGrantOption::FALSE)
might be a bit safer to express this as 'privilege.grantOption != TSentryGrantOption::TRUE'. It's a bit simpler, and in the unlikely case that an additional variant is added it will 'fail closed'.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@250
PS4, Line 250: Status s = SentryAction::FromString(privilege.action, &granted_action);
I'd add a WARN_NOT_OK or similar here so that we get an indication in the logs if we aren't handling a new action type. This is especially useful since we are returning a non-specific error.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256
PS4, Line 256: Substitute
substitute isn't necessary
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256
PS4, Line 256: return Status::NotAuthorized(Substitute("unauthorized action"));
Is there a security reason to have a generic error message? It may be helpful to try and explain why the action is unauthorized, but maybe that could be a sidechannel leak? I don't have anything specific in mind, but it seems like a possible risk. In any case there should probably be a comment explaining which way we go.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@275
PS4, Line 275: CHECK(false) << "unsupported authorizable scope";
Prefer LOG(FATAL)
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 00:25:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider
......................................................................
Patch Set 3: Verified+1
Unrelated flaky AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Oct 2018 20:04:09 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#12).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
14 files changed, 994 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/12
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc@270
PS10, Line 270: ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata(
: "db.table", kTestUser));
: });
nit: aesthetically, I generally prefer, but feel free to ignore if you like this more (elsewhere too):
ASSERT_EVENTUALLY([&] {
ASSERT_OK(sentry_authz_provider_->AuthorizeGetTabletMetadata(
"db.table", kTestUser));
});
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117
PS10, Line 117: TAG_FLAG(sentry_service_max_message_size_bytes, advanced);
: TAG_FLAG(sentry_service_max_message_size_bytes, experimental);
: TAG_FLAG(sentry_service_max_message_size_bytes, runtime);
These only get set once when the AuthzProvider starts up; should the be non-runtime then?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:37:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10: Code-Review+2
Looks good to me; maybe Andrew has some more feedback.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:14:03 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 14: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: u
> Do you have some recommendations on how to test the default values? Also, w
If the retry logic proves to be ineffective or anti-productive, we can add backoff in the future. I think Dan added the defaults based on his "gut feel"; we'll have to see how well the defaults work in practice.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 05 Nov 2018 18:32:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#8).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
13 files changed, 974 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/8
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: 1
> If the retry logic proves to be ineffective or anti-productive, we can add
I agree -- it doesn't make sense to change those defaults unless we have some data to base that decision upon.
In other words, current default values look good enough to me :)
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 05 Nov 2018 19:16:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@13
PS5, Line 13: is placed under
is placed in the
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14
PS5, Line 14: in future
in the future
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@78
PS5, Line 78:
: void TearDown() override {
: SentryTestBase::TearDown();
: }
If that's all this does, you don't need it.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@111
PS4, Line 111: or 'kudu'
: // (Kudu server name) is a non admin user in Sentry
> I think the best way is to add config validation as what we did in HMS inte
I'm not Dan but I wanted to make sure this was chased down. Is the API you're suggesting possible without being an information leak? What authorization does one need in order to learn whether a user is an admin?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187
PS4, Line 187: const string kSchemeSeparator = "://";
> Actually, I am leaning toward the second, since if any configuration is usi
I'm inclined to agree with Hao: reuse the bulk of ParseStrings() to share code, but expose it in a new public method so that callers using the existing ParseStrings won't suddenly start accepting schemes as valid input.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc@56
PS5, Line 56: void SetUp() override {
: SentryTestBase::SetUp();
: }
:
: void TearDown() override {
: SentryTestBase::TearDown();
: }
Here too; if all this is doing is calling the superclass, it can be omitted.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 31 Oct 2018 04:59:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#11).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
14 files changed, 994 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/11
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#5).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed under
master module. The idea is to decouple it from Sentry since in future
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
13 files changed, 1,010 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/5
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#3).
Change subject: [sentry] SentryAuthzProvider
......................................................................
[sentry] SentryAuthzProvider
This commit adds a higher-level API above the Sentry which handles
authorizations on Kudu operations, retries, reconnections, HA, error
handling, and concurrent requests.
A follow-up commit will integrate the SentryAuthzPrivider into the
CatalogManager to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_authz_provider-test.cc
A src/kudu/sentry/sentry_authz_provider.cc
A src/kudu/sentry/sentry_authz_provider.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
7 files changed, 757 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/3
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc@270
PS10, Line 270: ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata(
: "db.table", kTestUser));
: });
> nit: aesthetically, I generally prefer, but feel free to ignore if you like
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 00:42:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 04:17:22 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() { return Status::OK(); }
:
: // Stops the AuthzProvider instance.
: virtual void Stop() {}
> nit: if you have DefaultAuthzProvider with dummy implementation of all auth
Sure, but any specific reasons for doing this?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270
PS5, Line 270:
> That's what we do for wrapping long lines.
Ah, thanks!
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@55
PS7, Line 55: This following methods will fail if the operation is not authorized, or the
: // Sentry service is unreachable, or 'kudu' (Kudu server name) is a non-admin
: // user in Sentry, or Sentry fails to resolve the group mapping of
: // the user.
> How about we split these out so it's a bit easier to read.
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@90
PS7, Line 90: //
: // This method will fail if the Sentry service is unreachable, or 'kudu'
: // (Kudu server name) is a non admin user in Sentry, or Sentry fail to
: // resolve the group mapping of the user.
> nit: not needed given the comment up top?
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259
PS5, Line 259: return Status::OK();
: }
:
> Ah, thanks for the explanation. So based on where the Authorize() fails, a
Yeah, which we should be careful about what to return in the caller of AuthzProvider.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50
PS7, Line 50: sentry.service.client.server.rpc-addresses
> This seems confusing: ...service.client.server.... Is there any typo in th
No... this is the name defined in Sentry.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
> If that's to describe server namespace in Sentry terms, why not 'sentry_ser
Sentry has server level privilege and this is used to configure that. The idea is to define server namespace in Kudu to support authorization on multi clusters. Thus, 'sentry_server_namespace' sounds misleading to me.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@115
PS7, Line 115: on
> super nit: elsewhere we seem to favor capitalizing the whole of "ALL ON DAT
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@209
PS7, Line 209:
> nit: spacing here too
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:58:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 8: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@57
PS8, Line 57: "Configures which server namespace the Kudu instance belongs to for defining"
nit: add space
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@90
PS8, Line 90: timeout
nit for this and other timeout-related flags: it seems we have a convention to reflect time interval units in the name of the flags (e.g., check for various PKI-related parameters in master.cc or consensus-related parameters in raft_consensus.cc/consensus_queue.cc), so I would expect this to be sentry_service_send_timeout_seconds
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@111
PS8, Line 111: sentry_service_max_message_size
the same unit-related nit: I would expect it to be sentry_service_max_message_size_bytes.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 20:49:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 5:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14
PS5, Line 14: future
nit: ...in the future, other...
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h
File src/kudu/master/default_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@32
PS4, Line 32: virtual Status AuthorizeCreateTable(const std::string& table_name,
> Ignored, since those parameters are required in other implementation.
The parameter is required, but the preferred way to add arguments that aren't use is to comment out the unused parameters, eg:
Status AuthorizeCreateTable(const std::string& /*table_name*/, const std::string& /*user*/, ...
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@122
PS5, Line 122: void GetDatabasePrivilege(const string& db_name,
: const string& action,
: bool grant_option,
: TSentryPrivilege* privilege) {
nit: Any reason to not have this return the TSentryPrivilege directly? Same for GetTablePrivilege?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@95
PS5, Line 95: Status CreateRoleAndAddToGroups(const string& role_name, const string& group_name) {
: TCreateSentryRoleRequest role_req;
: role_req.__set_requestorUserName(kAdminUser);
: role_req.__set_roleName(role_name);
: RETURN_NOT_OK(sentry_client_->CreateRole(role_req));
:
: TSentryGroup group;
: group.groupName = group_name;
: set<TSentryGroup> groups;
: groups.insert(group);
: TAlterSentryRoleAddGroupsRequest group_request;
: TAlterSentryRoleAddGroupsResponse group_response;
: group_request.__set_requestorUserName(kAdminUser);
: group_request.__set_roleName(role_name);
: group_request.__set_groups(groups);
: return sentry_client_->AlterRoleAddGroups(group_request, &group_response);
: }
:
: Status AlterRoleGrantPrivilege(const string& role_name, const TSentryPrivilege& privilege) {
: TAlterSentryRoleGrantPrivilegeRequest privilege_request;
: TAlterSentryRoleGrantPrivilegeResponse privilege_response;
: privilege_request.__set_requestorUserName(kAdminUser);
: privilege_request.__set_roleName(role_name);
: privilege_request.__set_privilege(privilege);
: return sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response);
: }
:
: void GetDatabasePrivilege(const string& db_name,
: const string& action,
: bool grant_option,
: TSentryPrivilege* privilege) {
: privilege->__set_privilegeScope("DATABASE");
: privilege->__set_serverName(FLAGS_server_name);
: privilege->__set_dbName(db_name);
: privilege->__set_action(action);
: if (grant_option) {
: privilege->__set_grantOption(TSentryGrantOption::TRUE);
: }
: }
:
: void GetTablePrivilege(const string& db_name,
nit: docs for these?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@140
PS5, Line 140: privilege->__set_privilegeScope("DATABASE");
: privilege->__set_serverName(FLAGS_server_name);
: privilege->__set_dbName(db_name);
: privilege->__set_tableName(table_name);
: privilege->__set_action(action);
: if (grant_option) {
: privilege->__set_grantOption(TSentryGrantOption::TRUE);
: }
nit: maybe
GetDatabasePrivilege(...);
privilege->__set_tableName(table_name);
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@177
PS5, Line 177: false
nit: for boolean params, it's generally easier to read coming back to it as either an enum or by doing something like "/*grant_option=*/false", here and elsewhere
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@181
PS5, Line 181: ,
nit: comma not needed
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270
PS5, Line 270:
nit: spacing
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@288
PS5, Line 288: hostports, vector<HostPort>({ HostPort("foo-bar-baz", 1234) }
Here and below, the expected value should come first.
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@295
PS5, Line 295:
nit: spacing
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@57
PS5, Line 57: virtual Status Start() override;
Probably worth doc'ing this, if its behavior is specific to Sentry
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@62
PS5, Line 62: Sentry service is unreachable, or 'kudu' (Kudu server name) is a non
: // admin user in Sentry, or Sentry fails to resolve the group mapping of
: // the user.
Rather than repeating this in every comment, can you put this in the block comment at the top? It doesn't seem like these requirements vary from method to method.
Also what is "Kudu server name" in this context? Does it correspond to an external constant?
nit: "non-admin" with a hyphen
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@96
PS5, Line 96: FRIEND_TEST(SentryAuthzProviderStaticTest, TestParseAddresses);
Could this use ValidateAddresses instead? How useful are the Statuses provided by ParseAddresses (vs just having the bool)
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@59
PS5, Line 59: Must match the value of the hive.sentry.server option "
: "in the HiveServer2 configuration, and the value of the --server_name "
: "in Impala configuration."
How will this fail if these statements aren't true?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@149
PS5, Line 149: old_table == new_table
Does capitalization matter at this point?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259
PS5, Line 259: // Logs an error if the action is not authorized for debugging purpose, and
: // only returns generic error back to the users to avoid side channel leak,
: // e.g. 'whether table A exists'.
Does the user's request have more information than what's presented in this error message? How would a user be able to determine whether table A exists if this message were returned?
nit: Also maybe reword "Action <action> on authorizable <authorizable> is not permitted" or somesuch? Since the operation itself didn't fail; it's just the user isn't authorized, right?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@262
PS5, Line 262: << "Failed to perform action [" << action.action() << "] on authorizable ["
: << authorizable << "] for user [" << user << "]";
nit: use Substitute
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@264
PS5, Line 264: Substitute
Remove
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@275
PS5, Line 275: authorizable->__set_table(table.ToString());
nit: maybe don't set these until we know we'll return OK? Is there a move constructor for TSentryAuthorizable?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry-test-base.h@76
PS5, Line 76: kerberos_enabled
nit: maybe end with a _?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc@56
PS5, Line 56: void SetUp() override {
: SentryTestBase::SetUp();
: }
:
: void TearDown() override {
: SentryTestBase::TearDown();
: }
Don't need these since they're both the default.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 31 Oct 2018 05:08:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50
PS7, Line 50: sentry.service.client.server.rpc-addresses
> No... this is the name defined in Sentry.
Uh-oh... OK.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
> Sentry has server level privilege and this is used to configure that. The i
Ah, OK. Thank you for clarification.
Could you then add a sentence in the description of this flag to reflect the essence of your explanation above? Something like '... server name of the Kudu cluster used to grant server-level privileges on. Used to distinguish a particular Kudu cluster in case of a multi-cluster setup.'
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 18:12:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10: -Code-Review
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: 1
nit: does the default setting of 1 seem good enough for the production use?
Another question: is it possible to set this to some 'do not care' value but rely purely on the timeout settings instead?
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117
PS10, Line 117: TAG_FLAG(sentry_service_max_message_size_bytes, advanced);
: TAG_FLAG(sentry_service_max_message_size_bytes, experimental);
: TAG_FLAG(sentry_service_max_message_size_bytes, runtime);
> These only get set once when the AuthzProvider starts up; should the be non
+1
Indeed, all these flags for the underlying thrift client do not qualify for runtime ones as it seems from the implementation.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 23:00:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#13).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
14 files changed, 994 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/13
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#4).
Change subject: [sentry] add SentryAuthzProvider
......................................................................
[sentry] add SentryAuthzProvider
This commit adds a higher-level API above Sentry which handles
authorizations on Kudu operations, retries, reconnections, HA, error
handling, and concurrent requests.
A follow-up commit will integrate the SentryAuthzProvider into the
CatalogManager to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
13 files changed, 994 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/4
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Reviewed-on: http://gerrit.cloudera.org:8080/11659
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
14 files changed, 994 insertions(+), 117 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 15
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add SentryAuthzProvider
......................................................................
Patch Set 3:
(38 comments)
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@7
PS3, Line 7: [sentry] SentryAuthzProvider
> Nit: this is a little sparse; maybe convert into a sentence (or sentence fr
Done
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@9
PS3, Line 9: the Sentry
> Nit: "above Sentry"
Done
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@13
PS3, Line 13: SentryAuthzPrivider
> SentryAuthzProvider
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc
File src/kudu/sentry/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72
PS3, Line 72: if (enable_kerberos) {
> I think I've seen this block of code at least 3 times now. Can we consolida
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72
PS3, Line 72: if (enable_kerberos) {
> +1, pull this out into some Sentry test base and use it here and in sentry_
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@109
PS3, Line 109: ASSERT_OK(sentry_client_->Stop());
> Shouldn't we stop the client before the server?
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@121
PS3, Line 121: SentryAuthzProviderTest
> Is it worth adding a few scenarios to verify how the provider works if Sent
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@127
PS3, Line 127: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
> Why is this an error at all? Why not just set `authorized` to false? How do
This is an error returned from Sentry service. I think these errors should return it back to users.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@127
PS3, Line 127: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
> This is surprising; shouldn't it be NotFound() or something like that?
Yeah, it is strange Sentry is returning as AccessDenied error. I filed a jira https://issues.apache.org/jira/browse/SENTRY-2434 to track this.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@135
PS3, Line 135: // Authorize create table on a user without required privileges.
:
> nit: extra line
The extra line is to indicate the comment is for the following blocks.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@146
PS3, Line 146: group_requset
> group_request
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@137
PS3, Line 137: TCreateSentryRoleRequest role_req;
: role_req.__set_requestorUserName("test-admin");
: role_req.__set_roleName("developer");
: ASSERT_OK(sentry_client_->CreateRole(role_req));
:
: TSentryGroup group;
: group.groupName = "user";
: set<TSentryGroup> groups;
: groups.insert(group);
: TAlterSentryRoleAddGroupsRequest group_requset;
: TAlterSentryRoleAddGroupsResponse group_response;
: group_requset.__set_requestorUserName("test-admin");
: group_requset.__set_roleName("developer");
: group_requset.__set_groups(groups);
: ASSERT_OK(sentry_client_->AlterRoleAddGroups(group_requset, &group_response));
> This is copied in every test, so maybe pull it into its own function (or fu
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@204
PS3, Line 204: TSentryPrivilege privilege;
: privilege.__set_privilegeScope("DATABASE");
: privilege.__set_serverName(FLAGS_server_name);
: privilege.__set_dbName("db");
: privilege.__set_action("SELECT");
> nit: We're using these _a lot_ so maybe it's worth introducing PrivilegeFor
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h
File src/kudu/sentry/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@41
PS3, Line 41: the Sentry
> just Sentry, not "the Sentry"
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@60
PS3, Line 60: // Check if the table creation operation on a given table is authorized
> Nit: Start/Stop use present tense, so the rest of these methods should too.
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@61
PS3, Line 61: Return the authorization result in out parameter 'authorized',
: // and only set it on success.
> Why don't we merge 'authorized' directly into the returned Status? Isn't th
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@65
PS3, Line 65: fail
> fails
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@71
PS3, Line 71: creation
> deletion
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@93
PS3, Line 93: // Check if the table alteration with rename operation on a given table is
> It's worth adding here (or above) why rename is split out of the rest of Au
After a second thought, I think it would be better/more clean to combine these two methods together. Updated.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@45
PS3, Line 45: the
> Nit: drop this
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@45
PS3, Line 45: RPC addresses of the Sentry service(s)
> nit: Call out that it's a comma-separated list
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55
PS3, Line 55: server
> Is this a Sentry-specific thing? Hive-specific? Does it make sense to refer
This is sentry-specific, though I don't know what does it mean to call it 'Sentry server namespace'. I will rephrase the sentence to see if I can make it more clear.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55
PS3, Line 55: Configure
> Configures
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@79
PS3, Line 79: SentryAuthzProvider::SentryAuthzProvider() {
: }
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@121
PS3, Line 121: Status SentryAuthzProvider::AuthorizeAlterTable(const string& table_name,
: const string& user,
: bool* authorized)
> nit: consider either setting '*authorized' to 'false' in the beginning or a
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@135
PS3, Line 135: // Table alteration (without table rename) requires:
> But this is with rename...
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@157
PS3, Line 157: Slice database;
: Slice table;
: RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table));
> This isn't necessary for::SERVER.
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@170
PS3, Line 170: return Status::InvalidArgument("invalid authorizable scope");
> Seems like this should CHECK fail, no? The scopes are statically defined in
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@185
PS3, Line 185: privilege
> nit: privileges
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186
PS3, Line 186: implies
> nit: imply (the privileges imply the authorizable, right?)
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186
PS3, Line 186: valid
> validate
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@187
PS3, Line 187: action
> nit: actions
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@202
PS3, Line 202: Imply
> The usage here suggests this should be called "Implies" rather than "Imply"
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@211
PS3, Line 211:
: bool SentryAuthzProvider::ValidateAddresses(const char* flag_name,
: const string& addresses) {
: vector<HostPort> host_ports;
: Status s = ParseAddresses(addresses, &host_ports);
: if (!s.ok()) {
: LOG(ERROR) << "invalid flag " << flag_name << ": " << s.ToString();
: }
: return s.ok();
: }
> If this is only used as a flag validator, it doesn't need to be a part of t
Adding tests for this, so keep it as a class method.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@222
PS3, Line 222: Status SentryAuthzProvider::ParseAddresses
> nit: it would be nice if the order of methods in the .cc file matched the o
Done
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@234
PS3, Line 234: auto scheme_idx = uri.find(kSchemeSeparator, 1);
> If the URL starts with "://", that'll fail in ParseString below?
yeah, it should fail.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218
PS3, Line 218: request.__set_principalName("test-user");
> I'm curious what would be the response if not setting (or clearing/resettin
It will return an 'Invalid Input' error.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@223
PS3, Line 223: alter roles add groups
> We're trying to write this in plain English, so why not "...Sentry service
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 25 Oct 2018 23:46:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() { return Status::OK(); }
:
: // Stops the AuthzProvider instance.
: virtual void Stop() {}
nit: if you have DefaultAuthzProvider with dummy implementation of all authz-related methods, why not to make these two methods pure virtual and add the default implementation for these two methods into DefaultAuthzProvider as well?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147
PS7, Line 147: Authorize
Here and below for negative cases: did you actually mean 'Don't authorize' here?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@238
PS7, Line 238: TEST_P(SentryAuthzProviderTest, TestReconnect) {
What happens if Sentry server is not responsive e.g., due network errors or the server being too busy and the authz provider tries to get authz info from the server? Do you think it's worth adding a small test for that case to ensure the provider behaves appropriately? I think it's pretty easy to simulate a too busy server: just pause it by sending the SIGSTOP signal to the server process.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@51
PS7, Line 51: virtual
nit: here and below drop 'virtual' since that's a derived class and the 'override' is present for the overridden methods.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50
PS7, Line 50: sentry.service.client.server.rpc-addresses
This seems confusing: ...service.client.server.... Is there any typo in the name of the property?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
If that's to describe server namespace in Sentry terms, why not 'sentry_server_namespace? Is that just for compatibility with Impala's --server_name flag?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218
PS3, Line 218: set<TSentryGroup> groups;
> It will return an 'Invalid Input' error.
OK, thanks for the info.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:28:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: u
> I took a look into the thrift client code. Apparently, the approach used t
Do you have some recommendations on how to test the default values? Also, would you mind I address it in a different patch as this is more related to thrift client module.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 05 Nov 2018 17:54:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#9).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
13 files changed, 975 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/9
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed a vote on this change.
Change subject: [sentry] add AuthzProvider
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider
......................................................................
Patch Set 3:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@7
PS3, Line 7: [sentry] SentryAuthzProvider
Nit: this is a little sparse; maybe convert into a sentence (or sentence fragment)?
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@9
PS3, Line 9: the Sentry
Nit: "above Sentry"
http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@13
PS3, Line 13: SentryAuthzPrivider
SentryAuthzProvider
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc
File src/kudu/sentry/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72
PS3, Line 72: if (enable_kerberos) {
I think I've seen this block of code at least 3 times now. Can we consolidate it as a test util somewhere?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@109
PS3, Line 109: ASSERT_OK(sentry_client_->Stop());
Shouldn't we stop the client before the server?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@127
PS3, Line 127: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
This is surprising; shouldn't it be NotFound() or something like that?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@146
PS3, Line 146: group_requset
group_request
Below too
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h
File src/kudu/sentry/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@41
PS3, Line 41: the Sentry
just Sentry, not "the Sentry"
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@60
PS3, Line 60: // Check if the table creation operation on a given table is authorized
Nit: Start/Stop use present tense, so the rest of these methods should too. For example, "Checks if the table creation is authorized. Returns the authorization result..."
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@61
PS3, Line 61: Return the authorization result in out parameter 'authorized',
: // and only set it on success.
Why don't we merge 'authorized' directly into the returned Status? Isn't that what the NotAuthorized status is for?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@65
PS3, Line 65: fail
fails
Below too
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@71
PS3, Line 71: creation
deletion
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@93
PS3, Line 93: // Check if the table alteration with rename operation on a given table is
It's worth adding here (or above) why rename is split out of the rest of AuthorizeAlterTable.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@45
PS3, Line 45: the
Nit: drop this
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55
PS3, Line 55: Configure
Configures
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@135
PS3, Line 135: // Table alteration (without table rename) requires:
But this is with rename...
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@157
PS3, Line 157: Slice database;
: Slice table;
: RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table));
This isn't necessary for::SERVER.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@170
PS3, Line 170: return Status::InvalidArgument("invalid authorizable scope");
Seems like this should CHECK fail, no? The scopes are statically defined in the call sites, so adding an unrecognized scope should be a programming error.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186
PS3, Line 186: valid
validate
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@202
PS3, Line 202: Imply
The usage here suggests this should be called "Implies" rather than "Imply". Sort of like "Equals" and not "Equal"
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@234
PS3, Line 234: auto scheme_idx = uri.find(kSchemeSeparator, 1);
If the URL starts with "://", that'll fail in ParseString below?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@223
PS3, Line 223: alter roles add groups
We're trying to write this in plain English, so why not "...Sentry service to add roles to groups..."?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 22 Oct 2018 21:32:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 5:
(32 comments)
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@7
PS4, Line 7: [sentry] add AuthzProvider
> Could you add a paragraph explaining the module placement choices?
Done
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@10
PS4, Line 10: vider. It has a default implementation
: which always allow any operations
> this is all handled by the existing thrift::HaClient.
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@59
PS4, Line 59: AuthorizeAlterTable
> Should we wrap new_table in a boost::optional, to convey that it isn't nece
I don't think this is necessary. Since the caller, catalog_manager, cannot distinguish rename alters from non-rename ones without the name comparison. It makes more sense to put such comparison inside AuthorizeAlterTable where it results in different authorization logic.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@67
PS4, Line 67: AuthorizeGetTableMetadata
> Where is this intended to be used?
This is used for master RPCs where any privileges is good enough to get a table's metadata, e.g ListTables, GetTabletLocations.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h
File src/kudu/master/default_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@32
PS4, Line 32: virtual Status AuthorizeCreateTable(const std::string& table_name,
> warning: parameter 'table_name' is unused [misc-unused-parameters]
Ignored, since those parameters are required in other implementation.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@33
PS4, Line 33: const std::string& user,
> warning: parameter 'user' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@34
PS4, Line 34: const std::string& owner) override WARN_UNUSED_RESULT {
> warning: parameter 'owner' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@38
PS4, Line 38: virtual Status AuthorizeDropTable(const std::string& table_name,
> warning: parameter 'table_name' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@39
PS4, Line 39: const std::string& user) override WARN_UNUSED_RESULT {
> warning: parameter 'user' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@43
PS4, Line 43: virtual Status AuthorizeAlterTable(const std::string& old_table,
> warning: parameter 'old_table' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@44
PS4, Line 44: const std::string& new_table,
> warning: parameter 'new_table' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@45
PS4, Line 45: const std::string& user) override WARN_UNUSED_RESULT {
> warning: parameter 'user' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@49
PS4, Line 49: virtual Status AuthorizeGetTableMetadata(const std::string& table_name,
> warning: parameter 'table_name' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@50
PS4, Line 50: const std::string& user) override WARN_UNUSED_RESULT {
> warning: parameter 'user' is unused [misc-unused-parameters]
Same as above.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider-test.cc@67
PS4, Line 67: "
> whitespace here and below
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@57
PS4, Line 57: override
> 'override' here and below
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@56
PS4, Line 56:
: virtual Status Start() override;
:
: virtual void Stop() override;
> Nit: lower-case override for new code.
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@88
PS4, Line 88: virtual Status AuthorizeGetTableMetadata(const std::string& table_name,
> Can you remind me what master/catalog manager ops need to call this?
e.g. ListTables, GetTableLocations.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@111
PS4, Line 111: or 'kudu'
: // (Kudu server name) is a non admin user in Sentry
> Is there any way we can recognize this failure scenario as part of Start()?
I think the best way is to add config validation as what we did in HMS integration. However, since Kudu doesn't handle user group mapping, I don't see a way to validate it. Another way I can think of is adding an API in Sentry to check if a user is admin. Any other suggestions?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@110
PS4, Line 110: // then the creating user must have 'ALL on DATABASE' with grant. See
> Interesting - is there a link or somewhere you can reference in Sentry that
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@140
PS4, Line 140: // For table alteration (without table rename) requires 'ALTER ON TABLE'
> Same thing here, I think there's a SENTRY JIRA that explains this distincti
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187
PS4, Line 187: const string kSchemeSeparator = "://";
> We already have HostPort::ParseStrings which does almost the exact same thi
Actually, I am leaning toward the second, since if any configuration is using scheme, this won't work. Though I am thinking to introduce a new method in HostPort to add scheme support instead of adding it to ParseStrings, so that any current callers of ParseStrings won't be affected by this change. Any thoughts?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@244
PS4, Line 244: lege : response.privileges) {
: // A grant option cannot imply the other if the former is set
> might be a bit safer to express this as 'privilege.grantOption != TSentryGr
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@250
PS4, Line 250:
> I'd add a WARN_NOT_OK or similar here so that we get an indication in the l
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256
PS4, Line 256:
> substitute isn't necessary
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256
PS4, Line 256: }
> Is there a security reason to have a generic error message? It may be help
Discussed this offline with Dan. We decided to just use logging to facilitate debugging and only return generic information to users to avoid sidechannel leak such as 'whether table A exists'.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@262
PS4, Line 262: LOG(ERROR) << "Failed to perform action [" << action.action() << "] on authorizable ["
: << authorizable << "] for user [" << user << "]";
: return Status::NotAuthorized(Substitute("unauthorized action"));
> I commented on this before, but this can be scoped to TABLE or DATABASE; it
Oops, sorry that I skipped that.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@267
PS4, Line 267: Status SentryAuthzProvider::GetAuthorizable(const string& table_name,
: AuthorizableScope scope,
: TSentryAuthorizable* authorizable) {
: Slice database;
> Missing break statements here? Or if this is intentional, there's a FALLTHR
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@275
PS4, Line 275: authorizable->__set_table(table.ToString());
> Prefer LOG(FATAL)
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@33
PS4, Line 33: class SentryTestBase : public KuduTest,
> Seems like you could push the Kerberos boolean parameterization down into t
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@67
PS4, Line 67: }
> Nit: indent by one character.
Done
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc@60
PS4, Line 60: void TearDown() override {
> Note that when overriding SetUp()/TearDown() you should make sure to chain
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Oct 2018 07:01:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc
File src/kudu/sentry/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@121
PS3, Line 121: SentryAuthzProviderTest
Is it worth adding a few scenarios to verify how the provider works if Sentry server is unreachable?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@121
PS3, Line 121: Status SentryAuthzProvider::AuthorizeAlterTable(const string& table_name,
: const string& user,
: bool* authorized)
nit: consider either setting '*authorized' to 'false' in the beginning or add WARN_UNUSED_RESULT attribute for those methods; that's to avoid mistakes of using the result '*authorized' if methods like this return not-OK prematurely.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@222
PS3, Line 222: Status SentryAuthzProvider::ParseAddresses
nit: it would be nice if the order of methods in the .cc file matched the order of those in the .h file.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218
PS3, Line 218: request.__set_principalName("test-user");
I'm curious what would be the response if not setting (or clearing/resetting) this field?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 24 Oct 2018 05:13:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@57
PS8, Line 57: DEFINE_string(server_name, "server1",
> nit: add space
Done
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@90
PS8, Line 90:
> nit for this and other timeout-related flags: it seems we have a convention
Done
http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@111
PS8, Line 111:
> the same unit-related nit: I would expect it to be sentry_service_max_messa
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:00:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add SentryAuthzProvider
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add SentryAuthzProvider
......................................................................
Patch Set 4:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@7
PS4, Line 7: [sentry] add SentryAuthzProvider
Could you add a paragraph explaining the module placement choices?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@59
PS4, Line 59: AuthorizeAlterTable
Should we wrap new_table in a boost::optional, to convey that it isn't necessary for non-rename alters?
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@56
PS4, Line 56:
: virtual Status Start() OVERRIDE;
:
: virtual void Stop() OVERRIDE;
Nit: lower-case override for new code.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@262
PS4, Line 262: Slice database;
: Slice table;
: RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table));
I commented on this before, but this can be scoped to TABLE or DATABASE; it's not necessary for SERVER.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@267
PS4, Line 267: case AuthorizableScope::TABLE:
: authorizable->__set_table(table.ToString());
: case AuthorizableScope::DATABASE:
: authorizable->__set_db(database.ToString());
Missing break statements here? Or if this is intentional, there's a FALLTHROUGH_INTENDED macro.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h
File src/kudu/sentry/sentry-test-base.h:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@33
PS4, Line 33: class SentryTestBase : public KuduTest {
Seems like you could push the Kerberos boolean parameterization down into this fixture, as well as the SetUp() and TearDown() methods.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@67
PS4, Line 67: protected:
Nit: indent by one character.
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc@60
PS4, Line 60: void SetUp() override {
Note that when overriding SetUp()/TearDown() you should make sure to chain to the superclass implementations. For SetUp() it should be the first thing you do; for TearDown(), the last.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 00:19:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] wip: [sentry] add SentryAuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: wip: [sentry] add SentryAuthzProvider
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider-test.cc
File src/kudu/sentry/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider-test.cc@27
PS1, Line 27: #include "kudu/sentry/mini_sentry.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider-test.cc@36
PS1, Line 36: using ::sentry::TAlterSentryRoleAddGroupsRequest;
> warning: using decl 'SaslProtection' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.h
File src/kudu/sentry/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.h@32
PS1, Line 32:
> update doc
Done
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.h@51
PS1, Line 51:
> warning: function 'kudu::sentry::SentryAuthzProvider::AuthorizeCreateTable'
Done
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.cc@31
PS1, Line 31: using sentry::TListSentryPrivilegesRequest;
> warning: using decl 'optional' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.cc@60
PS1, Line 60:
> Sentry
Done
http://gerrit.cloudera.org:8080/#/c/11659/1/src/kudu/sentry/sentry_authz_provider.cc@119
PS1, Line 119: }
> warning: loop variable is copied but only used as const reference; consider
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 18 Oct 2018 06:41:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11659
to look at the new patch set (#6).
Change subject: [sentry] add AuthzProvider
......................................................................
[sentry] add AuthzProvider
This commit adds a high-level abstraction which handles authorizations
on Kudu operations, called AuthzProvider. It has a default implementation
which always allow any operations for any users, and a SentryAuthzProvider
implementation which connects to the Sentry service for authorization
metadata. AuthzProvider, along with its implementations, is placed in the
master module. The idea is to decouple it from Sentry since in the future,
other authorization implementations might be introduced.
A follow-up commit will integrate the AuthzProvider into CatalogManager
to perform authorization checks on Master RPCs.
Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/authz_provider.h
A src/kudu/master/default_authz_provider.h
A src/kudu/master/sentry_authz_provider-test.cc
A src/kudu/master/sentry_authz_provider.cc
A src/kudu/master/sentry_authz_provider.h
A src/kudu/sentry/sentry-test-base.h
M src/kudu/sentry/sentry_action-test.cc
M src/kudu/sentry/sentry_action.cc
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_client-test.cc
M src/kudu/sentry/sentry_client.cc
M src/kudu/sentry/sentry_client.h
13 files changed, 910 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/6
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84
PS10, Line 84: 1
> I think one time retry is general enough to avoid to end the connection too
I mean look how Kudu RPC works -- you don't specify number of retries, you specify just the timeout. The client retries the call with exponential back-off (where the max wait is limited at about 2 seconds), and the timeout is the only parameter that controls those retries.
Also, those timeouts -- are those timeouts per every retry or just overall including all retries? If the latter, how do you control which parameter actually controls the retry behavior: the overall timeout or maximum number of retries?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 17:15:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] SentryAuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider
......................................................................
Patch Set 3:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc
File src/kudu/sentry/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72
PS3, Line 72: if (enable_kerberos) {
> I think I've seen this block of code at least 3 times now. Can we consolida
+1, pull this out into some Sentry test base and use it here and in sentry_client-test, which has almost identical methods.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@127
PS3, Line 127: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
> This is surprising; shouldn't it be NotFound() or something like that?
Why is this an error at all? Why not just set `authorized` to false? How does the error make its way back to the user?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@135
PS3, Line 135: // Authorize create table on a user without required privileges.
:
nit: extra line
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@137
PS3, Line 137: TCreateSentryRoleRequest role_req;
: role_req.__set_requestorUserName("test-admin");
: role_req.__set_roleName("developer");
: ASSERT_OK(sentry_client_->CreateRole(role_req));
:
: TSentryGroup group;
: group.groupName = "user";
: set<TSentryGroup> groups;
: groups.insert(group);
: TAlterSentryRoleAddGroupsRequest group_requset;
: TAlterSentryRoleAddGroupsResponse group_response;
: group_requset.__set_requestorUserName("test-admin");
: group_requset.__set_roleName("developer");
: group_requset.__set_groups(groups);
: ASSERT_OK(sentry_client_->AlterRoleAddGroups(group_requset, &group_response));
This is copied in every test, so maybe pull it into its own function (or functions, if there's a clean division of functionality).
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@204
PS3, Line 204: TSentryPrivilege privilege;
: privilege.__set_privilegeScope("DATABASE");
: privilege.__set_serverName(FLAGS_server_name);
: privilege.__set_dbName("db");
: privilege.__set_action("SELECT");
nit: We're using these _a lot_ so maybe it's worth introducing PrivilegeForDb() and PrivilegeForTable() methods?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@45
PS3, Line 45: RPC addresses of the Sentry service(s)
nit: Call out that it's a comma-separated list
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55
PS3, Line 55: server
Is this a Sentry-specific thing? Hive-specific? Does it make sense to refer to this as "Sentry server namespace" or "Hive server namespace" instead?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@79
PS3, Line 79: SentryAuthzProvider::SentryAuthzProvider() {
: }
Remove
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@185
PS3, Line 185: privilege
nit: privileges
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186
PS3, Line 186: implies
nit: imply (the privileges imply the authorizable, right?)
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@187
PS3, Line 187: action
nit: actions
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@211
PS3, Line 211:
: bool SentryAuthzProvider::ValidateAddresses(const char* flag_name,
: const string& addresses) {
: vector<HostPort> host_ports;
: Status s = ParseAddresses(addresses, &host_ports);
: if (!s.ok()) {
: LOG(ERROR) << "invalid flag " << flag_name << ": " << s.ToString();
: }
: return s.ok();
: }
If this is only used as a flag validator, it doesn't need to be a part of this class at all, right? Could just be a regular static method, or put in an anonymous namespace.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 24 Oct 2018 06:10:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11659/6/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/6/src/kudu/master/sentry_authz_provider-test.cc@52
PS6, Line 52: namespace kudu {
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/11659/6/src/kudu/master/sentry_authz_provider-test.cc@56
PS6, Line 56: namespace master {
> warning: using decl 'SentryClient' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/11659/6/src/kudu/master/sentry_authz_provider-test.cc@122
PS6, Line 122: privilege.__set_serverName(FLAGS_server_name);
> warning: parameter 'grant_option' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/11659/6/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/6/src/kudu/master/sentry_authz_provider.h@59
PS6, Line 59: // TODO(hao): add early failure recognition when SENTRY-2440 is done.
> warning: missing username/bug in TODO [google-readability-todo]
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 05:11:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.cc@246
PS11, Line 246:
: // Validates the sentry_service_rpc_addresses gflag.
: bool SentryAuthzProvider::ValidateAddresses(const char* flag_name,
: const string& addresses) {
: vector<HostPort> host_ports;
: Status s = HostPort::ParseStringsWithScheme(addresses, SentryClient::kDefaultSentryPort,
: &host_ports);
: if (!s.ok()) {
: LOG(ERROR) << "invalid flag " << flag_name << ": " << s.ToString();
: }
: return s.ok();
: }
Now that this is tested elsewhere, it doesn't need to be part of the class. Stick it in an anonymous namespace?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 04:48:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 11: Code-Review+1
(2 comments)
Sorry, a couple small nits and LGTM.
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/authz_provider.h@63
PS11, Line 63: // Checks if retrieving table's metadata is authorized for the given user.
nit: "metadata" seems like it might be a Sentry construct, which is fine, but I think it'd help to flesh this out a bit more. IMO it's not obvious from just reading this how and where this should be used. WDYT? Something like:
// Checks if retrieving metadata about the table is authorized for the given user.
// E.g. when checking for table presence or locations, checking the status of DDL operations, etc.
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.h@39
PS11, Line 39:
: // An implementation of AuthzProvider, which connects to the Sentry Service
: // for authorization metadata and allow or deny the actions performed by
: // users based on the metadata.
nit: "An implementation of AuthzProvider that connects to Apache Sentry for authorization metadata and allows or denies actions performed by users based on the metadata."
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 02:09:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() { return Status::OK(); }
:
: // Stops the AuthzProvider instance.
: virtual void Stop() {}
> Sure, but any specific reasons for doing this?
The same reason as for having the AuthorizeXxx() method as pure virtual in this class: make sure particular provider implements those methods. Or Start()/Stop() are not important in that sense at all?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 18:15:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 5: Verified+1
Unrelated flaky test MultiThreadedHybridClockTabletTest/5.UpdateNoMergeCompaction.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Oct 2018 18:01:00 +0000
Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/authz_provider.h@63
PS11, Line 63: // Checks if retrieving metadata about the table is authorized for the
> nit: "metadata" seems like it might be a Sentry construct, which is fine, b
Done
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.h@39
PS11, Line 39:
: // An implementation of AuthzProvider that connects to the Sentry Service
: // for authorization metadata and allow or deny the actions performed by
: // users based on the metadata.
> nit: "An implementation of AuthzProvider that connects to Apache Sentry for
I would prefer to not use Apache Sentry here in order to match how we refer 'Sentry' in other places.
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.cc@246
PS11, Line 246: return Authorize(db_authorizable, db_action, user);
: }
:
: Status SentryAuthzProvider::AuthorizeGetTableMetadata(const std::string& table_name,
: const std::string& user) {
: // Retrieving table metadata requires 'METADATA ON TABLE' privilege.
: TSentryAuthorizable authorizable;
: RETURN_NOT_OK(GetAuthorizable(table_name, AuthorizableScope::TABLE, &authorizable));
: SentryAction action = SentryAction(SentryAction::Action::METADATA);
: return Authorize(authorizable, action, user);
: }
:
> Now that this is tested elsewhere, it doesn't need to be part of the class.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 16:24:07 +0000
Gerrit-HasComments: Yes