feat: CP-driven destination discovery (additive, static wins) - #57
Merged
Conversation
M4 of the dynamic sourcing plan: with discovery.enabled, viaduck fetches
the duckgres CP's read-only /api/v1/warehouses at startup (scoped
read-only token) and extends the static destination set — one
destination per (warehouse, team), table = the payload's events_table
VERBATIM (the CP owns naming; renames are forbidden upstream), metadata
credentials via a direct k8s Secret read with the ServiceAccount (RBAC
into the tenant namespace; no secret copies, no plaintext payloads).
Semantics: additive, static wins routing collisions (cutover = delete
the static entry; gap-free only when the static id already matches
org-<org>-team-<id>), destination set fixed until the runtime-mutable
workstream; a background DriftWatcher detects added/removed/CHANGED
(content equality over the mapped view — a moved metadata endpoint
after a reshard is a standing signal, not silence) and escalates a
startup org turning unwritable (reshard fence) with the lifecycle-pause
instruction. Discovered destinations initialize at the source head
regardless of pipeline seed_mode (discovery starts the stream, never
backfills) with convention defaults (projection on, captured_at
dropped).
Fail-open/fail-safe: any startup discovery failure degrades to
static-only with discovery_synced=0 (startup-owned — a later drift-poll
success cannot clear the alert); per-entry problems skip-and-count
(broken_entries_total{reason}) incl. pre-backfill cnpg rows, payload
dupes, allowlist violations; materialization is deadline-bounded with
liveness heartbeats so a blackholed k8s API cannot crashloop static
tenants; the drift thread survives any exception.
Two adversarial review rounds (four agents, empirical verification):
- ATTACH conninfo survives BOTH parse layers — libpq keyword form
(round 1: postgresql:// URLs hit DuckDB's FILE backend) with
SQL-doubled quotes (round 2: pyducklake embeds the string raw in
ATTACH '...'); a regression test runs a real Catalog ATTACH.
- DestinationPool is constructed AFTER the config merge (round 2: a
pool holding the pre-merge cfg let discovered dests buffer forever
without flushing — the third stale-captured-config defect this
effort; consumers are now constructed after the last cfg rebind).
- Credential hygiene: connect failures re-raise DestinationConnectError
from None with password scrubbing (kv/quoted/dict-repr/URL forms);
postgres_uri_direct is repr=False; error text provably secret-free.
- Spoofed-payload defense: endpoint + secret-namespace allowlists
(default .ducklings.svc* / ducklings); k8s reads validate RFC-1123
grammar (SA-token path traversal), explicit missing-CA error.
destination._create ensures the table's namespace before creation
(per-team schemas like evilco.events don't pre-exist). 60 new tests;
just ci green (609 unit + 79 integration, semgrep 0).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL on the PR, both classes valid: - py/redos: the quoted-value pattern (?:\\.|[^'])* is ambiguous (a backslash matches both alternatives) and backtracks exponentially on an unclosed quote + escape pairs — attacker-influenceable exception text in the flush path. Replaced with the unrolled-loop form [^'\\]*(?:\\.[^'\\]*)* in a shared viaduck/scrub.py; a timing test pins linearity on 20k-escape hostile input. - py/clear-text-logging: _broken's detail can derive from exception text raised near secret handling; SecretReadError messages are designed secret-free, but scrubbing at the log boundary makes that a structural property instead of a promise.
CodeQL's remaining clear-text-logging alerts trace to the missing-key error listing sorted(data) — key NAMES, but drawn from the tainted secret payload dict. Report a count (int, taint-free) instead; the boundary scrub in _broken stays as defense-in-depth but the flow now ends at the raise. Test pins that key names never reach the message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
M4 of the dynamic team-sourcing plan: with
discovery.enabled, viaduck fetches the duckgres CP's read-only/api/v1/warehousesat startup (scoped read-only token — never the internal secret) and extends the static destination set. One destination per (warehouse, team): table = the payload'sevents_tableverbatim (team 666 withschema_name: evilco→evilco.events; the CP owns naming, renames forbidden upstream), routing value = team id, metadata-store credentials via a direct k8s Secret read with viaduck's ServiceAccount (RBAC intoducklings— no secret copies, no plaintext payloads).destination._createensures the per-team namespace before table creation.Semantics
org-<org>-team-<id>so the cursor row carries over — documented). Runtime add/remove stays the next workstream; the C1 lifecycle machinery governs discovered destinations exactly like static ones (CP removal → operatordrainingtoday, automated in C3).seed_mode: discovery starts the stream, never backfills.discovery_synced=0(startup-owned; a later drift-poll success cannot clear the alert); per-entry problems skip-and-count (broken_entries_total{reason}) including pre-backfill cnpg rows and payload duplicates; materialization is deadline-bounded with liveness heartbeats (a blackholed k8s API cannot crashloop static tenants);min_destinationsrefuses a suspiciously empty payload; the drift thread survives any exception.Review
Two full adversarial rounds (four agents), with empirical verification as the standard — both rounds' worst findings were caught by running code, not reading it:
postgresql://URLs fall through to DuckDB's file backend. R2 FATAL: the fix's libpq quoting broke pyducklake's rawATTACH '…'embedding at the SQL-literal layer. Final form — libpq keyword conninfo with SQL-doubled quotes — validated against a live Postgres (auth reached, special-char passwords intact) and pinned by a regression test that runs a realCatalogATTACH.DestinationPoolcaptured the pre-merge config — discovered destinations could buffer but never flush (permanent rewind/re-read churn). Pool now constructed after the last cfg rebind, with the pattern named in a comment (third stale-captured-config defect across this effort).DestinationConnectErrorfrom None(suppression proven through the real log path);postgres_uri_directisrepr=False; scrubber handles kv/quoted/dict-repr/URL forms including@-bearing passwords..ducklings.svc*/ducklings) — the payload directs credential reads, so it gets a stricter standard than millpond's filter values; README documents the https guidance.Known follow-ups
run()-level integration test of the merge→pool→flush seam (currently guarded by structure + comment)Testing
just ciexit 0: 612 unit (60 new: mapping contracts incl. verbatim-table and enabled-ignored, materialize collisions/dedup/allowlists/deadline, real-ATTACH parse regression, scrub +from Nonesuppression, k8s reader incl. path-traversal and missing-CA, drift watcher incl. synced ownership and recovery-after-failed-startup) + 79 integration + semgrep 0.