Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-23 Thread Deep Singh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/
---

(Updated Nov. 23, 2020, 10:15 p.m.)


Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
and Sarath Subramanian.


Bugs: ATLAS-4036 and ATLAS-4037
https://issues.apache.org/jira/browse/ATLAS-4036
https://issues.apache.org/jira/browse/ATLAS-4037


Repository: atlas


Description
---

For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, we 
now have falback strategies. We first try to get user from request context, if 
it is not found then it is assumed that the operation is donw by system 
user(atlas). Similarly clientId is first determined using IP address 
information in request context, if not found we fallback to JDK InetAddress 
apis to fetch hostname and ip address, if not found there as well, we log 
clientId as "unknown"


Diffs
-

  
repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
 d843204f2 
  
repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
 bfc300ec9 
  
repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
 ed17b927e 
  server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
  webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
4db477eef 
  webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
1d1e08e43 
  webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 93e6513ff 


Diff: https://reviews.apache.org/r/73029/diff/2/


Testing (updated)
---

Manual testing done

Precommit test
https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/215/console


Thanks,

Deep Singh

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-23 Thread Madhan Neethiraj 


> On Nov. 20, 2020, 8:04 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
> > Lines 77 (patched)
> > 
> >
> > clientId => clientIp
> 
> Deep Singh wrote:
> Madhan, Variable will not contain IP all the time. Sometime it could be 
> IP, sometime it could be combination of host name and IP. I think 'clientId' 
> is an appropriate name for the variable.

Ok. I agree. Thanks!


- Madhan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review20
---


