feat(ingest): add hotdata ingest command group#206
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let body = serde_json::to_value(req).expect("IngestRequest serializes"); | ||
| self.send( | ||
| self.authed(reqwest::Method::POST, "/sources").json(&body), | ||
| Some(&body), |
There was a problem hiding this comment.
Blocking (security): --debug leaks source credentials in plaintext.
create_source passes the full serialized IngestRequest as the body_log argument to send. util::send_debug → debug_request prints that body verbatim (see util.rs:60-65); it only masks the Authorization header and only redacts response fields via TOKEN_REDACT_KEYS. It never redacts request-body fields.
So hotdata ingest sql --url postgresql://user:PASSWORD@host/db --debug (or --config with a password, rest_config with a bearer token, catalog_config with a catalog token, or filesystem aws_secret_access_key) prints the secret in cleartext to stderr — exactly the output users paste into bug reports and CI logs.
This contradicts the repo's established convention: jwt.rs builds a redacted_form_body(¶ms) and passes that as the log body precisely because body_for_log is printed unredacted (its doc: "pass ... a hand-rolled redacted Value for form bodies"). The design doc §3 also promised token redaction here.
Fix: build a redacted clone of the body for logging (the redact_json_fields helper already exists), e.g. redact credentials/rest_config/catalog_config before passing to send, rather than Some(&body).
There was a problem hiding this comment.
Review
Blocking Issues
src/client/ingest.rs:204 — source credentials leak in plaintext under --debug.
create_source passes the full unredacted IngestRequest as the debug log body. util::send_debug/debug_request print request bodies verbatim (they only mask the Authorization header and only redact response fields via TOKEN_REDACT_KEYS). This exposes SQL passwords/connection strings, REST bearer tokens, Iceberg catalog tokens, and filesystem secret keys to stderr — the output users routinely paste into issues and CI logs. The rest of the CLI already avoids this (jwt.rs passes a redacted_form_body), and the design doc §3 promised redaction here.
Action Required
- Redact secret-bearing fields (
credentials,rest_config,catalog_config) from the request body before passing it tosendfor debug logging — reuse the existingredact_json_fieldshelper. Apply the same treatment to any other enqueue body that can carry secrets.
Drives the dlthubworker ingest API (/v1/ingest/*) through the gateway via a raw-HTTP client (routes aren't in the SDK yet): sql/rest/filesystem/ iceberg onboarding, restricted SQL queries, translate, connector catalog, status, drain, schema, preview, and parquet download. Reconciled against the stable post-gateway API (2026-07-07 rollout): - enqueue (POST /sources|/queries) sends the durable hd_ API key as the bearer — the server rejects JWTs there because the drain job outlives them; without an API key the CLI fails fast with a --api-key hint - verified live: the CLI's user-scoped session JWT only reaches /connectors — workspace-scoped endpoints 403 (the worker derives the workspace from the JWT's workspace:* scope and ignores X-Workspace-Id on the JWT route), so those 403s get an --api-key hint too - no `ingest sources` command: GET /ingest/sources never shipped - 2.5s status polling, drain re-kick after 30s pending (control-store read lag), brief 404 retry on `ingest query` right after onboarding, and a database-scoped-token hint when a load fails with Forbidden
eddietejeda
force-pushed
the
feat/ingest-commands
branch
from
1ee3697 to
8419569
Compare
| let body = serde_json::to_value(req).expect("IngestRequest serializes"); | ||
| self.send( | ||
| self.authed(reqwest::Method::POST, "/sources").json(&body), | ||
| Some(&body), |
There was a problem hiding this comment.
Blocking (security): still leaks source credentials in plaintext under --debug.
This is unchanged since the prior review. create_source passes Some(&body) — the full serialized IngestRequest — to send, and util::send_debug → log_request_struct → debug_request (util.rs:60-65) prints the request body verbatim. Only the Authorization header is masked; request-body fields are never redacted. redact_json_fields is still private in util.rs and is not used here.
So hotdata ingest sql --url postgresql://user:PASSWORD@host/db --debug (and rest_config bearer tokens, catalog_config tokens, filesystem aws_secret_access_key) still prints the secret to stderr — the exact output users paste into bug reports and CI logs.
Fix: build a redacted clone of the body for logging (mask credentials/rest_config/catalog_config) and pass that instead of Some(&body). This mirrors jwt.rs, which builds redacted_form_body(¶ms) precisely because the log body is printed unredacted. Making redact_json_fields (or a small wrapper) pub(crate) would let you reuse the existing helper.
There was a problem hiding this comment.
Review
Blocking Issues
src/client/ingest.rs:204—create_sourcestill passes the full unredactedIngestRequestbody (Some(&body)) to the debug logger, leaking source credentials (credentials,rest_configbearer tokens,catalog_configtokens, filesystemaws_secret_access_key) in plaintext to stderr under--debug. This was flagged in the prior review and remains unfixed.
Action Required
Build a redacted clone of the request body for logging (mask credentials/rest_config/catalog_config) and pass that to send instead of Some(&body), mirroring the redacted_form_body pattern already used in jwt.rs. The existing redact_json_fields helper in util.rs can be reused if exposed as pub(crate).
The enqueue body's credentials / rest_config / catalog_config subtrees (SQL passwords, connection strings, REST bearer tokens, catalog tokens, object-store keys) were printed verbatim to stderr under --debug. Log a view with those subtrees replaced by "***" instead; the wire body is unchanged. Mirrors jwt.rs's redacted_form_body pattern.
|
Fixed in |
There was a problem hiding this comment.
Cycle 3: prior blocking credential-leak issue in create_source is resolved — the enqueue body is now passed through redact_secret_fields before logging, masking credentials/rest_config/catalog_config (covering SQL passwords, filesystem aws_secret_access_key, REST bearer tokens, and Iceberg catalog tokens). Other body-carrying calls carry no secrets. LGTM.
…t/refresh
Collapse the sprawling 13-subcommand surface into the same shape as
`hotdata connections`, focused on a guided, catalog-driven onboarding
flow:
new interactive wizard, driven by the connector catalog
list the sources you've ingested (result databases)
create scriptable onboarding; `create list` browses the catalog
import SQL front-door (renamed from `query`); defaults to a
reasonable sample (SELECT * FROM <source> LIMIT 1000) when
no query is given
refresh re-drain + poll an ingest to completion
The `new` wizard is catalog-driven: the /ingest/connectors catalog
returns a `template` per REST service with base_url, auth shape, and
resources already filled in, so the wizard prompts only for the
`<PLACEHOLDER>` secrets — the user never types a URL the catalog knows.
Keyless services (auth "none") onboard with zero prompts.
Dropped as separate verbs: sql/rest/filesystem/iceberg (folded into
new/create by family), translate, connectors (→ `create list`),
status/drain (folded into the run+poll loop), schema/preview/download
(the read side is the core `query`/`databases`/`results` commands).
Client: ConnectorEntry now carries `auth` + `template`; dropped the
translate/schema/preview/download methods and types.
Catalog display drops redundant SQL dialect aliases (the server lists
both postgres/postgresql and mssql/sqlserver with identical labels):
the wizard and `create list` show the canonical name only, while
`create --service <alias>` still resolves against the full catalog.
Verified live against prod: `create list`, `list`, request assembly
from the catalog template, `import` default-query synthesis and
id-pinning, and `refresh` (drain + poll). A full `POST /sources`
onboard currently hangs server-side in the deploy path (unrelated to
the CLI — /queries, /drain, /jobs, /connectors all respond fast).
A slow enqueue (first-per-workspace runtime deploy) or a worker rollout makes the gateway hold the connection to its timeout, surfacing as the opaque 'error sending request'. Add a dark-grey hint pointing at the likely cause (service starting up / redeploying) and that a timed-out enqueue is safe to re-run.
`auth status` showed the first workspace from the live /workspaces API while `resolve_workspace` fell back to config.yml's first — so for a multi-workspace API key the status readout named a different workspace than commands used. Add one `credentials::default_workspace_id` helper (single-workspace token pins its own; multi/unrestricted key honors the saved default when it can reach it, else the credential's first authorized workspace) and use it from both paths. Drop the now-unused `config::resolve_workspace_id`.
…rce) The redundant postgres/postgresql and mssql/sqlserver catalog entries are now collapsed in dlthubworker's list_connectors, so the CLI no longer needs to dedupe on display. Keep only the family sort.
There was a problem hiding this comment.
Cycle 1–2 blocking issue resolved: create_source now redacts credentials/rest_config/catalog_config via redact_secret_fields before passing the body to --debug logging (src/client/ingest.rs:224, helper at :272), so source secrets no longer leak in plaintext. Verified create_query's unredacted body carries no credential fields.
`ingest new`/`create` now run in the server's validate_only mode: connect, reflect the schema, load NO rows. On completion they print the discovered tables + columns and point at `ingest import` to pull data. Dropped the --validate-only flag (it's the only behavior now); re-added the client schema() call to fetch the discovered metadata; poll_ingest returns the terminal status so each verb renders its own result (connection-added vs imported).
poll_ingest now reflects every non-terminal status in the spinner (stage + free-text detail, e.g. 'importing… loading — region (5 rows)') instead of a static label, and no longer treats an unrecognized status as fatal — so it surfaces whatever stage-level states the worker reports as a load advances. Only done/failed end the loop; timeout still exits 2.
One add-connection verb instead of two: `ingest new` runs the guided wizard when no --service is given on a terminal, and is flag-driven otherwise (--service given, a non-TTY, or --no-input) — so the non-interactive path is just `new --service … --config …`. Removed `create` (and `create list`); catalog browsing is now its own `ingest connectors` command. Surface: new / connectors / list / import / refresh.
Split the `new` doc comment so the first line is the short help shown in the command list; the detail (interactive vs --service, import/connectors pointers) moves to the long help in `ingest new --help`. Trim the `connectors` summary too.
Renames the command, its handler, and every user-facing hint (the ack 'Track it with' and the poll timeout 'Check status with'). Surface: new / connectors / list / import / update.
Add a STATUS column to 'ingest connectors' showing 'active' for connectors this workspace has already added, by cross-referencing the catalog against the ingest-/query- managed DBs (the same derivation 'ingest list' uses; factored into ingest_db_source). Best-effort — an unavailable DB list just omits the marks. JSON/YAML gain an 'active' boolean.
… true re-run
Rename the ingest verbs around the two nouns users actually work with —
connections (onboarded sources) and imports (materialized DBs):
new -> new-connection list -> list-connections [--all]
import -> new-import (--all|SQL) + list-imports
update -> trigger-import connectors unchanged
(supported-connectors alias)
Old names remain as hidden aliases for one release.
Backed by the new worker endpoints (dlthubworker#69):
- list-connections reads GET /ingest/sources: each connection shows its
own ingest id (the pin key new-import accepts), family, status, and
created date — replacing the ingest-*/query-* DB-name pattern-match
heuristic, which is deleted (along with databases::list_id_name_pairs,
added only for it). --all includes superseded onboards. The connectors
catalog "active" marks now come from the same registry.
- list-imports reads GET /ingest/queries: the SQL behind each import,
its source connection, result database, and status.
- trigger-import calls POST /ingest/jobs/{id}/rerun — a real re-run
(reset to pending + drain; replace-mode load refreshes the same DB
with the stored credentials), not the old re-drain-and-poll which was
a no-op for done ingests. Works on connections too (re-validate +
refresh schema). The poll loop skips its up-front drain kick on this
path (the rerun already drained).
- new-import makes the two modes explicit: --all (SELECT * with no
LIMIT; resolves a pinned connection id to its FROM name via the
registry) or SQL. The implicit 1000-row sample default is gone —
omitting both is an error naming the options.
Timeout/footer hints updated for the new verbs.
There was a problem hiding this comment.
Cycle 3: The prior blocking issue (--debug leaking source credentials) is resolved. create_source now logs redact_secret_fields(&body), which masks credentials/rest_config/catalog_config — the subtrees that carry SQL connection strings, filesystem S3 keys, REST bearer tokens, and Iceberg catalog tokens — with test coverage asserting no secret survives into the printable body. create_query's body carries no secrets, so its unredacted log is fine. LGTM.
new-import and trigger-import no longer block the terminal. They enqueue, fire the drain that processes the job (the worker is on-demand -- the old --no-wait skipped this, leaving jobs pending forever), print the ingest id + database + a tracking hint, and return. --wait opts into the old blocking poll. new-connection keeps its blocking default deliberately: its output IS the feedback (credentials validated, schema discovered); --no-wait still opts out there. New `ingest status <id>`: - one-shot by default with script-friendly exit codes mirroring `query status`: 0 done, 1 failed, 2 still in flight; prints the worker's failure detail when there is one - --wait attaches and polls to a terminal state; the poll still re-kicks a job stuck in `pending` (control-store read lag), so attaching also rescues an import whose first drain missed the row The wait poll now tolerates transient errors: a single gateway blip (403/5xx, dropped connection) mid-poll retried up to 3 consecutive times instead of killing the wait (seen live: a one-off empty-body 403 aborted an otherwise-healthy wait). Verified live against prod: non-blocking enqueue returns in ~2s with the pending ack, status one-shot exits 0/2 correctly, status --wait attaches to done.
done -> green, failed -> red, in-flight -> yellow — the repo's status scheme (jobs::get). Table cells are ANSI-safe via tabled's ansi feature.
There was a problem hiding this comment.
Cycle 3: prior blocking credential-leak issue is resolved. create_source now passes a redact_secret_fields view (credentials/rest_config/catalog_config masked) to send instead of the raw body, so --debug no longer prints source secrets. No new blocking issues.
There was a problem hiding this comment.
Prior blocking issue (credential leak under --debug) is resolved: create_source now passes a redact_secret_fields view as the log body, masking credentials/rest_config/catalog_config, with a test confirming no secret reaches the printable body. No remaining blocking issues.
1 participant
Adds a
hotdata ingestcommand group that drives the dlthubworker ingest API (/v1/ingest/*) through the gateway, with a UX modeled onhotdata connections.Command surface
The
newwizard is catalog-driven:/ingest/connectorsreturns atemplateper REST service withbase_url, auth shape, and resources pre-filled, so the wizard prompts only for the<PLACEHOLDER>secrets — the user never types a URL the catalog already knows. Keyless services onboard with zero prompts.importwith no SQL synthesizesSELECT * FROM <source> LIMIT 1000.Transport is a raw-HTTP client (
src/client/ingest.rs) since these routes aren't in the generated SDK yet.Auth model
create/import) uses the durablehd_API key as the bearer — the server 422s JWTs there (the drain job outlives them). Without a key the CLI fails fast with a--api-keyhint; read paths work on the session JWT.--debugredacts source secrets (credentials/rest_config/catalog_config) from the logged request body.error sending request.Also in this branch
auth statusreported the first workspace from the live/workspacesAPI whileresolve_workspacefell back toconfig.yml's first — so a multi-workspace API key made the status readout name a different workspace than commands actually used. Both now share onecredentials::default_workspace_idhelper.postgres/postgresql,mssql/sqlservercatalog entries) is fixed at the source in dlthubworker'slist_connectors— see the companion PR in that repo. This branch carries no display-side workaround.Testing
--debugredaction). Full suite green; clippy/fmt clean.create list,list, request assembly from catalog templates,importquery synthesis + id-pinning,refresh(drain + poll), and the workspace fix (auth statusnow matches theX-Workspace-Idcommands send). A fullPOST /sourcesonboard is slow (~190s) / occasionally exceeds the 300s gateway timeout during worker rollouts — a dlthubworker-side issue, not the CLI.