Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 7: (62 comments) http://gerrit.cloudera.org:8080/#/c/11797/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11797/7//COMMIT_MSG@20 PS7, Line 20: exposedp > exposed Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/client/client-test.cc@293 PS7, Line 293: /* rpc = */ > Nit: convention is more like this: Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/CMakeLists.txt@91 PS7, Line 91: ADD_KUDU_TEST(master_sentry-itest RUN_SERIAL true PROCESSORS 4) : ADD_KUDU_TEST(master_sentry_advance-itest RUN_SERIAL true PROCESSORS 4) > Do these need to be sharded, given the parameterized tests in each and the Yeah, I think they should be sharded. After adding the shards (NUM_SHARDS 8), each shard took around 80s to run. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@52 PS7, Line 52: using client::KuduColumnSchema; : using client::KuduSchema; : using client::KuduSchemaBuilder; : using client::KuduTable; : using client::KuduTableCreator; : using client::sp::shared_ptr; : using hms::HmsClient; : using std::string; : using std::unique_ptr; : using strings::Substitute; > I'm not sure that 'using ...' in a header file is a good idea. Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@52 PS7, Line 52: using client::KuduColumnSchema; : using client::KuduSchema; : using client::KuduSchemaBuilder; : using client::KuduTable; : using client::KuduTableCreator; : using client::sp::shared_ptr; : using hms::HmsClient; : using std::string; : using std::unique_ptr; : using strings::Substitute; > Agreed with Alexey; I don't think this is a pattern we should promote. Almo Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@68 PS7, Line 68: RETURN_NOT_OK(cluster_->hms()->Stop()); : return Status::OK(); > nit for brevity here and below: maybe, just return the status of the last f Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@129 PS7, Line 129: return hms_client_->AlterTable(database_name, old_table_name, table); > Just out of curiosity (not sure it's a matter of concern right here): What It should be fine, as other than table name, we always use metadata in Kudu as the source of truth. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@154 PS7, Line 154: KuduSchema schema = table->schema(); > const auto& schema = table->schema(); Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@193 PS7, Line 193: env_ctx.__set_properties({ std::make_pair(hms::HmsClient::kKuduMasterEventKey, "true") }); > Would the shorter option like Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@199 PS7, Line 199: > nit: drop the extra line? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_hms-itest.cc@46 PS7, Line 46: using boost::none; : using client::KuduTable; : using client::KuduTableAlterer; : using client::sp::shared_ptr; : using cluster::ExternalMiniClusterOptions; : using hms::HmsClient; : using std::string; : using std::unique_ptr; : using std::vector; : using strings::Substitute; > nit: could you move these 'using' out of the 'namespace ...' scope? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@31 PS7, Line 31: // IWYU pragma: no_include "kudu/integration-tests/hms_itest-base.h" > Again, what happened here? Same here, if I included it, I got an 'duplicate symbols' compilation error. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@35 PS7, Line 35: using std::set; : using std::string; : using std::vector; : using strings::Substitute; > nit: could you move this out of the 'namespace ...' scope? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@51 PS7, Line 51: ASSERT_EQ(0, tables.size()); > nit: ASSERT_TRUE(tables.empty()); Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@62 PS7, Line 62: std:: > drop the prefix (there is corresponding 'using ...' above) Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@62 PS7, Line 62: std:: > ditto Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@63 PS7, Line 63: ASSERT_EQ(set<string>({ Substitute("$0.$1", kDatabaseName, kTableName), > Nit: unordered_set is more representative of the semantics you care about ( Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@75 PS7, Line 75: Substitute("$0.$1", desc.database, desc.table), : Substitute("$0.$1", desc.database, desc.new_table)); > here and in other places in this file: maybe, introduce a couple of const v Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@77 PS7, Line 77: ASSERT_TRUE(s.IsNotAuthorized()); > add: Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@98 PS7, Line 98: // Authorize create table. > These comments don't add any useful information. 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: PS7: > Maybe, be more specific with the name of the file? I actually moved all tests to master_sentry-itest.cc http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@31 PS7, Line 31: // IWYU pragma: no_include "kudu/integration-tests/hms_itest-base.h" > What happened here? Why shouldn't we include this? Hmm, when I included this, I got an 'duplicate symbols' compilation error. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@37 PS7, Line 37: advanced > What is "advanced" referring to? Actually these are also functionality tests, which should fit in master_sentry-itest.cc http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@37 PS7, Line 37: advanced > Also not really seeing the distinction; why can't these tests just live in You're right, removed them to master_sentry-itest. Originally, I divided them to include less test in each. However, I should use SHARDS as you suggested. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@40 PS7, Line 40: MasterAdvanceSentryTest > MasterAdvancedSentryTest ? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@57 PS7, Line 57: TEST_F(MasterAdvanceSentryTest, TestRenameTablePrivilegeTransfer) { : ASSERT_OK(GrantRenameTablePrivilege(kDatabaseName, kTableName)); : ASSERT_OK(RenameTable(Substitute("$0.$1", kDatabaseName, kTableName), : Substitute("$0.$1", kDatabaseName, "b"))); : NO_FATALS(CheckTable(kDatabaseName, "b", make_optional<const string &>(kAdminUser))); : : // TODO(hao): uncomment the following tests after SENTRY-2471 is fixed. : // Checks table rename in the HMS is synchronized with the Sentry privileges. : : // table_alterer.reset(client_->NewTableAlterer(Substitute("$0.$1", kDatabaseName, "b")) : // ->DropColumn("int16_val")); : : // Note that unlike table creation, there could be a delay between the table renaming : // in Kudu and the privilege renaming in Sentry. Because Kudu uses the transactional : // listener of the HMS to get notification of table alteration events, while Sentry : // uses post event listener (which is executed outside the HMS transaction). There : // is a chance that Kudu already finish the table renaming but the privilege renaming : // hasn't been reflected in the Sentry service. : // ASSERT_EVENTUALLY([&] { : // ASSERT_OK(table_alterer->Alter()); : // }); : // NO_FATALS(CheckTable(kDatabaseName, "b", make_optional<const string&>(kAdminUser))); : // : // table_alterer.reset(client_->NewTableAlterer(Substitute("$0.$1", kDatabaseName, "b")) : // ->RenameTo(Substitute("$0.$1", kDatabaseName, "c"))); : // ASSERT_OK(table_alterer->Alter()); : // NO_FATALS(CheckTable(kDatabaseName, "c", make_optional<const string&>(kAdminUser))); : } > Would it be easier just to add the special DISABLED_ (TestRenameTablePrivil Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@91 PS7, Line 91: altering > But we're not necessarily altering, right? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@95 PS7, Line 95: Substitute("$0.$1", kDatabaseName, "non_existent"), : Substitute("$0.$1", kDatabaseName, "b")); > here and in this file: introduce const varibles for the result of Substitut Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@125 PS7, Line 125: // Authz funcs for delete table . > Nit: misplaced Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@131 PS7, Line 131: // Authz funcs for alter table. > On second thought, these comments aren't adding anything useful. Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/registration-test.cc@197 PS7, Line 197: s = catalog->GetTabletLocations(tablet_id, master::VOTER_REPLICA, &loc, nullptr); > need /*rpc=*/ here Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h File src/kudu/integration-tests/sentry_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@54 PS7, Line 54: using sentry::TSentryGrantOption; : using sentry::TSentryPrivilege; : : namespace kudu { : : using boost::make_optional; : using client::KuduScanToken; : using client::KuduScanTokenBuilder; : using client::KuduTableAlterer; : using cluster::ExternalMiniClusterOptions; : using hms::HmsClient; : using master::MasterServiceProxy; : using sentry::SentryClient; : using master::TabletLocationsPB; : using std::string; : using std::unique_ptr; : using std::vector; : using strings::Substitute; > In general, adding 'using' into header files is not a good idea. It also c Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@75 PS7, Line 75: class SentryTestBase : public HmsTestBase, : public master::SentryAuthzProviderTestBase { > Let's not do multiple inheritance here, especially since SentryAuthzProvide Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@191 PS7, Line 191: Status GetTabletLocations(const string& table, const string& /*new_table*/) { > You could implement this in a different way, using the cluster's MiniCluste Actually it is not suitable for this use case, which is trying to figure out the tablets belonging to a given table (instead of listing all tablets). http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@194 PS7, Line 194: RETURN_NOT_OK(cluster_->CreateClient(nullptr, &client_)); > Could you create this second client instance without destroying client_, so Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@223 PS7, Line 223: // Always enable Kerberos, as in non-Kerberized environment the logged : // in user is not service users that allowed to connect to the Sentry : // service. > Can you rewrite this? The second half (beginning with "the logged in user.. Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@268 PS7, Line 268: // Log in as the 'test-user' and reset the client to pick up the change in user. : ASSERT_OK(cluster_->kdc()->Kinit(kTestUser)); : ASSERT_OK(cluster_->CreateClient(nullptr, &client_)); > See above comment about switching users. Hmm, it doesn't make much sense to create second client instance here, as 'client_' is the one used by all the tests. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@273 PS7, Line 273: void TearDown() override { > Reverse order of SetUp, so you should stop the Sentry client first, then th Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@298 PS7, Line 298: std::function<Status(SentryTestBase*, const string&, const string&)> > Introduce an intermediate typedef for this and use it to define OperatorFun Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@313 PS7, Line 313: table > Is this name or id of the table? Consider naming the fields unambiguously Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/authz_provider.cc File src/kudu/master/authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/authz_provider.cc@35 PS7, Line 35: without being authorized. > nit: perhaps "without being authorized against fine-grained permissions" or Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/authz_provider.cc@50 PS7, Line 50: static std::once_flag once; : std::call_once(once, [] { : debug::ScopedLeakCheckDisabler d; : vector<string> acls = strings::Split(FLAGS_trusted_user_acl, ",", strings::SkipEmpty()); : g_trusted_users = new unordered_set<string>(acls.begin(), acls.end()); : }); > We only expect there to be on AuthzProvider per master process anyway. Why Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.h@730 PS7, Line 730: If 'user' is provided, : // checks that the user is authorized to delete the table. > Mind adding a sentence explaining why 'user' might not be provided? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.h@865 PS7, Line 865: authorized to operate on table, > nit: "authorized to operate on the table" 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: table = FindPtrOrNull(table_ids_map_, table_identifier.table_id()); > Right, but I was specifically asking about this particular case, where the Hmm, I see. I am not sure either the request's table name or table ID is more trustworthy in this case. I don't see one is certainly better than the other. Do you have any thoughts? 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; > The past couple times I've read this, I've been confused why it's possible Yeah, it is only for tests. However, I don't see an easy way of defining dummy RpcContext in the test code, and also which user we should be passed in this case? Any thoughts? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1769 PS7, Line 1769: string table_name; : : auto > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1784 PS7, Line 1784: error > nit: maybe rename this something more descriptive, like `sanitized_error` o Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1785 PS7, Line 1785: table is authorized to : // avoid leaking whether the table exists > nit: I'm having trouble parsing "authorized to avoid leaking", mind rephras Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1786 PS7, Line 1786: empty table name will : // also trigger a NOT_AUTHORIZED error > Could we just check for this up front and return Status::InvalidArgument or You're right. Removed. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1794 PS7, Line 1794: shared_lock<LockType> l(lock_); > Let's make sure this is dropped before any authorization calls. Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@1828 PS7, Line 1828: RETURN_NOT_OK(authorize()); > It's unfortunate that we have to hold TableMetadataLock during the authoriz Right, this is actually documented here: https://gerrit.cloudera.org/#/c/11797/9/src/kudu/master/catalog_manager.h@857 http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@2729 PS7, Line 2729: shared_lock<LockType> l(lock_); > Can we avoid holding this during the authorization? Seems like we could do Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@2744 PS7, Line 2744: Status s = authz_provider_->AuthorizeGetTableMetadata(NormalizeTableName(table_name), : *user); > This is going to be really expensive when there are a lot of tables. Is the I don't see there is a good way to do bulk authorization based on the current design. But to reduce the number of RPCs to Sentry, maybe we can extend the usage of master authz cache to support privileges preloading. So that we can preload all privileges the user has on authoriziable 'server=server1' before this call, and do a per table authorization based on all the privileges the user has (This requires some changes in SentryAuthzProvider as well). How do you think? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4640 PS7, Line 4640: shared_lock<LockType> l(lock_); > Can we drop this before authorizing? Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4642 PS7, Line 4642: // is enabled. Because tablet id is a random generated string which doesn't > randomly Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4643 PS7, Line 4643: On the other hand, we can always return : // NOT_AUTHORIZED error, which can be too restricted. > Not really sure what this second sentence is offering. Is it suggesting an It is trying to explain more on why we choose to return NOT_FOUND error back, but I think the sentence is misleading and don't add any more information. Therefore, I will remove it. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4644 PS7, Line 4644: restricted > restrictive Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4651 PS7, Line 4651: TableMetadataLock table_lock(table.get(), LockMode::READ); > The 'table' variable is not used elsewhere, and tablet_info is staying here Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/master-test-util.h File src/kudu/master/master-test-util.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/master-test-util.h@56 PS7, Line 56: nullptr > /*rpc=*/ Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/sentry_authz_provider.cc@200 PS7, Line 200: if (AuthzProvider::IsTrustedUser(user)) { : return Status::OK(); : } > You could push all of these down into Authorize(). It means a little bit mo Done http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/sentry/mini_sentry.cc@210 PS7, Line 210: // derive OWNER/ALL privileges from object's ownership. 'all' indicates an > Nit: derives 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: 7 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: Thu, 07 Mar 2019 08:47:31 +0000 Gerrit-HasComments: Yes
