Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )
Change subject: KUDU-3090 Add delegate admin privilege ...................................................................... Patch Set 4: (7 comments) > Patch Set 4: > > (8 comments) > > We can probably break this out of the relation chain so it doesn't depend on > the owner patch. The behavior we're adding hinges on ownership in CreateTable > RPCs (which already exists), rather than storing ownership under the hood. I guess I could break it out, but it does require KuduTableCreator::set_owner() which I just moved to the owner patch. Do you think it's worth moving it here or to a separate patch and add tests? http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@414 PS4, Line 414: GrantAllWithGrantPrivilege > Let's follow suit with naming and call this GrantAllWithGrantDatabasePrivil Done http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@419 PS4, Line 419: AuthorizationPolicy tbl_policy; : tbl_policy.databases.emplace_back(p.db_name); : tbl_policy.tables.emplace_back("*"); : tbl_policy.items.emplace_back(PolicyItem({kTestUser}, {ActionPB::ALL}, true)); : RETURN_NOT_OK(ranger_->AddPolicy(move(tbl_policy))); > Why do we need this too? In Ranger a permission on a database doesn't imply permissions on tables and columns, and IsCreateTableDone requires METADATA on *, I thought it makes sense to do it this way. In real clusters policies are usually created for each scope too, for example in CDP. http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/integration-tests/master_authz-itest.cc@660 PS4, Line 660: TEST_P(MasterAuthzITest, TestCreateTableDifferentOwner) { > Consider adding some or all of the following test cases: Done http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/master/ranger_authz_provider.cc@95 PS4, Line 95: admin > nit: same here -- this naming makes it sound like we should be passing the Done http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: PS4: > Not totally related to this patch, but we should really consider migrating I think it makes sense to test the client itself on its own without depending on Ranger as it can help identify where the issue is if something breaks, but I don't feel strongly about it and I agree that it's a bit of a pain to maintain (although I don't expect many changes after KUDU-3090 land). http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@182 PS4, Line 182: false > nit: comment annotations here too? Done http://gerrit.cloudera.org:8080/#/c/16071/4/src/kudu/ranger/ranger_client-test.cc@398 PS4, Line 398: true > Why do any of these need to be true? They don't, it doesn't matter. Changed to false. -- To view, visit http://gerrit.cloudera.org:8080/16071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c Gerrit-Change-Number: 16071 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 26 Jun 2020 09:28:58 +0000 Gerrit-HasComments: Yes
