Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15414 )
Change subject: [java] fix Kudu Ranger plugin in a Kerberized env ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/15414/1/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/15414/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@77 PS1, Line 77: // Use UserGroupInformation.isSecurityEnabled() to determine if Kerberos is enabled : // as what Ranger client-side library RangerAdminRESTClient does. : // (https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/admin/client/RangerAdminRESTClient.java#L129) : if (UserGroupInformation.isSecurityEnabled()) Rather than saying what code is being run, could you describe what we're doing here? What is isSecurityEnabled() checking? How does a user control this? How is this coordinated with the Kudu master? Also this link will likely become stale at some point as the Ranger codebase changes. Also IMO the point about RangerAdminRESTClient is belongs in the commit message since it's meant for us as reviewers to understand what your rationale in writing the code this way, rather than readers of the code later down the line to understand how this code works and what it's doing. Same feedback for the comment below. http://gerrit.cloudera.org:8080/#/c/15414/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@81 PS1, Line 81: kuduPrincipal.isEmpty() || keytab.isEmpty() Should we log what these are, or would that be bad from a security perspective? http://gerrit.cloudera.org:8080/#/c/15414/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@94 PS1, Line 94: "when Kerberos is enabled" nit: this kind of goes without saying http://gerrit.cloudera.org:8080/#/c/15414/1/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java: http://gerrit.cloudera.org:8080/#/c/15414/1/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java@26 PS1, Line 26: import org.apache.kudu.subprocess.*; Revert? -- To view, visit http://gerrit.cloudera.org:8080/15414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe043293ea9cc1c2f43a331603dc1e3b36ff6ae0 Gerrit-Change-Number: 15414 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 12 Mar 2020 01:24:36 +0000 Gerrit-HasComments: Yes
