Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 14: (12 comments) http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/integration-tests/master_sentry-itest.cc@109 PS13, Line 109: > nit: rename to table_name Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/integration-tests/master_sentry-itest.cc@373 PS13, Line 373: ASSERT_OK(cluster_->kdc()->Kinit(kImpalaUser)); : ASSERT_OK(cluster_->CreateClient(nullptr, &client_)); : : vector<string> tables; : ASSERT_OK(client_->ListTables(&tables)); : unordered_set<string> tables_set(tables.begin(), tables.end()); : ASSERT_EQ(unordered_set<string>({ Substitute("$0.$1", kDatabaseName, kTableName), : Substitute("$0.$1", kDatabaseName, kSecondTable) }), : tables_set); : : ASSERT_OK(CreateKuduTable(kDatabaseName, "new_table")); > Maybe consider adding a case where ALL ON DATABASE is granted? Or is that c Yeah, I don't think it needs to be here. Test coverage for policy evaluation resides in sentry_authz_provider-test.cc. http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.h@853 PS13, Line 853: We expect that our external authorization metadata store > nit: Maybe make this agnostic to Sentry? e.g. "We expect that our external Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1333 PS13, Line 1333: name) { > nit: should be able to just do: Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1410 PS13, Line 1410: const string user = rpc->remote_user().username(); : const string > nit: cref these? Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1763 PS13, Line 1763: , an > nit: add a comma "Looking up, locking, and authorizing" Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1790 PS13, Line 1790: Status::NotFound("the table does not exist", SecureShortDebugString(table_identifier)), > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@1845 PS13, Line 1845: us::NotFound( : Substitute("the table ID refers to a different table '$0' than '$1'", : table_name, table_identifier.ta > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@2425 PS13, Line 2425: > nit: cref Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@2757 PS13, Line 2757: > nit: cref Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/catalog_manager.cc@4663 PS13, Line 4663: shared_lock<LockType> l(lock_); : // It's OK to return NOT_FOUND back to the client, even with authorization enabled, : // because tablet IDs are randomly ge > nit reword: Done http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11797/13/src/kudu/master/sentry_authz_provider-test.cc@67 PS13, Line 67: ASSERT_TRUE(authz_provider.IsTrustedUser("impala")); : ASSERT_TRUE(authz_provider.IsTrustedUser("hive")); : ASSERT_TRUE(authz_provider.IsTrustedUser("hdfs")); > Do we have tests that trusted users can actually sidestep authorization? Done -- 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: 14 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, 22 Mar 2019 23:47:29 +0000 Gerrit-HasComments: Yes
