feat(cosi): foundation admin client + CAS grant ownership - #219
feat(cosi): foundation admin client + CAS grant ownership#219BenjaminFuentesEviden wants to merge 6 commits into
Conversation
Extract shared rustfs-admin client, ship a tonic COSI driver with Helm toggle, and document BucketClass parameters for Tenant-backed S3. Co-authored-by: Cursor <cursoragent@cursor.com>
Align DriverGrantBucketAccess with Ceph-style isolation: deterministic secrets, never rotate existing users, and return AlreadyExists when preferredAccessKey is claimed by another BucketAccess. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace derivable secrets and marker-based IAM ownership with durable random credentials plus PendingCreate/Ready checkpoints so concurrent preferredAccessKey grants conflict safely and external BAC policies are never overwritten. Split the COSI driver into thin gRPC adapters over testable grant/bucket state machines. Co-authored-by: Cursor <cursoragent@cursor.com>
Existence probes and test fixtures often return an empty 200; treat that as an existing user with no policies instead of failing the parse. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@GatewayJ I applied the first PR here that is mainly impacting the code split you asked, once validated, I will add the extras on a next PR B |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb52aee2b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(owner) = self.find_by_account_id(account_id).await? | ||
| && owner != grant_name | ||
| { |
There was a problem hiding this comment.
When two grants concurrently request the same preferredAccessKey, both can pass this lookup before either proof is written. The losing cas_put then retries against the updated ConfigMap and inserts its own grant key without rechecking the account owner, leaving two proofs for one RustFS account; subsequent policy attachment can overwrite the other grant's access. Revalidate the uniqueness predicate on every CAS retry or represent the account claim with a single atomically created key.
Useful? React with 👍 / 👎.
| api.patch( | ||
| secret_name, | ||
| &PatchParams::apply("rustfs-cosi-driver").force(), | ||
| &Patch::Apply(&secret), | ||
| ) |
There was a problem hiding this comment.
When concurrent retries first provision the same grant, both callers can observe a 404, generate different secret keys, and force-apply this Secret while continuing with their respective local values. Kubernetes Secret write order and RustFS add_user order are independent, so the persisted Secret can contain a key different from the one accepted by RustFS, immediately returning unusable credentials. Use create-once semantics and reload the persisted values after an AlreadyExists conflict.
Useful? React with 👍 / 👎.
| data.insert( | ||
| "accessKeyID".to_string(), | ||
| ByteString(access_key.as_bytes().to_vec()), | ||
| ); | ||
| data.insert( |
There was a problem hiding this comment.
The durable credential Secret writes accessKeyID/accessSecretKey and AWS aliases, but never the required exact accesskey and secretkey keys; the returned COSI credential map has the same omission. Consequently these generated Credential Secrets violate the repository credential contract and cannot be consumed by code expecting the canonical Tenant key names. Add the canonical keys while retaining any compatibility aliases.
AGENTS.md reference: AGENTS.md:L60-L62
Useful? React with 👍 / 👎.
| use k8s_openapi::api::core::v1::{ConfigMap, Secret}; | ||
| use kube::{Api, Client}; | ||
| use rustfs_admin::RustfsAdminClient; | ||
| use thiserror::Error; |
There was a problem hiding this comment.
The new COSI crate introduces thiserror-derived error enums in backend, bucket, credentials, grant, and ownership, contrary to the repository-wide requirement that error handling use snafu. Convert these new errors to the established snafu context pattern rather than adding a second error framework.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| @@ -0,0 +1,146 @@ | |||
| //! Admin credential lookup + RustFS admin client construction. | |||
There was a problem hiding this comment.
The newly added COSI source files begin directly with module documentation and omit the mandatory Apache 2.0 license header; the same omission affects the other new COSI Rust sources, build.rs, and the protocol source. Add the required header to each new non-generated file.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| let msg = err.to_string(); | ||
| if msg.contains("not found") || msg.contains("NoSuch") { | ||
| GrantError::MissingExternalPolicy(policy_name.clone()) |
There was a problem hiding this comment.
When an external policy lookup returns a 404 with an empty or generic response body, the error string is upstream returned 404 Not Found; the case-sensitive checks here match neither lowercase not found nor NoSuch. The request is consequently reported as an internal admin failure rather than the intended MissingExternalPolicy/FailedPrecondition, causing controllers to retry a permanently invalid BucketAccessClass. Inspect the structured status or normalize the message before classifying it.
Useful? React with 👍 / 👎.
| #[tokio::test] | ||
| async fn get_user_info_parses_comma_separated_policy_names() { |
There was a problem hiding this comment.
These newly added tests, along with the existing tests in src/sts/tests.rs, are no longer compiled because this commit removes #[cfg(test)] #[path = "tests.rs"] mod tests; from rustfs_client.rs and no other module references the file. As a result, cargo test silently skips the HTTP signing, error-redaction, TLS, pool, bucket, and user-operation suite; move the tests to rustfs-admin or restore a test-module declaration.
Useful? React with 👍 / 👎.
| pub fn primary_bucket_id(&self, cosi_name: &str) -> String { | ||
| self.bucket_name | ||
| .clone() | ||
| .or_else(|| self.buckets_to_create(cosi_name).into_iter().next()) | ||
| .unwrap_or_else(|| cosi_name.to_string()) |
There was a problem hiding this comment.
When both overrides are supplied, buckets_to_create gives buckets precedence, while this method gives bucketName precedence. For example, bucketName=foo,buckets=bar creates bar but returns foo as the COSI bucket_id, so subsequent consumers are told to use a bucket that was never created. Reject the conflicting parameters or use the same precedence for creation and the returned identifier.
Useful? React with 👍 / 👎.
| Err(KubeError::Api(err)) if err.code == 404 => { | ||
| let secret_key = random_secret_key(40); | ||
| persist_credentials(kube, namespace, &secret_name, access_key, &secret_key).await?; | ||
| Ok(StoredCredentials { |
There was a problem hiding this comment.
If the durable credential Secret is deleted while its ownership proof and RustFS user still exist, an idempotent grant retry enters this 404 branch and persists a newly generated secret. The later user-exists path only reattaches policies and never updates the RustFS user's password, so the RPC returns credentials that cannot authenticate and permanently replaces the only stored secret with the invalid value. When a proof or user already exists, treat a missing Secret as an unrecoverable conflict or explicitly rotate the RustFS credential in the same state transition.
Useful? React with 👍 / 👎.
| match client | ||
| .create_bucket(bucket, Some(params.region.as_str()), false) | ||
| .await | ||
| .map_err(|err| BucketError::Admin(err.to_string()))? | ||
| { | ||
| CreateBucketResult::Created | CreateBucketResult::AlreadyExists => {} |
There was a problem hiding this comment.
On the dynamic path, any BucketAlreadyExists or BucketAlreadyOwnedByYou response is accepted without recording ownership. An existing bucket whose name matches the COSI request is therefore reported as provisioned, and DriverDeleteBucket later unconditionally deletes it because no static override is present; another RustFS user's pre-created bucket can consequently lose all of its data. Distinguish a driver-owned idempotent retry from an unowned name collision before returning success.
Useful? React with 👍 / 👎.
Request changes: COSI idempotency and ownership guarantees are not satisfiedThe overall direction is good: extracting However, the current implementation does not yet satisfy the retry, idempotency, and resource-identity guarantees required by the official [COSI v0.2 specification](https://github.com/kubernetes-sigs/container-object-storage-interface/blob/release-0.2/proto/spec.md) and [protocol definition](https://github.com/kubernetes-sigs/container-object-storage-interface/blob/release-0.2/proto/cosi.proto). COSI RPCs may time out and be retried. A retry with the same fields must continue the original operation safely. Blocking correctness issues
Required testsPlease add regression tests that would fail with the current implementation:
The existing Repository requirementsBefore re-review, please also:
CI being green confirms compilation and existing checks, but it does not cover the ownership races, crash recovery, stale revocation, or destructive bucket path described above. Once these state-machine and ownership guarantees are fixed and covered by fault-oriented tests, the architecture will be in a much better position for the follow-up deployment PR. |
2 participants
Type of Change
Related Issues
Summary of Changes
Foundation PR A (COSI+Helm / sidecar pin stay in PR B):
crates/rustfs-admin— kube-agnostic HTTP/admin/S3/STS client shared byoperatorandrustfs-cosi-driver(operator keeps Tenant credential loading wrappers).cosi-cred-{grant}beforeadd_user; retries reuse the Secret (never re-derive from access key).rustfs-cosi-ownershipcheckpointsPendingCreate→Ready(proof: grant_name, account_id, access_key_hash, cred_secret_name, state). ConcurrentpreferredAccessKeyconflicts; orphan RustFS users are not adopted.cosi-pol-{grant}; BACpolicyparam is validate/attach only (neveradd_canned_policyreplace).grant/bucketmodules; staticbucketName/bucketsdeletes return FailedPrecondition.Checklist
make pre-commit(fmt-check + clippy + test + console-lint + console-fmt-check)[Unreleased](if user-visible change)Impact
Verification
Additional Notes
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.