[jira] [Created] (HDDS-16734) Conditional delete and conditional MPU complete are ungated on both client and OM, so the condition can be silently ignored

["Siyao Meng (Jira)" <ji...@apache.org>]

Siyao Meng created HDDS-16734:
---------------------------------

             Summary: 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
         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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org