Report PII-free error templates in deploy telemetry - #6355
Draft
denik wants to merge 11 commits into
Draft
Conversation
Deploy telemetry reports error_message, which is scrubbed by a path/PII regex deny-list and still treated as privileged, so it cannot be aggregated. Add an allow-list counterpart: an error keeps its format string, with everything the user supplied left as a verb, and that is what gets reported. libs/safeerr wraps fmt.Errorf and retains the template. Error() is byte-identical to what fmt.Errorf would have produced and the unwrap chain is unchanged, so converting a call site changes nothing for existing consumers. Templates chain through %w. Values are unsafe unless marked Safe; a value that is only partly user-authored supplies its own stand-in via SafeStringer, which is how config.ResourceKey reports resources.jobs.* without the resource name. Backend errors are unsafe: an API message echoes resource names, workspace paths and principals. SafeAPIErrorDescription reports only the structured fields, and requires error_code to look like an enum member so free text cannot ride along. The template reaches telemetry through diag.Diagnostic.ErrorTemplate, since logdiag keeps only the diagnostic and drops the error value. It lands in experimental.string_values, mirroring bool_values so later keys need no proto change. Converted the migrate path, which also records why an automatic migration to the direct engine failed rather than only that it did. The universe proto field is a follow-up; until it lands the field is dropped on ingestion. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: ec7c5be
7 interesting tests: 4 SKIP, 2 flaky, 1 RECOVERED
Top 11 slowest tests (at least 2 minutes):
|
…tests The inline `jq 'select(...)' out.requests.txt` pipelines duplicated what print_migration_telemetry and print_telemetry_string_values already print from the same recorded request, and .agents/rules/testing.md calls that pipeline out as an anti-pattern precisely because the helpers exist. Removing out.telemetry.txt also drops a synced file, hence the Files: count in the migrate golden. Co-authored-by: Isaac
…g map The string_values map was justified as mirroring bool_values so later keys would need no proto change, but adding the map is itself a proto change, so that cost is paid either way and the flexibility only covers keys that do not exist. Three known slots are better as named fields: a typo is a compile error, the struct documents what exists, and the backend gets real columns instead of a map to explode. Splitting the migration template in two also drops a join: the field names match the booleans recorded beside them, so a conversion failure is distinguishable from a commit failure without consulting bool_values. Co-authored-by: Isaac
Every resource key starts with it, so it carries no information in a template: "jobs.*" says what "resources.jobs.*" said. Co-authored-by: Isaac
The same two values were rendered in opposite orders by two functions in the same package: the user-facing summary reads "(403 INJECTED)" and the template read "INJECTED 403". Co-authored-by: Isaac
A filer error is a fixed classification plus the path it concerns: "access denied: /Workspace/...". The classification is a source literal and safe to report; the path is not. Naming the literals lets Error() and SafeString() derive from one string so they cannot drift, and leaves all 47 construction sites untouched. safeerr consults a wrapped error's stand-in under %w when it has no template of its own, and diag.ErrorTemplate consults it at the top of the chain, so this reports usefully from the call sites not yet raised through safeerr — which is most of them: before pushing direct state to workspace: %w [403 INJECTED] after pushing direct state to workspace: access denied [403 INJECTED] It matters most where the HTTP status cannot discriminate: a missing file and a missing parent directory are both 404. Co-authored-by: Isaac
Only the auto-migrate tests asserted what migration telemetry a deploy reports; the rest exercised migration scenarios while the telemetry went unobserved. Every migration test that records requests now prints it, so a change in what a scenario reports shows up as a golden diff. print_migration_telemetry collapses identical lines, since out.requests.txt can hold several deploys and repeating a line per deploy says nothing, and reports missing requests rather than letting jq complain about the file. Four tests are left out: default-python, profile_arg and var_arg set RecordRequests = false, and the two engine-config tests run only a failing "deployment migrate", which is not a deploy and emits no event. Also table-tests the conversion failures in build_state_test.go, which only covered success paths, pinning each message against its template. Co-authored-by: Isaac
…ploy Rather than collapsing repeated lines, each block is printed right after the deploy it belongs to and the recorded requests are cleared, so a block covers exactly one deploy. default-python, profile_arg and var_arg now record requests too, and remove them once the telemetry is printed, so nothing large lands in a golden. default-python needs withdir: its script cd's into the generated bundle while the requests are recorded at the test root. Three blocks come out empty on purpose. Those deploys run after the state is already direct, and MigrateToDirect is skipped entirely for a direct deploy, so the empty block is the assertion. Co-authored-by: Isaac
Deploy errors are a separate concern from migration, so error_template and the plumbing it needed are gone: diag.Diagnostic's field, logdiag's FirstErrorTemplate, Metrics.ErrorTemplate, the proto field and the deploy-error-template test. That also reverts the exhaustruct additions in load_dbalert_files.go. Only direct_migrate_error_template and direct_migrate_commit_error_template remain. Telemetry logging is reverted from the migration tests where it only showed direct_drymigrate_success true, which fifteen other tests already assert. Nothing covered the conversion failure class, only commit failures. The new auto-migrate-conversion-failure test uses an unknown secret scope permission level: a warning for the terraform deploy, but SecretScopeFixups rejects it while preparing the config for the direct engine. It asserts both populations — a failed dry run when not opted in, and a stopped migration when opted in. A conversion failure makes no API call, so unlike the commit failures it cannot be produced by injecting a fault. Co-authored-by: Isaac
The migration's output preceded the deploy's own summary, so warnings from post-deploy work read as belonging to the deploy: Uploading bundle files to ... Warn: post-deploy dry-run migration to direct: ... Created secret_scopes.my_scope Files: 5 uploaded, 0 deleted The summary is deliberately printed last, so the migration moves rather than the summary: out of deployCore, which no longer needs requestedEngine, to the end of Deploy. It stays gated on the deploy alone, which the early return above already guarantees, and not on the postdeploy script: the resources were applied before that script ran, so the state is worth migrating even if it failed — the same reasoning that prints the summary regardless. Co-authored-by: Isaac
The fixups ran through their mutator, so the failure arrived as a diagnostic and the migration could only report a generic "failed to apply secret scope fixups" — losing which fixup failed, and printing two warnings for one problem. ApplySecretScopeFixups is now the body, returning a real error and the offending scope key, with the mutator as a wrapper around it. The migration calls it directly, so its safeerr template survives: before direct_migrate_error_template failed to apply secret scope fixups after direct_migrate_error_template unknown permission level %q for secret scope Only the level itself is withheld; the rest of the message is a literal. Co-authored-by: Isaac
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.
Changes
libs/safeerrretains an error's format string — the template — with everything the user supplied left as a verb.Error()is byte-identical tofmt.Errorfand the unwrap chain is unchanged, so converting a call site is invisible to existing consumers. Templates chain through%w. Values are unsafe unless markedSafe; a value that is only partly user-authored supplies its own stand-in viaSafeStringer, which is howconfig.ResourceKeyreportsjobs.*without the resource name.diag.SafeAPIErrorDescriptionreports an API error'serror_codeand status and nothing else, requiring the code to look like an enum member so free text cannot ride along.The template reaches telemetry as
diag.Diagnostic.ErrorTemplate—logdiagkeeps the diagnostic and drops the error value — and lands in three dedicatedexperimentalfields:error_template,direct_migrate_error_template, anddirect_migrate_commit_error_template. The migration pair is split so its field names match the booleans recorded beside them, and a conversion failure is distinguishable from a commit failure without joining againstbool_values.The CLI's own typed errors describe themselves: each
libs/filererror is a fixed classification plus a path, so it contributes the classification and withholds the path.safeerrconsults that under%wwhen the wrapped error has no template, anddiag.ErrorTemplateconsults it at the top of the chain — so it reports usefully from call sites not raised throughsafeerr, which is most of them.Converted the migrate path, so a failed automatic migration to the direct engine reports why rather than only that it failed.
bundle/directis not converted here.Why
error_messageis scrubbed by a path/PII regex deny-list and is still treated as privileged, so it cannot be aggregated. Same failure, both fields, from the new acceptance test:The deny-list had to mangle a literal to ship anything. For migrations it separates causes that used to be a single boolean, and the filer's classification survives where the status alone would not discriminate — a missing file and a missing parent directory are both 404:
#6321added a hand-maintainederror_categoryenum toSshTunnelEvent. The two are complementary: an enum survives rewording but has to be assigned per site, which does not scale to the ~210 error sites in the direct engine, while a template costs nothing per site.Tests
Unit tests cover the message being identical to
fmt.Errorf, chaining,Safe/SafeStringerprecedence, the bail-out on%[1]sand%*d,error_codeshape rejection, and — the point of the package — that no unmarked value reaches a template or is retained by the error.New
acceptance/bundle/telemetry/deploy-error-templateasserts both fields against the telemetry request the CLI actually recorded, and the migrate goldens do the same for the migration templates.go vetcannot infer thatsafeerr.Errorfis a printf wrapper, because it does not forwardargsverbatim, so it carries the documentedif false { _ = fmt.Errorf(...) }marker. Checked with a throwaway package that a wrong verb, a short arg list, and a non-constant format are all reported.The universe proto fields are a follow-up; until they land the fields are dropped on ingestion.
This pull request and its description were written by Isaac.