From 162cb62ba1e53a5a2182dc7c7ace467ba7a41bfc Mon Sep 17 00:00:00 2001 From: Bill Guowei Yang Date: Mon, 10 Aug 2026 00:30:22 -0400 Subject: [PATCH 1/2] Add dev multi-worker Trino benchmark lifecycle (fail-closed) Compare one Duckgres worker over PGWire against a multi-worker Trino cluster reading the same per-run DuckLake snapshot, driven by the `posthog_frozen_trino_perf` scenario. Control plane: - Authenticated provision/status/deprovision routes on the existing admin `/api/v1` router. Responses carry only cluster ID, state, endpoint, worker counts, and the pinned image; error bodies are fixed strings and infrastructure detail is logged, never returned. - A Kubernetes lifecycle manager rendering one coordinator, exactly the requested workers (default 4), a ClusterIP Service selecting only the coordinator, coordinator/worker/catalog ConfigMaps, and a short-lived Secret holding only the charts-created metadata reader password. requests == limits (Guaranteed QoS) so Trino does not contend with the Duckgres worker it is compared against. Provision is idempotent and 409s on different ownership, image, or worker count; readiness needs the coordinator AND every requested worker; cleanup selects on the ownership labels, so it is safe after a partial provision and cannot reach a worker pod, another cluster, or the charts Secret. - A reader-identity resolver over the config-store warehouse row and the Duckling CR's charts-published `status.benchmarkReader` block, read through a new credential-free status accessor. It fails closed on any missing field and refuses a configuration whose reader S3 role or reader database user equals the warehouse writer identity. There is no writer-credential fallback path. S3 access is an assumed read-only IAM role (renewable), never static keys; both engines are pinned to UTC. Scenario and artifacts: - The lifecycle client now polls (interval, timeout, max attempts) with `pending` a polling state and `failed` terminal, instead of issuing a single status request. Trino teardown is always_run and ordered before warehouse teardown. - `summary.json` records per-protocol comparison metadata: engine and version (Trino's from the coordinator's /v1/info), connector version, image reference/digest, requested/ready workers, catalog/schema, UTC. No latency thresholds and no CI gating. Configuration is env-only and DISABLED by default; enabling it without a pinned image leaves the lifecycle unbuilt, and a built lifecycle still refuses to provision until the companion charts reader resources exist. The PGWire-only frozen perf scenario is unchanged, with a test asserting it contains no Trino steps. Co-Authored-By: Claude Opus 5 --- .github/workflows/scenario-dev.yml | 12 + CLAUDE.md | 53 ++ configresolve/resolve.go | 50 ++ configresolve/resolve_trino_benchmark_test.go | 90 +++ controlplane/control.go | 5 + controlplane/multitenant.go | 44 ++ controlplane/provisioner/k8s_client.go | 43 +- controlplane/trino_benchmark_api.go | 301 +++++++ .../trino_benchmark_api_authz_test.go | 64 ++ controlplane/trino_benchmark_api_test.go | 391 +++++++++ controlplane/trino_benchmark_manager.go | 741 ++++++++++++++++++ controlplane/trino_benchmark_manager_test.go | 716 +++++++++++++++++ controlplane/trino_benchmark_reader.go | 211 +++++ controlplane/trino_benchmark_reader_k8s.go | 122 +++ .../trino_benchmark_reader_k8s_test.go | 162 ++++ controlplane/trino_benchmark_reader_test.go | 178 +++++ docs/runbooks/scenario-runner.md | 106 ++- justfile | 6 + main.go | 1 + tests/mw-dev/manifests.tmpl.yaml | 9 + tests/mw-dev/run.sh | 17 +- tests/mw-dev/run_sh_test.go | 74 ++ tests/mw-dev/scenario/perf/adapter_test.go | 245 ++++++ tests/mw-dev/scenario/perf/steps.go | 131 +++- tests/mw-dev/scenario/runner_test.go | 151 +++- .../scenarios/posthog_frozen_trino_perf.yaml | 106 +++ tests/mw-dev/scenario/script_test.go | 33 + tests/mw-dev/scenario/trino/client.go | 195 +++++ tests/mw-dev/scenario/trino/client_test.go | 264 +++++++ tests/mw-dev/scenario/trino/steps.go | 306 ++++++++ tests/mw-dev/scenario/trino/steps_test.go | 192 +++++ tests/perf/core/catalog.go | 37 +- tests/perf/core/catalog_test.go | 57 +- tests/perf/core/environment_test.go | 186 +++++ tests/perf/core/intent_matcher.go | 15 +- tests/perf/core/runner.go | 40 + tests/perf/core/types.go | 94 ++- tests/perf/drivers/pgwire/driver.go | 34 + tests/perf/drivers/trino/driver.go | 193 +++++ tests/perf/drivers/trino/driver_test.go | 100 +++ .../perf/queries/ducklake_posthog_tables.yaml | 12 + 41 files changed, 5755 insertions(+), 32 deletions(-) create mode 100644 configresolve/resolve_trino_benchmark_test.go create mode 100644 controlplane/trino_benchmark_api.go create mode 100644 controlplane/trino_benchmark_api_authz_test.go create mode 100644 controlplane/trino_benchmark_api_test.go create mode 100644 controlplane/trino_benchmark_manager.go create mode 100644 controlplane/trino_benchmark_manager_test.go create mode 100644 controlplane/trino_benchmark_reader.go create mode 100644 controlplane/trino_benchmark_reader_k8s.go create mode 100644 controlplane/trino_benchmark_reader_k8s_test.go create mode 100644 controlplane/trino_benchmark_reader_test.go create mode 100644 tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml create mode 100644 tests/mw-dev/scenario/trino/client.go create mode 100644 tests/mw-dev/scenario/trino/client_test.go create mode 100644 tests/mw-dev/scenario/trino/steps.go create mode 100644 tests/mw-dev/scenario/trino/steps_test.go create mode 100644 tests/perf/core/environment_test.go create mode 100644 tests/perf/drivers/trino/driver.go create mode 100644 tests/perf/drivers/trino/driver_test.go diff --git a/.github/workflows/scenario-dev.yml b/.github/workflows/scenario-dev.yml index 53e25000..14ae7861 100644 --- a/.github/workflows/scenario-dev.yml +++ b/.github/workflows/scenario-dev.yml @@ -13,6 +13,15 @@ on: required: false default: "" type: string + trino_benchmark_image: + description: >- + Pinned Trino+Brikk image for the posthog_frozen_trino_perf scenario. + Empty (the default) leaves the Trino benchmark lifecycle disabled. + Never pass credentials here — the metadata reader password and the + read-only S3 role are charts-created cluster resources. + required: false + default: "" + type: string schedule: - cron: "17 8 * * *" @@ -73,6 +82,9 @@ jobs: PR_NUMBER: ${{ github.run_id }} NAMESPACE: duckgres-ci-pr-${{ github.run_id }} DUCKGRES_SCENARIO_MAX_RUNTIME: 4h + # Opt-in only: with no pinned image the control plane keeps the Trino + # benchmark lifecycle disabled and its API answers 503. + DUCKGRES_TRINO_BENCHMARK_IMAGE: ${{ inputs.trino_benchmark_image || '' }} DUCKGRES_SCENARIO_GO_TEST_TIMEOUT: 4h15m # Add process headroom for repeated full-dataset pgwire aggregates. DUCKGRES_K8S_WORKER_CPU_REQUEST: "2" diff --git a/CLAUDE.md b/CLAUDE.md index e85ea0e4..1fe3696e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1201,6 +1201,59 @@ entrypoint), `controlplane/reshard_pod.go` (spawner) + cnpg→ext positive path is unit-only (harness lacks the RDS password); cnpg→cnpg positive path needs a second mw-dev shard (follow-up). +## Trino Benchmark Lifecycle (dev-only, `kubernetes` tag) — LOAD-BEARING CONTRACT + +An opt-in, disposable side-by-side benchmark: one Duckgres worker over PGWire +vs a multi-worker Trino cluster reading the SAME per-run DuckLake snapshot. +Scenario: `tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml`; +runbook: `docs/runbooks/scenario-runner.md`. Pieces: `controlplane/ +trino_benchmark_api.go` (untagged types + admin-authenticated routes), +`trino_benchmark_manager.go` (k8s lifecycle), `trino_benchmark_reader.go` + +`trino_benchmark_reader_k8s.go` (reader identity), `tests/mw-dev/scenario/trino/` +(lifecycle steps + HTTP client), `tests/perf/drivers/trino/` (statement driver). + +- **Fail-closed, with NO writer-credential fallback.** Env-only knobs + (`DUCKGRES_TRINO_BENCHMARK_*`, `configresolve/resolve.go`) default the feature + OFF; enabling it without a pinned image leaves the lifecycle unbuilt; and a + built lifecycle still refuses to provision unless the charts-created reader + identity resolves in full. `buildTrinoReaderIdentity` additionally REJECTS a + configuration whose reader S3 role or reader database user equals the + warehouse writer identity. Never add a degrade path here. +- **Credentials never cross the API boundary.** The lifecycle API returns only + cluster ID, state, endpoint, requested/ready worker counts, and the pinned + image. The metadata reader password exists in control-plane memory for exactly + one hop — read by exact `SecretReference` and written into the cluster-owned + short-lived Secret — and is never logged, returned, or stored on a struct. + Reader status is read through `DucklingClient.GetStatusWithoutCredentials`, so + resolving a reader never pulls the tenant WRITER password into memory. +- **Ownership labels are the cleanup boundary.** Every object carries + `app.kubernetes.io/name=duckgres-trino-benchmark` + + `duckgres.posthog.com/trino-benchmark-cluster=` + + `duckgres.posthog.com/org=`; every delete lists by BOTH the app and + cluster labels. Cleanup is idempotent, safe after a partial provision, and + structurally unable to reach a worker pod, another benchmark cluster, or the + charts-created reader Secret. +- **Readiness means the WHOLE requested topology**: coordinator ready AND + `ReadyWorkers >= RequestedWorkers`. A ready state with no endpoint stays a + polling state; `failed` is terminal and stops the poller. Provision is + idempotent for an identical request (200 vs 202) and 409s on a different org, + image, or worker count — never a silent adoption. +- **S3 access is an assumed read-only role, never static keys.** The catalog + properties set `s3.iam-role`/`s3.role-session-name` (renewable credentials); + `s3.aws-access-key`/`s3.aws-secret-key` must never appear. +- **UTC is pinned in both engines** (`-Duser.timezone=UTC` in jvm.config, the + driver's `X-Trino-Time-Zone`), or cross-engine TIMESTAMPTZ predicates diverge. +- **Artifacts record what ran, not what was intended**: `summary.json` + `environments[]` carries engine/version, connector version, image reference, + requested/ready workers, catalog/schema, and UTC per protocol. No thresholds, + no CI gating. +- Touching any of this → update `controlplane/trino_benchmark_api_test.go`, + `trino_benchmark_api_authz_test.go`, `trino_benchmark_manager_test.go`, + `trino_benchmark_reader_test.go`, `trino_benchmark_reader_k8s_test.go`, + `configresolve/resolve_trino_benchmark_test.go`, + `tests/mw-dev/scenario/trino/*_test.go`, `tests/mw-dev/run_sh_test.go`, and + the scenario DAG assertions in `tests/mw-dev/scenario/runner_test.go`. + ## TODO Reference `TODO.md` is a lightweight backlog for ideas that do not yet have a better diff --git a/configresolve/resolve.go b/configresolve/resolve.go index f2bdf4e3..6533495d 100644 --- a/configresolve/resolve.go +++ b/configresolve/resolve.go @@ -122,6 +122,7 @@ type Resolved struct { K8sExploratoryWorkerTTL time.Duration K8sReshardPodCPU string K8sReshardPodMemory string + TrinoBenchmark controlplane.TrinoBenchmarkSettings AWSRegion string ConfigStoreConn string ConfigPollInterval time.Duration @@ -803,6 +804,54 @@ func ResolveEffective(fileCfg *configloader.FileConfig, cli CLIInputs, getenv fu k8sReshardPodMemory = v } + // Dev-only Trino benchmark lifecycle (env-only, like the other pod-shape + // knobs). Defaults keep it OFF with no image: a deployment must opt in AND + // pin an image, and the charts-created reader identity must resolve, before + // any benchmark cluster can exist. See controlplane.TrinoBenchmarkSettings. + trinoBenchmark := controlplane.TrinoBenchmarkSettings{ + Workers: 4, + ImagePullPolicy: "IfNotPresent", + CoordinatorCPU: "2", + CoordinatorMemory: "8Gi", + WorkerCPU: "2", + WorkerMemory: "8Gi", + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_ENABLED"); v != "" { + if b, err := strconv.ParseBool(v); err == nil { + trinoBenchmark.Enabled = b + } else { + warn("Invalid DUCKGRES_TRINO_BENCHMARK_ENABLED: " + err.Error()) + } + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_IMAGE"); v != "" { + trinoBenchmark.Image = v + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_IMAGE_PULL_POLICY"); v != "" { + trinoBenchmark.ImagePullPolicy = v + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_SERVICE_ACCOUNT"); v != "" { + trinoBenchmark.ServiceAccount = v + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_WORKERS"); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 { + trinoBenchmark.Workers = n + } else { + warn("Invalid DUCKGRES_TRINO_BENCHMARK_WORKERS: " + v) + } + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_COORDINATOR_CPU"); v != "" { + trinoBenchmark.CoordinatorCPU = v + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_COORDINATOR_MEMORY"); v != "" { + trinoBenchmark.CoordinatorMemory = v + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_WORKER_CPU"); v != "" { + trinoBenchmark.WorkerCPU = v + } + if v := getenv("DUCKGRES_TRINO_BENCHMARK_WORKER_MEMORY"); v != "" { + trinoBenchmark.WorkerMemory = v + } + // Connection-string worker-profile config. if v := getenv("DUCKGRES_K8S_ALLOW_CLIENT_WORKER_PROFILE"); v != "" { if b, err := strconv.ParseBool(v); err == nil { @@ -1184,6 +1233,7 @@ func ResolveEffective(fileCfg *configloader.FileConfig, cli CLIInputs, getenv fu K8sExploratoryWorkerTTL: k8sExploratoryWorkerTTL, K8sReshardPodCPU: k8sReshardPodCPU, K8sReshardPodMemory: k8sReshardPodMemory, + TrinoBenchmark: trinoBenchmark, AWSRegion: awsRegion, ConfigStoreConn: configStoreConn, ConfigPollInterval: configPollInterval, diff --git a/configresolve/resolve_trino_benchmark_test.go b/configresolve/resolve_trino_benchmark_test.go new file mode 100644 index 00000000..7607a518 --- /dev/null +++ b/configresolve/resolve_trino_benchmark_test.go @@ -0,0 +1,90 @@ +package configresolve + +import "testing" + +// The Trino benchmark lifecycle is a dev-only comparison harness. It must be +// OFF unless a deployment explicitly turns it on AND pins an image, so an +// ordinary environment can never spin up benchmark clusters. +func TestResolveEffectiveDisablesTrinoBenchmarkByDefault(t *testing.T) { + resolved := ResolveEffective(nil, CLIInputs{}, nil, nil) + + if resolved.TrinoBenchmark.Enabled { + t.Fatal("Trino benchmark lifecycle must default to disabled") + } + if resolved.TrinoBenchmark.Image != "" { + t.Fatalf("Trino benchmark image = %q, want empty by default", resolved.TrinoBenchmark.Image) + } + if resolved.TrinoBenchmark.Workers != 4 { + t.Fatalf("Trino benchmark workers = %d, want the documented default of 4", resolved.TrinoBenchmark.Workers) + } + if resolved.TrinoBenchmark.CoordinatorCPU != "2" || resolved.TrinoBenchmark.CoordinatorMemory != "8Gi" { + t.Fatalf("coordinator shape = %s/%s, want the documented 2/8Gi default", + resolved.TrinoBenchmark.CoordinatorCPU, resolved.TrinoBenchmark.CoordinatorMemory) + } + if resolved.TrinoBenchmark.WorkerCPU != "2" || resolved.TrinoBenchmark.WorkerMemory != "8Gi" { + t.Fatalf("worker shape = %s/%s, want the documented 2/8Gi default", + resolved.TrinoBenchmark.WorkerCPU, resolved.TrinoBenchmark.WorkerMemory) + } + if resolved.TrinoBenchmark.ImagePullPolicy != "IfNotPresent" { + t.Fatalf("image pull policy = %q, want IfNotPresent", resolved.TrinoBenchmark.ImagePullPolicy) + } +} + +func TestResolveEffectiveReadsTrinoBenchmarkEnvironment(t *testing.T) { + env := map[string]string{ + "DUCKGRES_TRINO_BENCHMARK_ENABLED": "true", + "DUCKGRES_TRINO_BENCHMARK_IMAGE": "registry.example/trino-brikk@sha256:abc", + "DUCKGRES_TRINO_BENCHMARK_IMAGE_PULL_POLICY": "Always", + "DUCKGRES_TRINO_BENCHMARK_SERVICE_ACCOUNT": "duckgres-trino-benchmark", + "DUCKGRES_TRINO_BENCHMARK_WORKERS": "6", + "DUCKGRES_TRINO_BENCHMARK_COORDINATOR_CPU": "4", + "DUCKGRES_TRINO_BENCHMARK_COORDINATOR_MEMORY": "16Gi", + "DUCKGRES_TRINO_BENCHMARK_WORKER_CPU": "3", + "DUCKGRES_TRINO_BENCHMARK_WORKER_MEMORY": "12Gi", + } + resolved := ResolveEffective(nil, CLIInputs{}, func(key string) string { return env[key] }, nil) + + settings := resolved.TrinoBenchmark + if !settings.Enabled { + t.Fatal("Trino benchmark lifecycle should be enabled") + } + if settings.Image != "registry.example/trino-brikk@sha256:abc" { + t.Fatalf("image = %q", settings.Image) + } + if settings.ImagePullPolicy != "Always" { + t.Fatalf("image pull policy = %q", settings.ImagePullPolicy) + } + if settings.ServiceAccount != "duckgres-trino-benchmark" { + t.Fatalf("service account = %q", settings.ServiceAccount) + } + if settings.Workers != 6 { + t.Fatalf("workers = %d", settings.Workers) + } + if settings.CoordinatorCPU != "4" || settings.CoordinatorMemory != "16Gi" { + t.Fatalf("coordinator shape = %s/%s", settings.CoordinatorCPU, settings.CoordinatorMemory) + } + if settings.WorkerCPU != "3" || settings.WorkerMemory != "12Gi" { + t.Fatalf("worker shape = %s/%s", settings.WorkerCPU, settings.WorkerMemory) + } +} + +func TestResolveEffectiveWarnsOnInvalidTrinoBenchmarkValues(t *testing.T) { + env := map[string]string{ + "DUCKGRES_TRINO_BENCHMARK_ENABLED": "yes-please", + "DUCKGRES_TRINO_BENCHMARK_WORKERS": "not-a-number", + } + var warnings []string + resolved := ResolveEffective(nil, CLIInputs{}, + func(key string) string { return env[key] }, + func(message string) { warnings = append(warnings, message) }) + + if resolved.TrinoBenchmark.Enabled { + t.Fatal("an unparseable enable flag must leave the feature disabled") + } + if resolved.TrinoBenchmark.Workers != 4 { + t.Fatalf("workers = %d, want the default after an unparseable value", resolved.TrinoBenchmark.Workers) + } + if len(warnings) != 2 { + t.Fatalf("warnings = %v, want one per invalid value", warnings) + } +} diff --git a/controlplane/control.go b/controlplane/control.go index 3653d180..b7380a71 100644 --- a/controlplane/control.go +++ b/controlplane/control.go @@ -211,6 +211,11 @@ type K8sConfig struct { ExploratoryWorkerCPU string ExploratoryWorkerMemory string ExploratoryWorkerTTL time.Duration + + // TrinoBenchmark configures the dev-only, opt-in Trino benchmark + // lifecycle (env-only; see TrinoBenchmarkSettings). Disabled by default: + // the API then answers 503 and no benchmark cluster can be created. + TrinoBenchmark TrinoBenchmarkSettings } // ControlPlane manages the TCP listener and routes connections to Flight SQL workers. diff --git a/controlplane/multitenant.go b/controlplane/multitenant.go index d4bd2fe6..2419f53c 100644 --- a/controlplane/multitenant.go +++ b/controlplane/multitenant.go @@ -664,6 +664,50 @@ func SetupMultiTenant( if router.sharedPool != nil { clusterClient = router.sharedPool.clientset } + // Dev-only Trino benchmark lifecycle. Fail-closed by construction: the + // routes are always registered (so a caller gets a clear 503 rather than a + // 404), but a lifecycle is only built when the deployment opts in, pins an + // image, has a Kubernetes client, and can reach the Duckling client that + // resolves the charts-created read-only reader identity. Any missing piece + // leaves the lifecycle nil — there is no writer-credential fallback. + var trinoBenchmark TrinoBenchmarkLifecycle + if cfg.K8s.TrinoBenchmark.Enabled { + switch { + case clusterClient == nil: + slog.Warn("Trino benchmark lifecycle is enabled but no Kubernetes client is available; the API will report unavailable.") + case dcErr != nil || dc == nil: + slog.Warn("Trino benchmark lifecycle is enabled but the Duckling client is unavailable; the API will report unavailable.", "error", dcErr) + default: + resolver, err := newDucklingTrinoReaderResolver(store, dc) + if err != nil { + slog.Warn("Trino benchmark reader resolution is unavailable; the API will report unavailable.", "error", err) + break + } + manager, err := newTrinoBenchmarkManager(clusterClient, resolver, TrinoBenchmarkManagerConfig{ + Namespace: namespace, + Image: cfg.K8s.TrinoBenchmark.Image, + ImagePullPolicy: cfg.K8s.TrinoBenchmark.ImagePullPolicy, + ServiceAccount: cfg.K8s.TrinoBenchmark.ServiceAccount, + DefaultWorkers: cfg.K8s.TrinoBenchmark.Workers, + CoordinatorCPU: cfg.K8s.TrinoBenchmark.CoordinatorCPU, + CoordinatorMemory: cfg.K8s.TrinoBenchmark.CoordinatorMemory, + WorkerCPU: cfg.K8s.TrinoBenchmark.WorkerCPU, + WorkerMemory: cfg.K8s.TrinoBenchmark.WorkerMemory, + }) + if err != nil { + slog.Warn("Trino benchmark lifecycle is enabled but not configured; the API will report unavailable.", "error", err) + break + } + trinoBenchmark = manager + slog.Info("Trino benchmark lifecycle enabled.", + "namespace", namespace, "image", cfg.K8s.TrinoBenchmark.Image, + "default_workers", cfg.K8s.TrinoBenchmark.Workers) + } + } + // RequireAdmin: the scenario runner authenticates with the internal secret + // (⇒ admin); an SSO viewer must not reach these routes at all. + registerTrinoBenchmarkAPI(api, trinoBenchmark, admin.RequireAdmin()) + admin.RegisterExtras(api, admin.Extras{ Store: store, Live: clusterInfo, diff --git a/controlplane/provisioner/k8s_client.go b/controlplane/provisioner/k8s_client.go index fbb19994..242be865 100644 --- a/controlplane/provisioner/k8s_client.go +++ b/controlplane/provisioner/k8s_client.go @@ -53,7 +53,12 @@ type DucklingStatus struct { } ReshardMaintenance ReshardMaintenanceStatus MetadataCredentialSecretRef SecretReference - DataStore struct { + // BenchmarkReader is the charts-published, strictly read-only identity a + // benchmark Trino cluster uses (status.benchmarkReader). It is absent on + // every Duckling until the companion charts release is deployed, which is + // what keeps the Trino benchmark feature fail-closed. + BenchmarkReader DucklingBenchmarkReader + DataStore struct { Type string BucketName string S3Region string @@ -110,6 +115,16 @@ type ReshardMaintenanceStatus struct { MaintenanceNoLogin bool } +// DucklingBenchmarkReader carries NO credential value — only the reader's +// database role name, the exact Secret reference holding its password, and the +// read-only S3 role the Trino pods may assume. The tenant WRITER identity is +// deliberately not part of this block. +type DucklingBenchmarkReader struct { + MetadataUser string + CredentialSecretRef SecretReference + S3ReadOnlyRoleARN string +} + // SecretReference identifies one key in a namespaced Kubernetes Secret. The // Duckling composition publishes this non-sensitive reference instead of // copying the metadata database password into the CR status. @@ -414,6 +429,19 @@ func (d *DucklingClient) Get(ctx context.Context, name string) (*DucklingStatus, return status, nil } +// GetStatusWithoutCredentials parses a Duckling CR's status WITHOUT resolving +// any Secret value. Get() deliberately resolves the tenant metadata password +// because activation needs it; callers that only need non-secret status — most +// importantly the Trino benchmark reader resolver — must use this instead, so +// a writer credential is never pulled into memory on their behalf. +func (d *DucklingClient) GetStatusWithoutCredentials(ctx context.Context, name string) (*DucklingStatus, error) { + cr, err := d.getCR(ctx, name) + if err != nil { + return nil, fmt.Errorf("get duckling CR %q: %w", name, err) + } + return parseDucklingStatus(cr) +} + // SetReshardMaintenance prepares, fences, or removes the transient CNPG // reshard identity. The composition pins it to sourceShard, so it does not // follow the tenant Role managed resource to the target during cutover. @@ -1367,6 +1395,19 @@ func parseDucklingStatus(cr *unstructured.Unstructured) (*DucklingStatus, error) } } + // Parse status.benchmarkReader (charts-created Trino reader identity). + if reader, ok := status["benchmarkReader"].(map[string]interface{}); ok { + ds.BenchmarkReader.MetadataUser = getNestedString(reader, "metadataUser") + ds.BenchmarkReader.S3ReadOnlyRoleARN = getNestedString(reader, "s3ReadOnlyRoleArn") + if ref, ok := reader["credentialSecretRef"].(map[string]interface{}); ok { + ds.BenchmarkReader.CredentialSecretRef = SecretReference{ + Name: getNestedString(ref, "name"), + Namespace: getNestedString(ref, "namespace"), + Key: getNestedString(ref, "key"), + } + } + } + // Parse status.dataStore if store, ok := status["dataStore"].(map[string]interface{}); ok { ds.DataStore.Type = getNestedString(store, "type") diff --git a/controlplane/trino_benchmark_api.go b/controlplane/trino_benchmark_api.go new file mode 100644 index 00000000..a9cc41d9 --- /dev/null +++ b/controlplane/trino_benchmark_api.go @@ -0,0 +1,301 @@ +package controlplane + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "regexp" + + "github.com/gin-gonic/gin" +) + +// The Trino benchmark API is the ONLY way a scenario run creates or deletes a +// benchmark Trino cluster. Everything credential-shaped stays on the control +// plane: the scenario runner authenticates with the internal secret, names the +// org, and receives back nothing but a cluster ID, a lifecycle state, the +// non-secret in-cluster endpoint, worker counts, and the pinned image +// reference. The metadata reader password and the read-only S3 identity are +// resolved from charts-created resources inside the control plane and never +// cross this boundary — see TrinoReaderIdentity. +// +// The feature is fail-closed: with no configured lifecycle (the default) every +// route answers 503, so a Duckgres deployment without the companion charts +// reader resources simply cannot start a benchmark cluster. + +// TrinoBenchmarkState is the closed set of lifecycle states the API reports. +// pending and failed are distinguishable so a poller can treat one as "keep +// waiting" and the other as terminal. +type TrinoBenchmarkState string + +const ( + // TrinoBenchmarkStatePending means the cluster exists but the coordinator + // or some requested worker replica is not ready yet. Poll again. + TrinoBenchmarkStatePending TrinoBenchmarkState = "pending" + // TrinoBenchmarkStateReady means the coordinator is ready AND every + // requested worker replica is ready. Only then is Endpoint usable. + TrinoBenchmarkStateReady TrinoBenchmarkState = "ready" + // TrinoBenchmarkStateFailed is terminal: the cluster cannot become ready + // without operator action. A poller must stop, not keep waiting. + TrinoBenchmarkStateFailed TrinoBenchmarkState = "failed" +) + +// maxTrinoBenchmarkWorkers bounds what a caller may request, independent of the +// deployment's configured default. A benchmark cluster is a disposable +// side-by-side comparison, not a capacity-planning tool. +const maxTrinoBenchmarkWorkers = 16 + +// TrinoBenchmarkSettings is the deployment configuration for the benchmark +// lifecycle. Like the other pod-shape knobs it is env-only (see +// configresolve/resolve.go) and every value has a documented default: +// +// DUCKGRES_TRINO_BENCHMARK_ENABLED false (fail-closed) +// DUCKGRES_TRINO_BENCHMARK_IMAGE "" (required when enabled) +// DUCKGRES_TRINO_BENCHMARK_IMAGE_PULL_POLICY IfNotPresent +// DUCKGRES_TRINO_BENCHMARK_SERVICE_ACCOUNT "" (pod default SA) +// DUCKGRES_TRINO_BENCHMARK_WORKERS 4 +// DUCKGRES_TRINO_BENCHMARK_COORDINATOR_CPU 2 +// DUCKGRES_TRINO_BENCHMARK_COORDINATOR_MEMORY 8Gi +// DUCKGRES_TRINO_BENCHMARK_WORKER_CPU 2 +// DUCKGRES_TRINO_BENCHMARK_WORKER_MEMORY 8Gi +// +// Enabled alone is not sufficient: without a pinned image and a resolvable +// charts-created reader identity the lifecycle refuses to start. +type TrinoBenchmarkSettings struct { + Enabled bool + Image string + ImagePullPolicy string + ServiceAccount string + Workers int + CoordinatorCPU string + CoordinatorMemory string + WorkerCPU string + WorkerMemory string +} + +// TrinoBenchmarkCluster is the complete, credential-free response body. Every +// field here is safe to write into scenario state, HTTP responses, logs, and +// benchmark artifacts. +type TrinoBenchmarkCluster struct { + ID string `json:"id"` + State TrinoBenchmarkState `json:"state"` + // Endpoint is the in-cluster coordinator URL. Populated once ready. + Endpoint string `json:"endpoint,omitempty"` + // RequestedWorkers / ReadyWorkers make the readiness rule auditable from + // the artifact: a run is only comparable if they match. + RequestedWorkers int `json:"requested_workers,omitempty"` + ReadyWorkers int `json:"ready_workers"` + // Image is the pinned Trino+Brikk image reference (digest where the + // deployment pins one). It is the authoritative record of which engine and + // connector build produced the numbers. + Image string `json:"image,omitempty"` +} + +// TrinoBenchmarkRequest is the provision body. It deliberately cannot carry an +// image, a namespace, credentials, or any other infrastructure knob: those come +// from control-plane configuration only. Unknown fields are rejected so a +// caller cannot believe it configured something it did not. +type TrinoBenchmarkRequest struct { + // Workers is the requested worker replica count. 0 means "use the + // control plane's configured default" (4). + Workers int `json:"workers,omitempty"` + // RunID is an opaque, non-secret scenario run identifier recorded as a + // label so a leftover cluster can be traced back to its run. + RunID string `json:"run_id,omitempty"` +} + +// TrinoBenchmarkProvisionResult distinguishes a fresh provision from an +// idempotent no-op so the API can answer 202 vs 200 truthfully. +type TrinoBenchmarkProvisionResult struct { + Cluster TrinoBenchmarkCluster + // Created is true only when this call actually created resources. + Created bool +} + +// TrinoBenchmarkLifecycle owns the short-lived benchmark cluster associated +// with one managed warehouse. The Kubernetes implementation is +// trinoBenchmarkManager (kubernetes build tag). +type TrinoBenchmarkLifecycle interface { + // ProvisionTrinoBenchmark is idempotent for an identical request and + // returns ErrTrinoBenchmarkConflict when a cluster of the same name exists + // with different ownership or configuration. + ProvisionTrinoBenchmark(ctx context.Context, orgID string, request TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) + // TrinoBenchmarkStatus returns ErrTrinoBenchmarkNotFound for an unknown + // cluster. + TrinoBenchmarkStatus(ctx context.Context, clusterID string) (TrinoBenchmarkCluster, error) + // DeprovisionTrinoBenchmark is idempotent and safe after a partial + // provision; it deletes only resources owned by clusterID. + DeprovisionTrinoBenchmark(ctx context.Context, clusterID string) error +} + +// Lifecycle error sentinels. Handlers map these to status codes and NEVER +// forward the wrapped error text to the client — an infrastructure error can +// contain a connection string or a Secret value. +var ( + // ErrTrinoBenchmarkNotFound: no cluster with that ID. + ErrTrinoBenchmarkNotFound = errors.New("trino benchmark cluster not found") + // ErrTrinoBenchmarkConflict: a same-named cluster exists with different + // ownership or configuration. Never silently adopted. + ErrTrinoBenchmarkConflict = errors.New("trino benchmark cluster conflict") + // ErrTrinoBenchmarkDisabled: the feature is switched off in this + // deployment. + ErrTrinoBenchmarkDisabled = errors.New("trino benchmark lifecycle is disabled") + // ErrTrinoBenchmarkConfig: the deployment is missing required + // configuration — most importantly the charts-created reader identity. + // This is the fail-closed path: it never degrades to writer credentials. + ErrTrinoBenchmarkConfig = errors.New("trino benchmark configuration is incomplete") + // ErrTrinoBenchmarkInvalidRequest: caller-supplied input is unusable. + ErrTrinoBenchmarkInvalidRequest = errors.New("invalid trino benchmark request") +) + +// trinoBenchmarkNameRe constrains both the org ID and the cluster ID to +// characters that are safe in a Kubernetes object name and a label value. It is +// deliberately stricter than the API needs so a malformed caller can never +// steer resource naming. +var trinoBenchmarkNameRe = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$`) + +// registerTrinoBenchmarkAPI mounts the lifecycle routes on an +// admin-authenticated route group. requireAdmin is passed in (rather than +// assumed) so the caller decides the gate; the scenario runner authenticates +// with the internal secret, which resolves to admin. +// +// lifecycle may be nil: the deployment then answers 503 everywhere, which is +// the intended state until the companion charts reader resources exist. +func registerTrinoBenchmarkAPI(r gin.IRouter, lifecycle TrinoBenchmarkLifecycle, requireAdmin gin.HandlerFunc) { + h := trinoBenchmarkHandler{lifecycle: lifecycle} + r.POST("/trino-benchmarks/orgs/:org_id/provision", requireAdmin, h.provision) + r.GET("/trino-benchmarks/status/:cluster_id", requireAdmin, h.status) + r.POST("/trino-benchmarks/deprovision/:cluster_id", requireAdmin, h.deprovision) +} + +type trinoBenchmarkHandler struct{ lifecycle TrinoBenchmarkLifecycle } + +// available reports whether a lifecycle is wired, answering 503 when not. +func (h trinoBenchmarkHandler) available(c *gin.Context) bool { + if h.lifecycle == nil { + c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Trino benchmark lifecycle is not configured"}) + return false + } + return true +} + +func (h trinoBenchmarkHandler) provision(c *gin.Context) { + if !h.available(c) { + return + } + orgID := c.Param("org_id") + if !trinoBenchmarkNameRe.MatchString(orgID) { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid org id"}) + return + } + request, err := decodeTrinoBenchmarkRequest(c.Request.Body) + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + + result, err := h.lifecycle.ProvisionTrinoBenchmark(c.Request.Context(), orgID, request) + if err != nil { + writeTrinoBenchmarkError(c, "provision", orgID, err) + return + } + // 202 on a real create (resources are converging), 200 on an idempotent + // repeat so a retrying caller can tell the two apart. + status := http.StatusOK + if result.Created { + status = http.StatusAccepted + } + c.JSON(status, result.Cluster) +} + +func (h trinoBenchmarkHandler) status(c *gin.Context) { + if !h.available(c) { + return + } + clusterID := c.Param("cluster_id") + if !trinoBenchmarkNameRe.MatchString(clusterID) { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid cluster id"}) + return + } + cluster, err := h.lifecycle.TrinoBenchmarkStatus(c.Request.Context(), clusterID) + if err != nil { + writeTrinoBenchmarkError(c, "status", clusterID, err) + return + } + c.JSON(http.StatusOK, cluster) +} + +func (h trinoBenchmarkHandler) deprovision(c *gin.Context) { + if !h.available(c) { + return + } + clusterID := c.Param("cluster_id") + if !trinoBenchmarkNameRe.MatchString(clusterID) { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid cluster id"}) + return + } + err := h.lifecycle.DeprovisionTrinoBenchmark(c.Request.Context(), clusterID) + // Teardown is always_run in the scenario DAG, so "already gone" is the + // success case, not an error to escalate. + if err != nil && !errors.Is(err, ErrTrinoBenchmarkNotFound) { + writeTrinoBenchmarkError(c, "deprovision", clusterID, err) + return + } + c.Status(http.StatusNoContent) +} + +// decodeTrinoBenchmarkRequest parses an optional JSON body. An empty body is +// valid and means "all defaults"; unknown fields are rejected so a caller +// cannot think it passed an image, a namespace, or a credential. +func decodeTrinoBenchmarkRequest(body io.Reader) (TrinoBenchmarkRequest, error) { + var request TrinoBenchmarkRequest + if body == nil { + return request, nil + } + dec := json.NewDecoder(body) + dec.DisallowUnknownFields() + if err := dec.Decode(&request); err != nil { + if errors.Is(err, io.EOF) { + return TrinoBenchmarkRequest{}, nil + } + // The body is caller-supplied, but the decoder echoes offending field + // names, so keep the message generic. + return TrinoBenchmarkRequest{}, fmt.Errorf("request body must be a Trino benchmark request object") + } + if request.Workers < 0 || request.Workers > maxTrinoBenchmarkWorkers { + return TrinoBenchmarkRequest{}, fmt.Errorf("workers must be between 0 and %d", maxTrinoBenchmarkWorkers) + } + if request.RunID != "" && !trinoBenchmarkNameRe.MatchString(request.RunID) { + return TrinoBenchmarkRequest{}, fmt.Errorf("run_id must be a DNS-1123 label") + } + return request, nil +} + +// writeTrinoBenchmarkError maps a lifecycle error to a status code and a fixed, +// sanitized message. The underlying error is logged (server-side, where the +// operator can see it) but never returned: infrastructure errors routinely +// contain connection strings and Secret references. +func writeTrinoBenchmarkError(c *gin.Context, operation, subject string, err error) { + status := http.StatusInternalServerError + message := "Trino benchmark " + operation + " failed" + switch { + case errors.Is(err, ErrTrinoBenchmarkNotFound): + status, message = http.StatusNotFound, "Trino benchmark cluster not found" + case errors.Is(err, ErrTrinoBenchmarkConflict): + status, message = http.StatusConflict, "Trino benchmark cluster exists with different ownership or configuration" + case errors.Is(err, ErrTrinoBenchmarkDisabled): + status, message = http.StatusServiceUnavailable, "Trino benchmark lifecycle is disabled" + case errors.Is(err, ErrTrinoBenchmarkConfig): + status, message = http.StatusServiceUnavailable, "Trino benchmark reader identity or image is not configured" + case errors.Is(err, ErrTrinoBenchmarkInvalidRequest): + status, message = http.StatusBadRequest, "invalid Trino benchmark request" + } + if status >= http.StatusInternalServerError { + slog.Error("Trino benchmark lifecycle operation failed.", + "operation", operation, "subject", subject, "error", err) + } + c.JSON(status, gin.H{"error": message}) +} diff --git a/controlplane/trino_benchmark_api_authz_test.go b/controlplane/trino_benchmark_api_authz_test.go new file mode 100644 index 00000000..1ae7033b --- /dev/null +++ b/controlplane/trino_benchmark_api_authz_test.go @@ -0,0 +1,64 @@ +//go:build kubernetes + +package controlplane + +import ( + "net/http" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/posthog/duckgres/controlplane/admin" +) + +// The default-build API tests use a stub admin gate (controlplane/admin is +// kubernetes-tagged). These cases pin the REAL gate the control plane mounts in +// multitenant.go: internal secret ⇒ admin, SSO viewer ⇒ 403, anonymous ⇒ 401. + +func newTrinoBenchmarkAuthzEngine(t *testing.T) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + engine := gin.New() + tokens := admin.NewTokenSet(trinoTestInternalSecret, nil) + api := engine.Group("/api/v1", + admin.AuthMiddleware(tokens, func(string) admin.Role { return admin.RoleViewer }), + admin.RoleGate(), + ) + registerTrinoBenchmarkAPI(api, &fakeTrinoBenchmarkLifecycle{}, admin.RequireAdmin()) + return engine +} + +func TestTrinoBenchmarkAPIAcceptsInternalSecretIdentity(t *testing.T) { + engine := newTrinoBenchmarkAuthzEngine(t) + + rec := trinoBenchmarkRequest(t, engine, http.MethodGet, "/api/v1/trino-benchmarks/status/trino-bench-bench-org", "", true) + if rec.Code != http.StatusOK { + t.Fatalf("internal-secret status = %d body = %s, want 200", rec.Code, rec.Body.String()) + } +} + +func TestTrinoBenchmarkAPIRejectsAnonymousAndViewerIdentities(t *testing.T) { + engine := newTrinoBenchmarkAuthzEngine(t) + + anonymous := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":4}`, false) + if anonymous.Code != http.StatusUnauthorized { + t.Fatalf("anonymous provision = %d, want 401", anonymous.Code) + } + + // An SSO viewer authenticates but must not reach any Trino benchmark route, + // including the GET (RequireAdmin, not just RoleGate's mutation gate). + viewerEngine := gin.New() + viewerAPI := viewerEngine.Group("/api/v1", + func(c *gin.Context) { + c.Set("duckgres_identity", &admin.Identity{Email: "viewer@posthog.com", Role: admin.RoleViewer, Source: "sso"}) + c.Next() + }, + admin.RoleGate(), + ) + registerTrinoBenchmarkAPI(viewerAPI, &fakeTrinoBenchmarkLifecycle{}, admin.RequireAdmin()) + + viewer := trinoBenchmarkRequest(t, viewerEngine, http.MethodGet, "/api/v1/trino-benchmarks/status/trino-bench-bench-org", "", false) + if viewer.Code != http.StatusForbidden { + t.Fatalf("viewer status = %d, want 403", viewer.Code) + } +} diff --git a/controlplane/trino_benchmark_api_test.go b/controlplane/trino_benchmark_api_test.go new file mode 100644 index 00000000..e623fc2c --- /dev/null +++ b/controlplane/trino_benchmark_api_test.go @@ -0,0 +1,391 @@ +package controlplane + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "sort" + "strings" + "testing" + + "github.com/gin-gonic/gin" +) + +// fakeTrinoBenchmarkLifecycle is an in-memory stand-in for the Kubernetes +// lifecycle manager. It never holds credential material, mirroring the real +// manager's contract. +type fakeTrinoBenchmarkLifecycle struct { + provision func(context.Context, string, TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) + status func(context.Context, string) (TrinoBenchmarkCluster, error) + deprovision func(context.Context, string) error + + provisionCalls int + deprovisionCalls int + lastRequest TrinoBenchmarkRequest + lastOrgID string + lastClusterID string +} + +func (f *fakeTrinoBenchmarkLifecycle) ProvisionTrinoBenchmark(ctx context.Context, orgID string, request TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) { + f.provisionCalls++ + f.lastOrgID = orgID + f.lastRequest = request + if f.provision != nil { + return f.provision(ctx, orgID, request) + } + return TrinoBenchmarkProvisionResult{ + Cluster: TrinoBenchmarkCluster{ID: "trino-bench-" + orgID, State: TrinoBenchmarkStatePending}, + Created: true, + }, nil +} + +func (f *fakeTrinoBenchmarkLifecycle) TrinoBenchmarkStatus(ctx context.Context, clusterID string) (TrinoBenchmarkCluster, error) { + f.lastClusterID = clusterID + if f.status != nil { + return f.status(ctx, clusterID) + } + return TrinoBenchmarkCluster{ID: clusterID, State: TrinoBenchmarkStateReady, Endpoint: "http://trino:8080"}, nil +} + +func (f *fakeTrinoBenchmarkLifecycle) DeprovisionTrinoBenchmark(ctx context.Context, clusterID string) error { + f.deprovisionCalls++ + f.lastClusterID = clusterID + if f.deprovision != nil { + return f.deprovision(ctx, clusterID) + } + return nil +} + +const trinoTestInternalSecret = "test-internal-secret" + +// newTrinoBenchmarkTestEngine mounts the API with a stand-in for the admin +// gate. The real admin.AuthMiddleware/RoleGate/RequireAdmin wiring lives under +// the kubernetes build tag, so it is exercised in +// trino_benchmark_api_authz_test.go; here the stub keeps the handler behavior +// itself testable in the default build. +func newTrinoBenchmarkTestEngine(lifecycle TrinoBenchmarkLifecycle) *gin.Engine { + gin.SetMode(gin.TestMode) + engine := gin.New() + api := engine.Group("/api/v1") + registerTrinoBenchmarkAPI(api, lifecycle, stubRequireAdmin()) + return engine +} + +// stubRequireAdmin mirrors admin.RequireAdmin's contract: reject an +// unauthenticated caller with 401 before the handler runs. +func stubRequireAdmin() gin.HandlerFunc { + return func(c *gin.Context) { + if c.GetHeader("X-Duckgres-Internal-Secret") != trinoTestInternalSecret { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "not authenticated"}) + return + } + c.Next() + } +} + +func trinoBenchmarkRequest(t *testing.T, engine *gin.Engine, method, path, body string, auth bool) *httptest.ResponseRecorder { + t.Helper() + var reader *bytes.Reader + if body == "" { + reader = bytes.NewReader(nil) + } else { + reader = bytes.NewReader([]byte(body)) + } + req := httptest.NewRequest(method, path, reader) + if body != "" { + req.Header.Set("Content-Type", "application/json") + } + if auth { + req.Header.Set("X-Duckgres-Internal-Secret", trinoTestInternalSecret) + } + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + return rec +} + +func TestTrinoBenchmarkAPIRequiresAuthentication(t *testing.T) { + engine := newTrinoBenchmarkTestEngine(&fakeTrinoBenchmarkLifecycle{}) + + for _, tc := range []struct{ method, path string }{ + {http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision"}, + {http.MethodGet, "/api/v1/trino-benchmarks/status/trino-bench-bench-org"}, + {http.MethodPost, "/api/v1/trino-benchmarks/deprovision/trino-bench-bench-org"}, + } { + rec := trinoBenchmarkRequest(t, engine, tc.method, tc.path, "", false) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("%s %s unauthenticated = %d, want 401", tc.method, tc.path, rec.Code) + } + } +} + +func TestTrinoBenchmarkAPIRouteTopology(t *testing.T) { + engine := newTrinoBenchmarkTestEngine(&fakeTrinoBenchmarkLifecycle{}) + + var got []string + for _, route := range engine.Routes() { + if strings.Contains(route.Path, "trino-benchmarks") { + got = append(got, route.Method+" "+route.Path) + } + } + sort.Strings(got) + want := []string{ + "GET /api/v1/trino-benchmarks/status/:cluster_id", + "POST /api/v1/trino-benchmarks/deprovision/:cluster_id", + "POST /api/v1/trino-benchmarks/orgs/:org_id/provision", + } + if len(got) != len(want) { + t.Fatalf("routes = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("routes = %v, want %v", got, want) + } + } +} + +func TestTrinoBenchmarkAPIFailsClosedWithoutLifecycle(t *testing.T) { + engine := newTrinoBenchmarkTestEngine(nil) + + for _, tc := range []struct{ method, path, body string }{ + {http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":4}`}, + {http.MethodGet, "/api/v1/trino-benchmarks/status/trino-bench-bench-org", ""}, + {http.MethodPost, "/api/v1/trino-benchmarks/deprovision/trino-bench-bench-org", ""}, + } { + rec := trinoBenchmarkRequest(t, engine, tc.method, tc.path, tc.body, true) + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf("%s %s without lifecycle = %d, want 503", tc.method, tc.path, rec.Code) + } + } +} + +func TestTrinoBenchmarkAPIProvisionValidatesRequest(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{} + engine := newTrinoBenchmarkTestEngine(lifecycle) + + for name, tc := range map[string]struct{ path, body string }{ + "invalid org id": {"/api/v1/trino-benchmarks/orgs/Bench_Org!/provision", `{"workers":4}`}, + "malformed json": {"/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":`}, + "unknown field": {"/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"metadata_password":"hunter2"}`}, + "negative workers": {"/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":-1}`}, + "worker count large": {"/api/v1/trino-benchmarks/orgs/bench-org/provision", fmt.Sprintf(`{"workers":%d}`, maxTrinoBenchmarkWorkers+1)}, + } { + t.Run(name, func(t *testing.T) { + rec := trinoBenchmarkRequest(t, engine, http.MethodPost, tc.path, tc.body, true) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d body = %s, want 400", rec.Code, rec.Body.String()) + } + }) + } + if lifecycle.provisionCalls != 0 { + t.Fatalf("provision calls = %d, want 0 for rejected requests", lifecycle.provisionCalls) + } +} + +func TestTrinoBenchmarkAPIProvisionAcceptsEmptyBody(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{} + engine := newTrinoBenchmarkTestEngine(lifecycle) + + rec := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", "", true) + if rec.Code != http.StatusAccepted { + t.Fatalf("status = %d body = %s, want 202", rec.Code, rec.Body.String()) + } + if lifecycle.lastRequest.Workers != 0 { + t.Fatalf("workers = %d, want 0 so the control plane applies its configured default", lifecycle.lastRequest.Workers) + } +} + +func TestTrinoBenchmarkAPIProvisionIsIdempotent(t *testing.T) { + created := true + lifecycle := &fakeTrinoBenchmarkLifecycle{ + provision: func(_ context.Context, orgID string, request TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) { + result := TrinoBenchmarkProvisionResult{ + Cluster: TrinoBenchmarkCluster{ + ID: "trino-bench-" + orgID, + State: TrinoBenchmarkStatePending, + RequestedWorkers: request.Workers, + }, + Created: created, + } + created = false + return result, nil + }, + } + engine := newTrinoBenchmarkTestEngine(lifecycle) + + first := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":4}`, true) + if first.Code != http.StatusAccepted { + t.Fatalf("first provision = %d, want 202", first.Code) + } + second := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":4}`, true) + if second.Code != http.StatusOK { + t.Fatalf("repeat provision = %d, want 200", second.Code) + } + + var cluster TrinoBenchmarkCluster + if err := json.Unmarshal(second.Body.Bytes(), &cluster); err != nil { + t.Fatalf("decode repeat provision: %v", err) + } + if cluster.ID != "trino-bench-bench-org" || cluster.RequestedWorkers != 4 { + t.Fatalf("cluster = %+v", cluster) + } +} + +func TestTrinoBenchmarkAPIMapsLifecycleErrorsToStatusCodes(t *testing.T) { + for name, tc := range map[string]struct { + err error + want int + }{ + "conflict": {ErrTrinoBenchmarkConflict, http.StatusConflict}, + "not found": {ErrTrinoBenchmarkNotFound, http.StatusNotFound}, + "disabled": {ErrTrinoBenchmarkDisabled, http.StatusServiceUnavailable}, + "misconfigured": {ErrTrinoBenchmarkConfig, http.StatusServiceUnavailable}, + "invalid": {ErrTrinoBenchmarkInvalidRequest, http.StatusBadRequest}, + "unknown": {errors.New("boom"), http.StatusInternalServerError}, + } { + t.Run(name, func(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{ + provision: func(context.Context, string, TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) { + return TrinoBenchmarkProvisionResult{}, fmt.Errorf("wrapped: %w", tc.err) + }, + } + engine := newTrinoBenchmarkTestEngine(lifecycle) + rec := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":4}`, true) + if rec.Code != tc.want { + t.Fatalf("status = %d body = %s, want %d", rec.Code, rec.Body.String(), tc.want) + } + }) + } +} + +func TestTrinoBenchmarkAPIStatusReportsLifecycleStates(t *testing.T) { + for _, state := range []TrinoBenchmarkState{TrinoBenchmarkStatePending, TrinoBenchmarkStateReady, TrinoBenchmarkStateFailed} { + t.Run(string(state), func(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{ + status: func(_ context.Context, clusterID string) (TrinoBenchmarkCluster, error) { + return TrinoBenchmarkCluster{ + ID: clusterID, + State: state, + Endpoint: "http://trino-bench-bench-org.duckgres.svc.cluster.local:8080", + RequestedWorkers: 4, + ReadyWorkers: 4, + }, nil + }, + } + engine := newTrinoBenchmarkTestEngine(lifecycle) + rec := trinoBenchmarkRequest(t, engine, http.MethodGet, "/api/v1/trino-benchmarks/status/trino-bench-bench-org", "", true) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + var cluster TrinoBenchmarkCluster + if err := json.Unmarshal(rec.Body.Bytes(), &cluster); err != nil { + t.Fatalf("decode: %v", err) + } + if cluster.State != state { + t.Fatalf("state = %q, want %q", cluster.State, state) + } + }) + } +} + +func TestTrinoBenchmarkAPIStatusValidatesClusterID(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{} + engine := newTrinoBenchmarkTestEngine(lifecycle) + + rec := trinoBenchmarkRequest(t, engine, http.MethodGet, "/api/v1/trino-benchmarks/status/NOT%20A%20CLUSTER", "", true) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +func TestTrinoBenchmarkAPIDeprovisionIsIdempotent(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{ + deprovision: func(context.Context, string) error { return nil }, + } + engine := newTrinoBenchmarkTestEngine(lifecycle) + + for i := 0; i < 2; i++ { + rec := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/deprovision/trino-bench-bench-org", "", true) + if rec.Code != http.StatusNoContent { + t.Fatalf("deprovision %d = %d, want 204", i, rec.Code) + } + } + if lifecycle.deprovisionCalls != 2 { + t.Fatalf("deprovision calls = %d, want 2", lifecycle.deprovisionCalls) + } +} + +func TestTrinoBenchmarkAPIDeprovisionTreatsMissingClusterAsDeleted(t *testing.T) { + lifecycle := &fakeTrinoBenchmarkLifecycle{ + deprovision: func(context.Context, string) error { return ErrTrinoBenchmarkNotFound }, + } + engine := newTrinoBenchmarkTestEngine(lifecycle) + + rec := trinoBenchmarkRequest(t, engine, http.MethodPost, "/api/v1/trino-benchmarks/deprovision/trino-bench-bench-org", "", true) + if rec.Code != http.StatusNoContent { + t.Fatalf("deprovision of an absent cluster = %d, want 204", rec.Code) + } +} + +func TestTrinoBenchmarkAPISanitizesErrorsAndNeverEchoesSecrets(t *testing.T) { + const leak = "super-secret-metadata-password" + lifecycle := &fakeTrinoBenchmarkLifecycle{ + provision: func(context.Context, string, TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) { + return TrinoBenchmarkProvisionResult{}, fmt.Errorf("connect to postgres://reader:%s@metadata:5432/ducklake: refused", leak) + }, + status: func(context.Context, string) (TrinoBenchmarkCluster, error) { + return TrinoBenchmarkCluster{}, fmt.Errorf("read secret value %s", leak) + }, + deprovision: func(context.Context, string) error { + return fmt.Errorf("delete secret holding %s", leak) + }, + } + engine := newTrinoBenchmarkTestEngine(lifecycle) + + for _, tc := range []struct{ method, path, body string }{ + {http.MethodPost, "/api/v1/trino-benchmarks/orgs/bench-org/provision", `{"workers":4}`}, + {http.MethodGet, "/api/v1/trino-benchmarks/status/trino-bench-bench-org", ""}, + {http.MethodPost, "/api/v1/trino-benchmarks/deprovision/trino-bench-bench-org", ""}, + } { + rec := trinoBenchmarkRequest(t, engine, tc.method, tc.path, tc.body, true) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("%s %s = %d, want 500", tc.method, tc.path, rec.Code) + } + if strings.Contains(rec.Body.String(), leak) { + t.Fatalf("%s %s response leaked internal error detail: %s", tc.method, tc.path, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "postgres://") { + t.Fatalf("%s %s response leaked a connection string: %s", tc.method, tc.path, rec.Body.String()) + } + } +} + +func TestTrinoBenchmarkClusterJSONExposesOnlyNonSecretFields(t *testing.T) { + raw, err := json.Marshal(TrinoBenchmarkCluster{ + ID: "trino-bench-bench-org", + State: TrinoBenchmarkStateReady, + Endpoint: "http://trino-bench-bench-org.duckgres.svc.cluster.local:8080", + RequestedWorkers: 4, + ReadyWorkers: 4, + Image: "registry.example/trino-brikk@sha256:abc", + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var fields map[string]any + if err := json.Unmarshal(raw, &fields); err != nil { + t.Fatalf("unmarshal: %v", err) + } + allowed := map[string]bool{ + "id": true, "state": true, "endpoint": true, + "requested_workers": true, "ready_workers": true, "image": true, + } + for name := range fields { + if !allowed[name] { + t.Fatalf("cluster JSON exposes unexpected field %q", name) + } + } +} diff --git a/controlplane/trino_benchmark_manager.go b/controlplane/trino_benchmark_manager.go new file mode 100644 index 00000000..99da2c72 --- /dev/null +++ b/controlplane/trino_benchmark_manager.go @@ -0,0 +1,741 @@ +//go:build kubernetes + +package controlplane + +import ( + "context" + "fmt" + "log/slog" + "strconv" + "strings" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes" +) + +// trinoBenchmarkManager is the concrete Kubernetes TrinoBenchmarkLifecycle: it +// renders one coordinator, exactly the requested number of workers, a ClusterIP +// Service, the coordinator/worker/catalog ConfigMaps, and a short-lived Secret +// holding ONLY the charts-created metadata reader password. +// +// Everything it creates is labelled with the cluster ID and the owning org, and +// every read/delete is filtered by those labels — so a benchmark teardown can +// never reach a worker pod, another benchmark cluster, or the charts-created +// reader Secret itself. +// +// There is no cluster state outside Kubernetes: the Service is the ownership +// anchor and its annotations record the configuration fingerprint, so any +// control-plane replica can answer status and run cleanup. + +const ( + // trinoBenchmarkAppLabelKey/Value mark every object the manager owns. + trinoBenchmarkAppLabelKey = "app.kubernetes.io/name" + trinoBenchmarkAppLabelValue = "duckgres-trino-benchmark" + // trinoBenchmarkClusterLabel is the per-cluster ownership label. Cleanup + // selects on it, so an object without it is never deleted. + trinoBenchmarkClusterLabel = "duckgres.posthog.com/trino-benchmark-cluster" + // trinoBenchmarkOrgLabel records which warehouse the cluster reads. + trinoBenchmarkOrgLabel = "duckgres.posthog.com/org" + // trinoBenchmarkRoleLabel separates the coordinator from the workers; the + // Service selects on it so statements only ever reach the coordinator. + trinoBenchmarkRoleLabel = "duckgres.posthog.com/trino-role" + trinoBenchmarkRoleCoordinator = "coordinator" + trinoBenchmarkRoleWorker = "worker" + + // Configuration fingerprint. A repeat provision with a different value is a + // conflict, never a silent adoption. + trinoBenchmarkImageAnnotation = "duckgres.posthog.com/trino-image" + trinoBenchmarkWorkersAnnotation = "duckgres.posthog.com/trino-workers" + trinoBenchmarkRunIDAnnotation = "duckgres.posthog.com/run-id" + trinoBenchmarkOwnerAnnotation = "duckgres.posthog.com/owner" + + // trinoBenchmarkSecretPasswordKey is the only key the short-lived Secret + // ever holds. + trinoBenchmarkSecretPasswordKey = "metadata-password" + // trinoBenchmarkPasswordEnv is the env var the catalog properties + // interpolate with ${ENV:...}. + trinoBenchmarkPasswordEnv = "TRINO_DUCKLAKE_DB_PASSWORD" + + trinoBenchmarkHTTPPort = 8080 + // trinoBenchmarkCatalogName is the Trino catalog the benchmark queries + // address (ducklake..). + trinoBenchmarkCatalogName = "ducklake" + + // defaultTrinoBenchmarkWorkers is the comparison shape the dev benchmark + // was designed around: one Duckgres worker vs four Trino workers. + defaultTrinoBenchmarkWorkers = 4 + + // Documented resource defaults. requests == limits (Guaranteed QoS) so + // benchmark pods neither burst into nor get throttled by whatever else + // shares the node — the Duckgres worker being compared included. + defaultTrinoCoordinatorCPU = "2" + defaultTrinoCoordinatorMemory = "8Gi" + defaultTrinoWorkerCPU = "2" + defaultTrinoWorkerMemory = "8Gi" + + defaultTrinoBenchmarkPullPolicy = string(corev1.PullIfNotPresent) +) + +// TrinoBenchmarkManagerConfig is the explicit control-plane configuration for +// benchmark clusters. Nothing here is caller-supplied: the pinned image, the +// pod shape, and the namespace come from deployment configuration only. +type TrinoBenchmarkManagerConfig struct { + Namespace string + Image string // pinned Trino+Brikk image (digest preferred); required + ImagePullPolicy string // default IfNotPresent + ServiceAccount string // ServiceAccount whose IAM identity may assume the reader role + DefaultWorkers int // default 4 + + CoordinatorCPU string // default 2 + CoordinatorMemory string // default 8Gi + WorkerCPU string // default 2 + WorkerMemory string // default 8Gi + + // ControlPlaneID is recorded as the owner annotation, so an operator can + // see which control plane created a leftover cluster. + ControlPlaneID string +} + +func (c *TrinoBenchmarkManagerConfig) applyDefaults() { + if c.DefaultWorkers <= 0 { + c.DefaultWorkers = defaultTrinoBenchmarkWorkers + } + if c.ImagePullPolicy == "" { + c.ImagePullPolicy = defaultTrinoBenchmarkPullPolicy + } + if c.CoordinatorCPU == "" { + c.CoordinatorCPU = defaultTrinoCoordinatorCPU + } + if c.CoordinatorMemory == "" { + c.CoordinatorMemory = defaultTrinoCoordinatorMemory + } + if c.WorkerCPU == "" { + c.WorkerCPU = defaultTrinoWorkerCPU + } + if c.WorkerMemory == "" { + c.WorkerMemory = defaultTrinoWorkerMemory + } +} + +type trinoBenchmarkManager struct { + clientset kubernetes.Interface + resolver TrinoReaderResolver + cfg TrinoBenchmarkManagerConfig +} + +var _ TrinoBenchmarkLifecycle = (*trinoBenchmarkManager)(nil) + +// newTrinoBenchmarkManager fails closed: without a pinned image or a reader +// resolver there is no safe cluster to build, so the deployment gets no +// lifecycle at all rather than a partially configured one. +func newTrinoBenchmarkManager(clientset kubernetes.Interface, resolver TrinoReaderResolver, cfg TrinoBenchmarkManagerConfig) (*trinoBenchmarkManager, error) { + if clientset == nil { + return nil, fmt.Errorf("%w: no Kubernetes client", ErrTrinoBenchmarkConfig) + } + if resolver == nil { + return nil, fmt.Errorf("%w: no Trino reader identity resolver", ErrTrinoBenchmarkConfig) + } + if strings.TrimSpace(cfg.Image) == "" { + return nil, fmt.Errorf("%w: no pinned Trino benchmark image", ErrTrinoBenchmarkConfig) + } + if strings.TrimSpace(cfg.Namespace) == "" { + return nil, fmt.Errorf("%w: no Trino benchmark namespace", ErrTrinoBenchmarkConfig) + } + // Validate the pod shape up front so a bad quantity surfaces at startup + // rather than mid-scenario. + cfg.applyDefaults() + for name, quantity := range map[string]string{ + "coordinator CPU": cfg.CoordinatorCPU, "coordinator memory": cfg.CoordinatorMemory, + "worker CPU": cfg.WorkerCPU, "worker memory": cfg.WorkerMemory, + } { + if _, err := resource.ParseQuantity(quantity); err != nil { + return nil, fmt.Errorf("%w: invalid Trino benchmark %s %q", ErrTrinoBenchmarkConfig, name, quantity) + } + } + if cfg.DefaultWorkers > maxTrinoBenchmarkWorkers { + return nil, fmt.Errorf("%w: default worker count %d exceeds the %d maximum", + ErrTrinoBenchmarkConfig, cfg.DefaultWorkers, maxTrinoBenchmarkWorkers) + } + return &trinoBenchmarkManager{clientset: clientset, resolver: resolver, cfg: cfg}, nil +} + +// TrinoBenchmarkClusterID is the deterministic per-org cluster name. One +// benchmark cluster per warehouse at a time, so a repeat provision converges +// instead of piling up clusters. +func TrinoBenchmarkClusterID(orgID string) string { + return "trino-bench-" + orgID +} + +func (m *trinoBenchmarkManager) endpoint(clusterID string) string { + return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", clusterID, m.cfg.Namespace, trinoBenchmarkHTTPPort) +} + +func (m *trinoBenchmarkManager) ownershipLabels(clusterID, orgID string) map[string]string { + return map[string]string{ + trinoBenchmarkAppLabelKey: trinoBenchmarkAppLabelValue, + trinoBenchmarkClusterLabel: clusterID, + trinoBenchmarkOrgLabel: orgID, + } +} + +// ownedSelector is the ONLY selector cleanup and status use. It requires both +// the app label and the cluster label, so a stray object carrying one of them +// by accident is still out of reach. +func (m *trinoBenchmarkManager) ownedSelector(clusterID string) string { + return trinoBenchmarkAppLabelKey + "=" + trinoBenchmarkAppLabelValue + "," + + trinoBenchmarkClusterLabel + "=" + clusterID +} + +// ProvisionTrinoBenchmark creates the cluster, or converges onto an existing +// one with identical ownership and configuration. A same-named cluster with a +// different org, image, or worker count is a conflict. +func (m *trinoBenchmarkManager) ProvisionTrinoBenchmark(ctx context.Context, orgID string, request TrinoBenchmarkRequest) (TrinoBenchmarkProvisionResult, error) { + clusterID := TrinoBenchmarkClusterID(orgID) + workers := request.Workers + if workers == 0 { + workers = m.cfg.DefaultWorkers + } + if workers < 1 || workers > maxTrinoBenchmarkWorkers { + return TrinoBenchmarkProvisionResult{}, fmt.Errorf( + "%w: worker count %d is outside 1..%d", ErrTrinoBenchmarkInvalidRequest, workers, maxTrinoBenchmarkWorkers) + } + + existing, err := m.clientset.CoreV1().Services(m.cfg.Namespace).Get(ctx, clusterID, metav1.GetOptions{}) + switch { + case err == nil: + if conflict := trinoBenchmarkOwnershipConflict(existing, orgID, m.cfg.Image, workers); conflict != nil { + return TrinoBenchmarkProvisionResult{}, conflict + } + // Idempotent repeat: converge any object a previous attempt missed, + // then report the cluster without claiming to have created it. + if err := m.applyClusterResources(ctx, clusterID, orgID, workers, request.RunID); err != nil { + return TrinoBenchmarkProvisionResult{}, err + } + cluster, err := m.TrinoBenchmarkStatus(ctx, clusterID) + if err != nil { + return TrinoBenchmarkProvisionResult{}, err + } + return TrinoBenchmarkProvisionResult{Cluster: cluster, Created: false}, nil + case !apierrors.IsNotFound(err): + return TrinoBenchmarkProvisionResult{}, fmt.Errorf("read Trino benchmark service %s: %w", clusterID, err) + } + + if err := m.applyClusterResources(ctx, clusterID, orgID, workers, request.RunID); err != nil { + return TrinoBenchmarkProvisionResult{}, err + } + slog.Info("Provisioned Trino benchmark cluster.", + "cluster_id", clusterID, "org", orgID, "workers", workers, "image", m.cfg.Image) + return TrinoBenchmarkProvisionResult{ + Cluster: TrinoBenchmarkCluster{ + ID: clusterID, + State: TrinoBenchmarkStatePending, + RequestedWorkers: workers, + Image: m.cfg.Image, + }, + Created: true, + }, nil +} + +// applyClusterResources creates every object the cluster needs, treating an +// AlreadyExists as success. Ordering puts the Service (the ownership anchor) +// first so a crash mid-provision still leaves something cleanup can find. +func (m *trinoBenchmarkManager) applyClusterResources(ctx context.Context, clusterID, orgID string, workers int, runID string) error { + identity, err := m.resolver.ResolveTrinoReader(ctx, orgID) + if err != nil { + // Fail closed. Nothing has been created at this point, and there is no + // writer-credential fallback by design. + return fmt.Errorf("resolve Trino reader identity for org %s: %w", orgID, err) + } + + if err := m.applyService(ctx, clusterID, orgID, workers, runID); err != nil { + return err + } + if err := m.applyReaderSecret(ctx, clusterID, orgID, identity); err != nil { + return err + } + if err := m.applyConfigMaps(ctx, clusterID, orgID, identity); err != nil { + return err + } + if err := m.applyDeployment(ctx, clusterID, orgID, trinoBenchmarkRoleCoordinator, 1); err != nil { + return err + } + return m.applyDeployment(ctx, clusterID, orgID, trinoBenchmarkRoleWorker, workers) +} + +func trinoBenchmarkOwnershipConflict(service *corev1.Service, orgID, image string, workers int) error { + if got := service.Labels[trinoBenchmarkOrgLabel]; got != orgID { + return fmt.Errorf("%w: cluster %s is owned by org %q, not %q", + ErrTrinoBenchmarkConflict, service.Name, got, orgID) + } + if got := service.Annotations[trinoBenchmarkImageAnnotation]; got != image { + return fmt.Errorf("%w: cluster %s runs image %q, not the configured pinned image", + ErrTrinoBenchmarkConflict, service.Name, got) + } + if got := service.Annotations[trinoBenchmarkWorkersAnnotation]; got != strconv.Itoa(workers) { + return fmt.Errorf("%w: cluster %s was provisioned with %q workers, not %d", + ErrTrinoBenchmarkConflict, service.Name, got, workers) + } + return nil +} + +func (m *trinoBenchmarkManager) applyService(ctx context.Context, clusterID, orgID string, workers int, runID string) error { + labels := m.ownershipLabels(clusterID, orgID) + selector := map[string]string{ + trinoBenchmarkClusterLabel: clusterID, + trinoBenchmarkRoleLabel: trinoBenchmarkRoleCoordinator, + } + annotations := map[string]string{ + trinoBenchmarkImageAnnotation: m.cfg.Image, + trinoBenchmarkWorkersAnnotation: strconv.Itoa(workers), + trinoBenchmarkOwnerAnnotation: m.cfg.ControlPlaneID, + } + if runID != "" { + annotations[trinoBenchmarkRunIDAnnotation] = runID + } + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterID, + Namespace: m.cfg.Namespace, + Labels: labels, + Annotations: annotations, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Selector: selector, + Ports: []corev1.ServicePort{{ + Name: "http", + Port: trinoBenchmarkHTTPPort, + TargetPort: intstr.FromInt32(trinoBenchmarkHTTPPort), + Protocol: corev1.ProtocolTCP, + }}, + }, + } + _, err := m.clientset.CoreV1().Services(m.cfg.Namespace).Create(ctx, service, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + return fmt.Errorf("create Trino benchmark service %s: %w", clusterID, err) + } + return nil +} + +// applyReaderSecret copies the charts-created reader password into a +// short-lived, cluster-owned Secret. This is the ONLY point where a credential +// value exists in control-plane memory: it is read by exact reference and +// written straight into the Secret. It is never logged, returned, or stored +// on any struct. +func (m *trinoBenchmarkManager) applyReaderSecret(ctx context.Context, clusterID, orgID string, identity TrinoReaderIdentity) error { + ref := identity.MetadataPasswordSecret + source, err := m.clientset.CoreV1().Secrets(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) || apierrors.IsForbidden(err) { + // The companion charts reader resources are not deployed (or the + // RBAC grant is missing). Fail closed. + return fmt.Errorf("%w: metadata reader password Secret %s is unavailable", + ErrTrinoBenchmarkConfig, ref) + } + return fmt.Errorf("read metadata reader password Secret %s: %w", ref, err) + } + password, ok := source.Data[ref.Key] + if !ok || len(password) == 0 { + return fmt.Errorf("%w: metadata reader password Secret %s has no value", ErrTrinoBenchmarkConfig, ref) + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterID + "-metadata", + Namespace: m.cfg.Namespace, + Labels: m.ownershipLabels(clusterID, orgID), + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{trinoBenchmarkSecretPasswordKey: password}, + } + _, err = m.clientset.CoreV1().Secrets(m.cfg.Namespace).Create(ctx, secret, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + // Never wrap the Secret object here: %w on a create error is fine, but + // the object itself must not reach a log line. + return fmt.Errorf("create Trino benchmark metadata Secret for cluster %s: %w", clusterID, err) + } + return nil +} + +func (m *trinoBenchmarkManager) applyConfigMaps(ctx context.Context, clusterID, orgID string, identity TrinoReaderIdentity) error { + discovery := m.endpoint(clusterID) + configMaps := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: clusterID + "-coordinator-config", + Namespace: m.cfg.Namespace, + Labels: m.ownershipLabels(clusterID, orgID), + }, + Data: map[string]string{ + "config.properties": renderTrinoServerConfig(trinoBenchmarkRoleCoordinator, discovery), + "node.properties": renderTrinoNodeProperties(clusterID), + "jvm.config": renderTrinoJVMConfig(m.cfg.CoordinatorMemory), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: clusterID + "-worker-config", + Namespace: m.cfg.Namespace, + Labels: m.ownershipLabels(clusterID, orgID), + }, + Data: map[string]string{ + "config.properties": renderTrinoServerConfig(trinoBenchmarkRoleWorker, discovery), + "node.properties": renderTrinoNodeProperties(clusterID), + "jvm.config": renderTrinoJVMConfig(m.cfg.WorkerMemory), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: clusterID + "-catalog", + Namespace: m.cfg.Namespace, + Labels: m.ownershipLabels(clusterID, orgID), + }, + Data: map[string]string{ + trinoBenchmarkCatalogName + ".properties": renderTrinoCatalogProperties(identity, clusterID), + }, + }, + } + for _, cm := range configMaps { + _, err := m.clientset.CoreV1().ConfigMaps(m.cfg.Namespace).Create(ctx, cm, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + return fmt.Errorf("create Trino benchmark ConfigMap %s: %w", cm.Name, err) + } + } + return nil +} + +func (m *trinoBenchmarkManager) applyDeployment(ctx context.Context, clusterID, orgID, role string, replicas int) error { + cpu, memory := m.cfg.WorkerCPU, m.cfg.WorkerMemory + configMapName := clusterID + "-worker-config" + if role == trinoBenchmarkRoleCoordinator { + cpu, memory = m.cfg.CoordinatorCPU, m.cfg.CoordinatorMemory + configMapName = clusterID + "-coordinator-config" + } + cpuQuantity, err := resource.ParseQuantity(cpu) + if err != nil { + return fmt.Errorf("%w: invalid Trino %s CPU %q", ErrTrinoBenchmarkConfig, role, cpu) + } + memoryQuantity, err := resource.ParseQuantity(memory) + if err != nil { + return fmt.Errorf("%w: invalid Trino %s memory %q", ErrTrinoBenchmarkConfig, role, memory) + } + // requests == limits: Guaranteed QoS, so a benchmark pod's numbers are not + // a function of whatever else happens to share the node. + resources := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: cpuQuantity, + corev1.ResourceMemory: memoryQuantity, + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: cpuQuantity.DeepCopy(), + corev1.ResourceMemory: memoryQuantity.DeepCopy(), + }, + } + + labels := m.ownershipLabels(clusterID, orgID) + podLabels := map[string]string{trinoBenchmarkRoleLabel: role} + for k, v := range labels { + podLabels[k] = v + } + selector := map[string]string{ + trinoBenchmarkClusterLabel: clusterID, + trinoBenchmarkRoleLabel: role, + } + replicaCount := int32(replicas) + + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterID + "-" + role, + Namespace: m.cfg.Namespace, + Labels: labels, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: &replicaCount, + Selector: &metav1.LabelSelector{MatchLabels: selector}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: podLabels}, + Spec: corev1.PodSpec{ + ServiceAccountName: m.cfg.ServiceAccount, + Containers: []corev1.Container{{ + Name: "trino", + Image: m.cfg.Image, + ImagePullPolicy: corev1.PullPolicy(m.cfg.ImagePullPolicy), + Ports: []corev1.ContainerPort{{ + Name: "http", + ContainerPort: trinoBenchmarkHTTPPort, + Protocol: corev1.ProtocolTCP, + }}, + Env: []corev1.EnvVar{{ + // By reference only — the value never appears in a + // pod spec. + Name: trinoBenchmarkPasswordEnv, + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: clusterID + "-metadata"}, + Key: trinoBenchmarkSecretPasswordKey, + }, + }, + }}, + Resources: resources, + VolumeMounts: []corev1.VolumeMount{ + {Name: "trino-config", MountPath: "/etc/trino/config.properties", SubPath: "config.properties"}, + {Name: "trino-config", MountPath: "/etc/trino/node.properties", SubPath: "node.properties"}, + {Name: "trino-config", MountPath: "/etc/trino/jvm.config", SubPath: "jvm.config"}, + {Name: "trino-catalog", MountPath: "/etc/trino/catalog"}, + }, + SecurityContext: &corev1.SecurityContext{ + AllowPrivilegeEscalation: boolPtr(false), + }, + }}, + Volumes: []corev1.Volume{ + { + Name: "trino-config", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: configMapName}, + }, + }, + }, + { + Name: "trino-catalog", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: clusterID + "-catalog"}, + }, + }, + }, + }, + }, + }, + }, + } + _, err = m.clientset.AppsV1().Deployments(m.cfg.Namespace).Create(ctx, deployment, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + return fmt.Errorf("create Trino benchmark deployment %s: %w", deployment.Name, err) + } + return nil +} + +// TrinoBenchmarkStatus reports ready ONLY when the coordinator is ready AND the +// full requested worker replica count is ready — a four-worker comparison run +// against three workers is not the benchmark that was asked for. +func (m *trinoBenchmarkManager) TrinoBenchmarkStatus(ctx context.Context, clusterID string) (TrinoBenchmarkCluster, error) { + service, err := m.clientset.CoreV1().Services(m.cfg.Namespace).Get(ctx, clusterID, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return TrinoBenchmarkCluster{}, fmt.Errorf("%w: %s", ErrTrinoBenchmarkNotFound, clusterID) + } + if err != nil { + return TrinoBenchmarkCluster{}, fmt.Errorf("read Trino benchmark service %s: %w", clusterID, err) + } + if service.Labels[trinoBenchmarkAppLabelKey] != trinoBenchmarkAppLabelValue { + // Same name, not ours. Never report on something we do not own. + return TrinoBenchmarkCluster{}, fmt.Errorf("%w: %s", ErrTrinoBenchmarkNotFound, clusterID) + } + + requested, _ := strconv.Atoi(service.Annotations[trinoBenchmarkWorkersAnnotation]) + cluster := TrinoBenchmarkCluster{ + ID: clusterID, + State: TrinoBenchmarkStatePending, + RequestedWorkers: requested, + Image: service.Annotations[trinoBenchmarkImageAnnotation], + } + + coordinator, coordinatorErr := m.clientset.AppsV1().Deployments(m.cfg.Namespace).Get(ctx, clusterID+"-coordinator", metav1.GetOptions{}) + worker, workerErr := m.clientset.AppsV1().Deployments(m.cfg.Namespace).Get(ctx, clusterID+"-worker", metav1.GetOptions{}) + for _, err := range []error{coordinatorErr, workerErr} { + if err != nil && !apierrors.IsNotFound(err) { + return TrinoBenchmarkCluster{}, fmt.Errorf("read Trino benchmark deployments for %s: %w", clusterID, err) + } + } + if workerErr == nil { + cluster.ReadyWorkers = int(worker.Status.ReadyReplicas) + } + + // Terminal failure short-circuits polling: a deployment that blew its + // progress deadline or cannot create replicas will not recover on its own. + if (coordinatorErr == nil && trinoDeploymentFailed(coordinator)) || (workerErr == nil && trinoDeploymentFailed(worker)) { + cluster.State = TrinoBenchmarkStateFailed + return cluster, nil + } + if coordinatorErr != nil || workerErr != nil { + // A partial provision: still converging (or awaiting cleanup). + return cluster, nil + } + if coordinator.Status.ReadyReplicas >= 1 && requested > 0 && cluster.ReadyWorkers >= requested { + cluster.State = TrinoBenchmarkStateReady + cluster.Endpoint = m.endpoint(clusterID) + } + return cluster, nil +} + +func trinoDeploymentFailed(deployment *appsv1.Deployment) bool { + for _, condition := range deployment.Status.Conditions { + if condition.Type == appsv1.DeploymentProgressing && + condition.Status == corev1.ConditionFalse && + condition.Reason == "ProgressDeadlineExceeded" { + return true + } + if condition.Type == appsv1.DeploymentReplicaFailure && condition.Status == corev1.ConditionTrue { + return true + } + } + return false +} + +// DeprovisionTrinoBenchmark deletes exactly the objects labelled as owned by +// clusterID. It is idempotent, safe after a partial provision, and never +// touches the charts-created reader Secret (different namespace, and no +// ownership labels). +func (m *trinoBenchmarkManager) DeprovisionTrinoBenchmark(ctx context.Context, clusterID string) error { + selector := metav1.ListOptions{LabelSelector: m.ownedSelector(clusterID)} + var errs []string + + deployments, err := m.clientset.AppsV1().Deployments(m.cfg.Namespace).List(ctx, selector) + if err != nil { + errs = append(errs, fmt.Sprintf("list deployments: %v", err)) + } else { + for _, item := range deployments.Items { + if err := m.clientset.AppsV1().Deployments(m.cfg.Namespace).Delete(ctx, item.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Sprintf("delete deployment %s: %v", item.Name, err)) + } + } + } + + services, err := m.clientset.CoreV1().Services(m.cfg.Namespace).List(ctx, selector) + if err != nil { + errs = append(errs, fmt.Sprintf("list services: %v", err)) + } else { + for _, item := range services.Items { + if err := m.clientset.CoreV1().Services(m.cfg.Namespace).Delete(ctx, item.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Sprintf("delete service %s: %v", item.Name, err)) + } + } + } + + configMaps, err := m.clientset.CoreV1().ConfigMaps(m.cfg.Namespace).List(ctx, selector) + if err != nil { + errs = append(errs, fmt.Sprintf("list configmaps: %v", err)) + } else { + for _, item := range configMaps.Items { + if err := m.clientset.CoreV1().ConfigMaps(m.cfg.Namespace).Delete(ctx, item.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Sprintf("delete configmap %s: %v", item.Name, err)) + } + } + } + + // The short-lived credential Secret is deleted LAST so an earlier failure + // still leaves it reachable for a retry, and its deletion is the step the + // control plane most needs to be sure about. + secrets, err := m.clientset.CoreV1().Secrets(m.cfg.Namespace).List(ctx, selector) + if err != nil { + errs = append(errs, fmt.Sprintf("list secrets: %v", err)) + } else { + for _, item := range secrets.Items { + if err := m.clientset.CoreV1().Secrets(m.cfg.Namespace).Delete(ctx, item.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Sprintf("delete secret %s: %v", item.Name, err)) + } + } + } + + if len(errs) > 0 { + return fmt.Errorf("deprovision Trino benchmark cluster %s: %s", clusterID, strings.Join(errs, "; ")) + } + slog.Info("Deprovisioned Trino benchmark cluster.", "cluster_id", clusterID) + return nil +} + +// renderTrinoServerConfig produces config.properties for one role. Workers and +// the coordinator share one discovery URI (the coordinator Service), which is +// what makes this a real multi-node cluster rather than four isolated nodes; +// the coordinator is excluded from scheduling so worker parallelism is what is +// actually measured. +func renderTrinoServerConfig(role, discoveryURI string) string { + lines := []string{ + "http-server.http.port=" + strconv.Itoa(trinoBenchmarkHTTPPort), + "discovery.uri=" + discoveryURI, + } + if role == trinoBenchmarkRoleCoordinator { + lines = append([]string{ + "coordinator=true", + "node-scheduler.include-coordinator=false", + }, lines...) + } else { + lines = append([]string{"coordinator=false"}, lines...) + } + return strings.Join(lines, "\n") + "\n" +} + +// renderTrinoNodeProperties pins the environment name to the cluster so nodes +// from two benchmark clusters can never join each other's discovery. +func renderTrinoNodeProperties(clusterID string) string { + return "node.environment=" + trinoNodeEnvironment(clusterID) + "\n" +} + +// trinoNodeEnvironment sanitizes the cluster ID into Trino's node.environment +// alphabet (lowercase alphanumeric and underscore). +func trinoNodeEnvironment(clusterID string) string { + var b strings.Builder + for _, r := range strings.ToLower(clusterID) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + b.WriteRune(r) + default: + b.WriteRune('_') + } + } + return b.String() +} + +// renderTrinoJVMConfig sizes the heap at ~70% of the container memory limit and +// pins the JVM to UTC, so Trino and Duckgres interpret the same TIMESTAMPTZ +// predicates identically. +func renderTrinoJVMConfig(memory string) string { + heapMB := 4096 + if quantity, err := resource.ParseQuantity(memory); err == nil { + if mb := quantity.Value() / (1 << 20) * 70 / 100; mb > 512 { + heapMB = int(mb) + } + } + return strings.Join([]string{ + "-server", + fmt.Sprintf("-Xmx%dM", heapMB), + "-XX:+UseG1GC", + "-XX:G1HeapRegionSize=32M", + "-XX:+ExplicitGCInvokesConcurrent", + "-XX:+ExitOnOutOfMemoryError", + "-XX:-OmitStackTraceInFastThrow", + "-XX:ReservedCodeCacheSize=512M", + "-Djdk.attach.allowAttachSelf=true", + "-Dfile.encoding=UTF-8", + // The benchmark compares UTC results across engines; a JVM default + // timezone would silently shift date_trunc and partition predicates. + "-Duser.timezone=UTC", + }, "\n") + "\n" +} + +// renderTrinoCatalogProperties configures the Brikk DuckLake connector against +// the warehouse's own metadata Postgres and S3 data path, using ONLY the +// charts-created read-only identity: +// +// - the metadata password is interpolated from the env var backed by the +// short-lived Secret, never written here; and +// - S3 access is an assumed IAM role (renewable credentials), never a static +// access key, and never the tenant's writer role. +func renderTrinoCatalogProperties(identity TrinoReaderIdentity, clusterID string) string { + return strings.Join([]string{ + "connector.name=ducklake", + "ducklake.catalog.database-url=" + identity.JDBCURL(), + "ducklake.catalog.database-user=" + identity.MetadataUser, + "ducklake.catalog.database-password=${ENV:" + trinoBenchmarkPasswordEnv + "}", + "ducklake.data-path=" + identity.DataPath, + "fs.native-s3.enabled=true", + "s3.region=" + identity.Region, + "s3.iam-role=" + identity.ReadOnlyRoleARN, + "s3.role-session-name=" + clusterID, + }, "\n") + "\n" +} diff --git a/controlplane/trino_benchmark_manager_test.go b/controlplane/trino_benchmark_manager_test.go new file mode 100644 index 00000000..7e98aae8 --- /dev/null +++ b/controlplane/trino_benchmark_manager_test.go @@ -0,0 +1,716 @@ +//go:build kubernetes + +package controlplane + +import ( + "bytes" + "context" + "errors" + "log/slog" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" +) + +const ( + trinoTestNamespace = "duckgres" + trinoTestImage = "123456789012.dkr.ecr.us-east-1.amazonaws.com/trino-brikk@sha256:0123456789abcdef" + trinoTestReaderSecret = "duckling-bench-org-trino-reader" + trinoTestReaderPass = "reader-password-never-logged" + trinoTestWriterRoleARN = "arn:aws:iam::123456789012:role/duckling-bench-org" +) + +// fakeTrinoReaderResolver stands in for the charts-backed resolver. +type fakeTrinoReaderResolver struct { + identity TrinoReaderIdentity + err error + calls int +} + +func (r *fakeTrinoReaderResolver) ResolveTrinoReader(context.Context, string) (TrinoReaderIdentity, error) { + r.calls++ + return r.identity, r.err +} + +func testTrinoReaderIdentity(t *testing.T) TrinoReaderIdentity { + t.Helper() + identity, err := buildTrinoReaderIdentity(TrinoReaderSource{ + MetadataEndpoint: "duckling-bench-org-pgbouncer.ducklings.svc.cluster.local:6432", + MetadataDatabase: "ducklake_bench_org", + MetadataUser: "trino_reader_bench_org", + MetadataPasswordSecret: TrinoReaderSecretRef{ + Name: trinoTestReaderSecret, Namespace: "ducklings", Key: "password", + }, + Bucket: "posthog-duckling-benchorg-dev", + Region: "us-east-1", + ReadOnlyRoleARN: "arn:aws:iam::123456789012:role/duckling-bench-org-trino-reader", + SSLMode: "disable", + WriterRoleARN: trinoTestWriterRoleARN, + WriterUser: "ducklake_bench_org", + }) + if err != nil { + t.Fatalf("build test reader identity: %v", err) + } + return identity +} + +func newTrinoBenchmarkTestManager(t *testing.T, objects ...runtime.Object) (*trinoBenchmarkManager, kubernetes.Interface, *fakeTrinoReaderResolver) { + t.Helper() + // The charts-created reader Secret lives in the ducklings namespace; the + // control plane may read it by exact name and nothing else. + seeded := []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: trinoTestReaderSecret, Namespace: "ducklings"}, + Data: map[string][]byte{"password": []byte(trinoTestReaderPass)}, + }, + } + seeded = append(seeded, objects...) + clientset := fake.NewSimpleClientset(seeded...) + resolver := &fakeTrinoReaderResolver{identity: testTrinoReaderIdentity(t)} + manager, err := newTrinoBenchmarkManager(clientset, resolver, TrinoBenchmarkManagerConfig{ + Namespace: trinoTestNamespace, + Image: trinoTestImage, + ControlPlaneID: "duckgres-control-plane-0", + }) + if err != nil { + t.Fatalf("newTrinoBenchmarkManager: %v", err) + } + return manager, clientset, resolver +} + +func TestTrinoBenchmarkManagerRequiresPinnedImage(t *testing.T) { + _, err := newTrinoBenchmarkManager(fake.NewSimpleClientset(), &fakeTrinoReaderResolver{}, TrinoBenchmarkManagerConfig{ + Namespace: trinoTestNamespace, + }) + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig for a missing pinned image", err) + } +} + +func TestTrinoBenchmarkManagerRequiresResolver(t *testing.T) { + _, err := newTrinoBenchmarkManager(fake.NewSimpleClientset(), nil, TrinoBenchmarkManagerConfig{ + Namespace: trinoTestNamespace, Image: trinoTestImage, + }) + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig without a reader resolver", err) + } +} + +func TestTrinoBenchmarkManagerProvisionRendersCoordinatorAndRequestedWorkers(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + + result, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 4, RunID: "run-1"}) + if err != nil { + t.Fatalf("provision: %v", err) + } + if !result.Created { + t.Fatal("first provision should report Created") + } + clusterID := result.Cluster.ID + if clusterID != "trino-bench-bench-org" { + t.Fatalf("cluster id = %q", clusterID) + } + if result.Cluster.State != TrinoBenchmarkStatePending { + t.Fatalf("state = %q, want pending immediately after provision", result.Cluster.State) + } + if result.Cluster.RequestedWorkers != 4 { + t.Fatalf("requested workers = %d", result.Cluster.RequestedWorkers) + } + + coordinator, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(context.Background(), clusterID+"-coordinator", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get coordinator: %v", err) + } + if coordinator.Spec.Replicas == nil || *coordinator.Spec.Replicas != 1 { + t.Fatalf("coordinator replicas = %v, want exactly 1", coordinator.Spec.Replicas) + } + worker, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(context.Background(), clusterID+"-worker", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get workers: %v", err) + } + if worker.Spec.Replicas == nil || *worker.Spec.Replicas != 4 { + t.Fatalf("worker replicas = %v, want exactly the requested 4", worker.Spec.Replicas) + } + + // The Service selects ONLY the coordinator: a client statement must never + // land on a worker. + service, err := clientset.CoreV1().Services(trinoTestNamespace).Get(context.Background(), clusterID, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get service: %v", err) + } + if service.Spec.Type != corev1.ServiceTypeClusterIP { + t.Fatalf("service type = %q, want ClusterIP", service.Spec.Type) + } + if service.Spec.Selector[trinoBenchmarkRoleLabel] != trinoBenchmarkRoleCoordinator { + t.Fatalf("service selector = %v, want the coordinator role", service.Spec.Selector) + } + if service.Spec.Selector[trinoBenchmarkClusterLabel] != clusterID { + t.Fatalf("service selector = %v, want the cluster label", service.Spec.Selector) + } + + // Every object carries the ownership labels cleanup keys off. + for _, labels := range []map[string]string{ + coordinator.Labels, worker.Labels, service.Labels, + } { + if labels[trinoBenchmarkClusterLabel] != clusterID { + t.Fatalf("labels = %v missing the cluster label", labels) + } + if labels[trinoBenchmarkOrgLabel] != "bench-org" { + t.Fatalf("labels = %v missing the org label", labels) + } + if labels[trinoBenchmarkAppLabelKey] != trinoBenchmarkAppLabelValue { + t.Fatalf("labels = %v missing the app label", labels) + } + } +} + +func TestTrinoBenchmarkManagerDefaultsToFourWorkers(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + + result, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{}) + if err != nil { + t.Fatalf("provision: %v", err) + } + if result.Cluster.RequestedWorkers != defaultTrinoBenchmarkWorkers { + t.Fatalf("requested workers = %d, want the %d default", result.Cluster.RequestedWorkers, defaultTrinoBenchmarkWorkers) + } + worker, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(context.Background(), result.Cluster.ID+"-worker", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get workers: %v", err) + } + if *worker.Spec.Replicas != int32(defaultTrinoBenchmarkWorkers) { + t.Fatalf("worker replicas = %d, want %d", *worker.Spec.Replicas, defaultTrinoBenchmarkWorkers) + } +} + +func TestTrinoBenchmarkManagerPinsImageAndSetsExplicitResources(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + + result, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 2}) + if err != nil { + t.Fatalf("provision: %v", err) + } + if result.Cluster.Image != trinoTestImage { + t.Fatalf("reported image = %q, want the pinned image", result.Cluster.Image) + } + + for _, name := range []string{result.Cluster.ID + "-coordinator", result.Cluster.ID + "-worker"} { + deployment, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(context.Background(), name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get %s: %v", name, err) + } + container := deployment.Spec.Template.Spec.Containers[0] + if container.Image != trinoTestImage { + t.Fatalf("%s image = %q, want the pinned image", name, container.Image) + } + if container.Resources.Requests.Cpu().IsZero() || container.Resources.Requests.Memory().IsZero() { + t.Fatalf("%s has no explicit CPU/memory requests: %v", name, container.Resources.Requests) + } + if container.Resources.Limits.Cpu().IsZero() || container.Resources.Limits.Memory().IsZero() { + t.Fatalf("%s has no explicit CPU/memory limits: %v", name, container.Resources.Limits) + } + } +} + +func TestTrinoBenchmarkManagerConfiguresMultiNodeDiscoveryAndUTC(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + + result, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if err != nil { + t.Fatalf("provision: %v", err) + } + clusterID := result.Cluster.ID + discovery := "http://" + clusterID + "." + trinoTestNamespace + ".svc.cluster.local:8080" + + coordinatorCM, err := clientset.CoreV1().ConfigMaps(trinoTestNamespace).Get(context.Background(), clusterID+"-coordinator-config", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get coordinator config: %v", err) + } + coordinatorConfig := coordinatorCM.Data["config.properties"] + for _, want := range []string{ + "coordinator=true", + "node-scheduler.include-coordinator=false", + "discovery.uri=" + discovery, + } { + if !strings.Contains(coordinatorConfig, want) { + t.Fatalf("coordinator config.properties missing %q:\n%s", want, coordinatorConfig) + } + } + if !strings.Contains(coordinatorCM.Data["jvm.config"], "-Duser.timezone=UTC") { + t.Fatalf("coordinator jvm.config must pin UTC:\n%s", coordinatorCM.Data["jvm.config"]) + } + + workerCM, err := clientset.CoreV1().ConfigMaps(trinoTestNamespace).Get(context.Background(), clusterID+"-worker-config", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get worker config: %v", err) + } + workerConfig := workerCM.Data["config.properties"] + if !strings.Contains(workerConfig, "coordinator=false") { + t.Fatalf("worker config.properties must not declare a coordinator:\n%s", workerConfig) + } + if !strings.Contains(workerConfig, "discovery.uri="+discovery) { + t.Fatalf("worker config.properties must point at the coordinator Service:\n%s", workerConfig) + } + if !strings.Contains(workerCM.Data["jvm.config"], "-Duser.timezone=UTC") { + t.Fatalf("worker jvm.config must pin UTC:\n%s", workerCM.Data["jvm.config"]) + } +} + +func TestTrinoBenchmarkManagerWiresReadOnlyRoleAndReaderSecret(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + + result, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 2}) + if err != nil { + t.Fatalf("provision: %v", err) + } + clusterID := result.Cluster.ID + + catalogCM, err := clientset.CoreV1().ConfigMaps(trinoTestNamespace).Get(context.Background(), clusterID+"-catalog", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get catalog config: %v", err) + } + catalog := catalogCM.Data["ducklake.properties"] + for _, want := range []string{ + "connector.name=ducklake", + "ducklake.catalog.database-url=jdbc:postgresql://duckling-bench-org-pgbouncer.ducklings.svc.cluster.local:6432/ducklake_bench_org?sslmode=disable", + "ducklake.catalog.database-user=trino_reader_bench_org", + "ducklake.catalog.database-password=${ENV:TRINO_DUCKLAKE_DB_PASSWORD}", + "ducklake.data-path=s3://posthog-duckling-benchorg-dev/", + "fs.native-s3.enabled=true", + "s3.region=us-east-1", + "s3.iam-role=arn:aws:iam::123456789012:role/duckling-bench-org-trino-reader", + } { + if !strings.Contains(catalog, want) { + t.Fatalf("catalog properties missing %q:\n%s", want, catalog) + } + } + // Static long-lived keys would defeat the renewable read-only identity. + for _, banned := range []string{"s3.aws-access-key", "s3.aws-secret-key", "s3.session-token"} { + if strings.Contains(catalog, banned) { + t.Fatalf("catalog properties must not carry static S3 credentials (%q):\n%s", banned, catalog) + } + } + // The password lives in the short-lived Secret, never in the ConfigMap. + if strings.Contains(catalog, trinoTestReaderPass) { + t.Fatal("catalog ConfigMap contains the reader password") + } + + secret, err := clientset.CoreV1().Secrets(trinoTestNamespace).Get(context.Background(), clusterID+"-metadata", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get benchmark secret: %v", err) + } + if string(secret.Data[trinoBenchmarkSecretPasswordKey]) != trinoTestReaderPass { + t.Fatal("benchmark Secret does not carry the charts-created reader password") + } + if len(secret.Data) != 1 { + t.Fatalf("benchmark Secret carries %d keys, want only the reader password", len(secret.Data)) + } + if secret.Labels[trinoBenchmarkClusterLabel] != clusterID { + t.Fatal("benchmark Secret is not owned by the cluster") + } + + // Both roles read the password through a secretKeyRef, never a literal. + for _, name := range []string{clusterID + "-coordinator", clusterID + "-worker"} { + deployment, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(context.Background(), name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get %s: %v", name, err) + } + var found bool + for _, env := range deployment.Spec.Template.Spec.Containers[0].Env { + if env.Name != "TRINO_DUCKLAKE_DB_PASSWORD" { + continue + } + found = true + if env.Value != "" { + t.Fatalf("%s passes the reader password by value", name) + } + if env.ValueFrom == nil || env.ValueFrom.SecretKeyRef == nil { + t.Fatalf("%s does not use a secretKeyRef for the reader password", name) + } + if env.ValueFrom.SecretKeyRef.Name != clusterID+"-metadata" { + t.Fatalf("%s reads the wrong Secret %q", name, env.ValueFrom.SecretKeyRef.Name) + } + } + if !found { + t.Fatalf("%s has no reader password env var", name) + } + } +} + +func TestTrinoBenchmarkManagerRendersNoWriterCredentials(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + + result, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 2}) + if err != nil { + t.Fatalf("provision: %v", err) + } + clusterID := result.Cluster.ID + + configMaps, err := clientset.CoreV1().ConfigMaps(trinoTestNamespace).List(context.Background(), metav1.ListOptions{ + LabelSelector: trinoBenchmarkClusterLabel + "=" + clusterID, + }) + if err != nil { + t.Fatalf("list config maps: %v", err) + } + if len(configMaps.Items) == 0 { + t.Fatal("expected the cluster's ConfigMaps") + } + // Compare property VALUES exactly: the reader ARN legitimately has the + // writer ARN as a prefix, so a substring check would be meaningless here. + for _, cm := range configMaps.Items { + for key, value := range cm.Data { + for _, line := range strings.Split(value, "\n") { + name, propertyValue, ok := strings.Cut(strings.TrimSpace(line), "=") + if !ok { + continue + } + if propertyValue == trinoTestWriterRoleARN { + t.Fatalf("%s/%s property %s uses the warehouse WRITER role", cm.Name, key, name) + } + // The DuckLake writer login is the org's own catalog role. + if name == "ducklake.catalog.database-user" && propertyValue == "ducklake_bench_org" { + t.Fatalf("%s/%s uses the warehouse writer database user", cm.Name, key) + } + } + } + } +} + +func TestTrinoBenchmarkManagerFailsClosedWithoutReaderIdentity(t *testing.T) { + manager, clientset, resolver := newTrinoBenchmarkTestManager(t) + resolver.err = ErrTrinoBenchmarkConfig + + _, err := manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } + deployments, err := clientset.AppsV1().Deployments(trinoTestNamespace).List(context.Background(), metav1.ListOptions{}) + if err != nil { + t.Fatalf("list deployments: %v", err) + } + if len(deployments.Items) != 0 { + t.Fatalf("a fail-closed provision created %d deployments", len(deployments.Items)) + } +} + +func TestTrinoBenchmarkManagerFailsClosedWhenReaderSecretIsAbsent(t *testing.T) { + clientset := fake.NewSimpleClientset() + manager, err := newTrinoBenchmarkManager(clientset, &fakeTrinoReaderResolver{identity: testTrinoReaderIdentity(t)}, TrinoBenchmarkManagerConfig{ + Namespace: trinoTestNamespace, Image: trinoTestImage, + }) + if err != nil { + t.Fatalf("newTrinoBenchmarkManager: %v", err) + } + + _, err = manager.ProvisionTrinoBenchmark(context.Background(), "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig when the charts Secret is missing", err) + } +} + +func TestTrinoBenchmarkManagerProvisionIsIdempotent(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + ctx := context.Background() + + first, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if err != nil { + t.Fatalf("first provision: %v", err) + } + second, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if err != nil { + t.Fatalf("second provision: %v", err) + } + if second.Created { + t.Fatal("second provision must not report Created") + } + if second.Cluster.ID != first.Cluster.ID { + t.Fatalf("cluster id changed: %q -> %q", first.Cluster.ID, second.Cluster.ID) + } + + deployments, err := clientset.AppsV1().Deployments(trinoTestNamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("list deployments: %v", err) + } + if len(deployments.Items) != 2 { + t.Fatalf("deployments = %d, want exactly the coordinator and worker", len(deployments.Items)) + } +} + +func TestTrinoBenchmarkManagerRejectsConflictingOwnershipOrConfiguration(t *testing.T) { + ctx := context.Background() + + t.Run("different worker count", func(t *testing.T) { + manager, _, _ := newTrinoBenchmarkTestManager(t) + if _, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}); err != nil { + t.Fatalf("provision: %v", err) + } + _, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 2}) + if !errors.Is(err, ErrTrinoBenchmarkConflict) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConflict", err) + } + }) + + t.Run("different owning org", func(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + if _, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}); err != nil { + t.Fatalf("provision: %v", err) + } + service, err := clientset.CoreV1().Services(trinoTestNamespace).Get(ctx, "trino-bench-bench-org", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get service: %v", err) + } + service.Labels[trinoBenchmarkOrgLabel] = "someone-else" + if _, err := clientset.CoreV1().Services(trinoTestNamespace).Update(ctx, service, metav1.UpdateOptions{}); err != nil { + t.Fatalf("update service: %v", err) + } + _, err = manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if !errors.Is(err, ErrTrinoBenchmarkConflict) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConflict", err) + } + }) + + t.Run("different pinned image", func(t *testing.T) { + manager, clientset, resolver := newTrinoBenchmarkTestManager(t) + if _, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}); err != nil { + t.Fatalf("provision: %v", err) + } + repinned, err := newTrinoBenchmarkManager(clientset, resolver, TrinoBenchmarkManagerConfig{ + Namespace: trinoTestNamespace, Image: "registry.example/trino-brikk@sha256:beef", + }) + if err != nil { + t.Fatalf("newTrinoBenchmarkManager: %v", err) + } + _, err = repinned.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if !errors.Is(err, ErrTrinoBenchmarkConflict) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConflict on an image change", err) + } + }) +} + +func TestTrinoBenchmarkManagerStatusRequiresCoordinatorAndEveryWorker(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + ctx := context.Background() + + result, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if err != nil { + t.Fatalf("provision: %v", err) + } + clusterID := result.Cluster.ID + + pending, err := manager.TrinoBenchmarkStatus(ctx, clusterID) + if err != nil { + t.Fatalf("status: %v", err) + } + if pending.State != TrinoBenchmarkStatePending || pending.Endpoint != "" { + t.Fatalf("status = %+v, want pending with no endpoint", pending) + } + + setTrinoDeploymentReady(t, clientset, clusterID+"-coordinator", 1) + setTrinoDeploymentReady(t, clientset, clusterID+"-worker", 3) + partial, err := manager.TrinoBenchmarkStatus(ctx, clusterID) + if err != nil { + t.Fatalf("status: %v", err) + } + if partial.State != TrinoBenchmarkStatePending { + t.Fatalf("state = %q with 3/4 workers ready, want pending", partial.State) + } + if partial.ReadyWorkers != 3 || partial.RequestedWorkers != 4 { + t.Fatalf("worker counts = %d/%d", partial.ReadyWorkers, partial.RequestedWorkers) + } + + setTrinoDeploymentReady(t, clientset, clusterID+"-worker", 4) + ready, err := manager.TrinoBenchmarkStatus(ctx, clusterID) + if err != nil { + t.Fatalf("status: %v", err) + } + if ready.State != TrinoBenchmarkStateReady { + t.Fatalf("state = %q with all workers ready, want ready", ready.State) + } + if ready.Endpoint != "http://"+clusterID+"."+trinoTestNamespace+".svc.cluster.local:8080" { + t.Fatalf("endpoint = %q", ready.Endpoint) + } + if ready.Image != trinoTestImage { + t.Fatalf("image = %q, want the pinned image recorded for artifacts", ready.Image) + } +} + +func TestTrinoBenchmarkManagerStatusReportsTerminalFailure(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + ctx := context.Background() + + result, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 2}) + if err != nil { + t.Fatalf("provision: %v", err) + } + clusterID := result.Cluster.ID + + deployment, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(ctx, clusterID+"-worker", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get workers: %v", err) + } + deployment.Status.Conditions = []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentProgressing, + Status: corev1.ConditionFalse, + Reason: "ProgressDeadlineExceeded", + }} + if _, err := clientset.AppsV1().Deployments(trinoTestNamespace).UpdateStatus(ctx, deployment, metav1.UpdateOptions{}); err != nil { + t.Fatalf("update status: %v", err) + } + + status, err := manager.TrinoBenchmarkStatus(ctx, clusterID) + if err != nil { + t.Fatalf("status: %v", err) + } + if status.State != TrinoBenchmarkStateFailed { + t.Fatalf("state = %q, want failed", status.State) + } +} + +func TestTrinoBenchmarkManagerStatusReportsNotFound(t *testing.T) { + manager, _, _ := newTrinoBenchmarkTestManager(t) + + _, err := manager.TrinoBenchmarkStatus(context.Background(), "trino-bench-missing") + if !errors.Is(err, ErrTrinoBenchmarkNotFound) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkNotFound", err) + } +} + +func TestTrinoBenchmarkManagerCleanupIsIdempotentAndDeletesEverythingItOwns(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + ctx := context.Background() + + result, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 4}) + if err != nil { + t.Fatalf("provision: %v", err) + } + clusterID := result.Cluster.ID + + for i := 0; i < 2; i++ { + if err := manager.DeprovisionTrinoBenchmark(ctx, clusterID); err != nil { + t.Fatalf("deprovision %d: %v", i, err) + } + } + + deployments, _ := clientset.AppsV1().Deployments(trinoTestNamespace).List(ctx, metav1.ListOptions{}) + services, _ := clientset.CoreV1().Services(trinoTestNamespace).List(ctx, metav1.ListOptions{}) + configMaps, _ := clientset.CoreV1().ConfigMaps(trinoTestNamespace).List(ctx, metav1.ListOptions{}) + secrets, _ := clientset.CoreV1().Secrets(trinoTestNamespace).List(ctx, metav1.ListOptions{}) + if len(deployments.Items) != 0 || len(services.Items) != 0 || len(configMaps.Items) != 0 || len(secrets.Items) != 0 { + t.Fatalf("cleanup left resources: deployments=%d services=%d configmaps=%d secrets=%d", + len(deployments.Items), len(services.Items), len(configMaps.Items), len(secrets.Items)) + } +} + +func TestTrinoBenchmarkManagerCleanupIsSafeAfterPartialProvision(t *testing.T) { + manager, clientset, _ := newTrinoBenchmarkTestManager(t) + ctx := context.Background() + + // Simulate a provision that died after the Service and catalog ConfigMap. + if _, err := clientset.CoreV1().Services(trinoTestNamespace).Create(ctx, &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "trino-bench-bench-org", + Namespace: trinoTestNamespace, + Labels: map[string]string{ + trinoBenchmarkAppLabelKey: trinoBenchmarkAppLabelValue, + trinoBenchmarkClusterLabel: "trino-bench-bench-org", + trinoBenchmarkOrgLabel: "bench-org", + }, + }, + }, metav1.CreateOptions{}); err != nil { + t.Fatalf("seed partial service: %v", err) + } + + if err := manager.DeprovisionTrinoBenchmark(ctx, "trino-bench-bench-org"); err != nil { + t.Fatalf("deprovision after partial provision: %v", err) + } + services, _ := clientset.CoreV1().Services(trinoTestNamespace).List(ctx, metav1.ListOptions{}) + if len(services.Items) != 0 { + t.Fatalf("partial-provision cleanup left %d services", len(services.Items)) + } +} + +func TestTrinoBenchmarkManagerCleanupNeverTouchesUnownedResources(t *testing.T) { + foreignService := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "duckgres-control-plane", + Namespace: trinoTestNamespace, + Labels: map[string]string{"app": "duckgres-control-plane"}, + }, + } + otherCluster := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "trino-bench-other-org-catalog", + Namespace: trinoTestNamespace, + Labels: map[string]string{ + trinoBenchmarkAppLabelKey: trinoBenchmarkAppLabelValue, + trinoBenchmarkClusterLabel: "trino-bench-other-org", + trinoBenchmarkOrgLabel: "other-org", + }, + }, + } + manager, clientset, _ := newTrinoBenchmarkTestManager(t, foreignService, otherCluster) + ctx := context.Background() + + if _, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 2}); err != nil { + t.Fatalf("provision: %v", err) + } + if err := manager.DeprovisionTrinoBenchmark(ctx, "trino-bench-bench-org"); err != nil { + t.Fatalf("deprovision: %v", err) + } + + if _, err := clientset.CoreV1().Services(trinoTestNamespace).Get(ctx, "duckgres-control-plane", metav1.GetOptions{}); err != nil { + t.Fatalf("cleanup deleted an unrelated Service: %v", err) + } + if _, err := clientset.CoreV1().ConfigMaps(trinoTestNamespace).Get(ctx, "trino-bench-other-org-catalog", metav1.GetOptions{}); err != nil { + t.Fatalf("cleanup deleted another benchmark cluster's ConfigMap: %v", err) + } + // And the charts-created reader Secret in the ducklings namespace survives. + if _, err := clientset.CoreV1().Secrets("ducklings").Get(ctx, trinoTestReaderSecret, metav1.GetOptions{}); err != nil { + t.Fatalf("cleanup deleted the charts-created reader Secret: %v", err) + } +} + +func TestTrinoBenchmarkManagerLogsNoSecretValues(t *testing.T) { + var logs bytes.Buffer + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelDebug}))) + defer slog.SetDefault(previous) + + manager, _, _ := newTrinoBenchmarkTestManager(t) + ctx := context.Background() + result, err := manager.ProvisionTrinoBenchmark(ctx, "bench-org", TrinoBenchmarkRequest{Workers: 2}) + if err != nil { + t.Fatalf("provision: %v", err) + } + if _, err := manager.TrinoBenchmarkStatus(ctx, result.Cluster.ID); err != nil { + t.Fatalf("status: %v", err) + } + if err := manager.DeprovisionTrinoBenchmark(ctx, result.Cluster.ID); err != nil { + t.Fatalf("deprovision: %v", err) + } + + if strings.Contains(logs.String(), trinoTestReaderPass) { + t.Fatalf("the reader password reached the logs:\n%s", logs.String()) + } +} + +func setTrinoDeploymentReady(t *testing.T, clientset kubernetes.Interface, name string, ready int32) { + t.Helper() + ctx := context.Background() + deployment, err := clientset.AppsV1().Deployments(trinoTestNamespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get %s: %v", name, err) + } + deployment.Status.ReadyReplicas = ready + deployment.Status.AvailableReplicas = ready + deployment.Status.Replicas = ready + if _, err := clientset.AppsV1().Deployments(trinoTestNamespace).UpdateStatus(ctx, deployment, metav1.UpdateOptions{}); err != nil { + t.Fatalf("update %s status: %v", name, err) + } +} diff --git a/controlplane/trino_benchmark_reader.go b/controlplane/trino_benchmark_reader.go new file mode 100644 index 00000000..df5ec6b3 --- /dev/null +++ b/controlplane/trino_benchmark_reader.go @@ -0,0 +1,211 @@ +package controlplane + +import ( + "context" + "fmt" + "net" + "regexp" + "strconv" + "strings" +) + +// Reader identity resolution. +// +// A benchmark Trino cluster reads the SAME DuckLake snapshot the Duckgres +// worker is being compared against, but it must do so with a strictly separate, +// read-only identity created by the companion charts release: +// +// - a metadata-Postgres role with only the SELECT privileges the DuckLake +// catalog needs, whose password lives in a Kubernetes Secret the control +// plane may read by exact name; and +// - an IAM role with only s3:ListBucket + s3:GetObject on the warehouse's own +// data bucket, assumed by the Trino pods for renewable credentials. +// +// The tenant's DuckLake WRITER role and login are never an acceptable +// substitute. If any reader field is absent, resolution fails with +// ErrTrinoBenchmarkConfig and the whole benchmark is refused — there is no +// fallback path by construction. + +// TrinoReaderSecretRef identifies one key in a namespaced Kubernetes Secret. +// This is a reference, never a value: the control plane reads the value only at +// the moment it materializes the short-lived benchmark Secret. +type TrinoReaderSecretRef struct { + Name string + Namespace string + Key string +} + +func (r TrinoReaderSecretRef) String() string { + return r.Namespace + "/" + r.Name + "#" + r.Key +} + +func (r TrinoReaderSecretRef) complete() bool { + return r.Name != "" && r.Namespace != "" && r.Key != "" +} + +// TrinoReaderSource is the raw state a resolver collects before validation. It +// includes the WRITER identity purely so buildTrinoReaderIdentity can REFUSE a +// configuration that would hand Trino writer credentials. +type TrinoReaderSource struct { + MetadataEndpoint string // host or host:port + MetadataDatabase string + MetadataUser string + MetadataPasswordSecret TrinoReaderSecretRef + Bucket string + Region string + DataPath string // optional; derived from Bucket when empty + ReadOnlyRoleARN string + // SSLMode is the JDBC sslmode for the metadata connection. Empty defaults + // to "require"; an in-cluster PgBouncer hop is "disable" (the pooler + // carries TLS onward), mirroring MetadataPostgresURL. + SSLMode string + + // WriterRoleARN / WriterUser are the tenant's own write identities. They + // are never used to configure Trino — only compared against, so a + // misconfigured charts release cannot quietly grant write access. + WriterRoleARN string + WriterUser string +} + +// TrinoReaderIdentity is the validated, credential-free reader identity. Every +// field is safe to log; the password is represented only by its Secret +// reference. +type TrinoReaderIdentity struct { + MetadataHost string + MetadataPort int + MetadataDatabase string + MetadataUser string + MetadataPasswordSecret TrinoReaderSecretRef + Bucket string + Region string + DataPath string + ReadOnlyRoleARN string + SSLMode string +} + +// TrinoReaderResolver produces the reader identity for one org. The production +// implementation reads the Duckling CR status and the config store; unit tests +// use a fake. +type TrinoReaderResolver interface { + ResolveTrinoReader(ctx context.Context, orgID string) (TrinoReaderIdentity, error) +} + +// JDBCURL renders the connection URL the Brikk DuckLake connector's +// ducklake.catalog.database-url property takes. It never contains credentials — +// user and password are separate properties. +func (i TrinoReaderIdentity) JDBCURL() string { + return "jdbc:postgresql://" + net.JoinHostPort(i.MetadataHost, strconv.Itoa(i.MetadataPort)) + + "/" + i.MetadataDatabase + "?sslmode=" + i.SSLMode +} + +// String renders the identity for logs. Safe by construction: the only +// credential is a Secret reference. +func (i TrinoReaderIdentity) String() string { + return fmt.Sprintf("metadata=%s user=%s password_secret=%s data_path=%s region=%s role=%s", + net.JoinHostPort(i.MetadataHost, strconv.Itoa(i.MetadataPort)), + i.MetadataUser, i.MetadataPasswordSecret, i.DataPath, i.Region, i.ReadOnlyRoleARN) +} + +// iamRoleARNRe matches an IAM ROLE ARN specifically — a user ARN or a bucket +// ARN in this position means the charts release published the wrong thing. +var iamRoleARNRe = regexp.MustCompile(`^arn:aws[a-z-]*:iam::\d{12}:role/.+$`) + +// buildTrinoReaderIdentity validates a source and fails closed. The error text +// names the missing or colliding field so an operator can fix the charts +// release; it never contains a credential value (the source has none). +func buildTrinoReaderIdentity(source TrinoReaderSource) (TrinoReaderIdentity, error) { + var missing []string + for _, field := range []struct { + name string + value string + }{ + {"metadata endpoint", source.MetadataEndpoint}, + {"metadata database", source.MetadataDatabase}, + {"metadata reader user", source.MetadataUser}, + {"data bucket", source.Bucket}, + {"data bucket region", source.Region}, + {"read-only S3 role ARN", source.ReadOnlyRoleARN}, + } { + if strings.TrimSpace(field.value) == "" { + missing = append(missing, field.name) + } + } + if !source.MetadataPasswordSecret.complete() { + missing = append(missing, "metadata reader password Secret reference (name, namespace, key)") + } + if len(missing) > 0 { + return TrinoReaderIdentity{}, fmt.Errorf( + "%w: the charts-created Trino reader identity is missing %s", + ErrTrinoBenchmarkConfig, strings.Join(missing, ", ")) + } + + if !iamRoleARNRe.MatchString(source.ReadOnlyRoleARN) { + return TrinoReaderIdentity{}, fmt.Errorf( + "%w: read-only S3 role %q is not an IAM role ARN", + ErrTrinoBenchmarkConfig, source.ReadOnlyRoleARN) + } + // Fail closed rather than hand Trino the tenant's write identity. + if source.WriterRoleARN != "" && source.ReadOnlyRoleARN == source.WriterRoleARN { + return TrinoReaderIdentity{}, fmt.Errorf( + "%w: the Trino reader S3 role equals the warehouse writer role %q", + ErrTrinoBenchmarkConfig, source.WriterRoleARN) + } + if source.WriterUser != "" && source.MetadataUser == source.WriterUser { + return TrinoReaderIdentity{}, fmt.Errorf( + "%w: the Trino metadata reader user equals the warehouse writer user %q", + ErrTrinoBenchmarkConfig, source.WriterUser) + } + + host, port, err := splitMetadataEndpoint(source.MetadataEndpoint) + if err != nil { + return TrinoReaderIdentity{}, fmt.Errorf("%w: %v", ErrTrinoBenchmarkConfig, err) + } + + sslMode := strings.TrimSpace(source.SSLMode) + if sslMode == "" { + sslMode = "require" + } + switch sslMode { + case "disable", "prefer", "require", "verify-ca", "verify-full": + default: + return TrinoReaderIdentity{}, fmt.Errorf( + "%w: unsupported metadata sslmode %q", ErrTrinoBenchmarkConfig, sslMode) + } + + dataPath := strings.TrimSpace(source.DataPath) + if dataPath == "" { + dataPath = "s3://" + source.Bucket + "/" + } + if !strings.HasPrefix(dataPath, "s3://") { + return TrinoReaderIdentity{}, fmt.Errorf( + "%w: data path %q is not an s3:// URI", ErrTrinoBenchmarkConfig, dataPath) + } + + return TrinoReaderIdentity{ + MetadataHost: host, + MetadataPort: port, + MetadataDatabase: source.MetadataDatabase, + MetadataUser: source.MetadataUser, + MetadataPasswordSecret: source.MetadataPasswordSecret, + Bucket: source.Bucket, + Region: source.Region, + DataPath: dataPath, + ReadOnlyRoleARN: source.ReadOnlyRoleARN, + SSLMode: sslMode, + }, nil +} + +// splitMetadataEndpoint accepts "host" or "host:port"; a bare host defaults to +// the Postgres port. +func splitMetadataEndpoint(endpoint string) (string, int, error) { + endpoint = strings.TrimSpace(endpoint) + host, portText, err := net.SplitHostPort(endpoint) + if err != nil { + return endpoint, 5432, nil + } + port, err := strconv.Atoi(portText) + if err != nil || port <= 0 || port > 65535 { + return "", 0, fmt.Errorf("metadata endpoint %q has an invalid port", endpoint) + } + return host, port, nil +} diff --git a/controlplane/trino_benchmark_reader_k8s.go b/controlplane/trino_benchmark_reader_k8s.go new file mode 100644 index 00000000..f197d479 --- /dev/null +++ b/controlplane/trino_benchmark_reader_k8s.go @@ -0,0 +1,122 @@ +//go:build kubernetes + +package controlplane + +import ( + "context" + "fmt" + "net" + "strconv" + + "github.com/posthog/duckgres/controlplane/configstore" + "github.com/posthog/duckgres/controlplane/provisioner" +) + +// ducklingTrinoReaderResolver is the production TrinoReaderResolver. It composes +// two existing sources of truth and adds nothing of its own: +// +// - the config-store warehouse row: the authoritative Duckling CR name plus +// the fallback bucket/region and the tenant's WRITER role (compared +// against, never used); and +// - the Duckling CR status: the metadata endpoint/database, the data bucket, +// and — published only by the companion charts release — +// status.benchmarkReader, holding the reader's database role, the exact +// Secret reference for its password, and the read-only S3 role ARN. +// +// It reads the CR status through GetStatusWithoutCredentials, so resolving a +// reader identity never pulls the tenant's writer password into memory. If the +// charts release is not deployed, status.benchmarkReader is absent and +// buildTrinoReaderIdentity fails closed. +type ducklingTrinoReaderResolver struct { + warehouses trinoReaderWarehouseStore + ducklings trinoReaderDucklingSource +} + +// trinoReaderWarehouseStore is the config-store surface the resolver needs. +type trinoReaderWarehouseStore interface { + GetManagedWarehouse(orgID string) (*configstore.ManagedWarehouse, error) +} + +// trinoReaderDucklingSource is the Duckling surface the resolver needs. It is +// deliberately the credential-free read. +type trinoReaderDucklingSource interface { + GetStatusWithoutCredentials(ctx context.Context, name string) (*provisioner.DucklingStatus, error) +} + +func newDucklingTrinoReaderResolver(warehouses trinoReaderWarehouseStore, ducklings trinoReaderDucklingSource) (*ducklingTrinoReaderResolver, error) { + if warehouses == nil || ducklings == nil { + return nil, fmt.Errorf("%w: Trino reader resolution needs both the config store and the Duckling client", ErrTrinoBenchmarkConfig) + } + return &ducklingTrinoReaderResolver{warehouses: warehouses, ducklings: ducklings}, nil +} + +func (r *ducklingTrinoReaderResolver) ResolveTrinoReader(ctx context.Context, orgID string) (TrinoReaderIdentity, error) { + warehouse, err := r.warehouses.GetManagedWarehouse(orgID) + if err != nil { + return TrinoReaderIdentity{}, fmt.Errorf("read managed warehouse for org %s: %w", orgID, err) + } + if warehouse == nil { + return TrinoReaderIdentity{}, fmt.Errorf("%w: org %s has no managed warehouse", ErrTrinoBenchmarkConfig, orgID) + } + ducklingName := warehouse.DucklingName + if ducklingName == "" { + ducklingName = orgID + } + + status, err := r.ducklings.GetStatusWithoutCredentials(ctx, ducklingName) + if err != nil { + return TrinoReaderIdentity{}, fmt.Errorf("read duckling %s status: %w", ducklingName, err) + } + if status == nil { + return TrinoReaderIdentity{}, fmt.Errorf("%w: duckling %s has no status", ErrTrinoBenchmarkConfig, ducklingName) + } + + host, port, viaPgBouncer, err := ducklingMetadataStoreAddress(status, orgID) + if err != nil { + return TrinoReaderIdentity{}, fmt.Errorf("%w: %v", ErrTrinoBenchmarkConfig, err) + } + // Same rule the internal metadata callers use: plaintext to the in-cluster + // pooler (which carries TLS onward), TLS straight to a direct endpoint. + sslMode := "require" + if viaPgBouncer { + sslMode = "disable" + } + + bucket := status.DataStore.BucketName + if bucket == "" { + bucket = warehouse.DataStore.BucketName + } + region := status.DataStore.S3Region + if region == "" { + region = warehouse.DataStore.Region + } + + reader := status.BenchmarkReader + return buildTrinoReaderIdentity(TrinoReaderSource{ + MetadataEndpoint: net.JoinHostPort(host, strconv.Itoa(port)), + MetadataDatabase: status.MetadataStore.Database, + MetadataUser: reader.MetadataUser, + MetadataPasswordSecret: TrinoReaderSecretRef{ + Name: reader.CredentialSecretRef.Name, + Namespace: reader.CredentialSecretRef.Namespace, + Key: reader.CredentialSecretRef.Key, + }, + Bucket: bucket, + Region: region, + ReadOnlyRoleARN: reader.S3ReadOnlyRoleARN, + SSLMode: sslMode, + // Compared against, never used: a charts release that publishes the + // tenant's own write identity here is refused outright. + WriterRoleARN: firstNonEmptyTrinoValue(status.IAMRoleARN, warehouse.WorkerIdentity.IAMRoleARN), + WriterUser: status.MetadataStore.User, + }) +} + +func firstNonEmptyTrinoValue(values ...string) string { + for _, value := range values { + if value != "" { + return value + } + } + return "" +} diff --git a/controlplane/trino_benchmark_reader_k8s_test.go b/controlplane/trino_benchmark_reader_k8s_test.go new file mode 100644 index 00000000..b8f15678 --- /dev/null +++ b/controlplane/trino_benchmark_reader_k8s_test.go @@ -0,0 +1,162 @@ +//go:build kubernetes + +package controlplane + +import ( + "context" + "errors" + "testing" + + "github.com/posthog/duckgres/controlplane/configstore" + "github.com/posthog/duckgres/controlplane/provisioner" +) + +type fakeTrinoWarehouseStore struct { + warehouse *configstore.ManagedWarehouse + err error +} + +func (f fakeTrinoWarehouseStore) GetManagedWarehouse(string) (*configstore.ManagedWarehouse, error) { + return f.warehouse, f.err +} + +type fakeTrinoDucklingSource struct { + status *provisioner.DucklingStatus + err error + name string +} + +func (f *fakeTrinoDucklingSource) GetStatusWithoutCredentials(_ context.Context, name string) (*provisioner.DucklingStatus, error) { + f.name = name + return f.status, f.err +} + +func readyDucklingStatusWithReader() *provisioner.DucklingStatus { + status := &provisioner.DucklingStatus{} + status.MetadataStore.Type = configstore.MetadataStoreKindCnpgShard + status.MetadataStore.PgBouncerEndpoint = "duckling-bench-org-pgbouncer.ducklings.svc.cluster.local:6432" + status.MetadataStore.Database = "ducklake_bench_org" + status.MetadataStore.User = "ducklake_bench_org" + status.DataStore.BucketName = "posthog-duckling-benchorg-dev" + status.DataStore.S3Region = "us-east-1" + status.IAMRoleARN = trinoTestWriterRoleARN + status.BenchmarkReader = provisioner.DucklingBenchmarkReader{ + MetadataUser: "trino_reader_bench_org", + CredentialSecretRef: provisioner.SecretReference{ + Name: trinoTestReaderSecret, Namespace: "ducklings", Key: "password", + }, + S3ReadOnlyRoleARN: "arn:aws:iam::123456789012:role/duckling-bench-org-trino-reader", + } + return status +} + +func TestDucklingTrinoReaderResolverBuildsIdentityFromChartsState(t *testing.T) { + ducklings := &fakeTrinoDucklingSource{status: readyDucklingStatusWithReader()} + resolver, err := newDucklingTrinoReaderResolver(fakeTrinoWarehouseStore{ + warehouse: &configstore.ManagedWarehouse{OrgID: "bench-org", DucklingName: "duckling-bench-org"}, + }, ducklings) + if err != nil { + t.Fatalf("newDucklingTrinoReaderResolver: %v", err) + } + + identity, err := resolver.ResolveTrinoReader(context.Background(), "bench-org") + if err != nil { + t.Fatalf("ResolveTrinoReader: %v", err) + } + if ducklings.name != "duckling-bench-org" { + t.Fatalf("resolved duckling %q, want the warehouse row's authoritative name", ducklings.name) + } + if identity.MetadataUser != "trino_reader_bench_org" { + t.Fatalf("metadata user = %q", identity.MetadataUser) + } + if identity.MetadataPasswordSecret.Name != trinoTestReaderSecret || identity.MetadataPasswordSecret.Namespace != "ducklings" { + t.Fatalf("password Secret reference = %s", identity.MetadataPasswordSecret) + } + if identity.ReadOnlyRoleARN != "arn:aws:iam::123456789012:role/duckling-bench-org-trino-reader" { + t.Fatalf("read-only role = %q", identity.ReadOnlyRoleARN) + } + // PgBouncer hop ⇒ plaintext to the pooler, exactly like the other internal + // metadata callers. + if identity.SSLMode != "disable" { + t.Fatalf("sslmode = %q, want disable through the duckling pooler", identity.SSLMode) + } + if identity.DataPath != "s3://posthog-duckling-benchorg-dev/" { + t.Fatalf("data path = %q", identity.DataPath) + } +} + +func TestDucklingTrinoReaderResolverFailsClosedWithoutChartsReaderBlock(t *testing.T) { + status := readyDucklingStatusWithReader() + status.BenchmarkReader = provisioner.DucklingBenchmarkReader{} + + resolver, err := newDucklingTrinoReaderResolver(fakeTrinoWarehouseStore{ + warehouse: &configstore.ManagedWarehouse{OrgID: "bench-org", DucklingName: "duckling-bench-org"}, + }, &fakeTrinoDucklingSource{status: status}) + if err != nil { + t.Fatalf("newDucklingTrinoReaderResolver: %v", err) + } + + _, err = resolver.ResolveTrinoReader(context.Background(), "bench-org") + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig until the charts reader resources are deployed", err) + } +} + +func TestDucklingTrinoReaderResolverRefusesWriterRoleInTheReaderField(t *testing.T) { + status := readyDucklingStatusWithReader() + status.BenchmarkReader.S3ReadOnlyRoleARN = trinoTestWriterRoleARN + + resolver, err := newDucklingTrinoReaderResolver(fakeTrinoWarehouseStore{ + warehouse: &configstore.ManagedWarehouse{OrgID: "bench-org", DucklingName: "duckling-bench-org"}, + }, &fakeTrinoDucklingSource{status: status}) + if err != nil { + t.Fatalf("newDucklingTrinoReaderResolver: %v", err) + } + + if _, err := resolver.ResolveTrinoReader(context.Background(), "bench-org"); !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } +} + +func TestDucklingTrinoReaderResolverFallsBackToWarehouseBucketAndRegion(t *testing.T) { + status := readyDucklingStatusWithReader() + status.DataStore.BucketName = "" + status.DataStore.S3Region = "" + + warehouse := &configstore.ManagedWarehouse{OrgID: "bench-org", DucklingName: "duckling-bench-org"} + warehouse.DataStore.BucketName = "posthog-duckling-benchorg-dev" + warehouse.DataStore.Region = "us-east-1" + + resolver, err := newDucklingTrinoReaderResolver(fakeTrinoWarehouseStore{warehouse: warehouse}, &fakeTrinoDucklingSource{status: status}) + if err != nil { + t.Fatalf("newDucklingTrinoReaderResolver: %v", err) + } + + identity, err := resolver.ResolveTrinoReader(context.Background(), "bench-org") + if err != nil { + t.Fatalf("ResolveTrinoReader: %v", err) + } + if identity.Bucket != "posthog-duckling-benchorg-dev" || identity.Region != "us-east-1" { + t.Fatalf("bucket/region = %s/%s", identity.Bucket, identity.Region) + } +} + +func TestDucklingTrinoReaderResolverFailsClosedWithoutWarehouse(t *testing.T) { + resolver, err := newDucklingTrinoReaderResolver(fakeTrinoWarehouseStore{}, &fakeTrinoDucklingSource{status: readyDucklingStatusWithReader()}) + if err != nil { + t.Fatalf("newDucklingTrinoReaderResolver: %v", err) + } + + if _, err := resolver.ResolveTrinoReader(context.Background(), "bench-org"); !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } +} + +func TestDucklingTrinoReaderResolverRequiresBothSources(t *testing.T) { + if _, err := newDucklingTrinoReaderResolver(nil, &fakeTrinoDucklingSource{}); !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } + if _, err := newDucklingTrinoReaderResolver(fakeTrinoWarehouseStore{}, nil); !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } +} diff --git a/controlplane/trino_benchmark_reader_test.go b/controlplane/trino_benchmark_reader_test.go new file mode 100644 index 00000000..6cb27cd0 --- /dev/null +++ b/controlplane/trino_benchmark_reader_test.go @@ -0,0 +1,178 @@ +package controlplane + +import ( + "errors" + "reflect" + "strings" + "testing" +) + +func completeTrinoReaderSource() TrinoReaderSource { + return TrinoReaderSource{ + MetadataEndpoint: "duckling-bench-org-pgbouncer.ducklings.svc.cluster.local:6432", + MetadataDatabase: "ducklake_bench_org", + MetadataUser: "trino_reader_bench_org", + MetadataPasswordSecret: TrinoReaderSecretRef{ + Name: "duckling-bench-org-trino-reader", + Namespace: "ducklings", + Key: "password", + }, + Bucket: "posthog-duckling-benchorg-dev", + Region: "us-east-1", + ReadOnlyRoleARN: "arn:aws:iam::123456789012:role/duckling-bench-org-trino-reader", + SSLMode: "disable", + WriterRoleARN: "arn:aws:iam::123456789012:role/duckling-bench-org", + WriterUser: "ducklake_bench_org", + } +} + +func TestBuildTrinoReaderIdentityFromCompleteSource(t *testing.T) { + identity, err := buildTrinoReaderIdentity(completeTrinoReaderSource()) + if err != nil { + t.Fatalf("buildTrinoReaderIdentity returned error: %v", err) + } + if identity.MetadataHost != "duckling-bench-org-pgbouncer.ducklings.svc.cluster.local" || identity.MetadataPort != 6432 { + t.Fatalf("metadata endpoint = %s:%d", identity.MetadataHost, identity.MetadataPort) + } + if got, want := identity.JDBCURL(), "jdbc:postgresql://duckling-bench-org-pgbouncer.ducklings.svc.cluster.local:6432/ducklake_bench_org?sslmode=disable"; got != want { + t.Fatalf("JDBCURL = %q, want %q", got, want) + } + if identity.MetadataUser != "trino_reader_bench_org" { + t.Fatalf("metadata user = %q", identity.MetadataUser) + } + if identity.DataPath != "s3://posthog-duckling-benchorg-dev/" { + t.Fatalf("data path = %q", identity.DataPath) + } + if identity.ReadOnlyRoleARN != "arn:aws:iam::123456789012:role/duckling-bench-org-trino-reader" { + t.Fatalf("read-only role = %q", identity.ReadOnlyRoleARN) + } +} + +func TestBuildTrinoReaderIdentityDefaultsMetadataPort(t *testing.T) { + source := completeTrinoReaderSource() + source.MetadataEndpoint = "bench-org.rds.example.com" + + identity, err := buildTrinoReaderIdentity(source) + if err != nil { + t.Fatalf("buildTrinoReaderIdentity returned error: %v", err) + } + if identity.MetadataPort != 5432 { + t.Fatalf("metadata port = %d, want the Postgres default 5432", identity.MetadataPort) + } +} + +func TestBuildTrinoReaderIdentityHonoursExplicitDataPath(t *testing.T) { + source := completeTrinoReaderSource() + source.DataPath = "s3://posthog-duckling-benchorg-dev/ducklake/" + + identity, err := buildTrinoReaderIdentity(source) + if err != nil { + t.Fatalf("buildTrinoReaderIdentity returned error: %v", err) + } + if identity.DataPath != "s3://posthog-duckling-benchorg-dev/ducklake/" { + t.Fatalf("data path = %q", identity.DataPath) + } +} + +// Fail-closed: every reader field the charts publish is mandatory. A partially +// deployed charts release must never produce a half-configured Trino cluster. +func TestBuildTrinoReaderIdentityFailsClosedOnMissingFields(t *testing.T) { + for name, mutate := range map[string]func(*TrinoReaderSource){ + "metadata endpoint": func(s *TrinoReaderSource) { s.MetadataEndpoint = "" }, + "metadata database": func(s *TrinoReaderSource) { s.MetadataDatabase = "" }, + "metadata user": func(s *TrinoReaderSource) { s.MetadataUser = "" }, + "secret name": func(s *TrinoReaderSource) { s.MetadataPasswordSecret.Name = "" }, + "secret namespace": func(s *TrinoReaderSource) { s.MetadataPasswordSecret.Namespace = "" }, + "secret key": func(s *TrinoReaderSource) { s.MetadataPasswordSecret.Key = "" }, + "bucket": func(s *TrinoReaderSource) { s.Bucket = "" }, + "region": func(s *TrinoReaderSource) { s.Region = "" }, + "read-only role arn": func(s *TrinoReaderSource) { s.ReadOnlyRoleARN = "" }, + } { + t.Run(name, func(t *testing.T) { + source := completeTrinoReaderSource() + mutate(&source) + _, err := buildTrinoReaderIdentity(source) + if err == nil { + t.Fatalf("missing %s must fail closed", name) + } + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } + }) + } +} + +// The whole point of the feature gate: Trino gets the charts-created read-only +// identity or nothing. It must never silently reuse the tenant writer role or +// the DuckLake writer login. +func TestBuildTrinoReaderIdentityRefusesWriterCredentials(t *testing.T) { + t.Run("writer role arn", func(t *testing.T) { + source := completeTrinoReaderSource() + source.ReadOnlyRoleARN = source.WriterRoleARN + _, err := buildTrinoReaderIdentity(source) + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } + if !strings.Contains(err.Error(), "writer") { + t.Fatalf("error %q should name the writer-role collision", err) + } + }) + + t.Run("writer database user", func(t *testing.T) { + source := completeTrinoReaderSource() + source.MetadataUser = source.WriterUser + _, err := buildTrinoReaderIdentity(source) + if !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("error = %v, want ErrTrinoBenchmarkConfig", err) + } + if !strings.Contains(err.Error(), "writer") { + t.Fatalf("error %q should name the writer-user collision", err) + } + }) +} + +func TestBuildTrinoReaderIdentityRejectsMalformedRoleARN(t *testing.T) { + for _, arn := range []string{ + "duckling-bench-org-trino-reader", + "arn:aws:s3:::posthog-duckling-benchorg-dev", + "arn:aws:iam::123456789012:user/duckling-bench-org-trino-reader", + } { + source := completeTrinoReaderSource() + source.ReadOnlyRoleARN = arn + if _, err := buildTrinoReaderIdentity(source); !errors.Is(err, ErrTrinoBenchmarkConfig) { + t.Fatalf("role ARN %q: error = %v, want ErrTrinoBenchmarkConfig", arn, err) + } + } +} + +// Structural tripwire: the resolved identity must stay a pure REFERENCE. If +// someone adds a credential VALUE field here it lands in every struct that +// embeds it, and from there in logs and errors. +func TestTrinoReaderIdentityCarriesNoCredentialValues(t *testing.T) { + typ := reflect.TypeOf(TrinoReaderIdentity{}) + for i := 0; i < typ.NumField(); i++ { + name := strings.ToLower(typ.Field(i).Name) + for _, banned := range []string{"password", "secretkey", "accesskey", "token", "credential"} { + // The Secret REFERENCE is fine; a value is not. + if strings.Contains(name, banned) && typ.Field(i).Type != reflect.TypeOf(TrinoReaderSecretRef{}) { + t.Fatalf("TrinoReaderIdentity.%s looks like a credential value; keep only Secret references here", typ.Field(i).Name) + } + } + } +} + +func TestTrinoReaderIdentityStringOmitsNothingSecretAndNamesTheSecretRef(t *testing.T) { + identity, err := buildTrinoReaderIdentity(completeTrinoReaderSource()) + if err != nil { + t.Fatalf("buildTrinoReaderIdentity returned error: %v", err) + } + got := identity.String() + if !strings.Contains(got, "ducklings/duckling-bench-org-trino-reader#password") { + t.Fatalf("String() = %q, want the Secret reference (name only)", got) + } + for _, banned := range []string{"hunter2", "aws-access-key", "aws-secret-key"} { + if strings.Contains(got, banned) { + t.Fatalf("String() = %q leaked %q", got, banned) + } + } +} diff --git a/docs/runbooks/scenario-runner.md b/docs/runbooks/scenario-runner.md index 792f305b..9e809d7d 100644 --- a/docs/runbooks/scenario-runner.md +++ b/docs/runbooks/scenario-runner.md @@ -49,7 +49,9 @@ export DUCKGRES_SCENARIO_FROZEN_S3_URI="s3:///frozen_v1/" ``` The full suite, fast suite, and targeted frozen perf scenarios exercise PGWire -only. Frozen perf records per-query success and failure rows in +only. `posthog_frozen_trino_perf.yaml` compares the same DuckLake tables over +PGWire and Trino; its raw-Parquet-view control queries remain PGWire-only. +Frozen perf records per-query success and failure rows in `query_results.csv`. Measured query errors fail the perf DAG step after its artifacts are written; independent sibling steps continue to run. @@ -97,6 +99,18 @@ Run frozen perf queries: just scenario-frozen-perf ``` +Run the paired PGWire/Trino comparison: + +```bash +just scenario-frozen-trino-perf +``` + +The scenario provisions a four-worker Trino cluster, waits for it to become +ready, benchmarks both protocols, and always tears Trino down before the +warehouse. It gets nothing from the control plane but a cluster ID, a lifecycle +state, the in-cluster endpoint, the worker counts, and the pinned image +reference — see "Trino benchmark lifecycle" below. + This runs, in order: raw-view setup, source-column preflight, explicit PostHog table DDL, registration of the frozen Parquet files in DuckLake, then partition and file-metadata validation. Registration reads Parquet footers but does not @@ -138,10 +152,98 @@ The targeted frozen metadata scenario uses: The frozen perf scenario uses: - `tests/mw-dev/scenario/scenarios/posthog_frozen_perf.yaml` -- `tests/perf/queries/ducklake_frozen.yaml` +- `tests/perf/queries/ducklake_posthog_tables.yaml` Perf artifacts are written under `artifacts/scenario//perf/` using the existing `tests/perf/core` artifact schema, including `query_results.csv`, `summary.json`, and `server_metrics.prom`. +The paired Trino scenario uses +`tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml` with the same +table setup and query catalog. Its artifact contains a row per protocol, so +`query_results.csv` directly compares the existing PGWire workload with Trino +on the identical DuckLake snapshot. Its explicit lifecycle calls are the only +place that may create or delete Trino; the scenario YAML and runner do not +receive metadata or S3 credentials. + +### Trino benchmark lifecycle + +**This feature is disabled and fail-closed until the companion charts release +is deployed.** Duckgres alone cannot run the paired scenario: it needs a +per-Duckling read-only S3 role, a dedicated metadata-Postgres reader +role/password Secret, and RBAC letting the control plane read that Secret by +exact name. Until those exist, provisioning fails with a configuration error and +the API answers `503`. There is deliberately no fallback to the tenant's writer +credentials. + +**Configuration** (env-only on the control plane, resolved in +`configresolve/resolve.go`): + +| Variable | Default | Meaning | +| --- | --- | --- | +| `DUCKGRES_TRINO_BENCHMARK_ENABLED` | `false` | Master gate. Enabled alone is not enough. | +| `DUCKGRES_TRINO_BENCHMARK_IMAGE` | `""` | Pinned Trino+Brikk image; **required** when enabled. Prefer a digest reference — it is what the artifact records. | +| `DUCKGRES_TRINO_BENCHMARK_IMAGE_PULL_POLICY` | `IfNotPresent` | Pull policy for coordinator and workers. | +| `DUCKGRES_TRINO_BENCHMARK_SERVICE_ACCOUNT` | `""` | ServiceAccount whose IAM identity may assume the read-only S3 role. | +| `DUCKGRES_TRINO_BENCHMARK_WORKERS` | `4` | Default worker replicas when a request omits `workers`. | +| `DUCKGRES_TRINO_BENCHMARK_COORDINATOR_CPU` / `_COORDINATOR_MEMORY` | `2` / `8Gi` | Coordinator shape. | +| `DUCKGRES_TRINO_BENCHMARK_WORKER_CPU` / `_WORKER_MEMORY` | `2` / `8Gi` | Per-worker shape. | + +Requests equal limits (Guaranteed QoS) for every benchmark pod, so Trino neither +bursts into nor is throttled by the Duckgres worker it is being compared with. +A request may ask for at most 16 workers. + +The mw-dev harness passes the image through +`DUCKGRES_TRINO_BENCHMARK_IMAGE` and enables the lifecycle only when that image +is set (`tests/mw-dev/run.sh`); the `scenario-dev` workflow exposes it as the +optional `trino_benchmark_image` input. Neither ever carries a credential. + +**Lifecycle API** (internal, admin-authenticated, under the existing +`/api/v1` router): + +| Route | Success | Notes | +| --- | --- | --- | +| `POST /api/v1/trino-benchmarks/orgs/:org_id/provision` | `202` created, `200` idempotent repeat | Body is optional `{"workers": N, "run_id": "..."}`; unknown fields are rejected. | +| `GET /api/v1/trino-benchmarks/status/:cluster_id` | `200` | `state` is `pending`, `ready`, or `failed`. | +| `POST /api/v1/trino-benchmarks/deprovision/:cluster_id` | `204` | Idempotent; an already-absent cluster is also `204`. | + +Errors: `400` invalid request, `404` unknown cluster, `409` a same-named cluster +exists with different ownership or configuration, `503` the feature is disabled +or the reader identity is not configured, `500` otherwise. Error bodies are +fixed strings — infrastructure detail is logged on the control plane, never +returned. + +Provisioning creates, in the control plane's namespace and all labelled with the +cluster ID and owning org: a ClusterIP Service selecting only the coordinator, +coordinator/worker/catalog ConfigMaps, a short-lived Secret holding only the +charts-created metadata reader password, a one-replica coordinator Deployment, +and a worker Deployment with exactly the requested replicas. Status is `ready` +only when the coordinator is ready **and** every requested worker replica is +ready. Cleanup deletes only objects carrying those ownership labels, so it is +safe after a partial provision and cannot touch another cluster, a Duckgres +worker, or the charts-created reader Secret. + +**Artifacts.** `summary.json` gains an `environments` array, one entry per +protocol: engine and version (Trino's from the coordinator's own `/v1/info`), +connector version, the pinned image reference, requested/ready worker counts, +catalog and schema, and the `UTC` session time zone. `query_results.csv` carries +a row per protocol per query, so the two engines are compared directly. No +thresholds and no CI gating are attached to any of it. + +**Failure recovery.** + +- `503` from provision: the feature is off, no image is pinned, or the charts + reader resources are missing. Check the control-plane log for the named + missing field. Do not work around it with writer credentials. +- `409` from provision: a cluster for that org already exists with a different + image or worker count — usually a leftover from an interrupted run. Deprovision + it (`POST .../deprovision/trino-bench-`) and retry. +- Readiness times out: the wait step reports the attempt count and last observed + state. Inspect the Deployments with + `kubectl get deploy -l duckgres.posthog.com/trino-benchmark-cluster=trino-bench-`. + A `failed` state is terminal — the poller stops rather than burning the budget. +- Leftover cluster after an aborted run: `deprovision_trino` is `always_run` and + precedes warehouse teardown, so this should be rare. Clean up manually with the + deprovision route, or by deleting the labelled objects. + The frozen dbt scenario uses: - `tests/mw-dev/scenario/scenarios/posthog_frozen_dbt.yaml` diff --git a/justfile b/justfile index ba08dc73..7e55242b 100644 --- a/justfile +++ b/justfile @@ -451,6 +451,12 @@ scenario-frozen-metadata: scenario-frozen-perf: ./scripts/scenario_run.sh tests/mw-dev/scenario/scenarios/posthog_frozen_perf.yaml +# Compare the frozen DuckLake PostHog tables over PGWire and a separately +# deployed, read-only multi-worker Trino service. +[group('test')] +scenario-frozen-trino-perf: + ./scripts/scenario_run.sh tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml + # Run the dev frozen dataset dbt scenario [group('test')] scenario-frozen-dbt: diff --git a/main.go b/main.go index 1d61f5ba..94ca0b6f 100644 --- a/main.go +++ b/main.go @@ -422,6 +422,7 @@ func main() { ExploratoryWorkerTTL: resolved.K8sExploratoryWorkerTTL, ReshardPodCPU: resolved.K8sReshardPodCPU, ReshardPodMemory: resolved.K8sReshardPodMemory, + TrinoBenchmark: resolved.TrinoBenchmark, AWSRegion: resolved.AWSRegion, }, } diff --git a/tests/mw-dev/manifests.tmpl.yaml b/tests/mw-dev/manifests.tmpl.yaml index 1b900adf..78cef0c2 100644 --- a/tests/mw-dev/manifests.tmpl.yaml +++ b/tests/mw-dev/manifests.tmpl.yaml @@ -308,6 +308,15 @@ spec: - { name: DUCKGRES_EXPLORATORY_WORKER_CPU, value: "1" } - { name: DUCKGRES_EXPLORATORY_WORKER_MEMORY, value: "2Gi" } - { name: DUCKGRES_EXPLORATORY_WORKER_TTL, value: "10m" } + # Dev-only Trino benchmark lifecycle (the paired PGWire/Trino + # comparison scenario). Fail-closed twice over: it is disabled + # unless the run pins an image, and even when enabled the control + # plane refuses to provision until the companion charts release + # publishes the per-Duckling read-only reader identity. NO reader + # credential appears here — the control plane reads the + # charts-created Secret by exact reference at provision time. + - { name: DUCKGRES_TRINO_BENCHMARK_ENABLED, value: "${DUCKGRES_TRINO_BENCHMARK_ENABLED}" } + - { name: DUCKGRES_TRINO_BENCHMARK_IMAGE, value: "${DUCKGRES_TRINO_BENCHMARK_IMAGE}" } - { name: DUCKGRES_K8S_WORKER_PROFILE_MIN_CPU, value: "1" } - { name: DUCKGRES_K8S_WORKER_PROFILE_MAX_CPU, value: "8" } - { name: DUCKGRES_K8S_WORKER_PROFILE_MIN_MEMORY, value: "2Gi" } diff --git a/tests/mw-dev/run.sh b/tests/mw-dev/run.sh index df134db8..3b6ee165 100755 --- a/tests/mw-dev/run.sh +++ b/tests/mw-dev/run.sh @@ -28,6 +28,19 @@ SCENARIO_NAME="${SCENARIO_NAME:-full-suite}" SCENARIO_ARTIFACTS_DIR="${SCENARIO_ARTIFACTS_DIR:-$HERE/../../artifacts/scenario-dev}" DUCKGRES_K8S_WORKER_CPU_REQUEST="${DUCKGRES_K8S_WORKER_CPU_REQUEST:-750m}" DUCKGRES_K8S_WORKER_MEMORY_REQUEST="${DUCKGRES_K8S_WORKER_MEMORY_REQUEST:-1536Mi}" +# Dev-only Trino benchmark lifecycle (posthog_frozen_trino_perf). OFF unless a +# run supplies a pinned Trino+Brikk image; the flag alone does nothing. The +# harness passes ONLY the image and the flag — the metadata reader password and +# the read-only S3 role are charts-created Kubernetes resources the control +# plane resolves for itself, so no credential passes through this script. +DUCKGRES_TRINO_BENCHMARK_IMAGE="${DUCKGRES_TRINO_BENCHMARK_IMAGE:-}" +DUCKGRES_TRINO_BENCHMARK_ENABLED="${DUCKGRES_TRINO_BENCHMARK_ENABLED:-false}" +if [ -n "$DUCKGRES_TRINO_BENCHMARK_IMAGE" ]; then + DUCKGRES_TRINO_BENCHMARK_ENABLED=true +elif [ "$DUCKGRES_TRINO_BENCHMARK_ENABLED" = "true" ]; then + echo "DUCKGRES_TRINO_BENCHMARK_ENABLED=true requires DUCKGRES_TRINO_BENCHMARK_IMAGE (a pinned Trino+Brikk image)." >&2 + exit 2 +fi E2E_SUITE="${E2E_SUITE:-full}" case "$E2E_SUITE" in full|reshard) ;; @@ -77,7 +90,9 @@ render() { WORKER_IMAGE="$WORKER_IMAGE" CONTROLPLANE_IMAGE="$CONTROLPLANE_IMAGE" \ DUCKGRES_K8S_WORKER_CPU_REQUEST="$DUCKGRES_K8S_WORKER_CPU_REQUEST" \ DUCKGRES_K8S_WORKER_MEMORY_REQUEST="$DUCKGRES_K8S_WORKER_MEMORY_REQUEST" \ - envsubst '$NAMESPACE $PR_NUMBER $WORKER_IMAGE $CONTROLPLANE_IMAGE $INTERNAL_SECRET $INTERNAL_SECRET_FALLBACK $USER_SECRET_KEY $DUCKGRES_K8S_WORKER_CPU_REQUEST $DUCKGRES_K8S_WORKER_MEMORY_REQUEST' \ + DUCKGRES_TRINO_BENCHMARK_ENABLED="$DUCKGRES_TRINO_BENCHMARK_ENABLED" \ + DUCKGRES_TRINO_BENCHMARK_IMAGE="$DUCKGRES_TRINO_BENCHMARK_IMAGE" \ + envsubst '$NAMESPACE $PR_NUMBER $WORKER_IMAGE $CONTROLPLANE_IMAGE $INTERNAL_SECRET $INTERNAL_SECRET_FALLBACK $USER_SECRET_KEY $DUCKGRES_K8S_WORKER_CPU_REQUEST $DUCKGRES_K8S_WORKER_MEMORY_REQUEST $DUCKGRES_TRINO_BENCHMARK_ENABLED $DUCKGRES_TRINO_BENCHMARK_IMAGE' \ < "$HERE/manifests.tmpl.yaml" } diff --git a/tests/mw-dev/run_sh_test.go b/tests/mw-dev/run_sh_test.go index 79ee06fa..630b1597 100644 --- a/tests/mw-dev/run_sh_test.go +++ b/tests/mw-dev/run_sh_test.go @@ -1579,3 +1579,77 @@ func (f runSHFakes) calls(t *testing.T) string { } return string(b) } + +// The dev Trino benchmark lifecycle is OFF unless a run supplies a pinned +// image. The harness passes the image and the enable flag; it never passes +// reader credentials — those are charts-created Kubernetes resources the +// control plane resolves for itself. +func TestControlPlaneTrinoBenchmarkIsOptInAndCredentialFree(t *testing.T) { + raw, err := os.ReadFile("manifests.tmpl.yaml") + if err != nil { + t.Fatalf("read manifests template: %v", err) + } + rendered := strings.NewReplacer( + "${NAMESPACE}", "test-namespace", + "${PR_NUMBER}", "123", + "${CONTROLPLANE_IMAGE}", "example.invalid/duckgres:test", + "${WORKER_IMAGE}", "example.invalid/duckgres:test", + "${INTERNAL_SECRET}", "test-secret", + "${INTERNAL_SECRET_FALLBACK}", "test-secret-fallback", + "${USER_SECRET_KEY}", "test-user-secret-key", + "${DUCKGRES_K8S_WORKER_CPU_REQUEST}", "2", + "${DUCKGRES_K8S_WORKER_MEMORY_REQUEST}", "4Gi", + "${DUCKGRES_TRINO_BENCHMARK_ENABLED}", "true", + "${DUCKGRES_TRINO_BENCHMARK_IMAGE}", "example.invalid/trino-brikk@sha256:abc", + ).Replace(string(raw)) + + decoder := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(rendered), 4096) + for { + var manifest map[string]any + err := decoder.Decode(&manifest) + if err == io.EOF { + break + } + if err != nil { + t.Fatalf("decode manifests template: %v", err) + } + if manifest["kind"] != "Deployment" || manifestName(manifest) != "duckgres-control-plane" { + continue + } + env := deploymentContainerEnv(manifest, "controlplane") + if got := env["DUCKGRES_TRINO_BENCHMARK_ENABLED"]; got != "true" { + t.Fatalf("Trino benchmark enable flag = %q, want the rendered value", got) + } + if got := env["DUCKGRES_TRINO_BENCHMARK_IMAGE"]; got != "example.invalid/trino-brikk@sha256:abc" { + t.Fatalf("Trino benchmark image = %q, want the pinned image", got) + } + for name := range env { + if strings.HasPrefix(name, "DUCKGRES_TRINO_BENCHMARK_") && + (strings.Contains(name, "PASSWORD") || strings.Contains(name, "SECRET") || strings.Contains(name, "KEY")) { + t.Fatalf("control plane env %s passes a Trino reader credential; those are charts-created resources", name) + } + } + return + } + t.Fatal("duckgres-control-plane Deployment missing from manifests template") +} + +func TestRenderDefaultsTrinoBenchmarkToDisabled(t *testing.T) { + raw, err := os.ReadFile("run.sh") + if err != nil { + t.Fatalf("read run.sh: %v", err) + } + script := string(raw) + for _, want := range []string{ + `DUCKGRES_TRINO_BENCHMARK_IMAGE="${DUCKGRES_TRINO_BENCHMARK_IMAGE:-}"`, + `DUCKGRES_TRINO_BENCHMARK_ENABLED="${DUCKGRES_TRINO_BENCHMARK_ENABLED:-false}"`, + `$DUCKGRES_TRINO_BENCHMARK_ENABLED $DUCKGRES_TRINO_BENCHMARK_IMAGE`, + } { + if !strings.Contains(script, want) { + t.Fatalf("run.sh missing Trino benchmark render contract %q", want) + } + } + if strings.Contains(script, "TRINO_READER_PASSWORD") || strings.Contains(script, "TRINO_DUCKLAKE_DB_PASSWORD") { + t.Fatal("run.sh must never carry Trino reader credentials") + } +} diff --git a/tests/mw-dev/scenario/perf/adapter_test.go b/tests/mw-dev/scenario/perf/adapter_test.go index f03acaa4..27b4e116 100644 --- a/tests/mw-dev/scenario/perf/adapter_test.go +++ b/tests/mw-dev/scenario/perf/adapter_test.go @@ -2,6 +2,7 @@ package perf import ( "context" + "encoding/json" "errors" "os" "path/filepath" @@ -12,6 +13,7 @@ import ( "github.com/posthog/duckgres/tests/mw-dev/scenario/core" "github.com/posthog/duckgres/tests/mw-dev/scenario/provision" scenariosql "github.com/posthog/duckgres/tests/mw-dev/scenario/sql" + scenariotrino "github.com/posthog/duckgres/tests/mw-dev/scenario/trino" perfcore "github.com/posthog/duckgres/tests/perf/core" ) @@ -142,6 +144,98 @@ func TestExecutorRestrictsCatalogToStepTargets(t *testing.T) { } } +func TestExecutorRunsTrinoTargetFromConfiguredEndpoint(t *testing.T) { + catalogPath := writeTrinoPerfCatalog(t) + factory := &fakeDriverFactory{} + executor := NewExecutor(ExecutorConfig{ + OutputDir: t.TempDir(), + TrinoEndpoint: "http://trino.scenario.svc:8080", + DriverFactory: factory, + }) + + err := executor.ExecuteStep(context.Background(), core.Step{ + ID: "perf_queries", + Type: StepTypePerfQueries, + With: map[string]any{ + "org_id": "scenario-org", + "catalog_file": catalogPath, + "run_id": "scenario-run-1", + "targets": []any{"trino"}, + }, + }) + if err != nil { + t.Fatalf("ExecuteStep returned error: %v", err) + } + if factory.trinoConnection.Endpoint != "http://trino.scenario.svc:8080" { + t.Fatalf("Trino endpoint = %q", factory.trinoConnection.Endpoint) + } + result, ok := executor.State().Result("perf_queries") + if !ok || result.Summary.TotalQueries != 1 || result.Summary.TotalErrors != 0 { + t.Fatalf("summary = %+v", result.Summary) + } +} + +func TestExecutorUsesReadyTrinoLifecycleEndpoint(t *testing.T) { + catalogPath := writeTrinoPerfCatalog(t) + trinoState := scenariotrino.NewState() + trinoState.StoreCluster("scenario-org", scenariotrino.Cluster{ID: "trino-run-1", Endpoint: "http://trino.scenario.svc:8080"}) + factory := &fakeDriverFactory{} + executor := NewExecutor(ExecutorConfig{ + OutputDir: t.TempDir(), + TrinoState: trinoState, + DriverFactory: factory, + }) + + err := executor.ExecuteStep(context.Background(), core.Step{ + ID: "perf_queries", Type: StepTypePerfQueries, + With: map[string]any{ + "org_id": "scenario-org", "catalog_file": catalogPath, "run_id": "scenario-run-1", "targets": []any{"trino"}, + }, + }) + if err != nil { + t.Fatalf("ExecuteStep returned error: %v", err) + } + if factory.trinoConnection.Endpoint != "http://trino.scenario.svc:8080" { + t.Fatalf("Trino endpoint = %q, want lifecycle endpoint", factory.trinoConnection.Endpoint) + } +} + +func TestExecutorSkipsQueriesWithoutSQLForTarget(t *testing.T) { + catalogPath := writeTargetSpecificPerfCatalog(t) + provisionState := provision.NewState() + provisionState.StoreProvisionResponse("scenario-org", provision.ProvisionResponse{Username: "root", Password: "root-password"}) + factory := &fakeDriverFactory{} + executor := NewExecutor(ExecutorConfig{ + ProvisionState: provisionState, + Connection: scenariosql.ConnectionConfig{ + DialHost: "10.0.0.10", SNISuffix: ".dev.example", SSLMode: "require", + }, + OutputDir: t.TempDir(), + TrinoEndpoint: "http://trino.scenario.svc:8080", + DriverFactory: factory, + }) + + err := executor.ExecuteStep(context.Background(), core.Step{ + ID: "perf_queries", Type: StepTypePerfQueries, + With: map[string]any{ + "org_id": "scenario-org", "catalog_file": catalogPath, "run_id": "scenario-run-1", + }, + }) + if err != nil { + t.Fatalf("ExecuteStep returned error: %v", err) + } + if factory.pgwireDriver.calls != 2 { + t.Fatalf("pgwire calls = %d, want two queries", factory.pgwireDriver.calls) + } + if factory.trinoDriver.calls != 1 { + t.Fatalf("Trino calls = %d, want only the shared query", factory.trinoDriver.calls) + } + result, ok := executor.State().Result("perf_queries") + if !ok || result.Summary.TotalQueries != 3 { + t.Fatalf("summary = %+v, want three target-applicable queries", result.Summary) + } +} + func TestExecutorRejectsTargetOverrideOutsideCatalog(t *testing.T) { catalogPath := writePerfCatalog(t, []perfcore.Protocol{perfcore.ProtocolPGWire}) provisionState := provision.NewState() @@ -333,10 +427,63 @@ func writePerfCatalog(t *testing.T, targets []perfcore.Protocol) string { return path } +func writeTrinoPerfCatalog(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "trino_perf_catalog.yaml") + body := "name: scenario-trino-perf\n" + + "description: trino perf adapter test\n" + + "seed: 42\n" + + "dataset_scale: 1\n" + + "targets: [trino]\n" + + "warmup_iterations: 0\n" + + "measure_iterations: 1\n" + + "queries:\n" + + " - query_id: q1\n" + + " intent_id: i1\n" + + " tags: [test]\n" + + " params: {}\n" + + " trino_sql: SELECT 1\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write Trino perf catalog: %v", err) + } + return path +} + +func writeTargetSpecificPerfCatalog(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "target_specific_perf_catalog.yaml") + body := "name: target-specific-perf\n" + + "description: target-specific perf adapter test\n" + + "seed: 42\n" + + "dataset_scale: 1\n" + + "targets: [pgwire, trino]\n" + + "warmup_iterations: 0\n" + + "measure_iterations: 1\n" + + "queries:\n" + + " - query_id: pgwire_only\n" + + " intent_id: i1\n" + + " tags: [test]\n" + + " params: {}\n" + + " pgwire_sql: SELECT 1\n" + + " - query_id: shared\n" + + " intent_id: i2\n" + + " tags: [test]\n" + + " params: {}\n" + + " pgwire_sql: SELECT 2\n" + + " trino_sql: SELECT 2\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write target-specific perf catalog: %v", err) + } + return path +} + type fakeDriverFactory struct { pgwireConnection scenariosql.PGWireConnection pgwireErr error pgwireDriver *fakeProtocolDriver + trinoConnection TrinoConnection + trinoErr error + trinoDriver *fakeProtocolDriver } func (f *fakeDriverFactory) NewPGWire(connection scenariosql.PGWireConnection) (perfcore.ProtocolDriver, error) { @@ -345,15 +492,23 @@ func (f *fakeDriverFactory) NewPGWire(connection scenariosql.PGWireConnection) ( return f.pgwireDriver, nil } +func (f *fakeDriverFactory) NewTrino(connection TrinoConnection) (perfcore.ProtocolDriver, error) { + f.trinoConnection = connection + f.trinoDriver = &fakeProtocolDriver{protocol: perfcore.ProtocolTrino, err: f.trinoErr} + return f.trinoDriver, nil +} + type fakeProtocolDriver struct { protocol perfcore.Protocol err error closed bool + calls int } func (d *fakeProtocolDriver) Protocol() perfcore.Protocol { return d.protocol } func (d *fakeProtocolDriver) Execute(context.Context, perfcore.Query, []any) (perfcore.ExecutionResult, error) { + d.calls++ return perfcore.ExecutionResult{Rows: 1, Duration: time.Millisecond}, d.err } @@ -361,3 +516,93 @@ func (d *fakeProtocolDriver) Close() error { d.closed = true return nil } + +// The perf artifact must state WHAT ran on each side of the comparison, taken +// from the lifecycle state rather than assumed — and must contain nothing +// credential-shaped. +func TestExecutorRecordsTrinoComparisonMetadataInArtifacts(t *testing.T) { + catalogPath := writeTargetSpecificPerfCatalog(t) + trinoState := scenariotrino.NewState() + trinoState.StoreCluster("scenario-org", scenariotrino.Cluster{ + ID: "trino-bench-scenario-org", + State: scenariotrino.StateReady, + Endpoint: "http://trino.scenario.svc:8080", + RequestedWorkers: 4, + ReadyWorkers: 4, + Image: "registry.example/trino-brikk@sha256:abc", + }) + provisionState := provision.NewState() + provisionState.StoreProvisionResponse("scenario-org", provision.ProvisionResponse{Username: "root", Password: "root-password"}) + outputDir := t.TempDir() + executor := NewExecutor(ExecutorConfig{ + ProvisionState: provisionState, + Connection: scenariosql.ConnectionConfig{ + DialHost: "10.0.0.10", SNISuffix: ".dev.example", SSLMode: "require", + }, + OutputDir: outputDir, + TrinoState: trinoState, + DriverFactory: &fakeDriverFactory{}, + }) + + err := executor.ExecuteStep(context.Background(), core.Step{ + ID: "perf_queries", Type: StepTypePerfQueries, + With: map[string]any{ + "org_id": "scenario-org", "catalog_file": catalogPath, "run_id": "scenario-run-1", + "trino_catalog": "ducklake", "trino_schema": "posthog", + "trino_connector_version": "483-0.2.0", + }, + }) + if err != nil { + t.Fatalf("ExecuteStep returned error: %v", err) + } + + raw, err := os.ReadFile(filepath.Join(outputDir, "perf", "summary.json")) + if err != nil { + t.Fatalf("read summary: %v", err) + } + var summary perfcore.RunSummary + if err := json.Unmarshal(raw, &summary); err != nil { + t.Fatalf("decode summary: %v", err) + } + environments := map[perfcore.Protocol]perfcore.ProtocolEnvironment{} + for _, env := range summary.Environments { + environments[env.Protocol] = env + } + pgwire, ok := environments[perfcore.ProtocolPGWire] + if !ok || pgwire.Engine != "duckgres" || pgwire.TimeZone != "UTC" { + t.Fatalf("pgwire environment = %+v", pgwire) + } + trino, ok := environments[perfcore.ProtocolTrino] + if !ok { + t.Fatalf("summary environments = %+v, want a trino entry", summary.Environments) + } + if trino.Image != "registry.example/trino-brikk@sha256:abc" { + t.Fatalf("trino image = %q, want the lifecycle-reported pinned image", trino.Image) + } + if trino.RequestedWorkers != 4 || trino.ReadyWorkers != 4 { + t.Fatalf("trino worker counts = %d/%d", trino.RequestedWorkers, trino.ReadyWorkers) + } + if trino.Catalog != "ducklake" || trino.Schema != "posthog" || trino.TimeZone != "UTC" { + t.Fatalf("trino catalog identity = %+v", trino) + } + if trino.ConnectorVersion != "483-0.2.0" { + t.Fatalf("trino connector version = %q", trino.ConnectorVersion) + } + + // Nothing credential-shaped may reach any artifact in the run directory. + entries, err := os.ReadDir(filepath.Join(outputDir, "perf")) + if err != nil { + t.Fatalf("read artifacts: %v", err) + } + for _, entry := range entries { + body, err := os.ReadFile(filepath.Join(outputDir, "perf", entry.Name())) + if err != nil { + t.Fatalf("read %s: %v", entry.Name(), err) + } + for _, banned := range []string{"root-password", "arn:aws", "password="} { + if strings.Contains(string(body), banned) { + t.Fatalf("artifact %s contains %q", entry.Name(), banned) + } + } + } +} diff --git a/tests/mw-dev/scenario/perf/steps.go b/tests/mw-dev/scenario/perf/steps.go index 235d1305..e6b17c69 100644 --- a/tests/mw-dev/scenario/perf/steps.go +++ b/tests/mw-dev/scenario/perf/steps.go @@ -11,19 +11,39 @@ import ( "github.com/posthog/duckgres/tests/mw-dev/scenario/core" "github.com/posthog/duckgres/tests/mw-dev/scenario/provision" scenariosql "github.com/posthog/duckgres/tests/mw-dev/scenario/sql" + scenariotrino "github.com/posthog/duckgres/tests/mw-dev/scenario/trino" perfcore "github.com/posthog/duckgres/tests/perf/core" pgdriver "github.com/posthog/duckgres/tests/perf/drivers/pgwire" + trinodriver "github.com/posthog/duckgres/tests/perf/drivers/trino" ) const StepTypePerfQueries = "perf_queries" +// perfTimeZone is the session time zone every perf protocol runs in. Comparing +// TIMESTAMPTZ predicates across engines is only meaningful when both interpret +// them identically, and the artifact records it so a reader can check. +const perfTimeZone = "UTC" + type DriverFactory interface { NewPGWire(connection scenariosql.PGWireConnection) (perfcore.ProtocolDriver, error) + NewTrino(connection TrinoConnection) (perfcore.ProtocolDriver, error) +} + +// TrinoConnection contains only the non-secret settings required by the Trino +// HTTP statement driver. Credentials remain owned by the provisioned cluster. +type TrinoConnection struct { + Endpoint string + User string + Catalog string + Schema string + TimeZone string } type ExecutorConfig struct { ProvisionState *provision.State Connection scenariosql.ConnectionConfig + TrinoEndpoint string + TrinoState *scenariotrino.State OutputDir string DriverFactory DriverFactory State *State @@ -33,6 +53,8 @@ type ExecutorConfig struct { type Executor struct { provisionState *provision.State connection scenariosql.ConnectionConfig + trinoEndpoint string + trinoState *scenariotrino.State outputDir string driverFactory DriverFactory state *State @@ -59,6 +81,11 @@ type stepSpec struct { RunID string DatasetVersion string Database string + TrinoEndpoint string + TrinoUser string + TrinoCatalog string + TrinoSchema string + TrinoConnector string OutputSubdir string ReadOnly bool FailOnQueryErrors bool @@ -82,6 +109,8 @@ func NewExecutor(cfg ExecutorConfig) *Executor { return &Executor{ provisionState: cfg.ProvisionState, connection: cfg.Connection, + trinoEndpoint: cfg.TrinoEndpoint, + trinoState: cfg.TrinoState, outputDir: cfg.OutputDir, driverFactory: factory, state: state, @@ -162,6 +191,7 @@ func (e *Executor) ExecuteStep(ctx context.Context, step core.Step) error { Drivers: drivers, Sink: closingSink{sink: sink, closeFunc: closeSink}, Now: e.now, + Environments: e.environments(catalog, spec), }) summary, err := runner.Run(ctx) if err != nil { @@ -216,7 +246,7 @@ func (e *Executor) parseStep(step core.Step) (stepSpec, error) { username := stringFromWith(step, "username", "root") password := stringFromWith(step, "password", "") - if password == "" { + if (len(targets) == 0 || containsTarget(targets, perfcore.ProtocolPGWire)) && password == "" { if e.provisionState == nil { return stepSpec{}, classified(ErrorClassConfig, fmt.Errorf("provision state is required when with.password is omitted")) } @@ -239,6 +269,11 @@ func (e *Executor) parseStep(step core.Step) (stepSpec, error) { RunID: runID, DatasetVersion: stringFromWith(step, "dataset_version", ""), Database: stringFromWith(step, "catalog", "ducklake"), + TrinoEndpoint: stringFromWith(step, "trino_endpoint", e.trinoEndpoint), + TrinoUser: stringFromWith(step, "trino_user", "duckgres-perf"), + TrinoCatalog: stringFromWith(step, "trino_catalog", stringFromWith(step, "catalog", "ducklake")), + TrinoSchema: stringFromWith(step, "trino_schema", "posthog"), + TrinoConnector: stringFromWith(step, "trino_connector_version", ""), OutputSubdir: stringFromWith(step, "output_subdir", "perf"), ReadOnly: boolFromWith(step, "read_only", true), FailOnQueryErrors: boolFromWith(step, "fail_on_query_errors", true), @@ -264,7 +299,7 @@ func targetsFromWith(step core.Step) ([]perfcore.Protocol, error) { } target := perfcore.Protocol(value) switch target { - case perfcore.ProtocolPGWire: + case perfcore.ProtocolPGWire, perfcore.ProtocolTrino: default: return nil, classified(ErrorClassConfig, fmt.Errorf("step %s with.targets[%d] has unsupported perf protocol %q", step.ID, i, target)) } @@ -318,6 +353,16 @@ func (e *Executor) driversForCatalog(catalog perfcore.Catalog, spec stepSpec) (m return nil, classified(ErrorClassConfig, fmt.Errorf("create pgwire perf driver: %w", err)) } drivers[target] = driver + case perfcore.ProtocolTrino: + connection, err := e.trinoConnection(spec) + if err != nil { + return nil, err + } + driver, err := e.driverFactory.NewTrino(connection) + if err != nil { + return nil, classified(ErrorClassConfig, fmt.Errorf("create Trino perf driver: %w", err)) + } + drivers[target] = driver default: return nil, classified(ErrorClassConfig, fmt.Errorf("unsupported perf target protocol %q", target)) } @@ -326,6 +371,69 @@ func (e *Executor) driversForCatalog(catalog perfcore.Catalog, spec stepSpec) (m return drivers, nil } +// environments records the non-secret comparison metadata for each protocol in +// the run's summary.json. For Trino it reports what the control plane actually +// provisioned — the pinned image and the requested/ready worker counts — so a +// reader can tell whether the topology the benchmark claims is the topology +// that ran. Credentials are structurally absent: the lifecycle state carries +// none. +func (e *Executor) environments(catalog perfcore.Catalog, spec stepSpec) []perfcore.ProtocolEnvironment { + environments := make([]perfcore.ProtocolEnvironment, 0, len(catalog.Targets)) + for _, target := range catalog.Targets { + switch target { + case perfcore.ProtocolPGWire: + environments = append(environments, perfcore.ProtocolEnvironment{ + Protocol: perfcore.ProtocolPGWire, + Engine: "duckgres", + Catalog: spec.Database, + TimeZone: perfTimeZone, + }) + case perfcore.ProtocolTrino: + env := perfcore.ProtocolEnvironment{ + Protocol: perfcore.ProtocolTrino, + Engine: "trino", + ConnectorVersion: spec.TrinoConnector, + Catalog: spec.TrinoCatalog, + Schema: spec.TrinoSchema, + TimeZone: perfTimeZone, + } + if cluster, ok := e.trinoCluster(spec.OrgID); ok { + env.Image = cluster.Image + env.RequestedWorkers = cluster.RequestedWorkers + env.ReadyWorkers = cluster.ReadyWorkers + } + environments = append(environments, env) + } + } + return environments +} + +func (e *Executor) trinoCluster(orgID string) (scenariotrino.Cluster, bool) { + if e.trinoState == nil { + return scenariotrino.Cluster{}, false + } + return e.trinoState.Cluster(orgID) +} + +func (e *Executor) trinoConnection(spec stepSpec) (TrinoConnection, error) { + endpoint := spec.TrinoEndpoint + if endpoint == "" { + if cluster, ok := e.trinoCluster(spec.OrgID); ok { + endpoint = cluster.Endpoint + } + } + if endpoint == "" { + return TrinoConnection{}, classified(ErrorClassConfig, fmt.Errorf("trino endpoint is required when target includes trino")) + } + return TrinoConnection{ + Endpoint: endpoint, + User: spec.TrinoUser, + Catalog: spec.TrinoCatalog, + Schema: spec.TrinoSchema, + TimeZone: perfTimeZone, + }, nil +} + func (e *Executor) pgwireConnection(spec stepSpec) (scenariosql.PGWireConnection, error) { cfg := e.connection cfg.OrgID = spec.OrgID @@ -353,6 +461,25 @@ func (defaultDriverFactory) NewPGWire(connection scenariosql.PGWireConnection) ( return pgdriver.NewWithDB(db), nil } +func (defaultDriverFactory) NewTrino(connection TrinoConnection) (perfcore.ProtocolDriver, error) { + return trinodriver.New(trinodriver.Config{ + Endpoint: connection.Endpoint, + User: connection.User, + Catalog: connection.Catalog, + Schema: connection.Schema, + TimeZone: connection.TimeZone, + }) +} + +func containsTarget(targets []perfcore.Protocol, target perfcore.Protocol) bool { + for _, candidate := range targets { + if candidate == target { + return true + } + } + return false +} + func requiredString(step core.Step, key string) (string, error) { value, ok := step.With[key].(string) if !ok || value == "" { diff --git a/tests/mw-dev/scenario/runner_test.go b/tests/mw-dev/scenario/runner_test.go index 3a7cdac3..d8a70213 100644 --- a/tests/mw-dev/scenario/runner_test.go +++ b/tests/mw-dev/scenario/runner_test.go @@ -17,6 +17,7 @@ import ( scenarioperf "github.com/posthog/duckgres/tests/mw-dev/scenario/perf" "github.com/posthog/duckgres/tests/mw-dev/scenario/provision" scenariosql "github.com/posthog/duckgres/tests/mw-dev/scenario/sql" + scenariotrino "github.com/posthog/duckgres/tests/mw-dev/scenario/trino" ) var ( @@ -58,6 +59,13 @@ func TestScenarioRunner(t *testing.T) { if err != nil { t.Fatalf("create provision client: %v", err) } + trinoClient, err := scenariotrino.NewClient(scenariotrino.ClientConfig{ + BaseURL: mustEnv(t, "DUCKGRES_SCENARIO_API_BASE"), + InternalSecret: mustEnv(t, "DUCKGRES_SCENARIO_INTERNAL_SECRET"), + }) + if err != nil { + t.Fatalf("create Trino lifecycle client: %v", err) + } provisionState := provision.NewState() provisionExecutor := provision.NewExecutor(provision.ExecutorConfig{ Client: provisionClient, @@ -80,6 +88,7 @@ func TestScenarioRunner(t *testing.T) { }, }) scenarioOutputDir := filepath.Join(*scenarioOutputBase, runID) + trinoState := scenariotrino.NewState() perfExecutor := scenarioperf.NewExecutor(scenarioperf.ExecutorConfig{ ProvisionState: provisionState, Connection: scenariosql.ConnectionConfig{ @@ -90,7 +99,8 @@ func TestScenarioRunner(t *testing.T) { ConnectTimeout: intEnv(t, "DUCKGRES_SCENARIO_PG_CONNECT_TIMEOUT", 10), ApplicationName: "duckgres-scenario-runner", }, - OutputDir: scenarioOutputDir, + OutputDir: scenarioOutputDir, + TrinoState: trinoState, }) dbtExecutor := scenariodbt.NewExecutor(scenariodbt.ExecutorConfig{ ProvisionState: provisionState, @@ -105,13 +115,14 @@ func TestScenarioRunner(t *testing.T) { OutputDir: scenarioOutputDir, DBTBinary: envOrDefault("DUCKGRES_SCENARIO_DBT_BIN", "dbt"), }) + trinoExecutor := scenariotrino.NewExecutor(scenariotrino.ExecutorConfig{Lifecycle: trinoClient, State: trinoState}) ctx, cancel := context.WithTimeout(context.Background(), *scenarioMaxRuntime) defer cancel() runner := core.NewRunner(core.RunnerConfig{ RunID: runID, Scenario: loaded, - Executor: dispatchExecutor{provision: provisionExecutor, sql: sqlExecutor, perf: perfExecutor, dbt: dbtExecutor}, + Executor: dispatchExecutor{provision: provisionExecutor, sql: sqlExecutor, perf: perfExecutor, dbt: dbtExecutor, trino: trinoExecutor}, OutputDir: scenarioOutputDir, WriteFiles: true, CleanupTimeout: 15 * time.Minute, @@ -163,10 +174,12 @@ func TestProvisionSmokeScenarioUsesIsolatedStackWarehouseIdentityAndSupportedSte func TestFrozenSuccessScenariosUseIsolatedStackWarehouseIdentity(t *testing.T) { const scenarioOrgID = "ci-pr-123-cnpg" + t.Setenv("DUCKGRES_SCENARIO_FROZEN_S3_URI", "s3://example-frozen/frozen_v1/") for _, scenarioFile := range []string{ "posthog_frozen_metadata.yaml", "posthog_frozen_perf.yaml", + "posthog_frozen_trino_perf.yaml", "posthog_frozen_dbt.yaml", "fast-suite.yaml", "full-suite.yaml", @@ -209,6 +222,18 @@ func TestFrozenSuccessScenariosUseIsolatedStackWarehouseIdentity(t *testing.T) { } } +func TestDispatchSupportsTrinoLifecycleSteps(t *testing.T) { + for _, stepType := range []string{ + scenariotrino.StepTypeProvisionTrino, + scenariotrino.StepTypeWaitTrinoReady, + scenariotrino.StepTypeDeprovisionTrino, + } { + if !dispatchSupports(stepType) { + t.Fatalf("dispatch does not support Trino lifecycle step %q", stepType) + } + } +} + func TestFastSuiteScenarioComposesWorkloadsWithoutDBT(t *testing.T) { t.Setenv("DUCKGRES_SCENARIO_FROZEN_S3_URI", "s3://example-frozen/frozen_v1/") t.Setenv("DUCKGRES_SCENARIO_ORG_ID", "ci-pr-123-cnpg") @@ -445,6 +470,9 @@ func TestLoadScenarioForRunResolvesScenarioRelativeFiles(t *testing.T) { } } +// The PGWire-only frozen perf scenario is UNCHANGED by the Trino work: the +// paired comparison lives in its own scenario file. These two cases are the +// tripwire for that — Trino must never be silently enabled here. func TestFrozenPerfScenarioUsesSupportedStepsAndRelativeCatalog(t *testing.T) { t.Setenv("DUCKGRES_SCENARIO_FROZEN_S3_URI", "s3://example-frozen/frozen_v1/") t.Setenv("DUCKGRES_SCENARIO_ORG_ID", "ci-pr-123-cnpg") @@ -463,6 +491,10 @@ func TestFrozenPerfScenarioUsesSupportedStepsAndRelativeCatalog(t *testing.T) { if !dispatchSupports(step.Type) { t.Fatalf("step %s has unsupported type %q", step.ID, step.Type) } + switch step.Type { + case scenariotrino.StepTypeProvisionTrino, scenariotrino.StepTypeWaitTrinoReady, scenariotrino.StepTypeDeprovisionTrino: + t.Fatalf("PGWire-only frozen perf scenario must not contain Trino lifecycle step %s", step.ID) + } if containsTemplate(step.With) { t.Fatalf("step %s still contains unresolved template values: %#v", step.ID, step.With) } @@ -504,6 +536,91 @@ func TestFrozenPerfScenarioBuildsAndValidatesPostHogTablesBeforePerf(t *testing. t.Fatalf("resolve templates: %v", err) } + steps := make(map[string]core.Step, len(resolved.Steps)) + for _, step := range resolved.Steps { + steps[step.ID] = step + } + if got := steps["setup_posthog_tables"].DependsOn; len(got) != 1 || got[0] != "setup_frozen_views" { + t.Fatalf("posthog setup dependencies = %#v, want [setup_frozen_views]", got) + } + if got := steps["validate_posthog_tables"].DependsOn; len(got) != 1 || got[0] != "setup_posthog_tables" { + t.Fatalf("posthog validation dependencies = %#v, want [validate_posthog_tables]", got) + } + if got := steps["perf_queries"].DependsOn; len(got) != 1 || got[0] != "validate_posthog_tables" { + t.Fatalf("perf dependencies = %#v, want [validate_posthog_tables]", got) + } +} + +func TestFrozenTrinoPerfScenarioUsesSupportedStepsAndRelativeCatalog(t *testing.T) { + t.Setenv("DUCKGRES_SCENARIO_FROZEN_S3_URI", "s3://example-frozen/frozen_v1/") + t.Setenv("DUCKGRES_SCENARIO_ORG_ID", "ci-pr-123-cnpg") + + scenario, _, err := loadScenarioForRun(filepath.Join("scenarios", "posthog_frozen_trino_perf.yaml")) + if err != nil { + t.Fatalf("load frozen perf scenario: %v", err) + } + resolved, err := resolveRunTemplates(scenario, "scenario-frozen-trino-perf-20260102t030405z") + if err != nil { + t.Fatalf("resolve templates: %v", err) + } + + foundPerf := false + for _, step := range resolved.Steps { + if !dispatchSupports(step.Type) { + t.Fatalf("step %s has unsupported type %q", step.ID, step.Type) + } + if containsTemplate(step.With) { + t.Fatalf("step %s still contains unresolved template values: %#v", step.ID, step.With) + } + if step.Type != scenarioperf.StepTypePerfQueries { + continue + } + foundPerf = true + catalogFile, ok := step.With["catalog_file"].(string) + if !ok || !filepath.IsAbs(catalogFile) { + t.Fatalf("perf catalog_file = %#v, want absolute path", step.With["catalog_file"]) + } + if _, err := os.Stat(catalogFile); err != nil { + t.Fatalf("perf catalog file %q should exist: %v", catalogFile, err) + } + if runID, _ := step.With["run_id"].(string); runID != "scenario-frozen-trino-perf-20260102t030405z" { + t.Fatalf("perf run_id = %q, want scenario run id", runID) + } + if _, ok := step.With["flight_addr"]; ok { + t.Fatal("frozen perf scenario should not configure the deprecated Flight endpoint") + } + assertPerfQueryErrorsFailStep(t, step) + assertFrozenPerfTargets(t, step) + if _, ok := step.With["trino_endpoint"]; ok { + t.Fatal("frozen perf scenario must obtain the Trino endpoint from the lifecycle state, not YAML") + } + } + if !foundPerf { + t.Fatal("expected frozen perf scenario to include a perf_queries step") + } +} + +func assertFrozenPerfTargets(t *testing.T, step core.Step) { + t.Helper() + targets, ok := step.With["targets"].([]any) + if !ok || len(targets) != 2 || targets[0] != "pgwire" || targets[1] != "trino" { + t.Fatalf("perf targets = %#v, want [pgwire trino]", step.With["targets"]) + } +} + +func TestFrozenTrinoPerfScenarioOrdersTrinoLifecycleAroundThePerfStep(t *testing.T) { + t.Setenv("DUCKGRES_SCENARIO_FROZEN_S3_URI", "s3://example-frozen/frozen_v1/") + t.Setenv("DUCKGRES_SCENARIO_ORG_ID", "ci-pr-123-cnpg") + + scenario, _, err := loadScenarioForRun(filepath.Join("scenarios", "posthog_frozen_trino_perf.yaml")) + if err != nil { + t.Fatalf("load frozen perf scenario: %v", err) + } + resolved, err := resolveRunTemplates(scenario, "scenario-frozen-trino-perf-20260102t030405z") + if err != nil { + t.Fatalf("resolve templates: %v", err) + } + steps := make(map[string]core.Step, len(resolved.Steps)) for _, step := range resolved.Steps { steps[step.ID] = step @@ -529,8 +646,29 @@ func TestFrozenPerfScenarioBuildsAndValidatesPostHogTablesBeforePerf(t *testing. if got := validation.DependsOn; len(got) != 1 || got[0] != "setup_posthog_tables" { t.Fatalf("posthog validation dependencies = %#v, want [setup_posthog_tables]", got) } - if got := steps["perf_queries"].DependsOn; len(got) != 1 || got[0] != "validate_posthog_tables" { - t.Fatalf("perf dependencies = %#v, want [validate_posthog_tables]", got) + provisionTrino, ok := steps["provision_trino"] + if !ok || provisionTrino.Type != scenariotrino.StepTypeProvisionTrino { + t.Fatal("expected Trino provisioning step") + } + if got := provisionTrino.DependsOn; len(got) != 1 || got[0] != "validate_posthog_tables" { + t.Fatalf("Trino provisioning dependencies = %#v, want [validate_posthog_tables]", got) + } + request, _ := provisionTrino.With["request"].(map[string]any) + if workers, ok := request["workers"].(int); !ok || workers != 4 { + t.Fatalf("Trino workers = %#v, want 4", request["workers"]) + } + if got := steps["wait_trino_ready"].DependsOn; len(got) != 1 || got[0] != "provision_trino" { + t.Fatalf("Trino readiness dependencies = %#v, want [provision_trino]", got) + } + if got := steps["perf_queries"].DependsOn; len(got) != 1 || got[0] != "wait_trino_ready" { + t.Fatalf("perf dependencies = %#v, want [wait_trino_ready]", got) + } + teardownTrino, ok := steps["deprovision_trino"] + if !ok || !teardownTrino.AlwaysRun { + t.Fatal("expected always-run Trino teardown") + } + if got := steps["deprovision"].DependsOn; len(got) != 1 || got[0] != "deprovision_trino" { + t.Fatalf("warehouse teardown dependencies = %#v, want [deprovision_trino]", got) } } @@ -853,6 +991,7 @@ type dispatchExecutor struct { sql *scenariosql.Executor perf *scenarioperf.Executor dbt *scenariodbt.Executor + trino *scenariotrino.Executor } func (e dispatchExecutor) ExecuteStep(ctx context.Context, step core.Step) error { @@ -865,6 +1004,8 @@ func (e dispatchExecutor) ExecuteStep(ctx context.Context, step core.Step) error return e.perf.ExecuteStep(ctx, step) case scenariodbt.StepTypeDBTRun: return e.dbt.ExecuteStep(ctx, step) + case scenariotrino.StepTypeProvisionTrino, scenariotrino.StepTypeWaitTrinoReady, scenariotrino.StepTypeDeprovisionTrino: + return e.trino.ExecuteStep(ctx, step) default: return fmt.Errorf("unsupported scenario step type %q", step.Type) } @@ -887,6 +1028,8 @@ func dispatchSupports(stepType string) bool { return true case scenariodbt.StepTypeDBTRun: return true + case scenariotrino.StepTypeProvisionTrino, scenariotrino.StepTypeWaitTrinoReady, scenariotrino.StepTypeDeprovisionTrino: + return true default: return false } diff --git a/tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml b/tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml new file mode 100644 index 00000000..03c22c32 --- /dev/null +++ b/tests/mw-dev/scenario/scenarios/posthog_frozen_trino_perf.yaml @@ -0,0 +1,106 @@ +name: posthog-frozen-trino-perf +run_id_prefix: scenario-frozen-trino-perf +required_env: + - DUCKGRES_SCENARIO_ORG_ID + - DUCKGRES_SCENARIO_FROZEN_S3_URI +steps: + - id: provision + type: provision_warehouse + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + request: + database_name: scenario_frozen_trino_perf_${run_id_token} + team_id: 1 + metadata_store: + type: cnpg-shard + ducklake: + enabled: true + data_store: + type: s3bucket + + - id: wait_ready + type: wait_warehouse_ready + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + timeout: 15m + poll_interval: 10s + + - id: setup_frozen_views + type: sql + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + catalog: ducklake + file: ../sql/setup_frozen_views.sql + max_attempts: 12 + retry_interval: 10s + + - id: setup_posthog_tables + type: sql + depends_on: [setup_frozen_views] + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + catalog: ducklake + file: ../sql/setup_posthog_tables.sql + max_attempts: 3 + retry_interval: 10s + + - id: validate_posthog_tables + type: sql + depends_on: [setup_posthog_tables] + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + catalog: ducklake + file: ../sql/validate_posthog_tables.sql + max_attempts: 3 + retry_interval: 10s + + - id: provision_trino + type: provision_trino + depends_on: [validate_posthog_tables] + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + request: + workers: 4 + + - id: wait_trino_ready + type: wait_trino_ready + depends_on: [provision_trino] + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + timeout: 15m + poll_interval: 10s + + - id: perf_queries + type: perf_queries + depends_on: [wait_trino_ready] + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + catalog: ducklake + catalog_file: ../../../perf/queries/ducklake_posthog_tables.yaml + targets: [pgwire, trino] + trino_catalog: ducklake + trino_schema: posthog + # Recorded verbatim in summary.json alongside the image reference the + # control plane reports. Set it when the Trino+Brikk image is pinned; the + # image digest is the authoritative record either way. + # trino_connector_version: "483-0.2.0" + run_id: ${run_id} + dataset_version: posthog-file-views-v1 + fail_on_query_errors: true + + - id: deprovision_trino + type: deprovision_trino + depends_on: [perf_queries] + always_run: true + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + + - id: deprovision + type: deprovision_warehouse + depends_on: [deprovision_trino] + always_run: true + with: + org_id: ${env:DUCKGRES_SCENARIO_ORG_ID} + verify_deleted: true + cleanup_timeout: 15m + poll_interval: 10s diff --git a/tests/mw-dev/scenario/script_test.go b/tests/mw-dev/scenario/script_test.go index de9b5d83..0ec973f2 100644 --- a/tests/mw-dev/scenario/script_test.go +++ b/tests/mw-dev/scenario/script_test.go @@ -219,3 +219,36 @@ func TestScenarioWorkflowsUseNode24Actions(t *testing.T) { } } } + +// The dev scenario workflow can hand the control plane a PINNED Trino+Brikk +// image so the paired comparison scenario has a lifecycle to call. It must +// never hand it reader credentials: the metadata reader password and the +// read-only S3 role are charts-created cluster resources the control plane +// resolves for itself. +func TestDevScenarioWorkflowPassesOnlyThePinnedTrinoImage(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("..", "..", "..", ".github", "workflows", "scenario-dev.yml")) + if err != nil { + t.Fatalf("read dev scenario workflow: %v", err) + } + workflow := string(raw) + + for _, required := range []string{ + "trino_benchmark_image:", + "DUCKGRES_TRINO_BENCHMARK_IMAGE: ${{ inputs.trino_benchmark_image || '' }}", + } { + if !strings.Contains(workflow, required) { + t.Fatalf("workflow missing %q", required) + } + } + for _, forbidden := range []string{ + "DUCKGRES_TRINO_BENCHMARK_ENABLED: true", + "TRINO_DUCKLAKE_DB_PASSWORD", + "TRINO_READER_PASSWORD", + "trino_reader_password", + "s3.aws-secret-key", + } { + if strings.Contains(workflow, forbidden) { + t.Fatalf("workflow contains Trino credential or unconditional enable %q", forbidden) + } + } +} diff --git a/tests/mw-dev/scenario/trino/client.go b/tests/mw-dev/scenario/trino/client.go new file mode 100644 index 00000000..243d85e9 --- /dev/null +++ b/tests/mw-dev/scenario/trino/client.go @@ -0,0 +1,195 @@ +package trino + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + "time" +) + +// Client is the scenario runner's non-secret adapter to the control-plane Trino +// benchmark lifecycle. The API returns only a cluster ID, a lifecycle state, an +// in-cluster endpoint, worker counts, and the pinned image reference; no +// credential material crosses this boundary, so nothing the client stores or +// logs can leak one. +type Client struct { + baseURL string + internalSecret string + httpClient *http.Client +} + +type ClientConfig struct { + BaseURL string + InternalSecret string + HTTPClient *http.Client +} + +// Default polling controls. The scenario YAML normally sets its own; these keep +// a step that omits them from either hammering the API or hanging forever. +const ( + defaultWaitPollInterval = 10 * time.Second + defaultWaitTimeout = 15 * time.Minute +) + +// ErrClusterFailed is the TERMINAL lifecycle outcome. pending is a polling +// state; failed is not, and the wait must stop on it rather than burn the +// scenario's whole readiness budget. +var ErrClusterFailed = errors.New("trino benchmark cluster failed") + +// Client is the production Lifecycle the scenario runner uses. +var _ Lifecycle = (*Client)(nil) + +func NewClient(cfg ClientConfig) (*Client, error) { + baseURL := strings.TrimRight(strings.TrimSpace(cfg.BaseURL), "/") + if baseURL == "" { + return nil, fmt.Errorf("trino lifecycle API base URL is required") + } + if _, err := url.ParseRequestURI(baseURL); err != nil { + return nil, fmt.Errorf("parse trino lifecycle API base URL: %w", err) + } + client := cfg.HTTPClient + if client == nil { + client = http.DefaultClient + } + return &Client{baseURL: baseURL, internalSecret: cfg.InternalSecret, httpClient: client}, nil +} + +func (c *Client) ProvisionTrino(ctx context.Context, request ProvisionRequest) (Cluster, error) { + var cluster Cluster + if err := c.doJSON(ctx, http.MethodPost, request.OrgID, "provision", request.Config, &cluster); err != nil { + return Cluster{}, err + } + return cluster, nil +} + +// WaitTrinoReady polls the status endpoint until the control plane reports a +// ready cluster with a usable endpoint, the cluster fails terminally, or the +// caller's budget (timeout, attempts, or context) runs out. A transient +// transport/status error is a polling state too — the control plane may still +// be converging — but the last one is reported if the wait ultimately fails. +func (c *Client) WaitTrinoReady(ctx context.Context, cluster Cluster, options WaitOptions) (Cluster, error) { + interval := options.PollInterval + if interval <= 0 { + interval = defaultWaitPollInterval + } + timeout := options.Timeout + if timeout <= 0 { + timeout = defaultWaitTimeout + } + deadlineCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + var ( + attempts int + lastErr error + lastState string + ) + for { + if options.MaxAttempts > 0 && attempts >= options.MaxAttempts { + return Cluster{}, waitExhaustedError(cluster.ID, attempts, lastState, lastErr, + fmt.Sprintf("after %d attempt(s)", attempts)) + } + attempts++ + + var status Cluster + err := c.doJSON(deadlineCtx, http.MethodGet, "", "status/"+url.PathEscape(cluster.ID), nil, &status) + switch { + case err != nil: + // A failure caused purely by the expiring budget would overwrite + // the real reason polling never succeeded, so keep the earlier one. + if deadlineCtx.Err() == nil || lastErr == nil { + lastErr = err + } + default: + lastErr = nil + lastState = status.State + if status.State == StateFailed { + // Terminal: no amount of further polling changes this. + return Cluster{}, fmt.Errorf("%w: cluster %s reported state %q", ErrClusterFailed, cluster.ID, status.State) + } + // A ready cluster without an endpoint is not usable yet, so it + // stays a polling state rather than a false success. + if status.State == StateReady && status.Endpoint != "" { + if status.ID == "" { + status.ID = cluster.ID + } + return status, nil + } + } + + select { + case <-deadlineCtx.Done(): + return Cluster{}, waitExhaustedError(cluster.ID, attempts, lastState, lastErr, + fmt.Sprintf("within %s", timeout)) + case <-time.After(interval): + } + } +} + +func waitExhaustedError(clusterID string, attempts int, lastState string, lastErr error, budget string) error { + state := lastState + if state == "" { + state = "unknown" + } + if lastErr != nil { + return fmt.Errorf("trino benchmark cluster %s did not become ready %s (%d attempt(s), last state %q): %w", + clusterID, budget, attempts, state, lastErr) + } + return fmt.Errorf("trino benchmark cluster %s did not become ready %s (%d attempt(s), last state %q)", + clusterID, budget, attempts, state) +} + +func (c *Client) DeprovisionTrino(ctx context.Context, cluster Cluster) error { + return c.doJSON(ctx, http.MethodPost, "", "deprovision/"+url.PathEscape(cluster.ID), nil, nil) +} + +func (c *Client) doJSON(ctx context.Context, method, orgID, action string, body, out any) error { + path := "/api/v1/trino-benchmarks" + if orgID != "" { + path += "/orgs/" + url.PathEscape(orgID) + } + path += "/" + action + var reader *bytes.Reader + if body != nil { + raw, err := json.Marshal(body) + if err != nil { + return fmt.Errorf("encode trino lifecycle request: %w", err) + } + reader = bytes.NewReader(raw) + } else { + reader = bytes.NewReader(nil) + } + req, err := http.NewRequestWithContext(ctx, method, c.baseURL+path, reader) + if err != nil { + return fmt.Errorf("create trino lifecycle request: %w", err) + } + req.Header.Set("Accept", "application/json") + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + if c.internalSecret != "" { + req.Header.Set("X-Duckgres-Internal-Secret", c.internalSecret) + } + resp, err := c.httpClient.Do(req) + if err != nil { + return fmt.Errorf("execute trino lifecycle request: %w", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + // Report method, path, and status only. The control plane already + // sanitizes its own error bodies, and the request headers (which carry + // the internal secret) are never part of this message. + return fmt.Errorf("trino lifecycle request %s %s returned %s", method, path, resp.Status) + } + if out != nil { + if err := json.NewDecoder(resp.Body).Decode(out); err != nil { + return fmt.Errorf("decode trino lifecycle response: %w", err) + } + } + return nil +} diff --git a/tests/mw-dev/scenario/trino/client_test.go b/tests/mw-dev/scenario/trino/client_test.go new file mode 100644 index 00000000..5b6f3026 --- /dev/null +++ b/tests/mw-dev/scenario/trino/client_test.go @@ -0,0 +1,264 @@ +package trino + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" +) + +func newTestClient(t *testing.T, handler http.HandlerFunc) (*Client, *httptest.Server) { + t.Helper() + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + client, err := NewClient(ClientConfig{ + BaseURL: server.URL, + InternalSecret: "internal-secret", + HTTPClient: server.Client(), + }) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + return client, server +} + +func TestClientProvisionPostsRequestWithInternalSecret(t *testing.T) { + var ( + mu sync.Mutex + path string + secret string + body map[string]any + ) + client, _ := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + path = r.URL.Path + secret = r.Header.Get("X-Duckgres-Internal-Secret") + _ = json.NewDecoder(r.Body).Decode(&body) + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"pending"}`)) + }) + + cluster, err := client.ProvisionTrino(context.Background(), ProvisionRequest{ + OrgID: "bench-org", + Config: map[string]any{"workers": 4}, + }) + if err != nil { + t.Fatalf("ProvisionTrino: %v", err) + } + if cluster.ID != "trino-bench-bench-org" || cluster.State != StatePending { + t.Fatalf("cluster = %+v", cluster) + } + if path != "/api/v1/trino-benchmarks/orgs/bench-org/provision" { + t.Fatalf("path = %q", path) + } + if secret != "internal-secret" { + t.Fatalf("internal secret header = %q", secret) + } + if workers, ok := body["workers"].(float64); !ok || workers != 4 { + t.Fatalf("request body = %#v", body) + } +} + +// The lifecycle client must actually POLL: a single status request would report +// "not ready" for every cluster that has not finished converging. +func TestClientWaitPollsUntilEveryWorkerIsReady(t *testing.T) { + var mu sync.Mutex + requests := 0 + client, _ := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + requests++ + attempt := requests + mu.Unlock() + if r.Method != http.MethodGet { + t.Errorf("status request method = %s, want GET", r.Method) + } + switch attempt { + case 1: + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"pending","ready_workers":0,"requested_workers":4}`)) + case 2: + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"pending","ready_workers":3,"requested_workers":4}`)) + default: + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"ready","endpoint":"http://trino:8080","ready_workers":4,"requested_workers":4}`)) + } + }) + + ready, err := client.WaitTrinoReady(context.Background(), Cluster{ID: "trino-bench-bench-org"}, WaitOptions{ + PollInterval: time.Millisecond, + Timeout: 5 * time.Second, + }) + if err != nil { + t.Fatalf("WaitTrinoReady: %v", err) + } + if ready.Endpoint != "http://trino:8080" || ready.State != StateReady { + t.Fatalf("ready cluster = %+v", ready) + } + if ready.ReadyWorkers != 4 || ready.RequestedWorkers != 4 { + t.Fatalf("worker counts = %d/%d", ready.ReadyWorkers, ready.RequestedWorkers) + } + mu.Lock() + defer mu.Unlock() + if requests < 3 { + t.Fatalf("status requests = %d, want the client to keep polling", requests) + } +} + +func TestClientWaitStopsImmediatelyOnTerminalFailure(t *testing.T) { + var mu sync.Mutex + requests := 0 + client, _ := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + requests++ + mu.Unlock() + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"failed"}`)) + }) + + _, err := client.WaitTrinoReady(context.Background(), Cluster{ID: "trino-bench-bench-org"}, WaitOptions{ + PollInterval: time.Millisecond, + Timeout: 5 * time.Second, + }) + if !errors.Is(err, ErrClusterFailed) { + t.Fatalf("error = %v, want ErrClusterFailed", err) + } + mu.Lock() + defer mu.Unlock() + if requests != 1 { + t.Fatalf("status requests = %d, want the poller to stop at the terminal state", requests) + } +} + +func TestClientWaitHonoursMaxAttempts(t *testing.T) { + var mu sync.Mutex + requests := 0 + client, _ := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + requests++ + mu.Unlock() + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"pending"}`)) + }) + + _, err := client.WaitTrinoReady(context.Background(), Cluster{ID: "trino-bench-bench-org"}, WaitOptions{ + PollInterval: time.Millisecond, + Timeout: time.Minute, + MaxAttempts: 3, + }) + if err == nil { + t.Fatal("expected the attempt budget to be exhausted") + } + if errors.Is(err, ErrClusterFailed) { + t.Fatalf("error = %v, want an exhaustion error rather than a terminal failure", err) + } + mu.Lock() + defer mu.Unlock() + if requests != 3 { + t.Fatalf("status requests = %d, want exactly the 3 allowed attempts", requests) + } +} + +func TestClientWaitHonoursTimeout(t *testing.T) { + client, _ := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"pending"}`)) + }) + + started := time.Now() + _, err := client.WaitTrinoReady(context.Background(), Cluster{ID: "trino-bench-bench-org"}, WaitOptions{ + PollInterval: 5 * time.Millisecond, + Timeout: 60 * time.Millisecond, + }) + if err == nil { + t.Fatal("expected the wait to time out") + } + if elapsed := time.Since(started); elapsed > 5*time.Second { + t.Fatalf("wait took %s, want it bounded by the timeout", elapsed) + } + if !strings.Contains(err.Error(), "trino-bench-bench-org") { + t.Fatalf("timeout error %q should name the cluster", err) + } +} + +// A ready state with no endpoint is not usable, so it stays a polling state. +func TestClientWaitKeepsPollingWhenReadyClusterHasNoEndpoint(t *testing.T) { + var mu sync.Mutex + requests := 0 + client, _ := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + requests++ + attempt := requests + mu.Unlock() + if attempt == 1 { + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"ready"}`)) + return + } + _, _ = w.Write([]byte(`{"id":"trino-bench-bench-org","state":"ready","endpoint":"http://trino:8080"}`)) + }) + + ready, err := client.WaitTrinoReady(context.Background(), Cluster{ID: "trino-bench-bench-org"}, WaitOptions{ + PollInterval: time.Millisecond, + Timeout: 5 * time.Second, + }) + if err != nil { + t.Fatalf("WaitTrinoReady: %v", err) + } + if ready.Endpoint != "http://trino:8080" { + t.Fatalf("endpoint = %q", ready.Endpoint) + } +} + +// A transient 5xx must not end the wait — the control plane may still be +// converging — but it must be reported if the wait ultimately fails. +func TestClientWaitRetriesTransientErrorsAndReportsTheLastOne(t *testing.T) { + client, _ := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + }) + + _, err := client.WaitTrinoReady(context.Background(), Cluster{ID: "trino-bench-bench-org"}, WaitOptions{ + PollInterval: time.Millisecond, + Timeout: 50 * time.Millisecond, + }) + if err == nil { + t.Fatal("expected the wait to fail") + } + if !strings.Contains(err.Error(), "502") { + t.Fatalf("error %q should carry the last transport failure", err) + } +} + +func TestClientDeprovisionAcceptsNoContent(t *testing.T) { + var mu sync.Mutex + var path, method string + client, _ := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + path, method = r.URL.Path, r.Method + mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }) + + if err := client.DeprovisionTrino(context.Background(), Cluster{ID: "trino-bench-bench-org"}); err != nil { + t.Fatalf("DeprovisionTrino: %v", err) + } + mu.Lock() + defer mu.Unlock() + if method != http.MethodPost || path != "/api/v1/trino-benchmarks/deprovision/trino-bench-bench-org" { + t.Fatalf("request = %s %s", method, path) + } +} + +func TestClientErrorsNeverEchoTheInternalSecret(t *testing.T) { + client, _ := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"provision failed"}`)) + }) + + _, err := client.ProvisionTrino(context.Background(), ProvisionRequest{OrgID: "bench-org", Config: map[string]any{}}) + if err == nil { + t.Fatal("expected an error") + } + if strings.Contains(err.Error(), "internal-secret") { + t.Fatalf("error %q leaked the internal secret", err) + } +} diff --git a/tests/mw-dev/scenario/trino/steps.go b/tests/mw-dev/scenario/trino/steps.go new file mode 100644 index 00000000..f307985a --- /dev/null +++ b/tests/mw-dev/scenario/trino/steps.go @@ -0,0 +1,306 @@ +// Package trino defines scenario lifecycle contracts for a benchmark Trino +// cluster. The concrete control-plane/Kubernetes implementation is injected as +// a Lifecycle; this package does not create infrastructure itself. +package trino + +import ( + "context" + "fmt" + "strconv" + "sync" + "time" + + "github.com/posthog/duckgres/tests/mw-dev/scenario/core" +) + +const ( + StepTypeProvisionTrino = "provision_trino" + StepTypeWaitTrinoReady = "wait_trino_ready" + StepTypeDeprovisionTrino = "deprovision_trino" + + ErrorClassConfig = "configuration_error" + ErrorClassInvalidStepConfig = "invalid_step_config" + ErrorClassLifecycle = "trino_lifecycle_error" + ErrorClassCleanup = "trino_cleanup_error" + ErrorClassUnsupportedStep = "unsupported_step" +) + +// ProvisionRequest identifies the warehouse whose DuckLake tables Trino will +// read. Config is deliberately opaque to the scenario runner: it is passed to +// the control-plane implementation and must never contain credentials. +type ProvisionRequest struct { + OrgID string + Config map[string]any +} + +// Lifecycle states reported by the control-plane benchmark API. pending is a +// polling state; failed is terminal. +const ( + StatePending = "pending" + StateReady = "ready" + StateFailed = "failed" +) + +// Cluster is the non-secret state a subsequent Trino query executor needs, plus +// the comparison metadata the perf artifact records. It never carries +// credentials: the control plane owns the metadata reader password and the +// read-only S3 identity and returns neither. +type Cluster struct { + ID string `json:"id"` + State string `json:"state,omitempty"` + Endpoint string `json:"endpoint,omitempty"` + // RequestedWorkers / ReadyWorkers make a run's topology auditable from the + // artifact rather than assumed. + RequestedWorkers int `json:"requested_workers,omitempty"` + ReadyWorkers int `json:"ready_workers,omitempty"` + // Image is the pinned Trino+Brikk image reference (digest where pinned). + Image string `json:"image,omitempty"` +} + +// WaitOptions are lifecycle polling controls. The HTTP client honours all +// three: it polls at PollInterval until Timeout or MaxAttempts is reached +// rather than issuing a single status request. +type WaitOptions struct { + PollInterval time.Duration + Timeout time.Duration + MaxAttempts int +} + +// Lifecycle is implemented by Client, the control-plane-backed Trino +// provisioner. Keeping it narrow lets scenario contracts be tested without +// Kubernetes credentials or a live control plane. +type Lifecycle interface { + ProvisionTrino(context.Context, ProvisionRequest) (Cluster, error) + WaitTrinoReady(context.Context, Cluster, WaitOptions) (Cluster, error) + DeprovisionTrino(context.Context, Cluster) error +} + +type ExecutorConfig struct { + Lifecycle Lifecycle + State *State + WaitOptions WaitOptions +} + +type Executor struct { + lifecycle Lifecycle + state *State + waitOptions WaitOptions +} + +// State intentionally contains only non-secret cluster identity and endpoint. +type State struct { + mu sync.Mutex + clusters map[string]Cluster +} + +func NewState() *State { + return &State{clusters: make(map[string]Cluster)} +} + +func (s *State) StoreCluster(orgID string, cluster Cluster) { + s.mu.Lock() + defer s.mu.Unlock() + s.clusters[orgID] = cluster +} + +func (s *State) Cluster(orgID string) (Cluster, bool) { + s.mu.Lock() + defer s.mu.Unlock() + cluster, ok := s.clusters[orgID] + return cluster, ok +} + +func (s *State) DeleteCluster(orgID string) { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.clusters, orgID) +} + +func NewExecutor(cfg ExecutorConfig) *Executor { + state := cfg.State + if state == nil { + state = NewState() + } + return &Executor{lifecycle: cfg.Lifecycle, state: state, waitOptions: cfg.WaitOptions} +} + +func (e *Executor) ExecuteStep(ctx context.Context, step core.Step) error { + if e.lifecycle == nil { + return classified(ErrorClassConfig, fmt.Errorf("trino lifecycle is required")) + } + switch step.Type { + case StepTypeProvisionTrino: + return e.provision(ctx, step) + case StepTypeWaitTrinoReady: + return e.waitReady(ctx, step) + case StepTypeDeprovisionTrino: + return e.deprovision(ctx, step) + default: + return classified(ErrorClassUnsupportedStep, fmt.Errorf("unsupported trino step type %q", step.Type)) + } +} + +func (e *Executor) provision(ctx context.Context, step core.Step) error { + orgID, err := requiredString(step, "org_id") + if err != nil { + return err + } + request, err := requiredMap(step, "request") + if err != nil { + return err + } + cluster, err := e.lifecycle.ProvisionTrino(ctx, ProvisionRequest{OrgID: orgID, Config: request}) + if err != nil { + return classified(ErrorClassLifecycle, err) + } + if cluster.ID == "" { + return classified(ErrorClassLifecycle, fmt.Errorf("step %s provisioned Trino cluster without an ID", step.ID)) + } + e.state.StoreCluster(orgID, cluster) + return nil +} + +func (e *Executor) waitReady(ctx context.Context, step core.Step) error { + orgID, err := requiredString(step, "org_id") + if err != nil { + return err + } + cluster, ok := e.state.Cluster(orgID) + if !ok { + return classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s requires a provisioned Trino cluster for org %q", step.ID, orgID)) + } + opts, err := e.waitOptionsForStep(step) + if err != nil { + return err + } + ready, err := e.lifecycle.WaitTrinoReady(ctx, cluster, opts) + if err != nil { + return classified(ErrorClassLifecycle, err) + } + if ready.ID == "" { + ready.ID = cluster.ID + } + if ready.ID != cluster.ID { + return classified(ErrorClassLifecycle, fmt.Errorf("step %s ready Trino cluster ID %q does not match provisioned cluster %q", step.ID, ready.ID, cluster.ID)) + } + if ready.Endpoint == "" { + return classified(ErrorClassLifecycle, fmt.Errorf("step %s ready Trino cluster %q has no endpoint", step.ID, ready.ID)) + } + e.state.StoreCluster(orgID, ready) + return nil +} + +func (e *Executor) deprovision(ctx context.Context, step core.Step) error { + orgID, err := requiredString(step, "org_id") + if err != nil { + return err + } + cluster, ok := e.state.Cluster(orgID) + if !ok { + // Cleanup steps are always_run and must be safe after partial provision. + return nil + } + if err := e.lifecycle.DeprovisionTrino(ctx, cluster); err != nil { + return classified(ErrorClassCleanup, err) + } + e.state.DeleteCluster(orgID) + return nil +} + +func (e *Executor) waitOptionsForStep(step core.Step) (WaitOptions, error) { + opts := e.waitOptions + if timeout, ok, err := durationFromWith(step, "timeout"); err != nil { + return WaitOptions{}, err + } else if ok { + opts.Timeout = timeout + } + if interval, ok, err := durationFromWith(step, "poll_interval"); err != nil { + return WaitOptions{}, err + } else if ok { + opts.PollInterval = interval + } + if maxAttempts, ok, err := intFromWith(step, "max_attempts"); err != nil { + return WaitOptions{}, err + } else if ok { + opts.MaxAttempts = maxAttempts + } + return opts, nil +} + +func requiredString(step core.Step, key string) (string, error) { + value, ok := step.With[key] + if !ok { + return "", classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s requires with.%s", step.ID, key)) + } + text, ok := value.(string) + if !ok || text == "" { + return "", classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must be a non-empty string", step.ID, key)) + } + return text, nil +} + +func requiredMap(step core.Step, key string) (map[string]any, error) { + value, ok := step.With[key] + if !ok { + return nil, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s requires with.%s", step.ID, key)) + } + result, ok := value.(map[string]any) + if !ok { + return nil, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must be a map", step.ID, key)) + } + return result, nil +} + +func durationFromWith(step core.Step, key string) (time.Duration, bool, error) { + value, ok := step.With[key] + if !ok { + return 0, false, nil + } + text, ok := value.(string) + if !ok { + return 0, false, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must be a Go duration", step.ID, key)) + } + duration, err := time.ParseDuration(text) + if err != nil || duration < 0 { + return 0, false, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must be a non-negative Go duration", step.ID, key)) + } + return duration, true, nil +} + +func intFromWith(step core.Step, key string) (int, bool, error) { + value, ok := step.With[key] + if !ok { + return 0, false, nil + } + switch value := value.(type) { + case int: + if value < 0 { + return 0, false, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must not be negative", step.ID, key)) + } + return value, true, nil + case string: + parsed, err := strconv.Atoi(value) + if err != nil || parsed < 0 { + return 0, false, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must be a non-negative integer", step.ID, key)) + } + return parsed, true, nil + default: + return 0, false, classified(ErrorClassInvalidStepConfig, fmt.Errorf("step %s with.%s must be a non-negative integer", step.ID, key)) + } +} + +type classifiedError struct { + class string + err error +} + +func (e classifiedError) Error() string { return e.err.Error() } +func (e classifiedError) Unwrap() error { return e.err } +func (e classifiedError) ErrorClass() string { return e.class } + +func classified(class string, err error) error { + if err == nil { + return nil + } + return classifiedError{class: class, err: err} +} diff --git a/tests/mw-dev/scenario/trino/steps_test.go b/tests/mw-dev/scenario/trino/steps_test.go new file mode 100644 index 00000000..220264e2 --- /dev/null +++ b/tests/mw-dev/scenario/trino/steps_test.go @@ -0,0 +1,192 @@ +package trino + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/posthog/duckgres/tests/mw-dev/scenario/core" +) + +func TestExecutorProvisionWaitAndDeprovisionCluster(t *testing.T) { + lifecycle := &recordingLifecycle{ + provisioned: Cluster{ID: "trino-run-123"}, + ready: Cluster{ID: "trino-run-123", Endpoint: "http://trino.example:8080"}, + } + state := NewState() + executor := NewExecutor(ExecutorConfig{ + Lifecycle: lifecycle, + State: state, + WaitOptions: WaitOptions{ + Timeout: time.Minute, + PollInterval: 5 * time.Second, + }, + }) + + provision := core.Step{ + ID: "provision_trino", + Type: StepTypeProvisionTrino, + With: map[string]any{ + "org_id": "benchmark-org", + "request": map[string]any{ + "workers": 4, + "image": "registry.example/trino@sha256:abc", + }, + }, + } + if err := executor.ExecuteStep(context.Background(), provision); err != nil { + t.Fatalf("provision: %v", err) + } + if lifecycle.provisionRequest.OrgID != "benchmark-org" { + t.Fatalf("provision org = %q", lifecycle.provisionRequest.OrgID) + } + if workers, ok := lifecycle.provisionRequest.Config["workers"].(int); !ok || workers != 4 { + t.Fatalf("provision request = %#v", lifecycle.provisionRequest.Config) + } + + wait := core.Step{ + ID: "wait_trino_ready", + Type: StepTypeWaitTrinoReady, + With: map[string]any{ + "org_id": "benchmark-org", + "timeout": "2m", + "poll_interval": "2s", + "max_attempts": 3, + }, + } + if err := executor.ExecuteStep(context.Background(), wait); err != nil { + t.Fatalf("wait: %v", err) + } + if lifecycle.waitCluster.ID != "trino-run-123" { + t.Fatalf("wait cluster = %#v", lifecycle.waitCluster) + } + if lifecycle.waitOptions.Timeout != 2*time.Minute || lifecycle.waitOptions.PollInterval != 2*time.Second || lifecycle.waitOptions.MaxAttempts != 3 { + t.Fatalf("wait options = %#v", lifecycle.waitOptions) + } + cluster, ok := state.Cluster("benchmark-org") + if !ok || cluster.Endpoint != "http://trino.example:8080" { + t.Fatalf("stored ready cluster = %#v, present=%t", cluster, ok) + } + + deprovision := core.Step{ + ID: "deprovision_trino", + Type: StepTypeDeprovisionTrino, + With: map[string]any{ + "org_id": "benchmark-org", + }, + } + if err := executor.ExecuteStep(context.Background(), deprovision); err != nil { + t.Fatalf("deprovision: %v", err) + } + if lifecycle.deprovisionCluster.ID != "trino-run-123" { + t.Fatalf("deprovision cluster = %#v", lifecycle.deprovisionCluster) + } +} + +func TestExecutorDeprovisionIsNoopWhenProvisionDidNotReturnCluster(t *testing.T) { + lifecycle := &recordingLifecycle{} + executor := NewExecutor(ExecutorConfig{Lifecycle: lifecycle}) + + err := executor.ExecuteStep(context.Background(), core.Step{ + ID: "deprovision_trino", + Type: StepTypeDeprovisionTrino, + With: map[string]any{ + "org_id": "benchmark-org", + }, + }) + if err != nil { + t.Fatalf("deprovision: %v", err) + } + if lifecycle.deprovisionCalls != 0 { + t.Fatalf("deprovision calls = %d, want 0", lifecycle.deprovisionCalls) + } +} + +func TestExecutorRejectsInvalidLifecycleStepConfig(t *testing.T) { + executor := NewExecutor(ExecutorConfig{Lifecycle: &recordingLifecycle{}}) + for _, step := range []core.Step{ + {ID: "missing-request", Type: StepTypeProvisionTrino, With: map[string]any{"org_id": "benchmark-org"}}, + {ID: "missing-cluster", Type: StepTypeWaitTrinoReady, With: map[string]any{"org_id": "benchmark-org"}}, + {ID: "missing-org", Type: StepTypeDeprovisionTrino, With: map[string]any{}}, + } { + t.Run(step.ID, func(t *testing.T) { + err := executor.ExecuteStep(context.Background(), step) + if err == nil { + t.Fatal("expected invalid configuration error") + } + var classified core.ClassifiedError + if !errors.As(err, &classified) { + t.Fatalf("error = %T %v, want classified", err, err) + } + if classified.ErrorClass() != ErrorClassInvalidStepConfig { + t.Fatalf("error class = %q, want %q", classified.ErrorClass(), ErrorClassInvalidStepConfig) + } + }) + } + + state := NewState() + state.StoreCluster("benchmark-org", Cluster{ID: "trino-run-123"}) + err := NewExecutor(ExecutorConfig{Lifecycle: &recordingLifecycle{}, State: state}).ExecuteStep(context.Background(), core.Step{ + ID: "bad-timeout", + Type: StepTypeWaitTrinoReady, + With: map[string]any{"org_id": "benchmark-org", "timeout": "not-a-duration"}, + }) + if err == nil { + t.Fatal("expected invalid timeout to fail") + } + var classified core.ClassifiedError + if !errors.As(err, &classified) || classified.ErrorClass() != ErrorClassInvalidStepConfig { + t.Fatalf("error = %T %v, want %q", err, err, ErrorClassInvalidStepConfig) + } +} + +func TestExecutorRejectsReadyClusterWithoutEndpoint(t *testing.T) { + executor := NewExecutor(ExecutorConfig{ + Lifecycle: &recordingLifecycle{ + provisioned: Cluster{ID: "trino-run-123"}, + ready: Cluster{ID: "trino-run-123"}, + }, + }) + provision := core.Step{ID: "provision", Type: StepTypeProvisionTrino, With: map[string]any{"org_id": "benchmark-org", "request": map[string]any{}}} + if err := executor.ExecuteStep(context.Background(), provision); err != nil { + t.Fatalf("provision: %v", err) + } + err := executor.ExecuteStep(context.Background(), core.Step{ID: "wait", Type: StepTypeWaitTrinoReady, With: map[string]any{"org_id": "benchmark-org"}}) + if err == nil { + t.Fatal("expected ready cluster without endpoint to fail") + } + var classified core.ClassifiedError + if !errors.As(err, &classified) || classified.ErrorClass() != ErrorClassLifecycle { + t.Fatalf("error = %T %v, want %q", err, err, ErrorClassLifecycle) + } +} + +type recordingLifecycle struct { + provisioned Cluster + ready Cluster + err error + + provisionRequest ProvisionRequest + waitCluster Cluster + waitOptions WaitOptions + deprovisionCluster Cluster + deprovisionCalls int +} + +func (l *recordingLifecycle) ProvisionTrino(_ context.Context, request ProvisionRequest) (Cluster, error) { + l.provisionRequest = request + return l.provisioned, l.err +} + +func (l *recordingLifecycle) WaitTrinoReady(_ context.Context, cluster Cluster, options WaitOptions) (Cluster, error) { + l.waitCluster = cluster + l.waitOptions = options + return l.ready, l.err +} + +func (l *recordingLifecycle) DeprovisionTrino(_ context.Context, cluster Cluster) error { + l.deprovisionCalls++ + l.deprovisionCluster = cluster + return l.err +} diff --git a/tests/perf/core/catalog.go b/tests/perf/core/catalog.go index ec4df865..c07a7d48 100644 --- a/tests/perf/core/catalog.go +++ b/tests/perf/core/catalog.go @@ -42,7 +42,7 @@ func validateCatalog(c Catalog) error { } seenTargets := map[Protocol]struct{}{} for _, target := range c.Targets { - if target != ProtocolPGWire { + if !supportedProtocol(target) { return fmt.Errorf("unsupported target protocol %q", target) } if _, ok := seenTargets[target]; ok { @@ -65,8 +65,15 @@ func validateCatalog(c Catalog) error { if q.IntentID == "" { return fmt.Errorf("query %s missing intent_id", q.QueryID) } - if q.PGWireSQL == "" { - return fmt.Errorf("query %s missing pgwire_sql", q.QueryID) + applicable := false + for _, target := range c.Targets { + if q.SupportsProtocol(target) { + applicable = true + break + } + } + if !applicable { + return fmt.Errorf("query %s has no SQL for any catalog target", q.QueryID) } } return nil @@ -74,13 +81,33 @@ func validateCatalog(c Catalog) error { func ValidateReadOnlyCatalog(c Catalog) error { for _, q := range c.Queries { - if err := validateSelectOnlySQL("pgwire_sql", q.QueryID, q.PGWireSQL); err != nil { - return err + for _, sql := range []struct { + field string + text string + }{ + {field: "pgwire_sql", text: q.PGWireSQL}, + {field: "trino_sql", text: q.TrinoSQL}, + } { + if strings.TrimSpace(sql.text) == "" { + continue + } + if err := validateSelectOnlySQL(sql.field, q.QueryID, sql.text); err != nil { + return err + } } } return nil } +func supportedProtocol(protocol Protocol) bool { + switch protocol { + case ProtocolPGWire, ProtocolTrino: + return true + default: + return false + } +} + func validateSelectOnlySQL(field, queryID, sql string) error { trimmed := trimLeadingSQLComments(sql) trimmed = strings.TrimSpace(trimmed) diff --git a/tests/perf/core/catalog_test.go b/tests/perf/core/catalog_test.go index 9532e3f0..f3eb1ecd 100644 --- a/tests/perf/core/catalog_test.go +++ b/tests/perf/core/catalog_test.go @@ -3,6 +3,7 @@ package core import ( "os" "path/filepath" + "slices" "strings" "testing" ) @@ -23,8 +24,8 @@ func TestCheckedInCatalogsLoad(t *testing.T) { if err != nil { t.Fatalf("LoadCatalog(%s): %v", path, err) } - if len(catalog.Targets) != 1 || catalog.Targets[0] != ProtocolPGWire { - t.Fatalf("catalog targets = %v, want [pgwire]", catalog.Targets) + if !slices.Contains(catalog.Targets, ProtocolPGWire) { + t.Fatalf("catalog targets = %v, want pgwire", catalog.Targets) } raw, err := os.ReadFile(path) @@ -70,6 +71,58 @@ queries: } } +func TestParseCatalogAllowsTargetSpecificSQL(t *testing.T) { + raw := ` +name: target-specific +description: target-specific suite +seed: 7 +dataset_scale: 1 +targets: [pgwire, trino] +warmup_iterations: 0 +measure_iterations: 1 +queries: + - query_id: pgwire_only + intent_id: i1 + pgwire_sql: SELECT 1 + - query_id: shared + intent_id: i2 + pgwire_sql: SELECT 2 + trino_sql: SELECT 2 +` + catalog, err := ParseCatalog([]byte(raw)) + if err != nil { + t.Fatalf("ParseCatalog returned error: %v", err) + } + if !catalog.Queries[0].SupportsProtocol(ProtocolPGWire) { + t.Fatal("expected first query to support pgwire") + } + if catalog.Queries[0].SupportsProtocol(ProtocolTrino) { + t.Fatal("expected first query not to support trino") + } + if !catalog.Queries[1].SupportsProtocol(ProtocolTrino) { + t.Fatal("expected second query to support trino") + } +} + +func TestPostHogDuckLakeCatalogKeepsRawViewsPGWireOnly(t *testing.T) { + catalog, err := LoadCatalog(filepath.Join("..", "queries", "ducklake_posthog_tables.yaml")) + if err != nil { + t.Fatalf("LoadCatalog: %v", err) + } + if got, want := catalog.Targets, []Protocol{ProtocolPGWire, ProtocolTrino}; !slices.Equal(got, want) { + t.Fatalf("catalog targets = %v, want %v", got, want) + } + for _, query := range catalog.Queries { + isRawView := strings.Contains(query.QueryID, "__raw_view") + if isRawView && query.SupportsProtocol(ProtocolTrino) { + t.Fatalf("raw-view query %s must remain pgwire-only", query.QueryID) + } + if !isRawView && !query.SupportsProtocol(ProtocolTrino) { + t.Fatalf("DuckLake table query %s must have Trino SQL", query.QueryID) + } + } +} + func TestParseCatalogRejectsDuplicateQueryIDs(t *testing.T) { raw := ` name: bad diff --git a/tests/perf/core/environment_test.go b/tests/perf/core/environment_test.go new file mode 100644 index 00000000..fa95bcb1 --- /dev/null +++ b/tests/perf/core/environment_test.go @@ -0,0 +1,186 @@ +package core + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +type reportingDriver struct { + protocol Protocol + env ProtocolEnvironment + err error +} + +func (d reportingDriver) Protocol() Protocol { return d.protocol } + +func (d reportingDriver) Execute(context.Context, Query, []any) (ExecutionResult, error) { + return ExecutionResult{Rows: 1, Duration: time.Millisecond}, nil +} + +func (d reportingDriver) Close() error { return nil } + +func (d reportingDriver) Environment(context.Context) (ProtocolEnvironment, error) { + return d.env, d.err +} + +type silentDriver struct{ protocol Protocol } + +func (d silentDriver) Protocol() Protocol { return d.protocol } + +func (d silentDriver) Execute(context.Context, Query, []any) (ExecutionResult, error) { + return ExecutionResult{Rows: 1, Duration: time.Millisecond}, nil +} + +func (d silentDriver) Close() error { return nil } + +func environmentTestCatalog() Catalog { + return Catalog{ + Targets: []Protocol{ProtocolPGWire, ProtocolTrino}, + MeasureIterations: 1, + Queries: []Query{{ + QueryID: "q1", IntentID: "i1", + PGWireSQL: "SELECT 1", TrinoSQL: "SELECT 1", + }}, + } +} + +func TestRunnerRecordsConfiguredAndProbedEnvironments(t *testing.T) { + runner := NewQueryRunner(RunnerConfig{ + RunID: "run-1", + Catalog: environmentTestCatalog(), + Drivers: map[Protocol]ProtocolDriver{ + ProtocolPGWire: reportingDriver{protocol: ProtocolPGWire, env: ProtocolEnvironment{Engine: "duckgres", Version: "duckgres 1.2.3"}}, + ProtocolTrino: reportingDriver{protocol: ProtocolTrino, env: ProtocolEnvironment{Engine: "trino", Version: "483"}}, + }, + Environments: []ProtocolEnvironment{ + {Protocol: ProtocolPGWire, Catalog: "ducklake", TimeZone: "UTC"}, + { + Protocol: ProtocolTrino, Catalog: "ducklake", Schema: "posthog", TimeZone: "UTC", + Image: "registry.example/trino-brikk@sha256:abc", RequestedWorkers: 4, ReadyWorkers: 4, + ConnectorVersion: "483-0.2.0", + }, + }, + }) + + summary, err := runner.Run(context.Background()) + if err != nil { + t.Fatalf("Run: %v", err) + } + if len(summary.Environments) != 2 { + t.Fatalf("environments = %+v, want one per target", summary.Environments) + } + pgwire, trino := summary.Environments[0], summary.Environments[1] + if pgwire.Protocol != ProtocolPGWire || trino.Protocol != ProtocolTrino { + t.Fatalf("environments are not in catalog-target order: %+v", summary.Environments) + } + if pgwire.Version != "duckgres 1.2.3" || pgwire.Catalog != "ducklake" || pgwire.TimeZone != "UTC" { + t.Fatalf("pgwire environment = %+v", pgwire) + } + if trino.Version != "483" || trino.ConnectorVersion != "483-0.2.0" { + t.Fatalf("trino versions = %+v", trino) + } + if trino.Image != "registry.example/trino-brikk@sha256:abc" { + t.Fatalf("trino image = %q", trino.Image) + } + if trino.RequestedWorkers != 4 || trino.ReadyWorkers != 4 { + t.Fatalf("trino worker counts = %d/%d", trino.RequestedWorkers, trino.ReadyWorkers) + } + if trino.Schema != "posthog" || trino.TimeZone != "UTC" { + t.Fatalf("trino catalog identity = %+v", trino) + } +} + +// Configured values are the recorded pin; a driver may only add detail. +func TestRunnerEnvironmentPrefersConfiguredValues(t *testing.T) { + runner := NewQueryRunner(RunnerConfig{ + Catalog: Catalog{Targets: []Protocol{ProtocolTrino}, MeasureIterations: 1, + Queries: []Query{{QueryID: "q1", IntentID: "i1", TrinoSQL: "SELECT 1"}}}, + Drivers: map[Protocol]ProtocolDriver{ + ProtocolTrino: reportingDriver{protocol: ProtocolTrino, env: ProtocolEnvironment{ + Engine: "trino", Image: "some-other-image", ReadyWorkers: 1, + }}, + }, + Environments: []ProtocolEnvironment{{ + Protocol: ProtocolTrino, Image: "registry.example/trino-brikk@sha256:abc", ReadyWorkers: 4, + }}, + }) + + summary, err := runner.Run(context.Background()) + if err != nil { + t.Fatalf("Run: %v", err) + } + env := summary.Environments[0] + if env.Image != "registry.example/trino-brikk@sha256:abc" || env.ReadyWorkers != 4 { + t.Fatalf("environment = %+v, want the configured pin to win", env) + } +} + +// Comparison metadata is best-effort: a probe failure records what is known +// and never fails the benchmark. +func TestRunnerToleratesEnvironmentProbeFailures(t *testing.T) { + runner := NewQueryRunner(RunnerConfig{ + Catalog: Catalog{Targets: []Protocol{ProtocolPGWire, ProtocolTrino}, MeasureIterations: 1, + Queries: []Query{{QueryID: "q1", IntentID: "i1", PGWireSQL: "SELECT 1", TrinoSQL: "SELECT 1"}}}, + Drivers: map[Protocol]ProtocolDriver{ + ProtocolPGWire: silentDriver{protocol: ProtocolPGWire}, + ProtocolTrino: reportingDriver{protocol: ProtocolTrino, err: os.ErrDeadlineExceeded, + env: ProtocolEnvironment{Engine: "trino"}}, + }, + Environments: []ProtocolEnvironment{{Protocol: ProtocolTrino, Image: "registry.example/trino-brikk@sha256:abc"}}, + }) + + summary, err := runner.Run(context.Background()) + if err != nil { + t.Fatalf("Run: %v", err) + } + if len(summary.Environments) != 2 { + t.Fatalf("environments = %+v", summary.Environments) + } + if summary.Environments[1].Image != "registry.example/trino-brikk@sha256:abc" { + t.Fatalf("trino environment lost its configured pin: %+v", summary.Environments[1]) + } +} + +func TestArtifactSummaryCarriesEnvironmentsAndNoSecrets(t *testing.T) { + dir := t.TempDir() + sink, err := NewArtifactSink(dir) + if err != nil { + t.Fatalf("NewArtifactSink: %v", err) + } + summary := RunSummary{ + RunID: "run-1", + Environments: []ProtocolEnvironment{ + {Protocol: ProtocolPGWire, Engine: "duckgres", Catalog: "ducklake", TimeZone: "UTC"}, + { + Protocol: ProtocolTrino, Engine: "trino", Version: "483", ConnectorVersion: "483-0.2.0", + Image: "registry.example/trino-brikk@sha256:abc", RequestedWorkers: 4, ReadyWorkers: 4, + Catalog: "ducklake", Schema: "posthog", TimeZone: "UTC", + }, + }, + } + if err := sink.Close(summary, ""); err != nil { + t.Fatalf("Close: %v", err) + } + + raw, err := os.ReadFile(filepath.Join(dir, "summary.json")) + if err != nil { + t.Fatalf("read summary: %v", err) + } + var decoded RunSummary + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("decode summary: %v", err) + } + if len(decoded.Environments) != 2 { + t.Fatalf("summary environments = %+v", decoded.Environments) + } + for _, banned := range []string{"password", "secret", "aws_access", "iam", "arn:aws"} { + if strings.Contains(strings.ToLower(string(raw)), banned) { + t.Fatalf("summary.json contains %q:\n%s", banned, raw) + } + } +} diff --git a/tests/perf/core/intent_matcher.go b/tests/perf/core/intent_matcher.go index 16991fce..4404d168 100644 --- a/tests/perf/core/intent_matcher.go +++ b/tests/perf/core/intent_matcher.go @@ -1,10 +1,5 @@ package core -import ( - "fmt" - "strings" -) - type IntentMatcher struct{} func NewIntentMatcher() *IntentMatcher { @@ -12,13 +7,5 @@ func NewIntentMatcher() *IntentMatcher { } func (m *IntentMatcher) SQLFor(query Query, protocol Protocol) (string, error) { - switch protocol { - case ProtocolPGWire: - if strings.TrimSpace(query.PGWireSQL) == "" { - return "", fmt.Errorf("query %s missing pgwire_sql", query.QueryID) - } - return query.PGWireSQL, nil - default: - return "", fmt.Errorf("unknown protocol %q", protocol) - } + return query.SQLFor(protocol) } diff --git a/tests/perf/core/runner.go b/tests/perf/core/runner.go index 51c56dc3..2450aaea 100644 --- a/tests/perf/core/runner.go +++ b/tests/perf/core/runner.go @@ -20,6 +20,14 @@ type ResultSink interface { Close(summary RunSummary, serverMetrics string) error } +// EnvironmentReporter is optionally implemented by a driver that can describe +// the engine it is talking to. It is probed once per run, before any measured +// query, and any error is ignored: comparison metadata must never fail a +// benchmark. +type EnvironmentReporter interface { + Environment(ctx context.Context) (ProtocolEnvironment, error) +} + type RunnerConfig struct { RunID string Catalog Catalog @@ -29,6 +37,10 @@ type RunnerConfig struct { OnSetup func(context.Context) error OnTeardown func(context.Context) error Now func() time.Time + // Environments carries what the CALLER already knows about each protocol + // (the lifecycle-reported image and worker counts, the catalog/schema, the + // session time zone). Driver-probed detail fills the gaps. + Environments []ProtocolEnvironment } type QueryRunner struct { @@ -78,6 +90,8 @@ func (r *QueryRunner) Run(ctx context.Context) (RunSummary, error) { } } + summary.Environments = r.resolveEnvironments(ctx) + warmupIterations := r.cfg.Catalog.WarmupIterations for i := 0; i < warmupIterations; i++ { if err := r.executeIteration(ctx, false, 0, &summary); err != nil { @@ -104,6 +118,29 @@ func (r *QueryRunner) Run(ctx context.Context) (RunSummary, error) { return summary, nil } +// resolveEnvironments merges the caller-supplied comparison metadata with +// whatever each driver can report about its engine, one entry per catalog +// target in target order. +func (r *QueryRunner) resolveEnvironments(ctx context.Context) []ProtocolEnvironment { + configured := make(map[Protocol]ProtocolEnvironment, len(r.cfg.Environments)) + for _, env := range r.cfg.Environments { + configured[env.Protocol] = env + } + var environments []ProtocolEnvironment + for _, protocol := range r.cfg.Catalog.Targets { + env := configured[protocol] + env.Protocol = protocol + if reporter, ok := r.cfg.Drivers[protocol].(EnvironmentReporter); ok { + // Best-effort: a probe failure must never fail the benchmark. + if probed, err := reporter.Environment(ctx); err == nil { + env = env.Merge(probed) + } + } + environments = append(environments, env) + } + return environments +} + func (r *QueryRunner) MetricsGatherer() prometheus.Gatherer { return r.metrics.Gatherer() } @@ -112,6 +149,9 @@ func (r *QueryRunner) executeIteration(ctx context.Context, measure bool, measur for _, query := range r.cfg.Catalog.Queries { args := orderedParamValues(query.Params) for _, protocol := range r.cfg.Catalog.Targets { + if !query.SupportsProtocol(protocol) { + continue + } driver := r.cfg.Drivers[protocol] started := r.cfg.Now() result := QueryResult{ diff --git a/tests/perf/core/types.go b/tests/perf/core/types.go index 4e589f5c..d0928a86 100644 --- a/tests/perf/core/types.go +++ b/tests/perf/core/types.go @@ -1,11 +1,16 @@ package core -import "time" +import ( + "fmt" + "strings" + "time" +) type Protocol string const ( ProtocolPGWire Protocol = "pgwire" + ProtocolTrino Protocol = "trino" ) type Catalog struct { @@ -25,6 +30,32 @@ type Query struct { Tags []string `yaml:"tags"` Params map[string]any `yaml:"params"` PGWireSQL string `yaml:"pgwire_sql"` + TrinoSQL string `yaml:"trino_sql"` +} + +// SupportsProtocol reports whether a query has SQL for a protocol. A catalog +// may contain engine-specific queries, which the runner skips for other +// configured protocols. +func (q Query) SupportsProtocol(protocol Protocol) bool { + _, err := q.SQLFor(protocol) + return err == nil +} + +func (q Query) SQLFor(protocol Protocol) (string, error) { + var sql string + var field string + switch protocol { + case ProtocolPGWire: + sql, field = q.PGWireSQL, "pgwire_sql" + case ProtocolTrino: + sql, field = q.TrinoSQL, "trino_sql" + default: + return "", fmt.Errorf("unknown protocol %q", protocol) + } + if strings.TrimSpace(sql) == "" { + return "", fmt.Errorf("query %s missing %s", q.QueryID, field) + } + return sql, nil } type ExecutionResult struct { @@ -45,6 +76,65 @@ type QueryResult struct { StartedAt time.Time `json:"started_at"` } +// ProtocolEnvironment is the non-secret comparison metadata recorded per +// protocol in summary.json. Two engines' numbers are only comparable if the +// artifact says WHAT ran: engine and version, the pinned image (with digest +// where available), the topology that was actually ready, the catalog/schema +// identity, and the session time zone. Nothing here is or may become a +// credential. +type ProtocolEnvironment struct { + Protocol Protocol `json:"protocol"` + Engine string `json:"engine,omitempty"` + Version string `json:"version,omitempty"` + // ConnectorVersion identifies the storage connector (e.g. the Brikk + // DuckLake connector build) where the engine exposes one. + ConnectorVersion string `json:"connector_version,omitempty"` + // Image is the pinned container image reference; a digest reference is the + // authoritative record of both engine and connector build. + Image string `json:"image,omitempty"` + // RequestedWorkers / ReadyWorkers record the topology. They must match for + // a run to mean what it claims. + RequestedWorkers int `json:"requested_workers,omitempty"` + ReadyWorkers int `json:"ready_workers,omitempty"` + Catalog string `json:"catalog,omitempty"` + Schema string `json:"schema,omitempty"` + TimeZone string `json:"time_zone,omitempty"` +} + +// Merge fills empty fields of e from other. Configured values (what the +// control plane told the scenario) win over probed ones, so a driver can only +// ADD detail, never contradict the recorded pin. +func (e ProtocolEnvironment) Merge(other ProtocolEnvironment) ProtocolEnvironment { + if e.Engine == "" { + e.Engine = other.Engine + } + if e.Version == "" { + e.Version = other.Version + } + if e.ConnectorVersion == "" { + e.ConnectorVersion = other.ConnectorVersion + } + if e.Image == "" { + e.Image = other.Image + } + if e.RequestedWorkers == 0 { + e.RequestedWorkers = other.RequestedWorkers + } + if e.ReadyWorkers == 0 { + e.ReadyWorkers = other.ReadyWorkers + } + if e.Catalog == "" { + e.Catalog = other.Catalog + } + if e.Schema == "" { + e.Schema = other.Schema + } + if e.TimeZone == "" { + e.TimeZone = other.TimeZone + } + return e +} + type RunSummary struct { RunID string `json:"run_id"` DatasetVersion string `json:"dataset_version"` @@ -53,4 +143,6 @@ type RunSummary struct { TotalQueries int `json:"total_queries"` TotalErrors int `json:"total_errors"` WarmupQueries int `json:"warmup_queries"` + // Environments is one entry per catalog target, in target order. + Environments []ProtocolEnvironment `json:"environments,omitempty"` } diff --git a/tests/perf/drivers/pgwire/driver.go b/tests/perf/drivers/pgwire/driver.go index 48111283..200641e8 100644 --- a/tests/perf/drivers/pgwire/driver.go +++ b/tests/perf/drivers/pgwire/driver.go @@ -55,6 +55,31 @@ func (d *Driver) Execute(ctx context.Context, query core.Query, args []any) (cor }, err } +// scalarExecutor is OPTIONALLY implemented by an Executor that can return a +// single string value. Keeping it separate from Executor means existing fakes +// (and the perf harness's own) need no change: a driver whose executor does not +// implement it simply reports the engine without a version. +type scalarExecutor interface { + Scalar(ctx context.Context, query string) (string, error) +} + +// Environment reports the non-secret comparison metadata recorded in the perf +// artifact for this protocol. +func (d *Driver) Environment(ctx context.Context) (core.ProtocolEnvironment, error) { + env := core.ProtocolEnvironment{Protocol: core.ProtocolPGWire, Engine: "duckgres"} + scalar, ok := d.exec.(scalarExecutor) + if !ok { + return env, nil + } + version, err := scalar.Scalar(ctx, "SELECT version()") + if err != nil { + // Best-effort metadata: never fail a benchmark over it. + return env, err + } + env.Version = version + return env, nil +} + func (d *Driver) Close() error { if d.exec == nil { return nil @@ -105,6 +130,15 @@ func (e *sqlExecutor) Execute(ctx context.Context, query string, args []any) (in return affected, nil } +// Scalar runs a single-value query for engine-version reporting. +func (e *sqlExecutor) Scalar(ctx context.Context, query string) (string, error) { + var value string + if err := e.db.QueryRowContext(ctx, query).Scan(&value); err != nil { + return "", err + } + return value, nil +} + func (e *sqlExecutor) Close() error { return e.db.Close() } diff --git a/tests/perf/drivers/trino/driver.go b/tests/perf/drivers/trino/driver.go new file mode 100644 index 00000000..150fc93a --- /dev/null +++ b/tests/perf/drivers/trino/driver.go @@ -0,0 +1,193 @@ +// Package trino implements the Trino HTTP statement protocol for benchmarks. +package trino + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" + + "github.com/posthog/duckgres/tests/perf/core" +) + +type Config struct { + Endpoint string + User string + Catalog string + Schema string + TimeZone string + HTTPClient *http.Client +} + +type Driver struct { + endpoint *url.URL + user string + catalog string + schema string + timeZone string + httpClient *http.Client +} + +func New(cfg Config) (*Driver, error) { + endpoint, err := url.Parse(cfg.Endpoint) + if err != nil || endpoint.Scheme == "" || endpoint.Host == "" { + return nil, fmt.Errorf("trino endpoint must be an absolute HTTP URL") + } + if endpoint.Scheme != "http" && endpoint.Scheme != "https" { + return nil, fmt.Errorf("trino endpoint scheme must be http or https") + } + client := cfg.HTTPClient + if client == nil { + client = http.DefaultClient + } + user := cfg.User + if user == "" { + user = "duckgres-perf" + } + timeZone := cfg.TimeZone + if timeZone == "" { + timeZone = "UTC" + } + return &Driver{ + endpoint: endpoint, user: user, catalog: cfg.Catalog, schema: cfg.Schema, + timeZone: timeZone, httpClient: client, + }, nil +} + +func (d *Driver) Protocol() core.Protocol { return core.ProtocolTrino } + +func (d *Driver) Execute(ctx context.Context, query core.Query, args []any) (core.ExecutionResult, error) { + if len(args) > 0 { + return core.ExecutionResult{}, fmt.Errorf("trino driver does not support parameterized query %s", query.QueryID) + } + sql, err := query.SQLFor(core.ProtocolTrino) + if err != nil { + return core.ExecutionResult{}, err + } + started := time.Now() + rows, err := d.executeStatement(ctx, sql) + return core.ExecutionResult{Rows: rows, Duration: time.Since(started)}, err +} + +func (d *Driver) Close() error { return nil } + +// Environment reports the non-secret comparison metadata the artifact records +// for this protocol. The Trino version comes from the coordinator's own +// /v1/info endpoint, so the artifact states what actually answered the queries +// rather than what was expected to. +func (d *Driver) Environment(ctx context.Context) (core.ProtocolEnvironment, error) { + env := core.ProtocolEnvironment{ + Protocol: core.ProtocolTrino, + Engine: "trino", + Catalog: d.catalog, + Schema: d.schema, + TimeZone: d.timeZone, + } + version, err := d.serverVersion(ctx) + if err != nil { + // Best-effort metadata: report what is known rather than failing. + return env, err + } + env.Version = version + return env, nil +} + +// serverVersion reads GET /v1/info, Trino's unauthenticated server-info +// endpoint: {"nodeVersion":{"version":"483"},...}. +func (d *Driver) serverVersion(ctx context.Context) (string, error) { + infoURL := d.endpoint.ResolveReference(&url.URL{Path: "/v1/info"}) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, infoURL.String(), nil) + if err != nil { + return "", fmt.Errorf("create Trino info request: %w", err) + } + req.Header.Set("Accept", "application/json") + resp, err := d.httpClient.Do(req) + if err != nil { + return "", fmt.Errorf("execute Trino info request: %w", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("trino info request returned %s", resp.Status) + } + var info struct { + NodeVersion struct { + Version string `json:"version"` + } `json:"nodeVersion"` + } + if err := json.NewDecoder(resp.Body).Decode(&info); err != nil { + return "", fmt.Errorf("decode Trino info response: %w", err) + } + return info.NodeVersion.Version, nil +} + +type statementResponse struct { + NextURI string `json:"nextUri"` + Data []json.RawMessage `json:"data"` + Error *struct { + Message string `json:"message"` + } `json:"error"` +} + +func (d *Driver) executeStatement(ctx context.Context, sql string) (int64, error) { + statementURL := d.endpoint.ResolveReference(&url.URL{Path: "/v1/statement"}) + response, err := d.request(ctx, http.MethodPost, statementURL.String(), strings.NewReader(sql)) + if err != nil { + return 0, err + } + var rows int64 + for { + rows += int64(len(response.Data)) + if response.Error != nil { + return 0, fmt.Errorf("trino query failed: %s", response.Error.Message) + } + if response.NextURI == "" { + return rows, nil + } + nextURL, err := d.endpoint.Parse(response.NextURI) + if err != nil { + return 0, fmt.Errorf("parse Trino next URI: %w", err) + } + response, err = d.request(ctx, http.MethodGet, nextURL.String(), nil) + if err != nil { + return 0, err + } + } +} + +func (d *Driver) request(ctx context.Context, method, requestURL string, body io.Reader) (statementResponse, error) { + req, err := http.NewRequestWithContext(ctx, method, requestURL, body) + if err != nil { + return statementResponse{}, fmt.Errorf("create Trino request: %w", err) + } + req.Header.Set("X-Trino-User", d.user) + req.Header.Set("X-Trino-Time-Zone", d.timeZone) + if d.catalog != "" { + req.Header.Set("X-Trino-Catalog", d.catalog) + } + if d.schema != "" { + req.Header.Set("X-Trino-Schema", d.schema) + } + if method == http.MethodPost { + req.Header.Set("Content-Type", "text/plain") + } + + resp, err := d.httpClient.Do(req) + if err != nil { + return statementResponse{}, fmt.Errorf("execute Trino request: %w", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 4<<10)) + return statementResponse{}, fmt.Errorf("trino request returned %s: %s", resp.Status, bytes.TrimSpace(body)) + } + var result statementResponse + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return statementResponse{}, fmt.Errorf("decode Trino response: %w", err) + } + return result, nil +} diff --git a/tests/perf/drivers/trino/driver_test.go b/tests/perf/drivers/trino/driver_test.go new file mode 100644 index 00000000..808d0054 --- /dev/null +++ b/tests/perf/drivers/trino/driver_test.go @@ -0,0 +1,100 @@ +package trino + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/posthog/duckgres/tests/perf/core" +) + +func TestDriverExecutesStatementAndFollowsPages(t *testing.T) { + var serverURL string + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests == 1 { + if r.Method != http.MethodPost || r.URL.Path != "/v1/statement" { + t.Fatalf("first request = %s %s, want POST /v1/statement", r.Method, r.URL.Path) + } + if got := r.Header.Get("X-Trino-Catalog"); got != "ducklake" { + t.Fatalf("catalog header = %q, want ducklake", got) + } + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("read statement body: %v", err) + } + if string(body) != "SELECT 1" { + t.Fatalf("statement body = %q", body) + } + _, _ = w.Write([]byte(`{"id":"query-1","nextUri":"` + serverURL + `/v1/next/1","data":[[1],[2]]}`)) + return + } + if r.Method != http.MethodGet || r.URL.Path != "/v1/next/1" { + t.Fatalf("next request = %s %s, want GET /v1/next/1", r.Method, r.URL.Path) + } + _, _ = w.Write([]byte(`{"id":"query-1","data":[[3]]}`)) + })) + defer server.Close() + serverURL = server.URL + + driver, err := New(Config{Endpoint: server.URL, Catalog: "ducklake", Schema: "posthog", User: "perf"}) + if err != nil { + t.Fatalf("New returned error: %v", err) + } + result, err := driver.Execute(context.Background(), core.Query{QueryID: "q1", TrinoSQL: "SELECT 1"}, nil) + if err != nil { + t.Fatalf("Execute returned error: %v", err) + } + if result.Rows != 3 { + t.Fatalf("rows = %d, want 3", result.Rows) + } +} + +func TestDriverReportsEngineEnvironmentFromCoordinator(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/info" { + t.Fatalf("request path = %q, want /v1/info", r.URL.Path) + } + _, _ = w.Write([]byte(`{"nodeVersion":{"version":"483"},"starting":false}`)) + })) + defer server.Close() + + driver, err := New(Config{Endpoint: server.URL, Catalog: "ducklake", Schema: "posthog"}) + if err != nil { + t.Fatalf("New returned error: %v", err) + } + env, err := driver.Environment(context.Background()) + if err != nil { + t.Fatalf("Environment returned error: %v", err) + } + if env.Protocol != core.ProtocolTrino || env.Engine != "trino" || env.Version != "483" { + t.Fatalf("environment = %+v", env) + } + if env.Catalog != "ducklake" || env.Schema != "posthog" || env.TimeZone != "UTC" { + t.Fatalf("catalog identity = %+v", env) + } +} + +// Comparison metadata is best-effort: an unreachable info endpoint still yields +// the catalog identity the artifact needs. +func TestDriverEnvironmentDegradesWhenInfoIsUnavailable(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + + driver, err := New(Config{Endpoint: server.URL, Catalog: "ducklake", Schema: "posthog"}) + if err != nil { + t.Fatalf("New returned error: %v", err) + } + env, _ := driver.Environment(context.Background()) + if env.Engine != "trino" || env.Catalog != "ducklake" { + t.Fatalf("environment = %+v", env) + } + if env.Version != "" { + t.Fatalf("version = %q, want empty when the probe fails", env.Version) + } +} diff --git a/tests/perf/queries/ducklake_posthog_tables.yaml b/tests/perf/queries/ducklake_posthog_tables.yaml index 4a7895e0..f45782ed 100644 --- a/tests/perf/queries/ducklake_posthog_tables.yaml +++ b/tests/perf/queries/ducklake_posthog_tables.yaml @@ -4,6 +4,7 @@ seed: 42 dataset_scale: 1 targets: - pgwire + - trino warmup_iterations: 1 measure_iterations: 3 queries: @@ -20,6 +21,7 @@ queries: tags: [nightly, frozen, posthog, events, aggregate, paired, ducklake-table] params: {} pgwire_sql: SELECT COUNT(*) AS events FROM posthog.events + trino_sql: SELECT COUNT(*) AS events FROM ducklake.posthog.events - query_id: q_events_count_one_day__raw_view intent_id: intent_events_count_one_day @@ -40,6 +42,11 @@ queries: FROM posthog.events WHERE "timestamp" >= TIMESTAMPTZ '2026-03-01 00:00:00+00' AND "timestamp" < TIMESTAMPTZ '2026-03-02 00:00:00+00' + trino_sql: > + SELECT COUNT(*) AS events + FROM ducklake.posthog.events + WHERE "timestamp" >= from_iso8601_timestamp('2026-03-01T00:00:00Z') + AND "timestamp" < from_iso8601_timestamp('2026-03-02T00:00:00Z') - query_id: q_events_by_name_march_2026__raw_view intent_id: intent_events_by_name_march_2026 @@ -52,6 +59,7 @@ queries: tags: [nightly, frozen, posthog, events, aggregate, analytics, paired, ducklake-table] params: {} pgwire_sql: SELECT event, COUNT(*) AS events FROM posthog.events WHERE "timestamp" >= TIMESTAMPTZ '2026-03-01 00:00:00+00' AND "timestamp" < TIMESTAMPTZ '2026-03-18 00:00:00+00' GROUP BY event ORDER BY events DESC, event LIMIT 20 + trino_sql: SELECT event, COUNT(*) AS events FROM ducklake.posthog.events WHERE "timestamp" >= from_iso8601_timestamp('2026-03-01T00:00:00Z') AND "timestamp" < from_iso8601_timestamp('2026-03-18T00:00:00Z') GROUP BY event ORDER BY events DESC, event LIMIT 20 - query_id: q_events_distinct_persons__raw_view intent_id: intent_events_distinct_persons @@ -64,6 +72,7 @@ queries: tags: [nightly, frozen, posthog, events, aggregate, distinct, paired, ducklake-table] params: {} pgwire_sql: SELECT COUNT(DISTINCT person_id) AS distinct_persons FROM posthog.events WHERE person_id IS NOT NULL + trino_sql: SELECT COUNT(DISTINCT person_id) AS distinct_persons FROM ducklake.posthog.events WHERE person_id IS NOT NULL # Table-only probes track the DuckLake workload over time without a raw # Parquet control query. @@ -72,15 +81,18 @@ queries: tags: [nightly, frozen, posthog, persons, aggregate, ducklake-table] params: {} pgwire_sql: SELECT COUNT(*) AS persons FROM posthog.persons + trino_sql: SELECT COUNT(*) AS persons FROM ducklake.posthog.persons - query_id: q_persons_daily_april_2026__ducklake_table intent_id: intent_persons_daily_april_2026 tags: [nightly, frozen, posthog, persons, aggregate, time-series, ducklake-table] params: {} pgwire_sql: SELECT date_trunc('day', _timestamp) AS day, COUNT(*) AS persons FROM posthog.persons WHERE _timestamp >= TIMESTAMPTZ '2026-04-01 00:00:00+00' AND _timestamp < TIMESTAMPTZ '2026-05-01 00:00:00+00' GROUP BY 1 ORDER BY 1 + trino_sql: SELECT date_trunc('day', _timestamp) AS day, COUNT(*) AS persons FROM ducklake.posthog.persons WHERE _timestamp >= from_iso8601_timestamp('2026-04-01T00:00:00Z') AND _timestamp < from_iso8601_timestamp('2026-05-01T00:00:00Z') GROUP BY 1 ORDER BY 1 - query_id: q_events_daily_march_2026__ducklake_table intent_id: intent_events_daily_march_2026 tags: [nightly, frozen, posthog, events, aggregate, time-series, ducklake-table] params: {} pgwire_sql: SELECT date_trunc('day', "timestamp") AS day, COUNT(*) AS events FROM posthog.events WHERE "timestamp" >= TIMESTAMPTZ '2026-03-01 00:00:00+00' AND "timestamp" < TIMESTAMPTZ '2026-03-18 00:00:00+00' GROUP BY 1 ORDER BY 1 + trino_sql: SELECT date_trunc('day', "timestamp") AS day, COUNT(*) AS events FROM ducklake.posthog.events WHERE "timestamp" >= from_iso8601_timestamp('2026-03-01T00:00:00Z') AND "timestamp" < from_iso8601_timestamp('2026-03-18T00:00:00Z') GROUP BY 1 ORDER BY 1 From 64a9ffee94efea96561398a5677088ae09f12d29 Mon Sep 17 00:00:00 2001 From: Bill Guowei Yang Date: Mon, 10 Aug 2026 09:48:19 -0400 Subject: [PATCH 2/2] Assemble the control-plane config in one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both control-plane entry points hand-maintained their own ~56-field ControlPlaneConfig literal with nothing forcing them to agree, and two knobs had drifted out of the PRODUCTION one — cmd/duckgres-controlplane (Dockerfile.controlplane, its own CD pipeline): - UserSecretKey (pre-existing, unrelated to this PR): resolved and then discarded, so DUCKGRES_USER_SECRET_KEY never reached the control plane and CREATE PERSISTENT SECRET was rejected with 0A000 in production even when the chart supplied a key. - TrinoBenchmark (added earlier in this PR): every DUCKGRES_TRINO_BENCHMARK_* variable parsed and then dropped, so the lifecycle stayed unbuilt in production and its routes answered 503. Neither is visible to the mw-dev e2e or scenario runs: those build the root Dockerfile's all-in-one binary, whose literal wires both fields. configresolve.ControlPlaneConfig is now the single assembly site; the four per-binary values (the binary's own TLS/ACME-adjusted server.Config, its socket-dir and config-path flags, its already-running metrics server) arrive through ControlPlaneOverrides. Adding a knob to Resolved now reaches both binaries by construction, and 124 lines of duplicated literal are replaced by 12. The tripwire (configresolve/controlplane_test.go) checks MAPPING COVERAGE in both directions and deliberately asserts nothing about runtime values — zero is legitimate for an unset TTL, an empty PriorityClass meaning "headroom disabled", or a false feature gate. It requires that every destination field be movable by some input, and that every Resolved field change the output. Its two exemption maps carry stated structural reasons (Server arrives via overrides; SessionInitTimeout reaches the control plane inside the embedded server.Config; HealthCheckInterval has no configuration source at all and is defaulted in RunControlPlane). Also fixes two stale CLAUDE.md pointers to the long-renamed config_resolution.go. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 27 +++- cmd/duckgres-controlplane/main.go | 67 +-------- configresolve/controlplane.go | 111 +++++++++++++++ configresolve/controlplane_test.go | 221 +++++++++++++++++++++++++++++ main.go | 69 +-------- 5 files changed, 368 insertions(+), 127 deletions(-) create mode 100644 configresolve/controlplane.go create mode 100644 configresolve/controlplane_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 1fe3696e..1f324f19 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -94,7 +94,8 @@ In topologies 2 and 3, the control plane exposes only PostgreSQL wire protocol t ### Key Components -- **main.go / config_resolution.go**: CLI flags; effective config resolution (CLI > env > YAML > defaults), including env-only K8s knobs. +- **main.go / cmd/**: entry points. `main.go` is the all-in-one binary (all four modes); `cmd/duckgres-controlplane` is the production control plane, `cmd/duckgres-worker` the production worker. +- **configresolve/**: CLI flags (`cliflags.go`) and effective config resolution (`resolve.go`; CLI > env > YAML > defaults), including env-only K8s knobs. `controlplane.go` is the SINGLE place a resolved config becomes a `controlplane.ControlPlaneConfig` — both control-plane entry points call it (see Configuration below). - **server/** — PG wire protocol server and DuckDB execution - Wire protocol & connections: `server.go`, `conn.go`, `conn_errors.go`, `conn_query_exec.go`, `conn_results.go`, `conn_copy.go`, `conn_extended_query.go`, `conn_pg_stat_activity.go`, `conn_cursor.go`, `protocol.go`, `exports.go` - Execution: `executor.go`, `flight_executor.go`, `chsql.go`, `transient.go` @@ -140,7 +141,7 @@ Key CLI flags for control-plane mode: - K8s pool: `--k8s-worker-image`, `--k8s-worker-namespace`, `--k8s-control-plane-id`, `--k8s-worker-port`, `--k8s-worker-secret`, `--k8s-worker-configmap`, `--k8s-worker-image-pull-policy`, `--k8s-worker-service-account` (no global worker cap — per-org `Org.MaxWorkers`, 0=unbounded, is the only cap) - AWS / STS: `--aws-region` - Compute-usage billing needs no config: metering is always on for the remote backend and billing PULLS usage over the internal-secret-authed HTTP API (`GET /api/v1/billing/usage` + `POST /api/v1/billing/ack`). See `docs/design/billing-pull-api.md` and "Compute-Usage Billing" below. - - Pod scheduling knobs (CPU/memory requests, node selector, tolerations) are env-only — see `config_resolution.go`. + - Pod scheduling knobs (CPU/memory requests, node selector, tolerations) are env-only — see `configresolve/resolve.go`. Key CLI flags for duckdb-service mode: - `--duckdb-listen` (e.g., `unix:///...` or `:8816`) @@ -150,7 +151,7 @@ Key CLI flags for duckdb-service mode: ## Configuration -Configuration is resolved in `config_resolution.go` with the following precedence (highest to lowest): +Configuration is resolved in `configresolve/resolve.go` with the following precedence (highest to lowest): 1. CLI flags (`--port`, `--config`, etc.) 2. Environment variables (`DUCKGRES_PORT`, etc.) 3. YAML config file @@ -158,6 +159,26 @@ Configuration is resolved in `config_resolution.go` with the following precedenc Note: `--mode` is CLI-only (not loadable from YAML/env). A handful of K8s pod-scheduling knobs are env-only (no CLI flag). +**A resolved knob only reaches the control plane through +`configresolve.ControlPlaneConfig`** (`configresolve/controlplane.go`). That is +the SINGLE assembly site for `controlplane.ControlPlaneConfig`, shared by both +entry points — the all-in-one `duckgres --mode control-plane` (`main.go`) and the +production `cmd/duckgres-controlplane` (`Dockerfile.controlplane`, its own CD +pipeline). It exists because the two binaries previously each carried a +hand-maintained ~56-field literal with nothing forcing them to agree, and two +knobs had silently drifted out of the PRODUCTION one — `DUCKGRES_USER_SECRET_KEY` +and every `DUCKGRES_TRINO_BENCHMARK_*` variable were resolved into memory and +then dropped, so the features they configure were dead in production while the +mw-dev scenario (which builds the all-in-one binary) passed. Add a knob to +`Resolved` and wire it in that one function; never re-introduce a second literal. +`configresolve/controlplane_test.go` is the tripwire, and it checks MAPPING +COVERAGE in both directions rather than runtime values (zero is a legitimate +value for an unset TTL, an empty PriorityClass, or a false feature gate): every +destination field must be movable by some input, and every `Resolved` field must +change the output. Its two exemption maps are structural facts with stated +reasons, not a dumping ground. **When two Dockerfiles select different +entrypoints, treat duplicated config assembly as a field-by-field review item.** + ## Keep docs in sync with behavior When you change a behavior, default, flag, or invariant that is documented diff --git a/cmd/duckgres-controlplane/main.go b/cmd/duckgres-controlplane/main.go index 55ed7f27..4e80f9d0 100644 --- a/cmd/duckgres-controlplane/main.go +++ b/cmd/duckgres-controlplane/main.go @@ -224,66 +224,11 @@ func main() { _ = metricsSrv.Shutdown(ctx) }() - cpCfg := controlplane.ControlPlaneConfig{ - Config: cfg, - Process: controlplane.ProcessConfig{ - MinWorkers: resolved.ProcessMinWorkers, - MaxWorkers: resolved.ProcessMaxWorkers, - }, - SocketDir: *socketDir, - ConfigPath: *configFile, - WorkerQueueTimeout: resolved.WorkerQueueTimeout, - WorkerIdleTimeout: resolved.WorkerIdleTimeout, - RetireOnSessionEnd: resolved.ProcessRetireOnSessionEnd, - HandoverDrainTimeout: resolved.HandoverDrainTimeout, - MetricsServer: metricsSrv, - WorkerBackend: resolved.WorkerBackend, - ConfigStoreConn: resolved.ConfigStoreConn, - ConfigPollInterval: resolved.ConfigPollInterval, - InternalSecret: resolved.InternalSecret, - InternalSecretFallbacks: resolved.InternalSecretFallbacks, - ReadOnlySecret: resolved.ReadOnlySecret, - ReadOnlySecretFallbacks: resolved.ReadOnlySecretFallbacks, - SNIRoutingMode: resolved.SNIRoutingMode, - ManagedHostnameSuffixes: resolved.ManagedHostnameSuffixes, - MetadataHostnameSuffixes: resolved.MetadataHostnameSuffixes, - MetadataProxyMaxConns: resolved.MetadataProxyMaxConns, - DucklingBucketSuffix: resolved.DucklingBucketSuffix, - DuckLakeDefaultSpecVersion: resolved.DuckLakeDefaultSpecVersion, - - AdmissionReclaimerMaxReservations: resolved.AdmissionReclaimerMaxReservations, - K8s: controlplane.K8sConfig{ - WorkerImage: resolved.K8sWorkerImage, - WorkerNamespace: resolved.K8sWorkerNamespace, - ControlPlaneID: resolved.K8sControlPlaneID, - WorkerPort: resolved.K8sWorkerPort, - WorkerSecret: resolved.K8sWorkerSecret, - WorkerConfigMap: resolved.K8sWorkerConfigMap, - ImagePullPolicy: resolved.K8sWorkerImagePullPolicy, - ServiceAccount: resolved.K8sWorkerServiceAccount, - WorkerCPURequest: resolved.K8sWorkerCPURequest, - WorkerMemoryRequest: resolved.K8sWorkerMemoryRequest, - WorkerNodeSelector: resolved.K8sWorkerNodeSelector, - WorkerTolerationKey: resolved.K8sWorkerTolerationKey, - WorkerTolerationValue: resolved.K8sWorkerTolerationValue, - AllowClientWorkerProfile: resolved.K8sAllowClientWorkerProfile, - WorkerPriorityClassName: resolved.K8sWorkerPriorityClassName, - PlaceholderImage: resolved.K8sPlaceholderImage, - PlaceholderPriorityClassName: resolved.K8sPlaceholderPriorityClassName, - WorkerProfileMinCPU: resolved.K8sWorkerProfileMinCPU, - WorkerProfileMaxCPU: resolved.K8sWorkerProfileMaxCPU, - WorkerProfileMinMemory: resolved.K8sWorkerProfileMinMemory, - WorkerProfileMaxMemory: resolved.K8sWorkerProfileMaxMemory, - WorkerMaxTTL: resolved.K8sWorkerMaxTTL, - WorkerDefaultTTL: resolved.K8sWorkerDefaultTTL, - ExploratoryTierEnabled: resolved.K8sExploratoryTierEnabled, - ExploratoryWorkerCPU: resolved.K8sExploratoryWorkerCPU, - ExploratoryWorkerMemory: resolved.K8sExploratoryWorkerMemory, - ExploratoryWorkerTTL: resolved.K8sExploratoryWorkerTTL, - ReshardPodCPU: resolved.K8sReshardPodCPU, - ReshardPodMemory: resolved.K8sReshardPodMemory, - AWSRegion: resolved.AWSRegion, - }, - } + cpCfg := configresolve.ControlPlaneConfig(resolved, configresolve.ControlPlaneOverrides{ + Server: cfg, + SocketDir: *socketDir, + ConfigPath: *configFile, + MetricsServer: metricsSrv, + }) controlplane.RunControlPlane(cpCfg) } diff --git a/configresolve/controlplane.go b/configresolve/controlplane.go new file mode 100644 index 00000000..4611b78b --- /dev/null +++ b/configresolve/controlplane.go @@ -0,0 +1,111 @@ +package configresolve + +import ( + "net/http" + + "github.com/posthog/duckgres/controlplane" + "github.com/posthog/duckgres/server" +) + +// Control-plane config assembly. +// +// This is the SINGLE place a resolved config becomes a +// controlplane.ControlPlaneConfig. Both control-plane entry points call it: the +// all-in-one `duckgres --mode control-plane` (main.go) and the production +// `cmd/duckgres-controlplane` (Dockerfile.controlplane, its own CD pipeline). +// +// It is one function on purpose. The two binaries previously each carried a +// hand-maintained 56-field literal with nothing forcing them to agree, and both +// DUCKGRES_USER_SECRET_KEY and every DUCKGRES_TRINO_BENCHMARK_* variable had +// drifted out of the production one — resolved into memory, then dropped on the +// floor, so the features they configure were dead in production while the +// mw-dev scenario (which runs the all-in-one binary) passed. Adding a knob to +// Resolved and wiring it here now reaches both binaries, and +// configresolve/controlplane_test.go fails if either half of that contract +// breaks again. + +// ControlPlaneOverrides carries the few values that are NOT part of the +// resolved config because each binary owns them: its own (already +// TLS/ACME-adjusted) server config, the flags it parsed, and the metrics server +// it already started. +type ControlPlaneOverrides struct { + // Server is the binary's server.Config AFTER it has applied its own TLS / + // ACME adjustments — not resolved.Server. + Server server.Config + // SocketDir and ConfigPath come from the binary's own flags. + SocketDir string + ConfigPath string + // MetricsServer is the already-running metrics server, shut down during a + // handover. Nil when the binary runs none. + MetricsServer *http.Server +} + +// ControlPlaneConfig assembles the control-plane config both binaries boot +// from. Every field is either derived from resolved or taken from overrides; +// there is no third source. +func ControlPlaneConfig(resolved Resolved, overrides ControlPlaneOverrides) controlplane.ControlPlaneConfig { + return controlplane.ControlPlaneConfig{ + Config: overrides.Server, + Process: controlplane.ProcessConfig{ + MinWorkers: resolved.ProcessMinWorkers, + MaxWorkers: resolved.ProcessMaxWorkers, + }, + SocketDir: overrides.SocketDir, + ConfigPath: overrides.ConfigPath, + MetricsServer: overrides.MetricsServer, + WorkerQueueTimeout: resolved.WorkerQueueTimeout, + WorkerIdleTimeout: resolved.WorkerIdleTimeout, + RetireOnSessionEnd: resolved.ProcessRetireOnSessionEnd, + HandoverDrainTimeout: resolved.HandoverDrainTimeout, + WorkerBackend: resolved.WorkerBackend, + ConfigStoreConn: resolved.ConfigStoreConn, + ConfigPollInterval: resolved.ConfigPollInterval, + InternalSecret: resolved.InternalSecret, + InternalSecretFallbacks: resolved.InternalSecretFallbacks, + ReadOnlySecret: resolved.ReadOnlySecret, + ReadOnlySecretFallbacks: resolved.ReadOnlySecretFallbacks, + UserSecretKey: resolved.UserSecretKey, + SNIRoutingMode: resolved.SNIRoutingMode, + ManagedHostnameSuffixes: resolved.ManagedHostnameSuffixes, + MetadataHostnameSuffixes: resolved.MetadataHostnameSuffixes, + MetadataProxyMaxConns: resolved.MetadataProxyMaxConns, + DucklingBucketSuffix: resolved.DucklingBucketSuffix, + DuckLakeDefaultSpecVersion: resolved.DuckLakeDefaultSpecVersion, + + AdmissionReclaimerMaxReservations: resolved.AdmissionReclaimerMaxReservations, + + K8s: controlplane.K8sConfig{ + WorkerImage: resolved.K8sWorkerImage, + WorkerNamespace: resolved.K8sWorkerNamespace, + ControlPlaneID: resolved.K8sControlPlaneID, + WorkerPort: resolved.K8sWorkerPort, + WorkerSecret: resolved.K8sWorkerSecret, + WorkerConfigMap: resolved.K8sWorkerConfigMap, + ImagePullPolicy: resolved.K8sWorkerImagePullPolicy, + ServiceAccount: resolved.K8sWorkerServiceAccount, + WorkerCPURequest: resolved.K8sWorkerCPURequest, + WorkerMemoryRequest: resolved.K8sWorkerMemoryRequest, + WorkerNodeSelector: resolved.K8sWorkerNodeSelector, + WorkerTolerationKey: resolved.K8sWorkerTolerationKey, + WorkerTolerationValue: resolved.K8sWorkerTolerationValue, + AllowClientWorkerProfile: resolved.K8sAllowClientWorkerProfile, + WorkerPriorityClassName: resolved.K8sWorkerPriorityClassName, + PlaceholderImage: resolved.K8sPlaceholderImage, + PlaceholderPriorityClassName: resolved.K8sPlaceholderPriorityClassName, + WorkerProfileMinCPU: resolved.K8sWorkerProfileMinCPU, + WorkerProfileMaxCPU: resolved.K8sWorkerProfileMaxCPU, + WorkerProfileMinMemory: resolved.K8sWorkerProfileMinMemory, + WorkerProfileMaxMemory: resolved.K8sWorkerProfileMaxMemory, + WorkerMaxTTL: resolved.K8sWorkerMaxTTL, + WorkerDefaultTTL: resolved.K8sWorkerDefaultTTL, + ExploratoryTierEnabled: resolved.K8sExploratoryTierEnabled, + ExploratoryWorkerCPU: resolved.K8sExploratoryWorkerCPU, + ExploratoryWorkerMemory: resolved.K8sExploratoryWorkerMemory, + ExploratoryWorkerTTL: resolved.K8sExploratoryWorkerTTL, + ReshardPodCPU: resolved.K8sReshardPodCPU, + ReshardPodMemory: resolved.K8sReshardPodMemory, + TrinoBenchmark: resolved.TrinoBenchmark, + AWSRegion: resolved.AWSRegion, + }, + } +} diff --git a/configresolve/controlplane_test.go b/configresolve/controlplane_test.go new file mode 100644 index 00000000..236a427c --- /dev/null +++ b/configresolve/controlplane_test.go @@ -0,0 +1,221 @@ +package configresolve + +import ( + "net/http" + "reflect" + "testing" + + "github.com/posthog/duckgres/server" +) + +// The control-plane config used to be assembled by a hand-maintained literal in +// EACH binary: the all-in-one `duckgres --mode control-plane` and the +// production `cmd/duckgres-controlplane`. Nothing forced the two to agree, and +// two knobs had already drifted out of the production one — every +// DUCKGRES_TRINO_BENCHMARK_* variable and DUCKGRES_USER_SECRET_KEY were parsed +// and then discarded, so the feature they configure was dead in production +// while the mw-dev scenario (which runs the all-in-one binary) passed. +// +// ControlPlaneConfig is now the single assembly site, and these tests are the +// tripwire for that bug class. They check MAPPING COVERAGE in both directions +// and deliberately assert nothing about runtime values: +// +// - every field of the produced config can be moved by some input, so a field +// nothing wires is caught; and +// - every field of Resolved changes the output, so a knob that is parsed and +// then discarded is caught. +// +// Zero is a legitimate runtime value for many of these fields (an unset TTL, an +// empty PriorityClass meaning "headroom disabled", a false feature gate). +// Requiring non-zero values would turn this into an assertion about production +// configuration, which is not what it is for — the sentinels below are probes, +// not expected values. + +// sentinelOverrides is the non-zero probe for the per-binary values that +// legitimately do NOT come from Resolved. +func sentinelOverrides() ControlPlaneOverrides { + return ControlPlaneOverrides{ + Server: server.Config{Host: "127.0.0.1", Port: 5432}, + SocketDir: "/tmp/duckgres-sockets", + ConfigPath: "/etc/duckgres/duckgres.yaml", + MetricsServer: &http.Server{Addr: ":9090"}, + } +} + +// resolvedFieldsFromOverrides are Resolved fields that intentionally do NOT +// feed the control-plane config directly. Structural facts, not exemptions: +// +// - Server: each binary adjusts its own copy (TLS/ACME) and passes it in via +// the overrides, so the constructor must not read resolved.Server. +// - SessionInitTimeout: a convenience mirror of Server.SessionInitTimeout +// (see ResolveEffective); it reaches the control plane inside the embedded +// server.Config, not as a top-level control-plane field. +var resolvedFieldsFromOverrides = map[string]string{ + "Server": "supplied through ControlPlaneOverrides.Server", + "SessionInitTimeout": "reaches the control plane inside the embedded server.Config", +} + +// unconfiguredDestinationFields are control-plane config fields with NO +// configuration source anywhere — no flag, no env var, no YAML key — so nothing +// could move them and their absence here is not a dropped knob. Each entry +// names where the value actually comes from; if someone later adds a knob for +// one, this exemption becomes wrong and should be deleted rather than kept. +var unconfiguredDestinationFields = map[string]string{ + "HealthCheckInterval": "defaulted inside controlplane.RunControlPlane (2s); not configurable", +} + +// fullyPopulatedResolved fills every Resolved field the constructor reads with a +// distinctive non-zero probe value. +func fullyPopulatedResolved(t *testing.T) Resolved { + t.Helper() + var resolved Resolved + value := reflect.ValueOf(&resolved).Elem() + for i := 0; i < value.NumField(); i++ { + name := value.Type().Field(i).Name + if _, skip := resolvedFieldsFromOverrides[name]; skip { + continue + } + if !setSentinel(value.Field(i)) { + t.Fatalf("Resolved.%s has unsupported kind %s; teach setSentinel about it "+ + "so the control-plane wiring tripwire keeps covering it", name, value.Field(i).Kind()) + } + } + return resolved +} + +// setSentinel writes a non-zero value of the field's type. It returns false for +// a kind it does not know how to fill, which fails the test loudly rather than +// silently dropping a field from coverage. +func setSentinel(field reflect.Value) bool { + switch field.Kind() { + case reflect.String: + field.SetString("sentinel") + case reflect.Bool: + field.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + // time.Duration is an int64 kind; 7 is non-zero either way. + field.SetInt(7) + case reflect.Slice: + slice := reflect.MakeSlice(field.Type(), 1, 1) + if !setSentinel(slice.Index(0)) { + return false + } + field.Set(slice) + case reflect.Struct: + filled := false + for i := 0; i < field.NumField(); i++ { + if field.Field(i).CanSet() && setSentinel(field.Field(i)) { + filled = true + } + } + return filled + default: + return false + } + return true +} + +// A destination field is WIRED iff some input can move it. This says nothing +// about what its value should be — only that the assembly reads a source for +// it. A field that is identical whether every input is zero or every input is a +// sentinel has no source at all: whatever configures it is discarded. +func TestControlPlaneConfigMapsEveryDestinationField(t *testing.T) { + unwired := ControlPlaneConfig(Resolved{}, ControlPlaneOverrides{}) + wired := ControlPlaneConfig(fullyPopulatedResolved(t), sentinelOverrides()) + + assertEveryFieldInfluenced(t, reflect.ValueOf(unwired), reflect.ValueOf(wired), "ControlPlaneConfig") +} + +// assertEveryFieldInfluenced compares two builds field by field, recursing into +// nested CONFIG structs (Process, K8s, TrinoBenchmark) so a single unwired +// nested field is caught rather than masked by its siblings. +func assertEveryFieldInfluenced(t *testing.T, unwired, wired reflect.Value, path string) { + t.Helper() + for i := 0; i < wired.NumField(); i++ { + name := wired.Type().Field(i).Name + qualified := path + "." + name + unwiredField, wiredField := unwired.Field(i), wired.Field(i) + if reason, skip := unconfiguredDestinationFields[name]; skip { + t.Logf("%s: %s", qualified, reason) + continue + } + + // server.Config arrives wholesale from the overrides and has its own + // resolution/defaulting tests; compare it as one unit. + if name == "Config" { + if reflect.DeepEqual(unwiredField.Interface(), wiredField.Interface()) { + t.Errorf("%s is not wired: the assembly does not read the overrides' server config", qualified) + } + continue + } + if wiredField.Kind() == reflect.Struct { + assertEveryFieldInfluenced(t, unwiredField, wiredField, qualified) + continue + } + if reflect.DeepEqual(unwiredField.Interface(), wiredField.Interface()) { + t.Errorf("%s is not wired: no input moves it, so whatever configures it "+ + "is parsed and then discarded", qualified) + } + } +} + +// The other direction: a knob can be added to Resolved and simply never read. +// Setting exactly one Resolved field at a time must change the result. +func TestControlPlaneConfigConsumesEveryResolvedField(t *testing.T) { + baseline := ControlPlaneConfig(Resolved{}, sentinelOverrides()) + + var probe Resolved + value := reflect.ValueOf(&probe).Elem() + for i := 0; i < value.NumField(); i++ { + name := value.Type().Field(i).Name + if reason, skip := resolvedFieldsFromOverrides[name]; skip { + t.Logf("Resolved.%s: %s", name, reason) + continue + } + t.Run(name, func(t *testing.T) { + var one Resolved + field := reflect.ValueOf(&one).Elem().Field(i) + if !setSentinel(field) { + t.Fatalf("Resolved.%s has unsupported kind %s; teach setSentinel about it", name, field.Kind()) + } + if reflect.DeepEqual(ControlPlaneConfig(one, sentinelOverrides()), baseline) { + t.Fatalf("setting Resolved.%s does not change the control-plane config: "+ + "whatever env/flag populates it is parsed and then discarded", name) + } + }) + } +} + +// Regression guards for the two knobs that had actually drifted out of the +// production binary's literal. +func TestControlPlaneConfigWiresTrinoBenchmarkAndUserSecretKey(t *testing.T) { + resolved := Resolved{UserSecretKey: "base64-aes-key"} + resolved.TrinoBenchmark.Enabled = true + resolved.TrinoBenchmark.Image = "registry.example/trino-brikk@sha256:abc" + resolved.TrinoBenchmark.Workers = 4 + + cfg := ControlPlaneConfig(resolved, sentinelOverrides()) + + if cfg.UserSecretKey != "base64-aes-key" { + t.Fatal("UserSecretKey is not wired: CREATE PERSISTENT SECRET would be rejected in production") + } + if !cfg.K8s.TrinoBenchmark.Enabled || cfg.K8s.TrinoBenchmark.Image != "registry.example/trino-brikk@sha256:abc" { + t.Fatalf("TrinoBenchmark is not wired: %+v", cfg.K8s.TrinoBenchmark) + } + if cfg.K8s.TrinoBenchmark.Workers != 4 { + t.Fatalf("TrinoBenchmark worker count = %d", cfg.K8s.TrinoBenchmark.Workers) + } +} + +// The disabled default must survive the assembly: a deployment that sets no +// Trino benchmark variables gets a lifecycle that cannot be built. +func TestControlPlaneConfigPreservesDisabledTrinoBenchmarkDefault(t *testing.T) { + cfg := ControlPlaneConfig(ResolveEffective(nil, CLIInputs{}, nil, nil), sentinelOverrides()) + + if cfg.K8s.TrinoBenchmark.Enabled { + t.Fatal("Trino benchmark lifecycle must stay disabled with no configuration") + } + if cfg.K8s.TrinoBenchmark.Image != "" { + t.Fatalf("Trino benchmark image = %q, want empty", cfg.K8s.TrinoBenchmark.Image) + } +} diff --git a/main.go b/main.go index 94ca0b6f..acf63c89 100644 --- a/main.go +++ b/main.go @@ -363,69 +363,12 @@ func main() { // Handle control-plane mode if *mode == "control-plane" { - cpCfg := controlplane.ControlPlaneConfig{ - Config: cfg, - Process: controlplane.ProcessConfig{ - MinWorkers: resolved.ProcessMinWorkers, - MaxWorkers: resolved.ProcessMaxWorkers, - }, - SocketDir: *socketDir, - ConfigPath: *configFile, - WorkerQueueTimeout: resolved.WorkerQueueTimeout, - WorkerIdleTimeout: resolved.WorkerIdleTimeout, - RetireOnSessionEnd: resolved.ProcessRetireOnSessionEnd, - HandoverDrainTimeout: resolved.HandoverDrainTimeout, - MetricsServer: metricsSrv, - WorkerBackend: resolved.WorkerBackend, - ConfigStoreConn: resolved.ConfigStoreConn, - ConfigPollInterval: resolved.ConfigPollInterval, - InternalSecret: resolved.InternalSecret, - InternalSecretFallbacks: resolved.InternalSecretFallbacks, - ReadOnlySecret: resolved.ReadOnlySecret, - ReadOnlySecretFallbacks: resolved.ReadOnlySecretFallbacks, - UserSecretKey: resolved.UserSecretKey, - SNIRoutingMode: resolved.SNIRoutingMode, - ManagedHostnameSuffixes: resolved.ManagedHostnameSuffixes, - MetadataHostnameSuffixes: resolved.MetadataHostnameSuffixes, - MetadataProxyMaxConns: resolved.MetadataProxyMaxConns, - DucklingBucketSuffix: resolved.DucklingBucketSuffix, - DuckLakeDefaultSpecVersion: resolved.DuckLakeDefaultSpecVersion, - - AdmissionReclaimerMaxReservations: resolved.AdmissionReclaimerMaxReservations, - K8s: controlplane.K8sConfig{ - WorkerImage: resolved.K8sWorkerImage, - WorkerNamespace: resolved.K8sWorkerNamespace, - ControlPlaneID: resolved.K8sControlPlaneID, - WorkerPort: resolved.K8sWorkerPort, - WorkerSecret: resolved.K8sWorkerSecret, - WorkerConfigMap: resolved.K8sWorkerConfigMap, - ImagePullPolicy: resolved.K8sWorkerImagePullPolicy, - ServiceAccount: resolved.K8sWorkerServiceAccount, - WorkerCPURequest: resolved.K8sWorkerCPURequest, - WorkerMemoryRequest: resolved.K8sWorkerMemoryRequest, - WorkerNodeSelector: resolved.K8sWorkerNodeSelector, - WorkerTolerationKey: resolved.K8sWorkerTolerationKey, - WorkerTolerationValue: resolved.K8sWorkerTolerationValue, - AllowClientWorkerProfile: resolved.K8sAllowClientWorkerProfile, - WorkerPriorityClassName: resolved.K8sWorkerPriorityClassName, - PlaceholderImage: resolved.K8sPlaceholderImage, - PlaceholderPriorityClassName: resolved.K8sPlaceholderPriorityClassName, - WorkerProfileMinCPU: resolved.K8sWorkerProfileMinCPU, - WorkerProfileMaxCPU: resolved.K8sWorkerProfileMaxCPU, - WorkerProfileMinMemory: resolved.K8sWorkerProfileMinMemory, - WorkerProfileMaxMemory: resolved.K8sWorkerProfileMaxMemory, - WorkerMaxTTL: resolved.K8sWorkerMaxTTL, - WorkerDefaultTTL: resolved.K8sWorkerDefaultTTL, - ExploratoryTierEnabled: resolved.K8sExploratoryTierEnabled, - ExploratoryWorkerCPU: resolved.K8sExploratoryWorkerCPU, - ExploratoryWorkerMemory: resolved.K8sExploratoryWorkerMemory, - ExploratoryWorkerTTL: resolved.K8sExploratoryWorkerTTL, - ReshardPodCPU: resolved.K8sReshardPodCPU, - ReshardPodMemory: resolved.K8sReshardPodMemory, - TrinoBenchmark: resolved.TrinoBenchmark, - AWSRegion: resolved.AWSRegion, - }, - } + cpCfg := configresolve.ControlPlaneConfig(resolved, configresolve.ControlPlaneOverrides{ + Server: cfg, + SocketDir: *socketDir, + ConfigPath: *configFile, + MetricsServer: metricsSrv, + }) controlplane.RunControlPlane(cpCfg) return }