Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 11: (21 comments) http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/hms_itest-base.h@56 PS11, Line 56: boost::optional<const std::string&> user); > nit: Document what this is used for. Also does it make sense to add a defau Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@326 PS11, Line 326: // Functor that performs certain operation (e.g Create, Rename) on a table given the : // table name (and the new name to renamed to) > Nit: to emphasize that the second argument is only used for Rename, perhaps I am not sure wrapping it in boost::optional is a good option. Even though this is used for rename only, but it is not an optional input for rename (nor for other ops). I will try to update the comment to add more clarification. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@331 PS11, Line 331: // on a table given the database the table belong to and the name of the table. > Nit: belongs Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@543 PS11, Line 543: TestAuthzErrorHandling > nit: it generally looks like we name our test classes as nouns and test met Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@545 PS11, Line 545: TestNonExistentTable > For CreateTable (not parameterized here) would it make sense to add test ca I don't think it makes sense to parameterize CreateTable or GetTabletLocations. This error handling test is targeting for cases you need to find the table first (via FindLockAndAuthorizeTable). http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/authz_provider.cc File src/kudu/master/authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/authz_provider.cc@44 PS11, Line 44: vector<string> acls = strings::Split(FLAGS_trusted_user_acl, ",", strings::SkipEmpty()); : std::move(acls.begin(), acls.end(), std::inserter(trusted_users_, trusted_users_.end())); > Do we have tests for this? If not, maybe it's worth separating this change Good catch, added the test. Though this is a small test so I keep it in the same patch unless you have other opinion? http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.h@592 PS11, Line 592: // The RPC context is provided for retrieving the user information, : // but this function does not itself respond to the RPC. > There are a bunch of these comments still sprinkled around. Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1705 PS11, Line 1705: checks > nit: "check", same elsewhere Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1781 PS11, Line 1781: string table_name = table_identifier.has_table_name() ? : NormalizeTableName(table_identifier.table_name()) : ""; > Might be clearer if table_name were wrapped in boost::optional, to reflect Are you suggesting to use boost::optional for table_name until L1826? As in L1834 table_name should no longer be optional. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1797 PS11, Line 1797: // Indicating if the table name and table ID refer to different tables : // respectively. > Nit: "Set to true if the client-provided table name and ID refer to differe Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1799 PS11, Line 1799: bool mismatch_table = false; > Nit: mismatched_table Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1804 PS11, Line 1804: : // If the request contains both a table ID and table name, ensure that : // both match the same table. : if (table_identifier.has_table_name() && : table.get() != FindPtrOrNull(normalized_table_names_map_, table_name).get()) { : mismatch_table = true; > Just trying to grasp the specific scenario we're worried about here. Our re I think Adar described a good example at https://gerrit.cloudera.org/#/c/11797/3/src/kudu/master/catalog_manager.cc@1798. The scenario looks like: 1. a table identifier with both an ID and a name 2. the ID maps to table A. 3. the name maps to table B. We need to authorize against both tables and then return a custom NOT_FOUND error with both table names. I updated the comment to add the example for clarification. Are you suggesting to move 'authorize(table_name)' atL1824 or L1835 to before the block? Note that we don't want to do 'authorize(table_name)' before holding the table lock unless the table is not found. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1821 PS11, Line 1821: operation > operate Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1824 PS11, Line 1824: RETURN_NOT_OK(authorize(table_name)); > If the client provided a table ID but no table was found, we'll end up call Yeah, good point, updated. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1830 PS11, Line 1830: : // Validate if the operation on the table is authorized. Reset the table : // name in case the client provided a table identifier with an ID but : // without a name. > We're also resetting the table name in case the table got renamed just as w Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1837 PS11, Line 1837: respectively > nit: remove this since it's not clarifying the ordering of anything Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1840 PS11, Line 1840: if (mismatch_table) { > Would be great to add test coverage for this case. Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1843 PS11, Line 1843: refer > refers Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1850 PS11, Line 1850: NormalizeTableName(lock.data().name() > Can reuse table_name here. Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@2689 PS11, Line 2689: optional<const string&> user = rpc ? : make_optional<const string&>(rpc->remote_user().username()) : none; > Should plumb this up into master_service.cc. Done http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@4671 PS11, Line 4671: // Acquire the table lock. > nit: Coming into this code fresh, one might think from this comment that we 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: 11 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: Tue, 19 Mar 2019 19:09:31 +0000 Gerrit-HasComments: Yes
