Siyao Meng created HDDS-17439:
---------------------------------
Summary: 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
Reporter: Siyao Meng
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]
