direct: Fix permissions for resources.models (#4941)

denik · web-flow · commit eacf329943fd · 2026-04-13T13:30:13.000Z
## Changes
Pass correct ID to permissions API for resources.models.
 
## Why
This resource has two IDs, one coming from Name field - used for all
CRUD operations on the resource itself and another numerical ID that
needs to be used for permissions request. Since we handle "id" specially
in direct, we cannot reference it via $resources.models.foo.id anymore -
it always points to CRUD id. So we make a wrapper struct that stores
numerical ID under "model_id" and reference that.

## Tests
New acceptance test and invariant test.

Also fixes deployment of mlops-stack template with direct and enables
that test on local (previously it was cloud-only).
diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md
@@ -11,6 +11,7 @@
 
 ### Bundles
 * Added support for lifecycle.started option for apps ([#4672](https://github.com/databricks/cli/pull/4672))
+* engine/direct: Fix permissions for resources.models ([#4941](https://github.com/databricks/cli/pull/4941))
 
 ### Dependency updates
 
diff --git a/acceptance/bundle/deploy/mlops-stacks/out.test.toml b/acceptance/bundle/deploy/mlops-stacks/out.test.toml
diff --git a/acceptance/bundle/deploy/mlops-stacks/test.toml b/acceptance/bundle/deploy/mlops-stacks/test.toml
@@ -1,20 +1,12 @@
 Cloud=true
-Local=false
+Local=true
 
 Badness = "the newly initialized bundle from the 'mlops-stacks' template contains two validation warnings in the configuration"
 
 Ignore = [
     "config.json"
 ]
 
-EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform"]
-# On direct, this fails with
-#        +Endpoint: PUT [DATABRICKS_URL]/api/2.0/permissions/registered-models/%5Bdev%20[USERNAME]%5D%20dev-project_name_[UNIQUE_NAME]-model
-#        +HTTP Status: 400 Bad Request
-#        +API error_code: INVALID_PARAMETER_VALUE
-#        +API message: '[dev [USERNAME]] dev-project_name_[UNIQUE_NAME]-model' is not a valid registered model ID.
-
-
 [[Repls]]
 Old = "aws|azure|gcp"
 New = "[CLOUD_ENV_BASE]"
diff --git a/acceptance/bundle/invariant/configs/model_with_permissions.yml.tmpl b/acceptance/bundle/invariant/configs/model_with_permissions.yml.tmpl
@@ -0,0 +1,10 @@
+bundle:
+  name: test-bundle-$UNIQUE_NAME
+
+resources:
+  models:
+    foo:
+      name: test-model-$UNIQUE_NAME
+      permissions:
+        - level: CAN_READ
+          group_name: users
diff --git a/acceptance/bundle/invariant/continue_293/out.test.toml b/acceptance/bundle/invariant/continue_293/out.test.toml
diff --git a/acceptance/bundle/invariant/migrate/out.test.toml b/acceptance/bundle/invariant/migrate/out.test.toml
diff --git a/acceptance/bundle/invariant/no_drift/out.test.toml b/acceptance/bundle/invariant/no_drift/out.test.toml
diff --git a/acceptance/bundle/invariant/test.toml b/acceptance/bundle/invariant/test.toml
@@ -38,6 +38,7 @@ EnvMatrix.INPUT_CONFIG = [
     "job_with_permissions.yml.tmpl",
     "job_with_task.yml.tmpl",
     "model.yml.tmpl",
+    "model_with_permissions.yml.tmpl",
     "model_serving_endpoint.yml.tmpl",
     "pipeline.yml.tmpl",
     "postgres_branch.yml.tmpl",
diff --git a/acceptance/bundle/refschema/out.fields.txt b/acceptance/bundle/refschema/out.fields.txt
@@ -2105,6 +2105,7 @@ resources.models.*.latest_versions[*].user_id	string	REMOTE
 resources.models.*.latest_versions[*].version	string	REMOTE
 resources.models.*.lifecycle	resources.Lifecycle	INPUT
 resources.models.*.lifecycle.prevent_destroy	bool	INPUT
+resources.models.*.model_id	string	REMOTE
 resources.models.*.modified_status	string	INPUT
 resources.models.*.name	string	ALL
 resources.models.*.permission_level	ml.PermissionLevel	REMOTE
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/out.plan.direct.json b/acceptance/bundle/resources/permissions/models/current_can_manage/out.plan.direct.json
@@ -14,7 +14,7 @@
       "depends_on": [
         {
           "node": "resources.models.foo",
-          "label": "${resources.models.foo.id}"
+          "label": "${resources.models.foo.model_id}"
         }
       ],
       "action": "create",
@@ -41,7 +41,7 @@
           ]
         },
         "vars": {
-          "object_id": "/registered-models/${resources.models.foo.id}"
+          "object_id": "/registered-models/${resources.models.foo.model_id}"
         }
       }
     }
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/out.plan.terraform.json b/acceptance/bundle/resources/permissions/models/current_can_manage/out.plan.terraform.json
@@ -0,0 +1,11 @@
+{
+  "cli_version": "[DEV_VERSION]",
+  "plan": {
+    "resources.models.foo": {
+      "action": "create"
+    },
+    "resources.models.foo.permissions": {
+      "action": "create"
+    }
+  }
+}
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.deploy.direct.json b/acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.deploy.direct.json
@@ -1,6 +1,6 @@
 {
   "method": "PUT",
-  "path": "/api/2.0/permissions/registered-models/test-model",
+  "path": "/api/2.0/permissions/registered-models/[FOO_MODEL_ID]",
   "body": {
     "access_control_list": [
       {
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.deploy.terraform.json b/acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.deploy.terraform.json
@@ -0,0 +1,24 @@
+{
+  "method": "PUT",
+  "path": "/api/2.0/permissions/registered-models/[FOO_MODEL_ID]",
+  "body": {
+    "access_control_list": [
+      {
+        "permission_level": "CAN_READ",
+        "user_name": "viewer@example.com"
+      },
+      {
+        "permission_level": "CAN_MANAGE",
+        "service_principal_name": "[UUID]"
+      },
+      {
+        "group_name": "data-team",
+        "permission_level": "CAN_MANAGE"
+      },
+      {
+        "permission_level": "CAN_MANAGE",
+        "user_name": "[USERNAME]"
+      }
+    ]
+  }
+}
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.destroy.terraform.json b/acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.destroy.terraform.json
@@ -0,0 +1,12 @@
+{
+  "method": "PUT",
+  "path": "/api/2.0/permissions/registered-models/[FOO_MODEL_ID]",
+  "body": {
+    "access_control_list": [
+      {
+        "permission_level": "CAN_MANAGE",
+        "user_name": "[USERNAME]"
+      }
+    ]
+  }
+}
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/out.test.toml b/acceptance/bundle/resources/permissions/models/current_can_manage/out.test.toml
diff --git a/acceptance/bundle/resources/permissions/models/current_can_manage/script b/acceptance/bundle/resources/permissions/models/current_can_manage/script
@@ -10,6 +10,12 @@ print_requests() {
 
 rm out.requests.txt
 trace errcode $CLI bundle deploy &> out.deploy.txt
+
+# Register the model's numeric ID for output replacement.
+# The permissions API uses the numeric ID, not the model name.
+MODEL_ID=$($CLI model-registry get-model test-model | jq -r '.registered_model_databricks.id')
+add_repl.py "$MODEL_ID" "FOO_MODEL_ID"
+
 print_requests > out.requests.deploy.$DATABRICKS_BUNDLE_ENGINE.json
 
 trace $CLI bundle destroy --auto-approve
diff --git a/acceptance/bundle/resources/permissions/models/test.toml b/acceptance/bundle/resources/permissions/models/test.toml
@@ -1,2 +1,2 @@
 Env.RESOURCE = "models"  # for ../_script
-EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"]  # terraform mapping issue
+EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"]
diff --git a/acceptance/bundle/resources/permissions/output.txt b/acceptance/bundle/resources/permissions/output.txt
@@ -319,8 +319,25 @@ DIFF  jobs/viewers/out.requests.destroy.direct.json
 +    "path": "/api/2.0/permissions/jobs/[NUMID]"
 +  }
 +]
-ERROR models/current_can_manage/out.requests.deploy.direct.json: Missing terraform file models/current_can_manage/out.requests.deploy.terraform.json
-ERROR models/current_can_manage/out.requests.destroy.direct.json: Missing terraform file models/current_can_manage/out.requests.destroy.terraform.json
+MATCH models/current_can_manage/out.requests.deploy.direct.json
+DIFF  models/current_can_manage/out.requests.destroy.direct.json
+--- models/current_can_manage/out.requests.destroy.direct.json
++++ models/current_can_manage/out.requests.destroy.terraform.json
+@@ -1 +1,14 @@
+-[]+[
++  {
++    "body": {
++      "access_control_list": [
++        {
++          "permission_level": "CAN_MANAGE",
++          "user_name": "[USERNAME]"
++        }
++      ]
++    },
++    "method": "PUT",
++    "path": "/api/2.0/permissions/registered-models/[FOO_MODEL_ID]"
++  }
++]
 MATCH pipelines/current_can_manage/out.requests.deploy.direct.json
 EXACT pipelines/current_can_manage/out.requests.destroy.direct.json
 EXACT pipelines/current_is_owner/out.requests.deploy.direct.json
diff --git a/bundle/direct/dresources/model.go b/bundle/direct/dresources/model.go
@@ -13,6 +13,15 @@ type ResourceMlflowModel struct {
 	client *databricks.WorkspaceClient
 }
 
+// MlflowModelRemote wraps the API response with the numeric model ID.
+// The state ID for models is the model name (used for CRUD operations), but
+// the permissions API requires the numeric ID. This wrapper exposes the numeric
+// ID as model_id, analogous to RefreshOutput.EndpointId for serving endpoints.
+type MlflowModelRemote struct {
+	ml.ModelDatabricks
+	ModelId string `json:"model_id"`
+}
+
 func (*ResourceMlflowModel) New(client *databricks.WorkspaceClient) *ResourceMlflowModel {
 	return &ResourceMlflowModel{client: client}
 }
@@ -21,53 +30,39 @@ func (*ResourceMlflowModel) PrepareState(input *resources.MlflowModel) *ml.Creat
 	return &input.CreateModelRequest
 }
 
-func (*ResourceMlflowModel) RemapState(model *ml.ModelDatabricks) *ml.CreateModelRequest {
+func (*ResourceMlflowModel) RemapState(output *MlflowModelRemote) *ml.CreateModelRequest {
 	return &ml.CreateModelRequest{
-		Name:            model.Name,
-		Tags:            model.Tags,
-		Description:     model.Description,
-		ForceSendFields: utils.FilterFields[ml.CreateModelRequest](model.ForceSendFields),
+		Name:            output.Name,
+		Tags:            output.Tags,
+		Description:     output.Description,
+		ForceSendFields: utils.FilterFields[ml.CreateModelRequest](output.ForceSendFields),
 	}
 }
 
-func (r *ResourceMlflowModel) DoRead(ctx context.Context, id string) (*ml.ModelDatabricks, error) {
+func (r *ResourceMlflowModel) DoRead(ctx context.Context, id string) (*MlflowModelRemote, error) {
 	response, err := r.client.ModelRegistry.GetModel(ctx, ml.GetModelRequest{
 		Name: id,
 	})
 	if err != nil {
 		return nil, err
 	}
-	return response.RegisteredModelDatabricks, nil
+	return &MlflowModelRemote{
+		ModelDatabricks: *response.RegisteredModelDatabricks,
+		ModelId:         response.RegisteredModelDatabricks.Id,
+	}, nil
 }
 
-func (r *ResourceMlflowModel) DoCreate(ctx context.Context, config *ml.CreateModelRequest) (string, *ml.ModelDatabricks, error) {
+func (r *ResourceMlflowModel) DoCreate(ctx context.Context, config *ml.CreateModelRequest) (string, *MlflowModelRemote, error) {
 	response, err := r.client.ModelRegistry.CreateModel(ctx, *config)
 	if err != nil {
 		return "", nil, err
 	}
-	// Create API call returns [ml.Model] while DoRead returns [ml.ModelDatabricks].
-	// Thus we need to convert the response to the expected type.
-	modelDatabricks := &ml.ModelDatabricks{
-		Name:            response.RegisteredModel.Name,
-		Description:     response.RegisteredModel.Description,
-		Tags:            response.RegisteredModel.Tags,
-		ForceSendFields: utils.FilterFields[ml.ModelDatabricks](response.RegisteredModel.ForceSendFields, "CreationTimestamp", "Id", "LastUpdatedTimestamp", "LatestVersions", "PermissionLevel", "UserId"),
-
-		// Coping the fields only to satisfy the linter. These fields are not
-		// part of the configuration tree so they don't need to be copied.
-		// The linter works as a safeguard to ensure we add new fields to the bundle config tree
-		// to the mapping logic here as well.
-		CreationTimestamp:    0,
-		Id:                   "",
-		LastUpdatedTimestamp: 0,
-		LatestVersions:       nil,
-		PermissionLevel:      "",
-		UserId:               "",
-	}
-	return response.RegisteredModel.Name, modelDatabricks, nil
+	// Return nil for refresh output; the engine will call DoRead to populate the full state
+	// including the numeric model ID needed for permissions.
+	return response.RegisteredModel.Name, nil, nil
 }
 
-func (r *ResourceMlflowModel) DoUpdate(ctx context.Context, id string, config *ml.CreateModelRequest, _ *PlanEntry) (*ml.ModelDatabricks, error) {
+func (r *ResourceMlflowModel) DoUpdate(ctx context.Context, id string, config *ml.CreateModelRequest, entry *PlanEntry) (*MlflowModelRemote, error) {
 	updateRequest := ml.UpdateModelRequest{
 		Name:            id,
 		Description:     config.Description,
@@ -79,26 +74,27 @@ func (r *ResourceMlflowModel) DoUpdate(ctx context.Context, id string, config *m
 		return nil, err
 	}
 
-	// Update API call returns [ml.Model] while DoRead returns [ml.ModelDatabricks].
-	// Thus we need to convert the response to the expected type.
-	modelDatabricks := &ml.ModelDatabricks{
-		Name:            response.RegisteredModel.Name,
-		Description:     response.RegisteredModel.Description,
-		Tags:            response.RegisteredModel.Tags,
-		ForceSendFields: utils.FilterFields[ml.ModelDatabricks](response.RegisteredModel.ForceSendFields, "CreationTimestamp", "Id", "LastUpdatedTimestamp", "LatestVersions", "PermissionLevel", "UserId"),
-
-		// Coping the fields only to satisfy the linter. These fields are not
-		// part of the configuration tree so they don't need to be copied.
-		// The linter works as a safeguard to ensure we add new fields to the bundle config tree
-		// to the mapping logic here as well.
-		CreationTimestamp:    0,
-		Id:                   "",
-		LastUpdatedTimestamp: 0,
-		LatestVersions:       nil,
-		PermissionLevel:      "",
-		UserId:               "",
+	// Carry forward model_id from existing state since UpdateModelResponse doesn't include it.
+	var modelId string
+	if old, ok := entry.RemoteState.(*MlflowModelRemote); ok {
+		modelId = old.ModelId
 	}
-	return modelDatabricks, nil
+
+	return &MlflowModelRemote{
+		ModelDatabricks: ml.ModelDatabricks{
+			CreationTimestamp:    0,
+			Description:          response.RegisteredModel.Description,
+			Id:                   "",
+			LastUpdatedTimestamp: 0,
+			LatestVersions:       nil,
+			Name:                 response.RegisteredModel.Name,
+			PermissionLevel:      "",
+			Tags:                 response.RegisteredModel.Tags,
+			UserId:               "",
+			ForceSendFields:      utils.FilterFields[ml.ModelDatabricks](response.RegisteredModel.ForceSendFields, "CreationTimestamp", "Id", "LastUpdatedTimestamp", "LatestVersions", "PermissionLevel", "UserId"),
+		},
+		ModelId: modelId,
+	}, nil
 }
 
 func (r *ResourceMlflowModel) DoDelete(ctx context.Context, id string) error {
diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go
@@ -84,6 +84,12 @@ func PreparePermissionsInputConfig(inputConfig any, node string) (*structvar.Str
 		objectIdRef = prefix + "${" + baseNode + ".endpoint_id}"
 	}
 
+	// MLflow models use the model name as the state ID (for CRUD operations),
+	// but the permissions API requires the numeric model ID.
+	if strings.HasPrefix(baseNode, "resources.models.") {
+		objectIdRef = prefix + "${" + baseNode + ".model_id}"
+	}
+
 	// Postgres projects store their hierarchical name ("projects/{project_id}") as the state ID,
 	// but the permissions API expects just the project_id.
 	if strings.HasPrefix(baseNode, "resources.postgres_projects.") {
diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go
diff --git a/libs/testserver/models.go b/libs/testserver/models.go

Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@`
`14`	`14`	`"depends_on": [`
`15`	`15`	`{`
`16`	`16`	`"node": "resources.models.foo",`
`17`		`- "label": "${resources.models.foo.id}"`
	`17`	`+ "label": "${resources.models.foo.model_id}"`
`18`	`18`	`}`
`19`	`19`	`],`
`20`	`20`	`"action": "create",`
`@@ -41,7 +41,7 @@`
`41`	`41`	`]`
`42`	`42`	`},`
`43`	`43`	`"vars": {`
`44`		`- "object_id": "/registered-models/${resources.models.foo.id}"`
	`44`	`+ "object_id": "/registered-models/${resources.models.foo.model_id}"`
`45`	`45`	`}`
`46`	`46`	`}`
`47`	`47`	`}`