Skip to content

feat: cloud object stores, credential chains, and config secret injection - #8

Merged
sionsmith merged 3 commits into
osodevops:mainfrom
manudiv16:feat/cloud-object-stores-credential-chains
Jul 28, 2026
Merged

feat: cloud object stores, credential chains, and config secret injection#8
sionsmith merged 3 commits into
osodevops:mainfrom
manudiv16:feat/cloud-object-stores-credential-chains

Conversation

@manudiv16

@manudiv16 manudiv16 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Combined PR merging the cloud object-store work with the config secret-injection work (supersedes #9).

1. GCS / Azure object stores + credential chains

  • GCS backend: gs:// warehouse paths, optional gcs_bucket_name override and gcs_service_account_path; falls through to Application Default Credentials (GKE Workload Identity Federation, GOOGLE_APPLICATION_CREDENTIALS, gcloud).
  • Azure backend: az:// and abfs://container@account.dfs.core.windows.net/ forms, azure_storage_account_name (required), optional azure_container_name override and azure_access_key; falls through to DefaultAzureCredential (env → Managed Identity → Azure CLI).
  • S3 credential chain documented: when explicit keys are omitted the SDK uses env vars → IMDS → IRSA, enabling EKS IRSA and EC2 instance profiles with zero config.

2. Config secret injection (from #9)

  • Secret newtype for credential fields — deserializes from a plain string or { file = "path" } (Kubernetes projected volumes), Debug redacted (Secret(REDACTED)), values read via expose(). Covers kafka.security.sasl_username/sasl_password, iceberg.aws_access_key_id/aws_secret_access_key/azure_access_key, iceberg.rest.credential/oauth2_client_id/oauth2_client_secret.
  • K2I_* env var overrides with warnings on invalid numeric/enum values (TOML preserved) and on unrecognized K2I_* variables (typo detection).
  • docs/kubernetes.md — projected Secret volumes, env/secretKeyRef injection, Secrets Store CSI Driver, full variable table, rotation/visibility notes.

Merge resolution notes

  • example.toml: combined S3 credential-chain docs with { file = ... } syntax and GCS/Azure sections.
  • writer.rs: kept the IRSA/IMDS comment, uses Secret::expose() at builder call sites.
  • azure_access_key is a Secret for consistency with the other credential fields, plus K2I_ICEBERG_AZURE_ACCESS_KEY.

Verification

  • cargo check --workspace --tests — clean, zero warnings
  • cargo test --workspace — 289 passed, 0 failed
  • cargo fmt --all --check — clean
  • k2i validate --config config/example.toml — OK
  • Env typo smoke test — warns on K2I_KAFKA_TOPC, validate passes
  • 9 new tests: TOML file refs, missing-file error, plain-string compat, Debug redaction, env overrides (mutex-serialized)

GCS/Azure object stores + credential chain docs (PR #8):
- GCS backend with ADC / Workload Identity Federation
- Azure backend (az:// and abfs://) with DefaultAzureCredential chain
- S3 default credential chain docs (env -> IMDS -> IRSA)

Config secret injection (PR #9):
- Secret newtype for credential fields: plain string or
  { file = "path" } TOML forms, Debug redacted, expose() accessors
- K2I_* env var overrides with warnings on invalid numeric/enum
  values and unrecognized variables (typo detection)
- Mutex-serialized env-var tests
- docs/kubernetes.md: projected volumes, env injection, Secrets
  Store CSI Driver, full variable table
- azure_access_key uses Secret for consistency

Closes #1
@manudiv16
manudiv16 force-pushed the feat/cloud-object-stores-credential-chains branch from e5d1077 to 919957a Compare July 25, 2026 17:46
@manudiv16 manudiv16 changed the title feat(iceberg): implement GCS/Azure object stores and document S3 credential chain feat: cloud object stores, credential chains, and config secret injection Jul 25, 2026
@sionsmith

Copy link
Copy Markdown
Contributor

@manudiv16 are you ready for us to review this now?

@manudiv16

Copy link
Copy Markdown
Contributor Author

@manudiv16 are you ready for us to review this now?

Yes, the PR is ready

sionsmith and others added 2 commits July 28, 2026 14:12
The in-bucket warehouse prefix was applied inside generate_file_path, but
that path is also what goes to the catalog, the transaction log, and the
read path — all of which resolve it by joining against warehouse_path.
For a warehouse of s3://bucket/warehouse the catalog recorded
s3://bucket/warehouse/warehouse/data/... while the upload landed at
s3://bucket/warehouse/data/..., so every committed file was unreadable.

Apply the prefix in a dedicated storage_path() used only by upload_file,
keeping every externally-visible path warehouse-relative.

Also fixes three defects in the env-override layer:

- K2I_MONITORING_LOG_FORMAT never took effect: the tracing subscriber is
  configured before the config is loaded and parsed the TOML directly.
- K2I_RPC_ENABLED read any unrecognized value as false, so =yes silently
  disabled an RPC server the TOML had enabled.
- The cloud object-store fields had no overrides at all, including the
  Azure-required azure_storage_account_name.

And suppresses the spurious unrecognized-variable warnings for K2I_E2E_*
and the other harness variables that share the engine's environment.

Docs: docs/configuration.md promised ${VAR} shell substitution, which does
not exist — following it would have authenticated with the literal string.
Replaced with the two real mechanisms. README.md, docs/architecture.md and
docs/configuration.md still described GCS/Azure as unwired.

Verified locally: 319 tests (up from 289), clippy -D warnings, fmt,
cargo-audit, cargo-semver-checks, the Docker Kafka integration tests, and
all five docker/e2e suites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… for real

Three gaps remained after the warehouse-prefix fix.

Secret redacted Debug but not Serialize, and Config derives Serialize, so
any code that dumped or echoed the configuration would emit credentials in
the clear. Redact in both, following the secrecy crate's convention that
emitting a secret must be a conscious act. A serialized Config no longer
round-trips; that trade-off is deliberate and documented, since a visibly
broken credential beats a silently leaked one.

Cloud warehouse settings that cannot be derived from the path are now
checked in Config::validate rather than at writer construction. Azure needs
a storage account name that neither URL form carries; previously a
long-running ingest reported healthy and failed minutes later on its first
flush. Warehouse-path parsing now lives in one place and is shared with the
writer, so validation and store construction cannot drift.

The prefix fix was only unit-tested. Added container-backed S3 round-trip
tests (MinIO) for a prefixed warehouse, a multi-segment prefix, and a
bucket-root warehouse, each asserting that warehouse_path joined with the
reported path resolves to a real object and that nothing landed at the
doubled-prefix or bucket-root locations. Verified they fail against the
previous behaviour with the exact production symptom: a 404 on
s3://bucket/warehouse/prod/warehouse/prod/data/...

Those tests would not have run in CI: every container-backed test is
#[ignore = "requires Docker"], and the integration job never passed
--include-ignored, so it provisioned Docker and ran no Docker test at all,
including the pre-existing Kafka ones. Fixed, and the workflow now also
triggers on docs/ and config/ since a test asserts every K2I_* variable is
documented.

Verified locally: 326 tests, the CI integration command (13 tests incl. 5
container-backed), clippy -D warnings, fmt, cargo-audit, cargo-semver-checks,
and all five docker/e2e suites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sionsmith
sionsmith merged commit 6c207ca into osodevops:main Jul 28, 2026
13 checks passed
@sionsmith

Copy link
Copy Markdown
Contributor

Thanks @manudiv16 — merged and shipped as v0.3.0 🎉

The GCS/Azure backends, the credential chains, and the Secret newtype all landed as designed. I pushed two follow-up commits to this branch before merging, plus some release-side fixes. Writing up what changed and how to verify it, since a couple of these are worth knowing about for future work.


1. The one blocking issue: the warehouse prefix was applied one layer too high

You correctly spotted a real pre-existing bug — on S3, uploads landed at s3://bucket/data/... while the catalog recorded s3://bucket/warehouse/data/.... That diagnosis was right, and it was a good catch.

The fix applied the prefix inside generate_file_path(). The problem is that its return value isn't only used for the upload — it's also what goes to the catalog, the transaction log, and the read path, and all three resolve it by joining against warehouse_path:

  • official.rs::data_file_uri()warehouse_path + relative path
  • read/mod.rs::absolute_data_path() → same join
  • txlog ParquetWritten.file_path

So the prefix got applied twice. For warehouse_path = "s3://bucket/warehouse":

upload lands at:        bucket/warehouse/data/test_db/...
catalog records:  s3://bucket/warehouse/warehouse/data/test_db/...   ← 404

Net effect: every committed file unreadable, on all cloud backends, while the pipeline reports success. The local-filesystem path was unaffected, which is why the existing tests stayed green.

Fix (de8cdf7): the prefix is now applied only when addressing the object store, in a dedicated storage_path() used by upload_file(). Everything externally visible stays warehouse-relative.

// generate_file_path() → "data/db/tbl/....parquet"   (catalog, txlog, read path)
// storage_path(...)    → "warehouse/data/db/tbl/....parquet"  (object store only)

Worth internalising for next time: the parse-helper unit tests were good, but they tested the helpers in isolation — they couldn't catch a defect in where the helper output was used. That needed a test that composes the pieces.

2. Now covered by a real S3 round-trip test

Added container-backed MinIO tests (crates/k2i-core/tests/integration_tests.rs) for a prefixed warehouse, a multi-segment prefix, and a bucket-root warehouse. Each asserts that warehouse_path joined with the reported path resolves to an actual stored object, and that nothing landed at the doubled-prefix or bucket-root locations.

I verified they fail against the original code with the exact production symptom:

catalog URI s3://k2i-test-bucket/warehouse/prod/warehouse/prod/data/...
does not resolve to a stored object: 404 Not Found

3. Smaller fixes in the env-override layer

Issue Detail
K2I_MONITORING_LOG_FORMAT never worked The tracing subscriber is configured in main.rs before the config loads, and read the TOML directly — so the override was silently ignored for all output. Verified by building the pre-fix binary: =text still emitted JSON.
K2I_RPC_ENABLED failed unsafely Any unrecognised value read as false, so =yes would silently disable an RPC server the TOML had enabled. Now warns and preserves the configured value.
No overrides for cloud fields Including azure_storage_account_name, which is required for Azure — an env-only Kubernetes deployment couldn't configure Azure at all.
Spurious typo warnings K2I_E2E_* and the other harness vars share the engine's environment during e2e runs and were being flagged as typos.

4. Secret was only half-redacting

Secret redacted Debug but Serialize was #[serde(transparent)] — and Config derives Serialize. Any future config dump, RPC echo, or diagnostic bundle would have emitted credentials in cleartext.

Both now redact. This follows the secrecy convention of not implementing Serialize for secret-wrapped strings so that emitting one has to be deliberate. The trade-off — a serialized Config no longer round-trips — is intentional and documented: a visibly broken credential beats a silently leaked one.

5. Fail-fast validation

azure_storage_account_name was only checked at writer construction, so a long-running ingest would report healthy and then die on its first flush, minutes later. It's now checked in Config::validate() at startup, along with bucket-less s3:///gs:// paths. Warehouse-path parsing moved into config.rs and is shared with the writer, so validation and store construction can't drift.

6. Docs correction (pre-existing, but relevant here)

docs/configuration.md documented ${VAR} shell substitution for credentials. That mechanism does not exist — I confirmed "${KAFKA_PASSWORD}" is stored verbatim, so anyone following that page would have authenticated with the literal string. Replaced with the two real mechanisms this PR introduces ({ file = "..." } refs and K2I_* overrides), pinned by a test. README.md / docs/architecture.md also still described GCS/Azure as unwired.

7. CI — the container tests were never running

Every container-backed test is #[ignore = "requires Docker"], but the integration job never passed --include-ignored. It provisioned Docker-in-Docker and then ran zero Docker tests — including the pre-existing Kafka ones. Fixed; that job went from 8 to 13 tests.

Separately, the librdkafka 2.12 upgrade in #7 added libcurl4-openssl-dev to test.yml but missed release.yml, docker-publish.yml, and dist-workspace.toml. Those only run on tag pushes, so nothing caught it until v0.3.0 was tagged and failed. All fixed, with scripts/check-workflow-deps.sh now guarding the four copies of that list.


How to test

git checkout main && git pull

# Unit + lib tests (326)
cargo test --workspace

# Container-backed tests — MinIO S3 round-trip + Kafka. Needs Docker.
# Note the --include-ignored; without it these silently don't run.
cargo test --test '*' --all-features -- --include-ignored

# The Iceberg E2E (Kafka → k2i → REST catalog → DuckDB iceberg_scan)
scripts/e2e-docker-iceberg.sh

# New: validate a published release image rather than a local build
scripts/e2e-docker-released.sh v0.3.0

# New: guards the dependency lists that broke the release
scripts/check-workflow-deps.sh

To see the prefix bug for yourself, revert storage_path() to the identity function and move the prefix back into generate_file_path()test_s3_prefixed_warehouse_file_lands_where_catalog_points will fail with the 404 above.

Quick manual check of the config work:

# Secret file ref
echo 'hunter2' > /tmp/pw
# ...with sasl_password = { file = "/tmp/pw" } in your config
k2i validate --config config.toml

# Env override + typo detection
K2I_KAFKA_TOPIC=env-topic K2I_KAFKA_TOPC=typo k2i validate --config config.toml

# Azure fail-fast
K2I_ICEBERG_WAREHOUSE_PATH=az://cont/wh k2i validate --config config.toml

Verified before release

326 unit tests, 13 container-backed tests, all five docker/e2e suites, clippy -D warnings, fmt, cargo-audit, cargo-semver-checks, plus the published v0.3.0 image and both release binaries pulled and run for real.

Still open, flagged not fixed

  • GCS and Azure have no live round-trip test. They're covered at config and store-construction level (including that the credential fall-throughs build without credentials present), but no emulator exercises a real write. S3 now has one via MinIO — an equivalent for the other two would be a genuinely valuable follow-up. Noted honestly in the README rather than implying full cloud coverage.
  • Compaction is a no-op via the CLICompactionTask is constructed without a catalog or object store, so perform_compaction() returns early. Pre-existing and out of scope here, but it sits right next to this object-store work.

Nice work on the credential chains — the ADC / DefaultAzureCredential fall-through is exactly right, and I confirmed both builders construct successfully without credentials present, so Workload Identity and Managed Identity resolve at request time rather than failing at startup.

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.

2 participants