----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61021/#review183830 -----------------------------------------------------------
Qiang Zhang - it is good to see addition of new Ranger plugin. Good work! Please go through review comments. Overall, the plugin implementation looks good. plugin-kylin/src/main/java/org/apache/ranger/authorization/kylin/authorizer/RangerKylinAuthorizer.java Lines 95 (patched) <https://reviews.apache.org/r/61021/#comment259902> Looks like auditHandler will never be null here, given its initialization above in line #81. Consider moving line #96 inside the 'if' block that ends at #93. plugin-kylin/src/main/java/org/apache/ranger/authorization/kylin/authorizer/RangerKylinAuthorizer.java Lines 109 (patched) <https://reviews.apache.org/r/61021/#comment259903> This will get the IP address of the host where the plugin runs. Is this intentional? If yes, perhaps you can get this value during plugin initalization and use it for all request. This is unlikley to change across authorization requests. plugin-kylin/src/main/java/org/apache/ranger/authorization/kylin/authorizer/RangerKylinAuthorizer.java Lines 167 (patched) <https://reviews.apache.org/r/61021/#comment259904> RangerKylinAuditHandler doesn't seem to handle processResult() differently from RangerDefaultAuditHandler. You can remove this class and instead use RangerDefaultAuditHandler set in line #135. With this, you don't need to pass auditHandler argument in isAccessAllowed() call - line #89. - Madhan Neethiraj On Aug. 8, 2017, 2:08 a.m., Qiang Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61021/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2017, 2:08 a.m.) > > > Review request for ranger, Ankita Sinha, Don Bosco Durai, Colm O > hEigeartaigh, Gautam Borad, Madhan Neethiraj, Ramesh Mani, Selvamohan > Neethiraj, sam rome, and Velmurugan Periasamy. > > > Bugs: RANGER-1672 > https://issues.apache.org/jira/browse/RANGER-1672 > > > Repository: ranger > > > Description > ------- > > Ranger supports plugin to enable, monitor and manage apache kylin > > > Diffs > ----- > > agents-common/scripts/enable-agent.sh d31a264 > > agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBaseService.java > f1c6b9f > > agents-common/src/main/java/org/apache/ranger/plugin/store/EmbeddedServiceDefsUtil.java > 0bc09f6 > > agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java > 58cdd35 > agents-common/src/main/resources/service-defs/ranger-servicedef-kylin.json > PRE-CREATION > plugin-kylin/.gitignore PRE-CREATION > plugin-kylin/conf/ranger-kylin-audit-changes.cfg PRE-CREATION > plugin-kylin/conf/ranger-kylin-audit.xml PRE-CREATION > plugin-kylin/conf/ranger-kylin-security-changes.cfg PRE-CREATION > plugin-kylin/conf/ranger-kylin-security.xml PRE-CREATION > plugin-kylin/conf/ranger-policymgr-ssl-changes.cfg PRE-CREATION > plugin-kylin/conf/ranger-policymgr-ssl.xml PRE-CREATION > plugin-kylin/pom.xml PRE-CREATION > plugin-kylin/scripts/install.properties PRE-CREATION > > plugin-kylin/src/main/java/org/apache/ranger/authorization/kylin/authorizer/RangerKylinAuthorizer.java > PRE-CREATION > > plugin-kylin/src/main/java/org/apache/ranger/services/kylin/RangerServiceKylin.java > PRE-CREATION > > plugin-kylin/src/main/java/org/apache/ranger/services/kylin/client/KylinClient.java > PRE-CREATION > > plugin-kylin/src/main/java/org/apache/ranger/services/kylin/client/KylinResourceMgr.java > PRE-CREATION > > plugin-kylin/src/main/java/org/apache/ranger/services/kylin/client/json/model/KylinCubeResponse.java > PRE-CREATION > > plugin-kylin/src/main/java/org/apache/ranger/services/kylin/client/json/model/KylinProjectResponse.java > PRE-CREATION > pom.xml 01005b7 > ranger-kylin-plugin-shim/.gitignore PRE-CREATION > ranger-kylin-plugin-shim/pom.xml PRE-CREATION > > ranger-kylin-plugin-shim/src/main/java/org/apache/ranger/authorization/kylin/authorizer/RangerKylinAuthorizer.java > PRE-CREATION > security-admin/src/main/webapp/scripts/utils/XAEnums.js 164e939 > security-admin/src/main/webapp/scripts/utils/XAUtils.js 1979847 > security-admin/src/main/webapp/scripts/views/policies/PermissionList.js > 067bf3b > src/main/assembly/admin-web.xml cb1aad2 > src/main/assembly/plugin-kylin.xml PRE-CREATION > > > Diff: https://reviews.apache.org/r/61021/diff/2/ > > > Testing > ------- > > Tested > > > Thanks, > > Qiang Zhang > >
