From f9605f4f21ba7574c2176901294ba34b5fb11870 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 24 Aug 2026 10:50:34 +0200 Subject: [PATCH 01/11] Report PII-free error templates in deploy telemetry 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 --- .../out.telemetry.txt | 9 + .../auto-migrate-push-failure/output.txt | 3 +- .../migrate/auto-migrate-push-failure/script | 6 + .../auto-migrate-tfbackup-failure/output.txt | 1 + acceptance/bundle/migrate/script.prepare | 11 +- .../deploy-error-template/databricks.yml | 7 + .../deploy-error-template/out.telemetry.txt | 9 + .../deploy-error-template/out.test.toml | 2 + .../deploy-error-template/output.txt | 19 + .../telemetry/deploy-error-template/script | 20 + .../telemetry/deploy-error-template/test.toml | 5 + acceptance/script.prepare | 8 + bundle/bundle.go | 14 + bundle/config/mutator/load_dbalert_files.go | 52 +- bundle/config/resource_key.go | 39 ++ bundle/config/resource_key_test.go | 57 +++ bundle/metrics/metrics.go | 16 + bundle/migrate/build_state.go | 26 +- bundle/migrate/resolve.go | 15 +- bundle/migrate/tf_state.go | 16 +- bundle/phases/telemetry.go | 1 + bundle/statemgmt/direct_migration.go | 52 +- cmd/bundle/utils/process.go | 9 + libs/diag/diagnostic.go | 36 +- libs/diag/error_template_test.go | 122 +++++ libs/diag/sdk_error.go | 37 ++ libs/logdiag/logdiag.go | 18 + libs/safeerr/safeerr.go | 269 ++++++++++ libs/safeerr/safeerr_test.go | 463 ++++++++++++++++++ libs/telemetry/protos/bundle_deploy.go | 17 + 30 files changed, 1282 insertions(+), 77 deletions(-) create mode 100644 acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt create mode 100644 acceptance/bundle/telemetry/deploy-error-template/databricks.yml create mode 100644 acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt create mode 100644 acceptance/bundle/telemetry/deploy-error-template/out.test.toml create mode 100644 acceptance/bundle/telemetry/deploy-error-template/output.txt create mode 100644 acceptance/bundle/telemetry/deploy-error-template/script create mode 100644 acceptance/bundle/telemetry/deploy-error-template/test.toml create mode 100644 bundle/config/resource_key.go create mode 100644 bundle/config/resource_key_test.go create mode 100644 libs/diag/error_template_test.go create mode 100644 libs/safeerr/safeerr.go create mode 100644 libs/safeerr/safeerr_test.go diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt b/acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt new file mode 100644 index 00000000000..4e501c95795 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt @@ -0,0 +1,9 @@ +{ + "error_message": null, + "string_values": [ + { + "key": "direct_migrate_error_template", + "value": "pushing direct state to workspace: %w [INJECTED 403]" + } + ] +} diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt index 74412d94a92..27a8ab5bfb0 100644 --- a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt +++ b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt @@ -21,6 +21,7 @@ Resources: 0 created, 0 changed, 0 deleted, 1 unchanged >>> print_migration_telemetry direct_migrate_commit_error true +direct_migrate_error_template pushing direct state to workspace: %w [INJECTED 403] === Local state was NOT rewritten (still terraform) @@ -35,7 +36,7 @@ Warn: Direct engine requested in DATABRICKS_BUNDLE_ENGINE environment variable b Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... Migrating state to direct deployment engine (opted in via DATABRICKS_BUNDLE_ENGINE environment variable)... Migrated 1 resource to direct deployment engine. -Files: 2 uploaded, 0 deleted +Files: 3 uploaded, 0 deleted Resources: 0 created, 0 changed, 0 deleted, 1 unchanged >>> print_migration_telemetry diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/script b/acceptance/bundle/migrate/auto-migrate-push-failure/script index b1ccb759da5..08cfb53d345 100644 --- a/acceptance/bundle/migrate/auto-migrate-push-failure/script +++ b/acceptance/bundle/migrate/auto-migrate-push-failure/script @@ -12,6 +12,12 @@ title "Opt in via env var; inject 403 on the resources.json push so commit fails fault.py "POST /api/2.0/workspace-files/import-file/Workspace/Users/$CURRENT_USER_NAME/.bundle/test-bundle/default/state/resources.json" 403 0 1 trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy trace print_migration_telemetry + +# Record the migration error fields exactly as they leave the machine. The deploy +# itself succeeded, so error_message is absent and the only thing reported about +# the failed migration is the PII-free template — note the workspace path and +# user name in the warning above are nowhere in it. +cat out.requests.txt | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson | {error_message: .entry.databricks_cli_log.bundle_deploy_event.error_message, string_values: .entry.databricks_cli_log.bundle_deploy_event.experimental.string_values}' > out.telemetry.txt rm -f out.requests.txt title "Local state was NOT rewritten (still terraform)\n" diff --git a/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt index 19980cd3060..f18ddbe9d56 100644 --- a/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt +++ b/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt @@ -21,6 +21,7 @@ Resources: 0 created, 0 changed, 0 deleted, 1 unchanged >>> print_migration_telemetry direct_migrate_commit_error true +direct_migrate_error_template pushing direct state to workspace: deleting remote terraform state: %w [INJECTED 403] === Local state was NOT rewritten (still terraform) diff --git a/acceptance/bundle/migrate/script.prepare b/acceptance/bundle/migrate/script.prepare index 9de2f19d437..d72b46b5649 100644 --- a/acceptance/bundle/migrate/script.prepare +++ b/acceptance/bundle/migrate/script.prepare @@ -1,7 +1,10 @@ -# Filter print_telemetry_bool_values output to auto-migration keys -# (direct_drymigrate_*, direct_migrate_*, direct_migrated_via_*). Callers -# clear out.requests.txt themselves; some tests want to keep it for a -# subsequent print_requests.py assertion. +# Filter telemetry output to auto-migration keys (direct_drymigrate_*, +# direct_migrate_*, direct_migrated_via_*), booleans first and then the +# PII-free error template that says which failure a direct_migrate_error or +# direct_drymigrate_success=false was. Callers clear out.requests.txt +# themselves; some tests want to keep it for a subsequent print_requests.py +# assertion. print_migration_telemetry() { print_telemetry_bool_values | grep '^direct_' || true + print_telemetry_string_values | grep '^direct_' || true } diff --git a/acceptance/bundle/telemetry/deploy-error-template/databricks.yml b/acceptance/bundle/telemetry/deploy-error-template/databricks.yml new file mode 100644 index 00000000000..7a3859ce0ea --- /dev/null +++ b/acceptance/bundle/telemetry/deploy-error-template/databricks.yml @@ -0,0 +1,7 @@ +bundle: + name: test-bundle + +resources: + jobs: + my_job: + name: my job diff --git a/acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt b/acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt new file mode 100644 index 00000000000..413016c62da --- /dev/null +++ b/acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt @@ -0,0 +1,9 @@ +{ + "error_message": "access denied: [REDACTED_WORKSPACE_PATH](json) (403 INJECTED)", + "string_values": [ + { + "key": "error_template", + "value": "INJECTED 403" + } + ] +} diff --git a/acceptance/bundle/telemetry/deploy-error-template/out.test.toml b/acceptance/bundle/telemetry/deploy-error-template/out.test.toml new file mode 100644 index 00000000000..0938e678987 --- /dev/null +++ b/acceptance/bundle/telemetry/deploy-error-template/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/telemetry/deploy-error-template/output.txt b/acceptance/bundle/telemetry/deploy-error-template/output.txt new file mode 100644 index 00000000000..d78fe00c770 --- /dev/null +++ b/acceptance/bundle/telemetry/deploy-error-template/output.txt @@ -0,0 +1,19 @@ + +>>> [CLI] bundle deploy -qq +Error: access denied: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/resources.json (403 INJECTED) + +Endpoint: POST [DATABRICKS_URL]/api/2.0/workspace-files/import-file/Workspace%2FUsers%2F[USERNAME]%2F.bundle%2Ftest-bundle%2Fdefault%2Fstate%2Fresources.json?overwrite=true +HTTP Status: 403 Forbidden +API error_code: INJECTED +API message: Fault injected by test. + + +Exit code: 1 + +=== The message carries the workspace path and the user; the template carries neither + +>>> print_telemetry_error_message +access denied: [REDACTED_WORKSPACE_PATH](json) (403 INJECTED) + +>>> print_telemetry_string_values +error_template INJECTED 403 diff --git a/acceptance/bundle/telemetry/deploy-error-template/script b/acceptance/bundle/telemetry/deploy-error-template/script new file mode 100644 index 00000000000..e6a3f15f318 --- /dev/null +++ b/acceptance/bundle/telemetry/deploy-error-template/script @@ -0,0 +1,20 @@ +# error_message is scrubbed heuristically and stays privileged; error_template is +# the PII-free counterpart telemetry can aggregate on. Fail the deploy with an +# API error to show both: no safeerr wrapping is involved here, so the whole +# template comes from the error's structured fields. +fault.py "POST /api/2.0/workspace-files/import-file/Workspace/Users/$CURRENT_USER_NAME/.bundle/test-bundle/default/state/resources.json" 403 0 1 + +# -qq: the file and resource summaries are irrelevant here, and the uploaded-file +# count is unstable because the harness is still writing output.txt and +# out.requests.txt while the deploy syncs. +errcode trace $CLI bundle deploy -qq + +title "The message carries the workspace path and the user; the template carries neither\n" +trace print_telemetry_error_message +trace print_telemetry_string_values + +# Record the error fields exactly as they leave the machine, so the golden shows +# the payload rather than a helper's rendering of it. +cat out.requests.txt | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson | {error_message: .entry.databricks_cli_log.bundle_deploy_event.error_message, string_values: .entry.databricks_cli_log.bundle_deploy_event.experimental.string_values}' > out.telemetry.txt + +rm out.requests.txt diff --git a/acceptance/bundle/telemetry/deploy-error-template/test.toml b/acceptance/bundle/telemetry/deploy-error-template/test.toml new file mode 100644 index 00000000000..5fd96153d68 --- /dev/null +++ b/acceptance/bundle/telemetry/deploy-error-template/test.toml @@ -0,0 +1,5 @@ +# The injected path is the direct-engine state file (state/resources.json), so +# this runs on the direct engine only. The path is fixed, so the bundle name must +# be stable (test-bundle, not $UNIQUE_NAME). +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +Ignore = [".databricks"] diff --git a/acceptance/script.prepare b/acceptance/script.prepare index d71d37aae9b..1db315d650a 100644 --- a/acceptance/script.prepare +++ b/acceptance/script.prepare @@ -123,6 +123,14 @@ print_telemetry_bool_values() { jq -r 'select(.path? == "/telemetry-ext") | (.body.protoLogs // [])[] | fromjson | ( (.entry // .) | (.databricks_cli_log.bundle_deploy_event.experimental.bool_values // []) ) | map("\(.key) \(.value)") | .[]' out.requests.txt | grep -v '^engine_terraform_' | sort } +print_telemetry_error_message() { + jq -r 'select(.path? == "/telemetry-ext") | (.body.protoLogs // [])[] | fromjson | ( (.entry // .) | .databricks_cli_log.bundle_deploy_event.error_message // empty )' out.requests.txt +} + +print_telemetry_string_values() { + jq -r 'select(.path? == "/telemetry-ext") | (.body.protoLogs // [])[] | fromjson | ( (.entry // .) | (.databricks_cli_log.bundle_deploy_event.experimental.string_values // []) ) | map("\(.key) \(.value)") | .[]' out.requests.txt | sort +} + sethome() { local home="$1" mkdir -p "$home" diff --git a/bundle/bundle.go b/bundle/bundle.go index bcceb752088..9e3f8910167 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -80,6 +80,7 @@ type Metrics struct { TargetCount int64 DeploymentId uuid.UUID BoolValues []protos.BoolMapEntry + StringValues []protos.StringMapEntry PythonAddedResourcesCount int64 PythonUpdatedResourcesCount int64 ExecutionTimes []protos.IntMapEntry @@ -110,6 +111,19 @@ func (m *Metrics) SetBoolValue(key string, value bool) { m.BoolValues = append(m.BoolValues, protos.BoolMapEntry{Key: key, Value: value}) } +// SetStringValue sets the value of a string metric, replacing any previous value +// for the same key. Values must be PII-free: this map is reported without +// scrubbing. See protos.BundleDeployExperimental.StringValues. +func (m *Metrics) SetStringValue(key, value string) { + for i, v := range m.StringValues { + if v.Key == key { + m.StringValues[i].Value = value + return + } + } + m.StringValues = append(m.StringValues, protos.StringMapEntry{Key: key, Value: value}) +} + func (m *Metrics) AddBoolValue(key string, value bool) { m.BoolValues = append(m.BoolValues, protos.BoolMapEntry{Key: key, Value: value}) } diff --git a/bundle/config/mutator/load_dbalert_files.go b/bundle/config/mutator/load_dbalert_files.go index 5c3e6921b93..13dfae24a5e 100644 --- a/bundle/config/mutator/load_dbalert_files.go +++ b/bundle/config/mutator/load_dbalert_files.go @@ -81,12 +81,13 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia return diag.Diagnostics{ { - ID: "", - Severity: diag.Error, - Summary: fmt.Sprintf("field %s is not allowed in the bundle configuration.", k), - Detail: "When a .dbalert.json is specified, only the following fields are allowed in the bundle configuration: " + strings.Join(allowedInYAML, ", "), - Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.%s", alertKey, k))}, - Locations: v.Locations(), + ID: "", + ErrorTemplate: "", + Severity: diag.Error, + Summary: fmt.Sprintf("field %s is not allowed in the bundle configuration.", k), + Detail: "When a .dbalert.json is specified, only the following fields are allowed in the bundle configuration: " + strings.Join(allowedInYAML, ", "), + Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.%s", alertKey, k))}, + Locations: v.Locations(), }, } } @@ -103,12 +104,13 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia if err != nil { return diag.Diagnostics{ { - ID: diag.ID(""), - Severity: diag.Error, - Summary: fmt.Sprintf("failed to read .dbalert.json file %s: %s", alert.FilePath, err), - Detail: "", - Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.file_path", alertKey))}, - Locations: alertV.Get("file_path").Locations(), + ID: diag.ID(""), + ErrorTemplate: "", + Severity: diag.Error, + Summary: fmt.Sprintf("failed to read .dbalert.json file %s: %s", alert.FilePath, err), + Detail: "", + Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.file_path", alertKey))}, + Locations: alertV.Get("file_path").Locations(), }, } } @@ -118,12 +120,13 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia if err != nil { return diag.Diagnostics{ { - ID: diag.ID(""), - Severity: diag.Error, - Summary: fmt.Sprintf("failed to parse .dbalert.json file %s: %s", alert.FilePath, err), - Detail: "", - Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.file_path", alertKey))}, - Locations: alertV.Get("file_path").Locations(), + ID: diag.ID(""), + ErrorTemplate: "", + Severity: diag.Error, + Summary: fmt.Sprintf("failed to parse .dbalert.json file %s: %s", alert.FilePath, err), + Detail: "", + Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.file_path", alertKey))}, + Locations: alertV.Get("file_path").Locations(), }, } } @@ -132,12 +135,13 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia if dynvar.ContainsVariableReference(string(content)) { return diag.Diagnostics{ { - ID: diag.ID(""), - Severity: diag.Error, - Summary: fmt.Sprintf(".alert file %s must not contain variable interpolations.", alert.FilePath), - Detail: "Please inline the alert configuration in the bundle configuration to use variables", - Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.file_path", alertKey))}, - Locations: alertV.Get("file_path").Locations(), + ID: diag.ID(""), + ErrorTemplate: "", + Severity: diag.Error, + Summary: fmt.Sprintf(".alert file %s must not contain variable interpolations.", alert.FilePath), + Detail: "Please inline the alert configuration in the bundle configuration to use variables", + Paths: []dyn.Path{dyn.MustPathFromString(fmt.Sprintf("resources.alerts.%s.file_path", alertKey))}, + Locations: alertV.Get("file_path").Locations(), }, } } diff --git a/bundle/config/resource_key.go b/bundle/config/resource_key.go new file mode 100644 index 00000000000..bf9106baa9d --- /dev/null +++ b/bundle/config/resource_key.go @@ -0,0 +1,39 @@ +package config + +import "strings" + +// ResourceKey wraps a resource key (e.g. "resources.jobs.my_job" or +// "resources.jobs.my_job.permissions") for use as an error argument. It formats +// as the full key, so error messages are unchanged, but it reports only its +// resource type to telemetry — the resource name is user-authored and therefore +// PII, while the type is a value the CLI itself defines. +// +// Pass it wherever a resource key is interpolated into a safeerr error: +// +// safeerr.Errorf("%s: SaveState: %w", config.ResourceKey(node), err) +// message: resources.jobs.my_job: SaveState: ... +// template: resources.jobs.*: SaveState: %w +type ResourceKey string + +func (k ResourceKey) String() string { + return string(k) +} + +// SafeString implements safeerr.SafeStringer, standing in for the key with its +// name replaced by "*". A key this package cannot parse reports nothing beyond +// the redaction marker, since an unrecognized shape may be anything at all. +func (k ResourceKey) SafeString() string { + resourceType := GetResourceTypeFromKey(string(k)) + if resourceType == "" { + return "*" + } + + // GetResourceTypeFromKey collapses a sub-resource into "." + // (e.g. "jobs.permissions"), but in the key itself the kind trails the + // name. Rebuild the key's own shape so the stand-in reads like the value. + group, kind, hasKind := strings.Cut(resourceType, ".") + if hasKind { + return "resources." + group + ".*." + kind + } + return "resources." + group + ".*" +} diff --git a/bundle/config/resource_key_test.go b/bundle/config/resource_key_test.go new file mode 100644 index 00000000000..edf8c7f3515 --- /dev/null +++ b/bundle/config/resource_key_test.go @@ -0,0 +1,57 @@ +package config + +import ( + "fmt" + "testing" + + "github.com/databricks/cli/libs/safeerr" + "github.com/stretchr/testify/assert" +) + +func TestResourceKeySafeString(t *testing.T) { + tests := []struct { + key string + want string + }{ + {key: "resources.jobs.my_job", want: "resources.jobs.*"}, + {key: "resources.pipelines.my_pipeline", want: "resources.pipelines.*"}, + {key: "resources.jobs.my_job.permissions", want: "resources.jobs.*.permissions"}, + {key: "resources.schemas.my_schema.grants", want: "resources.schemas.*.grants"}, + {key: "resources.secret_scopes.my scope.permissions", want: "resources.secret_scopes.*.permissions"}, + + // Shapes GetResourceTypeFromKey does not recognize report nothing. + {key: "resources.jobs", want: "*"}, + {key: "jobs.my_job", want: "*"}, + {key: "", want: "*"}, + {key: "/Workspace/Users/someone@example.com/x", want: "*"}, + } + + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + assert.Equal(t, tt.want, ResourceKey(tt.key).SafeString()) + }) + } +} + +// TestResourceKeyFormatsAsTheFullKey is what keeps error messages unchanged +// when a call site starts passing ResourceKey instead of a bare string. +func TestResourceKeyFormatsAsTheFullKey(t *testing.T) { + const key = "resources.jobs.my_job" + + for _, format := range []string{"%s", "%q", "%v"} { + t.Run(format, func(t *testing.T) { + assert.Equal(t, + fmt.Sprintf(format, key), + fmt.Sprintf(format, ResourceKey(key))) + }) + } +} + +func TestResourceKeyInSafeerr(t *testing.T) { + err := safeerr.Errorf("%s: SaveState: %w", + ResourceKey("resources.jobs.my_job"), safeerr.New("disk full")) + + assert.Equal(t, "resources.jobs.my_job: SaveState: disk full", err.Error()) + assert.Equal(t, "resources.jobs.*: SaveState: disk full", safeerr.ErrorTemplate(err)) + assert.NotContains(t, safeerr.ErrorTemplate(err), "my_job") +} diff --git a/bundle/metrics/metrics.go b/bundle/metrics/metrics.go index f0a101b8ad0..df57f728fed 100644 --- a/bundle/metrics/metrics.go +++ b/bundle/metrics/metrics.go @@ -11,6 +11,22 @@ const ( SqlWarehouseLifecycleStarted = "sql_warehouse_lifecycle_started" SelectUsed = "select_used" + // PII-free descriptions of the errors a deploy hit, recorded in + // experimental.string_values. Each value is a libs/safeerr message template: + // the error's format string with everything the user supplied left as a verb, + // plus the safe fields of any API error at the end of the chain. They are the + // aggregatable counterpart to error_message, which is scrubbed heuristically + // and stays privileged. + // - error_template: the first error diagnostic the deploy logged. + // - direct_migrate_error_template: why the post-deploy migration to the + // direct engine failed. Recorded for both the opt-in population + // (alongside direct_migrate_error) and the dry run (alongside + // direct_drymigrate_success), so either failure population can be broken + // down by cause rather than just counted. A migration stopped by warnings + // alone has no error and so records nothing here. + ErrorTemplate = "error_template" + DirectMigrateErrorTemplate = "direct_migrate_error_template" + // Outcome of the dry-run migration to the direct engine attempted after a // successful terraform deploy WHEN THE USER DID NOT OPT IN. Only recorded // when the state conversion is truly a dry run (no auto-migrate). diff --git a/bundle/migrate/build_state.go b/bundle/migrate/build_state.go index e8b382b370d..e5238368ec0 100644 --- a/bundle/migrate/build_state.go +++ b/bundle/migrate/build_state.go @@ -2,7 +2,6 @@ package migrate import ( "context" - "fmt" "maps" "slices" "strings" @@ -15,6 +14,7 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/safeerr" "github.com/databricks/cli/libs/structs/structaccess" "github.com/databricks/cli/libs/structs/structpath" "github.com/databricks/cli/libs/structs/structvar" @@ -64,7 +64,7 @@ func BuildStateFromTF( group := config.GetResourceTypeFromKey(node) if group == "" { - return warningsSeen, fmt.Errorf("cannot determine resource type for %q", node) + return warningsSeen, safeerr.Errorf("cannot determine resource type for %q", config.ResourceKey(node)) } adapter, ok := adapters[group] @@ -76,22 +76,22 @@ func BuildStateFromTF( inputConfig, err := configRoot.GetResourceConfig(node) if err != nil { - return warningsSeen, fmt.Errorf("%s: getting config: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: getting config: %w", config.ResourceKey(node), err) } inputSV, err := adapter.PrepareInputConfig(inputConfig, node) if err != nil { - return warningsSeen, fmt.Errorf("%s: PrepareInputConfig: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: PrepareInputConfig: %w", config.ResourceKey(node), err) } newStateValue, err := adapter.PrepareState(inputSV.Value) if err != nil { - return warningsSeen, fmt.Errorf("%s: PrepareState: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: PrepareState: %w", config.ResourceKey(node), err) } refs, err := direct.ExtractReferences(configRoot.Value(), node, adapter.StateType()) if err != nil { - return warningsSeen, fmt.Errorf("%s: extracting references: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: extracting references: %w", config.ResourceKey(node), err) } maps.Copy(refs, inputSV.Refs) @@ -142,7 +142,7 @@ func BuildStateFromTF( // is absent there (model_serving_endpoints, database_instances). if _, ok := sv.Refs["object_id"]; ok { if err := structaccess.Set(sv.Value, structpath.NewStringKey(nil, "object_id"), id); err != nil { - return warningsSeen, fmt.Errorf("%s: setting object_id: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: setting object_id: %w", config.ResourceKey(node), err) } delete(sv.Refs, "object_id") } @@ -169,14 +169,14 @@ func BuildStateFromTF( for _, pending := range pendingRefs { fieldPath, err := structpath.ParsePath(pending.fieldPathStr) if err != nil { - return warningsSeen, fmt.Errorf("%s: parsing field path %q: %w", node, pending.fieldPathStr, err) + return warningsSeen, safeerr.Errorf("%s: parsing field path %q: %w", config.ResourceKey(node), pending.fieldPathStr, err) } // ResolveFieldRef returns the fully resolved value for this field, // using either Method A (TF state lookup) or Method B (template evaluation). value, warned, err := ResolveFieldRef(ctx, tfAttrs, srcGroup, srcName, fieldPath, pending.refTemplate, warnPrefix) if err != nil { - return warningsSeen, fmt.Errorf("%s: cannot resolve field %q (template %q): %w", node, pending.fieldPathStr, pending.refTemplate, err) + return warningsSeen, safeerr.Errorf("%s: cannot resolve field %q (template %q): %w", config.ResourceKey(node), pending.fieldPathStr, pending.refTemplate, err) } if warned { warningsSeen = true @@ -184,13 +184,13 @@ func BuildStateFromTF( // Set the resolved value directly and remove the ref entry. if err := structaccess.Set(sv.Value, fieldPath, value); err != nil { - return warningsSeen, fmt.Errorf("%s: cannot set resolved value for field %q: %w", node, pending.fieldPathStr, err) + return warningsSeen, safeerr.Errorf("%s: cannot set resolved value for field %q: %w", config.ResourceKey(node), pending.fieldPathStr, err) } delete(sv.Refs, pending.fieldPathStr) } if len(sv.Refs) > 0 { - return warningsSeen, fmt.Errorf("%s: unresolved references: %v", node, sv.Refs) + return warningsSeen, safeerr.Errorf("%s: unresolved references: %v", config.ResourceKey(node), sv.Refs) } // Handle etag for dashboards: read it directly from TF state attributes. @@ -200,13 +200,13 @@ func BuildStateFromTF( if v, err := LookupTFField(tfAttrs, group, srcName, structpath.NewStringKey(nil, "etag")); err == nil { if etag, ok := v.(string); ok && etag != "" { if err := structaccess.Set(sv.Value, structpath.NewStringKey(nil, "etag"), etag); err != nil { - return warningsSeen, fmt.Errorf("%s: cannot set etag: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: cannot set etag: %w", config.ResourceKey(node), err) } } } if err := stateDB.SaveState(node, id, sv.Value, dependsOn); err != nil { - return warningsSeen, fmt.Errorf("%s: SaveState: %w", node, err) + return warningsSeen, safeerr.Errorf("%s: SaveState: %w", config.ResourceKey(node), err) } } diff --git a/bundle/migrate/resolve.go b/bundle/migrate/resolve.go index 23d0180e3c6..15b2a31f3f8 100644 --- a/bundle/migrate/resolve.go +++ b/bundle/migrate/resolve.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/safeerr" "github.com/databricks/cli/libs/structs/structpath" ) @@ -23,16 +24,16 @@ func evaluateTemplate(state TFStateAttrs, template string) (string, error) { for _, pathString := range ref.References() { path, err := structpath.ParsePath(pathString) if err != nil { - return "", fmt.Errorf("cannot parse reference path %q: %w", pathString, err) + return "", safeerr.Errorf("cannot parse reference path %q: %w", pathString, err) } // Expect resources... if path.Len() < 4 { - return "", fmt.Errorf("unexpected reference format (too short): %q", pathString) + return "", safeerr.Errorf("unexpected reference format (too short): %q", pathString) } // Check first component is "resources" firstNode := path.Prefix(1) if firstNode.String() != "resources" { - return "", fmt.Errorf("unexpected reference format (expected resources.*): %q", pathString) + return "", safeerr.Errorf("unexpected reference format (expected resources.*): %q", pathString) } group := path.SkipPrefix(1).Prefix(1).String() @@ -41,7 +42,7 @@ func evaluateTemplate(state TFStateAttrs, template string) (string, error) { value, err := LookupTFField(state, group, name, fieldPath) if err != nil { - return "", fmt.Errorf("cannot look up %q: %w", pathString, err) + return "", safeerr.Errorf("cannot look up %q: %w", pathString, err) } result = strings.ReplaceAll(result, "${"+pathString+"}", fmt.Sprintf("%v", value)) @@ -88,7 +89,9 @@ func ResolveFieldRef(ctx context.Context, state TFStateAttrs, srcGroup, srcName case errB == nil: return valueB, false, nil default: - return nil, false, fmt.Errorf("%s.%s field %s: method A: %w; method B: %w", - srcGroup, srcName, fieldPath, errA, errB) + // srcGroup is a resource type the CLI defines, so it is safe to report; + // the resource name and field path are not. + return nil, false, safeerr.Errorf("%s.%s field %s: method A: %w; method B: %w", + safeerr.Safe(srcGroup), srcName, fieldPath, errA, errB) } } diff --git a/bundle/migrate/tf_state.go b/bundle/migrate/tf_state.go index 55634234051..04077b05411 100644 --- a/bundle/migrate/tf_state.go +++ b/bundle/migrate/tf_state.go @@ -5,11 +5,11 @@ import ( "context" "encoding/json" "errors" - "fmt" "os" "github.com/databricks/cli/bundle/deploy/terraform" "github.com/databricks/cli/bundle/terraform_dabs_map" + "github.com/databricks/cli/libs/safeerr" "github.com/databricks/cli/libs/structs/structpath" tfjson "github.com/hashicorp/terraform-json" ) @@ -94,7 +94,7 @@ func parseTFStateAttrsFromRaw(s *rawTFState) TFStateAttrs { func LookupTFField(state TFStateAttrs, group, name string, fieldPath *structpath.PathNode) (any, error) { tfType, ok := terraform.GroupToTerraformName[group] if !ok { - return nil, fmt.Errorf("unknown resource group %q", group) + return nil, safeerr.Errorf("unknown resource group %q", safeerr.Safe(group)) } // Translate field path to TF naming. @@ -107,7 +107,7 @@ func LookupTFField(state TFStateAttrs, group, name string, fieldPath *structpath attrsJSON, ok := state[tfType][name] if !ok { - return nil, fmt.Errorf("%s.%s not found in TF state", tfType, name) + return nil, safeerr.Errorf("%s.%s not found in TF state", safeerr.Safe(tfType), name) } // Unmarshal into map[string]any to handle TF list-blocks: in TF state, single-block @@ -120,7 +120,7 @@ func LookupTFField(state TFStateAttrs, group, name string, fieldPath *structpath dec := json.NewDecoder(bytes.NewReader(attrsJSON)) dec.UseNumber() if err := dec.Decode(&attrs); err != nil { - return nil, fmt.Errorf("cannot parse TF state for %s.%s: %w", tfType, name, err) + return nil, safeerr.Errorf("cannot parse TF state for %s.%s: %w", safeerr.Safe(tfType), name, err) } return navigateTFState(attrs, tfFieldPath) @@ -148,18 +148,18 @@ func navigateTFState(data map[string]any, path *structpath.PathNode) (any, error } m, ok := current.(map[string]any) if !ok { - return nil, fmt.Errorf("expected map at %q, got %T", key, current) + return nil, safeerr.Errorf("expected map at %q, got %T", key, current) } val, ok := m[key] if !ok { - return nil, fmt.Errorf("%q: key not found", key) + return nil, safeerr.Errorf("%q: key not found", key) } current = val } else if idx, ok := node.Index(); ok { switch v := current.(type) { case []any: if idx < 0 || idx >= len(v) { - return nil, fmt.Errorf("index %d out of range (len %d)", idx, len(v)) + return nil, safeerr.Errorf("index %d out of range (len %d)", safeerr.Safe(idx), safeerr.Safe(len(v))) } current = v[idx] default: @@ -167,7 +167,7 @@ func navigateTFState(data map[string]any, path *structpath.PathNode) (any, error if idx == 0 { continue } - return nil, fmt.Errorf("index %d: not a slice (%T)", idx, current) + return nil, safeerr.Errorf("index %d: not a slice (%T)", safeerr.Safe(idx), current) } } } diff --git a/bundle/phases/telemetry.go b/bundle/phases/telemetry.go index cbe74f467fe..9ace4200b70 100644 --- a/bundle/phases/telemetry.go +++ b/bundle/phases/telemetry.go @@ -296,6 +296,7 @@ func LogDeployTelemetry(ctx context.Context, b *bundle.Bundle, errMsg string) { TargetCount: b.Metrics.TargetCount, WorkspaceArtifactPathType: artifactPathType, BoolValues: b.Metrics.BoolValues, + StringValues: b.Metrics.StringValues, LocalCacheMeasurementsMs: b.Metrics.LocalCacheMeasurementsMs, PythonAddedResourcesCount: b.Metrics.PythonAddedResourcesCount, PythonUpdatedResourcesCount: b.Metrics.PythonUpdatedResourcesCount, diff --git a/bundle/statemgmt/direct_migration.go b/bundle/statemgmt/direct_migration.go index a13b7effd46..520363d7ce1 100644 --- a/bundle/statemgmt/direct_migration.go +++ b/bundle/statemgmt/direct_migration.go @@ -19,10 +19,12 @@ import ( "github.com/databricks/cli/bundle/metrics" "github.com/databricks/cli/bundle/migrate" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/logdiag" + "github.com/databricks/cli/libs/safeerr" ) // warnPrefix labels warnings emitted by the post-deploy dry-run so they are not @@ -55,6 +57,7 @@ func MigrateToDirect(ctx context.Context, b *bundle.Bundle, requestedEngine engi tfState, err := migrate.ParseTFStateFull(ctx, localTerraformPath) if err != nil { log.Warnf(ctx, "%sfailed to parse terraform state: %v", warnPrefix, err) + recordMigrateErrorTemplate(b, err) if requestedEngine.Type == engine.EngineDirect { b.Metrics.SetBoolValue(metrics.DirectMigrateError, true) log.Warnf(ctx, "%s", autoMigrateStoppedNotice) @@ -84,6 +87,7 @@ func MigrateToDirect(ctx context.Context, b *bundle.Bundle, requestedEngine engi cmdio.LogString(ctx, "Removing empty terraform state; direct engine will be used on the next deploy (opted in via "+requestedEngine.Source+")...") if err := backupTerraformState(ctx, b); err != nil { b.Metrics.SetBoolValue(metrics.DirectMigrateCommitError, true) + recordMigrateErrorTemplate(b, err) log.Warnf(ctx, "automatic migration to direct engine failed: %v", err) return } @@ -106,6 +110,7 @@ func MigrateToDirect(ctx context.Context, b *bundle.Bundle, requestedEngine engi if err != nil { log.Warnf(ctx, "%s%v", warnPrefix, err) + recordMigrateErrorTemplate(b, err) } if hasWarnings || err != nil { log.Warnf(ctx, "%s", feedbackNotice) @@ -136,6 +141,7 @@ func MigrateToDirect(ctx context.Context, b *bundle.Bundle, requestedEngine engi if err := commitMigration(ctx, b, tempStatePath, resourceCount); err != nil { b.Metrics.SetBoolValue(metrics.DirectMigrateCommitError, true) + recordMigrateErrorTemplate(b, err) log.Warnf(ctx, "automatic migration to direct engine failed: %v", err) return } @@ -143,6 +149,16 @@ func MigrateToDirect(ctx context.Context, b *bundle.Bundle, requestedEngine engi recordAutoMigrateSource(b, requestedEngine) } +// recordMigrateErrorTemplate records a PII-free description of why the migration +// failed. It is recorded for both populations — the opt-in one and the dry run — +// because the booleans only say that a migration failed, and the interesting +// question is which of the failures it was. See metrics.DirectMigrateErrorTemplate. +func recordMigrateErrorTemplate(b *bundle.Bundle, err error) { + if template := diag.ErrorTemplate(err); template != "" { + b.Metrics.SetStringValue(metrics.DirectMigrateErrorTemplate, template) + } +} + // recordDryRunNoop records dry-run telemetry for a no-op case (no state, or // state with no managed resources) when the user did NOT opt in. On opt-in // paths the caller uses direct_migrate_* keys instead. @@ -185,20 +201,20 @@ func backupTerraformState(ctx context.Context, b *bundle.Bundle) error { remoteTerraformPath, localTerraformPath := b.StateFilenameTerraform(ctx) reader, err := f.Read(ctx, remoteTerraformPath) if err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("reading remote terraform state %s: %w", remoteTerraformPath, err) + return safeerr.Errorf("reading remote terraform state %s: %w", remoteTerraformPath, err) } if err == nil { defer reader.Close() if err := f.Write(ctx, remoteTerraformPath+".backup", reader, filer.OverwriteIfExists); err != nil { - return fmt.Errorf("writing remote terraform backup: %w", err) + return safeerr.Errorf("writing remote terraform backup: %w", err) } if err := f.Delete(ctx, remoteTerraformPath); err != nil { - return fmt.Errorf("deleting remote terraform state: %w", err) + return safeerr.Errorf("deleting remote terraform state: %w", err) } } if err := os.Rename(localTerraformPath, localTerraformPath+".backup"); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("renaming local terraform state to %s.backup: %w", localTerraformPath, err) + return safeerr.Errorf("renaming local terraform state to %s.backup: %w", localTerraformPath, err) } return nil } @@ -261,7 +277,7 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr // the migrated state and config agree on .permissions entries. bundle.ApplyContext(ctx, b, resourcemutator.SecretScopeFixups(engine.EngineDirect)) if logdiag.HasError(ctx) { - return tempStatePath, resourceCount, false, errors.New("failed to apply secret scope fixups") + return tempStatePath, resourceCount, false, safeerr.New("failed to apply secret scope fixups") } // b.Config has been modified by terraform.Interpolate which converts bundle-style @@ -269,7 +285,7 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr // BuildStateFromTF expects ${resources.*} references, so reverse the interpolation first. uninterpolatedRoot, err := reverseInterpolate(b.Config.Value()) if err != nil { - return tempStatePath, resourceCount, false, fmt.Errorf("failed to reverse interpolation: %w", err) + return tempStatePath, resourceCount, false, safeerr.Errorf("failed to reverse interpolation: %w", err) } var uninterpolatedConfig config.Root @@ -277,7 +293,7 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr return uninterpolatedRoot, nil }) if err != nil { - return tempStatePath, resourceCount, false, fmt.Errorf("failed to create uninterpolated config: %w", err) + return tempStatePath, resourceCount, false, safeerr.Errorf("failed to create uninterpolated config: %w", err) } adapters, err := dresources.InitAll(nil) @@ -286,7 +302,7 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr } if err := stateDB.UpgradeToWrite(); err != nil { - return tempStatePath, resourceCount, false, fmt.Errorf("upgrading state for apply: %w", err) + return tempStatePath, resourceCount, false, safeerr.Errorf("upgrading state for apply: %w", err) } // warnPrefix labels the conversion's warnings as coming from the background dry run. @@ -301,7 +317,7 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr // BuildStateFromTF reports some failures via logdiag instead of returning an error. if logdiag.HasError(ctx) { - return tempStatePath, resourceCount, hasWarnings, errors.New("state conversion failed") + return tempStatePath, resourceCount, hasWarnings, safeerr.New("state conversion failed") } return tempStatePath, resourceCount, hasWarnings, nil @@ -320,13 +336,13 @@ func commitMigration(ctx context.Context, b *bundle.Bundle, tempStatePath string // "file is missing"; treat it as a hard failure to avoid renaming over // something we couldn't read. if _, err := os.Stat(localDirectPath); err == nil { - return fmt.Errorf("state file %s already exists", localDirectPath) + return safeerr.Errorf("state file %s already exists", localDirectPath) } else if !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("stat %s: %w", localDirectPath, err) + return safeerr.Errorf("stat %s: %w", localDirectPath, err) } if err := pushDirectState(ctx, b, tempStatePath); err != nil { - return fmt.Errorf("pushing direct state to workspace: %w", err) + return safeerr.Errorf("pushing direct state to workspace: %w", err) } // Remote is now authoritative for direct engine; make local match. Local @@ -336,13 +352,13 @@ func commitMigration(ctx context.Context, b *bundle.Bundle, tempStatePath string // on failure so telemetry reflects what actually happened here (the // migration is complete on the workspace but not on this checkout). if err := os.MkdirAll(filepath.Dir(localDirectPath), 0o700); err != nil { - return fmt.Errorf("workspace migrated but creating local state directory failed: %w", err) + return safeerr.Errorf("workspace migrated but creating local state directory failed: %w", err) } if err := os.Rename(tempStatePath, localDirectPath); err != nil { - return fmt.Errorf("workspace migrated but writing local direct state failed: %w", err) + return safeerr.Errorf("workspace migrated but writing local direct state failed: %w", err) } if err := os.Rename(localTerraformPath, localTerraformPath+".backup"); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("workspace migrated but backing up local terraform state failed: %w", err) + return safeerr.Errorf("workspace migrated but backing up local terraform state failed: %w", err) } suffix := "s" @@ -388,16 +404,16 @@ func pushDirectState(ctx context.Context, b *bundle.Bundle, localPath string) er return nil } if err != nil { - return fmt.Errorf("reading remote terraform state %s: %w", remoteTerraformPath, err) + return safeerr.Errorf("reading remote terraform state %s: %w", remoteTerraformPath, err) } defer reader.Close() if err := f.Write(ctx, remoteTerraformPath+".backup", reader, filer.OverwriteIfExists); err != nil { - return fmt.Errorf("writing remote terraform backup: %w", err) + return safeerr.Errorf("writing remote terraform backup: %w", err) } if err := f.Delete(ctx, remoteTerraformPath); err != nil { - return fmt.Errorf("deleting remote terraform state: %w", err) + return safeerr.Errorf("deleting remote terraform state: %w", err) } return nil diff --git a/cmd/bundle/utils/process.go b/cmd/bundle/utils/process.go index 7e14f35343f..af3695a26ac 100644 --- a/cmd/bundle/utils/process.go +++ b/cmd/bundle/utils/process.go @@ -15,6 +15,7 @@ import ( "github.com/databricks/cli/bundle/deployplan" "github.com/databricks/cli/bundle/direct" "github.com/databricks/cli/bundle/direct/dstate" + "github.com/databricks/cli/bundle/metrics" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/statemgmt" "github.com/databricks/cli/cmd/root" @@ -112,8 +113,16 @@ func ProcessBundleRet(cmd *cobra.Command, opts ProcessOptions) (b *bundle.Bundle return } errMsg := logdiag.GetFirstErrorSummary(ctx) + errTemplate := logdiag.GetFirstErrorTemplate(ctx) if errMsg == "" && retErr != nil && !errors.Is(retErr, root.ErrAlreadyPrinted) { errMsg = retErr.Error() + // Mirror the summary's fallback: a command that returned an error + // without logging a diagnostic never reached LogDiag, so the + // template has to come off the error value itself. + errTemplate = diag.ErrorTemplate(retErr) + } + if errTemplate != "" { + b.Metrics.SetStringValue(metrics.ErrorTemplate, errTemplate) } phases.LogDeployTelemetry(ctx, b, errMsg) }() diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index fe7090462d4..2a5634a8905 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/safeerr" ) type Diagnostic struct { @@ -28,6 +29,15 @@ type Diagnostic struct { // A diagnostic ID. Only used for select diagnostic messages. ID ID + + // ErrorTemplate is a PII-free description of the error this diagnostic came + // from, suitable for telemetry: the message template of a safeerr error plus + // the safe fields of any API error at the end of its chain. Empty when the + // diagnostic was not built from an error, or from an error carrying neither. + // + // Unlike Summary, this is never user-authored, so it can be aggregated + // across the fleet without scrubbing. See libs/safeerr. + ErrorTemplate string } // Errorf creates a new error diagnostic. @@ -47,13 +57,33 @@ func FromErr(err error) Diagnostics { } return []Diagnostic{ { - Severity: Error, - Summary: FormatAPIErrorSummary(err), - Detail: FormatAPIErrorDetails(err), + Severity: Error, + Summary: FormatAPIErrorSummary(err), + Detail: FormatAPIErrorDetails(err), + ErrorTemplate: ErrorTemplate(err), }, } } +// ErrorTemplate builds the PII-free description recorded in +// Diagnostic.ErrorTemplate, for callers holding an error rather than a +// diagnostic. The two halves are independent: a CLI error carries a template but +// may wrap no API error, and an API error reached without any safeerr wrapping +// has safe fields but no template. +func ErrorTemplate(err error) string { + template := safeerr.ErrorTemplate(err) + apiDescription := SafeAPIErrorDescription(err) + + switch { + case apiDescription == "": + return template + case template == "": + return apiDescription + default: + return template + " [" + apiDescription + "]" + } +} + // FromErr returns a new warning diagnostic from the specified error, if any. func WarningFromErr(err error) Diagnostics { if err == nil { diff --git a/libs/diag/error_template_test.go b/libs/diag/error_template_test.go new file mode 100644 index 00000000000..c99ebe3345d --- /dev/null +++ b/libs/diag/error_template_test.go @@ -0,0 +1,122 @@ +package diag + +import ( + "errors" + "fmt" + "net/http" + "testing" + + "github.com/databricks/cli/libs/safeerr" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/stretchr/testify/assert" +) + +// apiError builds an SDK error whose message carries user data, the way a real +// one does. +func apiError(code string, status int) error { + return &apierr.APIError{ + ErrorCode: code, + StatusCode: status, + Message: "User alice@example.com cannot access /Workspace/Users/alice@example.com/job", + } +} + +func TestSafeAPIErrorDescription(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + {name: "not an api error", err: errors.New("boom"), want: ""}, + {name: "code and status", err: apiError("PERMISSION_DENIED", http.StatusForbidden), want: "PERMISSION_DENIED 403"}, + {name: "code only", err: apiError("RESOURCE_DOES_NOT_EXIST", 0), want: "RESOURCE_DOES_NOT_EXIST"}, + {name: "status only", err: apiError("", http.StatusInternalServerError), want: "500"}, + {name: "nothing safe", err: apiError("", 0), want: ""}, + {name: "wrapped", err: fmt.Errorf("deploying: %w", apiError("QUOTA_EXCEEDED", 429)), want: "QUOTA_EXCEEDED 429"}, + + // A code that does not have the shape of an enum member is dropped + // rather than trusted, so free text cannot ride along. + {name: "code with a path", err: apiError("cannot find /Workspace/Users/a@b.com/x", 404), want: "404"}, + {name: "code with a quoted name", err: apiError(`job "Q4 forecast" missing`, 404), want: "404"}, + {name: "code lowercase", err: apiError("permission_denied", 403), want: "403"}, + {name: "code with a dot", err: apiError("PERMISSION.DENIED", 403), want: "403"}, + {name: "code with a space", err: apiError("PERMISSION DENIED", 403), want: "403"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, SafeAPIErrorDescription(tt.err)) + }) + } +} + +func TestFromErrErrorTemplate(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + { + name: "plain error has no template", + err: errors.New("resources.jobs.my_job failed"), + want: "", + }, + { + name: "safeerr error", + err: safeerr.Errorf("cannot update %s: %w", "resources.jobs.my_job", errors.New("boom")), + want: "cannot update %s: %w", + }, + { + name: "api error without any safeerr wrapping", + err: apiError("PERMISSION_DENIED", http.StatusForbidden), + want: "PERMISSION_DENIED 403", + }, + { + name: "safeerr error wrapping an api error", + err: safeerr.Errorf("cannot update %s: %w", "resources.jobs.my_job", apiError("PERMISSION_DENIED", http.StatusForbidden)), + want: "cannot update %s: %w [PERMISSION_DENIED 403]", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + diags := FromErr(tt.err) + assert.Len(t, diags, 1) + assert.Equal(t, tt.want, diags[0].ErrorTemplate) + }) + } +} + +// TestFromErrErrorTemplateCarriesNoUserData is the property the field exists +// for: whatever the summary holds, the template holds none of it. +func TestFromErrErrorTemplateCarriesNoUserData(t *testing.T) { + err := safeerr.Errorf("cannot update %s: %w", + "resources.jobs.my_secret_job", + apiError("PERMISSION_DENIED", http.StatusForbidden)) + + diags := FromErr(err) + assert.Len(t, diags, 1) + + for _, secret := range []string{"my_secret_job", "alice@example.com", "/Workspace/"} { + assert.Contains(t, diags[0].Summary, secret, "summary carries user data, as before") + assert.NotContains(t, diags[0].ErrorTemplate, secret, "template must not") + } +} + +func TestFromErrNil(t *testing.T) { + assert.Nil(t, FromErr(nil)) +} + +// TestErrorTemplateMatchesFromErr keeps the exported helper and the field in +// step, since callers holding an error use one and callers holding a diagnostic +// use the other. +func TestErrorTemplateMatchesFromErr(t *testing.T) { + for _, err := range []error{ + errors.New("plain"), + safeerr.Errorf("cannot update %s: %w", "resources.jobs.my_job", errors.New("boom")), + apiError("PERMISSION_DENIED", http.StatusForbidden), + safeerr.Errorf("x: %w", apiError("ABORTED", http.StatusConflict)), + } { + assert.Equal(t, FromErr(err)[0].ErrorTemplate, ErrorTemplate(err)) + } +} diff --git a/libs/diag/sdk_error.go b/libs/diag/sdk_error.go index d190498d6bf..3934460ceda 100644 --- a/libs/diag/sdk_error.go +++ b/libs/diag/sdk_error.go @@ -3,6 +3,7 @@ package diag import ( "errors" "fmt" + "regexp" "strconv" "strings" @@ -42,3 +43,39 @@ func FormatAPIErrorDetails(e error) string { } return fmt.Sprintf("Endpoint: %s\nHTTP Status: %s\nAPI error_code: %s\nAPI message: %s", endpoint, httpStatus, apiErr.ErrorCode, apiErr.Message) } + +// safeErrorCode matches the shape of a platform error code: SCREAMING_SNAKE_CASE +// and nothing else. The code is documented as a closed enum +// (RESOURCE_DOES_NOT_EXIST, PERMISSION_DENIED, ...) but that is a convention, +// not a contract, and the field is filled in by whichever service handled the +// request. Requiring this shape is what makes it structurally impossible for a +// path, principal, quoted resource name, or sentence of free text to reach +// telemetry through it, whatever a service decides to return. +var safeErrorCode = regexp.MustCompile(`^[A-Z][A-Z0-9_]{0,63}$`) + +// SafeAPIErrorDescription returns a PII-free description of an SDK API error, or +// "" when e is not one or carries nothing safe to report. +// +// Only the structured fields are reported. An API error's message is +// user-authored data — it echoes resource names, workspace paths, principals, +// and config values back to the caller — so it never appears here. What is left +// is still the most useful part for aggregation: which platform error the +// request failed with. +func SafeAPIErrorDescription(e error) string { + apiErr, ok := errors.AsType[*apierr.APIError](e) + if !ok { + return "" + } + + var parts []string + if safeErrorCode.MatchString(apiErr.ErrorCode) { + parts = append(parts, apiErr.ErrorCode) + } + if apiErr.StatusCode != 0 { + parts = append(parts, strconv.Itoa(apiErr.StatusCode)) + } + if len(parts) == 0 { + return "" + } + return strings.Join(parts, " ") +} diff --git a/libs/logdiag/logdiag.go b/libs/logdiag/logdiag.go index 28ed3b5ba21..11ec328e26e 100644 --- a/libs/logdiag/logdiag.go +++ b/libs/logdiag/logdiag.go @@ -33,6 +33,11 @@ type LogDiagData struct { // Summary of the first error diagnostic logged, if any. FirstErrorSummary string + + // ErrorTemplate of the same diagnostic FirstErrorSummary came from. Kept + // alongside it rather than derived later, because the error value itself + // does not survive past LogDiag. + FirstErrorTemplate string } // IsSetup returns whether InitContext() was already called. @@ -136,6 +141,18 @@ func GetFirstErrorSummary(ctx context.Context) string { return val.FirstErrorSummary } +// GetFirstErrorTemplate returns the PII-free description of the first error +// diagnostic logged, or an empty string if there was none or it carried none. +// Unlike GetFirstErrorSummary, the result needs no scrubbing before it is +// reported. See diag.Diagnostic.ErrorTemplate. +func GetFirstErrorTemplate(ctx context.Context) string { + val := read(ctx) + val.mu.Lock() + defer val.mu.Unlock() + + return val.FirstErrorTemplate +} + func LogDiag(ctx context.Context, d diag.Diagnostic) { val := read(ctx) val.mu.Lock() @@ -146,6 +163,7 @@ func LogDiag(ctx context.Context, d diag.Diagnostic) { val.Errors += 1 if val.FirstErrorSummary == "" { val.FirstErrorSummary = d.Summary + val.FirstErrorTemplate = d.ErrorTemplate } case diag.Warning: val.Warnings += 1 diff --git a/libs/safeerr/safeerr.go b/libs/safeerr/safeerr.go new file mode 100644 index 00000000000..169a9316a88 --- /dev/null +++ b/libs/safeerr/safeerr.go @@ -0,0 +1,269 @@ +// Package safeerr provides errors that retain their message template: the +// format string, with values that came from the user left as verbs. Telemetry +// can then report which error occurred without shipping the values +// interpolated into it. +// +// An error created here behaves exactly like the fmt.Errorf it wraps. Error() +// returns the fully interpolated message and errors.Is/errors.AsType walk the +// same chain, so converting a call site changes nothing for existing consumers. +// The template is reachable only through ErrorTemplate. +// +// The safe/unsafe vocabulary follows github.com/cockroachdb/redact: every +// value is unsafe unless the caller marks it Safe. +// See https://github.com/cockroachdb/errors#pii-free-error-reporting +package safeerr + +import ( + "errors" + "fmt" + "strings" +) + +// Errorf formats an error exactly like fmt.Errorf and retains format as the +// error's message template. +// +// format must be a string constant. It is the one part of the error reported to +// telemetry, so building it dynamically defeats the point of the package. +func Errorf(format string, args ...any) error { + if false { + // Tells the vet printf analyzer that this is a printf wrapper, which it + // cannot infer on its own because the call below does not forward args + // verbatim. fmt.Errorf rather than fmt.Sprintf, so %w is accepted here + // too. Documented at + // https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf + _ = fmt.Errorf(format, args...) + } + + return &templateError{ + err: fmt.Errorf(format, renderArgs(args)...), + template: format, + args: templateArgs(args), + } +} + +// New returns an error whose message is entirely literal, making the whole +// message its own template. +func New(text string) error { + return &templateError{err: errors.New(text), template: text, args: nil} +} + +// Safe marks v as free of user data, allowing its value to appear in the +// message template. Use it only for values drawn from a set the CLI itself +// defines — field paths, resource groups, API status codes — never for names, +// paths, or anything else that originates in the user's configuration. +// +// Marking a value Safe does not change the error message: the value is +// formatted by its verb exactly as it would have been without the marker. +// +// Safe does not override a value's own SafeStringer stand-in; see SafeStringer. +func Safe(v any) any { + return safeValue{v: v} +} + +// SafeStringer is implemented by values that are only partly free of user data +// and can supply a PII-free stand-in for themselves. The full value still +// appears in the error message; only the stand-in reaches the template. +// +// A resource key is the motivating case: "resources.jobs.my_job" mixes a group +// the CLI defines with a name the user chose, so it stands in for itself as +// "resources.jobs.*" — enough to tell a failing job from a failing pipeline +// without reporting which one. +// +// A value that is both an error and a SafeStringer is treated as an error, so +// that %w keeps chaining its template. Otherwise SafeStringer wins over Safe: +// wrapping such a value in Safe cannot put its user-supplied part back into the +// template. +type SafeStringer interface { + SafeString() string +} + +// ErrorTemplate returns the message template of the outermost templated error +// in err's chain, or "" when the chain holds none. +// +// Templates chain: a %w whose wrapped error is itself templated contributes its +// own template in place of the verb, recursively. A %w wrapping any other error +// stays a bare %w, since nothing about that error is known to be safe. +func ErrorTemplate(err error) string { + te, ok := errors.AsType[*templateError](err) + if !ok { + return "" + } + return te.expand() +} + +// safeValue marks one argument of Errorf as free of user data. +type safeValue struct{ v any } + +type templateError struct { + // err is the fmt.Errorf result. It carries the interpolated message and + // the unwrap chain, so this type never reimplements either. + err error + + // template is the format string passed to Errorf. + template string + + // args holds only what expand can use: safe values and wrapped errors. + // Every other argument is nil, so no user data stays reachable here. + args []any +} + +func (e *templateError) Error() string { + return e.err.Error() +} + +// Unwrap returns the fmt.Errorf result rather than the wrapped error itself, so +// an error built with several %w verbs keeps working: that value implements +// Unwrap() []error, which errors.Is and errors.AsType traverse. +func (e *templateError) Unwrap() error { + return e.err +} + +// renderArgs strips Safe markers so the message fmt produces is identical to +// the one a plain fmt.Errorf call with the same values would have produced. +func renderArgs(args []any) []any { + out := make([]any, len(args)) + for i, a := range args { + if s, ok := a.(safeValue); ok { + out[i] = s.v + } else { + out[i] = a + } + } + return out +} + +// templateArgs keeps the arguments expand may substitute — safe values, the +// stand-ins of SafeStringer values, and errors whose own template can be +// chained in — and drops the rest. +func templateArgs(args []any) []any { + out := make([]any, len(args)) + for i, a := range args { + switch v := a.(type) { + case safeValue: + // A value that declares a stand-in keeps it even here: the type + // knows which part of itself is user data, so it outranks a + // call-site assertion that the whole value is safe. + if ss, ok := v.v.(SafeStringer); ok { + out[i] = safeValue{v: ss.SafeString()} + } else { + out[i] = v + } + case error: + out[i] = v + case SafeStringer: + // Resolve the stand-in now and keep only that, so the value it came + // from — which holds user data — is not retained by the error. + out[i] = safeValue{v: v.SafeString()} + } + } + return out +} + +// expand walks the template, substituting safe values and chained templates and +// leaving every other verb in place. +func (e *templateError) expand() string { + var sb strings.Builder + argIndex := 0 + + for i := 0; i < len(e.template); { + if e.template[i] != '%' { + sb.WriteByte(e.template[i]) + i++ + continue + } + + spec, verb, next, ok := parseVerb(e.template, i) + if !ok { + // A construct expand does not model shifts which argument each + // later verb consumes, so substituting past it could attribute a + // safe value to the wrong verb. Fall back to the format string, + // which is a source literal and safe to report on its own. + return e.template + } + + i = next + if verb == '%' { + sb.WriteString(spec) + continue + } + + sb.WriteString(e.substitute(spec, verb, argIndex)) + argIndex++ + } + + return sb.String() +} + +// substitute returns the text to emit for a single verb. +func (e *templateError) substitute(spec string, verb byte, argIndex int) string { + if argIndex >= len(e.args) { + // More verbs than arguments; go vet reports the call itself. + return spec + } + + arg := e.args[argIndex] + if s, ok := arg.(safeValue); ok { + arg = s.v + } else if verb != 'w' { + return spec + } + + if verb == 'w' { + // %w is only valid in fmt.Errorf, so it is never rendered. Chain the + // wrapped error's template instead, when it has one. + if err, ok := arg.(error); ok { + if inner := ErrorTemplate(err); inner != "" { + return inner + } + } + return spec + } + + return fmt.Sprintf(spec, arg) +} + +// verbFlags are the flag characters fmt accepts between '%' and the verb. +const verbFlags = "+-# 0" + +// parseVerb parses the verb that starts at the '%' at index i, returning its +// full spec (e.g. "%-10q"), the verb letter, and the index just past it. +// +// It reports false for a dangling '%' and for the two constructs that change +// which argument a verb consumes: an explicit argument index (%[2]s) and a '*' +// width or precision (%*d). +func parseVerb(format string, i int) (spec string, verb byte, next int, ok bool) { + j := i + 1 + for j < len(format) && strings.IndexByte(verbFlags, format[j]) >= 0 { + j++ + } + + j, ok = skipNumber(format, j) + if !ok { + return "", 0, 0, false + } + + if j < len(format) && format[j] == '.' { + j, ok = skipNumber(format, j+1) + if !ok { + return "", 0, 0, false + } + } + + if j >= len(format) { + return "", 0, 0, false + } + + return format[i : j+1], format[j], j + 1, true +} + +// skipNumber advances past a width or precision, rejecting the '*' and '[' +// forms that consume an argument of their own. +func skipNumber(format string, j int) (int, bool) { + if j < len(format) && (format[j] == '*' || format[j] == '[') { + return 0, false + } + for j < len(format) && format[j] >= '0' && format[j] <= '9' { + j++ + } + return j, true +} diff --git a/libs/safeerr/safeerr_test.go b/libs/safeerr/safeerr_test.go new file mode 100644 index 00000000000..06b3215a6c4 --- /dev/null +++ b/libs/safeerr/safeerr_test.go @@ -0,0 +1,463 @@ +package safeerr + +import ( + "errors" + "fmt" + "io/fs" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unsafeValue appears in every argument that is not marked Safe, so a test can +// assert it never reaches a template. +const unsafeValue = "resources.jobs.my_secret_job" + +func TestErrorf(t *testing.T) { + tests := []struct { + name string + format string + args []any + wantMessage string + wantTemplate string + }{ + { + name: "no verbs", + format: "state conversion failed", + wantMessage: "state conversion failed", + wantTemplate: "state conversion failed", + }, + { + name: "unsafe string", + format: "%s: getting config", + args: []any{unsafeValue}, + wantMessage: unsafeValue + ": getting config", + wantTemplate: "%s: getting config", + }, + { + name: "unsafe int", + format: "unsupported deployment state version: %d", + args: []any{7}, + wantMessage: "unsupported deployment state version: 7", + wantTemplate: "unsupported deployment state version: %d", + }, + { + name: "safe value is substituted", + format: "%s: cannot set resolved value for field %q", + args: []any{unsafeValue, Safe("tasks[0].job_id")}, + wantMessage: unsafeValue + `: cannot set resolved value for field "tasks[0].job_id"`, + wantTemplate: `%s: cannot set resolved value for field "tasks[0].job_id"`, + }, + { + name: "only safe values", + format: "cannot convert %s to %s", + args: []any{Safe("string"), Safe("int64")}, + wantMessage: "cannot convert string to int64", + wantTemplate: "cannot convert string to int64", + }, + { + name: "escaped percent is preserved", + format: "100%% of %s", + args: []any{unsafeValue}, + wantMessage: "100% of " + unsafeValue, + wantTemplate: "100%% of %s", + }, + { + name: "flags and width on an unsafe verb", + format: "%-12s|", + args: []any{"job"}, + wantMessage: "job |", + wantTemplate: "%-12s|", + }, + { + name: "flags and width on a safe verb", + format: "%-12s|", + args: []any{Safe("job")}, + wantMessage: "job |", + wantTemplate: "job |", + }, + { + name: "width and precision on a safe verb", + format: "%08.3f", + args: []any{Safe(3.5)}, + wantMessage: "0003.500", + wantTemplate: "0003.500", + }, + { + name: "plus v on an unsafe verb", + format: "reading state: %+v", + args: []any{struct{ Path string }{unsafeValue}}, + wantMessage: "reading state: {Path:" + unsafeValue + "}", + wantTemplate: "reading state: %+v", + }, + { + name: "safe bool", + format: "recovery enabled: %v", + args: []any{Safe(true)}, + wantMessage: "recovery enabled: true", + wantTemplate: "recovery enabled: true", + }, + { + name: "mixed safe and unsafe in order", + format: "%s: %s for %s in %s", + args: []any{Safe("jobs"), unsafeValue, Safe("PrepareState"), "/home/user/bundle"}, + wantMessage: "jobs: " + unsafeValue + " for PrepareState in /home/user/bundle", + wantTemplate: "jobs: %s for PrepareState in %s", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Errorf(tt.format, tt.args...) + assert.Equal(t, tt.wantMessage, err.Error()) + assert.Equal(t, tt.wantTemplate, ErrorTemplate(err)) + }) + } +} + +// TestErrorfMessageMatchesFmt is the property that makes converting a call site +// a no-op for every existing consumer, including acceptance test goldens. +func TestErrorfMessageMatchesFmt(t *testing.T) { + tests := []struct { + name string + format string + safe []any + raw []any + }{ + { + name: "quoted safe value", + format: "field %q", + safe: []any{Safe("tasks[0].job_id")}, + raw: []any{"tasks[0].job_id"}, + }, + { + name: "safe struct with plus v", + format: "%+v", + safe: []any{Safe(struct{ A int }{1})}, + raw: []any{struct{ A int }{1}}, + }, + { + name: "safe nil", + format: "%v", + safe: []any{Safe(nil)}, + raw: []any{nil}, + }, + { + name: "safe error with s verb", + format: "%s", + safe: []any{Safe(fs.ErrNotExist)}, + raw: []any{fs.ErrNotExist}, + }, + { + name: "too few arguments", + format: "%s and %s", + safe: []any{Safe("one")}, + raw: []any{"one"}, + }, + { + name: "too many arguments", + format: "%s", + safe: []any{Safe("one"), Safe("two")}, + raw: []any{"one", "two"}, + }, + { + name: "wrong verb for type", + format: "%d", + safe: []any{Safe("not a number")}, + raw: []any{"not a number"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, fmt.Errorf(tt.format, tt.raw...).Error(), Errorf(tt.format, tt.safe...).Error()) + }) + } +} + +func TestErrorTemplateChains(t *testing.T) { + inner := Errorf("cannot convert %s to %s", Safe("string"), Safe("int64")) + middle := Errorf("%s: cannot set resolved value for field %q: %w", unsafeValue, Safe("tasks[0].job_id"), inner) + outer := Errorf("%s: SaveState: %w", unsafeValue, middle) + + assert.Equal(t, + unsafeValue+": SaveState: "+unsafeValue+`: cannot set resolved value for field "tasks[0].job_id": cannot convert string to int64`, + outer.Error()) + assert.Equal(t, + `%s: SaveState: %s: cannot set resolved value for field "tasks[0].job_id": cannot convert string to int64`, + ErrorTemplate(outer)) + + // Each level still reports its own template. + assert.Equal(t, `%s: cannot set resolved value for field "tasks[0].job_id": cannot convert string to int64`, ErrorTemplate(middle)) + assert.Equal(t, "cannot convert string to int64", ErrorTemplate(inner)) +} + +func TestErrorTemplateChainsThroughForeignError(t *testing.T) { + // A %w wrapping an error with no template keeps the bare verb. + err := Errorf("reading %s: %w", "/home/user/state.json", fs.ErrNotExist) + + assert.Equal(t, "reading /home/user/state.json: "+fs.ErrNotExist.Error(), err.Error()) + assert.Equal(t, "reading %s: %w", ErrorTemplate(err)) + assert.ErrorIs(t, err, fs.ErrNotExist) +} + +func TestErrorTemplateChainsThroughUntemplatedWrap(t *testing.T) { + // A plain fmt.Errorf in the middle of the chain contributes nothing, so the + // innermost template that is known reaches the top. + inner := Errorf("cannot look up %q", Safe("continuous.pause_status")) + err := fmt.Errorf("%s: %w", unsafeValue, inner) + + assert.Equal(t, `cannot look up "continuous.pause_status"`, ErrorTemplate(err)) +} + +func TestErrorTemplateChainsSeveralWrappedErrors(t *testing.T) { + first := Errorf("group %s has no adapter", Safe("quality_monitors")) + err := Errorf("%s: %w and %w", unsafeValue, first, fs.ErrPermission) + + assert.Equal(t, "%s: group quality_monitors has no adapter and %w", ErrorTemplate(err)) + assert.ErrorIs(t, err, fs.ErrPermission) + assert.ErrorIs(t, err, first) +} + +func TestErrorTemplateWithoutTemplate(t *testing.T) { + assert.Empty(t, ErrorTemplate(nil)) + assert.Empty(t, ErrorTemplate(errors.New(unsafeValue))) + assert.Empty(t, ErrorTemplate(fmt.Errorf("reading %s", unsafeValue))) + assert.Empty(t, ErrorTemplate(fs.ErrNotExist)) +} + +func TestNew(t *testing.T) { + err := New("state conversion failed") + assert.Equal(t, "state conversion failed", err.Error()) + assert.Equal(t, "state conversion failed", ErrorTemplate(err)) +} + +func TestNewDoesNotFormat(t *testing.T) { + // New takes a literal message, so verbs in it are neither expanded nor lost. + err := New("100% of %s attempts failed") + assert.Equal(t, "100% of %s attempts failed", err.Error()) + assert.Equal(t, "100% of %s attempts failed", ErrorTemplate(err)) +} + +func TestErrorTemplateFallsBackToRawFormat(t *testing.T) { + tests := []struct { + name string + format string + args []any + }{ + {name: "explicit argument index", format: "%[1]s and %[1]s", args: []any{Safe("jobs")}}, + {name: "star width", format: "%*d", args: []any{Safe(5), Safe(42)}}, + {name: "star precision", format: "%.*f", args: []any{Safe(2), Safe(3.5)}}, + {name: "dangling percent", format: "done: 100%", args: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // The format string is a source literal, so reporting it verbatim is + // safe even though its verbs were not analysed. + assert.Equal(t, tt.format, ErrorTemplate(Errorf(tt.format, tt.args...))) + }) + } +} + +func TestErrorsIs(t *testing.T) { + sentinel := errors.New("sentinel") + err := Errorf("%s: %w", unsafeValue, sentinel) + + assert.ErrorIs(t, err, sentinel) + assert.ErrorIs(t, Errorf("outer: %w", err), sentinel) + assert.NotErrorIs(t, Errorf("no wrapping: %s", sentinel), sentinel) +} + +func TestErrorsAsType(t *testing.T) { + err := Errorf("%s: %w", unsafeValue, &fs.PathError{Op: "open", Path: "/tmp/x", Err: fs.ErrNotExist}) + + pathErr, ok := errors.AsType[*fs.PathError](err) + require.True(t, ok) + assert.Equal(t, "open", pathErr.Op) +} + +// TestTemplateNeverLeaksUnsafeValues is the security property of the package: +// nothing that was not marked Safe reaches the template. +func TestTemplateNeverLeaksUnsafeValues(t *testing.T) { + const secret = "SECRET-a1b2c3" + + errs := []error{ + Errorf("%s", secret), + Errorf("%q: %v", secret, errors.New(secret)), + Errorf("%s: %w", secret, errors.New(secret)), + Errorf("%s: %w", secret, Errorf("inner %s: %w", secret, fs.ErrNotExist)), + Errorf("%v", struct{ Name string }{secret}), + Errorf("%s and %s", Safe("jobs"), secret), + Errorf("%[1]s", secret), + Errorf("%*s", Safe(4), secret), + } + + for _, err := range errs { + require.Contains(t, err.Error(), secret, "message should carry the value") + assert.NotContains(t, ErrorTemplate(err), secret, "template must not carry the value") + } +} + +func TestParseVerb(t *testing.T) { + tests := []struct { + format string + wantSpec string + wantVerb byte + wantNext int + wantOk bool + }{ + {format: "%s", wantSpec: "%s", wantVerb: 's', wantNext: 2, wantOk: true}, + {format: "%%", wantSpec: "%%", wantVerb: '%', wantNext: 2, wantOk: true}, + {format: "%+v", wantSpec: "%+v", wantVerb: 'v', wantNext: 3, wantOk: true}, + {format: "%#v", wantSpec: "%#v", wantVerb: 'v', wantNext: 3, wantOk: true}, + {format: "%-12q", wantSpec: "%-12q", wantVerb: 'q', wantNext: 5, wantOk: true}, + {format: "%08.3f", wantSpec: "%08.3f", wantVerb: 'f', wantNext: 6, wantOk: true}, + {format: "% d", wantSpec: "% d", wantVerb: 'd', wantNext: 3, wantOk: true}, + {format: "%w", wantSpec: "%w", wantVerb: 'w', wantNext: 2, wantOk: true}, + {format: "%[1]s", wantOk: false}, + {format: "%*d", wantOk: false}, + {format: "%.*f", wantOk: false}, + {format: "%", wantOk: false}, + {format: "%-", wantOk: false}, + } + + for _, tt := range tests { + t.Run(tt.format, func(t *testing.T) { + spec, verb, next, ok := parseVerb(tt.format, 0) + assert.Equal(t, tt.wantOk, ok) + if !tt.wantOk { + return + } + assert.Equal(t, tt.wantSpec, spec) + assert.Equal(t, string(tt.wantVerb), string(verb)) + assert.Equal(t, tt.wantNext, next) + }) + } +} + +// TestErrorTemplateIsStableAcrossCalls guards against expand mutating state. +func TestErrorTemplateIsStableAcrossCalls(t *testing.T) { + err := Errorf("%s: field %q: %w", unsafeValue, Safe("id"), Errorf("inner %d", Safe(2))) + first := ErrorTemplate(err) + assert.Equal(t, first, ErrorTemplate(err)) + assert.Equal(t, first, ErrorTemplate(err)) +} + +// TestUnsafeArgsAreNotRetained checks that unsafe values are dropped at +// construction rather than kept alive inside the error for later inspection. +func TestUnsafeArgsAreNotRetained(t *testing.T) { + err := Errorf("%s: %d", unsafeValue, 42) + + te, ok := errors.AsType[*templateError](err) + require.True(t, ok) + require.Len(t, te.args, 2) + for i, arg := range te.args { + assert.Nil(t, arg, "argument %d should not be retained", i) + } +} + +func TestSafeArgsAreRetainedForTemplating(t *testing.T) { + err := Errorf("%s: %s", Safe("jobs"), unsafeValue) + + te, ok := errors.AsType[*templateError](err) + require.True(t, ok) + require.Len(t, te.args, 2) + assert.Equal(t, safeValue{v: "jobs"}, te.args[0]) + assert.Nil(t, te.args[1]) +} + +func TestErrorTemplateDeepChain(t *testing.T) { + err := New("root cause") + for range 5 { + err = Errorf("%s: %w", unsafeValue, err) + } + + assert.Equal(t, strings.Repeat("%s: ", 5)+"root cause", ErrorTemplate(err)) +} + +// safeStringerKey stands in for a resource key: the full value carries a +// user-chosen name, the stand-in keeps only the group. +type safeStringerKey string + +func (k safeStringerKey) SafeString() string { return "resources.jobs.*" } + +// safeStringerErr implements both error and SafeStringer, which resolves as +// error so that %w keeps chaining. +type safeStringerErr struct{} + +func (safeStringerErr) Error() string { return "boom" } +func (safeStringerErr) SafeString() string { return "SHOULD-NOT-APPEAR" } + +func TestSafeStringer(t *testing.T) { + key := safeStringerKey("resources.jobs.my_job") + + tests := []struct { + name string + format string + args []any + wantMessage string + wantTemplate string + }{ + { + name: "s verb", + format: "cannot update %s: %w", + args: []any{key, fs.ErrPermission}, + wantMessage: "cannot update resources.jobs.my_job: " + fs.ErrPermission.Error(), + wantTemplate: "cannot update resources.jobs.*: %w", + }, + { + name: "q verb quotes the stand-in like the value", + format: "%q not found", + args: []any{key}, + wantMessage: `"resources.jobs.my_job" not found`, + wantTemplate: `"resources.jobs.*" not found`, + }, + { + name: "alongside Safe and unsafe args", + format: "cannot %s %s: field %q: %s", + args: []any{Safe("update"), key, Safe("tasks[0].job_id"), unsafeValue}, + wantMessage: "cannot update resources.jobs.my_job: field \"tasks[0].job_id\": " + unsafeValue, + wantTemplate: `cannot update resources.jobs.*: field "tasks[0].job_id": %s`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Errorf(tt.format, tt.args...) + assert.Equal(t, tt.wantMessage, err.Error()) + assert.Equal(t, tt.wantTemplate, ErrorTemplate(err)) + }) + } +} + +func TestSafeStringerValueIsNotRetained(t *testing.T) { + err := Errorf("%s", safeStringerKey("resources.jobs.my_job")) + + te, ok := errors.AsType[*templateError](err) + require.True(t, ok) + require.Len(t, te.args, 1) + // Only the stand-in survives; the key itself is gone. + assert.Equal(t, safeValue{v: "resources.jobs.*"}, te.args[0]) +} + +func TestSafeStringerErrorIsTreatedAsError(t *testing.T) { + // Both interfaces: error wins, so %w still chains and the stand-in is unused. + inner := Errorf("inner %d", Safe(1)) + err := Errorf("outer: %w", inner) + assert.Equal(t, "outer: inner 1", ErrorTemplate(err)) + + err = Errorf("outer: %w", safeStringerErr{}) + assert.Equal(t, "outer: %w", ErrorTemplate(err)) + assert.NotContains(t, ErrorTemplate(err), "SHOULD-NOT-APPEAR") +} + +func TestSafeStringerOutranksSafe(t *testing.T) { + // Wrapping a SafeStringer in Safe must not put its user-supplied part back + // into the template. + err := Errorf("%s", Safe(safeStringerKey("resources.jobs.my_job"))) + assert.Equal(t, "resources.jobs.my_job", err.Error()) + assert.Equal(t, "resources.jobs.*", ErrorTemplate(err)) +} diff --git a/libs/telemetry/protos/bundle_deploy.go b/libs/telemetry/protos/bundle_deploy.go index 69e01a30715..674b4090121 100644 --- a/libs/telemetry/protos/bundle_deploy.go +++ b/libs/telemetry/protos/bundle_deploy.go @@ -113,6 +113,18 @@ type BundleDeployExperimental struct { // Local cache measurements in milliseconds (compute duration, potential savings, etc.) LocalCacheMeasurementsMs []IntMapEntry `json:"local_cache_measurements_ms,omitempty"` + + // PII-free descriptions of errors, keyed by which error is described + // ("error_template", "direct_migrate_error_template", ...). Each value is a + // message template produced by libs/safeerr: the format string of the error, + // with everything the user supplied left as a verb, plus the safe fields of + // any API error at the end of the chain. + // + // This is the aggregatable counterpart to BundleDeployEvent.ErrorMessage, + // which is scrubbed heuristically and still treated as privileged. Values + // here are composed of source literals and closed enums only, so they need + // no scrubbing. As with BoolValues, a new key needs no proto change. + StringValues []StringMapEntry `json:"string_values,omitempty"` } // BundleResourcesMetadata mirrors the universe proto. Per-resource-type counts @@ -163,3 +175,8 @@ type IntMapEntry struct { Key string `json:"key,omitempty"` Value int64 `json:"value"` } + +type StringMapEntry struct { + Key string `json:"key,omitempty"` + Value string `json:"value,omitempty"` +} From a39582f1c80877f136054d93869d097580b313e4 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 24 Aug 2026 13:24:19 +0200 Subject: [PATCH 02/11] Drop redundant telemetry payload goldens from the two error-template 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 --- .../migrate/auto-migrate-push-failure/out.telemetry.txt | 9 --------- .../bundle/migrate/auto-migrate-push-failure/output.txt | 2 +- .../bundle/migrate/auto-migrate-push-failure/script | 6 ------ .../telemetry/deploy-error-template/out.telemetry.txt | 9 --------- acceptance/bundle/telemetry/deploy-error-template/script | 4 ---- 5 files changed, 1 insertion(+), 29 deletions(-) delete mode 100644 acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt delete mode 100644 acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt b/acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt deleted file mode 100644 index 4e501c95795..00000000000 --- a/acceptance/bundle/migrate/auto-migrate-push-failure/out.telemetry.txt +++ /dev/null @@ -1,9 +0,0 @@ -{ - "error_message": null, - "string_values": [ - { - "key": "direct_migrate_error_template", - "value": "pushing direct state to workspace: %w [INJECTED 403]" - } - ] -} diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt index 27a8ab5bfb0..cb18690cb1a 100644 --- a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt +++ b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt @@ -36,7 +36,7 @@ Warn: Direct engine requested in DATABRICKS_BUNDLE_ENGINE environment variable b Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... Migrating state to direct deployment engine (opted in via DATABRICKS_BUNDLE_ENGINE environment variable)... Migrated 1 resource to direct deployment engine. -Files: 3 uploaded, 0 deleted +Files: 2 uploaded, 0 deleted Resources: 0 created, 0 changed, 0 deleted, 1 unchanged >>> print_migration_telemetry diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/script b/acceptance/bundle/migrate/auto-migrate-push-failure/script index 08cfb53d345..b1ccb759da5 100644 --- a/acceptance/bundle/migrate/auto-migrate-push-failure/script +++ b/acceptance/bundle/migrate/auto-migrate-push-failure/script @@ -12,12 +12,6 @@ title "Opt in via env var; inject 403 on the resources.json push so commit fails fault.py "POST /api/2.0/workspace-files/import-file/Workspace/Users/$CURRENT_USER_NAME/.bundle/test-bundle/default/state/resources.json" 403 0 1 trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy trace print_migration_telemetry - -# Record the migration error fields exactly as they leave the machine. The deploy -# itself succeeded, so error_message is absent and the only thing reported about -# the failed migration is the PII-free template — note the workspace path and -# user name in the warning above are nowhere in it. -cat out.requests.txt | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson | {error_message: .entry.databricks_cli_log.bundle_deploy_event.error_message, string_values: .entry.databricks_cli_log.bundle_deploy_event.experimental.string_values}' > out.telemetry.txt rm -f out.requests.txt title "Local state was NOT rewritten (still terraform)\n" diff --git a/acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt b/acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt deleted file mode 100644 index 413016c62da..00000000000 --- a/acceptance/bundle/telemetry/deploy-error-template/out.telemetry.txt +++ /dev/null @@ -1,9 +0,0 @@ -{ - "error_message": "access denied: [REDACTED_WORKSPACE_PATH](json) (403 INJECTED)", - "string_values": [ - { - "key": "error_template", - "value": "INJECTED 403" - } - ] -} diff --git a/acceptance/bundle/telemetry/deploy-error-template/script b/acceptance/bundle/telemetry/deploy-error-template/script index e6a3f15f318..7a108a98b00 100644 --- a/acceptance/bundle/telemetry/deploy-error-template/script +++ b/acceptance/bundle/telemetry/deploy-error-template/script @@ -13,8 +13,4 @@ title "The message carries the workspace path and the user; the template carries trace print_telemetry_error_message trace print_telemetry_string_values -# Record the error fields exactly as they leave the machine, so the golden shows -# the payload rather than a helper's rendering of it. -cat out.requests.txt | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson | {error_message: .entry.databricks_cli_log.bundle_deploy_event.error_message, string_values: .entry.databricks_cli_log.bundle_deploy_event.experimental.string_values}' > out.telemetry.txt - rm out.requests.txt From a371041cb466e4a6858337920f30e51330c52e73 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 24 Aug 2026 13:48:21 +0200 Subject: [PATCH 03/11] Report error templates as dedicated fields instead of a generic string 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 --- .../auto-migrate-push-failure/output.txt | 2 +- .../auto-migrate-tfbackup-failure/output.txt | 2 +- acceptance/bundle/migrate/script.prepare | 2 +- .../deploy-error-template/output.txt | 2 +- .../telemetry/deploy-error-template/script | 2 +- acceptance/script.prepare | 6 ++- bundle/bundle.go | 21 ++++------- bundle/metrics/metrics.go | 16 -------- bundle/phases/telemetry.go | 7 +++- bundle/statemgmt/direct_migration.go | 21 ++++++----- cmd/bundle/utils/process.go | 5 +-- libs/telemetry/protos/bundle_deploy.go | 37 +++++++++++-------- 12 files changed, 56 insertions(+), 67 deletions(-) diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt index cb18690cb1a..3a08031b655 100644 --- a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt +++ b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt @@ -21,7 +21,7 @@ Resources: 0 created, 0 changed, 0 deleted, 1 unchanged >>> print_migration_telemetry direct_migrate_commit_error true -direct_migrate_error_template pushing direct state to workspace: %w [INJECTED 403] +direct_migrate_commit_error_template pushing direct state to workspace: %w [INJECTED 403] === Local state was NOT rewritten (still terraform) diff --git a/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt index f18ddbe9d56..3c3f2a02f3e 100644 --- a/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt +++ b/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt @@ -21,7 +21,7 @@ Resources: 0 created, 0 changed, 0 deleted, 1 unchanged >>> print_migration_telemetry direct_migrate_commit_error true -direct_migrate_error_template pushing direct state to workspace: deleting remote terraform state: %w [INJECTED 403] +direct_migrate_commit_error_template pushing direct state to workspace: deleting remote terraform state: %w [INJECTED 403] === Local state was NOT rewritten (still terraform) diff --git a/acceptance/bundle/migrate/script.prepare b/acceptance/bundle/migrate/script.prepare index d72b46b5649..df85baabe07 100644 --- a/acceptance/bundle/migrate/script.prepare +++ b/acceptance/bundle/migrate/script.prepare @@ -6,5 +6,5 @@ # assertion. print_migration_telemetry() { print_telemetry_bool_values | grep '^direct_' || true - print_telemetry_string_values | grep '^direct_' || true + print_telemetry_error_templates | grep '^direct_' || true } diff --git a/acceptance/bundle/telemetry/deploy-error-template/output.txt b/acceptance/bundle/telemetry/deploy-error-template/output.txt index d78fe00c770..396cde051a8 100644 --- a/acceptance/bundle/telemetry/deploy-error-template/output.txt +++ b/acceptance/bundle/telemetry/deploy-error-template/output.txt @@ -15,5 +15,5 @@ Exit code: 1 >>> print_telemetry_error_message access denied: [REDACTED_WORKSPACE_PATH](json) (403 INJECTED) ->>> print_telemetry_string_values +>>> print_telemetry_error_templates error_template INJECTED 403 diff --git a/acceptance/bundle/telemetry/deploy-error-template/script b/acceptance/bundle/telemetry/deploy-error-template/script index 7a108a98b00..1e13ea38cf9 100644 --- a/acceptance/bundle/telemetry/deploy-error-template/script +++ b/acceptance/bundle/telemetry/deploy-error-template/script @@ -11,6 +11,6 @@ errcode trace $CLI bundle deploy -qq title "The message carries the workspace path and the user; the template carries neither\n" trace print_telemetry_error_message -trace print_telemetry_string_values +trace print_telemetry_error_templates rm out.requests.txt diff --git a/acceptance/script.prepare b/acceptance/script.prepare index 1db315d650a..4a227c994c4 100644 --- a/acceptance/script.prepare +++ b/acceptance/script.prepare @@ -127,8 +127,10 @@ print_telemetry_error_message() { jq -r 'select(.path? == "/telemetry-ext") | (.body.protoLogs // [])[] | fromjson | ( (.entry // .) | .databricks_cli_log.bundle_deploy_event.error_message // empty )' out.requests.txt } -print_telemetry_string_values() { - jq -r 'select(.path? == "/telemetry-ext") | (.body.protoLogs // [])[] | fromjson | ( (.entry // .) | (.databricks_cli_log.bundle_deploy_event.experimental.string_values // []) ) | map("\(.key) \(.value)") | .[]' out.requests.txt | sort +# Print the PII-free error templates the deploy event carries, one " +#