You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Don Bosco Durai <bo...@apache.org> on 2020/03/07 22:06:26 UTC
Re: Review Request 71921: Add support for ElasticSearch as an Audit
Database
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71921/#review219835
-----------------------------------------------------------
agents-audit/pom.xml
Lines 118 (patched)
<https://reviews.apache.org/r/71921/#comment308058>
Any reason the version is hard coded?
agents-audit/src/main/java/org/apache/ranger/audit/destination/ElasticSearchAuditDestination.java
Lines 120 (patched)
<https://reviews.apache.org/r/71921/#comment308060>
Can we have this in if(LOG.isDebugEnabled()) { block }?
agents-audit/src/main/java/org/apache/ranger/audit/destination/ElasticSearchAuditDestination.java
Lines 160 (patched)
<https://reviews.apache.org/r/71921/#comment308061>
For debugging purpose, should we set this as member attributes and init load them and have it logged at INFO level (without the password)? This will help during debuging.
agents-audit/src/main/java/org/apache/ranger/audit/destination/ElasticSearchAuditDestination.java
Lines 200 (patched)
<https://reviews.apache.org/r/71921/#comment308062>
Can we print additional information like port, etc.
migration-util/ambari2.1-hdp2.3-ranger0.50/bin/import_ranger_to_ambari.py
Lines 292 (patched)
<https://reviews.apache.org/r/71921/#comment308063>
Do we need to update this file? Not sure whether anyone will need it. If it not used or tested, then we should probably not just add and risk breaking something else.
pom.xml
Lines 135 (patched)
<https://reviews.apache.org/r/71921/#comment308057>
Can we indent this as others?
security-admin/scripts/install.properties
Lines 85 (patched)
<https://reviews.apache.org/r/71921/#comment308064>
Can we keep plugin and admin config similar? In Plugin we are taking Port as config
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsService.java
Lines 76 (patched)
<https://reviews.apache.org/r/71921/#comment308065>
Just curious if there are additional information that can be printed which would be useful for debug?
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsService.java
Lines 105 (patched)
<https://reviews.apache.org/r/71921/#comment308066>
Can we print the request data? And also the execption?
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
Lines 58 (patched)
<https://reviews.apache.org/r/71921/#comment308067>
Seems we have the port configurable. But it is not in the properties file. It would be good to have it with the default value.
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
Lines 60 (patched)
<https://reviews.apache.org/r/71921/#comment308068>
Any reason this is WARN? Can we have it as INFO?
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
Lines 64 (patched)
<https://reviews.apache.org/r/71921/#comment308069>
I think we have org.apache.ranger.common.StringUtil.isEmpty() method which will check for null and empty string. It will take care if the value is ""
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
Lines 73 (patched)
<https://reviews.apache.org/r/71921/#comment308070>
Can we put an INFO log with the username that will be used to login? So we know it is trying authentication mechanism?
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
Lines 87 (patched)
<https://reviews.apache.org/r/71921/#comment308071>
Can we have other properties also?
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
Lines 99 (patched)
<https://reviews.apache.org/r/71921/#comment308072>
Since connect() is already synchronized, do we need this here also?
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchUtil.java
Lines 73 (patched)
<https://reviews.apache.org/r/71921/#comment308074>
Can we use StringUtil.isEmpty() here and remove empty string check? Same for other methods also
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchUtil.java
Lines 224 (patched)
<https://reviews.apache.org/r/71921/#comment308075>
Can we have curly braces here? It will avoid anyone make mistakes in the future wrt scope
security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchUtil.java
Lines 288 (patched)
<https://reviews.apache.org/r/71921/#comment308076>
Can we print other parameter from the method also? Same for other log messages in this method
security-admin/src/main/resources/conf.dist/ranger-admin-site.xml
Lines 49 (patched)
<https://reviews.apache.org/r/71921/#comment308077>
Could we have port also here?
- Don Bosco Durai
On Jan. 24, 2020, 7:56 p.m., Andrew Charneski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71921/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2020, 7:56 p.m.)
>
>
> Review request for ranger and Don Bosco Durai.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> RANGER-2634 Add support for ElasticSearch as an Audit Database
>
>
> Diffs
> -----
>
> agents-audit/pom.xml 8ac1edf4f
> agents-audit/src/main/java/org/apache/ranger/audit/destination/ElasticSearchAuditDestination.java PRE-CREATION
> agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditProviderFactory.java 88cf99b6e
> hbase-agent/conf/ranger-hbase-audit-changes.cfg 719c7cdbf
> hbase-agent/scripts/install.properties f4fdb14ac
> hdfs-agent/conf/ranger-hdfs-audit-changes.cfg e34d15451
> hdfs-agent/pom.xml 5fe8a41f7
> hdfs-agent/scripts/install.properties 54dc3a19e
> hive-agent/conf/ranger-hive-audit-changes.cfg 3fd7e14bf
> hive-agent/pom.xml f219a375a
> hive-agent/scripts/install.properties 3e8f59025
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuditHandler.java a3d575c86
> hive-agent/src/test/java/org/apache/ranger/services/hive/HIVERangerAuthorizerTest.java f901f71a3
> kms/pom.xml 3bf20fdd4
> kms/scripts/install.properties a30b1d3a9
> knox-agent/conf/ranger-knox-audit-changes.cfg f0571e767
> knox-agent/scripts/install.properties d2dbbc3fa
> migration-util/ambari2.1-hdp2.3-ranger0.50/bin/import_ranger_to_ambari.py e25b8ae89
> plugin-atlas/conf/ranger-atlas-audit-changes.cfg 07fc38279
> plugin-atlas/conf/ranger-atlas-audit.xml 93ad238df
> plugin-atlas/scripts/install.properties 511e6ae32
> plugin-elasticsearch/conf/ranger-elasticsearch-audit-changes.cfg 8071e7b46
> plugin-elasticsearch/scripts/install.properties 3a5b2132e
> plugin-kafka/conf/ranger-kafka-audit-changes.cfg 661b498a2
> plugin-kafka/scripts/install.properties 6b01aedcb
> plugin-kms/conf/ranger-kms-audit-changes.cfg 69849d6a3
> plugin-kylin/conf/ranger-kylin-audit-changes.cfg 8071e7b46
> plugin-kylin/scripts/install.properties 126eebad0
> plugin-ozone/conf/ranger-ozone-audit-changes.cfg e5adb7681
> plugin-ozone/scripts/install.properties 276d192a0
> plugin-presto/conf/ranger-presto-audit-changes.cfg 661b498a2
> plugin-presto/scripts/install.properties 3110e2d01
> plugin-solr/conf/ranger-solr-audit-changes.cfg 622052ed1
> plugin-solr/scripts/install.properties 48a9af211
> plugin-sqoop/conf/ranger-sqoop-audit-changes.cfg 8071e7b46
> plugin-sqoop/scripts/install.properties 44f16dac8
> plugin-yarn/conf/ranger-yarn-audit-changes.cfg 8071e7b46
> plugin-yarn/scripts/install.properties f776c5f6f
> pom.xml e3c5ce3bc
> ranger-elasticsearch-plugin-shim/src/main/java/org/apache/ranger/authorization/elasticsearch/plugin/RangerElasticsearchPlugin.java f9a683706
> ranger-hive-plugin-shim/pom.xml c27647034
> ranger-util/src/test/LICENSE.txt PRE-CREATION
> security-admin/pom.xml fc4a20020
> security-admin/scripts/install.properties 155c42ccc
> security-admin/scripts/ranger-admin-site-template.xml af345cf43
> security-admin/scripts/setup.sh 9677f57b8
> security-admin/scripts/upgrade_admin.py 28b7e9884
> security-admin/src/main/java/org/apache/ranger/AccessAuditsService.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/biz/AssetMgr.java 0f4488861
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java ebc72cf02
> security-admin/src/main/java/org/apache/ranger/biz/XAuditMgr.java 9eecc20a2
> security-admin/src/main/java/org/apache/ranger/common/PropertiesUtil.java 43bbdfb35
> security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsService.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchUtil.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/solr/SolrAccessAuditsService.java b422e7c00
> security-admin/src/main/resources/conf.dist/ranger-admin-site.xml 298f02b15
> security-admin/src/test/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsServiceTest.java PRE-CREATION
> security-admin/src/test/resources/log4j.xml 15ae2741c
> storm-agent/conf/ranger-storm-audit-changes.cfg 8071e7b46
> storm-agent/scripts/install.properties e805b75fd
>
>
> Diff: https://reviews.apache.org/r/71921/diff/2/
>
>
> Testing
> -------
>
> Semi-automated tests write and then read a value from a local ElasticSearch instance. Also set up an admin server and client running hive-plugin and verified audit events were stored and indexed.
>
>
> Thanks,
>
> Andrew Charneski
>
>
Re: Review Request 71921: Add support for ElasticSearch as an Audit
Database
Posted by Andrew Charneski <ac...@gmail.com>.
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> >
Changes made. Please re-review. Thank you.
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> > agents-audit/pom.xml
> > Lines 118 (patched)
> > <https://reviews.apache.org/r/71921/diff/2/?file=2209519#file2209519line118>
> >
> > Any reason the version is hard coded?
Since it was only used once, I didn't see any reason to make it a property, if that's what you mean by "hard coded". I assume that is your intent, since it appears many existing dependencies do not use dependencyManagement, and have introduced a property.
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> > agents-audit/src/main/java/org/apache/ranger/audit/destination/ElasticSearchAuditDestination.java
> > Lines 160 (patched)
> > <https://reviews.apache.org/r/71921/diff/2/?file=2209520#file2209520line160>
> >
> > For debugging purpose, should we set this as member attributes and init load them and have it logged at INFO level (without the password)? This will help during debuging.
Done
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> > migration-util/ambari2.1-hdp2.3-ranger0.50/bin/import_ranger_to_ambari.py
> > Lines 292 (patched)
> > <https://reviews.apache.org/r/71921/diff/2/?file=2209536#file2209536line292>
> >
> > Do we need to update this file? Not sure whether anyone will need it. If it not used or tested, then we should probably not just add and risk breaking something else.
I was merely trying to replicate all occurances of the solr config entries. Seems reasonable to remove this one.
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> > security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsService.java
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/71921/diff/2/?file=2209571#file2209571line76>
> >
> > Just curious if there are additional information that can be printed which would be useful for debug?
Added exception message.
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> > security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java
> > Lines 64 (patched)
> > <https://reviews.apache.org/r/71921/diff/2/?file=2209572#file2209572line64>
> >
> > I think we have org.apache.ranger.common.StringUtil.isEmpty() method which will check for null and empty string. It will take care if the value is ""
Changed as requested. Why is this method not static?
> On March 7, 2020, 10:06 p.m., Don Bosco Durai wrote:
> > security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchUtil.java
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/71921/diff/2/?file=2209573#file2209573line73>
> >
> > Can we use StringUtil.isEmpty() here and remove empty string check? Same for other methods also
This value isn't necessarily a string
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71921/#review219835
-----------------------------------------------------------
On Jan. 24, 2020, 7:56 p.m., Andrew Charneski wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71921/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2020, 7:56 p.m.)
>
>
> Review request for ranger and Don Bosco Durai.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> RANGER-2634 Add support for ElasticSearch as an Audit Database
>
>
> Diffs
> -----
>
> agents-audit/pom.xml 8ac1edf4f
> agents-audit/src/main/java/org/apache/ranger/audit/destination/ElasticSearchAuditDestination.java PRE-CREATION
> agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditProviderFactory.java 88cf99b6e
> hbase-agent/conf/ranger-hbase-audit-changes.cfg 719c7cdbf
> hbase-agent/scripts/install.properties f4fdb14ac
> hdfs-agent/conf/ranger-hdfs-audit-changes.cfg e34d15451
> hdfs-agent/pom.xml 5fe8a41f7
> hdfs-agent/scripts/install.properties 54dc3a19e
> hive-agent/conf/ranger-hive-audit-changes.cfg 3fd7e14bf
> hive-agent/pom.xml f219a375a
> hive-agent/scripts/install.properties 3e8f59025
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuditHandler.java a3d575c86
> hive-agent/src/test/java/org/apache/ranger/services/hive/HIVERangerAuthorizerTest.java f901f71a3
> kms/pom.xml 3bf20fdd4
> kms/scripts/install.properties a30b1d3a9
> knox-agent/conf/ranger-knox-audit-changes.cfg f0571e767
> knox-agent/scripts/install.properties d2dbbc3fa
> migration-util/ambari2.1-hdp2.3-ranger0.50/bin/import_ranger_to_ambari.py e25b8ae89
> plugin-atlas/conf/ranger-atlas-audit-changes.cfg 07fc38279
> plugin-atlas/conf/ranger-atlas-audit.xml 93ad238df
> plugin-atlas/scripts/install.properties 511e6ae32
> plugin-elasticsearch/conf/ranger-elasticsearch-audit-changes.cfg 8071e7b46
> plugin-elasticsearch/scripts/install.properties 3a5b2132e
> plugin-kafka/conf/ranger-kafka-audit-changes.cfg 661b498a2
> plugin-kafka/scripts/install.properties 6b01aedcb
> plugin-kms/conf/ranger-kms-audit-changes.cfg 69849d6a3
> plugin-kylin/conf/ranger-kylin-audit-changes.cfg 8071e7b46
> plugin-kylin/scripts/install.properties 126eebad0
> plugin-ozone/conf/ranger-ozone-audit-changes.cfg e5adb7681
> plugin-ozone/scripts/install.properties 276d192a0
> plugin-presto/conf/ranger-presto-audit-changes.cfg 661b498a2
> plugin-presto/scripts/install.properties 3110e2d01
> plugin-solr/conf/ranger-solr-audit-changes.cfg 622052ed1
> plugin-solr/scripts/install.properties 48a9af211
> plugin-sqoop/conf/ranger-sqoop-audit-changes.cfg 8071e7b46
> plugin-sqoop/scripts/install.properties 44f16dac8
> plugin-yarn/conf/ranger-yarn-audit-changes.cfg 8071e7b46
> plugin-yarn/scripts/install.properties f776c5f6f
> pom.xml e3c5ce3bc
> ranger-elasticsearch-plugin-shim/src/main/java/org/apache/ranger/authorization/elasticsearch/plugin/RangerElasticsearchPlugin.java f9a683706
> ranger-hive-plugin-shim/pom.xml c27647034
> ranger-util/src/test/LICENSE.txt PRE-CREATION
> security-admin/pom.xml fc4a20020
> security-admin/scripts/install.properties 155c42ccc
> security-admin/scripts/ranger-admin-site-template.xml af345cf43
> security-admin/scripts/setup.sh 9677f57b8
> security-admin/scripts/upgrade_admin.py 28b7e9884
> security-admin/src/main/java/org/apache/ranger/AccessAuditsService.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/biz/AssetMgr.java 0f4488861
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java ebc72cf02
> security-admin/src/main/java/org/apache/ranger/biz/XAuditMgr.java 9eecc20a2
> security-admin/src/main/java/org/apache/ranger/common/PropertiesUtil.java 43bbdfb35
> security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsService.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchMgr.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/elasticsearch/ElasticSearchUtil.java PRE-CREATION
> security-admin/src/main/java/org/apache/ranger/solr/SolrAccessAuditsService.java b422e7c00
> security-admin/src/main/resources/conf.dist/ranger-admin-site.xml 298f02b15
> security-admin/src/test/java/org/apache/ranger/elasticsearch/ElasticSearchAccessAuditsServiceTest.java PRE-CREATION
> security-admin/src/test/resources/log4j.xml 15ae2741c
> storm-agent/conf/ranger-storm-audit-changes.cfg 8071e7b46
> storm-agent/scripts/install.properties e805b75fd
>
>
> Diff: https://reviews.apache.org/r/71921/diff/2/
>
>
> Testing
> -------
>
> Semi-automated tests write and then read a value from a local ElasticSearch instance. Also set up an admin server and client running hive-plugin and verified audit events were stored and indexed.
>
>
> Thanks,
>
> Andrew Charneski
>
>