direct: Fix permissions for resources.models#4941
Merged
Conversation
The test server was returning the model name as the model ID in ModelRegistryGetModel, which masked a bug where the direct engine uses the model name instead of the numeric ID for permissions API calls. The existing model permissions test output at acceptance/bundle/resources/permissions/models/current_can_manage/out.requests.deploy.direct.json shows the bug: the permissions path is /api/2.0/permissions/registered-models/test-model (name) instead of /api/2.0/permissions/registered-models/[NUMID] (numeric ID). Co-authored-by: Isaac
The direct engine was using the model name as the object ID when calling
the permissions API (PUT /api/2.0/permissions/registered-models/{id}).
The real API requires a numeric model ID, causing failures on cloud while
passing locally due to a lenient mock server.
The fix follows the same pattern as model_serving_endpoints: wrap the API
response in MlflowModelRefreshOutput which exposes the numeric ID as
model_id, and reference it in the permissions config instead of the
state ID (which is the model name).
Co-authored-by: Isaac
Now that model permissions use the numeric ID instead of the model name, the mlops-stacks test works with both terraform and direct engines. Co-authored-by: Isaac
Co-authored-by: Isaac
- Enable terraform engine in the model permissions test (was direct-only due to a now-resolved mapping issue) - In DoUpdate, carry forward model_id from existing state instead of returning nil (which would trigger an extra DoRead request) Co-authored-by: Isaac
Adds a no_drift invariant test for models with permissions, verifying that deploy succeeds and produces no drift on subsequent plan. Co-authored-by: Isaac
Use add_repl.py with the model's numeric ID fetched via model-registry get-model, so the test output shows [FOO_MODEL_ID] instead of the generic [NUMID] replacement. Co-authored-by: Isaac
Follow the established *Remote naming convention used by JobRemote, PipelineRemote, and AppRemote. Co-authored-by: Isaac
andrewnester
approved these changes
Apr 13, 2026
janniklasrose
approved these changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
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).