From 954bad2fc8d62053c3989f21f3763f296b3826ce Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 7 Sep 2026 16:16:04 +0200 Subject: [PATCH] direct: report why a migration failed in telemetry A failed migration to the direct engine reported nothing about why, so the failures could not be told apart in aggregate. The blocker is that an error message is user data: it interpolates resource names, workspace paths and config values. libs/safeerr keeps the second, PII-free half of an error alongside the usual one. Errorf behaves exactly like fmt.Errorf, and SafeError returns the same message with only the format string and the arguments the call site vouched for: Safe() for a value the CLI itself defines, or a SafeStringer stand-in for a value that is partly the user's ("resources.jobs.my_job" reports as "jobs.*"). Every other verb is escaped to literal text, so an unmarked value cannot reach it. The migration path reports three of these: the error that stopped a dry run, the error that stopped a commit, and the first conversion warning. Co-authored-by: Isaac --- .../databricks.yml | 13 + .../out.test.toml | 3 + .../output.txt | 46 ++ .../auto-migrate-conversion-failure/script | 20 + .../auto-migrate-push-failure/output.txt | 1 + .../auto-migrate-secret-scope/databricks.yml | 10 + .../auto-migrate-secret-scope/out.test.toml | 3 + .../auto-migrate-secret-scope/output.txt | 41 ++ .../migrate/auto-migrate-secret-scope/script | 25 + .../auto-migrate-secret-scope/test.toml | 3 + .../auto-migrate-tfbackup-failure/output.txt | 1 + .../reference-dabs-only-field/databricks.yml | 24 + .../reference-dabs-only-field/out.test.toml | 3 + .../reference-dabs-only-field/output.txt | 59 ++ .../migrate/reference-dabs-only-field/script | 24 + .../reference-direct-only-resource/app/app.py | 1 + .../databricks.yml | 13 + .../out.test.toml | 3 + .../reference-direct-only-resource/output.txt | 22 + .../reference-direct-only-resource/script | 18 + .../reference-methods-disagree/databricks.yml | 15 + .../drift_stored_name.py | 19 + .../reference-methods-disagree/out.test.toml | 3 + .../reference-methods-disagree/output.txt | 38 ++ .../migrate/reference-methods-disagree/script | 17 + .../reference-terraform-syntax/databricks.yml | 13 + .../reference-terraform-syntax/out.test.toml | 3 + .../reference-terraform-syntax/output.txt | 26 + .../migrate/reference-terraform-syntax/script | 21 + acceptance/bundle/migrate/script.prepare | 13 +- .../bump_state_version.py | 11 + .../databricks.yml | 11 + .../tfstate-version-unsupported/out.test.toml | 3 + .../tfstate-version-unsupported/output.txt | 34 + .../tfstate-version-unsupported/script | 17 + acceptance/script.prepare | 6 + bundle/bundle.go | 7 + .../resourcemutator/secret_scope_fixups.go | 56 +- .../secret_scope_fixups_test.go | 23 + bundle/config/resource_key.go | 48 ++ bundle/config/resource_key_test.go | 95 +++ bundle/deploy/terraform/util.go | 3 +- bundle/migrate/build_state.go | 55 +- bundle/migrate/build_state_test.go | 159 ++++- bundle/migrate/resolve.go | 15 +- bundle/migrate/tf_state.go | 16 +- bundle/mutator.go | 24 + bundle/phases/telemetry.go | 4 + bundle/statemgmt/direct_migration.go | 75 ++- .../statemgmt/upload_state_for_yaml_sync.go | 2 +- cmd/bundle/deployment/migrate.go | 2 +- libs/diag/diagnostic.go | 20 + libs/diag/safe_error_test.go | 127 ++++ libs/diag/sdk_error.go | 51 +- libs/filer/errors.go | 68 +- libs/filer/errors_test.go | 71 ++ libs/safeerr/safeerr.go | 297 +++++++++ libs/safeerr/safeerr_test.go | 622 ++++++++++++++++++ libs/telemetry/protos/bundle_deploy.go | 28 + libs/telemetry/protos/bundle_deploy_test.go | 38 ++ 60 files changed, 2394 insertions(+), 95 deletions(-) create mode 100644 acceptance/bundle/migrate/auto-migrate-conversion-failure/databricks.yml create mode 100644 acceptance/bundle/migrate/auto-migrate-conversion-failure/out.test.toml create mode 100644 acceptance/bundle/migrate/auto-migrate-conversion-failure/output.txt create mode 100644 acceptance/bundle/migrate/auto-migrate-conversion-failure/script create mode 100644 acceptance/bundle/migrate/auto-migrate-secret-scope/databricks.yml create mode 100644 acceptance/bundle/migrate/auto-migrate-secret-scope/out.test.toml create mode 100644 acceptance/bundle/migrate/auto-migrate-secret-scope/output.txt create mode 100644 acceptance/bundle/migrate/auto-migrate-secret-scope/script create mode 100644 acceptance/bundle/migrate/auto-migrate-secret-scope/test.toml create mode 100644 acceptance/bundle/migrate/reference-dabs-only-field/databricks.yml create mode 100644 acceptance/bundle/migrate/reference-dabs-only-field/out.test.toml create mode 100644 acceptance/bundle/migrate/reference-dabs-only-field/output.txt create mode 100644 acceptance/bundle/migrate/reference-dabs-only-field/script create mode 100644 acceptance/bundle/migrate/reference-direct-only-resource/app/app.py create mode 100644 acceptance/bundle/migrate/reference-direct-only-resource/databricks.yml create mode 100644 acceptance/bundle/migrate/reference-direct-only-resource/out.test.toml create mode 100644 acceptance/bundle/migrate/reference-direct-only-resource/output.txt create mode 100644 acceptance/bundle/migrate/reference-direct-only-resource/script create mode 100644 acceptance/bundle/migrate/reference-methods-disagree/databricks.yml create mode 100644 acceptance/bundle/migrate/reference-methods-disagree/drift_stored_name.py create mode 100644 acceptance/bundle/migrate/reference-methods-disagree/out.test.toml create mode 100644 acceptance/bundle/migrate/reference-methods-disagree/output.txt create mode 100644 acceptance/bundle/migrate/reference-methods-disagree/script create mode 100644 acceptance/bundle/migrate/reference-terraform-syntax/databricks.yml create mode 100644 acceptance/bundle/migrate/reference-terraform-syntax/out.test.toml create mode 100644 acceptance/bundle/migrate/reference-terraform-syntax/output.txt create mode 100644 acceptance/bundle/migrate/reference-terraform-syntax/script create mode 100644 acceptance/bundle/migrate/tfstate-version-unsupported/bump_state_version.py create mode 100644 acceptance/bundle/migrate/tfstate-version-unsupported/databricks.yml create mode 100644 acceptance/bundle/migrate/tfstate-version-unsupported/out.test.toml create mode 100644 acceptance/bundle/migrate/tfstate-version-unsupported/output.txt create mode 100644 acceptance/bundle/migrate/tfstate-version-unsupported/script create mode 100644 bundle/config/resource_key.go create mode 100644 bundle/config/resource_key_test.go create mode 100644 libs/diag/safe_error_test.go create mode 100644 libs/safeerr/safeerr.go create mode 100644 libs/safeerr/safeerr_test.go create mode 100644 libs/telemetry/protos/bundle_deploy_test.go diff --git a/acceptance/bundle/migrate/auto-migrate-conversion-failure/databricks.yml b/acceptance/bundle/migrate/auto-migrate-conversion-failure/databricks.yml new file mode 100644 index 00000000000..25e8ccbf913 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-conversion-failure/databricks.yml @@ -0,0 +1,13 @@ +bundle: + name: test-bundle + +resources: + secret_scopes: + my_scope: + name: my-scope + permissions: + # An unknown level: SecretScopeFixups rejects it when it prepares the + # config for the direct engine, so the state conversion fails while the + # terraform deploy itself succeeds. + - level: BOGUS + user_name: someone@example.com diff --git a/acceptance/bundle/migrate/auto-migrate-conversion-failure/out.test.toml b/acceptance/bundle/migrate/auto-migrate-conversion-failure/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-conversion-failure/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/auto-migrate-conversion-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-conversion-failure/output.txt new file mode 100644 index 00000000000..c4b7ef2d150 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-conversion-failure/output.txt @@ -0,0 +1,46 @@ + +=== Not opted in: the conversion failure is reported as a failed dry run +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Warning: invalid value "BOGUS" for enum field. Valid values are [READ WRITE MANAGE] + at resources.secret_scopes.my_scope.permissions[0].level + +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created secret_scopes.my_scope +Created secret_scopes.my_scope.permissions +Files: 4 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged +Warn: post-deploy dry-run migration to direct: unknown permission level "BOGUS" for secret scope +Warn: The warnings above are from a dry-run migration to the direct deployment engine (https://docs.databricks.com/aws/en/dev-tools/bundles/direct). +Your deployment is not affected and works normally, but you may experience these issues when migrating to the direct deployment engine. +Please forward these warnings to dabs-feedback@databricks.com + +>>> print_migration_telemetry +direct_drymigrate_success false +direct_drymigrate_warnings false +direct_migrate_safe_error unknown permission level %q for secret scope + +=== Opted in: the same failure stops the automatic migration +>>> DATABRICKS_BUNDLE_ENGINE=direct [CLI] bundle deploy +Warning: invalid value "BOGUS" for enum field. Valid values are [READ WRITE MANAGE] + at resources.secret_scopes.my_scope.permissions[0].level + +Warn: Direct engine selected via DATABRICKS_BUNDLE_ENGINE environment variable but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Files: 2 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged +Warn: post-deploy dry-run migration to direct: unknown permission level "BOGUS" for secret scope +Warn: The warnings above are from a dry-run migration to the direct deployment engine (https://docs.databricks.com/aws/en/dev-tools/bundles/direct). +Your deployment is not affected and works normally, but you may experience these issues when migrating to the direct deployment engine. +Please forward these warnings to dabs-feedback@databricks.com +Warn: Direct engine was selected but the migration reported issues; automatic migration to the direct deployment engine is stopped. Address the issues above or run "databricks bundle deployment migrate" manually. + +>>> print_migration_telemetry +direct_migrate_error true +direct_migrate_safe_error unknown permission level %q for secret scope + +=== State is still terraform, so nothing was migrated + +>>> find .databricks/bundle -name resources.json -type f + +>>> find .databricks/bundle -name terraform.tfstate* -type f +.databricks/bundle/default/terraform/terraform.tfstate diff --git a/acceptance/bundle/migrate/auto-migrate-conversion-failure/script b/acceptance/bundle/migrate/auto-migrate-conversion-failure/script new file mode 100644 index 00000000000..efad1c5fab8 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-conversion-failure/script @@ -0,0 +1,20 @@ +export DATABRICKS_BUNDLE_ENGINE= + +# An unknown permission level is only a warning for the terraform deploy, but +# SecretScopeFixups rejects it while preparing the config for the direct engine. +# That is a state conversion failure: no API call is involved, so unlike the +# commit failures it cannot be produced by injecting a fault. + +title "Not opted in: the conversion failure is reported as a failed dry run" +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +title "Opted in: the same failure stops the automatic migration" +trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +title "State is still terraform, so nothing was migrated\n" +trace find .databricks/bundle -name "resources.json" -type f +trace find .databricks/bundle -name "terraform.tfstate*" -type f diff --git a/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-push-failure/output.txt index 94d30286b16..9e9c8b17eb6 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 @@ Warn: automatic migration to direct engine failed: pushing direct state to works >>> print_migration_telemetry direct_migrate_commit_error true +direct_migrate_commit_safe_error pushing direct state to workspace: access denied [403 INJECTED] === Local state was NOT rewritten (still terraform) diff --git a/acceptance/bundle/migrate/auto-migrate-secret-scope/databricks.yml b/acceptance/bundle/migrate/auto-migrate-secret-scope/databricks.yml new file mode 100644 index 00000000000..8e61df03c21 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-secret-scope/databricks.yml @@ -0,0 +1,10 @@ +bundle: + name: test-bundle + +resources: + secret_scopes: + my_scope: + name: my-scope + permissions: + - level: READ + user_name: someone@example.com diff --git a/acceptance/bundle/migrate/auto-migrate-secret-scope/out.test.toml b/acceptance/bundle/migrate/auto-migrate-secret-scope/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-secret-scope/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/auto-migrate-secret-scope/output.txt b/acceptance/bundle/migrate/auto-migrate-secret-scope/output.txt new file mode 100644 index 00000000000..8d674d1ba5c --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-secret-scope/output.txt @@ -0,0 +1,41 @@ + +=== Initial deploy uses the terraform engine +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created secret_scopes.my_scope +Created secret_scopes.my_scope.permissions +Files: 5 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +=== Opt in to the direct engine and redeploy, which migrates the state +>>> update_file.py databricks.yml name: test-bundle name: test-bundle + engine: direct + +>>> [CLI] bundle deploy +Warn: Direct engine selected via bundle.engine setting at [TEST_TMP_DIR]/databricks.yml:3:11 but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged +Migrating state to direct deployment engine (selected via bundle.engine setting at [TEST_TMP_DIR]/databricks.yml:3:11)... +Migrated 2 resources to direct deployment engine. + +>>> print_migration_telemetry +direct_migrated_via_config true + +=== The migrated state records the MANAGE ACL the fixups added + +>>> print_state.py +[ + { + "permission": "READ", + "principal": "someone@example.com" + }, + { + "permission": "MANAGE", + "principal": "[USERNAME]" + } +] + +=== So the first plan after migrating is a no-op +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged diff --git a/acceptance/bundle/migrate/auto-migrate-secret-scope/script b/acceptance/bundle/migrate/auto-migrate-secret-scope/script new file mode 100644 index 00000000000..947046c655e --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-secret-scope/script @@ -0,0 +1,25 @@ +export DATABRICKS_BUNDLE_ENGINE= + +# SecretScopeFixups adds a MANAGE ACL for the current user, which the backend +# creates implicitly. The fixups mutate typed config while the state is built +# from the dynamic tree, so the migration has to sync one into the other; +# without that the ACL is missing from the state below. The plan is a no-op +# either way, because the acls resource re-reads the live ACLs. + +title "Initial deploy uses the terraform engine" +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +rm -f out.requests.txt + +title "Opt in to the direct engine and redeploy, which migrates the state" +trace update_file.py databricks.yml "name: test-bundle" $'name: test-bundle\n engine: direct' +trace $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +title "The migrated state records the MANAGE ACL the fixups added\n" +trace print_state.py | jq '.state["resources.secret_scopes.my_scope.permissions"].state.acls' + +title "So the first plan after migrating is a no-op" +trace $CLI bundle plan | contains.py "2 unchanged" + +rm -f out.requests.txt diff --git a/acceptance/bundle/migrate/auto-migrate-secret-scope/test.toml b/acceptance/bundle/migrate/auto-migrate-secret-scope/test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/auto-migrate-secret-scope/test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt b/acceptance/bundle/migrate/auto-migrate-tfbackup-failure/output.txt index ce3f99f0f14..231285f3c86 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 @@ Warn: automatic migration to direct engine failed: pushing direct state to works >>> print_migration_telemetry direct_migrate_commit_error true +direct_migrate_commit_safe_error pushing direct state to workspace: deleting remote terraform state: %w [403 INJECTED] === Local state was NOT rewritten (still terraform) diff --git a/acceptance/bundle/migrate/reference-dabs-only-field/databricks.yml b/acceptance/bundle/migrate/reference-dabs-only-field/databricks.yml new file mode 100644 index 00000000000..ee0a2c57473 --- /dev/null +++ b/acceptance/bundle/migrate/reference-dabs-only-field/databricks.yml @@ -0,0 +1,24 @@ +bundle: + name: test-bundle + +resources: + jobs: + src: + name: source + tasks: + - task_key: t + new_cluster: &cluster + spark_version: 15.4.x-scala2.12 + node_type_id: Standard_DS3_v2 + num_workers: 1 + # autotermination_minutes has no Terraform equivalent for a job + # cluster, so it is absent from terraform.tfstate. Terraform never + # sees this reference either: the field is dropped on conversion. + autotermination_minutes: 20 + dst: + name: dst + tasks: + - task_key: t + new_cluster: + <<: *cluster + autotermination_minutes: ${resources.jobs.src.tasks[0].new_cluster.autotermination_minutes} diff --git a/acceptance/bundle/migrate/reference-dabs-only-field/out.test.toml b/acceptance/bundle/migrate/reference-dabs-only-field/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/reference-dabs-only-field/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/reference-dabs-only-field/output.txt b/acceptance/bundle/migrate/reference-dabs-only-field/output.txt new file mode 100644 index 00000000000..8c78095a59c --- /dev/null +++ b/acceptance/bundle/migrate/reference-dabs-only-field/output.txt @@ -0,0 +1,59 @@ + +=== Deploy on terraform: the dry run already reports the failure +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Warning: unknown field: autotermination_minutes + at task[0].new_cluster + in databricks.yml:17:13 + +Warning: unknown field: autotermination_minutes + at task[0].new_cluster + in databricks.yml:17:13 + +Created jobs.dst +Created jobs.src +Files: 4 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged +Warn: post-deploy dry-run migration to direct: resources.jobs.dst: cannot resolve field "tasks[0].new_cluster.autotermination_minutes" (template "${resources.jobs.src.tasks[0].new_cluster.autotermination_minutes}"): jobs.dst field tasks[0].new_cluster.autotermination_minutes: method A: jobs: "tasks[0].new_cluster.autotermination_minutes" is a DABs-only field with no Terraform equivalent; method B: cannot look up "resources.jobs.src.tasks[0].new_cluster.autotermination_minutes": jobs: "tasks[0].new_cluster.autotermination_minutes" is a DABs-only field with no Terraform equivalent +Warn: The warnings above are from a dry-run migration to the direct deployment engine (https://docs.databricks.com/aws/en/dev-tools/bundles/direct). +Your deployment is not affected and works normally, but you may experience these issues when migrating to the direct deployment engine. +Please forward these warnings to dabs-feedback@databricks.com + +>>> print_migration_telemetry +direct_drymigrate_success false +direct_drymigrate_warnings false +direct_migrate_safe_error jobs.*: cannot resolve field %q (template %q): jobs.%s field %s: method A: %w; method B: cannot look up %q: %w + +>>> update_file.py databricks.yml name: test-bundle name: test-bundle + engine: direct + +=== Opt in via config: the same failure stops the automatic migration +>>> [CLI] bundle deploy +Warn: Direct engine selected via bundle.engine setting at [TEST_TMP_DIR]/databricks.yml:3:11 but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Warning: unknown field: autotermination_minutes + at task[0].new_cluster + in databricks.yml:18:13 + +Warning: unknown field: autotermination_minutes + at task[0].new_cluster + in databricks.yml:18:13 + +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged +Warn: post-deploy dry-run migration to direct: resources.jobs.dst: cannot resolve field "tasks[0].new_cluster.autotermination_minutes" (template "${resources.jobs.src.tasks[0].new_cluster.autotermination_minutes}"): jobs.dst field tasks[0].new_cluster.autotermination_minutes: method A: jobs: "tasks[0].new_cluster.autotermination_minutes" is a DABs-only field with no Terraform equivalent; method B: cannot look up "resources.jobs.src.tasks[0].new_cluster.autotermination_minutes": jobs: "tasks[0].new_cluster.autotermination_minutes" is a DABs-only field with no Terraform equivalent +Warn: The warnings above are from a dry-run migration to the direct deployment engine (https://docs.databricks.com/aws/en/dev-tools/bundles/direct). +Your deployment is not affected and works normally, but you may experience these issues when migrating to the direct deployment engine. +Please forward these warnings to dabs-feedback@databricks.com +Warn: Direct engine was selected but the migration reported issues; automatic migration to the direct deployment engine is stopped. Address the issues above or run "databricks bundle deployment migrate" manually. + +>>> print_migration_telemetry +direct_migrate_error true +direct_migrate_safe_error jobs.*: cannot resolve field %q (template %q): jobs.%s field %s: method A: %w; method B: cannot look up %q: %w + +=== State is still terraform, so nothing was migrated + +>>> find .databricks/bundle -name resources.json -type f + +>>> find .databricks/bundle -name terraform.tfstate* -type f +.databricks/bundle/default/terraform/terraform.tfstate diff --git a/acceptance/bundle/migrate/reference-dabs-only-field/script b/acceptance/bundle/migrate/reference-dabs-only-field/script new file mode 100644 index 00000000000..1b4e4a4b449 --- /dev/null +++ b/acceptance/bundle/migrate/reference-dabs-only-field/script @@ -0,0 +1,24 @@ +export DATABRICKS_BUNDLE_ENGINE= + +# A DABs-only field pointing at another resource's DABs-only field. Neither is in +# terraform.tfstate, so both of the conversion's resolution methods fail. +# +# The deploy is unaffected: the field has no Terraform equivalent, so it is +# dropped on conversion and Terraform never sees the reference inside it. That is +# what lets the conversion, rather than the deploy, be the thing that fails. +title "Deploy on terraform: the dry run already reports the failure" +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +trace update_file.py databricks.yml "name: test-bundle" "name: test-bundle + engine: direct" + +title "Opt in via config: the same failure stops the automatic migration" +trace $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +title "State is still terraform, so nothing was migrated\n" +trace find .databricks/bundle -name "resources.json" -type f +trace find .databricks/bundle -name "terraform.tfstate*" -type f diff --git a/acceptance/bundle/migrate/reference-direct-only-resource/app/app.py b/acceptance/bundle/migrate/reference-direct-only-resource/app/app.py new file mode 100644 index 00000000000..48cdce85287 --- /dev/null +++ b/acceptance/bundle/migrate/reference-direct-only-resource/app/app.py @@ -0,0 +1 @@ +placeholder diff --git a/acceptance/bundle/migrate/reference-direct-only-resource/databricks.yml b/acceptance/bundle/migrate/reference-direct-only-resource/databricks.yml new file mode 100644 index 00000000000..95eae95f5f8 --- /dev/null +++ b/acceptance/bundle/migrate/reference-direct-only-resource/databricks.yml @@ -0,0 +1,13 @@ +bundle: + name: test-bundle + +resources: + #catalogs: {my_catalog: {name: my-catalog}} + apps: + my_app: + name: my-app + source_code_path: ./app + config: + env: + - name: CATALOG + value: plain diff --git a/acceptance/bundle/migrate/reference-direct-only-resource/out.test.toml b/acceptance/bundle/migrate/reference-direct-only-resource/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/reference-direct-only-resource/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/reference-direct-only-resource/output.txt b/acceptance/bundle/migrate/reference-direct-only-resource/output.txt new file mode 100644 index 00000000000..7f874544ffb --- /dev/null +++ b/acceptance/bundle/migrate/reference-direct-only-resource/output.txt @@ -0,0 +1,22 @@ + +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created apps.my_app +Files: 5 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +>>> update_file.py databricks.yml #catalogs catalogs + +>>> update_file.py databricks.yml value: plain value: ${resources.catalogs.my_catalog.name} + +=== Direct requested: the catalog is skipped by terraform and the migration still succeeds +>>> DATABRICKS_BUNDLE_ENGINE=direct [CLI] bundle deploy +Warn: Direct engine selected via DATABRICKS_BUNDLE_ENGINE environment variable but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged +Migrating state to direct deployment engine (selected via DATABRICKS_BUNDLE_ENGINE environment variable)... +Migrated 1 resource to direct deployment engine. + +>>> print_migration_telemetry +direct_migrated_via_env true diff --git a/acceptance/bundle/migrate/reference-direct-only-resource/script b/acceptance/bundle/migrate/reference-direct-only-resource/script new file mode 100644 index 00000000000..211129dfdff --- /dev/null +++ b/acceptance/bundle/migrate/reference-direct-only-resource/script @@ -0,0 +1,18 @@ +export DATABRICKS_BUNDLE_ENGINE= + +# Recording that a reference into a direct-only resource type does not break the +# migration. Terraform cannot deploy catalogs at all, so the reference resolves +# to nothing in terraform.tfstate — but apps.config is inline app.yaml rather +# than an API field, so ExtractReferences drops the reference before it is ever +# resolved. See bundle/direct/bundle_plan.go: refs are kept only for fields that +# exist in the state type. +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +rm -f out.requests.txt + +trace update_file.py databricks.yml "#catalogs" "catalogs" +trace update_file.py databricks.yml "value: plain" 'value: ${resources.catalogs.my_catalog.name}' + +title "Direct requested: the catalog is skipped by terraform and the migration still succeeds" +trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt diff --git a/acceptance/bundle/migrate/reference-methods-disagree/databricks.yml b/acceptance/bundle/migrate/reference-methods-disagree/databricks.yml new file mode 100644 index 00000000000..8a8c2860c58 --- /dev/null +++ b/acceptance/bundle/migrate/reference-methods-disagree/databricks.yml @@ -0,0 +1,15 @@ +bundle: + name: test-bundle + +# Enabled by the script before the second deploy, so the first one leaves a state +# the two resolution methods agree on. +#experimental: {scripts: {postdeploy: "python3 drift_stored_name.py"}} + +resources: + jobs: + src: + name: source + dst: + # Method A reads dst's own stored name; Method B evaluates this template + # against src's. The post-deploy script makes those two differ. + name: ${resources.jobs.src.name} diff --git a/acceptance/bundle/migrate/reference-methods-disagree/drift_stored_name.py b/acceptance/bundle/migrate/reference-methods-disagree/drift_stored_name.py new file mode 100644 index 00000000000..2c484a59baf --- /dev/null +++ b/acceptance/bundle/migrate/reference-methods-disagree/drift_stored_name.py @@ -0,0 +1,19 @@ +"""Make the two resolution methods disagree about dst's name. + +Runs as a post-deploy script, so the deploy applies against a state it wrote and +only the migration sees the drift. Editing the stored value is the point: a real +deploy stores the same string on both sides of a name-to-name reference, so the +disagreement the conversion warns about cannot be produced by config alone. +""" + +import json +import pathlib + +p = pathlib.Path(".databricks/bundle/default/terraform/terraform.tfstate") +state = json.loads(p.read_text()) + +for resource in state["resources"]: + if resource.get("type") == "databricks_job" and resource.get("name") == "dst": + resource["instances"][0]["attributes"]["name"] = "source-drifted" + +p.write_text(json.dumps(state, indent=2)) diff --git a/acceptance/bundle/migrate/reference-methods-disagree/out.test.toml b/acceptance/bundle/migrate/reference-methods-disagree/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/reference-methods-disagree/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/reference-methods-disagree/output.txt b/acceptance/bundle/migrate/reference-methods-disagree/output.txt new file mode 100644 index 00000000000..bc1251d663d --- /dev/null +++ b/acceptance/bundle/migrate/reference-methods-disagree/output.txt @@ -0,0 +1,38 @@ + +=== Deploy on terraform: the methods agree, so the dry run is clean +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created jobs.dst +Created jobs.src +Files: 5 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +>>> print_migration_telemetry +direct_drymigrate_success true +direct_drymigrate_warnings false + +>>> update_file.py databricks.yml #experimental experimental + +=== Opt in: the methods disagree, which stops the migration without an error +>>> DATABRICKS_BUNDLE_ENGINE=direct [CLI] bundle deploy +Warn: Direct engine selected via DATABRICKS_BUNDLE_ENGINE environment variable but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged +Executing 'postdeploy' script +Warn: post-deploy dry-run migration to direct: resource jobs.dst field name: method A value "source-drifted" and method B value "source" disagree; using longer (method A) +Warn: The warnings above are from a dry-run migration to the direct deployment engine (https://docs.databricks.com/aws/en/dev-tools/bundles/direct). +Your deployment is not affected and works normally, but you may experience these issues when migrating to the direct deployment engine. +Please forward these warnings to dabs-feedback@databricks.com +Warn: Direct engine was selected but the migration reported issues; automatic migration to the direct deployment engine is stopped. Address the issues above or run "databricks bundle deployment migrate" manually. + +>>> print_migration_telemetry +direct_migrate_warnings true +direct_migrate_warning_safe_error jobs.%s field %q: method A and method B disagree + +=== State is still terraform, so nothing was migrated + +>>> find .databricks/bundle -name resources.json -type f + +>>> find .databricks/bundle -name terraform.tfstate* -type f +.databricks/bundle/default/terraform/terraform.tfstate diff --git a/acceptance/bundle/migrate/reference-methods-disagree/script b/acceptance/bundle/migrate/reference-methods-disagree/script new file mode 100644 index 00000000000..b32586928c7 --- /dev/null +++ b/acceptance/bundle/migrate/reference-methods-disagree/script @@ -0,0 +1,17 @@ +export DATABRICKS_BUNDLE_ENGINE= + +title "Deploy on terraform: the methods agree, so the dry run is clean" +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +trace update_file.py databricks.yml "#experimental" "experimental" + +title "Opt in: the methods disagree, which stops the migration without an error" +trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +title "State is still terraform, so nothing was migrated\n" +trace find .databricks/bundle -name "resources.json" -type f +trace find .databricks/bundle -name "terraform.tfstate*" -type f diff --git a/acceptance/bundle/migrate/reference-terraform-syntax/databricks.yml b/acceptance/bundle/migrate/reference-terraform-syntax/databricks.yml new file mode 100644 index 00000000000..c4db417e0ab --- /dev/null +++ b/acceptance/bundle/migrate/reference-terraform-syntax/databricks.yml @@ -0,0 +1,13 @@ +bundle: + name: test-bundle + +resources: + jobs: + src: + name: source + dst: + name: dst + # Terraform's own reference syntax, which users copy from Terraform docs or + # carry over from a raw Terraform project. DABs does not resolve it, so it + # reaches Terraform verbatim and Terraform resolves it. + description: ${databricks_job.src.id} diff --git a/acceptance/bundle/migrate/reference-terraform-syntax/out.test.toml b/acceptance/bundle/migrate/reference-terraform-syntax/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/reference-terraform-syntax/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/reference-terraform-syntax/output.txt b/acceptance/bundle/migrate/reference-terraform-syntax/output.txt new file mode 100644 index 00000000000..1fde5409f8e --- /dev/null +++ b/acceptance/bundle/migrate/reference-terraform-syntax/output.txt @@ -0,0 +1,26 @@ + +=== Deploy on terraform +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created jobs.dst +Created jobs.src +Files: 4 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +=== Opt in: the migration reports success +>>> DATABRICKS_BUNDLE_ENGINE=direct [CLI] bundle deploy +Warn: Direct engine selected via DATABRICKS_BUNDLE_ENGINE environment variable but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Files: 2 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged +Migrating state to direct deployment engine (selected via DATABRICKS_BUNDLE_ENGINE environment variable)... +Migrated 2 resources to direct deployment engine. + +>>> print_migration_telemetry +direct_migrated_via_env true + +=== Badness: the migrated state cannot be planned + +>>> musterr [CLI] bundle plan +Error: invalid dependency "${databricks_job.src.id}", no such node "" + diff --git a/acceptance/bundle/migrate/reference-terraform-syntax/script b/acceptance/bundle/migrate/reference-terraform-syntax/script new file mode 100644 index 00000000000..d68e7b760ff --- /dev/null +++ b/acceptance/bundle/migrate/reference-terraform-syntax/script @@ -0,0 +1,21 @@ +export DATABRICKS_BUNDLE_ENGINE= + +# Terraform's own reference syntax, which users copy from Terraform docs or carry +# over from a raw Terraform project. Terraform resolves it, so the deploy and the +# migration both succeed. +title "Deploy on terraform" +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +rm -f out.requests.txt + +title "Opt in: the migration reports success" +trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +# Badness: the migration reports success but writes an unusable state. +# BuildStateFromTF derives depends_on from each reference via GetNodeAndType, +# which returns an empty node for a path that does not start with "resources", +# and the empty node lands in resources.json. Nothing warns until the next plan. +title "Badness: the migrated state cannot be planned\n" +trace musterr $CLI bundle plan 2>&1 | contains.py 'invalid dependency' 'no such node' +rm -f out.requests.txt diff --git a/acceptance/bundle/migrate/script.prepare b/acceptance/bundle/migrate/script.prepare index 9de2f19d437..930fed8860d 100644 --- a/acceptance/bundle/migrate/script.prepare +++ b/acceptance/bundle/migrate/script.prepare @@ -1,7 +1,12 @@ -# 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 description that says which failure a direct_migrate_error or +# direct_drymigrate_success=false was. +# +# Call it right after the deploy whose telemetry you want, so a block covers +# exactly one deploy. It does not clear out.requests.txt: callers do that +# themselves, since some want it kept for a subsequent print_requests.py. print_migration_telemetry() { print_telemetry_bool_values | grep '^direct_' || true + print_telemetry_safe_errors | grep '^direct_' || true } diff --git a/acceptance/bundle/migrate/tfstate-version-unsupported/bump_state_version.py b/acceptance/bundle/migrate/tfstate-version-unsupported/bump_state_version.py new file mode 100644 index 00000000000..b2c87b87899 --- /dev/null +++ b/acceptance/bundle/migrate/tfstate-version-unsupported/bump_state_version.py @@ -0,0 +1,11 @@ +"""Bump the recorded terraform state format version. + +Runs as a post-deploy script. The deploy runs it after applying and before the +migration, so the deploy succeeds against the state it wrote and only the +migration sees a format it does not understand. +""" + +import pathlib + +p = pathlib.Path(".databricks/bundle/default/terraform/terraform.tfstate") +p.write_text(p.read_text().replace('"version": 4', '"version": 5')) diff --git a/acceptance/bundle/migrate/tfstate-version-unsupported/databricks.yml b/acceptance/bundle/migrate/tfstate-version-unsupported/databricks.yml new file mode 100644 index 00000000000..90da5e69f94 --- /dev/null +++ b/acceptance/bundle/migrate/tfstate-version-unsupported/databricks.yml @@ -0,0 +1,11 @@ +bundle: + name: test-bundle + +# Enabled by the script before the second deploy: with it enabled for the first +# one too, that deploy would leave a state terraform itself cannot read. +#experimental: {scripts: {postdeploy: "python3 bump_state_version.py"}} + +resources: + jobs: + my_job: + name: my-job diff --git a/acceptance/bundle/migrate/tfstate-version-unsupported/out.test.toml b/acceptance/bundle/migrate/tfstate-version-unsupported/out.test.toml new file mode 100644 index 00000000000..27ec2a7fcd6 --- /dev/null +++ b/acceptance/bundle/migrate/tfstate-version-unsupported/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [""] diff --git a/acceptance/bundle/migrate/tfstate-version-unsupported/output.txt b/acceptance/bundle/migrate/tfstate-version-unsupported/output.txt new file mode 100644 index 00000000000..40876f16a3a --- /dev/null +++ b/acceptance/bundle/migrate/tfstate-version-unsupported/output.txt @@ -0,0 +1,34 @@ + +=== Deploy on terraform, leaving a state the migration can read +>>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Created jobs.my_job +Files: 5 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +>>> print_migration_telemetry +direct_drymigrate_success true +direct_drymigrate_warnings false + +>>> update_file.py databricks.yml #experimental experimental + +=== Opt in: the deploy succeeds, the migration cannot read the state +>>> DATABRICKS_BUNDLE_ENGINE=direct [CLI] bundle deploy +Warn: Direct engine selected via DATABRICKS_BUNDLE_ENGINE environment variable but the existing state uses "terraform". Deploying on "terraform"; will attempt to migrate the state to the direct engine after this deploy. +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Files: 3 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 1 unchanged +Executing 'postdeploy' script +Warn: post-deploy dry-run migration to direct: failed to parse terraform state: unsupported deployment state version: 5. Try re-deploying the bundle +Warn: Direct engine was selected but the migration reported issues; automatic migration to the direct deployment engine is stopped. Address the issues above or run "databricks bundle deployment migrate" manually. + +>>> print_migration_telemetry +direct_migrate_error true +direct_migrate_safe_error unsupported deployment state version: 5. Try re-deploying the bundle + +=== State is still terraform, so nothing was migrated + +>>> find .databricks/bundle -name resources.json -type f + +>>> find .databricks/bundle -name terraform.tfstate* -type f +.databricks/bundle/default/terraform/terraform.tfstate diff --git a/acceptance/bundle/migrate/tfstate-version-unsupported/script b/acceptance/bundle/migrate/tfstate-version-unsupported/script new file mode 100644 index 00000000000..a45d94ef242 --- /dev/null +++ b/acceptance/bundle/migrate/tfstate-version-unsupported/script @@ -0,0 +1,17 @@ +export DATABRICKS_BUNDLE_ENGINE= + +title "Deploy on terraform, leaving a state the migration can read" +trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +trace update_file.py databricks.yml "#experimental" "experimental" + +title "Opt in: the deploy succeeds, the migration cannot read the state" +trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle deploy +trace print_migration_telemetry +rm -f out.requests.txt + +title "State is still terraform, so nothing was migrated\n" +trace find .databricks/bundle -name "resources.json" -type f +trace find .databricks/bundle -name "terraform.tfstate*" -type f diff --git a/acceptance/script.prepare b/acceptance/script.prepare index d71d37aae9b..883c0c64639 100644 --- a/acceptance/script.prepare +++ b/acceptance/script.prepare @@ -123,6 +123,12 @@ 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 the PII-free error descriptions the deploy event carries, one +# " " line each, skipping the ones this deploy did not set. +print_telemetry_safe_errors() { + jq -r 'select(.path? == "/telemetry-ext") | (.body.protoLogs // [])[] | fromjson | ( (.entry // .) | (.databricks_cli_log.bundle_deploy_event.experimental // {}) ) | to_entries | map(select((.key | endswith("_safe_error")) and (.value // "") != "")) | 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 1eb9ce15654..626c2bcd48e 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -80,6 +80,13 @@ type Metrics struct { ExecutionTimes []protos.IntMapEntry LocalCacheMeasurementsMs []protos.IntMapEntry // Local cache measurements stored as milliseconds + // PII-free descriptions of the errors this deploy hit, reported without + // scrubbing. Each is a libs/safeerr message template; see the matching fields + // on protos.BundleDeployExperimental for what each one covers. + DirectMigrateSafeErr string + DirectMigrateWarningSafeErr string + DirectMigrateCommitSafeErr string + // StateEngine is the engine that ran the deploy, set in deployCore. Empty when // telemetry is emitted without a deploy having run. StateEngine engine.EngineType diff --git a/bundle/config/mutator/resourcemutator/secret_scope_fixups.go b/bundle/config/mutator/resourcemutator/secret_scope_fixups.go index e85584da3f0..54d298cd0c1 100644 --- a/bundle/config/mutator/resourcemutator/secret_scope_fixups.go +++ b/bundle/config/mutator/resourcemutator/secret_scope_fixups.go @@ -2,7 +2,7 @@ package resourcemutator import ( "context" - "fmt" + "maps" "slices" "strings" @@ -12,6 +12,7 @@ import ( "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/iamutil" + "github.com/databricks/cli/libs/safeerr" "github.com/databricks/databricks-sdk-go/service/iam" ) @@ -63,7 +64,7 @@ func collapsePermissions(scope *resources.SecretScope) error { for _, perm := range scope.Permissions { // Validate permission level if _, ok := permissionRank[perm.Level]; !ok { - return fmt.Errorf("unknown permission level %q for secret scope", perm.Level) + return safeerr.Errorf("unknown permission level %q for secret scope", perm.Level) } // Add a prefix to retain the original principal type. In practice collisions here should @@ -76,7 +77,7 @@ func collapsePermissions(scope *resources.SecretScope) error { } else if perm.ServicePrincipalName != "" { principal = "sp:" + perm.ServicePrincipalName } else { - return fmt.Errorf("missing principal in permissions for secret scope %q", scope.Name) + return safeerr.Errorf("missing principal in permissions for secret scope %q", scope.Name) } existing, exists := principalPermissions[principal] @@ -120,19 +121,28 @@ func collapsePermissions(scope *resources.SecretScope) error { return nil } -func (m *secretScopeFixups) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +// ApplySecretScopeFixups is the mutator's body, exported so a caller that wants +// the failure as an error rather than a diagnostic can have it. The migration to +// the direct engine is one: it reports the error's template to telemetry, which a +// diagnostic's summary cannot carry. The returned key names the offending scope. +func ApplySecretScopeFixups(b *bundle.Bundle, eng engine.EngineType) (string, error) { // Secret scopes by default have the current user as a MANAGE ACL. We need to add it to the client ACL list // to prevent a phantom persistent diff. // We do not need to do this in terraform because terraform naively always applies the config during ACL // creation without checking if the ACL already exists. // https://github.com/databricks/terraform-provider-databricks/blob/5cb5d3fa46bc4843be1a4c4bce89296eaa2e14fc/secrets/resource_secret_acl.go#L43 - if !m.engine.IsDirect() { - return nil + if !eng.IsDirect() { + return "", nil } // Secret scopes assigns the create MANAGE ACL on it by default. So we always add it to // the client ACL list as a default. - for key, scope := range b.Config.Resources.SecretScopes { + // + // Sorted because the key travels out to a diagnostic and to migration + // telemetry: with two invalid scopes, map order would decide which one is + // reported and the output would vary between runs. + for _, key := range slices.Sorted(maps.Keys(b.Config.Resources.SecretScopes)) { + scope := b.Config.Resources.SecretScopes[key] if scope == nil { continue } @@ -140,19 +150,27 @@ func (m *secretScopeFixups) Apply(ctx context.Context, b *bundle.Bundle) diag.Di currentUser := b.Config.Workspace.CurrentUser.User addManageForCurrentUser(scope, currentUser) - err := collapsePermissions(scope) - if err != nil { - return diag.Diagnostics{ - { - Severity: diag.Error, - Summary: "Failed to collapse permissions for secret scope", - Detail: err.Error(), - Paths: []dyn.Path{dyn.MustPathFromString("resources.secret_scopes." + key)}, - Locations: []dyn.Location{b.Config.GetLocation("resources.secret_scopes." + key)}, - }, - } + if err := collapsePermissions(scope); err != nil { + return key, err } } - return nil + return "", nil +} + +func (m *secretScopeFixups) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + key, err := ApplySecretScopeFixups(b, m.engine) + if err == nil { + return nil + } + + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "Failed to collapse permissions for secret scope", + Detail: err.Error(), + Paths: []dyn.Path{dyn.MustPathFromString("resources.secret_scopes." + key)}, + Locations: []dyn.Location{b.Config.GetLocation("resources.secret_scopes." + key)}, + }, + } } diff --git a/bundle/config/mutator/resourcemutator/secret_scope_fixups_test.go b/bundle/config/mutator/resourcemutator/secret_scope_fixups_test.go index 24cd7aa06a1..892c5a145be 100644 --- a/bundle/config/mutator/resourcemutator/secret_scope_fixups_test.go +++ b/bundle/config/mutator/resourcemutator/secret_scope_fixups_test.go @@ -3,9 +3,13 @@ package resourcemutator import ( "testing" + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/engine" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/databricks-sdk-go/service/iam" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestCollapsePermissions(t *testing.T) { @@ -252,3 +256,22 @@ func TestAddManageForCurrentUser(t *testing.T) { }) } } + +// TestApplySecretScopeFixupsReportsLowestKey pins which scope is reported when +// more than one is invalid: map order would otherwise decide, and the key +// reaches both a diagnostic and migration telemetry. +func TestApplySecretScopeFixupsReportsLowestKey(t *testing.T) { + b := &bundle.Bundle{Config: config.Root{Resources: config.Resources{ + SecretScopes: map[string]*resources.SecretScope{ + "zebra": {Name: "zebra", Permissions: []resources.SecretScopePermission{{Level: "BOGUS", UserName: "u"}}}, + "apple": {Name: "apple", Permissions: []resources.SecretScopePermission{{Level: "BOGUS", UserName: "u"}}}, + }, + }}} + b.Config.Workspace.CurrentUser = &config.User{User: &iam.User{UserName: "u"}} + + // The keys are sorted, so this is deterministic. Dropping the sort would make + // it fail about half the time, which is enough to notice. + key, err := ApplySecretScopeFixups(b, engine.EngineDirect) + require.Error(t, err) + assert.Equal(t, "apple", key) +} diff --git a/bundle/config/resource_key.go b/bundle/config/resource_key.go new file mode 100644 index 00000000000..bc181ab69ec --- /dev/null +++ b/bundle/config/resource_key.go @@ -0,0 +1,48 @@ +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: 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. + // The "resources." prefix is dropped: every key carries it, so it is noise. + group, kind, hasKind := strings.Cut(resourceType, ".") + + // GetResourceTypeFromKey reads the group straight out of the key without + // checking it, so a key of an unexpected shape would put its second segment + // in a telemetry field. Report only a group this package defines. + if _, ok := SupportedResources()[group]; !ok { + return "*" + } + + if hasKind { + return group + ".*." + kind + } + return group + ".*" +} diff --git a/bundle/config/resource_key_test.go b/bundle/config/resource_key_test.go new file mode 100644 index 00000000000..4697a69cde3 --- /dev/null +++ b/bundle/config/resource_key_test.go @@ -0,0 +1,95 @@ +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: "jobs.*", + }, + { + key: "resources.pipelines.my_pipeline", + want: "pipelines.*", + }, + { + key: "resources.jobs.my_job.permissions", + want: "jobs.*.permissions", + }, + { + key: "resources.schemas.my_schema.grants", + want: "schemas.*.grants", + }, + { + key: "resources.secret_scopes.my scope.permissions", + want: "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: "*", + }, + + // A key whose second segment is not a resource type this package defines + // must not put that segment in a telemetry field. + { + key: "resources.ALICE_EXAMPLE_COM.job", + want: "*", + }, + { + key: "resources.someone@example.com.job", + 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, "jobs.*: SaveState: disk full", safeerr.SafeError(err)) + assert.NotContains(t, safeerr.SafeError(err), "my_job") +} diff --git a/bundle/deploy/terraform/util.go b/bundle/deploy/terraform/util.go index 3e078e8ae73..90d451d7e7b 100644 --- a/bundle/deploy/terraform/util.go +++ b/bundle/deploy/terraform/util.go @@ -11,6 +11,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/statemgmt/resourcestate" "github.com/databricks/cli/libs/log" + "github.com/databricks/cli/libs/safeerr" tfjson "github.com/hashicorp/terraform-json" ) @@ -73,7 +74,7 @@ func parseResourcesState(ctx context.Context, path string) (ExportedResourcesMap func resourcesStateToMap(ctx context.Context, state *resourcesState) (ExportedResourcesMap, error) { if state.Version != SupportedStateVersion { - return nil, fmt.Errorf("unsupported deployment state version: %d. Try re-deploying the bundle", state.Version) + return nil, safeerr.Errorf("unsupported deployment state version: %d. Try re-deploying the bundle", safeerr.Safe(state.Version)) } result := make(ExportedResourcesMap) diff --git a/bundle/migrate/build_state.go b/bundle/migrate/build_state.go index e8b382b370d..2c5a2097a23 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" @@ -31,8 +31,12 @@ func BuildStateFromTF( tfAttrs TFStateAttrs, tfIDs map[string]string, warnPrefix string, -) (bool, error) { +) (bool, string, error) { warningsSeen := false + // PII-free description of the first warning, for telemetry. A warning stops + // an automatic migration just as an error does, but carries no error to + // describe it, so it is built here from the parts that are safe to report. + warnSafeErr := "" // Collect all resource nodes (same patterns as makePlan). var nodes []string patterns := []dyn.Pattern{ @@ -50,11 +54,15 @@ func BuildStateFromTF( }, ) if err != nil { - return warningsSeen, err + return warningsSeen, warnSafeErr, err } } for _, node := range nodes { + // Errors below report the key rather than the bare string, so their + // templates name the resource type without the user's resource name. + key := config.ResourceKey(node) + id, ok := tfIDs[node] if !ok { // Resource is in config but not in TF state (new resource); skip. @@ -64,34 +72,35 @@ func BuildStateFromTF( group := config.GetResourceTypeFromKey(node) if group == "" { - return warningsSeen, fmt.Errorf("cannot determine resource type for %q", node) + return warningsSeen, warnSafeErr, safeerr.Errorf("cannot determine resource type for %q", key) } adapter, ok := adapters[group] if !ok { warningsSeen = true log.Warnf(ctx, warnPrefix+"unsupported resource type %q for %s, skipping", group, node) + setWarnSafeErr(&warnSafeErr, safeerr.Errorf("unsupported resource type %q for %s, skipping", safeerr.Safe(group), key)) continue } inputConfig, err := configRoot.GetResourceConfig(node) if err != nil { - return warningsSeen, fmt.Errorf("%s: getting config: %w", node, err) + return warningsSeen, warnSafeErr, safeerr.Errorf("%s: getting config: %w", key, err) } inputSV, err := adapter.PrepareInputConfig(inputConfig, node) if err != nil { - return warningsSeen, fmt.Errorf("%s: PrepareInputConfig: %w", node, err) + return warningsSeen, warnSafeErr, safeerr.Errorf("%s: PrepareInputConfig: %w", key, err) } newStateValue, err := adapter.PrepareState(inputSV.Value) if err != nil { - return warningsSeen, fmt.Errorf("%s: PrepareState: %w", node, err) + return warningsSeen, warnSafeErr, safeerr.Errorf("%s: PrepareState: %w", key, 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, warnSafeErr, safeerr.Errorf("%s: extracting references: %w", key, err) } maps.Copy(refs, inputSV.Refs) @@ -142,7 +151,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, warnSafeErr, safeerr.Errorf("%s: setting object_id: %w", key, err) } delete(sv.Refs, "object_id") } @@ -169,28 +178,33 @@ 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, warnSafeErr, safeerr.Errorf("%s: parsing field path %q: %w", key, 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, warnSafeErr, safeerr.Errorf("%s: cannot resolve field %q (template %q): %w", key, pending.fieldPathStr, pending.refTemplate, err) } if warned { warningsSeen = true + // The disagreeing values are user data; the resource type and the + // stage are not. + setWarnSafeErr(&warnSafeErr, safeerr.Errorf( + "%s.%s field %q: method A and method B disagree", + safeerr.Safe(srcGroup), srcName, pending.fieldPathStr)) } // 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, warnSafeErr, safeerr.Errorf("%s: cannot set resolved value for field %q: %w", key, 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, warnSafeErr, safeerr.Errorf("%s: unresolved references: %v", key, sv.Refs) } // Handle etag for dashboards: read it directly from TF state attributes. @@ -200,15 +214,24 @@ 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, warnSafeErr, safeerr.Errorf("%s: cannot set etag: %w", key, err) } } } if err := stateDB.SaveState(node, id, sv.Value, dependsOn); err != nil { - return warningsSeen, fmt.Errorf("%s: SaveState: %w", node, err) + return warningsSeen, warnSafeErr, safeerr.Errorf("%s: SaveState: %w", key, err) } } - return warningsSeen, nil + return warningsSeen, warnSafeErr, nil +} + +// setWarnSafeErr records err's message template in target unless one is already +// there: the first warning is the one reported, matching how the first error +// diagnostic is the one a deploy reports. +func setWarnSafeErr(target *string, err error) { + if *target == "" { + *target = safeerr.SafeError(err) + } } diff --git a/bundle/migrate/build_state_test.go b/bundle/migrate/build_state_test.go index a495436008c..211ed00efc5 100644 --- a/bundle/migrate/build_state_test.go +++ b/bundle/migrate/build_state_test.go @@ -15,6 +15,7 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/convert" "github.com/databricks/cli/libs/dyn/yamlloader" + "github.com/databricks/cli/libs/safeerr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -50,7 +51,7 @@ func runBuildStateFromTF( db.OpenWithData(statePath, dstate.NewDatabase("lineage", 1)) require.NoError(t, db.UpgradeToWrite()) - _, err = migrate.BuildStateFromTF(t.Context(), &root, adapters, &db, tfAttrs, tfIDs, "") + _, _, err = migrate.BuildStateFromTF(t.Context(), &root, adapters, &db, tfAttrs, tfIDs, "") require.NoError(t, err) _, err = db.Finalize(t.Context()) @@ -303,3 +304,159 @@ resources: }) } } + +// openEmptyWriteState opens an empty write-mode deployment state under a fresh +// temp dir and registers cleanup that closes the WAL file. The error and warning +// paths below never reach the Finalize that the success helper relies on to close +// it, and Windows cannot remove the temp dir while the .wal handle is still open. +func openEmptyWriteState(t *testing.T) *dstate.DeploymentState { + t.Helper() + + db := &dstate.DeploymentState{} + db.OpenWithData(filepath.Join(t.TempDir(), "resources.json"), dstate.NewDatabase("lineage", 1)) + require.NoError(t, db.UpgradeToWrite()) + t.Cleanup(func() { _, _ = db.Finalize(t.Context()) }) + return db +} + +// buildStateErrFromTF is runBuildStateFromTF's failure counterpart: it returns +// the error instead of requiring success. +func buildStateErrFromTF( + t *testing.T, + yaml string, + tfAttrs migrate.TFStateAttrs, + tfIDs map[string]string, +) error { + t.Helper() + + root := rootFromYAML(t, yaml) + adapters, err := dresources.InitAll(nil) + require.NoError(t, err) + + db := openEmptyWriteState(t) + + _, _, err = migrate.BuildStateFromTF(t.Context(), &root, adapters, db, tfAttrs, tfIDs, "") + return err +} + +// TestBuildStateFromTFErrors covers the ways a conversion fails, and pins the +// message against the template reported to telemetry: the message names the +// resource and field, the template names neither but still says which failure +// it was. Together these are what a migration failure population can be broken +// down by. +func TestBuildStateFromTFErrors(t *testing.T) { + tests := []struct { + name string + + yaml string + tfAttrs migrate.TFStateAttrs + tfIDs map[string]string + + // wantErrContains is a substring of the user-facing message; secrets + // below must appear there and must not appear in the template. + wantErrContains string + wantTemplate string + }{ + { + name: "referenced resource missing from TF state", + yaml: ` +resources: + pipelines: + src_secret: + name: "source" + jobs: + dst_secret: + name: "${resources.pipelines.src_secret.name}" +`, + tfAttrs: migrate.TFStateAttrs{ + "databricks_job": {"dst_secret": json.RawMessage(`{"id": "j1"}`)}, + }, + tfIDs: map[string]string{"resources.jobs.dst_secret": "j1"}, + wantErrContains: "databricks_pipeline.src_secret not found in TF state", + wantTemplate: `jobs.*: cannot resolve field %q (template %q): jobs.%s field %s: method A: %q: key not found; method B: cannot look up %q: databricks_pipeline.%s not found in TF state`, + }, + { + name: "referenced field absent from TF attributes", + yaml: ` +resources: + pipelines: + src_secret: + name: "source" + jobs: + dst_secret: + name: "${resources.pipelines.src_secret.name}" +`, + tfAttrs: migrate.TFStateAttrs{ + "databricks_pipeline": {"src_secret": json.RawMessage(`{"id": "p1"}`)}, + "databricks_job": {"dst_secret": json.RawMessage(`{"id": "j1"}`)}, + }, + tfIDs: map[string]string{ + "resources.pipelines.src_secret": "p1", + "resources.jobs.dst_secret": "j1", + }, + wantErrContains: "key not found", + wantTemplate: `jobs.*: cannot resolve field %q (template %q): jobs.%s field %s: method A: %q: key not found; method B: cannot look up %q: %q: key not found`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := buildStateErrFromTF(t, tc.yaml, tc.tfAttrs, tc.tfIDs) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrContains) + assert.Equal(t, tc.wantTemplate, safeerr.SafeError(err)) + + // The names the message carries must not reach the template. + for _, secret := range []string{"src_secret", "dst_secret"} { + assert.Contains(t, err.Error(), secret, "message should name the resource") + assert.NotContains(t, safeerr.SafeError(err), secret, "template must not") + } + }) + } +} + +// TestBuildStateFromTFMethodsDisagree covers the one blocking outcome that is +// not an error: both resolution methods succeed but return different values, so +// the conversion warns and keeps going. MigrateToDirect stops on a warning just +// as it does on an error, but there is no error to describe — nothing records +// which field disagreed. +func TestBuildStateFromTFMethodsDisagree(t *testing.T) { + yaml := ` +resources: + jobs: + src: + name: source + dst: + name: dst + description: ${resources.jobs.src.name} +` + // Method A reads dst's own description; Method B reads src's name. They + // disagree here, which is what the backend normalizing a value on write + // looks like to the conversion. + tfAttrs := migrate.TFStateAttrs{ + "databricks_job": { + "src": json.RawMessage(`{"id": "1", "name": "source"}`), + "dst": json.RawMessage(`{"id": "2", "name": "dst", "description": "stale-value"}`), + }, + } + tfIDs := map[string]string{"resources.jobs.src": "1", "resources.jobs.dst": "2"} + + root := rootFromYAML(t, yaml) + adapters, err := dresources.InitAll(nil) + require.NoError(t, err) + + db := openEmptyWriteState(t) + + warnings, warnSafeErr, err := migrate.BuildStateFromTF(t.Context(), &root, adapters, db, tfAttrs, tfIDs, "") + + // No error: the conversion completed. But it warned, which is enough to stop + // an automatic migration, so the warning carries its own PII-free template. + require.NoError(t, err) + assert.True(t, warnings, "disagreeing methods must be reported as a warning") + assert.Equal(t, `jobs.%s field %q: method A and method B disagree`, warnSafeErr) + + // Neither the resource name nor the disagreeing values reach the template. + for _, secret := range []string{"dst", "stale-value", "source"} { + assert.NotContains(t, warnSafeErr, secret) + } +} 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/mutator.go b/bundle/mutator.go index 90fdba28c9b..a0abe3a3582 100644 --- a/bundle/mutator.go +++ b/bundle/mutator.go @@ -122,6 +122,30 @@ func ApplyFuncContext(ctx context.Context, b *Bundle, fn func(context.Context, * ApplyContext(ctx, b, funcMutator{fn}) } +type errFuncMutator struct { + fn func(context.Context, *Bundle) error + err error +} + +func (m *errFuncMutator) Name() string { + return "" +} + +func (m *errFuncMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { + m.err = m.fn(ctx, b) + return nil +} + +// ApplyFuncErr applies an inline-specified function mutator and returns its error +// rather than turning it into a diagnostic. The mutator machinery still runs, so +// typed config changes are synced back to the dynamic tree. A failure to sync is +// logged as a diagnostic, not returned. +func ApplyFuncErr(ctx context.Context, b *Bundle, fn func(context.Context, *Bundle) error) error { + m := &errFuncMutator{fn: fn} + ApplyContext(ctx, b, m) + return m.err +} + // Test helpers. TODO: move to separate package. func Apply(ctx context.Context, b *Bundle, m Mutator) diag.Diagnostics { diff --git a/bundle/phases/telemetry.go b/bundle/phases/telemetry.go index cbe74f467fe..b32d399908f 100644 --- a/bundle/phases/telemetry.go +++ b/bundle/phases/telemetry.go @@ -305,6 +305,10 @@ func LogDeployTelemetry(ctx context.Context, b *bundle.Bundle, errMsg string) { ComplexVariableCount: complexVariableCount, LookupVariableCount: lookupVariableCount, BundleMutatorExecutionTimeMs: getExecutionTimes(b), + + DirectMigrateSafeErr: b.Metrics.DirectMigrateSafeErr, + DirectMigrateCommitSafeErr: b.Metrics.DirectMigrateCommitSafeErr, + DirectMigrateWarningSafeErr: b.Metrics.DirectMigrateWarningSafeErr, }, }, }) diff --git a/bundle/statemgmt/direct_migration.go b/bundle/statemgmt/direct_migration.go index 7b88290930c..d3d751731f2 100644 --- a/bundle/statemgmt/direct_migration.go +++ b/bundle/statemgmt/direct_migration.go @@ -20,10 +20,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 @@ -56,6 +58,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) + recordSafeErr(&b.Metrics.DirectMigrateSafeErr, err) if requestedEngine.Type == engine.EngineDirect { b.Metrics.SetBoolValue(metrics.DirectMigrateError, true) log.Warnf(ctx, "%s", autoMigrateStoppedNotice) @@ -85,6 +88,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 (selected via "+requestedEngine.Source+")...") if err := backupTerraformState(ctx, b); err != nil { b.Metrics.SetBoolValue(metrics.DirectMigrateCommitError, true) + recordSafeErr(&b.Metrics.DirectMigrateCommitSafeErr, err) log.Warnf(ctx, "automatic migration to direct engine failed: %v", err) return } @@ -107,6 +111,7 @@ func MigrateToDirect(ctx context.Context, b *bundle.Bundle, requestedEngine engi if err != nil { log.Warnf(ctx, "%s%v", warnPrefix, err) + recordSafeErr(&b.Metrics.DirectMigrateSafeErr, err) } if hasWarnings || err != nil { log.Warnf(ctx, "%s", feedbackNotice) @@ -146,6 +151,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) + recordSafeErr(&b.Metrics.DirectMigrateCommitSafeErr, err) log.Warnf(ctx, "automatic migration to direct engine failed: %v", err) return } @@ -180,6 +186,17 @@ func checkPlanOnTempState(ctx context.Context, b *bundle.Bundle, tempStatePath s return err } +// recordSafeErr stores a PII-free description of err in target, which is +// the metric for the same failure class as the boolean recorded alongside it — +// so a query tells a conversion failure from a commit failure by field, without +// joining against the booleans. Recorded for both populations, the opt-in one +// and the dry run, since the booleans only say that a migration failed. +func recordSafeErr(target *string, err error) { + if safe := diag.SafeError(err); safe != "" { + *target = safe + } +} + // recordDryRunNoop records dry-run telemetry for a no-op case (no state, or // state with no managed resources) when direct was NOT selected. On the // migrating paths the caller uses direct_migrate_* keys instead. @@ -225,20 +242,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 } @@ -297,12 +314,19 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr var stateDB dstate.DeploymentState stateDB.OpenWithData(tempStatePath, migratedDB) - // Apply SecretScopeFixups so the config matches what the direct engine expects. - // This adds MANAGE ACL for the current user to all secret scopes, ensuring - // 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, nil, errors.New("failed to apply secret scope fixups") + // Apply the secret scope fixups so the config matches what the direct engine + // expects. This adds MANAGE ACL for the current user to all secret scopes, + // ensuring the migrated state and config agree on .permissions entries. + // + // ApplyFuncErr rather than a mutator, so the failure stays an error: a + // diagnostic's summary reaches telemetry as a generic "failed to apply secret + // scope fixups". The mutator machinery is still needed, because the fixups + // mutate typed config and reverseInterpolate below reads the dynamic tree. + if err := bundle.ApplyFuncErr(ctx, b, func(_ context.Context, b *bundle.Bundle) error { + _, err := resourcemutator.ApplySecretScopeFixups(b, engine.EngineDirect) + return err + }); err != nil { + return tempStatePath, resourceCount, false, nil, err } // b.Config has been modified by terraform.Interpolate which converts bundle-style @@ -310,7 +334,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, nil, fmt.Errorf("failed to reverse interpolation: %w", err) + return tempStatePath, resourceCount, false, nil, safeerr.Errorf("failed to reverse interpolation: %w", err) } var uninterpolatedConfig config.Root @@ -318,7 +342,7 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr return uninterpolatedRoot, nil }) if err != nil { - return tempStatePath, resourceCount, false, nil, fmt.Errorf("failed to create uninterpolated config: %w", err) + return tempStatePath, resourceCount, false, nil, safeerr.Errorf("failed to create uninterpolated config: %w", err) } adapters, err := dresources.InitAll(nil) @@ -327,11 +351,14 @@ func convertTFStateToDirect(ctx context.Context, b *bundle.Bundle, tfState *migr } if err := stateDB.UpgradeToWrite(); err != nil { - return tempStatePath, resourceCount, false, nil, fmt.Errorf("upgrading state for apply: %w", err) + return tempStatePath, resourceCount, false, nil, safeerr.Errorf("upgrading state for apply: %w", err) } // warnPrefix labels the conversion's warnings as coming from the background dry run. - hasWarnings, err := migrate.BuildStateFromTF(ctx, &uninterpolatedConfig, adapters, &stateDB, tfState.Attrs, tfState.IDs, warnPrefix) + hasWarnings, warnSafeErr, err := migrate.BuildStateFromTF(ctx, &uninterpolatedConfig, adapters, &stateDB, tfState.Attrs, tfState.IDs, warnPrefix) + // Recorded even when the conversion goes on to fail: a warning is enough to + // stop an automatic migration on its own, and nothing else describes it. + b.Metrics.DirectMigrateWarningSafeErr = warnSafeErr if err != nil { return tempStatePath, resourceCount, hasWarnings, nil, err } @@ -342,7 +369,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, nil, errors.New("state conversion failed") + return tempStatePath, resourceCount, hasWarnings, nil, safeerr.New("state conversion failed") } return tempStatePath, resourceCount, hasWarnings, &uninterpolatedConfig, nil @@ -361,13 +388,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 @@ -377,13 +404,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" @@ -429,16 +456,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/bundle/statemgmt/upload_state_for_yaml_sync.go b/bundle/statemgmt/upload_state_for_yaml_sync.go index 80d4a3dbd12..c8541c9ab2e 100644 --- a/bundle/statemgmt/upload_state_for_yaml_sync.go +++ b/bundle/statemgmt/upload_state_for_yaml_sync.go @@ -172,7 +172,7 @@ func (m *uploadStateForYamlSync) convertState(ctx context.Context, b *bundle.Bun return false, fmt.Errorf("upgrading state for apply: %w", err) } - if _, err := migrate.BuildStateFromTF(ctx, &uninterpolatedConfig, adapters, &stateDB, tfState.Attrs, tfState.IDs, ""); err != nil { + if _, _, err := migrate.BuildStateFromTF(ctx, &uninterpolatedConfig, adapters, &stateDB, tfState.Attrs, tfState.IDs, ""); err != nil { return false, err } diff --git a/cmd/bundle/deployment/migrate.go b/cmd/bundle/deployment/migrate.go index 93e229f71e8..288eadf23e8 100644 --- a/cmd/bundle/deployment/migrate.go +++ b/cmd/bundle/deployment/migrate.go @@ -172,7 +172,7 @@ To start using direct engine, set "engine: direct" under bundle in your databric return fmt.Errorf("upgrading state for apply: %w", err) } - if _, err := migrate.BuildStateFromTF(ctx, &b.Config, adapters, &stateDB, tfState.Attrs, tfState.IDs, ""); err != nil { + if _, _, err := migrate.BuildStateFromTF(ctx, &b.Config, adapters, &stateDB, tfState.Attrs, tfState.IDs, ""); err != nil { return err } diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index fe7090462d4..a2bb92f964f 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 { @@ -54,6 +55,25 @@ func FromErr(err error) Diagnostics { } } +// SafeError returns a PII-free description of err for telemetry: the safe +// message of a safeerr error, or a typed error's own stand-in, plus the safe +// fields of any API error at the end of its chain. The 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 SafeError(err error) string { + safe := safeerr.SafeError(err) + apiDescription := SafeAPIErrorDescription(err) + + switch { + case apiDescription == "": + return safe + case safe == "": + return apiDescription + default: + return safe + " [" + 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/safe_error_test.go b/libs/diag/safe_error_test.go new file mode 100644 index 00000000000..72b71bd75ab --- /dev/null +++ b/libs/diag/safe_error_test.go @@ -0,0 +1,127 @@ +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: "403 PERMISSION_DENIED", + }, + { + 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: "429 QUOTA_EXCEEDED", + }, + + // 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)) + }) + } +} + +// TestFromErrSafeErrorCarriesNoUserData is the property the field exists +// for: whatever the summary holds, the template holds none of it. + +func TestFromErrNil(t *testing.T) { + assert.Nil(t, FromErr(nil)) +} + +// TestSafeErrorMatchesFromErr keeps the exported helper and the field in +// step, since callers holding an error use one and callers holding a diagnostic +// use the other. + +// standInErr models a typed CLI error such as libs/filer's: user data in the +// message, a PII-free classification as its stand-in. +type standInErr struct{} + +func (standInErr) Error() string { return "access denied: /Workspace/Users/a@b.com/x" } +func (standInErr) SafeString() string { return "access denied" } + +// TestSafeErrorUsesStandInWithoutSafeerr covers the call sites not raised +// through safeerr, which is most of them: a typed error still describes itself. +func TestSafeErrorUsesStandInWithoutSafeerr(t *testing.T) { + assert.Equal(t, "access denied", SafeError(standInErr{})) + + // Combined with an API error at the end of the chain. + err := safeerr.Errorf("pushing state: %w", standInErr{}) + assert.Equal(t, "pushing state: access denied", SafeError(err)) + + // A stand-in never displaces a real template. + assert.Equal(t, "cannot update %s: %w", + SafeError(safeerr.Errorf("cannot update %s: %w", "resources.jobs.x", errors.New("boom")))) +} diff --git a/libs/diag/sdk_error.go b/libs/diag/sdk_error.go index d190498d6bf..e7e3585d5d5 100644 --- a/libs/diag/sdk_error.go +++ b/libs/diag/sdk_error.go @@ -3,23 +3,24 @@ package diag import ( "errors" "fmt" + "regexp" "strconv" "strings" "github.com/databricks/databricks-sdk-go/apierr" ) -func FormatAPIErrorSummary(e error) string { - apiErr, ok := errors.AsType[*apierr.APIError](e) +func FormatAPIErrorSummary(err error) string { + apiErr, ok := errors.AsType[*apierr.APIError](err) if !ok { - return e.Error() + return err.Error() } extra := strings.TrimSpace(fmt.Sprintf("%d %s", apiErr.StatusCode, apiErr.ErrorCode)) - return e.Error() + " (" + extra + ")" + return err.Error() + " (" + extra + ")" } -func FormatAPIErrorDetails(e error) string { - apiErr, ok := errors.AsType[*apierr.APIError](e) +func FormatAPIErrorDetails(err error) string { + apiErr, ok := errors.AsType[*apierr.APIError](err) if !ok { return "" } @@ -42,3 +43,41 @@ 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 err 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(err error) string { + apiErr, ok := errors.AsType[*apierr.APIError](err) + if !ok { + return "" + } + + // Status first, then code, matching FormatAPIErrorSummary so the template and + // the user-facing message order the same two values the same way. + var parts []string + if apiErr.StatusCode != 0 { + parts = append(parts, strconv.Itoa(apiErr.StatusCode)) + } + if safeErrorCode.MatchString(apiErr.ErrorCode) { + parts = append(parts, apiErr.ErrorCode) + } + if len(parts) == 0 { + return "" + } + return strings.Join(parts, " ") +} diff --git a/libs/filer/errors.go b/libs/filer/errors.go index 8648502c229..7cd62c1b083 100644 --- a/libs/filer/errors.go +++ b/libs/filer/errors.go @@ -2,6 +2,25 @@ package filer import "io/fs" +// Each error below is a fixed classification followed by the path it concerns. +// The classification is a source literal and safe to report to telemetry; the +// path is user data. Naming the literals here lets Error() and SafeString() +// derive from one string, so the message and what telemetry sees cannot drift. +// +// SafeString implements safeerr.SafeStringer: an error wrapped with %w by a +// safeerr error contributes its classification to that error's message +// template. See libs/safeerr. +const ( + msgFileAlreadyExists = "file already exists" + msgFileDoesNotExist = "file does not exist" + msgNoSuchDirectory = "no such directory" + msgNotADirectory = "not a directory" + msgNotAFile = "not a file" + msgDirectoryNotEmpty = "directory not empty" + msgCannotDeleteRoot = "unable to delete filer root" + msgPermissionDenied = "access denied" +) + // fileAlreadyExistsError is returned when attempting to write a file at a path // that already exists, without using the OverwriteIfExists WriteMode flag. type fileAlreadyExistsError struct { @@ -9,7 +28,11 @@ type fileAlreadyExistsError struct { } func (err fileAlreadyExistsError) Error() string { - return "file already exists: " + err.path + return msgFileAlreadyExists + ": " + err.path +} + +func (err fileAlreadyExistsError) SafeString() string { + return msgFileAlreadyExists } func (err fileAlreadyExistsError) Is(other error) bool { @@ -28,7 +51,11 @@ func (err fileDoesNotExistError) Is(other error) bool { } func (err fileDoesNotExistError) Error() string { - return "file does not exist: " + err.path + return msgFileDoesNotExist + ": " + err.path +} + +func (err fileDoesNotExistError) SafeString() string { + return msgFileDoesNotExist } // noSuchDirectoryError is returned when attempting to write a file to a path @@ -39,7 +66,11 @@ type noSuchDirectoryError struct { } func (err noSuchDirectoryError) Error() string { - return "no such directory: " + err.path + return msgNoSuchDirectory + ": " + err.path +} + +func (err noSuchDirectoryError) SafeString() string { + return msgNoSuchDirectory } func (err noSuchDirectoryError) Is(other error) bool { @@ -53,7 +84,11 @@ type notADirectory struct { } func (err notADirectory) Error() string { - return "not a directory: " + err.path + return msgNotADirectory + ": " + err.path +} + +func (err notADirectory) SafeString() string { + return msgNotADirectory } func (err notADirectory) Is(other error) bool { @@ -67,7 +102,11 @@ type notAFile struct { } func (err notAFile) Error() string { - return "not a file: " + err.path + return msgNotAFile + ": " + err.path +} + +func (err notAFile) SafeString() string { + return msgNotAFile } func (err notAFile) Is(other error) bool { @@ -82,7 +121,11 @@ type directoryNotEmptyError struct { } func (err directoryNotEmptyError) Error() string { - return "directory not empty: " + err.path + return msgDirectoryNotEmpty + ": " + err.path +} + +func (err directoryNotEmptyError) SafeString() string { + return msgDirectoryNotEmpty } func (err directoryNotEmptyError) Is(other error) bool { @@ -95,7 +138,12 @@ func (err directoryNotEmptyError) Is(other error) bool { type cannotDeleteRootError struct{} func (err cannotDeleteRootError) Error() string { - return "unable to delete filer root" + return msgCannotDeleteRoot +} + +// SafeString is the whole message: this error carries no path. +func (err cannotDeleteRootError) SafeString() string { + return msgCannotDeleteRoot } func (err cannotDeleteRootError) Is(other error) bool { @@ -113,7 +161,11 @@ type permissionError struct { } func (err permissionError) Error() string { - return "access denied: " + err.path + return msgPermissionDenied + ": " + err.path +} + +func (err permissionError) SafeString() string { + return msgPermissionDenied } func (err permissionError) Is(other error) bool { diff --git a/libs/filer/errors_test.go b/libs/filer/errors_test.go index e12fe946450..9f0c207045d 100644 --- a/libs/filer/errors_test.go +++ b/libs/filer/errors_test.go @@ -2,8 +2,10 @@ package filer import ( "io/fs" + "strings" "testing" + "github.com/databricks/cli/libs/safeerr" "github.com/databricks/databricks-sdk-go/apierr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -109,3 +111,72 @@ func TestPermissionError_Unwrap(t *testing.T) { require.ErrorAs(t, err, &got) assert.Equal(t, "MAX_CHILD_NODE_SIZE_EXCEEDED", got.ErrorCode) } + +// TestErrorSafeStringOmitsPath is the property that lets these errors be +// reported to telemetry: the classification survives, the path does not. +func TestErrorSafeStringOmitsPath(t *testing.T) { + const path = "/Workspace/Users/someone@example.com/secret_project" + + tests := []struct { + err error + wantSafeString string + }{ + { + err: fileAlreadyExistsError{path: path}, + wantSafeString: "file already exists", + }, + { + err: fileDoesNotExistError{path: path}, + wantSafeString: "file does not exist", + }, + { + err: noSuchDirectoryError{path: path}, + wantSafeString: "no such directory", + }, + { + err: notADirectory{path: path}, + wantSafeString: "not a directory", + }, + { + err: notAFile{path: path}, + wantSafeString: "not a file", + }, + { + err: directoryNotEmptyError{path: path}, + wantSafeString: "directory not empty", + }, + { + err: permissionError{path: path}, + wantSafeString: "access denied", + }, + { + err: cannotDeleteRootError{}, + wantSafeString: "unable to delete filer root", + }, + } + + for _, tt := range tests { + t.Run(tt.wantSafeString, func(t *testing.T) { + safe, ok := tt.err.(interface{ SafeString() string }) + require.True(t, ok, "%T must supply a stand-in", tt.err) + + assert.Equal(t, tt.wantSafeString, safe.SafeString()) + assert.NotContains(t, safe.SafeString(), path) + + // The message still leads with the same classification. + assert.True(t, strings.HasPrefix(tt.err.Error(), tt.wantSafeString), + "%q should start with %q", tt.err.Error(), tt.wantSafeString) + }) + } +} + +// TestErrorSafeStringReachesTemplate covers the end-to-end path: a filer error +// wrapped by safeerr contributes its classification to the template. +func TestErrorSafeStringReachesTemplate(t *testing.T) { + const path = "/Workspace/Users/someone@example.com/x" + err := safeerr.Errorf("pushing direct state to workspace: %w", permissionError{path: path}) + + assert.Equal(t, "pushing direct state to workspace: access denied: "+path, err.Error()) + assert.Equal(t, "pushing direct state to workspace: access denied", safeerr.SafeError(err)) + assert.NotContains(t, safeerr.SafeError(err), path) +} diff --git a/libs/safeerr/safeerr.go b/libs/safeerr/safeerr.go new file mode 100644 index 00000000000..d74e010328f --- /dev/null +++ b/libs/safeerr/safeerr.go @@ -0,0 +1,297 @@ +// Package safeerr provides errors that maintain two string representations of the error message: +// - the usual, accessible via Error() +// - "safe", accessible via SafeError() +// +// The format string is considered safe. The arguments to format strings are unsafe by default +// unless wrapped with safeerr.Safe() or implement SafeStringer() interface. +package safeerr + +import ( + "errors" + "fmt" + "strings" +) + +const maxSafeErrorSize = 1000 + +// safeValue marks one argument of Errorf as free of user data. +type safeValue struct{ v any } + +type safeErr struct { + // err is the fmt.Errorf result + err error + + // safe error counterpart + safeErr string +} + +// Errorf formats an error exactly like fmt.Errorf and in addition maintains safe error message. +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 &safeErr{ + err: fmt.Errorf(format, unpackArgs(args)...), + safeErr: SafeSprintf(format, args...), + } +} + +// New returns an error whose message is entirely safe literal +func New(text string) error { + return &safeErr{err: errors.New(text), safeErr: truncateSafe(text)} +} + +// 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 +// "jobs.*" — enough to tell a failing job from a failing pipeline without +// reporting which one. +// +// An error may also be a SafeStringer: under %w its own template is preferred, +// and the stand-in is used only when it has none. SafeStringer otherwise wins +// over Safe, so wrapping such a value in Safe cannot put its user-supplied part +// back into the template. +type SafeStringer interface { + SafeString() string +} + +// SafeError returns the redacted error message: only format strings and safe values are visible. +func SafeError(err error) string { + if se, ok := errors.AsType[*safeErr](err); ok { + return se.safeErr + } + + // Not raised through this package, but a typed error can still describe + // itself — libs/filer's errors do. That covers the call sites not converted. + if ss, ok := err.(SafeStringer); ok { + return ss.SafeString() + } + + return "" +} + +func (e *safeErr) 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 *safeErr) Unwrap() error { + return e.err +} + +// unpackArgs 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 unpackArgs(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 +} + +// SafeSprintf renders format with only its safe arguments substituted. Every +// other verb is escaped so it appears literally in the result — "%s: getting +// config" stays "%s: getting config" — which is what makes the result reportable +// without scrubbing: the format string is a source literal, and nothing else +// reaches the output. +func SafeSprintf(format string, args ...any) string { + safeFormat, safeArgs, ok := escapeUnsafeVerbs(format, args) + if !ok { + // The format uses a construct the scanner does not model, so which + // argument each verb consumes is no longer known and substituting any of + // them could pair a safe value with the wrong verb. The format is a + // source literal, so reporting it unrendered is still safe. + return truncateSafe(format + " (safeerr failed)") + } + + // fmt.Errorf rather than fmt.Sprintf so %w stays valid: a nested safe error + // is passed through as a proxy whose Error() is its own safe message. + return truncateSafe(fmt.Errorf(safeFormat, safeArgs...).Error()) +} + +// escapeUnsafeVerbs rewrites format so that only verbs with a safe argument +// still consume one, and returns those arguments in order. It reports false for +// a construct parseVerb does not model. +func escapeUnsafeVerbs(format string, args []any) (string, []any, bool) { + var sb strings.Builder + var safeArgs []any + argIndex := 0 + + for i := 0; i < len(format); { + if format[i] != '%' { + sb.WriteByte(format[i]) + i++ + continue + } + + spec, verb, next, ok := parseVerb(format, i) + if !ok { + return "", nil, false + } + i = next + + // %% consumes no argument, so it passes through untouched. + if verb == '%' { + sb.WriteString(spec) + continue + } + + // More verbs than arguments is a malformed call, which go vet reports at + // the call site; here the surplus verb simply has nothing to render. + var value any + safeToRender := false + if argIndex < len(args) { + value, safeToRender = safeValueFor(args[argIndex]) + } + argIndex++ + + if !safeToRender { + // Escaping the percent leaves the verb as literal text and consumes + // no argument, which is what keeps the rest of the format in step. + sb.WriteString("%" + spec) + continue + } + + sb.WriteString(spec) + safeArgs = append(safeArgs, value) + } + + return sb.String(), safeArgs, true +} + +// safeErrProxy carries a safe message under a verb expecting an error, so %w and +// %s render that rather than the wrapped error's full text. +type safeErrProxy struct{ msg string } + +func (p safeErrProxy) Error() string { return p.msg } + +// standIn returns ss's stand-in in a form the verb can consume: one that is +// itself an error stays an error, so %w renders the stand-in rather than +// %!w(string=...). +func standIn(ss SafeStringer) any { + if _, ok := ss.(error); ok { + return safeErrProxy{msg: ss.SafeString()} + } + return ss.SafeString() +} + +// safeValueFor returns what to render for arg, and whether rendering it is safe +// at all. Everything else is dropped, so a value the caller did not vouch for +// cannot reach the output. +func safeValueFor(arg any) (any, bool) { + if v, ok := arg.(safeValue); ok { + // A value that declares a stand-in keeps it even under Safe: 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 { + return standIn(ss), true + } + return v.v, true + } + + // A nested safe error contributes its own safe message. Only the error itself + // is consulted, not its chain: an inner safe message is not the wrapper's own + // text, and walking the chain would not terminate when Unwrap loops back. + if se, ok := arg.(*safeErr); ok { + return safeErrProxy{msg: se.safeErr}, true + } + + // Failing that, a typed error can still describe itself, which is how + // libs/filer reports a classification without the path it carries. + if ss, ok := arg.(SafeStringer); ok { + return standIn(ss), true + } + + return nil, false +} + +// truncateSafe caps a safe message so a deep chain cannot produce an unbounded +// telemetry field. +// +// The cap counts bytes, so it can land inside a rune; ToValidUTF8 drops the +// partial one rather than shipping invalid UTF-8. +func truncateSafe(s string) string { + if len(s) > maxSafeErrorSize { + return strings.ToValidUTF8(s[:maxSafeErrorSize], "") + } + return s +} + +// 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] == '*' { + return 0, false + } + for j < len(format) && format[j] >= '0' && format[j] <= '9' { + j++ + } + // An explicit argument index may also follow a literal width or precision + // (%2[2]s, %.2[2]s), so reject '[' here and not only ahead of the digits. + if j < len(format) && format[j] == '[' { + return 0, false + } + return j, true +} diff --git a/libs/safeerr/safeerr_test.go b/libs/safeerr/safeerr_test.go new file mode 100644 index 00000000000..7f35a772343 --- /dev/null +++ b/libs/safeerr/safeerr_test.go @@ -0,0 +1,622 @@ +package safeerr + +import ( + "errors" + "fmt" + "io/fs" + "strings" + "testing" + "unicode/utf8" + + "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" + +// nilError is a nil error interface, for the row that wraps nothing. +var nilError error + +// unsafeMarkers are the user-data-shaped strings the fixtures use. A safe message +// must never contain one, whichever row produced it. +var unsafeMarkers = []string{unsafeValue, "/Workspace", "a@b.com", "my_job"} + +func TestErrorf(t *testing.T) { + tests := []struct { + name string + format string + args []any + wantMessage string + wantSafeMessage string + }{ + { + name: "no verbs", + format: "state conversion failed", + wantMessage: "state conversion failed", + wantSafeMessage: "state conversion failed", + }, + { + name: "unsafe string", + format: "%s: getting config", + args: []any{unsafeValue}, + wantMessage: unsafeValue + ": getting config", + wantSafeMessage: "%s: getting config", + }, + { + name: "unsafe int", + format: "unsupported deployment state version: %d", + args: []any{7}, + wantMessage: "unsupported deployment state version: 7", + wantSafeMessage: "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"`, + wantSafeMessage: `%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", + wantSafeMessage: "cannot convert string to int64", + }, + { + // SafeError is a message rather than a format string, so %% renders as + // a literal percent just as it does in the real message. + name: "escaped percent renders as a percent", + format: "100%% of %s", + args: []any{unsafeValue}, + wantMessage: "100% of " + unsafeValue, + wantSafeMessage: "100% of %s", + }, + { + name: "flags and width on an unsafe verb", + format: "%-12s|", + args: []any{"job"}, + wantMessage: "job |", + wantSafeMessage: "%-12s|", + }, + { + name: "flags and width on a safe verb", + format: "%-12s|", + args: []any{Safe("job")}, + wantMessage: "job |", + wantSafeMessage: "job |", + }, + { + name: "width and precision on a safe verb", + format: "%08.3f", + args: []any{Safe(3.5)}, + wantMessage: "0003.500", + wantSafeMessage: "0003.500", + }, + { + name: "plus v on an unsafe verb", + format: "reading state: %+v", + args: []any{struct{ Path string }{unsafeValue}}, + wantMessage: "reading state: {Path:" + unsafeValue + "}", + wantSafeMessage: "reading state: %+v", + }, + { + name: "safe bool", + format: "recovery enabled: %v", + args: []any{Safe(true)}, + wantMessage: "recovery enabled: true", + wantSafeMessage: "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", + wantSafeMessage: "jobs: %s for PrepareState in %s", + }, + { + name: "wrapping a safe error contributes its safe message", + format: "%s: SaveState: %w", + args: []any{unsafeValue, Errorf("cannot convert %s to %s", Safe("string"), Safe("int64"))}, + wantMessage: unsafeValue + ": SaveState: cannot convert string to int64", + wantSafeMessage: "%s: SaveState: cannot convert string to int64", + }, + { + name: "wrapping a foreign error keeps the verb", + format: "reading %s: %w", + args: []any{"/home/user/state.json", fs.ErrNotExist}, + wantMessage: "reading /home/user/state.json: " + fs.ErrNotExist.Error(), + wantSafeMessage: "reading %s: %w", + }, + { + name: "several wrapped errors", + format: "%s: %w and %w", + args: []any{unsafeValue, Errorf("group %s has no adapter", Safe("quality_monitors")), fs.ErrPermission}, + wantMessage: unsafeValue + ": group quality_monitors has no adapter and " + fs.ErrPermission.Error(), + wantSafeMessage: "%s: group quality_monitors has no adapter and %w", + }, + + // Malformed calls. go vet reports these at a real call site; the point here + // is that the message still matches fmt's and the safe half stays sane. + { + name: "too few arguments", + format: "%s and %s", + args: []any{Safe("one")}, + wantMessage: "one and %!s(MISSING)", + wantSafeMessage: "one and %s", + }, + { + // The extra value is dropped rather than reported, since only verbs + // with an argument survive into the safe format. + name: "too many arguments", + format: "%s", + args: []any{Safe("one"), Safe("two")}, + wantMessage: "one%!(EXTRA string=two)", + wantSafeMessage: "one", + }, + { + // A safe value renders through the wrong verb, marker and all. An + // unsafe one would have had its verb escaped instead. + name: "wrong verb for type", + format: "%d", + args: []any{Safe("not a number")}, + wantMessage: "%!d(string=not a number)", + wantSafeMessage: "%!d(string=not a number)", + }, + { + name: "safe nil", + format: "%v", + args: []any{Safe(nil)}, + wantMessage: "", + wantSafeMessage: "", + }, + { + name: "safe struct with plus v", + format: "%+v", + args: []any{Safe(struct{ A int }{1})}, + wantMessage: "{A:1}", + wantSafeMessage: "{A:1}", + }, + { + // Marked Safe, so the error's own message is what is reported. + name: "safe error under an ordinary verb", + format: "%s", + args: []any{Safe(fs.ErrNotExist)}, + wantMessage: fs.ErrNotExist.Error(), + wantSafeMessage: fs.ErrNotExist.Error(), + }, + { + name: "wrapping nothing keeps the verb", + format: "wrapping: %w", + args: []any{nilError}, + wantMessage: "wrapping: %!w()", + wantSafeMessage: "wrapping: %w", + }, + + // A SafeStringer contributes its stand-in, whatever the verb. + { + name: "stand-in under s verb", + format: "cannot update %s: %w", + args: []any{safeStringerKey("resources.jobs.my_job"), fs.ErrPermission}, + wantMessage: "cannot update resources.jobs.my_job: " + fs.ErrPermission.Error(), + wantSafeMessage: "cannot update jobs.*: %w", + }, + { + name: "stand-in quoted by q verb like the value", + format: "%q not found", + args: []any{safeStringerKey("resources.jobs.my_job")}, + wantMessage: `"resources.jobs.my_job" not found`, + wantSafeMessage: `"jobs.*" not found`, + }, + { + name: "stand-in alongside safe and unsafe args", + format: "cannot %s %s: field %q: %s", + args: []any{Safe("update"), safeStringerKey("resources.jobs.my_job"), Safe("tasks[0].job_id"), unsafeValue}, + wantMessage: "cannot update resources.jobs.my_job: field \"tasks[0].job_id\": " + unsafeValue, + wantSafeMessage: `cannot update jobs.*: field "tasks[0].job_id": %s`, + }, + { + // Safe cannot put a stand-in value's user-supplied part back in. + name: "stand-in outranks Safe", + format: "%s", + args: []any{Safe(safeStringerKey("resources.jobs.my_job"))}, + wantMessage: "resources.jobs.my_job", + wantSafeMessage: "jobs.*", + }, + { + // Safe must not cost a stand-in error its error-ness, or %w would have + // a string to render and report %!w(string=...). + name: "Safe keeps a stand-in error usable by w verb", + format: "pushing state: %w", + args: []any{Safe(standInOnlyErr{})}, + wantMessage: "pushing state: " + standInOnlyErr{}.Error(), + wantSafeMessage: "pushing state: access denied", + }, + { + // An empty safe message is still one, so its verb is rendered, not + // escaped. + name: "wrapped empty safe message", + format: "outer: %w", + args: []any{New("")}, + wantMessage: "outer: ", + wantSafeMessage: "outer: ", + }, + { + // An error with a safe message of its own prefers that to a stand-in. + name: "wrapped safe error beats a stand-in", + format: "outer: %w", + args: []any{Errorf("inner %d", Safe(1))}, + wantMessage: "outer: inner 1", + wantSafeMessage: "outer: inner 1", + }, + { + // A typed error with no safe message of its own falls back to its + // stand-in, which is how libs/filer reports a classification. + name: "typed error stand-in under w verb", + format: "writing state: %w", + args: []any{standInOnlyErr{}}, + wantMessage: "writing state: access denied: /Workspace/Users/a@b.com/x", + wantSafeMessage: "writing state: access denied", + }, + { + name: "typed error stand-in under s verb", + format: "writing state: %s", + args: []any{standInOnlyErr{}}, + wantMessage: "writing state: access denied: /Workspace/Users/a@b.com/x", + wantSafeMessage: "writing state: access denied", + }, + } + + 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.wantSafeMessage, SafeError(err)) + }) + } + + // The same table drives SafeSprintf, which is what Errorf stores and is worth + // exercising on its own rather than only through an error. + for _, tt := range tests { + t.Run("SafeSprintf/"+tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantSafeMessage, SafeSprintf(tt.format, tt.args...)) + }) + } + + // And the message half: Errorf must render exactly what fmt.Errorf would from + // the same values, which is what makes converting a call site invisible. + for _, tt := range tests { + t.Run("MatchesFmt/"+tt.name, func(t *testing.T) { + //nolint:govet // tt.format is a table value, so vet cannot check the verbs + assert.Equal(t, tt.wantMessage, fmt.Errorf(tt.format, unpackArgs(tt.args)...).Error()) + }) + } + + // The security property of the package, over every row rather than a + // hand-picked list: whatever the message carries, the safe message does not + // carry the value that was not marked safe. + for _, tt := range tests { + t.Run("NoLeak/"+tt.name, func(t *testing.T) { + err := Errorf(tt.format, tt.args...) + safe := SafeError(err) + for _, marker := range unsafeMarkers { + if strings.Contains(err.Error(), marker) { + assert.NotContains(t, safe, marker, "marker %q reached the safe message", marker) + } + } + }) + } +} + +func TestSafeErrorChainsEveryLevel(t *testing.T) { + // The table covers what the outermost error renders; this is the part it + // cannot express, that every level still reports its own safe message. + inner := Errorf("cannot convert %s to %s", Safe("string"), Safe("int64")) + middle := Errorf("%s: cannot set field %q: %w", unsafeValue, Safe("tasks[0].job_id"), inner) + outer := Errorf("%s: SaveState: %w", unsafeValue, middle) + + assert.Equal(t, "cannot convert string to int64", SafeError(inner)) + assert.Equal(t, `%s: cannot set field "tasks[0].job_id": cannot convert string to int64`, SafeError(middle)) + assert.Equal(t, `%s: SaveState: %s: cannot set field "tasks[0].job_id": cannot convert string to int64`, SafeError(outer)) +} + +func TestSafeErrorChainsThroughPlainWrap(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"`, SafeError(err)) +} + +func TestSafeErrorWithoutSafeErr(t *testing.T) { + assert.Empty(t, SafeError(nil)) + assert.Empty(t, SafeError(errors.New(unsafeValue))) + assert.Empty(t, SafeError(fmt.Errorf("reading %s", unsafeValue))) + assert.Empty(t, SafeError(fs.ErrNotExist)) +} + +func TestNew(t *testing.T) { + // New takes a literal message, so it is its own safe message and any verbs in + // it are neither expanded nor lost. + tests := []struct { + name string + text string + }{ + { + name: "plain", + text: "state conversion failed", + }, + { + name: "contains verbs", + text: "100% of %s attempts failed", + }, + { + name: "contains an escaped percent", + text: "100%% done", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := New(tt.text) + assert.Equal(t, tt.text, err.Error()) + assert.Equal(t, tt.text, SafeError(err)) + }) + } +} + +func TestSafeErrorFallsBackToRawFormat(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. The marker says the + // scan bailed rather than that the format happened to render this way. + assert.Equal(t, tt.format+" (safeerr failed)", SafeError(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) + + // Several %w in one call: each branch stays reachable. + first := Errorf("group %s has no adapter", Safe("quality_monitors")) + both := Errorf("%s: %w and %w", unsafeValue, first, fs.ErrPermission) + assert.ErrorIs(t, both, first) + assert.ErrorIs(t, both, fs.ErrPermission) +} + +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) +} + +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: "%2[2]s", + wantOk: false, + }, + { + format: "%.2[2]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) + }) + } +} + +func TestSafeErrorDeepChain(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", SafeError(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 "jobs.*" } + +// standInOnlyErr is a typed error carrying user data in its message and a +// PII-free classification as its stand-in, modelled on libs/filer's errors. +type standInOnlyErr struct{} + +func (standInOnlyErr) Error() string { return "access denied: /Workspace/Users/a@b.com/x" } +func (standInOnlyErr) SafeString() string { return "access denied" } + +// cyclicErr unwraps to whatever it is pointed at, which a test uses to close a +// loop back to an ancestor. +type cyclicErr struct{ inner error } + +func (*cyclicErr) Error() string { return "cyclic" } +func (c *cyclicErr) Unwrap() error { return c.inner } +func (*cyclicErr) SafeString() string { return "cyclic" } + +func TestSafeErrorCyclicChainTerminates(t *testing.T) { + // An error whose Unwrap reaches back to the templated error that wraps it. + // Following it would recurse forever, so the traversal is bounded. + c := &cyclicErr{} + err := Errorf("outer: %w", c) + c.inner = err + + // The assertion is that this returns at all rather than exhausting the stack. + assert.NotEmpty(t, SafeError(err)) +} + +func TestSafeErrorDeepChainTerminates(t *testing.T) { + // Each layer's safe message is rendered at construction from the layer below, + // so a deep chain costs nothing at read time and cannot recurse. + err := New("root") + for range 100 { + err = Errorf("%w", err) + } + assert.NotPanics(t, func() { SafeError(err) }) + assert.Equal(t, "root", SafeError(err)) +} + +func TestSafeErrorCapKeepsValidUTF8(t *testing.T) { + // The cap counts bytes, so it can land inside a rune. The field still has to + // be valid UTF-8. + err := New(strings.Repeat("a", maxSafeErrorSize-1) + "é") + + safe := SafeError(err) + assert.True(t, utf8.ValidString(safe), "safe message must be valid UTF-8") + assert.Len(t, safe, maxSafeErrorSize-1, "the split rune is dropped, not half-kept") +} + +func TestSafeErrorCyclicChainInArgumentTerminates(t *testing.T) { + // The cycle is closed before Errorf is called, so walking the argument's chain + // would not terminate. fmt.Errorf copes with such an error; so must Errorf. + c := &cyclicErr{} + c.inner = c + + err := Errorf("outer: %w", c) + + assert.Equal(t, "outer: cyclic", err.Error()) + assert.Equal(t, "outer: cyclic", SafeError(err)) +} + +func TestSafeErrorIsCapped(t *testing.T) { + // A safe message is a telemetry field, so it cannot grow without bound even + // though every part of it comes from source literals. + err := New(strings.Repeat("x", maxSafeErrorSize*2)) + assert.Len(t, SafeError(err), maxSafeErrorSize) + + err = Errorf("%s", Safe(strings.Repeat("y", maxSafeErrorSize*2))) + assert.Len(t, SafeError(err), maxSafeErrorSize) +} diff --git a/libs/telemetry/protos/bundle_deploy.go b/libs/telemetry/protos/bundle_deploy.go index 69e01a30715..7d6dd67c00a 100644 --- a/libs/telemetry/protos/bundle_deploy.go +++ b/libs/telemetry/protos/bundle_deploy.go @@ -113,6 +113,34 @@ 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 why a post-deploy migration to the direct engine + // failed. Each 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 its chain. + // + // These are the aggregatable counterpart to BundleDeployEvent.ErrorMessage, + // which is scrubbed heuristically and still treated as privileged. They are + // composed of source literals and closed enums only, so they need no + // scrubbing and can be grouped on directly. + + // DirectMigrateSafeErr describes a post-deploy migration to the direct + // engine whose state could not be read or converted. Set alongside + // direct_migrate_error on opt-in deploys and direct_drymigrate_success on the + // dry run. + DirectMigrateSafeErr string `json:"direct_migrate_safe_error,omitempty"` + + // DirectMigrateCommitSafeErr describes a migration whose state + // converted cleanly but could not be committed. Set alongside + // direct_migrate_commit_error. + DirectMigrateCommitSafeErr string `json:"direct_migrate_commit_safe_error,omitempty"` + + // DirectMigrateWarningSafeErr describes the first warning a conversion + // emitted. A warning stops an automatic migration just as an error does, but + // carries no error to describe it. Set alongside direct_migrate_warnings, or + // direct_drymigrate_warnings on the dry run. + DirectMigrateWarningSafeErr string `json:"direct_migrate_warning_safe_error,omitempty"` } // BundleResourcesMetadata mirrors the universe proto. Per-resource-type counts diff --git a/libs/telemetry/protos/bundle_deploy_test.go b/libs/telemetry/protos/bundle_deploy_test.go new file mode 100644 index 00000000000..900e48679d4 --- /dev/null +++ b/libs/telemetry/protos/bundle_deploy_test.go @@ -0,0 +1,38 @@ +package protos + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestBundleDeployExperimentalSafeErrFields pins the wire names of the PII-free +// error descriptions. They are the only fields here that a dashboard groups by, +// so a renamed or duplicated tag would silently stop populating a column rather +// than fail anything. +func TestBundleDeployExperimentalSafeErrFields(t *testing.T) { + raw, err := json.Marshal(BundleDeployExperimental{ + DirectMigrateSafeErr: "a", + DirectMigrateCommitSafeErr: "b", + DirectMigrateWarningSafeErr: "c", + }) + require.NoError(t, err) + + var got map[string]any + require.NoError(t, json.Unmarshal(raw, &got)) + + assert.Equal(t, "a", got["direct_migrate_safe_error"]) + assert.Equal(t, "b", got["direct_migrate_commit_safe_error"]) + assert.Equal(t, "c", got["direct_migrate_warning_safe_error"]) +} + +// TestBundleDeployExperimentalSafeErrOmitted keeps a successful deploy from +// carrying three empty strings. +func TestBundleDeployExperimentalSafeErrOmitted(t *testing.T) { + raw, err := json.Marshal(BundleDeployExperimental{}) + require.NoError(t, err) + + assert.NotContains(t, string(raw), "safe_error") +}