Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@84 PS10, Line 84: static const char* const kAdminGroup; : static const char* const kAdminUser; : static const char* const kUserGroup; : static const char* const kTestUser; : static const char* const kDevRole; : static const char* const kAdminRole; : static const char* const kDatabaseName; : static const char* const kTableName; : static const char* const kSecondTable; Why not define these here? http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@211 PS10, Line 211: Status GetTabletLocations(const string& table, nit: spacing http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@326 PS10, Line 326: typedef function<Status(SentryITestBase*, const string&, const string&)> AuthzFunc; : typedef AuthzFunc OperatorFunc; : typedef AuthzFunc PrivilegeFunc; nit: it's nice that these share the same signature, but given the semantics of each are different, may be worth dropping AuthzFunc entirely and writing out the typedef explicitly for each, and then documenting the expected inputs of each individually. http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@326 PS10, Line 326: typedef function<Status(SentryITestBase*, const string&, const string&)> AuthzFunc; : typedef AuthzFunc OperatorFunc; : typedef AuthzFunc PrivilegeFunc; : : // A description of the operation function as well as the privilege granting function : // for an authorization process. : struct AuthzFuncs { : OperatorFunc op_func; : PrivilegeFunc priv_func; : }; Could you add a comment and maybe an example of what it means to have an op_func and priv_func? Also document what the inputs of AuthzFunc should be. Nits: Also since you're using these directly as functions, maybe name them to be more action-focused, e.g: OperatorFunc do_action; PrivilegeFunc grant_privileges; so when you use them, it would look like: desc.funcs.do_action(...); desc.funcs.grant_privileges(...); Also what's the rational behind having a separate AuthzFuncs struct? Why not just stuff both into the AuthzDescriptor? Then it'd be even easier to compose new tests, e.g.: desc.do_action(...); desc.grant_privileges(...); http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@390 PS10, Line 390: uncomment nit: enable http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@426 PS10, Line 426: const auto new_table_name = Substitute("$0.$1", desc.database, desc.new_table_name); Doesn't this still produce a valid name if desc.new_table_name were empty? Should be have new_table_name be optional? Ah, I see; many of the functions ignore the second field. http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@545 PS10, Line 545: operating a nit: operating on a Same elsewhere http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@557 PS10, Line 557: without with http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/sentry/mini_sentry.cc@209 PS10, Line 209: Configures Sentry to enable owner privileges feature which automatically : // derives OWNER/ALL privileges from object's ownership What does this mean for Kudu? Why is this necessary? -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[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: Sat, 09 Mar 2019 06:20:04 +0000 Gerrit-HasComments: Yes
