Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 10: (19 comments) http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/hms_itest-base.h@36 PS10, Line 36: // Create a database in the HMS catalog. > Nit: "Creates" vs. "Create" (see how CheckTable and CheckTableDoesNotExist Done http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_hms-itest.cc@128 PS10, Line 128: NO_FATALS(CheckTable(hms_database_name, hms_table_name, /* user = */ none)); > Nit: as before, should be /*user=*/ and not /* user = */ Done 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? These are static data members. 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 Done 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 Done 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 TestAuthzErrorHandling only needs OperatorFunc and PrivilegeFunc, as TestAuthzTable requires other information, so I separated them as two struct. http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@390 PS10, Line 390: uncomment > nit: enable Done 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? Right, new_table is ignored if not used. 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 Done http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/integration-tests/master_sentry-itest.cc@557 PS10, Line 557: without > with Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc File src/kudu/integration-tests/master_sentry_advance-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@31 PS7, Line 31: > Sounds like we have a header without an "include once" guard. Can you post Yeah, I think it is the missing of "include once" guard in 'hms_itest-base.h. Once I added that in the later patch, the error was gone. http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/master/catalog_manager.h@56 PS10, Line 56: class AuthzTokenTest_TestSingleMasterUnavailable_Test;; > Nit: there's an extra semicolon here. Done http://gerrit.cloudera.org:8080/#/c/11797/9/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/9/src/kudu/master/catalog_manager.h@861 PS9, Line 861: // The authorization step happens after table locking to ensure the validation > This doesn't seem quite right; how can Alice rename B to A if Alice just dr Done http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1798 PS3, Line 1798: auto authorized_error = [&] { > Generally speaking we trust the table ID more than its name, because the na Makes sense, updated. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1713 PS7, Line 1713: optional<const string&> user = rpc ? : make_optional<const string&>(rpc->remote_user().username()) : none; > One option might be to pass around the optional 'user' instead of 'rpc', si Right AFAIU, functions of master_service.cc should not have null RpcContexts and should have remote users Yeah, I don't see a reason to must use 'rpc' instead of the optional 'user' (The only reason I see is to be consistent with other method call, but it is not a must-do ). http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1794 PS7, Line 1794: "the table does not exist", SecureShortDebugString(table_identifier)), > Not done? 'l' is still in scope when authorized_error() is called, which ca Ah, somehow I misunderstood your suggestion. Updated. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@2744 PS7, Line 2744: shared_lock<LockType> l(lock_); : for (const TableInfoMap::value_type &entry : normalized_table_n > That could work, but it's somewhat complicated and will make reasoning abou We can probably do a bulk version of current listing sentry privilege, however it is not a bulk authorization API which return a set of Yes or No but a collection of matching privileges for each input authorizable. Adding a TODO here. http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/10/src/kudu/master/catalog_manager.cc@1816 PS10, Line 1816: NormalizeTableName(table_identifier.table_name()) > Can reuse the local variable table_name here. Done 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? This means the user who creates the table in Kudu will automatically get OWNER/ALL privilege without having the admin to explicitly grant the permissions to the user. -- 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: Fri, 15 Mar 2019 07:24:49 +0000 Gerrit-HasComments: Yes
