Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 )
Change subject: [WIP] Add all with grant and changing ownership ...................................................................... Patch Set 3: (3 comments) Perhaps consider separating the delegate admin part of this patch from the ownership part. E.g. we could implement the delegate admin part of this, adding back the CreateTable privileges check (i.e. ALL WITH GRANT / ALL && ADMIN ALL + delegate admin), and add unit tests for the RangerAuthzProvider. Then, once the rest of ownership lands, we can add more end-to-end tests with the master. That way we can get the more controversial "admin" bits squared away separately from the native ownership changes. http://gerrit.cloudera.org:8080/#/c/16071/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/16071/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@165 PS3, Line 165: action Another reason to use a boolean, it seems we should be auditing this as the action string, rather than "ADMIN", though it's probably worth confirming that with what other systems do. http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/master/ranger_authz_provider.cc@95 PS3, Line 95: // Table creation requires 'CREATE ON DATABASE' privilege. Can we plug the owner in here and require ALL and the delegate admin option here if it's a different owner than the user? A la the documented required Sentry privileges? http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/16071/3/src/kudu/ranger/ranger.proto@51 PS3, Line 51: ADMIN = 9; I would much rather have this be decoupled from ActionPB entirely, e.g. through a bool. IMO it'd be confusing for other developers who are at all familiar with Ranger because this is not a first-class action in the Kudu-Ranger policy, while the others are. -- To view, visit http://gerrit.cloudera.org:8080/16071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c Gerrit-Change-Number: 16071 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 18:29:38 +0000 Gerrit-HasComments: Yes
