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 <aser...@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
