GeezerVison is showing github.com — [pipeline-manager] Redact connection errors and bundle failures by bestouff · Pull Request #7788 · feldera/feldera
Conversation
bestouff
commented
Nov 2, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Part of feldera/cloud#2136. Stacked on #7787 (cloud-2136-redaction).
The manager can return credentials from diagnostics written by older runtimes or retained support collections. Scrub connection credentials when writing and reading diagnostics and when exporting collections with an older policy. Preserve operational configuration, SQL, UDFs, error codes and the existing bundle schemas.
- Recognize encoded password parameters, whitespace in URL userinfo, and escaped or multiline keyword values. Preserve unrelated text when credential boundaries are known; omit ambiguous diagnostics. Include JSON keys and nested details.
- Advance collection policy to 8 and re-scrub policies 0–7, including missing versions and
collect=false exports. Undecodable payloads are omitted with an actionable reason.
- Project the pipeline record explicitly, and scrub logs, stats, events and collection failure reasons in both manifests. Inline SQL/configuration policy remains the separate INT-31152 decision.
Validation: 47 manager regression tests pass with embedded PostgreSQL under a non-root user. The old scrubber from 8fb678c0c (unchanged at rebased 50c6a3e1b) fails seven assertions covering syntax, database serialization, retained MessagePack exports and both manifests. Trusting policy 7 fails the new retained-collection test. Formatting and Clippy with warnings denied pass.
Enterprise/Fremont validation passed; results are recorded in feldera/cloud#2160. This change does not rewrite historical database rows or external copies; production cleanup and credential rotation remain separate work.
Live validation also verified stored runtime diagnostics/events and read-side scrubbing of an injected legacy row without a database rewrite. SQL compiler source excerpts can retain inline credentials, as original SQL does; that remains part of INT-31152.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Review summary: changes requested (because of the logs.txt bug; see the inline comments)
| What I ran |
Result |
cargo test --locked -p pipeline-manager --lib on the PR's 13 new tests (redaction, parsing, bundle and collector) |
✅ pass |
pre-commit run --files <changed files> |
✅ pass |
| My own QA tests (not committed) |
❌ 3 of 4 fail: whole logs wiped by a URL followed later by @, whole logs wiped by one password= line, JSON key collision drops a value. MySQL userinfo with an IPv6 host is handled correctly. |
Missing tests (medium):
- A multi-line
logs.txt where only one line holds a credential. The other lines must survive.
- Over-redaction cases: a URL with no userinfo followed by an email address, and a non-secret
password= mention.
- Live API: a
GET /pipelines/{name} round-trip that checks the returned deployment_error is redacted. Only the parsing function is unit-tested.
- I did not run the database-backed test
collected_record_and_events_carry_no_url_password (it needs embedded Postgres). The upgrade test against rows from a pre-fix release is not done, as the PR description says.
Gates: tests are present, I saw no flakiness risk, docs and changelog are updated, there are no new dependencies and no unsafe. The commit message is fine.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
What I ran
| Check |
Result |
cargo test -p pipeline-manager --lib filtered to diagnostic_redaction, pipeline_parsing, support_bundle, support_data_collector, collected_record_and_events (embedded PG, non-root) |
52 passed, 0 failed |
pre-commit run on the changed files |
all passed |
Scratch QA test on redact_connection_credentials (not committed) |
found the truncation bug (see inline comment) |
Request changes
- bug (high): any
host:port URL followed later by an @ cuts off the rest of the text. A whole logs.txt can lose everything after its first http://localhost:8080 line. Details are inline.
- Commit history gate (medium): the second commit only fixes the first ("Restore complete credential scrubbing"). Please squash them before merge. The skipped versions 6 and 7 are covered inline.
Test cases the PR doesn't cover
- Logs with a URL that has a port, plus an unrelated
@ later in the text: the PR's own logs test happens to use s3://bucket/..., which has no port.
- The same thing inside
pipeline_events / stats strings.
passwd= and JSON-style "password": "..." are not scrubbed. That may be by design, but the docs only promise password=, so a sentence saying the other forms are out of scope would help.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Old runtimes and retained diagnostic records can contain connection
credentials. Scrub URL userinfo, including quoted passwords, and the
value of a password parameter on error write, diagnostic read,
collection, and export. Replace only the password token, so the rest
of a diagnostic or a log stays readable. This covers encoded password
parameters and escaped or multiline keyword values.
Where the end of a credential cannot be proven, cut the text off at
the credential instead of exposing a suffix or dropping the whole
diagnostic. A port after a host is not a userinfo colon.
A URL that holds an `@` is complete: a later `@`, such as an email
address in the same log, does not cut the log off. The cost is a
password with both an unencoded `@` and whitespace, which is replaced
only up to its first `@`: postgres://u:pa@ss word@host/db becomes
postgres://u:[REDACTED]@ss word@host/db. The docs state this limit.
Sanitize failed collection requests too: their reasons are copied into
both bundle manifests even when no file payload was collected. Project
all existing pipeline-record fields explicitly, preserving SQL, UDFs,
and operational configuration. Collection version 6 marks this policy;
older or unversioned data is sanitized on export. Exported file schemas
and stored MessagePack field layout stay unchanged.
Tests cover supported Postgres forms, database serialization and
collection, unchanged record projection, stored MessagePack
collections and their export, fresh collection failures in both
manifests, and unversioned records. Document the scope and historical
limits.
Related: feldera/cloud#2136
Signed-off-by: Xavier Bestel <[email protected]>
This branch has not been deployed
No deployments
Reviewers
mihaibudiu
Awaiting requested review from mihaibudiu
At least 1 approving review is required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|