feat(clp-rust-utils): Add the config and S3 helpers the Spider compression tasks need: - #2406
Merged
LinZhihao-723 merged 18 commits intoJul 28, 2026
Merged
Conversation
The initial and maximum job-state poll backoffs were fixed constants inside the Spider implementation, so a caller had no way to tune how aggressively a job is polled. Both are now `Duration` parameters of `run_s3_compression_job_to_completion`, leaving the pacing decision with the caller that knows how long its jobs typically run.
# Conflicts: # Cargo.lock # Cargo.toml
…asks need: * Add `ArchiveOutputStorage` and the archive output's `storage` and `retention_period` fields. * Add `Database`'s fixed `table_prefix`. * Fill in `SpiderTaskExecutorConfig` and resolve its relative paths against `CLP_HOME`. * Add `resolve_dataset_name` and the default dataset name. * Add `generate_s3_url`.
Contributor
WalkthroughThe PR adds shared dataset, executor configuration, archive storage, database, and S3 URL helpers with tests. Compression job submission now uses the shared default dataset constant and updated configuration exports. ChangesConfiguration, dataset, and compression utilities
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant generate_s3_url
participant ClientError
Caller->>generate_s3_url: endpoint, region, bucket, object key
generate_s3_url-->>Caller: generated S3 URL
generate_s3_url->>ClientError: UnsupportedS3Endpoint
ClientError-->>Caller: InvalidInput
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/clp-rust-utils/src/clp_config/package/config.rs`:
- Around line 334-343: Update dataset_archive_storage_directory to branch on
ArchiveOutputStorage: retain Path::join for filesystem storage, but construct S3
keys with forward-slash separators between s3_config.key_prefix and
resolve_dataset_name(dataset), avoiding OS-specific path joining.
In `@components/clp-tdl-package/src/task/compression/mod.rs`:
- Around line 13-33: Remove the task registrations for s3_compress_task and
commit_task until their handlers are implemented, or replace the unimplemented!
calls with callable logic that returns TdlError on failure rather than
panicking. Ensure no registered compression or commit endpoint can
unconditionally panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6dd5319b-be3d-46de-804f-6950ec98b7e0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlcomponents/clp-rust-utils/src/clp_config/package/config.rscomponents/clp-rust-utils/src/dataset.rscomponents/clp-rust-utils/src/s3.rscomponents/clp-rust-utils/src/s3/url.rscomponents/clp-tdl-package/Cargo.tomlcomponents/clp-tdl-package/src/common.rscomponents/clp-tdl-package/src/lib.rscomponents/clp-tdl-package/src/task/compression/commit.rscomponents/clp-tdl-package/src/task/compression/compress.rscomponents/clp-tdl-package/src/task/compression/mod.rscomponents/clp-tdl-package/src/task/mod.rstaskfile.yaml
Merged
3 tasks
5 tasks
5 tasks
5 tasks
Bill-hbrhbr
reviewed
Jul 28, 2026
| } | ||
|
|
||
| #[test] | ||
| fn custom_endpoint_ignores_region() { |
Contributor
There was a problem hiding this comment.
@coderabbitai open an issue.
Python's generate_s3_url (
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="components/clp-py-utils/clp_py_utils/s3_utils.py"
ast-grep outline "$file" --match generate_s3_url --view expanded
rg -n -A45 -B8 '^\s*(async\s+)?def\s+generate_s3_url\s*\(' "$file"
issue_body="$(cat <<'EOF'
## Summary
Keep S3 object URL generation consistent between the Rust and Python utility libraries when both a custom endpoint and a region are configured.
`components/clp-rust-utils/src/s3/url.rs::generate_s3_url` treats `endpoint_url` as authoritative: it emits a path-style URL rooted at the custom endpoint and ignores `region_code`. The associated Rust test explicitly verifies this contract.
The Python helper, `components/clp-py-utils/clp_py_utils/s3_utils.py::generate_s3_url`, currently incorporates the region while processing a custom endpoint. This produces behaviour that conflicts with the Rust helper.
## Required changes
Choose and document one shared contract, then update either the Python or Rust implementation so both helpers apply the same semantics for:
- a custom endpoint with no region;
- a custom endpoint with a region; and
- an AWS default endpoint with and without a region.
Update or add unit tests for both implementations to lock down the aligned behaviour.
## Rationale
Callers should obtain equivalent S3 URLs regardless of whether they use the Rust or Python CLP utilities. Divergent handling can generate different object locations for identical configuration.
## Affected areas
- `components/clp-rust-utils/src/s3/url.rs`
- `components/clp-py-utils/clp_py_utils/s3_utils.py`
## Acceptance criteria
- Rust and Python `generate_s3_url` functions have identical custom-endpoint/region precedence semantics.
- Tests cover the custom-endpoint-with-region case in both language implementations.
- Public documentation/comments describe the selected precedence rule.
## Backlinks
- Pull request: https://github.com/y-scope/clp/pull/2406
- Review discussion: https://github.com/y-scope/clp/pull/2406#discussion_r3661902378
- Requested by: `@Bill-hbrhbr`
EOF
)"
gh issue create \
--repo y-scope/clp \
--title "Align Python and Rust S3 URL generation for custom endpoints and regions" \
--body "$issue_body" \
--assignee "Bill-hbrhbr"Length of output: 4824
@Bill-hbrhbr, created and assigned #2431: Align Python and Rust S3 URL generation for custom endpoints and regions.
The issue documents that Python currently injects region_code into a custom endpoint, whereas the new Rust helper ignores it, and requires one consistent contract with coverage in both implementations.
✏️ Learnings added
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 2406
File: components/clp-rust-utils/src/s3/url.rs:66-66
Timestamp: 2026-07-28T00:36:29.109Z
Learning: In CLP, `components/clp-py-utils/clp_py_utils/s3_utils.py::generate_s3_url` injects `region_code` into a custom endpoint URL, while `components/clp-rust-utils/src/s3/url.rs::generate_s3_url` ignores `region_code` for custom endpoints. This cross-language contract mismatch is tracked in GitHub issue `#2431`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Member
Author
There was a problem hiding this comment.
Since this may affect some of our users, I've updated the implementation to match the feature parity with the Python implementation. Please check.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Bill-hbrhbr
reviewed
Jul 28, 2026
| S3 { | ||
| #[serde(default = "default_archive_staging_directory")] | ||
| staging_directory: String, | ||
| s3_config: S3Config, |
Contributor
There was a problem hiding this comment.
@coderabbitai Open an issue:
Support credential-free workload in Rust.
Long-term speaking, AWS credentials should never be serialized into
ClpIoConfig, persisted in compression job records, or forwarded through Spider task inputs. Instead, the coordinator and scheduler should pass only non-sensitive S3 location metadata, such as the endpoint, region, bucket, and object key. Each task executor should retrieve credentials locally using the AWS default credential provider chain, a mounted AWS profile, workload identity, or environment variables injected at runtime.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="y-scope/clp"
title="Support credential-free S3 workloads in Rust"
body_file="$(mktemp)"
cat >"$body_file" <<'EOF'
## Summary
Support credential-free S3 workloads in the Rust components.
AWS credentials must not be serialized into `ClpIoConfig`, persisted in compression-job records, or forwarded through Spider task inputs. The coordinator and scheduler should instead carry only non-sensitive S3 location metadata (endpoint, region, bucket, and object key). Each task executor must obtain AWS credentials locally at runtime through the AWS default credential provider chain, such as a mounted AWS profile, workload identity, or environment variables.
## Rationale
Keeping credentials out of configuration objects and task payloads reduces the risk of credential disclosure through persistence, logs, job records, and cross-component transport. It also enables workload-identity-based deployments without embedding static credentials.
## Affected areas
- `components/clp-rust-utils/src/clp_config/s3_config.rs`
- Rust `ClpIoConfig` modelling and serialization/deserialization
- Compression job record creation and persistence
- Coordinator/scheduler-to-Spider task input contracts
- Rust S3 client construction and task-executor authentication resolution
## Required changes
1. Define credential-free S3 location/task metadata that contains only endpoint, region, bucket, and object key (plus other non-sensitive fields required for S3 access).
2. Remove or prevent credentials from being represented in serialized `ClpIoConfig`, compression-job records, and Spider task inputs.
3. Update Rust S3 client/task-executor setup to resolve credentials locally through the AWS default credential provider chain.
4. Preserve support for runtime credential sources including mounted profiles, workload identity, and environment-injected credentials.
5. Add tests demonstrating that serialized/persisted/forwarded payloads do not contain access keys or secret keys, and that credential-free metadata can be used by a task executor.
## Acceptance criteria
- [ ] No AWS access key ID, secret access key, session token, or equivalent credential material is serialized into `ClpIoConfig`.
- [ ] Compression job records and Spider task inputs include only non-sensitive S3 location metadata.
- [ ] A Rust task executor can create/use an S3 client using the AWS default credential provider chain without credentials in its input payload.
- [ ] Tests cover credential exclusion and successful credential-free configuration/task handling.
Requested by `@Bill-hbrhbr`.
Backlinks:
- PR `#2406`: https://github.com/y-scope/clp/pull/2406
- Review comment: https://github.com/y-scope/clp/pull/2406#discussion_r3661977290
EOF
issue_url="$(gh issue create \
--repo "$repo" \
--title "$title" \
--body-file "$body_file" \
--assignee "Bill-hbrhbr")"
rm -f "$body_file"
printf '%s\n' "$issue_url"Length of output: 3107
@Bill-hbrhbr, opened and assigned #2432: Support credential-free S3 workloads in Rust.
It captures the requirement to keep credentials out of ClpIoConfig, job records, and Spider task inputs, while resolving them locally through the AWS default credential provider chain.
Member
Author
There was a problem hiding this comment.
To be fair: I think this is out of the scope for "Rust" components. This is a package-level design problem, which we already have a solution for through a new component called credential-manager. This component has been completed in a private branch, but we haven't had time to open-source it.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
4 tasks
Bill-hbrhbr
previously approved these changes
Jul 28, 2026
Bill-hbrhbr
left a comment
Bill-hbrhbr
left a comment
Contributor
There was a problem hiding this comment.
opened 2 issues for future improvements; the rest looks good.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/clp-rust-utils/src/s3/url.rs`:
- Around line 54-80: Update the S3 URL construction around the endpoint parsing
and final format calls to percent-encode bucket and object-key path components
while preserving slash separators within object_key segments. Validate
region_code as a safe authority label before interpolating it, rejecting values
containing URL delimiters or otherwise invalid hostname characters; apply this
validation to both default-endpoint and custom-endpoint branches without
changing valid URL output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3fe71a35-97bc-402c-b7fb-7c014d269831
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
components/api-server/src/error.rscomponents/clp-rust-utils/Cargo.tomlcomponents/clp-rust-utils/src/error.rscomponents/clp-rust-utils/src/s3/url.rs
Bill-hbrhbr
approved these changes
Jul 28, 2026
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
2 participants
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.
SpiderTaskExecutorConfigand resolve its relative paths againstCLP_HOME.ArchiveOutputStoragefor bothS3andFs.Database's fixedtable_prefix.resolve_dataset_nameto encapsulate the default dataset name resolution.generate_s3_url.Description
This PR depends on #2404, which adds the shared config types and helpers that the Spider compression tasks depend on. Split out of the compression-task implementation so that the changes to CLP's config mirror can be reviewed on their own; nothing in this PR has a caller yet.
Archive output storage (
clp_config::package::config)ArchiveOutputgains astoragefield, typed as a newArchiveOutputStorageenum withFs { directory }andS3 { staging_directory, s3_config }variants. This mirrors the Python config, where archive output is either filesystem- or S3-backed and the two carry different fields.dataset_archive_storage_directoryderives a dataset's storage base —s3_config.key_prefixfor S3,directoryforFs— joined with the dataset name. It mirrors Python'sadd_dataset, which computes the same value when registering a dataset, and it defaults aNonedataset todefaultso theCLP_Snaming rule lives in one place rather than at each call site.Database::table_prefixMirrors Python's
CLP_METADATA_TABLE_PREFIX("clp_"). It is a fixed constant rather than a configurable value, so it is#[serde(skip)]: a value supplied in the YAML is ignored rather than honoured. The metadata table names are built from it, so allowing it to be overridden per-deployment would let a config file silently point a component at tables that don't exist.SpiderTaskExecutorConfigPreviously an empty placeholder; now carries the four sections the task executor actually reads (
package,archive_output,tmp_directory,database).Its two accessors,
abs_tmp_directoryandabs_archive_output_staging, resolve the config's relative paths againstCLP_HOMEthrough a newmake_config_path_absolutehelper. That helper mirrorsclp_py_utils.core.make_config_path_absoluteexactly, including its early return for paths that are already absolute. This distinction is load-bearing: CLP's deployment tooling rewrites these paths to absolute container paths before writing the config a worker reads, and in that caseCLP_HOMEmust not be prepended. Note the config file itself is located directly fromCLP_CONFIG_PATHand is never joined withCLP_HOME— only relative path values inside it are resolved.Dataset naming (
dataset)Adds
CLP_DEFAULT_DATASET_NAME("default", mirroring Python) andresolve_dataset_name, which maps aNonedataset onto it.CLP_Snever omits the dataset — the CLI defaults it — so callers that receive an optional dataset resolve it through this one function instead of each deciding what a missing dataset means.S3 URLs (
s3)Adds
generate_s3_url, which builds an object URL from an optional endpoint, optional region, bucket, and key. A custom endpoint takes precedence (for MinIO and similar); otherwise it produces the virtual-hosted AWS form, with or without a region.clp-sconsumes these URLs through its--files-fromlist.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
clp_during configuration handling.