You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Deep Singh <de...@gmail.com> on 2020/11/20 04:49:48 UTC
Review Request 73029: ATLAS-4036, ATLAS-4037,
Fix for empty Users and Client Id field in Admin Audit
-----------------------------------------------------------
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
Re: Review Request 73029: ATLAS-4036, ATLAS-4037,
Fix for empty Users and Client Id field in Admin Audit
Posted by Sarath Subramanian <sa...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review222225
-----------------------------------------------------------
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
Posted by Madhan Neethiraj <ma...@apache.org>.
> 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)
> > <https://reviews.apache.org/r/73029/diff/2/?file=2242081#file2242081line77>
> >
> > 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/#review222220
-----------------------------------------------------------
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
Posted by Deep Singh <de...@gmail.com>.
> 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)
> > <https://reviews.apache.org/r/73029/diff/2/?file=2242081#file2242081line77>
> >
> > 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/#review222220
-----------------------------------------------------------
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
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review222220
-----------------------------------------------------------
Fix it, then Ship it!
repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
Lines 77 (patched)
<https://reviews.apache.org/r/73029/#comment311268>
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
Posted by Mandar Ambawane <ma...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review222219
-----------------------------------------------------------
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
Posted by Deep Singh <de...@gmail.com>.
-----------------------------------------------------------
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
Posted by Deep Singh <de...@gmail.com>.
-----------------------------------------------------------
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
Posted by Mandar Ambawane <ma...@freestoneinfotech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73029/#review222217
-----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/audit/AtlasAuditService.java
Lines 77 (patched)
<https://reviews.apache.org/r/73029/#comment311265>
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)
<https://reviews.apache.org/r/73029/#comment311266>
Please remove unused obect variable "entry"
server-api/src/main/java/org/apache/atlas/RequestContext.java
Lines 132 (patched)
<https://reviews.apache.org/r/73029/#comment311267>
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
>
>