You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/03/30 06:01:29 UTC
[kudu-CR] wip sentry: generate authz tokens
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12897
Change subject: wip sentry: generate authz tokens
......................................................................
wip sentry: generate authz tokens
wip:
- not completely satisfied with the API
- break utility changes into separate patches?
- tests
Change-Id: I052432408045c48f6fe9bf921fd3cb6bcc36e9ad
---
M src/kudu/gutil/map-util.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/sentry/sentry_action.h
M src/kudu/sentry/sentry_authorizable_scope.h
9 files changed, 291 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/12897/1
--
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: newchange
Gerrit-Change-Id: I052432408045c48f6fe9bf921fd3cb6bcc36e9ad
Gerrit-Change-Number: 12897
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] wip sentry: generate authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/12897 )
Change subject: wip sentry: generate authz tokens
......................................................................
Abandoned
This is split into multiple other patches.
--
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: abandon
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] wip sentry: generate authz tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12897 )
Change subject: wip sentry: generate authz tokens
......................................................................
Patch Set 1:
(5 comments)
I need to rebase this on top of https://gerrit.cloudera.org/c/12919/
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 == boo
token_signer != nullptr, user == boost::none: user==none AFAIK is a test-only scenario.
token_signer == nullptr, user != boost::none: non-standard, but this would be the case if a user were to set the --master_supports_authz_tokens=false.
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.
Done
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
Changed this to not use boost at all. With sufficient checking up front, I don't think this will complicate the mental model for this struct, and it avoids overhead of boost::optional.
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?
I don't think so, it's not one we expect anyway.
E.g. say we have
INSERT ON DATABASE for db->table->col
What should we do with this? Treat it as INSERT ON DATABASE for db? Treat it as INSERT ON COL for db->table->col? Both of these would seem incorrect, so it's probably safest to just ignore it.
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 i
Not necessary, but convenient. Done
--
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 03 Apr 2019 21:08:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] wip sentry: generate authz tokens
Posted by "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