Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 16: (13 comments) http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger.proto@27 PS16, Line 27: // SELECT - select rows in a table : // INSERT - insert to a table : // UPDATE - update existing rows in a table : // DELETE - delete rows in a table : // ALTER - alter schema : // CREATE - creating a table : // DROP - drop a table It's not your fault, as this is introduced in the plugin patch. But if you are adding more detail information about possible privileges for the action type. Maybe refer https://docs.cloudera.com/documentation/enterprise/latest/topics/kudu_security.html for better accuracy? e.g. Select on Table/Column, Create on Database. And probably also mention we try to map the action types to the existing one in Sentry as the same privileges will be enforced between different authorization providers(e.g. Ranger, Sentry). http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@38 PS16, Line 38: Create nit: Creates? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@47 PS16, Line 47: const std::string& table_name nit: indent. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@54 PS16, Line 54: std::uno Same here. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@60 PS16, Line 60: . nit: is authorized to access if OK status is returned. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@62 PS16, Line 62: const std::string& table_name, : std: Same here. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@69 PS16, Line 69: AuthorizeAction nit: should it be named to AuthorizeActions given its functionality? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@70 PS16, Line 70: std::unordered_set<ActionPB>* actions, : const s And here. http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@97 PS10, Line 97: q_list, &resp_list)); : > not sure if it's worth spamming the logs with it, a future audit log might We have a warning log for debug purpose for Sentry if the access is not granted. But it is in the SentryAuthorizerProvider level https://github.com/apache/kudu/blob/master/src/kudu/master/sentry_authz_provider.cc#L357. But as Ranger has a audit log that can be enabled, I am Ok with not having log here. http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@193 PS10, Line 193: req->set_database(db); : req->set_table(tbl.ToString()); : > as they can't be managed by Ranger anyway I thought it would be better to r Left my thoughts on your latest patch. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@55 PS16, Line 55: return server_->Init(); Add a TODO here if you are going to have a follow up patch to actually call into Ranger subprocess? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@67 PS16, Line 67: CHECK(sresp.response().UnpackTo(resp)) Maybe use DCHECK instead? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@79 PS16, Line 79: As these tables cannot be : // whitelisted in Ranger we should allow access to them to avoid making tables : // inaccessible with Ranger integration enabled I think we should disallow the access to such tables to follow the deny by default security principle. And ask the users to correct the table name with the rename tool. But open to hear what others think. -- To view, visit http://gerrit.cloudera.org:8080/15206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 Gerrit-Change-Number: 15206 Gerrit-PatchSet: 16 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: Mon, 02 Mar 2020 23:45:58 +0000 Gerrit-HasComments: Yes
