Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider ...................................................................... Patch Set 3: (22 comments) http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@7 PS3, Line 7: [sentry] SentryAuthzProvider Nit: this is a little sparse; maybe convert into a sentence (or sentence fragment)? http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@9 PS3, Line 9: the Sentry Nit: "above Sentry" http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@13 PS3, Line 13: SentryAuthzPrivider SentryAuthzProvider http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc File src/kudu/sentry/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72 PS3, Line 72: if (enable_kerberos) { I think I've seen this block of code at least 3 times now. Can we consolidate it as a test util somewhere? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@109 PS3, Line 109: ASSERT_OK(sentry_client_->Stop()); Shouldn't we stop the client before the server? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@127 PS3, Line 127: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); This is surprising; shouldn't it be NotFound() or something like that? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@146 PS3, Line 146: group_requset group_request Below too http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h File src/kudu/sentry/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@41 PS3, Line 41: the Sentry just Sentry, not "the Sentry" http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@60 PS3, Line 60: // Check if the table creation operation on a given table is authorized Nit: Start/Stop use present tense, so the rest of these methods should too. For example, "Checks if the table creation is authorized. Returns the authorization result..." http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@61 PS3, Line 61: Return the authorization result in out parameter 'authorized', : // and only set it on success. Why don't we merge 'authorized' directly into the returned Status? Isn't that what the NotAuthorized status is for? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@65 PS3, Line 65: fail fails Below too http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@71 PS3, Line 71: creation deletion http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@93 PS3, Line 93: // Check if the table alteration with rename operation on a given table is It's worth adding here (or above) why rename is split out of the rest of AuthorizeAlterTable. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc File src/kudu/sentry/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@45 PS3, Line 45: the Nit: drop this http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55 PS3, Line 55: Configure Configures http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@135 PS3, Line 135: // Table alteration (without table rename) requires: But this is with rename... http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@157 PS3, Line 157: Slice database; : Slice table; : RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table)); This isn't necessary for::SERVER. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@170 PS3, Line 170: return Status::InvalidArgument("invalid authorizable scope"); Seems like this should CHECK fail, no? The scopes are statically defined in the call sites, so adding an unrecognized scope should be a programming error. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186 PS3, Line 186: valid validate http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@202 PS3, Line 202: Imply The usage here suggests this should be called "Implies" rather than "Imply". Sort of like "Equals" and not "Equal" http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@234 PS3, Line 234: auto scheme_idx = uri.find(kSchemeSeparator, 1); If the URL starts with "://", that'll fail in ParseString below? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@223 PS3, Line 223: alter roles add groups We're trying to write this in plain English, so why not "...Sentry service to add roles to groups..."? -- 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: 3 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 22 Oct 2018 21:32:32 +0000 Gerrit-HasComments: Yes
