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: [email protected]
For additional commands, e-mail: [email protected]
