diff --git a/acceptance/bin/verify_update.py b/acceptance/bin/verify_update.py new file mode 100755 index 00000000000..c1363a151fc --- /dev/null +++ b/acceptance/bin/verify_update.py @@ -0,0 +1,49 @@ +#!/usr/bin/env python3 +""" +Check that a plan applies at least one "update" and nothing more drastic. + +Used by the field_removal invariant test after dropping an optional field: the +removal must be observable as an in-place update, never ignored (all "skip") and +never a create/delete/recreate. +""" + +import json +import sys + + +def check_plan(path): + with open(path) as fobj: + raw = fobj.read() + + updates = 0 + unexpected = 0 + + try: + data = json.loads(raw) + for key, value in data["plan"].items(): + action = value.get("action") + if action == "update": + updates += 1 + elif action != "skip": + print(f"Unexpected {action=} for {key}") + unexpected += 1 + except Exception: + print(raw, flush=True) + raise + + if not updates: + print("Expected at least one update action, found none") + unexpected += 1 + + if unexpected: + print(raw, flush=True) + sys.exit(10) + + +def main(): + for path in sys.argv[1:]: + check_plan(path) + + +if __name__ == "__main__": + main() diff --git a/acceptance/bundle/invariant/README.md b/acceptance/bundle/invariant/README.md index 184d3f541c4..3dc1bf04fab 100644 --- a/acceptance/bundle/invariant/README.md +++ b/acceptance/bundle/invariant/README.md @@ -4,3 +4,12 @@ no_drift test checks that there are no actions planned after successful deploy. test will dump full JSON plan to the output. In order to add a new test, add a config to configs/ and include it in test.toml. + +The no_drift test additionally exercises removing non-required fields. Tag a field with a +`# CAN_REMOVE` comment on each of its lines; after the baseline no-drift check the test strips those +lines (`grep -v '# CAN_REMOVE'`), then asserts the resulting plan is an update and that deploying it +leaves no drift. This guards the class of bug where a field the config stops declaring is dropped from +the update request instead of being cleared. Configs without `# CAN_REMOVE` only run the baseline. If a +field's removal does not converge, tag it `# CANNOT_REMOVE` with a comment explaining the error or drift, +rather than leaving a failing `# CAN_REMOVE`. The marker is matched with its `# ` prefix, so +`# CANNOT_REMOVE` and prose that mentions the marker are neither stripped nor treated as a tag. diff --git a/acceptance/bundle/invariant/configs/app.yml.tmpl b/acceptance/bundle/invariant/configs/app.yml.tmpl index 153b04e9b56..bfd0b9bec6a 100644 --- a/acceptance/bundle/invariant/configs/app.yml.tmpl +++ b/acceptance/bundle/invariant/configs/app.yml.tmpl @@ -6,6 +6,7 @@ resources: foo: name: app-$UNIQUE_NAME source_code_path: ./app + description: This is a test app # CAN_REMOVE permissions: - level: CAN_USE group_name: users diff --git a/acceptance/bundle/invariant/configs/catalog.yml.tmpl b/acceptance/bundle/invariant/configs/catalog.yml.tmpl index c1bdbd3f9ec..75901f6061a 100644 --- a/acceptance/bundle/invariant/configs/catalog.yml.tmpl +++ b/acceptance/bundle/invariant/configs/catalog.yml.tmpl @@ -5,7 +5,7 @@ resources: catalogs: foo: name: test-catalog-$UNIQUE_NAME - comment: This is a test catalog + comment: This is a test catalog # CAN_REMOVE grants: - principal: account users privileges: diff --git a/acceptance/bundle/invariant/configs/catalog_optional_fields.yml.tmpl b/acceptance/bundle/invariant/configs/catalog_optional_fields.yml.tmpl index 9950f35211c..e13b9d45289 100644 --- a/acceptance/bundle/invariant/configs/catalog_optional_fields.yml.tmpl +++ b/acceptance/bundle/invariant/configs/catalog_optional_fields.yml.tmpl @@ -6,6 +6,11 @@ resources: foo: name: test-catalog-$UNIQUE_NAME comment: This is a test catalog + # CANNOT_REMOVE: not force-sent on update (terraform omits it too), so + # clearing it drops the field from the PATCH and the old value drifts back every plan. custom_max_retention_hours: 48 managed_encryption_settings: customer_managed_key_id: 00000000-0000-0000-0000-000000000000 + properties: + tier: bronze + team: data # CAN_REMOVE diff --git a/acceptance/bundle/invariant/configs/cluster_policy.yml.tmpl b/acceptance/bundle/invariant/configs/cluster_policy.yml.tmpl index 35c1dd4a02f..2bd9d90e538 100644 --- a/acceptance/bundle/invariant/configs/cluster_policy.yml.tmpl +++ b/acceptance/bundle/invariant/configs/cluster_policy.yml.tmpl @@ -6,6 +6,7 @@ resources: foo: name: test-cluster-policy-$UNIQUE_NAME definition: '{"spark_version":{"type":"fixed","value":"13.3.x-scala2.12"}}' + description: This is a test cluster policy # CAN_REMOVE permissions: - level: CAN_USE group_name: users diff --git a/acceptance/bundle/invariant/configs/dashboard.yml.tmpl b/acceptance/bundle/invariant/configs/dashboard.yml.tmpl index 857e0a2543c..c739ec72b52 100644 --- a/acceptance/bundle/invariant/configs/dashboard.yml.tmpl +++ b/acceptance/bundle/invariant/configs/dashboard.yml.tmpl @@ -5,6 +5,7 @@ resources: dashboards: foo: warehouse_id: $TEST_DEFAULT_WAREHOUSE_ID + # CANNOT_REMOVE: display_name is required; removing it fails validation ("dashboard display_name is required"). display_name: test-dashboard-$UNIQUE_NAME file_path: ./dashboard.lvdash.json permissions: diff --git a/acceptance/bundle/invariant/configs/external_location.yml.tmpl b/acceptance/bundle/invariant/configs/external_location.yml.tmpl index 28d36924a83..3fe403dbf71 100644 --- a/acceptance/bundle/invariant/configs/external_location.yml.tmpl +++ b/acceptance/bundle/invariant/configs/external_location.yml.tmpl @@ -7,7 +7,7 @@ resources: name: test_location_$UNIQUE_NAME url: s3://test-bucket/path credential_name: test_storage_credential - comment: "Test external location from DABs" + comment: "Test external location from DABs" # CAN_REMOVE grants: - principal: account users privileges: diff --git a/acceptance/bundle/invariant/configs/genie_space.yml.tmpl b/acceptance/bundle/invariant/configs/genie_space.yml.tmpl index 2e8c534df39..8b5756d50ab 100644 --- a/acceptance/bundle/invariant/configs/genie_space.yml.tmpl +++ b/acceptance/bundle/invariant/configs/genie_space.yml.tmpl @@ -6,6 +6,9 @@ resources: foo: warehouse_id: $TEST_DEFAULT_WAREHOUSE_ID title: test-genie-space-$UNIQUE_NAME + # CANNOT_REMOVE: description is not force-sent on update, so clearing it drops the + # field from the request and the old value drifts back on every subsequent plan. + description: This is a test genie space # Structured (inline) serialized_space is marshalled to a JSON string by # ConfigureGenieSpaceSerializedSpace; this config doubles as a regression # guard that the normalization produces a drift-free deploy. Kept minimal diff --git a/acceptance/bundle/invariant/configs/instance_pool.yml.tmpl b/acceptance/bundle/invariant/configs/instance_pool.yml.tmpl index 4a8d6bc5afd..7e6b231f487 100644 --- a/acceptance/bundle/invariant/configs/instance_pool.yml.tmpl +++ b/acceptance/bundle/invariant/configs/instance_pool.yml.tmpl @@ -6,6 +6,8 @@ resources: foo: instance_pool_name: test-instance-pool-$UNIQUE_NAME node_type_id: $NODE_TYPE_ID + idle_instance_autotermination_minutes: 60 # CAN_REMOVE + enable_elastic_disk: true # CAN_REMOVE permissions: - level: CAN_ATTACH_TO group_name: users diff --git a/acceptance/bundle/invariant/configs/job.yml.tmpl b/acceptance/bundle/invariant/configs/job.yml.tmpl index 696483648b5..7d54aec4f83 100644 --- a/acceptance/bundle/invariant/configs/job.yml.tmpl +++ b/acceptance/bundle/invariant/configs/job.yml.tmpl @@ -5,6 +5,7 @@ resources: jobs: foo: name: test-job-$UNIQUE_NAME + description: This is a test job # CAN_REMOVE permissions: - level: CAN_VIEW group_name: users diff --git a/acceptance/bundle/invariant/configs/model.yml.tmpl b/acceptance/bundle/invariant/configs/model.yml.tmpl index e105a731a33..8c8deed8aa2 100644 --- a/acceptance/bundle/invariant/configs/model.yml.tmpl +++ b/acceptance/bundle/invariant/configs/model.yml.tmpl @@ -5,3 +5,7 @@ resources: models: foo: name: test-model-$UNIQUE_NAME + # CANNOT_REMOVE: the MLflow model registry rejects an empty description on update with + # 400 "Description cannot be empty" (only reproducible on cloud; the local testserver + # accepts the cleared value). + description: This is a test model diff --git a/acceptance/bundle/invariant/configs/pipeline.yml.tmpl b/acceptance/bundle/invariant/configs/pipeline.yml.tmpl index 9cb1b4c7c21..331fa7124d2 100644 --- a/acceptance/bundle/invariant/configs/pipeline.yml.tmpl +++ b/acceptance/bundle/invariant/configs/pipeline.yml.tmpl @@ -5,6 +5,7 @@ resources: pipelines: foo: name: test-pipeline-$UNIQUE_NAME + development: true # CAN_REMOVE libraries: - file: path: pipeline.py diff --git a/acceptance/bundle/invariant/configs/postgres_branch.yml.tmpl b/acceptance/bundle/invariant/configs/postgres_branch.yml.tmpl index e17859b0b90..2a7fe6d2dc7 100644 --- a/acceptance/bundle/invariant/configs/postgres_branch.yml.tmpl +++ b/acceptance/bundle/invariant/configs/postgres_branch.yml.tmpl @@ -11,4 +11,8 @@ resources: foo: parent: ${resources.postgres_projects.project.name} branch_id: test-branch-$UNIQUE_NAME + # CANNOT_REMOVE: clearing no_expiry adds spec.expiration to the update_mask but drops the field + # from the request body (omitempty), which the backend rejects with 400 "in update_mask but not + # provided in request". (is_protected behaves the same but is omitted here: a + # protected branch cannot be deleted, which wedges the test's destroy-based cleanup.) no_expiry: true diff --git a/acceptance/bundle/invariant/configs/postgres_endpoint.yml.tmpl b/acceptance/bundle/invariant/configs/postgres_endpoint.yml.tmpl index fbffd428ff6..2e678e70ad2 100644 --- a/acceptance/bundle/invariant/configs/postgres_endpoint.yml.tmpl +++ b/acceptance/bundle/invariant/configs/postgres_endpoint.yml.tmpl @@ -18,3 +18,9 @@ resources: parent: ${resources.postgres_branches.branch.name} endpoint_id: test-endpoint-$UNIQUE_NAME endpoint_type: ENDPOINT_TYPE_READ_WRITE + # CANNOT_REMOVE: clearing these adds them to the update_mask but drops the fields from the + # request body (omitempty), which the backend rejects with 400 "Field 'spec.autoscaling_limit_max_cu' + # is in update_mask but not provided in request". + autoscaling_limit_min_cu: 1 + autoscaling_limit_max_cu: 4 + disabled: true diff --git a/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl b/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl index 7fb119b7060..c8f5dbd13ea 100644 --- a/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl +++ b/acceptance/bundle/invariant/configs/postgres_project.yml.tmpl @@ -5,8 +5,15 @@ resources: postgres_projects: foo: project_id: test-pg-project-$UNIQUE_NAME + # CANNOT_REMOVE: clearing display_name adds spec.display_name to the update_mask but drops + # the field from the request body (omitempty), which the backend rejects with 400 + # "Field 'spec.display_name' is in update_mask but not provided in request". display_name: Test Postgres Project enable_pg_native_login: false + # CANNOT_REMOVE: same update_mask 400 as display_name above — clearing it adds + # spec.default_endpoint_settings to the update_mask but drops it from the request body. + # Kept on one line so a future fix can flip it to CAN_REMOVE and have grep -v strip it cleanly. + default_endpoint_settings: {"autoscaling_limit_max_cu": 4} permissions: - level: CAN_USE group_name: users diff --git a/acceptance/bundle/invariant/configs/registered_model.yml.tmpl b/acceptance/bundle/invariant/configs/registered_model.yml.tmpl index 8a146c70dd9..8b1fa846e6a 100644 --- a/acceptance/bundle/invariant/configs/registered_model.yml.tmpl +++ b/acceptance/bundle/invariant/configs/registered_model.yml.tmpl @@ -7,6 +7,7 @@ resources: name: test-model-$UNIQUE_NAME catalog_name: main schema_name: default + comment: This is a test registered model # CAN_REMOVE grants: - principal: account users privileges: diff --git a/acceptance/bundle/invariant/configs/schema.yml.tmpl b/acceptance/bundle/invariant/configs/schema.yml.tmpl index d9aebda0cef..f64f43c511f 100644 --- a/acceptance/bundle/invariant/configs/schema.yml.tmpl +++ b/acceptance/bundle/invariant/configs/schema.yml.tmpl @@ -6,6 +6,11 @@ resources: foo: catalog_name: main name: test-schema-$UNIQUE_NAME + comment: This is a test schema # CAN_REMOVE + # CANNOT_REMOVE: properties is a map, and ForceSendFields is inert for maps, so + # clearing the whole map drops it from the request and the old value drifts back every plan. + # Kept on one line so a future fix can flip it to CAN_REMOVE and have grep -v strip it cleanly. + properties: {"tier": "bronze"} grants: - principal: account users privileges: diff --git a/acceptance/bundle/invariant/configs/secret.yml.tmpl b/acceptance/bundle/invariant/configs/secret.yml.tmpl index 2218b827032..ed8ba82eb32 100644 --- a/acceptance/bundle/invariant/configs/secret.yml.tmpl +++ b/acceptance/bundle/invariant/configs/secret.yml.tmpl @@ -12,6 +12,9 @@ resources: schema_name: default name: test-secret-$UNIQUE_NAME value: ${var.secret_value} + # CANNOT_REMOVE: comment is not force-sent on update, so clearing it drops the field + # from the request and the old value drifts back on every subsequent plan. + comment: This is a test secret grants: - principal: account users privileges: diff --git a/acceptance/bundle/invariant/configs/sql_warehouse.yml.tmpl b/acceptance/bundle/invariant/configs/sql_warehouse.yml.tmpl index 56901354c6e..0110ebdbb2d 100644 --- a/acceptance/bundle/invariant/configs/sql_warehouse.yml.tmpl +++ b/acceptance/bundle/invariant/configs/sql_warehouse.yml.tmpl @@ -6,8 +6,8 @@ resources: foo: name: test-warehouse-$UNIQUE_NAME cluster_size: 2X-Small - auto_stop_mins: 10 - max_num_clusters: 1 + auto_stop_mins: 10 # CAN_REMOVE + max_num_clusters: 1 # CAN_REMOVE min_num_clusters: 1 warehouse_type: CLASSIC permissions: diff --git a/acceptance/bundle/invariant/configs/vector_search_endpoint.yml.tmpl b/acceptance/bundle/invariant/configs/vector_search_endpoint.yml.tmpl index cea1a4d026c..1a8ff4c4ce5 100644 --- a/acceptance/bundle/invariant/configs/vector_search_endpoint.yml.tmpl +++ b/acceptance/bundle/invariant/configs/vector_search_endpoint.yml.tmpl @@ -6,6 +6,10 @@ resources: foo: name: test-endpoint-$UNIQUE_NAME endpoint_type: STANDARD + # CANNOT_REMOVE: target_qps is not re-sent on update, so clearing it leaves the old value + # and the plan re-proposes the change on every deploy (only reproducible on cloud; the + # local testserver converges). + target_qps: 2 bar: # Endpoint names must be < 50 chars, so keep this prefix short. name: test-vse-perm-$UNIQUE_NAME diff --git a/acceptance/bundle/invariant/configs/volume.yml.tmpl b/acceptance/bundle/invariant/configs/volume.yml.tmpl index 7868893619b..fc61b3311e7 100644 --- a/acceptance/bundle/invariant/configs/volume.yml.tmpl +++ b/acceptance/bundle/invariant/configs/volume.yml.tmpl @@ -7,6 +7,7 @@ resources: name: test-volume-$UNIQUE_NAME catalog_name: main schema_name: default + comment: This is a test volume # CAN_REMOVE grants: - principal: account users privileges: diff --git a/acceptance/bundle/invariant/no_drift/script b/acceptance/bundle/invariant/no_drift/script index 9f031c0f613..0961e8edac8 100644 --- a/acceptance/bundle/invariant/no_drift/script +++ b/acceptance/bundle/invariant/no_drift/script @@ -1,4 +1,4 @@ -# Invariant to test: no drift after deploy +# Invariant to test: no drift after deploy, and that removing a non-required field converges. # Additional checks: no internal errors / panics in validate/plan/deploy invariant_setup @@ -14,3 +14,26 @@ fi invariant_deploy LOG.deploy $CLI bundle deploy $(readplanarg plan.json) invariant_verify_no_drift + +# Field removal: dropping a non-required field (tagged `# CAN_REMOVE` on each of its lines) must be +# applied as an update that converges -- a field the config stops declaring must be sent as its zero +# value, not silently dropped from the request (which either fails or drifts forever). Runs only on the +# non-READPLAN variant (the saved-plan path is orthogonal) and only for configs that tag a field. +# `# CANNOT_REMOVE` documents a field whose removal is broken and is deliberately kept; the marker is +# matched with its `# ` prefix so neither it nor prose mentioning the marker is stripped. +if [[ -z "$READPLAN" ]] && grep -q '# CAN_REMOVE' databricks.yml; then + grep -v '# CAN_REMOVE' databricks.yml > databricks.yml.new + mv databricks.yml.new databricks.yml + cp databricks.yml LOG.config.removed + + $CLI bundle plan -o json > LOG.plan_removed.json 2>LOG.plan_removed.err + cat LOG.plan_removed.err | contains.py '!panic:' '!internal error' > /dev/null + verify_update.py LOG.plan_removed.json + + # Plain deploy (not invariant_deploy): a second INPUT_CONFIG_OK would make output.txt differ + # from the variants that skip this block. + trace $CLI bundle deploy &> LOG.deploy_removed + cat LOG.deploy_removed | contains.py '!panic:' '!internal error' > /dev/null + + invariant_verify_no_drift +fi