Add audit hooks for inter-node user data transfers - #18569
Conversation
HTHou
left a comment
There was a problem hiding this comment.
Main findings are inline. Please also add behavior-level tests that capture emitted events for success, remote-status failure, exception/retry, and source/destination direction, plus tests proving that a throwing audit handler never changes the transfer outcome and that audit-log writes do not recursively generate USER_DATA_TRANSFER events. The current tests only cover the event value object and the PlanNode classifier.
|
Addressed the latest audit review feedback in 85ff1e2:
Validation passed: focused consensus/DataNode tests (10 tests), full English reactor |
…ta-transfer # Conflicts: # iotdb-core/node-commons/src/main/i18n/en/org/apache/iotdb/commons/i18n/CommonMessages.java # iotdb-core/node-commons/src/main/i18n/zh/org/apache/iotdb/commons/i18n/CommonMessages.java
| } catch (RuntimeException ignored) { | ||
| // Audit recording must not affect consensus replication. | ||
| } |
There was a problem hiding this comment.
Leave a warn log for debugging?
There was a problem hiding this comment.
Addressed in 9e6cbee. Audit callback/classification RuntimeExceptions remain isolated from consensus replication, but now emit a WARN with the stack trace for diagnosis. The new operator-facing message is localized in both en/zh.
| public static boolean containsUserData(ConsensusGroupId consensusGroupId) { | ||
| if (!(consensusGroupId instanceof DataRegionId)) { | ||
| return false; | ||
| } | ||
| final DataRegion dataRegion = | ||
| StorageEngine.getInstance().getDataRegion((DataRegionId) consensusGroupId); | ||
| return dataRegion != null && containsUserData(dataRegion.getDatabaseName()); | ||
| } |
There was a problem hiding this comment.
Is it reasonable to exclude schema regions?
Attributes also seem to be part of the user data.
There was a problem hiding this comment.
Agreed that schema and attribute data are user data. In IoTDB, SchemaRegion supports Ratis (and SimpleConsensus when the replication factor is 1); IoTConsensus and IoTConsensusV2 are rejected for SchemaRegion, and this PR wires the IoTConsensus classifier only into DataRegionConsensusImpl. The Ratis-side transfer audit hook needs to be implemented inside Ratis and will be handled under RATIS-2681, so this PR intentionally does not add a non-functional SchemaRegion classifier branch.
| } else if (request instanceof IoTConsensusRequest) { | ||
| planNode = WALEntry.deserializeForConsensus(request.serializeToByteBuffer().duplicate()); | ||
| } else if (request instanceof ByteBufferConsensusRequest) { | ||
| planNode = PlanNodeType.deserialize(request.serializeToByteBuffer().duplicate()); |
There was a problem hiding this comment.
Will the cost be too high for these branches?
Can we somehow store and encode the flag in IConsensusRequest?
There was a problem hiding this comment.
Addressed in 9e6cbee. The live sender path classifies a PlanNode once and stores the bit on IndexedConsensusRequest. During WAL reconstruction, the bit is now restored directly from WALEntryType, then combined with the consensus-group classifier to preserve the audit-database exclusion. The remote receiver no longer classifies the serialized request because auditing is sender-side. This removes the IoTConsensusRequest/ByteBufferConsensusRequest serialization and deserialization branches, without adding a wire-format field. A test also verifies that a generic IConsensusRequest is not serialized solely for audit classification.
| final TSStatus failedStatus = | ||
| status.stream() | ||
| .filter(tsStatus -> tsStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) | ||
| .findFirst() | ||
| .orElse(null); | ||
| connector.recordUserDataTransferAudit( | ||
| failedStatus == null, | ||
| failedStatus == null ? null : String.valueOf(failedStatus.getCode()), | ||
| null); | ||
| transferAuditRecorded = true; |
There was a problem hiding this comment.
Not sure if it is ok to report only the first failure.
Maybe we should conclude/concat the other failures too?
There was a problem hiding this comment.
I kept one representative error intentionally and clarified it in 9e6cbee. The batch RPC is one physical transfer attempt, so it produces one audit event. The event result is failure if any sub-response fails, and the first failed status code fills the minimum schema single error value. Concatenating every sub-status would make the audit payload grow with batch size; detailed per-item failures are still processed by the existing status handler.
| @Override | ||
| public void onError(final Exception exception) { | ||
| if (!transferAuditRecorded) { | ||
| connector.recordUserDataTransferAudit(false, null, exception); | ||
| transferAuditRecorded = true; | ||
| } |
There was a problem hiding this comment.
The same here, not sure if we should only record the first one.
There was a problem hiding this comment.
Clarified in 9e6cbee. onError receives one exception for that physical RPC attempt. A retry is sent with a new handler and therefore emits a new audit event. transferAuditRecorded only prevents double-recording when onComplete has already recorded the response and later response-processing code throws into onError.
Description
Add sender-side audit hooks for inter-node user-data transfers required by the FDP_ITT.1 audit requirements from GB/T 20273-2019.
This PR only adds audit event modeling, audit hooks, and routing through the existing audit logger. It does not introduce a new audit sink, persistence mechanism, protection mechanism, or policy implementation.
Audit record
Each transfer attempt records only:
The event is routed as
AuditEventType.USER_DATA_TRANSFER. Payload contents are never retained.Instrumented transfer paths
Review follow-ups
Explicitly out of scope
This PR has:
Verification
mvn spotless:apply -pl iotdb-core/node-commons,iotdb-core/consensus,iotdb-core/datanodemvn test-compile -DskipTestsmvn test-compile -DskipTests -P with-zh-localegit diff --checkAll commands above pass locally.