Repository navigation
deps: upgrade opendal to 0.59.5 with the 0.59 safety audit - #536
Merged
Merged
Conversation
opendal 0.59 moves the reqwest client into opendal-http-transport-reqwest and installs it only from the auto-register-services constructor, which default-features = false disables. Without an explicit install every S3 request fails with "default HTTP transport is not installed", so S3ArtifactStore::new now calls opendal::install_default(). Operator::new returns a finished operator and full_capability() is replaced by the effective capability(). The seal gate keeps requiring write_with_if_match and write_with_if_not_exists; a test pins those effective flags to the S3 service's own declaration so no default layer can emulate them. Service conflicts now arrive as ErrorKind::Conflict instead of ConditionNotMatch. seal_immutable reobserves them as before rather than reporting SealAmbiguous; create_immutable already reconciles every failure from the stored bytes, so its redundant kind arm is collapsed. Wire-level tests against an in-process S3 endpoint pin the 409 conflict retry, the If-Match-on-missing-key change (apache/opendal#8172), and the new rejection of HEAD responses without Content-Length, which 0.57 accepted as an empty seal. The lockfile moves only opendal and the packages its requirements force. Signed-off-by: wchwawa <wch19961116@gmail.com>
Signed-off-by: wchwawa <wch19961116@gmail.com>
Rebuilding Cargo.lock on top of main's lockfile resolved opendal to 0.59.5, the current patch release, so the manifest now names it. 0.59.5 declares no breaking changes, and the seven wire tests in crates/nokv-object/src/s3_store_test.rs pass on it: the installed HTTP transport, the conditional-write capability gate, conflict reobservation, the vanished-payload fence, and the rejected HEAD without Content-Length. Signed-off-by: wchwawa <wch19961116@gmail.com>
opendal, opendal-core, opendal-service-s3 and opendal-http-transport-reqwest 0.59 declare rust-version 1.91, and crc-fast 1.10.0 declares 1.89. The Docker image still built with rust 1.88, so `cargo build --release --locked` refused to resolve and the image check failed. The workspace rust-version, the Dockerfile default, both image workflows and CONTRIBUTING now name 1.91, the lowest toolchain the locked dependency graph accepts. `cargo check --release --locked -p nokv --bin nokv` passes on rustc 1.91.1. Signed-off-by: wchwawa <wch19961116@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
opendal 0.57 is two minor versions behind, and Dependabot's bump (#530) does not compile. This PR moves
nokv-objectto opendal 0.59.5 and keeps every immutable-object safety path behaving as before, with wire-level tests for the behaviors 0.59 changed.Problem
Dependency chore, so no issue; the template exempts dependency chores. It supersedes #530, which fails every CI check: 0.58 and 0.59 removed
Operator::finish()andOperatorInfo::full_capability(). One more 0.59 change does not show at compile time. The reqwest HTTP transport moved into its own crate and is installed only by theauto-register-servicesfeature, which NoKV'sdefault-features = falseturns off. Every S3 request then fails with "default HTTP transport is not installed".Consequences
Without the migration, Dependabot keeps reopening a bump that cannot build, and NoKV stays on a 0.57 line that no longer receives fixes. Taking the bump with only the compile fixes would take down every object-store request, including the live admission probes.
Example
Before, on 0.59 without the transport fix:
S3ArtifactStore::newsucceeds, then the firstheadorcreate_immutablefails withConfigInvalid: default HTTP transport is not installed, and both live admission profiles reportInconclusive. After: requests reach the endpoint, and a 409ConditionalRequestConflicton the seal PUT is reobserved instead of being reported asSealAmbiguous.Solution
S3ArtifactStore::newcallsopendal::install_default(), which is idempotent and first-installed-wins, uses the finished operator thatOperator::newnow returns, and readscapability()in place offull_capability(). In the seal loop, the newErrorKind::Conflictjoins the reobserve arm, because a conflict means the PUT was not applied. The create path already reconciles every failure against the stored bytes, so its redundant kind-routed arm is collapsed. Seven hermetic tests drive OpenDAL's real S3 service against an in-process endpoint with AWS conditional-write semantics, so OpenDAL's status-code and error mapping is what gets tested. The lockfile moves only what opendal 0.59.5 requires. opendal 0.59 and its service and transport crates declarerust-version = "1.91", andcrc-fast1.10.0 declares 1.89. So the workspacerust-version, the DockerfileRUST_VERSIONdefault,docker-image.yml, therelease-images.ymldefault and CONTRIBUTING now all say 1.91. On 1.88 the Docker build stops withrustc 1.88.0 is not supported by the following packages.Gain
NoKV builds on the current opendal line, and two 0.59 behavior changes improve safety. A HEAD without
Content-Lengthis now an error instead of a false "already sealed" proof over a non-empty payload, and anIf-MatchPUT against a payload that vanished creates the fence instead of reporting ambiguity.Tradeoffs
A provider that answers a conditional PUT with a bare or unknown 409 now surfaces as
Unexpected, which the seal loop treats asSealAmbiguous: the key stays reserved and the seal retries, so it fails closed. Before, it was reobserved. The transport is installed explicitly rather than through constructor registration, so another store type built on opendal would need the same call. The minimum supported Rust rises from 1.88 to 1.91, so anyone building on 1.88 to 1.90 has to upgrade. CI already pins 1.99.1 and the release images build on the declared minimum.Validation
cargo fmt --all -- --checkis clean,cargo clippy --locked --workspace --all-targets -- -D warningspasses, andgit diff --checkis clean.cargo test --locked --workspaceon the merged tree (mainat17cb1c02d6plus this branch): 1239 passed, 0 failed, 13 ignored.cargo test --locked -p nokv-object s3_store_test: the 7 wire tests pass on 0.59.5.main's withcargo update -p opendal@0.57.0 --precise, so only what opendal 0.59.5 requires moved:opendal-http-transport-reqwest,reqsign-aws-core,crc-fast, andasyncbandwere added;log,reqsign-aws-v4,reqsign-core,reqsign-file-read-tokio, andquick-xmlmoved to the versions opendal 0.59.5 and reqsign 3.3 require; the RSA/PKCS chain that reqsign 3.0 pulled in,sha20.10,rand0.8,mea,lazy_static, andfutures-executorare no longer reachable and were removed.cargo check --release --locked -p nokv --bin nokvpasses; theimagecheck rebuilds it in Docker. The 7 wire tests were re-run on 0.59.5 by the original author withcargo test --locked -p nokv-object --lib: 59 passed, 0 failed, 3 ignored. opendal 0.59.4 and 0.59.5 have byte-identicalsrc/trees in all four opendal crates; only thequick-xmlrequirement moves from 0.41 to 0.42.live_provider_admission_passes_the_single_put_profile,live_provider_admission_proves_append_sealing),scripts/workbench/object_namespace_recovery_gate.py, and the two mutation checks (removinginstall_default()fails 6 of the 7 wire tests; removing theConflictarm fails the conflict test). The original author ran all of them on 0.59.4 against the pinned RustFS image, each passing. 0.59.5 is a weekly patch with no declared breaking change, and CI runsobject-namespace-recoveryas a required check on this PR.