Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 2: (31 comments) http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/hms_itest-base.h@56 PS2, Line 56: using client::KuduTableAlterer; > warning: using decl 'KuduTableAlterer' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/hms_itest-base.h@59 PS2, Line 59: using cluster::ExternalMiniClusterOptions; > warning: using decl 'ExternalMiniClusterOptions' is unused [misc-unused-usi Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/hms_itest-base.h@63 PS2, Line 63: using std::vector; > warning: using decl 'vector' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_hms-itest.cc@49 PS2, Line 49: using client::KuduTableCreator; > warning: using decl 'KuduTableCreator' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@49 PS2, Line 49: using sentry::TAlterSentryRoleAddGroupsRequest; > warning: using decl 'TAlterSentryRoleAddGroupsRequest' is unused [misc-unus Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@50 PS2, Line 50: using sentry::TAlterSentryRoleAddGroupsResponse; > warning: using decl 'TAlterSentryRoleAddGroupsResponse' is unused [misc-unu Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@51 PS2, Line 51: using sentry::TAlterSentryRoleGrantPrivilegeRequest; > warning: using decl 'TAlterSentryRoleGrantPrivilegeRequest' is unused [misc Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@52 PS2, Line 52: using sentry::TAlterSentryRoleGrantPrivilegeResponse; > warning: using decl 'TAlterSentryRoleGrantPrivilegeResponse' is unused [mis Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@53 PS2, Line 53: using sentry::TCreateSentryRoleRequest; > warning: using decl 'TCreateSentryRoleRequest' is unused [misc-unused-using Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@55 PS2, Line 55: using sentry::TSentryGroup; > warning: using decl 'TSentryGroup' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@61 PS2, Line 61: using client::KuduTable; > warning: using decl 'KuduTable' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@63 PS2, Line 63: using client::KuduTableCreator; > warning: using decl 'KuduTableCreator' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/integration-tests/master_sentry-itest.cc@68 PS2, Line 68: using std::set; > warning: using decl 'set' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/authz_provider.cc File src/kudu/master/authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/authz_provider.cc@33 PS2, Line 33: "Comma-separated list of trusted users who may access the cluster "/master_sentry-itest.cc > error: use of undeclared identifier 'master_sentry' [clang-diagnostic-error Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/authz_provider.cc@33 PS2, Line 33: "Comma-separated list of trusted users who may access the cluster "/master_sentry-itest.cc > error: use of undeclared identifier 'itest' [clang-diagnostic-error] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/authz_provider.cc@34 PS2, Line 34: "without being authorized."); > error: expected ')' [clang-diagnostic-error] Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@101 PS1, Line 101: #include "kudu/master/default_authz_provider.h" > missing authz_provider.h? Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@257 PS1, Line 257: DECLARE_bool(raft_prepare_replacement_before_eviction); > spitballing: in order to keep as much of the authz behavior in an easily au Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@257 PS1, Line 257: DECLARE_bool(raft_prepare_replacement_before_eviction); > +1, I think it makes sense to put this in authz_provider.cc, and maybe keep Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1238 PS1, Line 1238: if (hms_catalog_) { > It should always be initialized, right? Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1403 PS1, Line 1403: } > Since this isn't a runtime flag, consider doing this once and caching it in Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1403 PS1, Line 1403: > Probably won't be an issue, but just to head off any potential issues, cons Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1813 PS1, Line 1813: > This looks like a potential TOCTOU issue. We're also locking the table on Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@2227 PS1, Line 2227: } > Same thing here. Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/master.proto@85 PS1, Line 85: NOT_AUTHORIZED = 14; > add docs Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/master_service.cc@79 PS2, Line 79: using boost::none; > warning: using decl 'none' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/master_service.cc@80 PS2, Line 80: using boost::make_optional; > warning: using decl 'make_optional' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/master_service.cc@81 PS2, Line 81: using boost::optional; > warning: using decl 'optional' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: PS1: > These changes look like they should be in the SentryAuthzProvider patch. Done http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/sentry_authz_provider.cc@146 PS1, Line 146: entOpt > iequals? After more thoughts, I think the caller (catalog manager) should handle the name normalization, as it more aligns with the current code. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/sentry/sentry_policy_service.thrift File src/kudu/sentry/sentry_policy_service.thrift: http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/sentry/sentry_policy_service.thrift@26 PS1, Line 26: # - Rename enum TSentryGrantOption.TRUE and TSentryGrantOption.FALSE to avoid : # conflict > What's the significance of this change? What is the conflict? It conflicts with the macro defined in system header 'boolean.h'. You will see error: expected identifier FALSE = 0, ^ /usr/include/mach/boolean.h:85:15: note: expanded from macro 'FALSE' #define FALSE 0 -- 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: 2 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 10 Dec 2018 02:11:35 +0000 Gerrit-HasComments: Yes
