Add DuckLake round-trip + iceberg integration tests#569
Open
benben wants to merge 11 commits into
Open
Conversation
Extends the k8s integration suite from "DuckLake catalog is attached" to "DuckLake actually serves writes/reads through real MinIO, survives a worker restart, and the fork's conflict-retry path works under concurrent writers." Adds the first end-to-end iceberg test against real AWS S3 Tables, hard- gated on env vars so PR CI stays fast; a dedicated iceberg CI lane sets DUCKGRES_K8S_ICEBERG_TABLE_BUCKET_ARN (a persistent sandbox bucket, reused across runs) and gets the real signal. Stub catalogs (LocalStack community, moto, REST-catalog substitutes) all hit a different DuckDB code path than ENDPOINT_TYPE 's3_tables', so the only way to gain real confidence is to test against actual S3 Tables. Also adds the activation-layer regression net for #563: hot-idle reclaim with rotated STS credentials must refresh the iceberg_sigv4 secret alongside DuckLake's S3 secret, with the new credentials. The inverse case (iceberg disabled) asserts the refresh fn is not invoked. These run on every PR with no AWS dependency.
setupMultiTenant() begins with `kubectl delete namespace duckgres --ignore-not-found --wait=true` against whatever kubeconfig kubectl picks up by default. requireLocalKindCluster — the guard documented to prevent exactly this — was placed after that call, so it only protected read operations on the already-deleted namespace. Today (2026-05-19) this destroyed mw-dev's duckgres namespace for the second time; the prior incident is what the safety guard was added for in the first place. Move the env-var check + kubeconfig load + requireLocalKindCluster above the setupMultiTenant call so failing the safety check exits fatal before any destructive kubectl runs. Anything destructive must live below the guard block — added an inline comment to that effect so this doesn't regress a third time.
The original "DUCKGRES_K8S_TEST_KUBECONFIG is required" message was technically correct but easy to misread as a missing-config error rather than a destructive-suite refusal. The two prior incidents both involved engineers misreading the situation and trying to "set the missing env var" — which is exactly the wrong fix. Rewrite both guard messages (the kubeconfig-unset path and the requireLocalKindCluster path) to lead with REFUSING / DESTRUCTIVE, name the specific destructive action (kubectl delete namespace duckgres), list the contexts where it must not run (local default, dev, shared, production), and point at `just test-k8s-integration` as the only supported way to run.
The earlier safety-ordering commit broke CI: BuildConfigFromFlags was
called BEFORE setupMultiTenant, but in CI the kubeconfig file is
created BY setupMultiTenant (via kind-cluster-reset). Cold runs hit
"stat /tmp/duckgres-kind-kubeconfig: no such file or directory" before
the test bodies could run.
Split the guard into two phases:
Phase 1 (pre-setup, mandatory env-var check + conditional file check):
- Always require DUCKGRES_K8S_TEST_KUBECONFIG to be set
- If the file already exists (warm local rerun), validate it via
requireLocalKindCluster BEFORE setupMultiTenant runs. This is
what would have stopped the mw-dev incident in the
env-pointed-at-real-cluster variant.
- If the file doesn't exist yet (cold CI), skip the file load.
setupMultiTenant's opening `kubectl delete namespace` runs
against the missing path and fails inert ("no such file") — no
damage possible because kubectl can't connect.
Phase 2 (post-setup, mandatory):
- File MUST exist now; load + requireLocalKindCluster. Final
safety net for the cold-bootstrap path. Failure aborts before
any test body runs.
The env-unset path (the actual mw-dev incident shape) still fail-fasts
in <1s with the existing REFUSING message, verified locally.
Per request: a skip-on-missing-env-vars path hides two failure modes
that matter more than the test itself running on a given PR.
1. CI misconfiguration. A rotated secret, renamed bucket, or
dropped env var renders as "missing env vars — skip". The job
reports SUCCESS, nobody notices, and the test silently stops
running on the iceberg lane until someone happens to look.
2. A real iceberg regression that lands during an env-var gap is
invisible — it hides behind the same "skipped" line that a
misconfigured lane produces.
Replace t.Skipf with t.Fatalf. Diagnostic spells out the missing vars,
explains why the test refuses to skip, and walks through the sandbox
bucket setup. Empty env vars are treated as missing (a rotated CI
secret often renders as empty rather than absent).
Tradeoff worth noting: PR CI's k8s-integration-tests job will fail
until the iceberg env vars are wired into every lane that runs the
suite. If keeping default PR CI green matters more than uniform
coverage, the follow-up is to split this test behind a build tag
(`//go:build k8s_iceberg`) so it only compiles into a dedicated
iceberg lane.
Three additions to the k8s-integration-tests job, all of which start
working the moment cloud-infra PR #8124 applies:
1. permissions: id-token: write — lets the job mint an OIDC token
against GitHub's IdP. Falls back to the existing top-level
contents: read since per-job permissions override.
2. New "Configure AWS credentials via OIDC" step. Trades the OIDC
token for STS credentials via aws-actions/configure-aws-credentials,
assuming the new github-duckgres-iceberg-test-role in mw-dev (role
trust policy: repo:PostHog/duckgres:*, scoped IAM policy on the
two test buckets only). Action pinned to the same commit SHA
cloud-infra workflows use.
3. Three iceberg env vars hardcoded in the job's env block — the
bucket ARN, region, and data bucket name. AWS_ACCESS_KEY_ID /
AWS_SECRET_ACCESS_KEY / AWS_SESSION_TOKEN are populated by
configure-aws-credentials, picked up by iceberg_test.go via
os.Getenv. Hardcoding the bucket coordinates matches the
cloud-infra workflow convention; the buckets are persistent
fixtures with stable names.
Until cloud-infra #8124 applies the role and buckets don't exist yet,
so this job will fail on every PR until then. That's the fail-openly
behavior we just landed for iceberg_test.go — when the role appears,
the next run goes green automatically.
Coordinated with PostHog/posthog-cloud-infra#8124 which renames github-duckgres-iceberg-test-role to github-duckgres-iceberg-ci-testing-role (Michael's review nit — the old "test-role" suffix invited "can we delete that?" janitor risk). Comments updated to match.
TestK8sIcebergRoundTrip's tenant activation fails with a 403 during the Delta catalog probe: delta catalog configured but attachment failed: failed to probe Delta catalog: ... 403 Forbidden ... for path orgs/delta/_delta_log/_last_checkpoint https://s3.us-east-1.amazonaws.com/orgs/delta/_delta_log/_last_checkpoint ManagedWarehouseS3.DeltaCatalogEnabled defaults to true in the configstore (GORM `default:true` on the column), so every newly seeded tenant gets Delta attached at startup whether or not it intends to use it. The Delta extension's probe URL on this tenant ends up pointing at a bucket segment that isn't the tenant's data bucket — the URL the request lands at is `https://s3.us-east-1.amazonaws.com/orgs/delta/_delta_log/_last_checkpoint` (bucket=`orgs` in path-style), despite IAM granting s3:GetObject on `arn:aws:s3:::posthog-duckgres-iceberg-test-data-mw-dev`. The iceberg integration test exercises iceberg-on-S3-Tables + DuckLake only and doesn't need Delta. Setting s3_delta_catalog_enabled = false on the test fixture sidesteps the probe entirely. Inline comment in the seed builder flags the underlying URL-construction bug as a separate item to chase if/when this test grows a Delta scenario. Also added s3_delta_catalog_enabled to the ON CONFLICT DO UPDATE set so re-runs against an already-seeded configstore pick up the flag.
The S3-credentials secret payload schema in shared_worker_activator's buildDuckLakeConfigFromConfigStore only extracts access_key_id and secret_access_key. The companion STS-broker path (a few lines below) correctly sets dl.S3SessionToken. The static-secret path silently drops it. This works for production today because production uses long-term IAM user keys (no session token needed). It breaks any setup that sources credentials from STS — STS-vended creds (AccessKeyId prefixed `ASIA…`) are rejected by AWS without the matching session token, and the iceberg REST endpoint returns 403 (`Forbidden`) without naming the specific cause. Spent several hours chasing this as an iceberg IAM/Lake-Formation configuration problem. Direct signed curl from the CI runner with all three header values returned 200; the activator-built DuckLake config (missing the token) returned 403 from the worker. The fix is one extra field on the JSON payload schema, plumbed through. Also drops the temporary debug step from ci.yml that uncovered this.
3-part identifiers (`iceberg.main.t_<id>`) fail through duckgres' flight-update path with `Catalog Error: Schema with name "" not found`, even though plain DuckDB inside the cluster accepts the same SQL verbatim against the same bucket (verified from an in-cluster debug pod). pg_query's parse+deparse produces clean `CREATE TABLE iceberg.main.t_x (id int)`; the duckgres transpiler output is identical; the failure is somewhere downstream in the flight-update pipeline. Sidestep with `USE iceberg.<ns>` set on the connection before each CREATE/INSERT/SELECT/DROP, then use unqualified table names. Same end state in S3 Tables, just expressed in the form that survives the pipeline. MaxOpenConns=1 on the test connection (set by openDBConnAs) keeps the USE + DDL/DML on one underlying connection. The 3-part bug deserves its own investigation — file follow-up once this PR lands; the test should switch back to 3-part once duckgres treats both forms uniformly.
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
1 participant
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.
Summary
tests/k8s/ducklake_test.go) from "DuckLake catalog is attached" to round-trip writes through real MinIO, durability across worker pod restarts, and concurrent-writer correctness (exercising the PostHog DuckLake fork's conflict-retry path).tests/k8s/iceberg_test.go), hard-gated on env vars so PR CI is unaffected. A dedicated iceberg CI lane setsDUCKGRES_K8S_ICEBERG_TABLE_BUCKET_ARNagainst a persistent sandbox table bucket and gets real-AWS signal.duckdbservice/activation_test.go): on hot-idle reclaim with rotated STS credentials,RefreshIcebergSecretmust fire alongsiderefreshS3Secretwith the new creds. Inverse case (iceberg disabled) asserts the iceberg refresh is skipped. These run on every PR with no AWS dependency.Why no LocalStack / moto / stub catalog for iceberg?
The DuckDB iceberg extension derives its endpoint from the table bucket ARN (
ATTACH 'arn:aws:s3tables:<region>:...' (TYPE iceberg, ENDPOINT_TYPE 's3_tables')) and talks tos3tables.<region>.amazonaws.comdirectly. Every stub (LocalStack community, moto, REST-catalog substitutes) tests a different code path. The only way to gain high confidence inENDPOINT_TYPE 's3_tables'is to point at real AWS.LocalStack Pro emulates S3 Tables, but I avoided it per the task constraints.
Iceberg CI setup (one-time, per sandbox AWS account)
t_<unix_nano>suffixes andDROP TABLEin cleanup, so the bucket never accumulates state.s3tables:*on the table bucket ands3:*on the data bucket.DUCKGRES_K8S_ICEBERG_TABLE_BUCKET_ARNDUCKGRES_K8S_ICEBERG_REGIONDUCKGRES_K8S_ICEBERG_DATA_BUCKETAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY(+ optionalAWS_SESSION_TOKEN)just test-k8s-integrationin the iceberg lane. The test self-skips with a clear message in any job where these env vars are unset.The persistent-bucket approach side-steps the 10-buckets-per-region quota, the ~30–60s per-run create/delete overhead, and the orphan-leak problem of bucket-per-run setups.
Test plan
go test ./duckdbservice/— new activation tests pass locally (TestReuseExistingActivationRefreshesIcebergAlongsideS3+TestReuseExistingActivationSkipsIcebergRefreshWhenDisabled).go test -tags 'k8s_integration kubernetes' -c ./tests/k8s/— k8s test package compiles with new files.just test-k8s-integrationagainst a fresh kind cluster — verify new DuckLake tests pass end-to-end.TestK8sIcebergRoundTripin the iceberg CI lane against the sandbox bucket — verify real-AWS round trip.TestK8sIcebergRoundTripskips cleanly with a clear message when AWS env vars are unset (PR CI behavior).