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:23 UTC

[kudu-CR] [sentry] SentryPrivilege

Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11656


Change subject: [sentry] SentryPrivilege
......................................................................

[sentry] SentryPrivilege

This commit adds SentryPrivilege class to represent a Sentry privilge
in HiveSQL authorizable model. This class provides validation on whether
a privilege can imply another, which allows a higher-level authorization
provider to determine if an operation on a object should be allowed.

Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_privilege-test.cc
A src/kudu/sentry/sentry_privilege.cc
A src/kudu/sentry/sentry_privilege.h
4 files changed, 541 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] [sentry] SentryAction

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

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: [sentry] SentryAction
......................................................................

[sentry] SentryAction

This commit adds SentryAction class to represent a Sentry action in
HiveSQL authorizable model. This class provides validation on whether
an action can imply another, which allows a higher-level authorization
provider to determine if an operation on a object should be allowed.

Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_action-test.cc
A src/kudu/sentry/sentry_action.cc
A src/kudu/sentry/sentry_action.h
4 files changed, 270 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11656/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11656/3/src/kudu/sentry/sentry_action-test.cc
File src/kudu/sentry/sentry_action-test.cc:

http://gerrit.cloudera.org:8080/#/c/11656/3/src/kudu/sentry/sentry_action-test.cc@95
PS3, Line 95:   ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
> Thanks for capturing this. Removed the default constructor to avoid undefin
By default, default constructor is just empty, so the UB is still the same without explicitly declaring the default constructor. To avoid it, perhaps add an UNINITIALIZED or INVALID SentryAction enum and have it be initialized to that in the default constructor.

See "Implicitly-defined default constructor" here: https://en.cppreference.com/w/cpp/language/default_constructor



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 17:53:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 6: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@79
PS6, Line 79: Action action_;
> Isn't it mutated in FromString?
Ah my bad, you're right.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:55:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 7:

(9 comments)

Addressed the comments in https://gerrit.cloudera.org/#/c/11720/.

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@27
PS6, Line 27: replication
> The word 'replication' is a bit overloaded considering its meaning for Kudu
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@28
PS6, Line 28: In this case, HiveSQL model is chosen
            : // to define the actions.
> For those of us unfamiliar with HiveSQL, could you expand on the significan
Done, though I assume you are asking about HiveSQL privilege model which is this class is partly about instead of Hive Query Language.


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@32
PS6, Line 32: // This class is not thread-safe.
> I mean, sure, but it's got so little state that I don't think this warning 
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@40
PS6, Line 40: the
> Nit: drop 'the'
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@46
PS6, Line 46:     UNINITIALIZED,
> It's not clear from this patch alone, but why is it important to allow Sent
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@70
PS6, Line 70: Check if an action implies the other
> Nit: Check if this action implies 'other'.
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc
File src/kudu/sentry/sentry_action.cc:

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@77
PS6, Line 77: Any
> Every
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@78
PS6, Line 78:   CHECK(action() != Action::UNINITIALIZED);
            :   CHECK(other.action() != Action::UNINITIALIZED);
> Use CHECK_NE here.
Done


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@87
PS6, Line 87:   // Any action subsumes Action METADATA
> Nit: terminate with period.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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, 18 Oct 2018 05:48:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11656/3/src/kudu/sentry/sentry_action-test.cc
File src/kudu/sentry/sentry_action-test.cc:

http://gerrit.cloudera.org:8080/#/c/11656/3/src/kudu/sentry/sentry_action-test.cc@95
PS3, Line 95: 
> By default, default constructor is just empty, so the UB is still the same 
Makes sense. Updated.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 19:28:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action-test.cc
File src/kudu/sentry/sentry_action-test.cc:

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action-test.cc@53
PS5, Line 53: 
> This would be simpler as a vector<SentryAction>, since it could use an inti
Done


http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.h@76
PS5, Line 76:   bool Imply(const SentryAction& other) const;
> Can this be const?
Done


http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.cc
File src/kudu/sentry/sentry_action.cc:

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.cc@77
PS5, Line 77:   // Any action must be initialized.
> Maybe this should CHECK fail?  It should be a bug if an uninitialized actio
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:33:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )

