You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/12/24 09:19:27 UTC
[GitHub] [ozone] ChenSammi opened a new pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
ChenSammi opened a new pull request #1734:
URL: https://github.com/apache/ozone/pull/1734
https://issues.apache.org/jira/browse/HDDS-4621
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#issuecomment-750856983
Did not find an obvious error message from the integration(client) output.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555662391
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
Hi @dineshchitlangia, I have the same idea to throw the ACLAlreadyExistsException exception when add an acl and ACL does not exist exception when remove an acl. I plan to put the logic in ozoneAclUtil.java so that we don't need to do the exception == null check in every ACL related request. So currently, there are OzoneAclUtils.java which used by bucket, key and prefix ACL operation, and OmOzoneAclMap.java which used by volume. I'd like to unify them and do some refactor which will be quite a big patch. Will do it in HDDS-4650.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dineshchitlangia commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555178513
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
Yikes, I intended to say that we set a not null value to exception and accordingly achieve it. Once I get to my laptop I will update here with proper suggestion. Thanks for catching it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dineshchitlangia commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555129517
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
Sorry for bad formatting, github on Phone is funny :)
Basically, I am suggesting we update exception=null in the else block after you have logged log.error().
Then call the auditlog() at the end of the method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dineshchitlangia commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555773861
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
Sound good to me. +1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi closed pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi closed pull request #1734:
URL: https://github.com/apache/ozone/pull/1734
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r553095264
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java
##########
@@ -88,6 +92,12 @@ public String getVolumeName() {
return volumeName;
}
+ @Override
+ OzoneObjInfo getObjectInfo() {
+ return OzoneObjInfo.fromProtobuf(
+ getOmRequest().getSetAclRequest().getObj());
Review comment:
fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi edited a comment on pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi edited a comment on pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#issuecomment-750819896
ACL logs with patch applied.
**2020-12-24 16:47:47,191 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:14,929 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:33,259 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:55,554 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:49:11,722 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:49:25,417 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:52:05,233 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null} | ret=SUCCESS |
2020-12-24 16:52:22,651 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null} | ret=SUCCESS |
2020-12-24 16:54:41,820 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/} | ret=SUCCESS |
2020-12-24 16:54:53,892 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:54:59,528 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:55:04,806 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:01:24,818 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/io_control/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:01:31,140 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/io_control/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:01:42,191 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/io_control/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:03:12,863 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=tmp/hadoop-yarn/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dineshchitlangia commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555211744
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
```suggestion
if (operationResult) {
LOG.debug("Add acl: {} to path: {} success!", getAcls(), getPath());
} else {
omMetrics.incNumBucketUpdateFails();
if (exception == null) {
LOG.error("Add acl {} to path {} failed, because acl already exist",
getAcls(), getPath());
exception = new ACLAlreadyExistsException(); //we can create this new type of exception
} else {
LOG.error("Add acl {} to path {} failed!", getAcls(), getPath(),
exception);
}
}
}
auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
exception, getOmRequest().getUserInfo()));
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555662391
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
Hi @dineshchitlangia, I have the same idea to throw the ACLAlreadyExistsException exception when add an acl and ACL does not exist exception when remove an acl. I plan to put the logic in ozoneAclUtil.java so that we don't need to do the exception == null check in every ACL related request.
And currently, there are OzoneAclUtils.java which used by bucket, key and prefix ACL operation, and OmOzoneAclMap.java which used by volume. I'd like to unify them and do some refactor which will be quite a big patch. So leave it in HDDS-4650.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #1734:
URL: https://github.com/apache/ozone/pull/1734
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dineshchitlangia commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555211744
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
```suggestion
if (operationResult) {
LOG.debug("Add acl: {} to path: {} success!", getAcls(), getPath());
} else {
omMetrics.incNumBucketUpdateFails();
if (exception == null) {
LOG.error("Add acl {} to path {} failed, because acl already exist",
getAcls(), getPath());
exception = new ACLAlreadyExistsException(); //we can create this new type of exception
} else {
LOG.error("Add acl {} to path {} failed!", getAcls(), getPath(),
exception);
}
}
auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
exception, getOmRequest().getUserInfo()));
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555662391
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
Hi @dineshchitlangia, I have the same idea to throw the ACLAlreadyExistsException exception when add an acl and ACL does not exist exception when remove an acl. I plan to put the logic in ozoneAclUtil.java so that we don't need to do the exception == null check in every ACL related request.
And currently, there are OzoneAclUtils.java which used by bucket, key and prefix ACL operation, and OmOzoneAclMap.java which used by volume. I'd like to unify them and do some refactor which will be quite a big patch.
Will do it in HDDS-4650.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#issuecomment-755908175
Thanks @cxorm, @xiaoyuyao and @adoroszlai for the code review. I have updated new commits to address the comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r553130648
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
@adoroszlai , thanks for the review and feedback. Basically, I think ACL error handling with both Result and Exception is complex. I fired HDDS-4650m, ACL related error handling refactor, to cleanly resolve this issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#issuecomment-750819896
ACL logs with patch applied.
2020-12-24 16:47:47,191 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:14,929 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:33,259 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:55,554 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:49:11,722 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:49:25,417 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:52:05,233 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null} | ret=SUCCESS |
2020-12-24 16:52:22,651 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null} | ret=SUCCESS |
2020-12-24 16:54:41,820 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/} | ret=SUCCESS |
2020-12-24 16:54:53,892 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:54:59,528 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:55:04,806 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:01:24,818 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/io_control/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:01:31,140 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/io_control/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:01:42,191 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/io_control/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 17:03:12,863 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=tmp/hadoop-yarn/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r551647922
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java
##########
@@ -88,6 +92,12 @@ public String getVolumeName() {
return volumeName;
}
+ @Override
+ OzoneObjInfo getObjectInfo() {
+ return OzoneObjInfo.fromProtobuf(
+ getOmRequest().getSetAclRequest().getObj());
Review comment:
NIT: this can be put into the constructor with a member variable so that the protobuf conversion is done only once and the getter() simply return the member variable. Same apply to other places.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi edited a comment on pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
ChenSammi edited a comment on pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#issuecomment-750819896
ACL logs with patch applied.
Volume
**2020-12-24 16:47:47,191 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:14,929 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:48:33,259 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=volume, storageType=ozone, volume=s3v, bucket=null, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
Bucket
2020-12-24 16:48:55,554 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:49:11,722 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:49:25,417 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:52:05,233 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=bucket, storageType=ozone, volume=s3v, bucket=ozone, key=null} | ret=SUCCESS |
Key
2020-12-24 16:54:41,820 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=GET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/} | ret=SUCCESS |
2020-12-24 16:54:53,892 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=ADD_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:54:59,528 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=SET_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |
2020-12-24 16:55:04,806 | INFO | OMAudit | user=root | ip=10.120.110.183 | op=REMOVE_ACL {resourceType=key, storageType=ozone, volume=s3v, bucket=ozone, key=test1/, acl=[user:apple:a[ACCESS]]} | ret=SUCCESS |**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r551789557
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
`buildAuditMessage` determines success/failure based on whether `exception` is `null`. But here we can also have failure without exception, indicated by `operationResult = false`. I think audit log should indicate failure in this case.
https://github.com/apache/ozone/blob/c3f6b4b96a5c50834df223d36d2fe691da0e67ee/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java#L124-L126
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dineshchitlangia commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555127920
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
@ChenSammi Thank you for working on this. To address this situation, how about this idea:
1. Let's not call the auditlog() in the first line of the current method.
2. We can do something like this:
` if (operationResult) {
LOG.debug("Add acl: {} to path: {} success!", getAcls(), getPath());
} else {
omMetrics.incNumBucketUpdateFails();
if (exception == null) {
LOG.error("Add acl {} to path {} failed, because acl already exist",
getAcls(), getPath());
} else {
LOG.error("Add acl {} to path {} failed!", getAcls(), getPath(),
exception);
exception = null; //
}
}
auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
exception, getOmRequest().getUserInfo()));
}`
This will ensure this covers the edge case mentioned by @adoroszlai and we won't have to modify the Audit Log base framework since this scenario is for a very limited number of occurrence in auditing ACL ops only.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #1734: HDDS-4621. Set, add and remove ACL have no audit logs
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555173187
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
@dineshchitlangia wouldn't `exception=null` have the effect of always logging `SUCCESS` in audit?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org