Skip to content

fix(provisioning): skip CreateBucket for buckets that already exist - #287

Open
shurkus wants to merge 1 commit into
rustfs:mainfrom
shurkus:fix/issue-286-create-bucket-head-first
Open

shurkus wants to merge 1 commit into
rustfs:mainfrom
shurkus:fix/issue-286-create-bucket-head-first

Conversation

@shurkus

@shurkus shurkus commented Oct 20, 2026

Copy link
Copy Markdown

Closes #286

Type of Change

  • New Feature
  • Bug Fix
  • Documentation
  • Performance Improvement
  • Test/CI
  • Refactor
  • Other:

Related Issues

Closes #286
Server side: rustfs/rustfs#8741

Summary of Changes

  • RustfsAdminClient::create_bucket now sends HeadBucket first. A successful HeadBucket returns CreateBucketResult::AlreadyOwnedByYou without sending CreateBucket. Any other answer goes on to CreateBucket, so the existing 409/503/4xx handling is unchanged. The COSI driver uses the same client method and gets the same behaviour.
  • Why: on RustFS, CreateBucket for an existing bucket takes the bucket's exclusive locks before it checks existence. Under steady PutObject traffic it times out with 503, and the bucket stays Pending untilthe writes stop.
  • Tests: existing_bucket_skips_create_bucket (HEAD 200, PUT 503 → Ready, "already exists") and missing_bucket_is_created (HEAD 404 → PUT). explicit_private_removes_operator_managed_anonymous_policy gets an explicit HEAD route, because axum's get route also answers HEAD and its handler asserts the ?policy query.

Checklist

  • I have read and followed the CONTRIBUTING.md guidelines
  • Passed make pre-commit (fmt-check + clippy + test + console-lint + console-fmt-check)
  • Added/updated necessary tests
  • Documentation updated (if needed)
  • CHANGELOG.md updated under [Unreleased] (if user-visible change)
  • CI/CD passed (if applicable)

Impact

  • Breaking change (CRD/API compatibility)
  • Requires doc/config/deployment update
  • Other impact: one extra HeadBucket per bucket per provisioning run

Verification

cargo fmt --all -- --check
cargo clippy --all-features -- -D warnings
cargo nextest run --all
make release-metadata-check e2e-check

Additional Notes

Rust gates ran locally: fmt, clippy, cargo nextest run --all (770 passed), doc tests, release-metadata-check, e2e-check. Console checks were not run because no console code changed.

Seen on a production Tenant (RustFS 1.0.2, steady mc mirror writes into the bucket): PUT /main returned 503 after 5 s on every retry for over 30 minutes, while HEAD /main answered at once.

Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.

@GatewayJ GatewayJ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new HEAD fast path changes the ownership semantics of RustfsAdminClient::create_bucket and breaks the COSI driver's existing bucket ownership check.

A successful HeadBucket confirms that the bucket exists and the credentials can access it. Credentials with s3:ListBucket can receive 200 for a bucket owned by another account; the response does not establish ownership. See the HeadBucket API documentation.

At crates/rustfs-admin/src/s3_ops.rs:753-754, that response becomes AlreadyOwnedByYou. The COSI caller accepts this result and persists a Ready ownership proof in crates/cosi-driver/src/bucket.rs:267-285. Its delete path then accepts that proof and sends DeleteBucket.

Reproduction scenario:

  1. A bucket already exists under the requested dynamic COSI bucket name. The configured credentials can access it but do not own it.
  2. HeadBucket returns 200. CreateBucket would return 409 with <Error><Code>BucketAlreadyExists</Code></Error>.
  3. Start with no COSI ownership ConfigMap and no static bucketName/buckets override. Invoke the production COSI create_bucket function.
  4. If creation reports success, invoke the production COSI delete_bucket function for the returned bucket ID.

I reproduced this with local mocked S3 and Kubernetes HTTP endpoints, exercising the real shared client and COSI bucket create/delete code. The mock records requests and ownership ConfigMap updates.

Observation Base client at ca8b6d4 PR client at 5c72d76
Shared client result AlreadyExists AlreadyOwnedByYou
COSI create result NameConflict Success
Ownership proof after create Removed Ready
S3 HEAD / PUT / DELETE requests in the COSI scenario 0 / 1 / 0 1 / 0 / 1

The two added ownership regression checks fail on the PR and pass when the base client's s3_ops.rs is restored. A separate check confirmed that HEAD 403/404/405/500/503 still falls through to PUT and preserves BucketAlreadyExists.

The confirmed regression is an incorrect ownership claim and an allowed delete attempt. Whether an actual deletion succeeds still depends on the server's authorization and bucket state. This reproduction used local mocks and did not delete a live bucket.

Please keep the existence check in the Tenant provisioning flow, where accepting an existing bucket is already supported, and preserve the shared create_bucket ownership classification used by COSI. Expose a dedicated existence-check operation as needed. Add regression coverage for a readable bucket owned by another account, asserting NameConflict, removal of the pending proof, and refusal of subsequent deletion. Retain the existing-bucket-under-write-load test for the original provisioning fix.

Validation performed:

  • cargo test --workspace --lib --bins --locked --quiet: 749 tests passed.
  • make -j8 fmt-check release-metadata-check: passed.
  • Temporary HTTP regression harness: two ownership checks failed on the PR; all three checks passed with the base client.
  • The local full test run stopped at the chart tests because Helm was unavailable. The PR's CI run passed.

This ownership regression needs to be fixed before merging.

// RustFS takes the bucket's exclusive locks before it finds an existing
// bucket, so a repeated PUT times out with 503 while objects are written.
if self.bucket_exists(bucket).await? {
return Ok(CreateBucketResult::AlreadyOwnedByYou);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the ownership distinction used by COSI

HeadBucket 200 proves that these credentials can access the bucket; it does not prove ownership. For a readable bucket owned by another account, this return skips the PUT that would report BucketAlreadyExists. COSI accepts AlreadyOwnedByYou, stores a Ready ownership proof, and subsequently permits a DeleteBucket request. A local mock-based test using the production COSI create/delete functions produces NameConflict and zero DELETE requests with the base client, but success, a Ready proof, and one DELETE request with this change. Please apply the HEAD existence check in Tenant provisioning and retain the shared client's ownership classification for COSI, with regression coverage for this scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Existing bucket stays Pending: CreateBucket returns 503 while the bucket is written to

2 participants