Change subject: [sentry] SentryAction
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action-test.cc
File src/kudu/sentry/sentry_action-test.cc:

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action-test.cc@53
PS5, Line 53:   unordered_set<SentryAction*> actions;
This would be simpler as a vector<SentryAction>, since it could use an intializer list to populate instead of having to create and insert the pointers.  You could do the METADATA loop first, then add metadata and do theother loop.


http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.h@76
PS5, Line 76:   bool Imply(const SentryAction& other);
Can this be const?


http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.cc
File src/kudu/sentry/sentry_action.cc:

http://gerrit.cloudera.org:8080/#/c/11656/5/src/kudu/sentry/sentry_action.cc@77
PS5, Line 77:   if (action() == Action::UNINITIALIZED) {
Maybe this should CHECK fail?  It should be a bug if an uninitialized action is used, right?

In either case it should probably check both this and other.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:42:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 7:

(9 comments)

Missed this one too.

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@27
PS6, Line 27: replication
The word 'replication' is a bit overloaded considering its meaning for Kudu. Perhaps "carbon copy"? Or "C++ implementation of the SentryAction Java class" (if that's what it is)?


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@28
PS6, Line 28: In this case, HiveSQL model is chosen
            : // to define the actions.
For those of us unfamiliar with HiveSQL, could you expand on the significance of this and provide a link for more information?


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@32
PS6, Line 32: // This class is not thread-safe.
I mean, sure, but it's got so little state that I don't think this warning is that interesting. You could even make it thread-safe with a minimum of fuss, by converting FromString into a static factory method and removing the default constructor. Then action_ will be immutable.


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@40
PS6, Line 40: the
Nit: drop 'the'


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@46
PS6, Line 46:     UNINITIALIZED,
It's not clear from this patch alone, but why is it important to allow SentryAction to be uninitialized? If you convert FromString() into a static factory method and remove the default constructor, it won't be possible to create a SentryAction without a valid Action.


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.h@70
PS6, Line 70: Check if an action implies the other
Nit: Check if this action implies 'other'.


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc
File src/kudu/sentry/sentry_action.cc:

http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@77
PS6, Line 77: Any
Every


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@78
PS6, Line 78:   CHECK(action() != Action::UNINITIALIZED);
            :   CHECK(other.action() != Action::UNINITIALIZED);
Use CHECK_NE here.


http://gerrit.cloudera.org:8080/#/c/11656/6/src/kudu/sentry/sentry_action.cc@87
PS6, Line 87:   // Any action subsumes Action METADATA
Nit: terminate with period.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 17 Oct 2018 03:41:10 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

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

Change subject: [sentry] SentryAction
......................................................................


Patch Set 1:

(14 comments)

Discussed with Dan offline, and we agreed that it is sufficient to only add SentryAction for checking if an action can imply another since Sentry provides API to query privileges filtered by authorizables.  Updated this patch and removed SentryPrivilege.

http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege-test.cc
File src/kudu/sentry/sentry_privilege-test.cc:

http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege-test.cc@65
PS1, Line 65: db_select.set_action(SentryPrivilege::SELECT);
            :   ASSERT_TRUE(db_all.Imply(db_select));
            : 
            :   SentryPrivilege db_insert = db_all;
            :   db_insert.set_action(SentryPrivilege::INSERT);
            :   ASSERT_TRUE(db_all.Imply(db_insert));
            : 
            :   SentryPrivilege db_update = db_all;
            :   db_update.set_action(SentryPrivilege::UPDATE);
            :   ASSERT_TRUE(db_all.Imply(db_update));
            : 
            :   SentryPrivilege db_delete = db_all;
            :   db_delete.set_action(SentryPrivilege::DELETE);
            :   ASSERT_TRUE(db_all.Imply(db_delete));
            : 
            :   SentryPrivilege db_alter = db_all;
            :   db_alter.set_action(SentryPrivilege::ALTER);
            :   ASSERT_TRUE(db_all.Imply(db_alter));
            : 
            :   SentryPrivilege db_create = db_all;
            :   db_create.set_action(SentryPrivilege::CREATE);
            :   ASSERT_TRUE(db_all.Imply(db_create));
            : 
            :   SentryPrivilege db_drop = db_all;
            :   db_drop.set_action(SentryPrivilege::DROP);
            :   ASSERT_TRUE(db_all.Imply(db_drop));
            : 
            :   SentryPrivilege db_owner = db_all;
            :   db_owner.set_action(SentryPrivilege::OWNER);
            :   ASSERT_TRUE(db_all.Imply(db_owner));
            : 
            :   SentryPrivilege db_metadata = db_all;
            :   db_metadata.set_action(SentryPrivilege::METADATA);
            :   ASSERT_TRUE(db_all.Imply(db_metadata));
> Mind writing this as a loop over all the Action types? Also the OWNER and M
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege-test.cc@147
PS1, Line 147: SentryPrivilege column_insert(table_all);
             :   column_insert.set_column(kColumn);
             :   column_insert.set_action(SentryPrivilege::INSERT);
> May be worth investing in a constructor to which we can set the action and 
Removed.


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h
File src/kudu/sentry/sentry_privilege.h:

http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@32
PS1, Line 32: Privilege
> CommonPrivilege?
Removed.


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@79
PS1, Line 79: SentryPrivilege
> const ref
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.h@100
PS1, Line 100:   void set_table(std::string table) {
> Should this also check that `column_` is empty? Same for `set_database()` a
Not really as the authorizable is in a linear hierarchy, but removing this logic anyway.


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc
File src/kudu/sentry/sentry_privilege.cc:

http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@39
PS1, Line 39:  &
> nit: string&
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@43
PS1, Line 43: "ALL"
> kWildCardAll
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@44
PS1, Line 44: kWildCardAll
> kWildCard? Might be a missing test case somewhere
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@71
PS1, Line 71: const Action&
> Since this is an enum, we don't need the const ref.
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@76
PS1, Line 76: database_(""),
            :       table_(""),
            :       column_("")
> These aren't needed.
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@95
PS1, Line 95:     CHECK(!database_.empty());
            :     CHECK(!table_.empty());
> What is enforcing these? Is it possible that we're given a sentry privilege
Done


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@102
PS1, Line 102: this == &other
> Do we ever expect to compare a SentryPrivilege object with its own pointer?
I don't think we will normally do that, just adding the logic in case this ever happen.


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@111
PS1, Line 111: 
             :   if (!ImplyAuthorizable(server(), other.server())) {
             :     return false;
             :   }
             : 
             :   if (!ImplyAction(action(), other.action())) {
             :     return false;
             :   }
             : 
             :   if (!database().empty() &&
             :       !ImplyAuthorizable(database(), other.database())) {
             :     return false;
             :   }
             : 
             :   if (!table().empty() &&
             :       !ImplyAuthorizable(table(), other.table())) {
             :     return false;
             :   }
             : 
             :   if (!column().empty() &&
             :       !ImplyAuthorizable(column(), other.column())) {
             :     return false;
             :   }
> It's probably worth commenting why this is ordered this way. Also maybe put
Removed.


http://gerrit.cloudera.org:8080/#/c/11656/1/src/kudu/sentry/sentry_privilege.cc@144
PS1, Line 144: boost::iequals(required_authorizable, kWildCardSome)
> kWildCardSome is already case insensitive, no? Just use ==?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Comment-Date: Sun, 14 Oct 2018 00:39:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] [sentry] SentryAction

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

Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11656 )

Change subject: [sentry] SentryAction
......................................................................


Patch Set 6: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:51:49 +0000
Gerrit-HasComments: No

[kudu-CR] [sentry] SentryAction

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

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: [sentry] SentryAction
......................................................................

[sentry] SentryAction

This commit adds SentryAction class to represent a Sentry action in
HiveSQL authorizable model. This class provides validation on whether
an action can imply another, which allows a higher-level authorization
provider to determine if an operation on a object should be allowed.

Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_action-test.cc
A src/kudu/sentry/sentry_action.cc
A src/kudu/sentry/sentry_action.h
4 files changed, 284 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11656/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@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
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [sentry] SentryAction

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

Hello Dan Burkert, Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: [sentry] SentryAction
......................................................................

[sentry] SentryAction

This commit adds SentryAction class to represent a Sentry action in
HiveSQL authorizable model. This class provides validation on whether
an action can imply another, which allows a higher-level authorization
provider to determine if an operation on a object should be allowed.

Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
---
M src/kudu/sentry/CMakeLists.txt
A src/kudu/sentry/sentry_action-test.cc
A src/kudu/sentry/sentry_action.cc
A src/kudu/sentry/sentry_action.h
4 files changed, 273 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11656/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib2e60b79a60fd791ec966f6271c676323bf74d49
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins