You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "priyeshkaratha (via GitHub)" <gi...@apache.org> on 2025/04/09 06:53:39 UTC
[PR] HDDS-12704. adding missing audit logs for operations. [ozone]
priyeshkaratha opened a new pull request, #8251:
URL: https://github.com/apache/ozone/pull/8251
## What changes were proposed in this pull request?
Currently, SCMClientProtocolServer exhibits different behaviors across various operations. In this PR, I have addressed two key issues:
1. **Auditing Enhancements**: Added auditing for operations where it was previously missing.
2. **Admin Access Checks**: Identified and fixed cases where admin access checks were not properly handled due to missing try-catch blocks, which also led to auditing being skipped. These have now been correctly implemented to ensure consistent auditing and access control.
## What is the link to the Apache JIRA
[HDDS-12704](https://issues.apache.org/jira/browse/HDDS-12704)
## How was this patch tested?
There are no functional changes introduced in this PR. I have verified that all existing integration and other test cases pass successfully through [CI actions](https://github.com/priyeshkaratha/ozone/actions/runs/14350106116).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2046242741
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +807,34 @@ public Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("pipelineID", String.valueOf(pipelineID));
+ try {
+ Pipeline pipeline = scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
Review Comment:
Sorry, I just noticed that this logs `PipelineID` as proto message, which does not look nice in the log:
```
2025-04-15 11:00:56,041 | INFO | SCMAudit | user=recon/recon@EXAMPLE.COM | ip=172.20.0.2 | op=GET_PIPELINE {pipelineID=id: "84c958af-43c3-42cd-943f-5ab172cfd6b2"
uuid128 {
mostSigBits: -8878467680604831027
leastSigBits: -7764387514381510990
}
} | ret=SUCCESS |
```
Please use `pipelineID.getId()` for audit map value, like it is for `activatePipeline` and other similar methods.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048277225
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
Review Comment:
Keeping Exception since it is better because it handles unchecked exceptions
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2037195161
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -382,10 +411,14 @@ public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
Review Comment:
Since auditing is done inside a loop and we're not stopping on exceptions, we need to initialize a separate audit map here, apart from the one used for successful cases. Handled in other place as per your recommendation.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048304325
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to decommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> recommissionNodes(List<String> nodes)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().recommissionNodes(nodes);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().recommissionNodes(nodes);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
Review Comment:
Thanks @adoroszlai, will keep in mind.
What I refer here is I think we can return the result retrieved from `scm.getScmDecommissionManager().recommissionNodes(nodes)` directly, as it doesnt throw exception.
```java
try {
...
AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
SCMAction.DECOMMISSION_NODES, auditMap));
return scm.getScmDecommissionManager().recommissionNodes(nodes);
} catch ...
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048223598
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -605,16 +643,21 @@ public List<HddsProtos.Node> queryNode(
.addNodeOperationalStates(ns.getOperationalState())
.build());
} catch (NodeNotFoundException e) {
- throw new IOException(
- "An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, e));
+ throw new IOException("An unexpected error occurred querying the NodeStatus", e);
Review Comment:
No, we want to log the original exception, not the wrapper.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2037200941
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -382,10 +411,14 @@ public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
}
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerIDs", containerIDs.toString())));
Review Comment:
here on IOException, its continue on loop for next entries and there are chances for success and failures, so should I add list of ids that is success into success audit logs.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048308772
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to decommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> recommissionNodes(List<String> nodes)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().recommissionNodes(nodes);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().recommissionNodes(nodes);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
Review Comment:
Thanks @peterxcli for the clarification. Any call may throw unchecked exceptions, so it's better to only have `return` statement after logging 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048236972
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to decommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> recommissionNodes(List<String> nodes)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().recommissionNodes(nodes);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().recommissionNodes(nodes);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
Review Comment:
This excerpt is unlike the one immediately before it. GitHub may not display your comments in the order you intended. So comments like "same as above" or "ditto" are hard to understand for the author.
I think it's better to mention in a single comment that there are similar issues, with specific code references if there aren't too many and you have the time.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048224531
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -230,43 +230,55 @@ public void join() throws InterruptedException {
public ContainerWithPipeline allocateContainer(HddsProtos.ReplicationType
replicationType, HddsProtos.ReplicationFactor factor,
String owner) throws IOException {
- if (scm.getScmContext().isInSafeMode()) {
- throw new SCMException("SafeModePrecheck failed for allocateContainer",
- ResultCodes.SAFE_MODE_EXCEPTION);
+
+ Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("replicationType", String.valueOf(replicationType));
+ auditMap.put("factor", String.valueOf(factor));
+ auditMap.put("owner", String.valueOf(owner));
+
+ try {
+ if (scm.getScmContext().isInSafeMode()) {
+ throw new SCMException("SafeModePrecheck failed for allocateContainer",
+ ResultCodes.SAFE_MODE_EXCEPTION);
+ }
+ getScm().checkAdminAccess(getRemoteUser(), false);
+ final ContainerInfo container = scm.getContainerManager()
+ .allocateContainer(
+ ReplicationConfig.fromProtoTypeAndFactor(replicationType, factor),
+ owner);
+ final Pipeline pipeline = scm.getPipelineManager()
+ .getPipeline(container.getPipelineID());
+ ContainerWithPipeline cp = new ContainerWithPipeline(container, pipeline);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.ALLOCATE_CONTAINER, auditMap)
+ );
+ return cp;
+ } catch (Exception ex) {
Review Comment:
Let's keep catching `Exception`, which is better because it handles unchecked exceptions, too. We may want to change occurrences of `catch (IOException)` to `catch (Exception)` instead.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2043734071
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1421,17 +1607,30 @@ public DecommissionScmResponseProto decommissionScm(
getScm().checkAdminAccess(getRemoteUser(), false);
decommissionScmResponseBuilder
.setSuccess(scm.removePeerFromHARing(scmId));
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId", scmId)));
} catch (IOException ex) {
decommissionScmResponseBuilder
.setSuccess(false)
.setErrorMsg(ex.getMessage());
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId", scmId), ex));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/ozone/audit/SCMAction.java:
##########
@@ -54,7 +54,20 @@ public enum SCMAction implements AuditAction {
GET_REPLICATION_MANAGER_REPORT,
RESET_DELETED_BLOCK_RETRY_COUNT,
TRANSFER_LEADERSHIP,
- GET_FAILED_DELETED_BLOCKS_TRANSACTION;
+ GET_CONTAINER_REPLICAS,
+ GET_FAILED_DELETED_BLOCKS_TRANSACTION,
Review Comment:
Note: usually enum values should be added at the end, keeping existing ones unchanged, for compatibility. In this case I don't think it matters, since values of `SCMAction` are not persisted or sent to remote node.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +808,32 @@ public Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ try {
+ Pipeline pipeline = scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID", String.valueOf(pipelineID))));
+ return pipeline;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID", String.valueOf(pipelineID)), ex));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -903,8 +983,8 @@ public List<DeletedBlocksTransactionInfo> getFailedDeletedBlockTxn(int count,
@Override
public int resetDeletedBlockRetryCount(List<Long> txIDs) throws IOException {
Map<String, String> auditMap = Maps.newHashMap();
- getScm().checkAdminAccess(getRemoteUser(), false);
try {
+ getScm().checkAdminAccess(getRemoteUser(), false);
int count = scm.getScmBlockManager().getDeletedBlockLog().
resetCount(txIDs);
auditMap.put("txIDs", txIDs.toString());
Review Comment:
Move `auditMap.put` before `try` block. Also in `closePipeline`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -815,33 +901,29 @@ public ScmInfo getScmInfo() {
.setClusterId(scm.getScmStorageConfig().getClusterID())
.setScmId(scm.getScmStorageConfig().getScmId())
.setPeerRoles(scm.getScmHAManager().getRatisServer().getRatisRoles());
- return builder.build();
+ ScmInfo info = builder.build();
Review Comment:
nit:
```suggestion
ScmInfo info = builder.build();
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,45 +670,72 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(SCMAction.QUERY_NODE,
+ Collections.singletonMap("uuid", String.valueOf(uuid)), ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(SCMAction.QUERY_NODE,
+ Collections.singletonMap("uuid", String.valueOf(uuid))));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1421,17 +1607,30 @@ public DecommissionScmResponseProto decommissionScm(
getScm().checkAdminAccess(getRemoteUser(), false);
decommissionScmResponseBuilder
.setSuccess(scm.removePeerFromHARing(scmId));
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId", scmId)));
} catch (IOException ex) {
decommissionScmResponseBuilder
.setSuccess(false)
.setErrorMsg(ex.getMessage());
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId", scmId), ex));
}
return decommissionScmResponseBuilder.build();
}
@Override
public String getMetrics(String query) throws IOException {
- FetchMetrics fetchMetrics = new FetchMetrics();
- return fetchMetrics.getMetrics(query);
+ try {
+ FetchMetrics fetchMetrics = new FetchMetrics();
+ String metrics = fetchMetrics.getMetrics(query);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_METRICS, Collections.singletonMap("query", query)));
+ return metrics;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_METRICS, Collections.singletonMap("query", query), ex));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +808,32 @@ public Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ try {
+ Pipeline pipeline = scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID", String.valueOf(pipelineID))));
+ return pipeline;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID", String.valueOf(pipelineID)), ex));
Review Comment:
This should be `SCMAction.GET_PIPELINE`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1270,62 +1402,116 @@ public List<HddsProtos.DatanodeUsageInfoProto> getDatanodeUsageInfo(
boolean mostUsed, int count, int clientVersion)
throws IOException, IllegalArgumentException {
- // check admin authorisation
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("mostUsed", String.valueOf(mostUsed));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
+
try {
+ // check admin authorisation
getScm().checkAdminAccess(getRemoteUser(), true);
- } catch (IOException e) {
- LOG.error("Authorization failed", e);
- throw e;
- }
+ if (count < 1) {
+ throw new IllegalArgumentException("The specified parameter count must " +
+ "be an integer greater than zero.");
+ }
+ List<DatanodeUsageInfo> datanodeUsageInfoList =
+ scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
- if (count < 1) {
- throw new IllegalArgumentException("The specified parameter count must " +
- "be an integer greater than zero.");
- }
+ // if count is greater than the size of list containing healthy,
+ // in-service nodes, just set count to that size
+ if (count > datanodeUsageInfoList.size()) {
+ count = datanodeUsageInfoList.size();
+ }
- List<DatanodeUsageInfo> datanodeUsageInfoList =
- scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
+ // return count number of DatanodeUsageInfoProto
+ List<HddsProtos.DatanodeUsageInfoProto> result = datanodeUsageInfoList.stream()
+ .map(each -> each.toProto(clientVersion))
+ .limit(count)
+ .collect(Collectors.toList());
- // if count is greater than the size of list containing healthy,
- // in-service nodes, just set count to that size
- if (count > datanodeUsageInfoList.size()) {
- count = datanodeUsageInfoList.size();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap));
+ return result;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap, ex));
+ throw ex;
}
-
- // return count number of DatanodeUsageInfoProto
- return datanodeUsageInfoList.stream()
- .map(each -> each.toProto(clientVersion))
- .limit(count)
- .collect(Collectors.toList());
}
@Override
public Token<?> getContainerToken(ContainerID containerID)
throws IOException {
- UserGroupInformation remoteUser = getRemoteUser();
- getScm().checkAdminAccess(remoteUser, true);
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("containerID", String.valueOf(containerID));
+ try {
+ UserGroupInformation remoteUser = getRemoteUser();
+ getScm().checkAdminAccess(getRemoteUser(), true);
- return scm.getContainerTokenGenerator()
- .generateToken(remoteUser.getUserName(), containerID);
+ Token<?> token = scm.getContainerTokenGenerator()
Review Comment:
nit:
```suggestion
Token<?> token = scm.getContainerTokenGenerator()
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -863,17 +945,14 @@ public void transferLeadership(String newLeaderId)
RatisHelper.transferRatisLeadership(scm.getConfiguration(), group,
targetPeerId, tlsConfig);
+
} catch (Exception ex) {
Review Comment:
nit: let's not add empty lines at the end of blocks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048182925
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -230,43 +230,55 @@ public void join() throws InterruptedException {
public ContainerWithPipeline allocateContainer(HddsProtos.ReplicationType
replicationType, HddsProtos.ReplicationFactor factor,
String owner) throws IOException {
- if (scm.getScmContext().isInSafeMode()) {
- throw new SCMException("SafeModePrecheck failed for allocateContainer",
- ResultCodes.SAFE_MODE_EXCEPTION);
+
+ Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("replicationType", String.valueOf(replicationType));
+ auditMap.put("factor", String.valueOf(factor));
+ auditMap.put("owner", String.valueOf(owner));
+
+ try {
+ if (scm.getScmContext().isInSafeMode()) {
+ throw new SCMException("SafeModePrecheck failed for allocateContainer",
+ ResultCodes.SAFE_MODE_EXCEPTION);
+ }
+ getScm().checkAdminAccess(getRemoteUser(), false);
+ final ContainerInfo container = scm.getContainerManager()
+ .allocateContainer(
+ ReplicationConfig.fromProtoTypeAndFactor(replicationType, factor),
+ owner);
+ final Pipeline pipeline = scm.getPipelineManager()
+ .getPipeline(container.getPipelineID());
+ ContainerWithPipeline cp = new ContainerWithPipeline(container, pipeline);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.ALLOCATE_CONTAINER, auditMap)
+ );
+ return cp;
+ } catch (Exception ex) {
Review Comment:
nit
```suggestion
} catch (IOException ex) {
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -605,16 +643,21 @@ public List<HddsProtos.Node> queryNode(
.addNodeOperationalStates(ns.getOperationalState())
.build());
} catch (NodeNotFoundException e) {
- throw new IOException(
- "An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, e));
+ throw new IOException("An unexpected error occurred querying the NodeStatus", e);
}
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
Review Comment:
same question.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -815,33 +902,29 @@ public ScmInfo getScmInfo() {
.setClusterId(scm.getScmStorageConfig().getClusterID())
.setScmId(scm.getScmStorageConfig().getScmId())
.setPeerRoles(scm.getScmHAManager().getRatisServer().getRatisRoles());
- return builder.build();
+ ScmInfo info = builder.build();
Review Comment:
please remove the above `boolean auditSuccess = true;` (I cant comment at there)
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to decommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> recommissionNodes(List<String> nodes)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().recommissionNodes(nodes);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().recommissionNodes(nodes);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
Review Comment:
ditto
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -605,16 +643,21 @@ public List<HddsProtos.Node> queryNode(
.addNodeOperationalStates(ns.getOperationalState())
.build());
} catch (NodeNotFoundException e) {
- throw new IOException(
- "An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, e));
+ throw new IOException("An unexpected error occurred querying the NodeStatus", e);
Review Comment:
Should this be:
```suggestion
IOException ex = new IOException("An unexpected error occurred querying the NodeStatus", e);
AUDIT.logReadFailure(buildAuditMessageForFailure(
SCMAction.QUERY_NODE, auditMap, ex));
throw ex
```
or just keep it?
I do see different behaviour on this. https://github.com/apache/ozone/pull/8251/files#diff-02d31f948c607eae697c655f6ec08e36eb5aefec42f57dc53c50751aaadda2e4R672-R680
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -326,22 +336,35 @@ public ContainerWithPipeline getContainerWithPipeline(long containerID)
public List<HddsProtos.SCMContainerReplicaProto> getContainerReplicas(
long containerId, int clientVersion) throws IOException {
List<HddsProtos.SCMContainerReplicaProto> results = new ArrayList<>();
+ Map<String, String> auditMap = new HashMap<>();
+ auditMap.put("containerId", String.valueOf(containerId));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
- Set<ContainerReplica> replicas = getScm().getContainerManager()
- .getContainerReplicas(ContainerID.valueOf(containerId));
- for (ContainerReplica r : replicas) {
- results.add(
- HddsProtos.SCMContainerReplicaProto.newBuilder()
- .setContainerID(containerId)
- .setState(r.getState().toString())
- .setDatanodeDetails(r.getDatanodeDetails().toProto(clientVersion))
- .setBytesUsed(r.getBytesUsed())
- .setPlaceOfBirth(r.getOriginDatanodeId().toString())
- .setKeyCount(r.getKeyCount())
- .setSequenceID(r.getSequenceId())
- .setReplicaIndex(r.getReplicaIndex()).build()
- );
+ try {
+ Set<ContainerReplica> replicas = getScm().getContainerManager()
+ .getContainerReplicas(ContainerID.valueOf(containerId));
+ for (ContainerReplica r : replicas) {
+ results.add(
+ HddsProtos.SCMContainerReplicaProto.newBuilder()
+ .setContainerID(containerId)
+ .setState(r.getState().toString())
+ .setDatanodeDetails(r.getDatanodeDetails().toProto(clientVersion))
+ .setBytesUsed(r.getBytesUsed())
+ .setPlaceOfBirth(r.getOriginDatanodeId().toString())
+ .setKeyCount(r.getKeyCount())
+ .setSequenceID(r.getSequenceId())
+ .setReplicaIndex(r.getReplicaIndex()).build()
+ );
+ }
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_CONTAINER_REPLICAS,
+ auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_WITH_PIPELINE_BATCH,
+ auditMap));
Review Comment:
Should we also put this into the end of the try block, just like others?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
Review Comment:
```suggestion
} catch (IOException ex) {
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +807,34 @@ public Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
Review Comment:
This whole process doesn't throw any error.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -815,33 +902,29 @@ public ScmInfo getScmInfo() {
.setClusterId(scm.getScmStorageConfig().getClusterID())
.setScmId(scm.getScmStorageConfig().getScmId())
.setPeerRoles(scm.getScmHAManager().getRatisServer().getRatisRoles());
- return builder.build();
+ ScmInfo info = builder.build();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_SCM_INFO, null));
+ return info;
} catch (Exception ex) {
- auditSuccess = false;
- AUDIT.logReadFailure(
- buildAuditMessageForFailure(SCMAction.GET_SCM_INFO, null, ex)
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
Review Comment:
And I think this doest throw, too.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to decommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> recommissionNodes(List<String> nodes)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().recommissionNodes(nodes);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().recommissionNodes(nodes);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to recommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> startMaintenanceNodes(List<String> nodes,
int endInHours, boolean force) throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("endInHours", String.valueOf(endInHours));
+ auditMap.put("force", String.valueOf(force));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager()
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager()
Review Comment:
`scm.getScmDecommissionManager().startMaintenanceNodes` doesnt throw, too.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
Review Comment:
`scm.getScmDecommissionManager().decommissionNodes` doesnt throw exception.
```suggestion
AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
SCMAction.DECOMMISSION_NODES, auditMap));
return List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);;
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +807,34 @@ public Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("pipelineID", pipelineID.getId());
+ try {
+ Pipeline pipeline = scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_PIPELINE, auditMap));
+ return pipeline;
+ } catch (Exception ex) {
Review Comment:
```suggestion
} catch (PipelineNotFoundException ex) {
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048278979
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,52 +670,78 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.QUERY_NODE, auditMap));
return result;
}
@Override
public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("force", String.valueOf(force));
+
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().decommissionNodes(nodes, force);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to decommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> recommissionNodes(List<String> nodes)
throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager().recommissionNodes(nodes);
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager().recommissionNodes(nodes);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_NODES, auditMap));
+ return result;
} catch (Exception ex) {
- LOG.error("Failed to recommission nodes", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_NODES, auditMap, ex));
throw ex;
}
}
@Override
public List<DatanodeAdminError> startMaintenanceNodes(List<String> nodes,
int endInHours, boolean force) throws IOException {
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("nodes", String.valueOf(nodes));
+ auditMap.put("endInHours", String.valueOf(endInHours));
+ auditMap.put("force", String.valueOf(force));
try {
getScm().checkAdminAccess(getRemoteUser(), false);
- return scm.getScmDecommissionManager()
+ List<DatanodeAdminError> result = scm.getScmDecommissionManager()
Review Comment:
checking admin access may throw exception.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8251:
URL: https://github.com/apache/ozone/pull/8251#issuecomment-2803938840
Please remove duplicate `checkAdminAccess` (the one outside try block):
https://github.com/apache/ozone/blob/929d7178762ffcf92ffd77ac42be78f800aeb3be/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java#L744-L751
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048231932
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -326,22 +336,35 @@ public ContainerWithPipeline getContainerWithPipeline(long containerID)
public List<HddsProtos.SCMContainerReplicaProto> getContainerReplicas(
long containerId, int clientVersion) throws IOException {
List<HddsProtos.SCMContainerReplicaProto> results = new ArrayList<>();
+ Map<String, String> auditMap = new HashMap<>();
+ auditMap.put("containerId", String.valueOf(containerId));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
- Set<ContainerReplica> replicas = getScm().getContainerManager()
- .getContainerReplicas(ContainerID.valueOf(containerId));
- for (ContainerReplica r : replicas) {
- results.add(
- HddsProtos.SCMContainerReplicaProto.newBuilder()
- .setContainerID(containerId)
- .setState(r.getState().toString())
- .setDatanodeDetails(r.getDatanodeDetails().toProto(clientVersion))
- .setBytesUsed(r.getBytesUsed())
- .setPlaceOfBirth(r.getOriginDatanodeId().toString())
- .setKeyCount(r.getKeyCount())
- .setSequenceID(r.getSequenceId())
- .setReplicaIndex(r.getReplicaIndex()).build()
- );
+ try {
+ Set<ContainerReplica> replicas = getScm().getContainerManager()
+ .getContainerReplicas(ContainerID.valueOf(containerId));
+ for (ContainerReplica r : replicas) {
+ results.add(
+ HddsProtos.SCMContainerReplicaProto.newBuilder()
+ .setContainerID(containerId)
+ .setState(r.getState().toString())
+ .setDatanodeDetails(r.getDatanodeDetails().toProto(clientVersion))
+ .setBytesUsed(r.getBytesUsed())
+ .setPlaceOfBirth(r.getOriginDatanodeId().toString())
+ .setKeyCount(r.getKeyCount())
+ .setSequenceID(r.getSequenceId())
+ .setReplicaIndex(r.getReplicaIndex()).build()
+ );
+ }
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_CONTAINER_REPLICAS,
+ auditMap, ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_WITH_PIPELINE_BATCH,
+ auditMap));
Review Comment:
I agree, it's better to move success log and `return` into the `try` block. This makes it easier to spot problems where exception is not re-thrown (HDDS-12703).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8251:
URL: https://github.com/apache/ozone/pull/8251#issuecomment-2813713424
Thanks @priyeshkaratha for the patch, @peterxcli for the review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2037200941
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -382,10 +411,14 @@ public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
}
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerIDs", containerIDs.toString())));
Review Comment:
here on IOException, its continue on loop. So need to figure how to handle auditing here.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #8251:
URL: https://github.com/apache/ozone/pull/8251
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048230027
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -815,33 +902,29 @@ public ScmInfo getScmInfo() {
.setClusterId(scm.getScmStorageConfig().getClusterID())
.setScmId(scm.getScmStorageConfig().getScmId())
.setPeerRoles(scm.getScmHAManager().getRatisServer().getRatisRoles());
- return builder.build();
+ ScmInfo info = builder.build();
Review Comment:
Thanks for spotting this. There are two unused `auditSuccess` declarations:
https://github.com/apache/ozone/blob/d6fcef930ea38ff49e8ac79523afa76b3c1b0829/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java#L898
https://github.com/apache/ozone/blob/d6fcef930ea38ff49e8ac79523afa76b3c1b0829/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java#L921
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048277675
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +807,34 @@ public Pipeline createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("pipelineID", pipelineID.getId());
+ try {
+ Pipeline pipeline = scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_PIPELINE, auditMap));
+ return pipeline;
+ } catch (Exception ex) {
Review Comment:
Keeping Exception since it is better because it handles unchecked exceptions
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2048282562
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -815,33 +902,29 @@ public ScmInfo getScmInfo() {
.setClusterId(scm.getScmStorageConfig().getClusterID())
.setScmId(scm.getScmStorageConfig().getScmId())
.setPeerRoles(scm.getScmHAManager().getRatisServer().getRatisRoles());
- return builder.build();
+ ScmInfo info = builder.build();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_SCM_INFO, null));
+ return info;
} catch (Exception ex) {
- auditSuccess = false;
- AUDIT.logReadFailure(
- buildAuditMessageForFailure(SCMAction.GET_SCM_INFO, null, ex)
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
Review Comment:
Added for handling unhandled exceptions
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2037653908
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -377,15 +402,21 @@ public List<ContainerWithPipeline> getContainerWithPipelineBatch(
public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
List<Long> containerIDs) {
List<ContainerWithPipeline> cpList = new ArrayList<>();
+ List<Long> succeededContainers = new ArrayList<>();
for (Long containerID : containerIDs) {
try {
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
+ succeededContainers.add(containerID);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
}
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerIDs", succeededContainers.toString())));
return cpList;
}
Review Comment:
Thanks @adoroszlai . I have handled 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on PR #8251:
URL: https://github.com/apache/ozone/pull/8251#issuecomment-2811624649
@adoroszlai Updated PR with suggested changes. Thanks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. Add missing audit logs for SCM client operations [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on PR #8251:
URL: https://github.com/apache/ozone/pull/8251#issuecomment-2811843551
@peterxcli Thanks for your reviews updated PR with the suggested changes which are applicable
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on PR #8251:
URL: https://github.com/apache/ozone/pull/8251#issuecomment-2804513130
Thank you @adoroszlai for the detailed review. Addressed all the mentioned changes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2037315768
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -377,15 +402,21 @@ public List<ContainerWithPipeline> getContainerWithPipelineBatch(
public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
List<Long> containerIDs) {
List<ContainerWithPipeline> cpList = new ArrayList<>();
+ List<Long> succeededContainers = new ArrayList<>();
for (Long containerID : containerIDs) {
try {
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
+ succeededContainers.add(containerID);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
}
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerIDs", succeededContainers.toString())));
return cpList;
}
Review Comment:
I just realized that this is duplicate of `getContainerWithPipelineBatch`, intended as internal request exposed for Recon. Let's not add audit for now, we'll check later.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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
Re: [PR] HDDS-12704. adding missing audit logs for operations. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2034639687
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -215,10 +215,14 @@ public void stop() {
try {
LOG.info("Stopping the RPC server for Client Protocol");
getClientRpcServer().stop();
+
} catch (Exception ex) {
- LOG.error("Client Protocol RPC stop failed.", ex);
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.STOP_SERVER, null, ex));
}
IOUtils.cleanupWithLogger(LOG, scm.getScmNodeManager());
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.STOP_SERVER, null));
Review Comment:
Stopping the server is not a client operation, does not need to be audited.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1414,6 +1582,7 @@ public void close() throws IOException {
@Override
public DecommissionScmResponseProto decommissionScm(
String scmId) {
+
Review Comment:
nit: let's not add empty lines
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1270,60 +1409,89 @@ public List<HddsProtos.DatanodeUsageInfoProto> getDatanodeUsageInfo(
boolean mostUsed, int count, int clientVersion)
throws IOException, IllegalArgumentException {
- // check admin authorisation
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("mostUsed", String.valueOf(mostUsed));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
+
try {
+ // check admin authorisation
getScm().checkAdminAccess(getRemoteUser(), true);
- } catch (IOException e) {
- LOG.error("Authorization failed", e);
- throw e;
- }
+ if (count < 1) {
+ throw new IllegalArgumentException("The specified parameter count must " +
+ "be an integer greater than zero.");
+ }
+ List<DatanodeUsageInfo> datanodeUsageInfoList =
+ scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
- if (count < 1) {
- throw new IllegalArgumentException("The specified parameter count must " +
- "be an integer greater than zero.");
- }
+ // if count is greater than the size of list containing healthy,
+ // in-service nodes, just set count to that size
+ if (count > datanodeUsageInfoList.size()) {
+ count = datanodeUsageInfoList.size();
+ }
- List<DatanodeUsageInfo> datanodeUsageInfoList =
- scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
+ // return count number of DatanodeUsageInfoProto
+ List<HddsProtos.DatanodeUsageInfoProto> result = datanodeUsageInfoList.stream()
+ .map(each -> each.toProto(clientVersion))
+ .limit(count)
+ .collect(Collectors.toList());
- // if count is greater than the size of list containing healthy,
- // in-service nodes, just set count to that size
- if (count > datanodeUsageInfoList.size()) {
- count = datanodeUsageInfoList.size();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap));
+ return result;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap, ex));
+ throw ex;
}
-
- // return count number of DatanodeUsageInfoProto
- return datanodeUsageInfoList.stream()
- .map(each -> each.toProto(clientVersion))
- .limit(count)
- .collect(Collectors.toList());
}
@Override
public Token<?> getContainerToken(ContainerID containerID)
throws IOException {
- UserGroupInformation remoteUser = getRemoteUser();
- getScm().checkAdminAccess(remoteUser, true);
+ try {
+ UserGroupInformation remoteUser = getRemoteUser();
+ getScm().checkAdminAccess(getRemoteUser(), true);
- return scm.getContainerTokenGenerator()
- .generateToken(remoteUser.getUserName(), containerID);
+ Token<?> token = scm.getContainerTokenGenerator()
+ .generateToken(remoteUser.getUserName(), containerID);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID))));
+ return token;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
+ throw ex;
+ }
}
@Override
public long getContainerCount() throws IOException {
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_COUNT, null));
return scm.getContainerManager().getContainers().size();
}
@Override
public long getContainerCount(HddsProtos.LifeCycleState state)
throws IOException {
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_COUNT, Collections.singletonMap("state", String.valueOf(state))));
return scm.getContainerManager().getContainers(state).size();
Review Comment:
1. This could log success and then throw exception.
2. Failure is not logged.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1270,60 +1409,89 @@ public List<HddsProtos.DatanodeUsageInfoProto> getDatanodeUsageInfo(
boolean mostUsed, int count, int clientVersion)
throws IOException, IllegalArgumentException {
- // check admin authorisation
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("mostUsed", String.valueOf(mostUsed));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
+
try {
+ // check admin authorisation
getScm().checkAdminAccess(getRemoteUser(), true);
- } catch (IOException e) {
- LOG.error("Authorization failed", e);
- throw e;
- }
+ if (count < 1) {
+ throw new IllegalArgumentException("The specified parameter count must " +
+ "be an integer greater than zero.");
+ }
+ List<DatanodeUsageInfo> datanodeUsageInfoList =
+ scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
- if (count < 1) {
- throw new IllegalArgumentException("The specified parameter count must " +
- "be an integer greater than zero.");
- }
+ // if count is greater than the size of list containing healthy,
+ // in-service nodes, just set count to that size
+ if (count > datanodeUsageInfoList.size()) {
+ count = datanodeUsageInfoList.size();
+ }
- List<DatanodeUsageInfo> datanodeUsageInfoList =
- scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
+ // return count number of DatanodeUsageInfoProto
+ List<HddsProtos.DatanodeUsageInfoProto> result = datanodeUsageInfoList.stream()
+ .map(each -> each.toProto(clientVersion))
+ .limit(count)
+ .collect(Collectors.toList());
- // if count is greater than the size of list containing healthy,
- // in-service nodes, just set count to that size
- if (count > datanodeUsageInfoList.size()) {
- count = datanodeUsageInfoList.size();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap));
+ return result;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap, ex));
+ throw ex;
}
-
- // return count number of DatanodeUsageInfoProto
- return datanodeUsageInfoList.stream()
- .map(each -> each.toProto(clientVersion))
- .limit(count)
- .collect(Collectors.toList());
}
@Override
public Token<?> getContainerToken(ContainerID containerID)
throws IOException {
- UserGroupInformation remoteUser = getRemoteUser();
- getScm().checkAdminAccess(remoteUser, true);
+ try {
+ UserGroupInformation remoteUser = getRemoteUser();
+ getScm().checkAdminAccess(getRemoteUser(), true);
- return scm.getContainerTokenGenerator()
- .generateToken(remoteUser.getUserName(), containerID);
+ Token<?> token = scm.getContainerTokenGenerator()
+ .generateToken(remoteUser.getUserName(), containerID);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID))));
+ return token;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
+ throw ex;
+ }
}
@Override
public long getContainerCount() throws IOException {
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_COUNT, null));
return scm.getContainerManager().getContainers().size();
}
@Override
public long getContainerCount(HddsProtos.LifeCycleState state)
throws IOException {
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_COUNT, Collections.singletonMap("state", String.valueOf(state))));
return scm.getContainerManager().getContainers(state).size();
}
@Override
public List<ContainerInfo> getListOfContainers(
long startContainerID, int count, HddsProtos.LifeCycleState state)
throws IOException {
+
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("startContainerID", String.valueOf(startContainerID));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("state", String.valueOf(state));
+
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_CONTAINER, auditMap));
return scm.getContainerManager().getContainers(
ContainerID.valueOf(startContainerID), count, state);
Review Comment:
1. This could log success and then throw exception.
2. Failure is not logged.
Please wrap in `try-catch`, and make sure no processing is performed after `logReadSuccess`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1425,13 +1594,26 @@ public DecommissionScmResponseProto decommissionScm(
decommissionScmResponseBuilder
.setSuccess(false)
.setErrorMsg(ex.getMessage());
+ AUDIT.logWriteSuccess(buildAuditMessageForFailure(
Review Comment:
Both should be failure.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1425,13 +1594,26 @@ public DecommissionScmResponseProto decommissionScm(
decommissionScmResponseBuilder
.setSuccess(false)
.setErrorMsg(ex.getMessage());
+ AUDIT.logWriteSuccess(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId", scmId), ex));
}
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId", scmId)));
Review Comment:
Please move inside `try`. Instead of throwing exception, this method returns response for both success and failure. Thus, two audit log entries would be created for failure case.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -605,10 +653,14 @@ public List<HddsProtos.Node> queryNode(
.addNodeOperationalStates(ns.getOperationalState())
.build());
} catch (NodeNotFoundException e) {
- throw new IOException(
- "An unexpected error occurred querying the NodeStatus", e);
+ IOException ex = new IOException("An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.QUERY_NODE, auditMap, ex));
Review Comment:
Pass the original exception (`NodeNotFoundException e`) to audit instead of the constructed one (`IOException ex`).
nit: let's avoid unnecessary change (i.e. renaming `e -> ex`)
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1270,60 +1409,89 @@ public List<HddsProtos.DatanodeUsageInfoProto> getDatanodeUsageInfo(
boolean mostUsed, int count, int clientVersion)
throws IOException, IllegalArgumentException {
- // check admin authorisation
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("mostUsed", String.valueOf(mostUsed));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
+
try {
+ // check admin authorisation
getScm().checkAdminAccess(getRemoteUser(), true);
- } catch (IOException e) {
- LOG.error("Authorization failed", e);
- throw e;
- }
+ if (count < 1) {
+ throw new IllegalArgumentException("The specified parameter count must " +
+ "be an integer greater than zero.");
+ }
+ List<DatanodeUsageInfo> datanodeUsageInfoList =
+ scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
- if (count < 1) {
- throw new IllegalArgumentException("The specified parameter count must " +
- "be an integer greater than zero.");
- }
+ // if count is greater than the size of list containing healthy,
+ // in-service nodes, just set count to that size
+ if (count > datanodeUsageInfoList.size()) {
+ count = datanodeUsageInfoList.size();
+ }
- List<DatanodeUsageInfo> datanodeUsageInfoList =
- scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
+ // return count number of DatanodeUsageInfoProto
+ List<HddsProtos.DatanodeUsageInfoProto> result = datanodeUsageInfoList.stream()
+ .map(each -> each.toProto(clientVersion))
+ .limit(count)
+ .collect(Collectors.toList());
- // if count is greater than the size of list containing healthy,
- // in-service nodes, just set count to that size
- if (count > datanodeUsageInfoList.size()) {
- count = datanodeUsageInfoList.size();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap));
+ return result;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap, ex));
+ throw ex;
}
-
- // return count number of DatanodeUsageInfoProto
- return datanodeUsageInfoList.stream()
- .map(each -> each.toProto(clientVersion))
- .limit(count)
- .collect(Collectors.toList());
}
@Override
public Token<?> getContainerToken(ContainerID containerID)
throws IOException {
- UserGroupInformation remoteUser = getRemoteUser();
- getScm().checkAdminAccess(remoteUser, true);
+ try {
+ UserGroupInformation remoteUser = getRemoteUser();
+ getScm().checkAdminAccess(getRemoteUser(), true);
- return scm.getContainerTokenGenerator()
- .generateToken(remoteUser.getUserName(), containerID);
+ Token<?> token = scm.getContainerTokenGenerator()
+ .generateToken(remoteUser.getUserName(), containerID);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID))));
+ return token;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
+ throw ex;
+ }
}
@Override
public long getContainerCount() throws IOException {
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_COUNT, null));
return scm.getContainerManager().getContainers().size();
Review Comment:
1. This could log success and then throw exception.
2. Failure is not logged.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -382,10 +411,14 @@ public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
}
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerIDs", containerIDs.toString())));
Review Comment:
Please move inside `try`, otherwise two audit entries will be created in case of failure.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,45 +679,72 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(SCMAction.QUERY_NODE_BY_UUID,
Review Comment:
`QUERY_NODE` action can be used, instead of introducing another one (`QUERY_NODE_BY_UUID`). Parameters will indicate the specific method invoked.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -382,10 +411,14 @@ public List<ContainerWithPipeline> getExistContainerWithPipelinesInBatch(
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_EXIST_CONTAINER_WITH_PIPELINE_BATCH,
+ Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
Review Comment:
nit: Please store map in variable to reduce duplication.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -986,35 +1106,47 @@ public ReplicationManagerReport getReplicationManagerReport() {
@Override
public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
IOException {
- // check admin authorization
try {
+ // check admin authorization
getScm().checkAdminAccess(getRemoteUser(), false);
- } catch (IOException e) {
- LOG.error("Authorization failed for finalize scm upgrade", e);
- throw e;
+ // TODO HDDS-6762: Return to the client once the FINALIZATION_STARTED
+ // checkpoint has been crossed and continue finalizing asynchronously.
+ StatusAndMessages result = scm.getFinalizationManager().finalizeUpgrade(upgradeClientID);
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.FINALIZE_SCM_UPGRADE, Collections.singletonMap("upgradeClientID", upgradeClientID)));
Review Comment:
nit: please store audit map in variable to avoid duplication.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1270,60 +1409,89 @@ public List<HddsProtos.DatanodeUsageInfoProto> getDatanodeUsageInfo(
boolean mostUsed, int count, int clientVersion)
throws IOException, IllegalArgumentException {
- // check admin authorisation
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("mostUsed", String.valueOf(mostUsed));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
+
try {
+ // check admin authorisation
getScm().checkAdminAccess(getRemoteUser(), true);
- } catch (IOException e) {
- LOG.error("Authorization failed", e);
- throw e;
- }
+ if (count < 1) {
+ throw new IllegalArgumentException("The specified parameter count must " +
+ "be an integer greater than zero.");
+ }
+ List<DatanodeUsageInfo> datanodeUsageInfoList =
+ scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
- if (count < 1) {
- throw new IllegalArgumentException("The specified parameter count must " +
- "be an integer greater than zero.");
- }
+ // if count is greater than the size of list containing healthy,
+ // in-service nodes, just set count to that size
+ if (count > datanodeUsageInfoList.size()) {
+ count = datanodeUsageInfoList.size();
+ }
- List<DatanodeUsageInfo> datanodeUsageInfoList =
- scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
+ // return count number of DatanodeUsageInfoProto
+ List<HddsProtos.DatanodeUsageInfoProto> result = datanodeUsageInfoList.stream()
+ .map(each -> each.toProto(clientVersion))
+ .limit(count)
+ .collect(Collectors.toList());
- // if count is greater than the size of list containing healthy,
- // in-service nodes, just set count to that size
- if (count > datanodeUsageInfoList.size()) {
- count = datanodeUsageInfoList.size();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap));
+ return result;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap, ex));
+ throw ex;
}
-
- // return count number of DatanodeUsageInfoProto
- return datanodeUsageInfoList.stream()
- .map(each -> each.toProto(clientVersion))
- .limit(count)
- .collect(Collectors.toList());
}
@Override
public Token<?> getContainerToken(ContainerID containerID)
throws IOException {
- UserGroupInformation remoteUser = getRemoteUser();
- getScm().checkAdminAccess(remoteUser, true);
+ try {
+ UserGroupInformation remoteUser = getRemoteUser();
+ getScm().checkAdminAccess(getRemoteUser(), true);
- return scm.getContainerTokenGenerator()
- .generateToken(remoteUser.getUserName(), containerID);
+ Token<?> token = scm.getContainerTokenGenerator()
+ .generateToken(remoteUser.getUserName(), containerID);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID))));
+ return token;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_CONTAINER_TOKEN, Collections.singletonMap("containerID", String.valueOf(containerID)), ex));
Review Comment:
Please store audit map in local variable to reduce duplication.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -579,10 +612,15 @@ public void deleteContainer(long containerID) throws IOException {
@Override
public Map<String, List<ContainerID>> getContainersOnDecomNode(DatanodeDetails dn) throws IOException {
+ Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("datanodeDetails", String.valueOf(dn));
try {
- return scm.getScmDecommissionManager().getContainersPendingReplication(dn);
- } catch (NodeNotFoundException e) {
- throw new IOException("Failed to get containers list. Unable to find required node", e);
+ Map<String, List<ContainerID>> result = scm.getScmDecommissionManager().getContainersPendingReplication(dn);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(SCMAction.GET_CONTAINERS_ON_DECOM_NODE, auditMap));
+ return result;
+ } catch (NodeNotFoundException ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(SCMAction.GET_CONTAINERS_ON_DECOM_NODE, auditMap, ex));
+ throw new IOException("Failed to get containers list. Unable to find required node", ex);
Review Comment:
nit: let's avoid unnecessary change (i.e. renaming `e -> ex`)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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