Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15207 )
Change subject: [WIP] Ranger authorization provider ...................................................................... Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/catalog_manager.cc@754 PS12, Line 754: if (SentryAuthzProvider::IsEnabled() && : RangerAuthzProvider::IsEnabled()) { : LOG(FATAL) << "Ranger and Sentry authorization cannot be enabled at the " : "same time."; Handle this as a GROUP_FLAG_VALIDATOR instead. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.h File src/kudu/master/ranger_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.h@38 PS12, Line 38: class RangerAuthzProvider : public AuthzProvider { Provide a high level class doc. Also I don't think you need to provide per-function docs for the overrides, unless the behavior deviates in some interesting way from the docs in authz_provider.h. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.h@40 PS12, Line 40: RangerAuthzProvider(); Can this be omitted? Don't you get a public constructor by default? http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.h@48 PS12, Line 48: // Returns Status::NotSupported() as Ranger authz provider doesn't support : // resetting its cache. You can implement this function inline and remove the docs since it'll be obvious what's going on. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@40 PS12, Line 40: "Ranger authorization. sentry_service_rpc_addresses must not be " We should update the equivalent flag in Sentry to indicate this incompatibility too. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@57 PS12, Line 57: std::vector<std::string> Don't need std prefixes. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@65 PS12, Line 65: void RangerAuthzProvider::Stop() { : } Can inline in the header. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@95 PS12, Line 95: if (old_table == new_table) { : return client_.AuthorizeAction(user, Action::ALTER, old_table); : } : : RETURN_NOT_OK(client_.AuthorizeAction(user, Action::ALL, old_table)); : return client_.AuthorizeAction(user, Action::CREATE, new_table); Could you write a comment similar to what's in SentryAuthzProvider to explain this? http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@128 PS12, Line 128: return client_.AuthorizeAction(user, Action::SELECT, table_name); Why not METADATA? Please add a comment. http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@131 PS12, Line 131: Status RangerAuthzProvider::FillTablePrivilegePB(const string& table_name, This looks like a lot of round-trips to the plugin and, if the cache is cold, to the Ranger server. Can we do it in bulk instead? http://gerrit.cloudera.org:8080/#/c/15207/12/src/kudu/master/ranger_authz_provider.cc@168 PS12, Line 168: if (find(column_names.begin(), column_names.end(), col.name()) != column_names.end()) { This is O(n^2) given that column_names is a vector. -- To view, visit http://gerrit.cloudera.org:8080/15207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e7672a5947d6406e0cad83a0c900bf5b2c03012 Gerrit-Change-Number: 15207 Gerrit-PatchSet: 12 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 26 Feb 2020 21:59:52 +0000 Gerrit-HasComments: Yes
