[kudu-CR] wip sentry: generate authz tokens

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12897 )

Change subject: wip sentry: generate authz tokens
......................................................................


Patch Set 1:

(7 comments)

left a few comments skimming through

http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/gutil/map-util.h@841
PS1, Line 841: std::move(entry.second)
Interesting, I didn't not expect it would allow to use std::move(entry.second) while entry is a const references.


http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/catalog_manager.cc@2752
PS1, Line 2752: token_signer && user
Is it valid to have situations when  { token_signer != nullptr, user == boost::none } or { token_signed == nullptr, user != boost::none } ?


http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:

http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@43
PS1, Line 43: struct SentryPrivilege {
Add a few lines of comments explaining why this structure is useful.


http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@47
PS1, Line 47:                   boost::optional<std::string> column)
nit: for convenience, maybe make last two parameters boost::none by default?


http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@52
PS1, Line 52: #ifndef NDEBUG
What if set 'scope' to DATABASE and specify db, table, and column as well?  Will it be a valid structure?


http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@70
PS1, Line 70: sentry::SentryAuthorizableScope::Scope scope;
Is it necessary to store the scope once the structure is constructed?  If it's really so, why not to make it const?


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

http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/sentry/sentry_action.h@a50
PS1, Line 50: 
Instead, is it possible to update FixedBitSet to work with enums like this as well?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I052432408045c48f6fe9bf921fd3cb6bcc36e9ad
Gerrit-Change-Number: 12897
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 01 Apr 2019 22:46:29 +0000
Gerrit-HasComments: Yes