HTHou commented on code in PR #18569:
URL: https://github.com/apache/iotdb/pull/18569#discussion_r3911515501
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/audit/AbstractAuditLogger.java:
##########
@@ -136,4 +138,53 @@ public void recordTrustedChannelFailureAuditLogIfNecessary(
RECORDING_TRUSTED_CHANNEL_FAILURE.remove();
}
}
+
+ /** Records one user-data transfer attempt without retaining any transferred
payload. */
+ public void recordUserDataTransferAuditLog(UserDataTransferAuditEvent event)
{
+ if (!IS_AUDIT_LOG_ENABLED
+ || event == null
+ || event.getInitiator() == null
+ || event.getSource() == null
+ || event.getTarget() == null
+ || Boolean.TRUE.equals(RECORDING_USER_DATA_TRANSFER.get())) {
Review Comment:
[P1] This guard only covers the caller thread. In TimechoDB, log(...) can
enqueue the audit insert and execute it later on an async worker; that insert
then goes through the PlanNode/IoTConsensus transfer hooks with this
ThreadLocal unset and can recursively produce USER_DATA_TRANSFER events. Please
carry an audit-origin marker across the async request, or explicitly exclude
the audit database/DataRegion before emitting transfer events. A ThreadLocal
alone does not close the recursion path.
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/client/DispatchLogHandler.java:
##########
@@ -63,6 +66,15 @@ public DispatchLogHandler(
@Override
public void onComplete(TSyncLogEntriesRes response) {
+ final TSStatus failedStatus =
+ response.getStatuses().stream()
+ .filter(status -> status.getCode() !=
TSStatusCode.SUCCESS_STATUS.getStatusCode())
+ .findFirst()
+ .orElse(null);
+ recordTransferAttempt(
Review Comment:
[P1] This emits an event for every IoTConsensus log batch, without checking
whether the batch contains user data. It therefore includes metadata/control
traffic and, downstream, audit-log writes themselves. Please classify the batch
(similar to containsUserData in the PlanNode path) and call the hook only for
user-data batches; otherwise this over-audits FDP_ITT.1 and contributes to the
audit recursion described above.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/exchange/source/SourceHandle.java:
##########
@@ -654,6 +666,8 @@ public void run() {
}
return;
}
+ recordTransferAttempt(attempt, true, null, null);
Review Comment:
[P1] Success is recorded before the response is proven usable. A non-empty
but short tsBlocks list reaches this line, then tsBlocks.get(...) throws below;
because transferAttemptRecorded is already true, the catch does not emit a
failure. Please verify tsBlockNum == endSequenceId - startSequenceId, and
finish any required acceptance step, before marking the attempt successful.
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/client/DispatchLogHandler.java:
##########
@@ -172,4 +185,35 @@ private void completeBatch(Batch batch) {
// removeBatch
thread.updateSafelyDeletedSearchIndex();
}
+
+ private void recordTransferAttempt(boolean success, String errorCode,
Throwable error) {
+ if (!thread.getImpl().getUserDataTransferAuditHandler().isEnabled()) {
Review Comment:
[P1] isEnabled() is outside the try block. Since the handler is injected and
may be implemented downstream, an exception here escapes onComplete/onError
before the replication callback can finish its normal completion or retry
logic. Please put handler lookup, isEnabled(), event construction, and
onAttempt() inside one guarded helper. The handler contract should also require
non-blocking behavior because this runs on the consensus callback path.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/audit/UserDataTransferAuditEvent.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.commons.audit;
+
+import org.apache.iotdb.common.rpc.thrift.TEndPoint;
+
+import javax.annotation.Nullable;
+
+/**
+ * Describes one attempt to transfer user data between physically separated
parts of IoTDB. Payload
+ * contents and exception messages must not be included in this event.
+ */
+public final class UserDataTransferAuditEvent {
+
+ private final long timestamp;
+ private final UserDataTransferType transferType;
+ private final TEndPoint initiator;
+ private final TEndPoint source;
+ private final TEndPoint target;
+ private final UserDataTransferProtectionMethod protectionMethod;
+ private final String protectionProtocol;
Review Comment:
[P2] Please keep AuditEventType.USER_DATA_TRANSFER as the event_type, but
reduce this event payload to the intended minimum FDP_ITT.1 record: timestamp,
subject/initiator, source, destination, protection method, result, and one
error value. protectionProtocol, context, and attempt are not required, and
errorCode/errorType can be merged. This keeps the audit schema stable and
avoids collecting correlation details that the requirement does not ask for.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
