feat(server): explicit OURIOS_S3_* S3 credentials (RFC 0019 §9 / RFC0019.8) - #307
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional explicit S3 credential fields ( ChangesExplicit S3 Credentials (RFC0019.8)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
Add optional access_key_id/secret_access_key/session_token to S3Config (applied explicit-over-chain in Store::s3, which fails fast on a partial pair / token alone, naming only the key). Hand-written Debug redacts the credential values (deriving would leak them). RFC 0019 §9 / RFC0019.8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…chain config_from_env reads OURIOS_S3_ACCESS_KEY_ID/SECRET_ACCESS_KEY/SESSION_TOKEN and applies them via with_s3_credentials (trim, empty→unset); the from_env chain is the fallback IRSA depends on. Never logged. RFC 0019 §9 / RFC0019.8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… authenticate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
rfc0019-credential-config-impl
branch
from
June 28, 2026 20:25
ed4a11c to
8bb41cb
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements RFC 0019’s amendment to allow supplying S3 credentials via Ourios/S3-named environment variables (OURIOS_S3_ACCESS_KEY_ID, OURIOS_S3_SECRET_ACCESS_KEY, OURIOS_S3_SESSION_TOKEN) with an explicit-over-chain model (fallback remains the AWS credential chain to preserve IRSA support), while extending redaction guarantees to these newly read secrets.
Changes:
- Extend
S3Config/Store::s3to accept optional explicit credentials, validate partial-set failures, and redact credentials inDebug. - Teach
ourios-serverconfig resolution to read/applyOURIOS_S3_*credentials intoStoreConfig(trim/blank-as-unset) and widen no-leak assertions. - Update RFC 0019 doc to promote the amendment into the body (incl. new RFC0019.8 scenario) and add an integration test running with only
OURIOS_S3_*keys set.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| docs/rfcs/0019-storage-backend-selection.md | Promotes explicit S3 credential env vars into the RFC body; adds RFC0019.8 scenario and updates testing strategy/history. |
| crates/ourios-server/tests/rfc0019_storage_backend.rs | Adds ignored localstack integration test that runs receiver+querier with only OURIOS_S3_* creds set (no AWS_* in child env). |
| crates/ourios-server/src/main.rs | Reads OURIOS_S3_* creds and applies them to StoreConfig::S3; extends redaction/no-leak tests; adds unit test for credential application semantics. |
| crates/ourios-parquet/src/store.rs | Adds credential fields to S3Config, manual redacting Debug, explicit-over-chain application in Store::s3, and validation/unit tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address #307 review: - Store::s3 partial-credential errors now name only the missing/offending key with a generic 'required together' explanation (was naming both keys). - The §9 amendment was folded into the RFC body (§3.4 / §5 RFC0019.8), so the code/doc comments that referenced §9.3/§9.4/§9.5/§9.6/§9 now point at §3.4 and §5 (RFC0019.8). RFC 0003 §9 references are unrelated and unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/ourios-parquet/src/store.rs`:
- Around line 429-435: The explicit-credential path in AmazonS3Builder setup is
inheriting ambient session credentials from from_env(), which can leak an
AWS_SESSION_TOKEN into the access-key/secret-key branch. Update the builder
logic in the S3 store setup so the explicit credentials branch starts from a
clean builder and only applies with_access_key_id, with_secret_access_key, and
optional with_token there, while reserving from_env() for the fallback path when
no explicit credentials are provided.
In `@crates/ourios-server/tests/rfc0019_storage_backend.rs`:
- Around line 222-224: The test helper only clears the static AWS keys, so the
child can still pick up credentials from profile, IRSA, container, or metadata
sources. Update the test setup in the storage backend RFC test helper to remove
the remaining AWS credential-chain environment variables alongside the existing
env_remove calls, and keep metadata lookup disabled in the same setup so the
server is forced to use only OURIOS_S3_*.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a67db44-90a5-42d0-b2d1-f732ceedc0e3
📒 Files selected for processing (4)
crates/ourios-parquet/src/store.rscrates/ourios-server/src/main.rscrates/ourios-server/tests/rfc0019_storage_backend.rsdocs/rfcs/0019-storage-backend-selection.md
Address #307 review: - Don't let chain credentials bleed into the explicit pair (CodeRabbit, Major): from_env() can retain an ambient AWS_SESSION_TOKEN that the with_* setters do not clear. The explicit-credential branch now builds from AmazonS3Builder::new() (clean); from_env() is used only for the no-explicit-creds fallback. - Normalize the explicit credential fields in Store::s3 (trim, empty/whitespace → unset) so a blank reads consistently across every S3Config caller and can't trip the partial-set fail-fast. New s3_treats_blank_credentials_as_unset test. - Test docs: don't overclaim 'only the explicit path can authenticate' (a shared profile / IRSA env could still feed the chain); the rfc0019_8 #[ignore] reason no longer claims AWS_* env is required (creds default to 'test'). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-creds helper CodeRabbit hardening: the helper cleared only the static AWS keys, so the child could still source credentials from a shared profile / IRSA / container / metadata. Remove the rest of the chain env and disable EC2 metadata, so the explicit OURIOS_S3_* keys are demonstrably the only credentials available. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jun 28, 2026
…ox test image Now that the binary reads explicit OURIOS_S3_* credentials (RFC 0019 §3.4 / #307), the chart's static-credential Secret documents the S3-named keys (OURIOS_S3_ACCESS_KEY_ID / OURIOS_S3_SECRET_ACCESS_KEY [/ OURIOS_S3_SESSION_TOKEN]) instead of the AWS-SDK names — injected via envFrom, working with any S3-compatible provider. IRSA stays the AWS-EKS option (AWS credential chain). Also pins the receiver-only helm-test image to busybox:1.37.0 (a floating tag could change nc behaviour and break helm test) — the last open #304 review point. helm lint + template verified (envFrom secretRef renders; busybox pinned). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jun 29, 2026
* feat(helm): s3-native chart for the rfc 0019 split topology Reworks the deploy/helm/ourios chart from the single local-PVC StatefulSet into the RFC 0019 S3-native topology: a receiver StatefulSet with a per-replica WAL PVC, a stateless querier Deployment that scales independently and reads S3, and a singleton compactor Deployment. Only the data/audit/manifest live on S3; the WAL is always a local durable PVC, never S3 or emptyDir (CLAUDE.md §3.4/§3.6). Credentials are never plaintext config: an existing Secret (envFrom) or IRSA via serviceAccount annotations. A `local` backend remains as a single-node/dev fallback with a shared data PVC. The binary always runs the compaction role, so the dedicated compactor is the designated sweeper while receiver/querier pods also sweep (safe via publish-CAS); flagged in the README for review. Validated with helm lint + helm template (default, s3, local, IRSA); renders confirm the WAL is a volumeClaimTemplate, the querier has no WAL, S3 env vars are set, and no plaintext AWS keys appear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(helm): only the dedicated compactor sweeps via OURIOS_COMPACTION_ENABLED The receiver and querier workloads now set OURIOS_COMPACTION_ENABLED=0 (the new binary flag), so a single dedicated compactor Deployment sweeps instead of every pod. Move the sweep interval to a compactor-only env helper; rewrite the README 'Compactor topology' note (no more N+1 sweepers; replicas>1 safe-but-redundant via publish-CAS, no leader election needed) and the NOTES warning for the all-compaction-off misconfig. helm lint + template (default and s3) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): quote the test-pod query, correct interval + NOTES guidance Review: run the helm-test querier probe via sh -c with single-quoted header/body so the spaces survive; fix the values-table description for compactor.intervalSecs (compactor-only, not every workload); and drop the bad NOTES advice to unset OURIOS_COMPACTION_ENABLED via extraEnv (it would duplicate the env name) — point at compactor.enabled=true instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): address pr #304 review — fail-fast guards, merged annotations, curl test Applies the CodeRabbit/Copilot review on the s3-native chart: - merge per-role podAnnotations with the chart-level map (role wins) via a new ourios.podAnnotations helper, instead of one replacing the other; matches the podLabels behaviour (covers the duplicate receiver-line-36 thread). - fail render when storage.backend=s3 has no storage.s3.bucket (required), and flip the default backend to local so a bare helm template/lint renders zero-config; s3 is documented as the production path. - fail render when compactor.enabled=false (receiver/querier set OURIOS_COMPACTION_ENABLED=0, so the dedicated compactor is the only sweeper — hazard #4). - fail render when both aws.existingSecret and the IRSA role-arn annotation are set (static keys would shadow web-identity creds). - default image.tag to latest, not appVersion 0.0.0 (no image is published for the pre-release crate version); appVersion stays the version label. - gate the NOTES credential warning on the specific eks.amazonaws.com/role-arn key, so unrelated ServiceAccount annotations no longer suppress it. - switch the querier helm-test to curlimages/curl:8.11.1 (BusyBox wget lacks --post-data); curl -f asserts a 2xx. Receiver nc check unchanged. - document local mode as single-node/RWX dev-only (no heavy guard). Validated: helm lint; default(local) + s3 renders; s3-no-bucket, compactor-off, and mixed-auth all fail with their messages; IRSA render has no envFrom and no cred warning; podAnnotations merge confirmed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): cloud-agnostic framing + nil-safe IRSA, test-pod cleanup The chart is S3-compatible, not AWS-specific. Reframe "S3-native"/"AWS" throughout to "S3-compatible object storage" (AWS S3, MinIO, Cloudflare R2, Hetzner, Ceph/RADOS, GCS S3-interop), and make non-AWS providers first-class: storage.s3.endpoint is the knob for any S3-compatible store. Restructure the credential surface to follow the backend, not the cloud: fold credentials under storage.s3.existingSecret (was top-level aws.*), collapse the duplicate region knob into storage.s3.region (drives both OURIOS_S3_REGION and the SDK chain's AWS_DEFAULT_REGION), and rename the ourios.awsEnvFrom helper to ourios.s3CredentialsEnvFrom. The AWS_* key names in the Secret are the S3 SDK convention every S3-compatible provider uses (not AWS-the-cloud-specific); IRSA stays clearly labeled as the AWS EKS-specific option. Also carry the prior review fixes: nil-safe IRSA annotation lookup (index ... | default dict), helm.sh/hook-delete-policy on the test pod, and the curl-based query helm-test (BusyBox wget lacks --post-data). helm lint + helm template verified for local, s3+endpoint+secret, the mutual-exclusion fail, and the missing-bucket fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): use S3-named OURIOS_S3_* credential Secret keys; pin busybox test image Now that the binary reads explicit OURIOS_S3_* credentials (RFC 0019 §3.4 / #307), the chart's static-credential Secret documents the S3-named keys (OURIOS_S3_ACCESS_KEY_ID / OURIOS_S3_SECRET_ACCESS_KEY [/ OURIOS_S3_SESSION_TOKEN]) instead of the AWS-SDK names — injected via envFrom, working with any S3-compatible provider. IRSA stays the AWS-EKS option (AWS credential chain). Also pins the receiver-only helm-test image to busybox:1.37.0 (a floating tag could change nc behaviour and break helm test) — the last open #304 review point. helm lint + template verified (envFrom secretRef renders; busybox pinned). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(helm): correct template/NOTES wording for the local default + cred chain Address #304 review: - receiver/querier headers said the store is 'S3 by default'; the chart defaults to local, so reword to 'local by default, S3 in production'. - compactor header: it is compactor-only because it sets no receiver/querier env, not because those roles are unset (they can run in parallel). - NOTES: the credential note now mentions the AWS credential-chain fallback (shared profile / node IAM / container creds), not just existingSecret/IRSA; the compactor-disabled line no longer asserts receiver/querier state the condition doesn't check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): fail render on an invalid backend; gate S3 creds on the s3 backend Address #304 review: - ourios.storageEnv now fails at render time unless storage.backend is exactly 'local' or 's3' (a typo like 'S3' previously rendered and crashlooped). - ourios.s3CredentialsEnvFrom is gated on backend==s3, so a stray storage.s3.existingSecret on the local backend is neither mounted nor cross-checked against IRSA. - values: reword the storage.s3.region comment — region is optional (applied only when set), may be required depending on provider/SDK, and can also come from the standard AWS env/config. helm template verified: bad backend fails; local ignores a stray secret+IRSA; s3 mounts the secret; s3 + secret + IRSA still fails (mutual exclusion). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): fail render on non-positive querier window / compaction interval Address #304 review: OURIOS_QUERIER_DEFAULT_WINDOW_SECS and OURIOS_COMPACTION_INTERVAL_SECS must be positive integers or the server refuses to start. Validate querier.defaultWindowSecs and compactor.intervalSecs at render time (like the storage.backend guard) so a 0/negative value fails fast instead of crashlooping the pod. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(helm): add the k8s topology diagram (mingrammer diagrams) Restores the Python diagrams (mingrammer) topology PNG for the chart, refreshed to the current design: receiver/querier carry OURIOS_COMPACTION_ENABLED=0 (only the dedicated compactor sweeps — no 'also sweeps' edges), the Secret holds the S3-named OURIOS_S3_* keys, and the store is labelled 'object store (S3 API)' (any S3-compatible provider). Source script committed alongside the PNG and embedded in the README, with the ASCII block kept as a text fallback. docs/ is .helmignore'd so the diagram isn't shipped in the chart package. This README is outside the docs/ mdBook tree, so the §6.7 (Mermaid/SVG) conventions don't apply. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): route diagram cred edges via workloads; ship PNG; guard IRSA SA Address #304 review (diagram + IRSA edge cases): - topology.py: credentials feed the workloads (envFrom/IRSA), which then read/write/sweep the store — not Secret/SA pointing at the store directly (correct trust boundary). PNG regenerated. - .helmignore: ship docs/topology.png (so the README image resolves from the chart artifact / Artifact Hub), exclude only docs/*.py (the generator). - README: chart-relative regen command; note IRSA requires serviceAccount.create=true. - _helpers: fail render when an IRSA role-arn is set with serviceAccount.create=false (the chart renders no SA, so the annotation would silently have no effect). helm lint + template verified: default + s3+IRSA render; s3+IRSA+create=false fails; package ships the PNG and excludes the script. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): bound the helm-test pod (deadline + curl timeouts); clarify diagram source Address #304 review: - test-connection pod gets activeDeadlineSeconds: 60 so a stuck DNS/TCP connect fails helm test deterministically instead of hanging; the querier curl gets --connect-timeout 5 / --max-time 15 (the receiver nc already has -w5). - README: note the topology.py source ships in the repo checkout only (excluded from the packaged chart via .helmignore), so the link resolves on GitHub, not from a chart artifact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(helm): test both roles when both are enabled (split topology) Address #304 review: the test pod used if/else, so a default split-topology install (receiver + querier both enabled) only checked the querier — a broken receiver Service would pass helm test. Emit one container per enabled role (independent ifs); the Pod succeeds only when every container exits 0. Verified: both→query+otlp-http, querier-only→query, receiver-only→otlp-http. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(helm): region is optional; querier reads the configured store (not just S3) Address #304 review: the storageEnv header implied the region is needed for every backend (it's optional, applied only when set, and can come from standard AWS env/config), and the querier values comment said 'reads S3' though both local and s3 backends are supported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jul 10, 2026
* docs(rfc): accept RFC 0019 — storage-backend selection (maintainer sign-off) Promote RFC 0019 green -> accepted (terminal). All eight §5 criteria pass (green since #301, amended #306/#307): backend selection + credential scrub unit tests, and the localstack S3 integration covering WAL-stays-local, ingest→query on S3, the compactor manifest swap, and cross-tenant isolation. No validated stage applies — it's server wiring, not a thesis gate — so acceptance follows green directly (RFC 0001/0008 precedent). §9 records the sign-off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(rfc): sync §6 stage line with accepted status (RFC 0019) Copilot review on #455: §6 still said "the RFC is `green`" after the status flipped to `accepted` — internal inconsistency. Now "accepted (§9)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.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.
What
Implements the RFC 0019 §9 amendment (#306): S3 credentials can be supplied via S3-named env vars, so the credential surface isn't AWS-specific (Ourios runs on AWS S3 and MinIO/R2/Hetzner/Ceph/…).
OURIOS_S3_ACCESS_KEY_ID/OURIOS_S3_SECRET_ACCESS_KEY/OURIOS_S3_SESSION_TOKEN, read by the server and applied to theAmazonS3Builder(with_access_key_id/with_secret_access_key/with_token).AmazonS3Builder::from_env()is the fallback — retained because AWS IRSA needs its ownAWS_ROLE_ARN/AWS_WEB_IDENTITY_TOKEN_FILE(EKS webhook), which have no Ourios-named equivalent.Also promotes the §9 amendment into the RFC body (§3.1 table, §3.4, §5 RFC0019.8, §6) and collapses §9 to a history note; the RFC stays
green(all eight §5 criteria pass).Invariant touched — §3.4 secret hygiene (RFC0019.6 → RFC0019.8)
Widened, not relaxed. Ourios now reads credential material, so it owns redaction:
S3Config'sDebugis hand-written to redact the credential fields (shows presence only) — derivingDebugwould have leaked them.Store::s3names only the key, never the value.rfc0019_6_*no-leak unit test now covers theOURIOS_S3_*secret keys.Tests
ourios-parquet(store):Store::s3accepts a full credential pair; rejects a partial set without echoing the value;S3ConfigDebugredacts credential values.ourios-server(main.rs):rfc0019_8_explicit_s3_credentials_applied(with_s3_credentialslands keys inS3Config, blank→unset,localcarries none); extendedrfc0019_6_*redaction.ourios-server(localstack, CIs3-integration):rfc0019_8_explicit_s3_credentials_authenticate— server configured with the S3 credential keys only (AWS_*removed), ingest→query round-trip succeeds.Verification (local)
cargo fmt --all --check✅cargo clippy --all-targets --all-features✅cargo test -p ourios-parquet -p ourios-server --lib --bins✅ (116 tests)cargo doc --no-deps✅ ·mdbook build✅#[ignore]d locally (no containerd) → run in thes3-integrationCI job.Follow-up
The S3-native Helm chart (#304) adopts the
OURIOS_S3_*credential Secret keys once this lands.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests