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..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 @@ -1201,6 +1222,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/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/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..acf63c89 100644 --- a/main.go +++ b/main.go @@ -363,68 +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, - AWSRegion: resolved.AWSRegion, - }, - } + cpCfg := configresolve.ControlPlaneConfig(resolved, configresolve.ControlPlaneOverrides{ + Server: cfg, + SocketDir: *socketDir, + ConfigPath: *configFile, + MetricsServer: metricsSrv, + }) controlplane.RunControlPlane(cpCfg) return } 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