Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16072 )
Change subject: [WIP] Implement owner privileges ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/16072/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/16072/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@113 PS3, Line 113: .setAllowed(false) nit: seems we typically have wrapped lines space at 4 spaces, or at an alignment point (e.g. periods, parens, etc) http://gerrit.cloudera.org:8080/#/c/16072/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@157 PS3, Line 157: * Creates a list of <code>RangerAccessRequest</code> for the given : * <code>RangerRequestListPB</code>. nit: update this http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/client/client.h@884 PS3, Line 884: KuduTableCreator& set_owner(const std::string& owner); Hrm, this seems like it belongs in the patch that introduces table ownership, or in a separate patch that only introduces the client API for setting ownership, since I imagine we'll want some tests specific to setting ownership in the client, regardless of privileges. http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/integration-tests/master_authz-itest.cc@866 PS3, Line 866: // this case doesn't make sense semantically as we don't have database : // owners which would be required to create a table What if the CreateTable statement requires ALL and delegate admin, e.g. if 'impala' is granted 'owner' privileges on some namespace of tables, and Impala creates a table within that namespace, setting the owner to someone else? http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/master/default_authz_provider.h File src/kudu/master/default_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/master/default_authz_provider.h@51 PS3, Line 51: const std::string& /*owner*/ It seems confusing for this to have the same API as AuthorizeCreateTable(), because unlike for AuthorizeCreateTable(), this 'owner' field isn't user-specified. Rather, the only thing we care about from an authorization POV is whether or not this request was made by the owner, so maybe pass 'is_owner' as a bool instead? http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/master/ranger_authz_provider.cc@106 PS3, Line 106: false, Shouldn't this be 'user == owner' or somesuch? Mind adding a test for this if there isn't one? http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client-test.cc@176 PS3, Line 176: unordered_set<ActionPB, ActionHash> actions = { : ActionPB::CREATE : }; Why these changes? http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client-test.cc@179 PS3, Line 179: false nit: could you annotate these with comments, e.g. "/*is_owner*/false"? http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/16072/3/src/kudu/ranger/ranger_client.h@88 PS3, Line 88: owner nit: IMO call-sites would be clearer if this were called 'is_owner' or somesuch instead. 'owner' makes it seem like this should be a string. Same in the protobuf. -- To view, visit http://gerrit.cloudera.org:8080/16072 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109 Gerrit-Change-Number: 16072 Gerrit-PatchSet: 3 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: Mon, 22 Jun 2020 19:31:18 +0000 Gerrit-HasComments: Yes
