Repository navigation
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new IAM module designed to enforce the principle of least privilege for Slurm clusters on GCP. By replacing broad, project-wide predefined roles with granular, profile-based custom roles and resource-scoped bindings, it significantly reduces the attack surface of service accounts. The changes include support for various cluster features via opt-in capability flags and ensure that sensitive permissions like metadata modification are strictly scoped to relevant resources. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new community module, service-account-profiles, which applies named least-privilege permission profiles to existing service accounts. It updates several Slurm and image-builder examples to utilize this module instead of broad predefined roles, adds a dedicated example blueprint, updates documentation indexes, and introduces comprehensive validation tests. Feedback on the changes to ghpcfe_c2daemon.py highlights a critical bug where a return statement is incorrectly indented inside a loop, causing premature termination, and notes that the changes accidentally revert robust timeout and retry logic; it is recommended to revert this file entirely.
|
/gcbrun |
ep-nag
changed the title
6044b35 to
25516a9
Compare
|
/gcbrun |
AdarshK15
added
the
release-new-modules
|
this PR has been inactive for 2 days and has no unresolved comments. @AdarshK15, please review. |
Name the modules/project/service-account block <x>_sa and the service-account-profiles block <x>_profile, with use: [<x>_sa]. Consumers reference <x>_profile so they still wait for iam_propagation_wait. hpc-enterprise-slurm and hpc-slurm-ha had controller_sa, login_sa and compute_sa on develop. Renaming them moved the accounts to new Terraform addresses, so a redeploy planned to destroy and recreate each account. Both blueprints now keep the develop ids. hpc-slurm-ha also restores its develop account names (controller-sa, login-sa, compute-sa), description and display_name, so the account ids are unchanged too. The other blueprints, the test config, the README examples and two daily-test comments follow the same convention.
gcluster passes a deployment variable to every module with an input of the same name, so a blueprint that sets enable_public_ips in vars made the slurm-login and slurm-compute profiles fail validation. Profiles that accept no capability flags (slurm-login, slurm-compute, image-builder) now ignore them, and the generated role is built only from flags the profile accepts. custom still rejects enable_mig, enable_mig_flex and enable_tpu_gce, whose setMetadata cannot be name-scoped. enable_placement_policies now defaults to null, meaning the profile default: on for slurm-controller, because nodesets default to enable_placement = true and partitions to exclusive = true, and off for custom. The TPU and HA blueprints opt out explicitly because they use no placement, which keeps the TPU daily test's allowlist unchanged.
A deleted custom role can be undeleted only within 7 days, and its id cannot be reused for up to 44 days, so redeploying a destroyed deployment under the same deployment_name fails in between. The optional role_id_suffix input makes the generated role ids <deployment_name>.<name>.<role_id_suffix>[.scoped]. It must be 1-8 lowercase letters or digits and not "scoped", and the name length check counts it. The README's "Custom role lifecycle" section and each converted blueprint explain when to set it. metadata.yaml gains allowed_enum validators for custom_role_stage and custom_role_deletion_policy, and regex validators for the deployment_name and name format and their combined length, so these fail at gcluster create instead of terraform plan.
Integration tests that use community/modules/iam/service-account-profiles create custom roles per run. A killed run or failed destroy leaves them behind, counting toward the project's custom-role limit. process_custom_roles deletes only roles whose id has the module's <deployment_name>.<name>[.<role_id_suffix>][.scoped] shape and whose description starts with one the module sets. It skips roles matching the exclusion list, including active Cloud Build prefixes, with dashes compared as underscores. It runs in Phase 5 after service accounts.
No profile grants project-wide Cloud Storage access, so any account whose VMs run a modules/scripts/startup-script script needs that module's bucket_viewers on its staging bucket. Slurm nodes and the controller need this too, not only the Packer build VM. A new "Storage access" section says so, and says to use bucket_viewers, not bucket_bindings, for that bucket, because startup-script grants roles/storage.objectViewer authoritatively. Also explain why enable_tpu_gce needs compute.images.get and compute.subnetworks.get.
Redeploying an existing hpc-slurm-ha or hpc-enterprise-slurm deployment moved each account's logging.logWriter and monitoring.metricWriter grant from modules/project/service-account to the profile. Terraform created the profile's grant and then destroyed the account module's grant of the same binding in the same apply. Both are non-authoritative, so the later delete removed the binding and logging and metrics stopped silently. Add grant_baseline_roles (default true). The two converted core blueprints keep the baseline roles in the account blocks' project_roles and set grant_baseline_roles: false, so each binding keeps one owner. Document this in a new "Converting an existing deployment" README section, and quote the log line a missing bucket_viewers grant actually produces.
|
this PR has been inactive for 6 days and has no unresolved comments. @ep-nag, please review. |
|
This is a second reminder that this PR has been inactive for 7 days and has no unresolved comments. @ep-nag, please review. |
Resolve conflict in community/examples/hpc-slurm-ha.yaml: keep develop's vars and the service-account-profiles identities, and default homefs_size_gb and controller_fs_size_gb to 2560 (BASIC_SSD minimum).
|
Hi @ep-nag Could you please take a look at the pre-commit failures? |
|
@ep-nag, Could you please rebase and fix the pre-commit failures? |
|
/gcbrun |
AdarshK15
left a comment
AdarshK15
left a comment
There was a problem hiding this comment.
LGTM, Please resolve pre-commit failures.
|
this PR has been inactive for 2 days and has no unresolved comments. @ep-nag, please review. |
|
/gcbrun |
shubpal07
left a comment
shubpal07
left a comment
There was a problem hiding this comment.
LGTM overall. The permission profiles, scoping rules, and example updates are clean and well-tested. Left a few minor, non-blocking suggestions on variable validation when settings are placed in top-level vars:.
…e checks for top-level vars
shubpal07
left a comment
shubpal07
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing all the review comments!
What this does
Adds
community/modules/iam/service-account-profiles, which grants a named, least-privilegepermission profile to an existing service account. It replaces the broad predefined roles the
Slurm example blueprints grant today, such as the controller's project-wide
roles/compute.instanceAdmin.v1androles/iam.serviceAccountUser(the latter lets it impersonateevery service account in the project).
Account creation stays with the existing
modules/project/service-accountmodule; this module onlygrants permissions. Each identity is therefore two blocks:
modules/project/service-account(withproject_roles: []) creates the keyless account, and aservice-account-profilesblock withuse: [<account module id>]grants the profile's permissions to it.Profiles:
slurm-controllergets a custom role holding only the Compute Engine permissions the Slurmcontroller's resume, suspend and sync scripts call.
image-buildergets a custom role for the Packer build VM.customgets a custom role built from the permissions and existing roles you list.slurm-loginandslurm-computeget no custom role: only the baseline below, plus anyresource-scoped bindings you add.
Every profile also gets the
logging.logWriter/monitoring.metricWriterbaseline.Name-scoped
compute.instances.setMetadata: the controller and image builder need thispermission, but it is never granted project-wide. It is held in a second custom role, bound with an
IAM condition that matches only instances whose names start with
instance_name_prefixes. Thedefaults are the cluster's
<slurm_cluster_name>-and Packer'spacker-.Opt-in capability flags, accepted by
slurm-controller, each adding only the permissions forone feature:
enable_placement_policies: compact placement policies;enable_public_ips: nodes with external IPs;enable_bigquery_load: BigQuery job accounting;enable_tpu: Cloud TPU VM nodesets (schedmd-slurm-gcp-v6-nodeset-tpu);enable_mig: nodesets in a Terraform-created MIG (provisioning_engine = "MIG");enable_mig_flex: DWS Flex nodesets;enable_tpu_gce: TPU machine types on Compute Engine (ct*/tpu*in a regular nodeset).The three MIG-based flags (
enable_mig,enable_mig_flexandenable_tpu_gce) grantsetMetadataproject-wide, because IAM Conditions cannot scope an instance group manager. TheREADME warns about this, and the
customprofile refuses those three; it accepts the rest.roles/iam.serviceAccountUseron individual accounts only: the controller gets it on eachcompute account it attaches to nodes, and the image builder on itself. Never project-wide, and only
on accounts in the same project.
Resource-scoped bindings: grant a role on one bucket, secret, Artifact Registry repository,
BigQuery dataset, Pub/Sub topic or subscription, or subnetwork (for Shared VPC), instead of
project-wide.
Plan-time input checks:
custom_permissionsrefuses permissions that allow escalation: impersonation, key management,any
*.setIamPolicyexceptpubsub.subscriptions.setIamPolicy, secret and KMS access, and VMlogin or metadata.
roles/iam.*/roles/resourcemanager.*roles.Lifecycle inputs:
custom_role_stageandcustom_role_deletion_policyfor the generatedroles, and
iam_propagation_wait, a pause before theservice_account_emailoutput is availableso the grants have time to take effect.
Files touched
community/modules/iam/service-account-profiles/:main.tf,variables.tf,outputs.tf,versions.tf,metadata.yamlandREADME.md.community/examples/hpc-slurm-service-account-profiles.yaml(new): the reference example, withtwo compute nodesets, each with its own compute account.
examples/hpc-enterprise-slurm.yaml: converted from broad predefined roles.community/examples/hpc-slurm-ha.yaml: converted. Both HA controllers share one identity. ItsFilestore
size_gbwas raised to theBASIC_SSDminimum (2560); the blueprint could not deploywithout it.
community/examples/hpc-slurm6-tpu.yaml: converted, withenable_tpu, a dedicated account forthe TPU nodes, and the controller's actAs on that account.
examples/image-builder.yaml: converted, with theimage-builderprofile on the Packer build VMand separate controller, login and compute identities for the cluster.
.../slurm_files/scripts/resume.pyandtests/test_resume.py(see"Fix outside the module" below).
modules/README.mdandexamples/README.mdlist the module and the new example.tools/validate_configs/test_configs/service-account-profiles.yamlexercises every profile andcapability flag; its README describes it.
tools/cloud-build/daily-tests/{builds,tests}/hpc-slurm-service-account-profiles.*, theIAM-assertion playbook
ansible_playbooks/test-validation/test-service-account-profiles.yml, andits Kueue test lock.
hpc-enterprise-slurm,slurm-gcp-v6-tpuandpackerdaily-test build files gain themodule's tags, so those tests run when the module changes. The TPU daily test also runs the
IAM-assertion playbook.
How the permission set was derived
By deploying real clusters, reading
resume.log/slurmsync.log/ audit logs, and tightening therole until nothing extra remained; each permission traces to an observed API call or to Google's
per-method documentation. Notable finds:
bulkInsert(how the controller creates nodes) needscompute.instances.setMetadataon each newinstance, even though the controller sets no metadata, and whether or not the instance template
carries metadata. That is why the controller has it, name-scoped. The same is true of
setLabels,setServiceAccount,setTagsanddisks.setLabels.compute.zoneOperations.list,globalOperations.listandregionOperations.listare needed byresume.py's failure diagnosis andrepair.py's operation polling.slurmsyncswallows theexception, so missing them fails silently.
compute.resourcePolicies.list/.deleteare in the base controller role, becauseslurmsynccleans up placement policies on every cycle, whether or not placement is used.
compute.images.useReadOnlyis needed to create nodes from a custom image in the project.compute.instanceGroups.deleteis needed byenable_mig_flexandenable_tpu_gce: deleting a MIGalso deletes its instance group, which is checked separately.
compute.instanceGroups.createisneeded by
enable_tpu_gcefor the zonal MIG it creates per TPU slice.bigquery.jobs.createandtpu.nodes.updateare never called, so they are not granted.What's been tested
make testspasses, includingvalidate_configsfor all six blueprints that use the module.pre-commitpasses on every changed file.denylist entry and rejected input combination.
permission allowlist, baseline roles, role stage, actAs scope and scoped condition. Jobs on each
nodeset run under that nodeset's own compute account. A 2-node job runs with a placement policy
the controller creates.
setMetadata:setMetadataon an instance whose name matches the prefixsucceeds, and on one that doesn't is denied (403).
bulkInsertof a non-matching name is denied, whether or not the template carries metadata.restoring it restores creation.
from the resulting image.
enable_mig: a static MIG nodeset starts and runs jobs. Without the flag, the controller'scalls to list the MIG and delete its instances fail with a 403.
enable_mig_flex: a DWS Flex job runs in a per-job MIG that the controller creates anddeletes.
enable_tpu: the controller's TPU node create is authorised and reaches provisioning, thenstops on a capacity error (
429), not a permission error.enable_tpu_gce: the controller creates the TPU slice's workload policy, instance templateand MIG, and requests the resize, with no permission errors. The node then stops on TPU capacity
(
ZONE_RESOURCE_POOL_EXHAUSTED).slurmctldstopped, the backupcreates new nodes and runs jobs, with no extra permissions.
service-account-profiles.yamlread back exactly asconfigured, including every generated role's permissions and all seven binding types.
performMaintenanceandcontroller_network_attachmenthave nocapability flag. Each fails with a clear 403, as the README documents.
Fix outside the module
resume.py's placement-policy error handler unpacked each failed result as a(request, error)tuple, but the value is the bare
HttpError. A denied placement-policy create (for example, acontroller without
compute.resourcePolicies.create) therefore crashed resume with aTypeError,and the missing permission was never logged. This is a one-line fix with a regression test. Now
resume.lognames the denied permission, which is how a least-privilege controller's missingpermissions are diagnosed.
Gaps, maintainer decisions and future work
Gaps
enable_tpuis verified up to node creation; booting a node is blocked by TPU capacity (429).enable_tpu_gceis verified up to MIG creation and resize; booting a node is blocked by TPUcapacity. On-demand (dynamic) TPU-on-GCE jobs also need a slurm-gcp image with Slurm 26.05 or
later (current images ship 25.11).
Maintainer decisions
develop;on pull requests it runs only if a maintainer approves it (
auto_approved_pr_tests = []). So alater PR that makes the controller call a new API would be caught by the nightly run after merge,
not before. Adding this test to
auto_approved_pr_testswould catch it before merge, at the costof a cluster deployment per PR.
hpc-enterprise-slurm.yamlis a core-badged example that now depends on thiscommunity/experimental module. Maintainers may want to keep the badge, re-badge the example, or
promote the module.
Deferred work
are available.
controller_network_attachment. The permissions each needs are identified from source but havenot been proven against a working node, so no flag was added.
project-wide roles. The
customprofile with resource-scoped bindings already covers its webidentity (see the README example), but moving OFE itself onto the module is separate work.