Add per-resource permissions support to cluster_policies - #6304
Merged
Conversation
Collaborator
Integration test reportCommit: 366aa22
Top 3 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
marked this pull request as ready for review
August 18, 2026 13:45
| @@ -0,0 +1,22 @@ | |||
| cleanup() { | |||
Contributor
There was a problem hiding this comment.
the basic test isn't really interesting (should be covered by invariant test already).
maybe add a test for out-of-band recreate & eventual consistency of the resource (remember we discussed v1 vs v2 permissions API), since that's not covered by tests
Collaborator
Author
There was a problem hiding this comment.
This resource like genie_spaces also uses v1 permissions, so there can be drift on out_of_band changes. Should this be left as is, or something be fixed.
I have removed basic/ and added 2 out_of_band* tests
Sankalp-Mittal
force-pushed
the
sankalp-mittal/cluster-policy-permissions
branch
from
August 24, 2026 15:38
ebd6fbf to
1c3b7c2
Compare
janniklasrose
approved these changes
Aug 24, 2026
Base automatically changed from
sankalp-mittal/cluster-policy-resource
to
main
August 24, 2026 22:39
Cluster policies support a single ACL level, CAN_USE (SDK compute.ClusterPolicyPermissionLevel). Wire up the per-resource `permissions` field so a policy can grant CAN_USE to users, groups, and service principals via the direct engine's generic permissions applier: - Declare the ClusterPolicyPermission type and add the Permissions field. - Register cluster_policies.permissions in the direct engine and map the permissions-API object type to /cluster-policies/. - Regenerate the bundle JSON schema and fill in the field descriptions. cluster_policies is intentionally left out of the top-level levelsMap (it stays in unsupportedResources): the bundle-wide permissions block is expressed in CAN_MANAGE/CAN_VIEW/CAN_RUN, none of which a cluster policy can accept, so only the explicit per-resource form is supported. Co-authored-by: Isaac
Add basic and out_of_band_change acceptance tests for the cluster_policies `permissions` field, filtering permission requests with print_requests.py. The basic test surfaced that FixPermissions injects the current user as CAN_MANAGE onto every resource with a permissions field. Cluster policies only accept CAN_USE, so the real backend would reject that. Add cluster_policies to fix_permissions.go's ignoredResources (like secret_scopes) so only the user-specified CAN_USE grants are sent. Co-authored-by: Isaac
Add a Cloud=true test that deploys a policy with CAN_USE (accepted) and then asserts a direct CAN_MANAGE set-permissions call is rejected. Model the rejection in the testserver: the permissions API only accepts CAN_USE for cluster policies, so a non-CAN_USE level returns 400. This keeps the local run and the cloud run in agreement. The backend error text differs between the fake and a real workspace, so it is routed to a LOG file and musterr asserts only that the request fails. Co-authored-by: Isaac
Runs generate-check output: the refschema field list, the level enum (CAN_USE only), and the required-field entry for the new cluster_policies permissions block. Co-authored-by: Isaac
TestInvariantConfigsCoverage requires every resource with a permissions field to have an invariant config exercising it. Attach a CAN_USE grant to the cluster_policies invariant config. Co-authored-by: Isaac
Address review feedback: instead of a raw set-permissions CLI call, flip
the grant to CAN_MANAGE with update_file.py and assert the bundle deploy
fails.
Sync the testserver's rejection message to the real permissions API
("Unknown Cluster Policy Permission Level: <level>", captured from a
cloud run) so the fake and cloud agree, and assert it verbatim in the
golden. Verified against a real workspace via deco env run.
Co-authored-by: Isaac
The failing redeploy's upload count differs between the fake and cloud, so its output cannot live in the golden. Follow the repo convention: capture the deploy to LOG and assert the backend's error message with contains.py. Verified against a real workspace via deco env run. Co-authored-by: Isaac
The base branch now sets Cloud=true for cluster_policies tests. The basic and out_of_band_change permission tests use a fixed policy name and are not cloud-safe for concurrent runs, so pin them Cloud=false. The levels test remains the cloud-run coverage (it uses UNIQUE_NAME). Co-authored-by: Isaac
More precise than out_of_band_change (which, at the resource level, means a definition change): this test grants an extra principal out of band and checks the bundle reconciles it. Parallels the out_of_band_deletion naming from the genie_spaces precedent. Co-authored-by: Isaac
Add v1 out-of-band deletion coverage for cluster_policies permissions: the policy is deleted out of band, its ACL lingers (no 404, unlike a v2 cascade delete), and plan recreates it. Modeled on the genie_spaces out_of_band_deletion precedent. Remove the basic test: its create/deploy path is already covered by the invariant config, per review feedback. Co-authored-by: Isaac
Sankalp-Mittal
enabled auto-merge
August 24, 2026 22:41
Sankalp-Mittal
force-pushed
the
sankalp-mittal/cluster-policy-permissions
branch
from
August 24, 2026 22:45
84d6c30 to
4c3cd95
Compare
The test passes a leading-slash path to `api get`; on Windows, Git Bash rewrites /api/... to a Windows path before the CLI sees it, so the request misses the testserver stub. Set Env.MSYS_NO_PATHCONV, matching the genie_spaces out_of_band_deletion precedent. Co-authored-by: Isaac
With MSYS_NO_PATHCONV set (needed for the api get leading-slash path), Git Bash on Windows no longer converts the PATH-resolved location of the read_id.py helper, so python3 cannot open it. Fetch the policy id via bundle summary + add_repl (a shell function), matching the genie_spaces precedent. Golden output is unchanged. Co-authored-by: Isaac
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.
Summary
Stacked on #6225 (cluster policy resource). Adds the per-resource
permissionsfield tocluster_policiesso a policy can grant access via the direct engine's generic permissions applier.Cluster policies support a single ACL level —
CAN_USE(SDKcompute.ClusterPolicyPermissionLevel), meaning "may create clusters governed by this policy." There is noCAN_MANAGE/CAN_VIEW/CAN_RUN; policy administration is an admin-only function.Changes
ClusterPolicyPermission, add thePermissionsfield, registercluster_policies.permissionsin the direct engine, and map the permissions-API object type to/cluster-policies/.FixPermissions: addcluster_policiestoignoredResourcesso the deploying user is not auto-injected asCAN_MANAGE/IS_OWNER— cluster policies can't accept it, and the real backend would reject the deploy (same treatment assecret_scopes).CAN_USElevel on a cluster policy returns400, so local and cloud runs agree.levelenum =CAN_USEonly,levelrequired).Design note
cluster_policiesis intentionally not added to the top-levellevelsMap; it stays inunsupportedResources. The bundle-widepermissions:block is expressed inCAN_MANAGE/CAN_VIEW/CAN_RUN(and auto-grants the deployerCAN_MANAGE), none of which a cluster policy can accept. Supporting only the explicit per-resource form avoids silently dropping those grants. Two current-user injection paths had to be handled for this: the top-levellevelsMapand theFixPermissionsmutator.Testing
Acceptance tests under
acceptance/bundle/resources/cluster_policies/permissions/:basic/— deploy applies theCAN_USEgrants, redeploy updates the ACL; requests asserted withprint_requests.py.out_of_band_change/— an out-of-band ACL change is detected bybundle planand reconciled on redeploy.levels/(Cloud=true) —CAN_USEdeploys successfully; a directCAN_MANAGEset-permissionscall is rejected (asserted withmusterr; the backend error differs between the fake and cloud, so it is routed to aLOGfile rather than pinned).Plus the
cluster_policies.permissionsCRUD roundtrip fixture indresources/all_test.go. Unit + acceptance suites pass locally.This pull request and its description were written by Isaac.