On Nov. 20, 2020, 4:05 p.m., Deep Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73029/
> ---
> 
> (Updated Nov. 20, 2020, 4:05 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4036 and ATLAS-4037
> https://issues.apache.org/jira/browse/ATLAS-4036
> https://issues.apache.org/jira/browse/ATLAS-4037
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
> SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, 
> we now have falback strategies. We first try to get user from request 
> context, if it is not found then it is assumed that the operation is donw by 
> system user(atlas). Similarly clientId is first determined using IP address 
> information in request context, if not found we fallback to JDK InetAddress 
> apis to fetch hostname and ip address, if not found there as well, we log 
> clientId as "unknown"
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
>  d843204f2 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
>  bfc300ec9 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
>  ed17b927e 
>   server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> 4db477eef 
>   webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
> 1d1e08e43 
>   webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 
> 93e6513ff 
> 
> 
> Diff: https://reviews.apache.org/r/73029/diff/2/
> 
> 
> Testing
> ---
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Deep Singh
> 
>

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-23 Thread Deep Singh 


> On Nov. 20, 2020, 8:04 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
> > Lines 77 (patched)
> > 
> >
> > clientId => clientIp

Madhan, Variable will not contain IP all the time. Sometime it could be IP, 
sometime it could be combination of host name and IP. I think 'clientId' is an 
appropriate name for the variable.


- Deep


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review20
---


On Nov. 20, 2020, 4:05 p.m., Deep Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73029/
> ---
> 
> (Updated Nov. 20, 2020, 4:05 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4036 and ATLAS-4037
> https://issues.apache.org/jira/browse/ATLAS-4036
> https://issues.apache.org/jira/browse/ATLAS-4037
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
> SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, 
> we now have falback strategies. We first try to get user from request 
> context, if it is not found then it is assumed that the operation is donw by 
> system user(atlas). Similarly clientId is first determined using IP address 
> information in request context, if not found we fallback to JDK InetAddress 
> apis to fetch hostname and ip address, if not found there as well, we log 
> clientId as "unknown"
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
>  d843204f2 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
>  bfc300ec9 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
>  ed17b927e 
>   server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> 4db477eef 
>   webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
> 1d1e08e43 
>   webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 
> 93e6513ff 
> 
> 
> Diff: https://reviews.apache.org/r/73029/diff/2/
> 
> 
> Testing
> ---
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Deep Singh
> 
>

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-22 Thread Sarath Subramanian 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review25
---


Ship it!




Ship It!

- Sarath Subramanian


On Nov. 20, 2020, 8:05 a.m., Deep Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73029/
> ---
> 
> (Updated Nov. 20, 2020, 8:05 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4036 and ATLAS-4037
> https://issues.apache.org/jira/browse/ATLAS-4036
> https://issues.apache.org/jira/browse/ATLAS-4037
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
> SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, 
> we now have falback strategies. We first try to get user from request 
> context, if it is not found then it is assumed that the operation is donw by 
> system user(atlas). Similarly clientId is first determined using IP address 
> information in request context, if not found we fallback to JDK InetAddress 
> apis to fetch hostname and ip address, if not found there as well, we log 
> clientId as "unknown"
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
>  d843204f2 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
>  bfc300ec9 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
>  ed17b927e 
>   server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> 4db477eef 
>   webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
> 1d1e08e43 
>   webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 
> 93e6513ff 
> 
> 
> Diff: https://reviews.apache.org/r/73029/diff/2/
> 
> 
> Testing
> ---
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Deep Singh
> 
>

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-20 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review20
---


Fix it, then Ship it!





repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
Lines 77 (patched)


clientId => clientIp


- Madhan Neethiraj


On Nov. 20, 2020, 4:05 p.m., Deep Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73029/
> ---
> 
> (Updated Nov. 20, 2020, 4:05 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4036 and ATLAS-4037
> https://issues.apache.org/jira/browse/ATLAS-4036
> https://issues.apache.org/jira/browse/ATLAS-4037
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
> SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, 
> we now have falback strategies. We first try to get user from request 
> context, if it is not found then it is assumed that the operation is donw by 
> system user(atlas). Similarly clientId is first determined using IP address 
> information in request context, if not found we fallback to JDK InetAddress 
> apis to fetch hostname and ip address, if not found there as well, we log 
> clientId as "unknown"
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
>  d843204f2 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
>  bfc300ec9 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
>  ed17b927e 
>   server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> 4db477eef 
>   webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
> 1d1e08e43 
>   webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 
> 93e6513ff 
> 
> 
> Diff: https://reviews.apache.org/r/73029/diff/2/
> 
> 
> Testing
> ---
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Deep Singh
> 
>

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-20 Thread Mandar Ambawane 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review19
---


Ship it!




Ship It!

- Mandar Ambawane


On Nov. 20, 2020, 4:05 p.m., Deep Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73029/
> ---
> 
> (Updated Nov. 20, 2020, 4:05 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4036 and ATLAS-4037
> https://issues.apache.org/jira/browse/ATLAS-4036
> https://issues.apache.org/jira/browse/ATLAS-4037
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
> SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, 
> we now have falback strategies. We first try to get user from request 
> context, if it is not found then it is assumed that the operation is donw by 
> system user(atlas). Similarly clientId is first determined using IP address 
> information in request context, if not found we fallback to JDK InetAddress 
> apis to fetch hostname and ip address, if not found there as well, we log 
> clientId as "unknown"
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
>  d843204f2 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
>  bfc300ec9 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
>  ed17b927e 
>   server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> 4db477eef 
>   webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
> 1d1e08e43 
>   webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 
> 93e6513ff 
> 
> 
> Diff: https://reviews.apache.org/r/73029/diff/2/
> 
> 
> Testing
> ---
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Deep Singh
> 
>

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-20 Thread Deep Singh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/
---

(Updated Nov. 20, 2020, 4:05 p.m.)


Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
and Sarath Subramanian.


Changes
---

Incorporated suggestion given by Mandar.


Bugs: ATLAS-4036 and ATLAS-4037
https://issues.apache.org/jira/browse/ATLAS-4036
https://issues.apache.org/jira/browse/ATLAS-4037


Repository: atlas


Description
---

For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, we 
now have falback strategies. We first try to get user from request context, if 
it is not found then it is assumed that the operation is donw by system 
user(atlas). Similarly clientId is first determined using IP address 
information in request context, if not found we fallback to JDK InetAddress 
apis to fetch hostname and ip address, if not found there as well, we log 
clientId as "unknown"


Diffs (updated)
-

  
repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
 d843204f2 
  
repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
 bfc300ec9 
  
repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
 ed17b927e 
  server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
  webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
4db477eef 
  webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
1d1e08e43 
  webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 93e6513ff 


Diff: https://reviews.apache.org/r/73029/diff/2/

Changes: https://reviews.apache.org/r/73029/diff/1-2/


Testing
---

Manual testing done


Thanks,

Deep Singh

Re: Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-20 Thread Mandar Ambawane 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review17
---




repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
Lines 77 (patched)


Please consider replacing 
if (clientId == null || clientId.isEmpty()) 
>
if (StringUtils.isEmpty(clientId))



repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
Lines 86 (patched)


Please remove unused obect variable "entry"



server-api/src/main/java/org/apache/atlas/RequestContext.java
Lines 132 (patched)


Consider adding one more fallback before fetching from property file or 
hardcoding username string.

if (StringUtils.isBlank(ret)) {
try {
ret = 
UserGroupInformation.getLoginUser().getShortUserName();
} catch (IOException e) {
LOG.error("Exception occurred while retrieving Login User");
}
if (StringUtils.isNotBlank(ret)) {
return ret;
}
ret = System.getProperty("user.name");
if (StringUtils.isBlank(ret)) {
ret = "atlas";
}
}


- Mandar Ambawane


On Nov. 20, 2020, 4:49 a.m., Deep Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73029/
> ---
> 
> (Updated Nov. 20, 2020, 4:49 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4036 and ATLAS-4037
> https://issues.apache.org/jira/browse/ATLAS-4036
> https://issues.apache.org/jira/browse/ATLAS-4037
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
> SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, 
> we now have falback strategies. We first try to get user from request 
> context, if it is not found then it is assumed that the operation is donw by 
> system user(atlas). Similarly clientId is first determined using IP address 
> information in request context, if not found we fallback to JDK InetAddress 
> apis to fetch hostname and ip address, if not found there as well, we log 
> clientId as "unknown"
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
>  d843204f2 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
>  bfc300ec9 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
>  ed17b927e 
>   server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> 4db477eef 
>   webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
> 1d1e08e43 
>   webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 
> 93e6513ff 
> 
> 
> Diff: https://reviews.apache.org/r/73029/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing done
> 
> 
> Thanks,
> 
> Deep Singh
> 
>

Review Request 73029: ATLAS-4036, ATLAS-4037, Fix for empty Users and Client Id field in Admin Audit

2020-11-19 Thread Deep Singh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/
---

Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, 
and Sarath Subramanian.


Bugs: ATLAS-4036 and ATLAS-4037
https://issues.apache.org/jira/browse/ATLAS-4036
https://issues.apache.org/jira/browse/ATLAS-4037


Repository: atlas


Description
---

For System operations like TYPE_DEF_CREATE, TYPE_DEF_UPDATE, SERVER_START and 
SERVER_STATE_ACTIVE the users field and client Id was empty. With this fix, we 
now have falback strategies. We first try to get user from request context, if 
it is not found then it is assumed that the operation is donw by system 
user(atlas). Similarly clientId is first determined using IP address 
information in request context, if not found we fallback to JDK InetAddress 
apis to fetch hostname and ip address, if not found there as well, we log 
clientId as "unknown"


Diffs
-

  
repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
 d843204f2 
  
repository/src/main/java/org/apache/atlas/repository/audit/TypeDefAuditListener.java
 bfc300ec9 
  
repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasTypeDefGraphStoreV2.java
 ed17b927e 
  server-api/src/main/java/org/apache/atlas/RequestContext.java befd726ae 
  webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
4db477eef 
  webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java 
1d1e08e43 
  webapp/src/main/java/org/apache/atlas/web/service/ServiceState.java 93e6513ff 


Diff: https://reviews.apache.org/r/73029/diff/1/


Testing
---

Manual testing done


Thanks,

Deep Singh