Add Customer Managed Encryption Key (CMEK) support to Cluster Toolkit - #6143
Open
ep-nag wants to merge 17 commits into
Open
Add Customer Managed Encryption Key (CMEK) support to Cluster Toolkit#6143ep-nag wants to merge 17 commits into
ep-nag wants to merge 17 commits into
Conversation
Three modules for encrypting Cluster Toolkit resources with a
customer-managed key:
kms-key creates a CryptoKey, in a key ring it creates or
one that already exists
kms-key-iam grants the service agents that encrypt with a key
pre-existing-kms-key looks up a key the blueprint does not own
Creating and granting are separate because the grant is what consumers
must depend on. A CryptoKey id is available as soon as the key exists,
which is before any service agent can use it, so a resource wired to the
creating module can be created first and fail with a KMS
PERMISSION_DENIED that depends only on how Terraform scheduled the two.
kms-key-iam re-exports the id behind its grants, which also reverses the
order on destroy so an encrypted resource is removed before the grant it
relies on.
Each output is named after the input variable it feeds --
kms_key_name for Filestore, disk_encryption_key for the Slurm instance
modules, slurm_bucket_kms_key for the controller's bucket -- because
`use` matches an output name to an input name exactly. Wiring a consumer
to the creating module instead is a loud failure rather than a silent
loss of ordering: kms-key publishes no output matching any consumer's
input, so `use` matches nothing and test_module_not_used rejects the
blueprint.
Service-agent addresses are derived from the project number via a
google_project data source, following the pattern in bigquery-sub, so a
blueprint says `service_agents: [compute, storage]` rather than carrying
service-PROJECT_NUMBER@... strings. Explicit principals remain available
for agents in another project, which cannot be derived.
The CryptoKey defaults to deletion_policy = "ABANDON", so teardown never
destroys key material: data encrypted with a key routinely outlives the
deployment that created it, and destroying versions cannot be undone.
pre-existing-kms-key is stronger still -- the key is never in Terraform
state, so destroy cannot touch it.
Adds a customer-managed encryption key input to the modules a CMEK cluster needs but which had no way to take one: modules/file-system/filestore kms_key_name modules/packer/custom-image image_encryption_key community/modules/container/artifact-registry community/modules/database/slurm-cloudsql-federation Filestore gains a precondition restricting CMEK to the ZONAL, REGIONAL and ENTERPRISE tiers. The BASIC tiers do not support it at all, so a key set on one is a mistake worth reporting at plan time rather than silently dropping. Cloud SQL and Artifact Registry also encrypt the Secret Manager secret holding their credentials, so both take a key for the secret as well as for the instance or repository. The packer golden-copy expectations move with the custom-image change.
Two blueprints, both complete Slurm clusters. kms-key.yaml is the single-key case: one CryptoKey encrypting the Filestore /home, the controller, login and compute boot disks, an additional scratch disk, and the Slurm configuration bucket. Everything is wired with `use`, so no key name is written out by hand. kms-key-per-service.yaml is the same cluster with one key per class of resource, each granted only to the agent that uses it, so compromising one agent does not expose data belonging to another. It also shows adopting a key created outside the blueprint, and overriding the encryption identity with disk_encryption_key_service_account. The additional_disks entries name their key explicitly. `use` matches a module output to an input variable, and additional_disks is a caller-supplied list of objects with no input to bind to, so the boot disk is covered automatically and an additional disk would otherwise fall back to Google-managed encryption without saying so. Both were applied and destroyed on real infrastructure. The per-service cluster confirmed the three keys were granted to disjoint sets of agents, that a dynamically created compute node carried both its boot and scratch disks on the disk key, and that the adopted key was left untouched by destroy.
…liases The kms-key, kms-key-iam and pre-existing-kms-key modules all declare cloudkms.googleapis.com, but neither example enabled it, so a project that had never used Cloud KMS failed test_apis_enabled before apply. Add a service-enablement module to both, with the validator skip that makes it reachable -- test_apis_enabled runs at expansion time, before any module applies, so without the skip the module never gets to run. Add repository_kms_key_name and encryption_key_name outputs to kms-key-iam so `use` wires artifact-registry and slurm-cloudsql-federation the way it already wires Filestore, disks, the Slurm bucket and images. Both carry the same depends_on as the other outputs, so consumers are ordered behind the grants rather than racing them. Outputs are deliberately not added for boot_disk_kms_key (GKE), kms_key (managed-lustre) or disk_encryption_kms_key (gke-storage): kms-key-iam has no service agent for those, so wiring a key without a grantable agent would produce exactly the silent PERMISSION_DENIED the two-module split exists to prevent.
Paginate the Cloud KMS listing with list_next: a project with more than one page of key rings, or a ring with more than one page of keys, silently lost the remainder, and a missing suggestion is indistinguishable from a key that does not exist. Accept HIGH_SCALE_SSD wherever CMEK is allowed. It is Google's legacy name for ZONAL and the same tier underneath, so matching on the literal string blocked callers using the older name -- including OFE, which offers HIGH_SCALE_SSD rather than ZONAL -- from any CMEK Filestore below the far more expensive ENTERPRISE tier. Fixed in the module precondition, not just in OFE, since the module is where the rule lives. Validate rotation_period and destroy_scheduled_duration at plan time. Both are duration strings the API constrains, and destroy_scheduled_duration is immutable, so a bad value applied by mistake cannot be corrected in place. Detect the test environment with settings.TESTING rather than sys.argv, so pytest and coverage runners are covered too; the argv check stays as a fallback for `manage.py test` run without --settings=website.test_settings, which would otherwise fail on a missing configuration.yaml. Guard normalize_location against a missing location, cast the credentials Path to str before putting it in a subprocess environment, and drop a redundant json round-trip over a value that was already a dict. The last two are pre-existing issues in files this change already touches. Drop the migrations rule from the committed .gitignore: generating Django migrations at deploy time is a real problem, but it predates this change and fixing it means altering OFE's deployment model. The local exclude keeps them out of a working tree without asserting the practice here. Make every credentials env value a str, not just the one in _run_ghpc: _run_ghpc_import_inputs and the three extra_env dicts passed to run_terraform/run_packer all reached subprocess env as Path objects. Python coerces os.PathLike env values, so this was inconsistency rather than a defect, but the file already used .as_posix() elsewhere. Fix the Filestore CMEK remediation text, which still told users to move to ENTERPRISE after HIGH_SCALE_SSD became acceptable -- advice to pay for a more expensive tier than they needed. Add a test that parses the message and asserts every tier it names is actually allowed
The per-service example and the pre-existing-kms-key README named a key ring and key that exist in a real test project, so a reader who forgot to substitute them could resolve against live resources and encrypt with someone else's key instead of failing at plan time. Use the my-* form the neighbouring hpc-slurm-kms.yaml already uses; names that cannot accidentally exist.
…MBER rather than a numeric project number, and my-keyring/my-key rather than four spellings of the same thing, matching hpc-slurm-kms.yaml.
A malformed key_ring_id aborted the plan instead of failing validation.
"my-keyring" or "projects/p" has fewer than four slash-separated segments,
so indexing [1] or [3] raised an evaluation error before Terraform printed
any validation message -- the caller never saw the format error that would
have told them what was wrong. Index through try() so a short id yields ""
and falls through to a clean message, with the format error reported first.
This also removes the coalesce() fallbacks, which existed only to keep
those indexes in range.
A CryptoKey whose only version was destroyed comes back with
"primary": null, and .get("primary", {}) returns None for an explicit null
because a default only applies to a missing key -- so reading .state off it
raised AttributeError and the whole listing failed. Use `or {}`.
A non-numeric credential id reached the ORM, where the field conversion
raises ValueError rather than the DoesNotExist that get_object_or_404
catches, surfacing as an unhandled 500. Reject it with a 400 before the
lookup. A 400 rather than an empty list: a 200 saying "no keys" would be
untrue when the parameter was never a valid id, and the datalist treats any
non-2xx the same way.
Adds KmsKeyListingRobustnessTests covering all three, including that a
numeric id still reaches the lookup and queries both the region and
"global".
terraform-readme and packer-readme had drifted from their modules' variable definitions: artifact-registry, slurm-cloudsql-federation, filestore and custom-image all gained a CMEK input in an earlier commit without a doc regeneration. Also fixes an escaping inconsistency in kms-key-iam's generated table and two missing trailing newlines.
…ardown kms-key's deletion_policy defaulted to ABANDON, so terraform destroy always left a generated key's CryptoKeyVersion enabled in Cloud KMS regardless of destroy_scheduled_duration -- that setting existed on the resource but never took effect, since ABANDON removes the CryptoKey from Terraform state without calling the API at all. A key this module created is this deployment's to own, so its teardown should destroy the key material with it by default, not strand it enabled forever. Flip the default to DELETE, matching the google_kms_crypto_key resource's own provider default. terraform destroy now schedules the CryptoKeyVersion for destruction (confirmed live: ENABLED -> DESTROY_SCHEDULED, destroyTime set destroy_scheduled_duration out). Set deletion_policy = ABANDON on a key whose data must outlive its deployment. pre-existing-kms-key needs no change and is unaffected either way: it only reads a key via data sources and never creates a google_kms_crypto_key resource, so it has nothing for `terraform destroy` to act on regardless of this module's deletion_policy (confirmed live: terraform destroy reports nothing to destroy, key stays ENABLED). Neither setting frees the CryptoKey name or key ring -- Cloud KMS never deletes either at the API level -- so redeploying under the same names still requires a fresh key_name/key_ring_name or deployment_name either way. Updates kms-key's and pre-existing-kms-key's READMEs and the two CMEK example blueprints' teardown notes to match the new default.
…ADMEs Add a Testing section to kms-key, pre-existing-kms-key and kms-key-iam covering what was actually verified for each: static terraform validate, and live-GCP results specific to that module (grant placement, consumer wiring, lockout/recovery behavior, and the kms-key deletion_policy default of DELETE vs pre-existing-kms-key's unconditional no-touch guarantee). Keeps the evidence next to the code it documents rather than only in the PR description.
…ADMEs Add a Testing section to kms-key, pre-existing-kms-key and kms-key-iam, each showing the actual gcloud/ghpc commands and output used to confirm the module does what it claims, rather than prose describing testing that happened elsewhere: - kms-key: gcloud kms keys versions list before/after `terraform destroy` under both deletion_policy values, showing DESTROY_SCHEDULED vs ENABLED - pre-existing-kms-key: deploy/destroy against an out-of-band key showing both report nothing to do, terraform state list showing only data sources, and the key still ENABLED afterward regardless of any kms-key module's deletion_policy elsewhere in the blueprint - kms-key-iam: gcloud kms keys get-iam-policy showing the compute/storage/ filestore service-agent grants actually landed Each section covers only what that module is responsible for and links to the others rather than repeating the same evidence three times. All command/resource names are placeholders (PROJECT_ID, KEYRING, KEY, etc.), matching the placeholder convention already used elsewhere in these READMEs.
Contributor
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 adds comprehensive support for Customer Managed Encryption Keys (CMEK) to the Cluster Toolkit. By introducing dedicated security modules for key management and IAM, users can now encrypt various infrastructure components—including disks, buckets, registries, and databases—using their own keys. This change enhances security posture and provides better control over data encryption, while ensuring proper ordering of resource creation and IAM grants to prevent race conditions. 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces new security modules (kms-key, kms-key-iam, and pre-existing-kms-key) to support Customer-Managed Encryption Keys (CMEK) across various Cluster Toolkit components, including Slurm clusters, Filestore, Artifact Registry, Cloud SQL, and Packer custom images. The review feedback highlights potential plan-time evaluation issues in the new modules. Specifically, it recommends using try() when indexing primary[0] in outputs to prevent crashes when values are computed or empty, and suggests using conditional operators in variable validation blocks to avoid plan-time crashes caused by the lack of short-circuit evaluation in Terraform when parsing duration strings.
| # initial version: Cloud KMS leaves `primary` unset for other purposes, which | ||
| # would make this fail with an index error. If a `purpose` input is ever | ||
| # added, this must become a conditional (for example a one() or try()). | ||
| value = google_kms_crypto_key.this.primary[0].name |
Contributor
There was a problem hiding this comment.
Directly indexing primary[0] can cause evaluation errors during terraform plan if the list is empty or computed (not yet known). It is safer to use try() to handle cases where the primary version is not yet populated or known.
value = try(google_kms_crypto_key.this.primary[0].name, null)
References
- To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.
Author
There was a problem hiding this comment.
primary is (known after apply) at plan time (a single unknown value, not an empty list), so indexing [0] never crashes — confirmed via terraform plan and dozens of successful live deploys this session. purpose is hardcoded to ENCRYPT_DECRYPT here, so primary is always populated; try(..., null) would just hide it if that ever changed, rather than failing loudly at the exact spot that needs updating.
| # Indexing [0] is safe because the postcondition in main.tf rejects any | ||
| # key that is not ENCRYPT_DECRYPT, and Cloud KMS always reports a | ||
| # primary version for those. | ||
| value = data.google_kms_crypto_key.this.primary[0].name |
Contributor
There was a problem hiding this comment.
Directly indexing primary[0] can cause evaluation errors during terraform plan if the data source is not yet read (e.g., if the key name or ring name is computed). It is safer to use try() to handle cases where the primary version is not yet populated or known.
value = try(data.google_kms_crypto_key.this.primary[0].name, null)
References
- To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.
Author
There was a problem hiding this comment.
Same result empirically (terraform plan shows the attribute as a single unknown value, not an empty list, so [0] can't crash), and here it's backed by an actual enforced postcondition in main.tf (self.purpose == "ENCRYPT_DECRYPT"), not just a comment. try(..., null) would mask a postcondition failure that should surface immediately instead.
| # seconds-suffixed duration. Checking here names the variable; failing in | ||
| # the API names the resource. | ||
| validation { | ||
| condition = can(regex("^[0-9]+s$", var.rotation_period)) && tonumber(trimsuffix(var.rotation_period, "s")) >= 86400 |
Contributor
There was a problem hiding this comment.
Terraform does not guarantee short-circuit evaluation for validation conditions. If var.rotation_period is invalid and does not end with "s", trimsuffix will not remove "s", and tonumber will fail with a fatal error, crashing the plan instead of showing the validation error message. Using a conditional operator ? : ensures that tonumber is only evaluated when the regex matches.
condition = can(regex("^[0-9]+s$", var.rotation_period)) ? tonumber(trimsuffix(var.rotation_period, "s")) >= 86400 : false
Author
There was a problem hiding this comment.
Tested this directly with the exact adversarial input (abcs — ends in "s", non-numeric prefix) on Terraform v1.15.8, within this module's required >= 1.12.2: it produces the clean validation message, not a crash. HCL's && short-circuits correctly on this toolchain, so this doesn't reproduce here.
Comment on lines
+167
to
+170
| condition = can(regex("^[0-9]+s$", var.destroy_scheduled_duration)) && ( | ||
| tonumber(trimsuffix(var.destroy_scheduled_duration, "s")) >= 86400 && | ||
| tonumber(trimsuffix(var.destroy_scheduled_duration, "s")) <= 10368000 | ||
| ) |
Contributor
There was a problem hiding this comment.
Terraform does not guarantee short-circuit evaluation for validation conditions. If var.destroy_scheduled_duration is invalid and does not end with "s", trimsuffix will not remove "s", and tonumber will fail with a fatal error, crashing the plan instead of showing the validation error message. Using a conditional operator ? : ensures that tonumber is only evaluated when the regex matches.
condition = can(regex("^[0-9]+s$", var.destroy_scheduled_duration)) ? (
tonumber(trimsuffix(var.destroy_scheduled_duration, "s")) >= 86400 &&
tonumber(trimsuffix(var.destroy_scheduled_duration, "s")) <= 10368000
) : false
Author
There was a problem hiding this comment.
Same test, same result — the identical &&/tonumber() pattern here was verified against the same adversarial-input case and shows the clean validation message on Terraform v1.15.8, no crash. This is the same non-short-circuiting concern as the rotation_period comment, and doesn't reproduce on the version this module actually targets.
Contributor
|
/gcbrun |
arpit974
self-requested a review
August 17, 2026 03:28
arpit974
reviewed
Aug 17, 2026
arpit974
reviewed
Aug 17, 2026
| depends_on = [google_kms_crypto_key_iam_member.this] | ||
| } | ||
|
|
||
| output "slurm_bucket_kms_key" { |
Contributor
There was a problem hiding this comment.
By broadcasting the same key under specific variable names (disk_encryption_key, slurm_bucket_kms_key, etc.), this IAM module becomes tightly coupled to every downstream module that consumes a key.
I understand this is required to make the Cluster Toolkit use: auto-wiring function correctly without forcing the user to do manual variable mapping. Since we are accepting this coupling as a design pattern for better UX, please add a dedicated section to this module's README.md. It needs to explicitly document that whenever a new module is added to the toolkit that requires a KMS key, this outputs.tf file must be updated to include that new variable name.
arpit974
reviewed
Aug 17, 2026
| nullable = false | ||
| } | ||
|
|
||
| variable "service_agents" { |
Contributor
There was a problem hiding this comment.
This IAM module is currently missing support for two strict enterprise scenarios involving custom service accounts and external keys. We need to add a couple of variables to handle them and maintain feature parity with PR #5407.
- Support for Custom Service Accounts PR Support Customer-Managed Encryption Keys (CMEK) in Slurm GCP deployments #5407 introduced disk_encryption_key_service_account so users could provide a custom SA for encryption rather than relying on the default Compute Engine service agent. This module currently seems to only derive and grant IAM roles to the default Google-managed service agents.
Suggestion: Add an input (e.g., custom_service_accounts as a list of strings) so this module can also attach the cryptoKeyEncrypterDecrypter role to user-specified custom SAs.
- The "Pass-Through" Toggle for Strict Environments When a strict central security team provisions an external KMS key (via pre-existing-kms-key) and a custom SA, they often configure the IAM permissions out-of-band. The cluster operator running Cluster Toolkit has no cloudkms.admin rights on that key. If that operator tries to use this kms-key-iam module just to get the benefit of the clean use: auto-wiring outputs in their YAML, Terraform will force the creation of IAM bindings and crash with a 403 Permission Denied because they lack admin rights.
Suggestion:: Add a skip_iam_role_grants = true variable. This will allow users in strict environments to bypass the IAM attachment code entirely, avoiding 403 errors, while still letting the module broadcast the aliased output variables so their blueprint auto-wiring continues to work.
…vice_accounts Upstream review (PR GoogleCloudPlatform#180, comment on kms-key-iam/variables.tf) pointed out that a custom/user-managed service account is a distinct, common case service_agents can't derive, and asked for a dedicated input rather than overloading a generic "any principal string" variable for it. In practice service_agent_principals was only ever used for custom SAs, so replace it outright rather than add a second variable alongside it. custom_service_accounts takes bare SA emails (no "serviceAccount:" prefix -- the module adds it) instead of fully-qualified principal strings, with validation rejecting a mistakenly-prefixed value and requiring a ".gserviceaccount.com" address. Unioned with service_agents exactly as service_agent_principals was; the underlying google_kms_crypto_key_iam_member for_each loop is unchanged. Updates the one caller (community/examples/kms-key-per-service.yaml, now using service_account_email instead of service_account_iam_email) and the example blocks in all three module READMEs that had been using service_agent_principals to spell out a derived agent by hand instead of using service_agents, which existed for exactly that case. Live-verified against a real GCP project, replacing the two scenarios this was meant to address: - a single custom SA granted with no service_agents at all, confirmed via IAM policy and disk kmsKeyServiceAccount on the controller, login and a dynamically-provisioned compute node, plus a real Slurm job on the latter - two custom SAs granted on one key and pointed at two different resources (controller+login on one, the nodeset on the other), confirming each disk used the SA it was actually assigned rather than either being a catch-all Both scenarios reproduced independently. terraform validate, make tests and pre-commit run --all-files all pass with no new failures beyond this sandbox's pre-existing environment-tooling gaps (terraform-docs, addlicense, goimports, gocyclo not installed). Documents both scenarios with real gcloud command/output evidence in kms-key-iam's README, matching the existing per-module Testing section convention.
Upstream review (PR GoogleCloudPlatform#180) flagged that defaulting deletion_policy to DELETE is dangerous for anyone who copies a blueprint without reading the variable docs: running terraform destroy on what they thought was just tearing down compute would also cryptographically shred their key's data, silently. Google's guidance on the thread: make the field mandatory so Terraform fails without it, and document ABANDON as the recommended choice for most blueprints -- rather than picking either value silently on the caller's behalf. Drop the "DELETE" default from kms-key's deletion_policy variable (already nullable = false, so omitting it now fails validation instead of falling back). Rewrite the variable description, the main.tf comment, and the "Lifecycle and naming" section of kms-key's README to lead with "required, no default" and explain why ABANDON is recommended over DELETE rather than asserting either as the default. Fix pre-existing-kms-key's README cross-reference, which described the old default. Update both example blueprints (kms-key.yaml, kms-key-per-service.yaml), which relied on the removed default, to set deletion_policy: ABANDON explicitly on every kms-key module instance, with a comment on why; their teardown-note comments are rewritten to match. Live-verified: omitting deletion_policy now fails at `ghpc create` time, before Terraform is invoked at all, surfacing the full ABANDON/DELETE guidance directly in the error rather than sending the caller to the README. Both example blueprints re-confirmed to expand cleanly with deletion_policy set. DELETE's and ABANDON's actual teardown behavior are unchanged from prior testing (DESTROY_SCHEDULED vs. ENABLED respectively) -- this change only affects whether a value must be chosen, not what either value does. make tests and pre-commit run --all-files both pass, no new failures beyond this sandbox's pre-existing environment-tooling gaps (terraform-docs, addlicense, goimports, gocyclo not installed). Documents the required-field behavior with real command/output evidence in kms-key's README Testing section, matching the existing convention.
…missions Upstream review (PR GoogleCloudPlatform#180, comment 3b) identified a real gap: an operator adopting a key via pre-existing-kms-key whose IAM is managed out-of-band by a separate security team has no way to get kms-key-iam's output-aliasing convenience (use: auto-wiring to kms_key_name, disk_encryption_key, etc.) without Terraform also attempting to create the google_kms_crypto_key_iam_member grants itself -- which fails with a 403 if the identity running Terraform lacks cloudkms.admin/setIamPolicy on that key, exactly the point of "managed out-of-band." Add skip_iam_role_grants (bool, default false, matching the reviewer's suggested name): when true, gates google_kms_crypto_key_iam_member.this's for_each to an empty set, following this repo's existing `cond ? real : []` idiom for conditionally-created for_each resources (see schedmd-slurm-gcp-v6-controller/controller.tf). Every output still resolves crypto_key_id normally -- depends_on on a for_each resource is a graph-level dependency and resolves cleanly even at zero instances. Cross-variable validation rejects skip_iam_role_grants = true combined with a non-empty service_agents or custom_service_accounts, since either would otherwise look like a grant request that silently does nothing. Live-verified against a real GCP project: - skip_iam_role_grants = true with a non-empty service_agents fails terraform plan immediately with the new validation error -- confirmed zero resources are created, not even the key ring, since plan-level variable validation aborts before any resource is planned - a key created and granted entirely by hand (gcloud kms keys add-iam-policy-binding, simulating a security team managing permissions out-of-band), then deployed via pre-existing-kms-key -> kms-key-iam (skip_iam_role_grants = true) -> a Slurm controller and login node: terraform state list shows zero google_kms_crypto_key_iam_member resources, both disks correctly resolved and used the manually-granted key, and the login instance booted and was reachable terraform validate, make tests and pre-commit run --all-files all pass with no new failures beyond this sandbox's pre-existing environment- tooling gaps (terraform-docs, addlicense, goimports, gocyclo not installed) and one confirmed-unrelated, non-reproducing flaky module-fetch failure in make tests (transient HTTP 500 from an upstream git host, hitting a different unrelated example config on each retry). Documents the new variable with a "Out-of-band permissions" section and real command/output evidence in kms-key-iam's README, and cross-references it from pre-existing-kms-key's Requirements section.
…missions Upstream review (PR GoogleCloudPlatform#180, comment 3b) identified a real gap: an operator adopting a key via pre-existing-kms-key whose IAM is managed out-of-band by a separate security team has no way to get kms-key-iam's output-aliasing convenience (use: auto-wiring to kms_key_name, disk_encryption_key, etc.) without Terraform also attempting to create the google_kms_crypto_key_iam_member grants itself -- which fails with a 403 if the identity running Terraform lacks cloudkms.admin/setIamPolicy on that key, exactly the point of "managed out-of-band." Add skip_iam_role_grants (bool, default false, matching the reviewer's suggested name): when true, gates google_kms_crypto_key_iam_member.this's for_each to an empty set, following this repo's existing `cond ? real : []` idiom for conditionally-created for_each resources (see schedmd-slurm-gcp-v6-controller/controller.tf). Every output still resolves crypto_key_id normally -- depends_on on a for_each resource is a graph-level dependency and resolves cleanly even at zero instances. Cross-variable validation rejects skip_iam_role_grants = true combined with a non-empty service_agents or custom_service_accounts, since either would otherwise look like a grant request that silently does nothing. Live-verified against a real GCP project, including a negative control to rule out a false positive: - skip_iam_role_grants = true with a non-empty service_agents fails terraform plan immediately with the new validation error -- confirmed zero resources are created, not even the key ring - a key created and granted entirely by hand (gcloud kms keys add-iam-policy-binding, simulating a security team managing permissions out-of-band), then deployed via pre-existing-kms-key -> kms-key-iam (skip_iam_role_grants = true) -> a Slurm controller and login node: terraform state list shows zero google_kms_crypto_key_iam_member resources, both disks correctly resolved and used the manually-granted key, and the login instance booted and was reachable - negative control: removed the manual grant on that same key and forced a fresh decrypt via stop/start on the same controller instance -- the start API call itself was explicitly rejected (Permission 'cloudkms.cryptoKeyVersions.useToDecrypt' denied, citing the exact key), and the instance went to TERMINATED. Re-adding the identical binding immediately restored it. Same key, same instance, only the grant changed, confirming the earlier success was actually caused by that grant rather than some unrelated permission already present in the project. Reproduced independently by the user as well. terraform validate, make tests and pre-commit run --all-files all pass with no new failures beyond this sandbox's pre-existing environment- tooling gaps (terraform-docs, addlicense, goimports, gocyclo not installed) and one confirmed-unrelated, non-reproducing flaky module-fetch failure in make tests (transient HTTP 500 from an upstream git host, hitting a different unrelated example config on each retry). Documents the new variable with a "Out-of-band permissions" section and real command/output evidence -- including the negative control -- in kms-key-iam's README, and cross-references it from pre-existing-kms-key's Requirements section.
Author
Changes made in response to review
|
The entry still described the single-module design: it credited kms-key with making the grants and told authors to write `use: [kms_key]`, which does not merely fail to wire anything -- none of kms-key's output names (crypto_key_id, key_ring_id, primary_crypto_key_version_id) match a consumer's CMEK input, so gcluster rejects the blueprint with test_module_not_used. Only kms-key-iam's outputs match. The blueprint itself was already correct; only its index entry was left behind. Also drops filestore_service_agent/compute_service_agent/ storage_service_agent, settings that do not exist anywhere in this codebase -- the blueprint provisions service agents via gcloud comments and kms-key-iam derives each address from project_id, no manual address ever appears. Corrects the teardown paragraph, which claimed destroy retains key material unconditionally. deletion_policy is now required with no default; this blueprint chooses ABANDON, and DELETE does the opposite.
Contributor
|
Please NOTE : We recently upgraded our repository's Go version to 1.26 (#6173). We strongly recommend rebasing your open branches onto the latest develop branch to avoid or resolve any PR test failures. Thank you! |
arpit974
approved these changes
Aug 24, 2026
arpit974
left a comment
arpit974
left a comment
Contributor
There was a problem hiding this comment.
LGTM,
+1 to aslam,
please rebase this PR
arpit974
added
the
release-improvements
|
this PR has been inactive for 7 days and has no unresolved comments. @GoogleCloudPlatform/hpc-toolkit, please review. |
Contributor
|
/gcbrun |
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.
Add Cloud KMS (CMEK) support for customer-managed encryption
Summary
Adds Cluster Toolkit support for encrypting deployed resources with a customer-managed encryption key (CMEK) instead of Google-managed keys, via three new security modules plus wiring into the modules that can already accept a CMEK key.
What's new
Three new modules (
community/modules/security/):kms-key— creates a symmetric Cloud KMS CryptoKey, either in a new key ring or an existing one. Defaults todeletion_policy = "DELETE", matching the underlyinggoogle_kms_crypto_keyresource's own provider default:terraform destroyschedules the key's version(s) for destruction, making anything encrypted with them permanently unrecoverable. Setdeletion_policy = "ABANDON"for a key whose data must outlive the deployment. Neither setting frees the CryptoKey name or key ring — Cloud KMS never deletes either at the API level.pre-existing-kms-key— looks up a CryptoKey that already exists (e.g. provisioned by a security team, or in a dedicated key project) and publishes its id. Nothing is created, soterraform destroynever touches the key, regardless ofkms-key'sdeletion_policysetting elsewhere in the same blueprint.kms-key-iam— grantsroles/cloudkms.cryptoKeyEncrypterDecrypterto the service agents that need to use the key, and re-exports the key id ordered behind those grants under the input name each consumer expects (souse:wires it automatically). CMEK consumers shouldusethis module rather thankms-key/pre-existing-kms-keydirectly, to avoid a race between resource creation and the IAM grant.Wiring into existing modules:
modules/file-system/filestore— acceptskms_key_name(ZONAL/REGIONAL/ENTERPRISE tiers only)modules/packer/custom-image— acceptsimage_encryption_keycommunity/modules/container/artifact-registry— acceptsrepository_kms_key_namecommunity/modules/database/slurm-cloudsql-federation— acceptsencryption_key_namedisk_encryption_key/slurm_bucket_kms_key;kms-key-iamre-exports under those names tooTwo new example blueprints (
community/examples/):kms-key.yaml— one shared generated key for an entire Slurm cluster (controller, login, nodesets, Filestore)kms-key-per-service.yaml— one key per resource class (least privilege), including a Filestore key adopted viapre-existing-kms-keyto demonstrate the import pathTesting
Per-module testing detail now lives in each module's own README (
kms-key,pre-existing-kms-key,kms-key-iam).Static:
terraform validatepasses on all three new modulesmake testspasses — fullvalidate_configssweep including both new example blueprintspre-commit run --all-files— all substantive hooks pass (Terraform fmt/validate/tflint, pytest, mypy, go vet, golangci-lint, yamllint, shellcheck, codespell)Live GCP verification (trimmed full-Slurm-cluster blueprints, plus minimal standalone key deployments, against a real project):
gcloud compute disks describe)gcloud filestore instances describe)terraform state listconfirms it's held only as a data source, never a managed resourceDISABLED-state rejection on Compute Engine instance start (plus akmsKeyErrorsystem event forcingTERMINATED), FilestoreSUSPENDED, Cloud Storage read failed withKEY_DISABLEDdeletion_policy = "DELETE"(new default): generated key'sterraform destroyschedules it for destruction (DESTROY_SCHEDULED,destroyTimesetdestroy_scheduled_durationout) — reproduced twice, including an independent rundeletion_policy = "ABANDON":terraform destroyleaves the key ring, CryptoKey and every version intact andENABLEDterraform destroyregardless of the other module'sdeletion_policy— reproduced independently