[
https://issues.apache.org/jira/browse/HDDS-17439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133677#comment-18133677
]
Tsz-wo Sze commented on HDDS-17439:
-----------------------------------
cc [~amaliujia]
> Datanode does not check that a PutBlock committed through a data stream is
> for the stream's block and carries a valid block token
> ---------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17439
> URL: https://issues.apache.org/jira/browse/HDDS-17439
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 1.4.0, 1.4.1, 2.3.0
> Reporter: Siyao Meng
> Priority: Critical
> Attachments: CR-3-check-stream-putblock-block-and-token.patch
>
>
> h3. Mechanism
> When a client commits PutBlock through the data stream instead of the Raft
> log ({{ozone.client.datastream.putblock.without.raft.enabled}}, which opens
> the stream with {{StreamInitWithPutBlock}}), the datanode itself executes two
> kinds of PutBlock that it receives on the stream: a PutBlock sent as a stream
> command in the middle of the stream ({{ContainerStateMachine.streamCommand}},
> added by HDDS-16008) and the PutBlock at the end of the stream, executed when
> the stream is closed ({{KeyValueStreamDataChannel.closeWithStreamPutBlock}}
> calling {{ContainerStateMachine.streamPutBlock}}, added by HDDS-15792). Both
> hand the request to {{HddsDispatcher.dispatch}} with the dispatcher context
> ops {{STREAM_COMMAND}} and {{STREAM_LINK}}.
> A PutBlock sent through the Raft log has its block token checked by the
> leader in {{ContainerStateMachine.startTransaction}}. On the two stream
> paths, {{DispatcherContext.Op.validateToken}} returns false for
> {{STREAM_COMMAND}} and {{STREAM_LINK}}, so {{HddsDispatcher.dispatch}} does
> not check the token of the PutBlock. {{STREAM_LINK}} was added to that list
> by HDDS-9697, when it was used for the PutBlock executed while applying the
> stream's Raft log entry, where a second check of a token that was already
> checked could fail after the token expired. Since HDDS-12007 and HDDS-15792
> it is only used for the PutBlock executed at stream close, which is not part
> of a log entry and is not replayed. {{STREAM_COMMAND}} was added to the list
> by HDDS-16008.
> Neither path compares the PutBlock with the block the stream was opened for.
> The invariant the datanode should keep: a PutBlock received on a data stream
> is for the stream's own block (the container ID and block ID of the stream's
> init request) and carries a valid block token for that block, like every
> other PutBlock.
> h3. Trigger
> Applies only when data streaming is enabled on the datanodes
> ({{hdds.container.ratis.datastream.enabled}}, default {{false}}) and the
> stream is opened with {{StreamInitWithPutBlock}}, which the client does only
> with {{ozone.client.datastream.putblock.without.raft.enabled}} (default
> {{false}}). The stock {{BlockDataStreamOutput}} builds both PutBlocks for the
> stream's own block with the stream's token
> ({{ContainerProtocolCalls.getPutBlockRequest}}), so its writes meet the
> invariant and are unaffected by the change below.
> h3. Impact
> * On these two paths the datanode commits block metadata without the token
> check that applies to every other PutBlock.
> * A PutBlock at stream close that does not match the stream's block is still
> committed, and the stream is then linked as if its own PutBlock had been
> committed, so the stream reports success while its own block has no committed
> metadata.
> h3. Reproduction
> No standalone reproduction is attached. The regression tests in the patch
> cover the checks and fail on unmodified source:
> *
> {{ContainerStateMachineTests.testStreamCommandPutBlockNotMatchingStreamBlockIsRejected}}
> ({{hadoop-hdds/container-service}}, run by
> {{TestContainerStateMachineLeader}} and
> {{TestContainerStateMachineFollower}}): a stream command whose PutBlock does
> not match the stream's block must fail with {{MALFORMED_REQUEST}} and must
> not reach the dispatcher. Without the change it fails with {{Expected
> java.util.concurrent.ExecutionException to be thrown, but nothing was
> thrown.}}
> *
> {{ContainerStateMachineTests.testStreamClosePutBlockNotMatchingStreamBlockIsRejected}}:
> the same for the PutBlock executed at stream close; a matching PutBlock is
> still dispatched with {{STREAM_LINK}}. Without the change it fails with
> {{Expected
> org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException
> to be thrown, but nothing was thrown.}}
> * {{TestHddsDispatcher.testValidateToken}}: {{STREAM_LINK}} and
> {{STREAM_COMMAND}} move to the list of ops that check the token. Without the
> change it fails with {{expected: <true> but was: <false>}}.
> {{TestSecureContainerServer.testStreamPutBlocksWithTokenForStreamBlock}}
> ({{hadoop-ozone/integration-test}}) covers the compatible case on three
> secure Ratis datanodes with data streaming: a stream opened with
> {{StreamInitWithPutBlock}} whose PutBlocks are built as
> {{BlockDataStreamOutput}} builds them (own block, own token) commits both the
> mid stream and the close PutBlock. It passes with and without the change.
> h3. Patch
> [^CR-3-check-stream-putblock-block-and-token.patch], against
> 7212465c0bb19615708c144238317db51c2a696d with the patch of HDDS-17431
> applied. It applies on top of the patch of HDDS-17431 and depends on it: a
> PutBlock rejected at stream close must fail the stream close, which
> HDDS-17431 adds. With the patch of HDDS-17431 applied first, it also applies
> to master at 1acf315d546. The patch of HDDS-17438 also edits the stream test
> setup in {{ContainerStateMachineTests}}; whichever lands second needs a small
> rebase there.
> * {{DispatcherContext.Op.validateToken}} now returns true for {{STREAM_LINK}}
> and {{STREAM_COMMAND}}, so {{HddsDispatcher.dispatch}} checks the PutBlock's
> own token on both stream paths. Neither op is used while applying or
> replaying a Raft log entry any more, so this does not bring back the double
> check that HDDS-9697 removed; {{APPLY_TRANSACTION}},
> {{WRITE_STATE_MACHINE_DATA}} and {{READ_STATE_MACHINE_DATA}} keep returning
> false.
> * {{ContainerStateMachine}} keeps the stream's init request in the two
> callbacks it creates for the stream, and a new method,
> {{checkStreamPutBlock}}, checks that each PutBlock is for the stream's
> container and block before it is dispatched. A mismatch fails the command or
> the stream close with {{MALFORMED_REQUEST}}.
> Both checks are needed: the token check verifies the PutBlock's own token,
> and the block check ties the PutBlock to the stream. There is no wire or
> protocol change and no new configuration. With the patch
> {{TestHddsDispatcher}}, {{TestContainerStateMachineLeader}},
> {{TestContainerStateMachineFollower}}, {{TestKeyValueStreamDataChannel}} and
> {{TestKeyValueHandler}} pass (94 tests), {{TestBlockDataStreamOutput}} in
> {{hadoop-ozone/integration-test}} (mini cluster, with and without PutBlock
> through the data stream, 20 tests) and {{TestSecureContainerServer}} (3
> tests) pass, and checkstyle on {{hadoop-hdds/container-service}} and
> {{hadoop-ozone/integration-test}} is clean.
> Found by code review of the datanode streaming PutBlock paths, as part of the
> TLA+ verification effort under HDDS-15926, on commit
> 7212465c0bb19615708c144238317db51c2a696d. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-9697 (removed the token check from the state machine paths
> other than {{startTransaction}}), HDDS-16008 (added the stream command),
> HDDS-15792 (added the PutBlock at stream close), HDDS-16828 (finalized block
> check on the same paths). The attached patch is a proposal for review.
> Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
