fix(clp-tdl-package): Resolve AWS credentials via the SDK's default provider chain for default authentication. - #2438
Conversation
…rovider chain for `default` authentication.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds optional AWS session-token support to credential configuration and S3/SQS clients. The compression worker now resolves regional credentials through the AWS SDK and exports resolved credentials, including session tokens, with updated tests. ChangesAWS session token support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CompressionWorker
participant CredentialResolver
participant AWSSDK
CompressionWorker->>CredentialResolver: Resolve credentials with region and authentication
CredentialResolver->>AWSSDK: Resolve default credentials
AWSSDK-->>CredentialResolver: Return credentials and session token
CredentialResolver-->>CompressionWorker: Return credentials or an error
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-tdl-package/src/task/compression/compress.rs`:
- Around line 949-969: The s3_credential_env tests lack coverage for the
AwsAuthentication::Default resolution path. Add a unit test alongside
s3_credential_env_credentials that configures controlled AWS credential
environment variables, invokes s3_credential_env with
AwsAuthentication::Default, and verifies the resolved credentials map to
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_SESSION_TOKEN without
requiring external infrastructure.
- Around line 91-100: Extract the shared region fallback into a resolve_region
helper accepting Option<&NonEmptyString> and returning &str. Replace the inline
resolution in the current compression flow and the corresponding logic in
build_s3_client, preserving AWS_DEFAULT_REGION for absent values and the
configured region otherwise.
- Around line 353-412: Update s3_credential_env and the
AwsAuthentication::Default path so long-running clp-s subprocesses do not
receive one-time expanded STS credentials through static AWS_* environment
variables. Keep credentials managed by the parent SDK provider, or implement
expiry-aware credential refresh and propagation before expiration; preserve the
existing explicit AwsAuthentication::Credentials behavior unless refresh
handling is required for correctness.
🪄 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: 5408d4aa-0dec-4f98-b718-db12da6d8f74
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
components/clp-rust-utils/src/clp_config/s3_config.rscomponents/clp-rust-utils/src/s3/client.rscomponents/clp-rust-utils/src/sqs/client.rscomponents/clp-rust-utils/tests/clp_config_test.rscomponents/clp-tdl-package/Cargo.tomlcomponents/clp-tdl-package/src/task/compression/compress.rscomponents/log-ingestor/tests/test_ingestion_job.rscomponents/log-ingestor/tests/test_scan.rs
… in `s3_credential_env`.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-tdl-package/src/task/compression/compress.rs (1)
353-412: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClear inherited
AWS_SESSION_TOKENwhen no token is resolved.
run_log_converterandrun_clp_sonly add the returned credential env vars, so any pre-existingAWS_SESSION_TOKENremains in the child unless removed explicitly. Either add anenv_remove("AWS_SESSION_TOKEN")branch whencredentials.session_token()isNone, or change the env contract to carry removals.🤖 Prompt for 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. In `@components/clp-tdl-package/src/task/compression/compress.rs` around lines 353 - 412, The credential environment handling around s3_credential_env must clear any inherited AWS_SESSION_TOKEN when resolved credentials have no session token. Update the returned environment contract and the consumers run_log_converter and run_clp_s so the no-token case explicitly removes AWS_SESSION_TOKEN, while retaining the existing insertion behavior when a token is present.
🤖 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-tdl-package/src/task/compression/compress.rs`:
- Around line 971-992: Update the s3_credential_env_default test to preserve and
restore the original AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and
AWS_SESSION_TOKEN values using a guard that cleans up on every exit path,
including assertion or credential-resolution failure. Keep the existing spoofed
credentials and assertions unchanged.
---
Outside diff comments:
In `@components/clp-tdl-package/src/task/compression/compress.rs`:
- Around line 353-412: The credential environment handling around
s3_credential_env must clear any inherited AWS_SESSION_TOKEN when resolved
credentials have no session token. Update the returned environment contract and
the consumers run_log_converter and run_clp_s so the no-token case explicitly
removes AWS_SESSION_TOKEN, while retaining the existing insertion behavior when
a token is present.
🪄 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: dc2475b7-678e-4085-80a0-286c5cdfe1b0
📒 Files selected for processing (1)
components/clp-tdl-package/src/task/compression/compress.rs
| #[test] | ||
| fn s3_credential_env_default() { | ||
| let runtime = tokio::runtime::Runtime::new().expect("failed to create Tokio runtime"); | ||
| // SAFETY: No other test in this binary reads or writes these env vars, and the env | ||
| // provider is the first source in the SDK's default chain, so the test is deterministic | ||
| // and network-free. | ||
| unsafe { | ||
| std::env::set_var("AWS_ACCESS_KEY_ID", "the-env-access-key"); | ||
| std::env::set_var("AWS_SECRET_ACCESS_KEY", "the-env-secret-key"); | ||
| std::env::set_var("AWS_SESSION_TOKEN", "the-env-session-token"); | ||
| } | ||
|
|
||
| assert_eq!( | ||
| s3_credential_env(runtime.handle(), "us-east-1", &AwsAuthentication::Default) | ||
| .expect("failed to resolve credentials"), | ||
| vec![ | ||
| ("AWS_ACCESS_KEY_ID", "the-env-access-key".to_string()), | ||
| ("AWS_SECRET_ACCESS_KEY", "the-env-secret-key".to_string()), | ||
| ("AWS_SESSION_TOKEN", "the-env-session-token".to_string()), | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files matching compress.rs =="
fd -a 'compress\.rs$' . | sed 's#^\./##'
echo "== relevant test lines =="
file="$(fd 'compress\.rs$' . | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '930,1020p' "$file" | cat -n | sed 's/^/ /'
fi
echo "== tests around default/aws env usage =="
rg -n 's3_credential_env_env|AWS_ACCESS_KEY_ID|AWS_SECRET_ACCESS_KEY|AWS_SESSION_TOKEN|AwsAuthentication::Default|s3_credential_env\(' components/clp-tdl-package/src/task/compression/compress.rs || true
echo "== package/test config references to parallel/run order =="
fd -a 'Cargo.toml|clippy.toml|rust-toolchain|rustfmt|deny.toml' . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
rg -n 'test|future|parallel|threads|deny|rustflags|xargo|rust-analyzer' "$f" || true
done
echo "== all AWS env mutations in tests =="
rg -n 'set_var\("AWS_|remove_var\("AWS_|mock\(AWS_|Env::from_raw|AWS_SESSION_TOKEN|AWS_SECRET_ACCESS_KEY|AWS_ACCESS_KEY_ID' components/clp-tdl-package/components/components/clp-tdl-package/src || true
echo "== module test markers =="
rg -n '#\[cfg\(test\)\]|#\[tokio::test|#\[test|#\[serial|#\[serial_test|serial_test' components/clp-tdl-package/src components -g '*.rs' | head -n 200Repository: y-scope/clp
Length of output: 13029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== compress.rs s3_credential_env implementation =="
sed -n '340,425p' components/clp-tdl-package/src/task/compression/compress.rs | cat -n
echo "== clp-tdl-package Cargo.toml =="
cat -n components/clp-tdl-package/Cargo.toml | sed -n '1,180p'
echo "== AWS env var read access in clp-tdl-package tests =="
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('components/clp-tdl-package').rglob('*.rs'):
txt = p.read_text(errors='ignore')
# only print relevant files and matches
if any(env in txt for env in ('AWS_ACCESS_KEY_ID','AWS_SECRET_ACCESS_KEY','AWS_SESSION_TOKEN','AWS_DEFAULT_REGION')):
print(f'--- {p}')
for i,line in enumerate(txt.splitlines(),1):
if any(env in line for env in ('AWS_ACCESS_KEY_ID','AWS_SECRET_ACCESS_KEY','AWS_SESSION_TOKEN','AWS_DEFAULT_REGION')):
print(f"{i}: {line.strip()}")
PY
echo "== clp-tdl-package test attributes with env vars =="
python3 - <<'PY'
from pathlib import Path
for p in Path('components/clp-tdl-package').rglob('*.rs'):
txt = p.read_text(errors='ignore')
if any(env in txt for env in ('AWS_ACCESS_KEY_ID','AWS_SECRET_ACCESS_KEY','AWS_SESSION_TOKEN')):
print(f'--- {p}')
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if '#[test]' in l or '#[tokio::test' in l or 'AWS_ACCESS_KEY_ID' in l or 'AWS_SECRET_ACCESS_KEY' in l or 'AWS_SESSION_TOKEN' in l:
start=max(1,i-3); end=min(len(lines),i+10)
for j in range(start,end+1):
print(f"{j}: {lines[j-1]}")
print()
PY
echo "== lockfile/test parallel config references =="
fd 'Cargo.lock|deny.toml|clippy.toml|rust-toolchain|rustfmt.toml' . | sed 's#^\./##' | while read -r f; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n 'threads|parallel|deny|lint|future|auto-tests|autotest|aws|AWS|test' "$f" || true
fi
doneRepository: y-scope/clp
Length of output: 28113
Restore the AWS environment after spoofing credentials.
This test sets process-global AWS_* variables but leaves them in place after returning. Tests under #[cfg(test)] may run in parallel, and later tests in the same process can inherit the fake credentials or fail due to the mutated environment. Save the previous values with a guard and restore/remove them in all code paths.
🤖 Prompt for 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.
In `@components/clp-tdl-package/src/task/compression/compress.rs` around lines 971
- 992, Update the s3_credential_env_default test to preserve and restore the
original AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_SESSION_TOKEN values
using a guard that cleans up on every exit path, including assertion or
credential-resolution failure. Keep the existing spoofed credentials and
assertions unchanged.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
…st; drop the now-unused `Credentials` import.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/clp-tdl-package/src/task/compression/compress.rs (1)
974-994: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the AWS environment variables after the test.
s3_credential_env_defaultsetsAWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY, andAWS_SESSION_TOKENwithunsafe { std::env::set_var(...) }and never removes or restores them. Process env vars are global state. If another test in the same binary reads these variables, or the default credential provider chain runs elsewhere in the same process, that test can pick up the spoofed values. TheSAFETYcomment asserts no other test touches these variables, but nothing enforces that guarantee, so the assumption can break silently as the test suite grows.This was already raised in a prior review round on the same lines, and a human reviewer separately stated a preference not to add this test for the same reason. The test was reinstated without addressing the underlying env-mutation issue.
Add a guard that captures the prior values (if any) and restores or removes them on every exit path, including assertion failure.
🛡️ Proposed fix using a restore guard
+ struct EnvVarGuard { + key: &'static str, + prev: Option<String>, + } + + impl EnvVarGuard { + fn set(key: &'static str, value: &str) -> Self { + let prev = std::env::var(key).ok(); + // SAFETY: test-only, restored on drop. + unsafe { std::env::set_var(key, value) }; + Self { key, prev } + } + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + // SAFETY: test-only, restoring the pre-test value. + unsafe { + match &self.prev { + Some(v) => std::env::set_var(self.key, v), + None => std::env::remove_var(self.key), + } + } + } + } + fn s3_credential_env_default() { let runtime = tokio::runtime::Runtime::new().expect("failed to create Tokio runtime"); - // SAFETY: No other test in this binary reads or writes these env vars, and the env - // provider is the first source in the SDK's default chain, so the test is deterministic - // and network-free. - unsafe { - std::env::set_var("AWS_ACCESS_KEY_ID", "the-env-access-key"); - std::env::set_var("AWS_SECRET_ACCESS_KEY", "the-env-secret-key"); - std::env::set_var("AWS_SESSION_TOKEN", "the-env-session-token"); - } + let _access_key_guard = EnvVarGuard::set("AWS_ACCESS_KEY_ID", "the-env-access-key"); + let _secret_key_guard = EnvVarGuard::set("AWS_SECRET_ACCESS_KEY", "the-env-secret-key"); + let _session_token_guard = EnvVarGuard::set("AWS_SESSION_TOKEN", "the-env-session-token");Consider also marking this test
#[serial](via theserial_testcrate) if other tests in this binary may set overlapping AWS env vars, to remove the race entirely rather than relying on convention.🤖 Prompt for 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. In `@components/clp-tdl-package/src/task/compression/compress.rs` around lines 974 - 994, Update s3_credential_env_default to capture each AWS environment variable’s original value before setting test values, then use an unconditional restore guard whose cleanup runs during unwinding as well as normal completion, restoring existing values or removing previously absent variables. If this test binary contains other tests that mutate overlapping AWS variables, mark the test with the project’s serial_test #[serial] attribute.
🤖 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.
Duplicate comments:
In `@components/clp-tdl-package/src/task/compression/compress.rs`:
- Around line 974-994: Update s3_credential_env_default to capture each AWS
environment variable’s original value before setting test values, then use an
unconditional restore guard whose cleanup runs during unwinding as well as
normal completion, restoring existing values or removing previously absent
variables. If this test binary contains other tests that mutate overlapping AWS
variables, mark the test with the project’s serial_test #[serial] attribute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 947322cc-5d46-4e29-8103-1c93d93a32fe
📒 Files selected for processing (1)
components/clp-tdl-package/src/task/compression/compress.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-tdl-package/src/task/compression/compress.rs (1)
352-413: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
AWS_SESSION_TOKENbefore applying the resolved credentials.
run_clp_sandrun_log_converteruseCommand::envs(credential_env), so they inherit existing environment variables. Whens3_credential_envomits the optional token, an unrelated or expiredAWS_SESSION_TOKENfrom the parent can mix with the explicit access/secret key. Set child env with a clean environment before addingcredential_env, or removeAWS_SESSION_TOKENexplicitly.🤖 Prompt for 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. In `@components/clp-tdl-package/src/task/compression/compress.rs` around lines 352 - 413, Update the child-process environment setup in run_clp_s and run_log_converter so AWS_SESSION_TOKEN is cleared when s3_credential_env returns no session token, before applying credential_env. Preserve the resolved access and secret keys, and ensure parent-process token values cannot be inherited or combined with them.
🤖 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.
Outside diff comments:
In `@components/clp-tdl-package/src/task/compression/compress.rs`:
- Around line 352-413: Update the child-process environment setup in run_clp_s
and run_log_converter so AWS_SESSION_TOKEN is cleared when s3_credential_env
returns no session token, before applying credential_env. Preserve the resolved
access and secret keys, and ensure parent-process token values cannot be
inherited or combined with them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72608fa5-d2ca-417e-bb78-689221dcd358
📒 Files selected for processing (1)
components/clp-tdl-package/src/task/compression/compress.rs
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
I don't have more comments, but before merge please make sure:
- It's tested within @hoophalab's cloud environment.
- The CI build can pass.
3 participants
Description
Background
On a cloud K8s deployment with
logs_input.aws_authentication.type: "default"and pod identity provided by an IAM role (IRSA), every Spider-orchestrated compression task fails:clp-s checks exactly three env vars for S3 credentials —
AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY, and (optionally)AWS_SESSION_TOKEN— and nothing else. A pod whose identity comes from an IAM role does not have these vars; it carries a token that only an AWS SDK can resolve for actual env vars.For
type: "default", the Celery compression worker already resolves the credentials through the AWS SDK for Python (boto3) and sets the three env vars on the clp-s subprocess (get_credential_env_varsinclp_py_utils/s3_utils.py). The Spider TDL package set nothing — clp-s only inherited the pod's ambient env, which works when literal keys are set on the pod but fails under role-based identity.Summary
This PR makes the TDL package do the same using the AWS SDK for Rust: resolve credentials through the SDK's default provider chain once, and set the three env vars for clp-s. Resolution failures now fail the task immediately with a descriptive error instead of clp-s's per-input retry noise.
Additionally, we add the missing
session_tokenfield to the RustAwsCredentials(mirror of Python'sS3Credentials), forwarded to the S3/SQS clients and clp-s's env.Checklist
Validation performed
All unit tests and CI pass
We have incorporated the changes in this PR to rebuild the worker and deploy it on YScope Cloud; compression and search jobs completed successfully under IRSA pod identity with
type: "default".Summary by CodeRabbit
Summary by CodeRabbit