[
https://issues.apache.org/jira/browse/HDDS-16734?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16734:
------------------------------
Attachment: MC-3-gate-conditional-delete-and-mpu-complete.patch
> Conditional delete and conditional MPU complete are ungated on both client
> and OM, so the condition can be silently ignored
> ---------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16734
> URL: https://issues.apache.org/jira/browse/HDDS-16734
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Critical
> Attachments: MC-3-gate-conditional-delete-and-mpu-complete.patch
>
>
> h3. Mechanism
> The conditional write feature is gated twice, client side through an OM
> version check and server side through {{checkFeatureEnabled}}, and both gates
> are missing on exactly the delete and MPU complete paths.
> Client side, {{RpcClient}} guards every two phase conditional entry point on
> {{ATOMIC_REWRITE_KEY}} at lines 1435, 1462, 1489, 1596 and 1624. But
> {{deleteKey(..., expectedETag)}} at {{:1782}} and
> {{completeMultipartUpload(..., expectedDataGeneration, expectedETag)}} at
> {{:2295}} set the conditional fields with no version check at all.
> Server side, {{OMKeyCreateRequest.java:105}} and
> {{OMKeyCommitRequest.java:100}} call {{checkFeatureEnabled}}, while
> {{S3MultipartUploadCompleteRequest.java:284}} and
> {{OMKeyDeleteRequest.java:154}} do not.
> Against an OM predating these fields the unknown optional protobuf field is
> silently ignored. The client issues a conditional operation, receives
> success, and the OM has acted unconditionally.
> h3. Trigger
> A mixed version cluster during upgrade, or {{ATOMIC_REWRITE_KEY}} listed in
> {{ozone.om.features.disabled}}.
> * A create-if-absent MPU complete overwrites a pre-existing object.
> * A conditional delete removes a value newer than the one its ETag named,
> while the S3 caller is told its compare and delete was honoured.
> Separately, at this head {{OMKeyDeleteRequest}} carries no
> {{expectedDataGeneration}} at all, so a conditional overwrite races an
> unconditional delete even on a fully current cluster.
> h3. Impact
> Silent destruction or overwrite of another writer's newer committed value,
> with success reported to the caller. Reachable from the S3 object endpoint in
> a normal one-release-behind deployment. No mask exists in the gateway, the
> translator, or the handler.
> h3. Reproduction
> PASS. {{repro/test_bugMC-3_conditional_downgrade.java}} and {{.sh}}, 4 tests.
> Evidence in {{confirmation/MC-3/}}: {{evidence.txt}}, {{level0.log}},
> {{level0_mpu.log}}, {{level2.log}}.
> The {{[delete@CURRENT]}} case is the one that needs no downgrade at all:
> because there is no sentinel for conditional delete, the client has nothing
> to compare, so the gap is reachable on a current client too.
> h3. Suggested fix
> # Add {{OzoneManagerVersion}} sentinels for the two features (for example
> {{CONDITIONAL_DELETE}} and {{CONDITIONAL_MPU_COMPLETE}}) and gate the client:
> in {{RpcClient.deleteKey(..., expectedETag)}} at {{:1782}} and
> {{completeMultipartUpload(..., expectedDataGeneration, expectedETag)}} at
> {{:2295}}, throw when {{omVersion}} is below the sentinel and the caller
> asked for a condition, matching {{:1489}} exactly. The sentinel is the load
> bearing part: without one the client has nothing to compare.
> # Add the missing {{ozoneManager.checkFeatureEnabled(...)}} to
> {{OMKeyDeleteRequest}} (and {{OMKeyDeleteRequestWithFSO}}) and
> {{S3MultipartUploadCompleteRequest}} when the request carries
> {{expectedETag}} or {{expectedDataGeneration}}, so
> {{ozone.om.features.disabled}} is honoured uniformly instead of only on
> create and commit.
> # While touching this, note that the five existing client gates all compare
> against {{ATOMIC_REWRITE_KEY}} for features that landed much later, so they
> under protect too. A per feature sentinel per conditional entry point is the
> consistent end state.
> # Cover both sides with tests: a client side case asserting the conditional
> delete and MPU complete are refused below the sentinel (the {{TestRpcClient}}
> {{MockOmTransport}} pattern suffices), and an OM side case asserting
> {{NOT_SUPPORTED_OPERATION}} when the feature is disabled, extending
> {{TestOMKeyDeleteRequest}} and {{TestS3MultipartUploadCompleteRequest}}
> rather than adding new suites.
> h3. Patch
> [^MC-3-gate-conditional-delete-and-mpu-complete.patch], against
> 20e6a1c0f6039248373efd3eb1d7f2afcf7f1535.
> Server side, replaces the block duplicated in
> {{OMKeyCreateRequest.preExecute}} and {{OMKeyCommitRequest.preExecute}} with
> one shared {{checkConditionalWriteEnabled}} in {{OMKeyRequest}}, and calls it
> from {{OMKeyDeleteRequest.preExecute}} and
> {{S3MultipartUploadCompleteRequest.preExecute}} as well. The shared gate also
> closes a gap the original two call sites had: they only looked at
> {{expectedDataGeneration}}, so an If-Match ETag, which only becomes a
> generation later in {{validateAndUpdateCache}}, was never gated even on
> create and commit. Putting the check in one place is what makes all four
> paths honour {{ozone.om.features.disabled}} uniformly.
> Client side, adds the missing version checks to {{RpcClient.deleteKey(...,
> expectedETag)}} and {{RpcClient.completeMultipartUpload(...,
> expectedDataGeneration, expectedETag)}}, each firing only when the caller
> actually asked for a condition.
> Scope note, for reviewers to decide: both new client checks compare against
> {{ATOMIC_REWRITE_KEY}}, matching the five existing gates rather than
> introducing new sentinels. Per feature sentinels, as suggested above, would
> be the consistent end state for all seven sites and are better done in one
> pass than piecemeal here.
> Covered by {{testPreExecuteRefusesConditionalDeleteWhenFeatureDisabled}} in
> {{TestOMKeyDeleteRequest}} (inherited by the FSO subclass),
> {{testPreExecuteRefusesConditionalCompleteWhenFeatureDisabled}} in
> {{TestS3MultipartUploadCompleteRequest}} (inherited by the FSO subclass), and
> {{testConditionalDeleteAndCompleteMpuRefuseAnOlderOm}} plus
> {{testConditionalDeleteReachesACurrentOm}} in {{TestRpcClient}}. The last two
> needed {{MockOmTransport.getServiceList}} to be able to report an OM version,
> which it previously could not, so these are the first unit tests covering any
> of the client side conditional gates. All four verified to fail without the
> change and pass with it. Checkstyle clean. 171 tests across the eight
> affected classes pass.
> Found by TLA+ model checking and code review of the OM conditional create,
> overwrite and delete paths under HDDS-15926, on commit
> 20e6a1c0f6039248373efd3eb1d7f2afcf7f1535. Checked against open HDDS issues
> for duplicates before filing, in addition to the repository's merged git
> history. Generated with Specula (Claude Opus 5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
