You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2019/03/07 08:47:31 UTC
[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 07 Mar 2019 08:47:31 +0000
Gerrit-HasComments: Yes