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 <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@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
