Skip to content

fix: error when importing keys #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: error when importing keys #175

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee8090a
fix error when importing keys
haydenjohansenbt Jul 10, 2025
964b2e9
Merge branch 'main' into chore--Fix-error-when-importing-keys
bharathkkb Aug 26, 2025
083cf1d
modified indexing to use key name instead of numeric
haydenjohansenbt Sep 2, 2025
642e8e4
Merge branch 'main' into chore--Fix-error-when-importing-keys
hjoh95 Oct 1, 2025
7f4a0ba
Merge branch 'main' into chore--Fix-error-when-importing-keys
hjoh95 Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

locals {
keys_by_name = zipmap(var.keys, var.prevent_destroy ? slice(google_kms_crypto_key.key[*].id, 0, length(var.keys)) : slice(google_kms_crypto_key.key_ephemeral[*].id, 0, length(var.keys)))
keys_by_name = zipmap(var.keys, [for k in var.keys : "${google_kms_key_ring.key_ring.id}/cryptoKeys/${k}"])
Copy link
Member

@bharathkkb bharathkkb Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling prevent_destroy uses a different resource google_kms_crypto_key.key_ephemeral as opposed to google_kms_key_ring.key_ring so we would still need the conditional.

Copy link
Author

@hjoh95 hjoh95 Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the google_kms_crypto_key.key_ephemeral resource instead of the google_kms_crypto_key.key resource, but both use the same google_kms_key_ring.key_ring. The conditional was there to select which key resource to reference. I switched the keys_by_name local to set the values to the resultant resource id of the key (because these are always keyring/cryptoKeys/keyname instead of attempting to slice that out of the key resource. This should eliminate the need for that conditional.

Copy link
Author

@hjoh95 hjoh95 Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked Gemini to explain the caveats for both methods:

My change:

  • This method assumes that the resource IDs follow a predictable pattern.
  • If a key does not exist, this will still generate an ID string, but it won’t correspond to a real resource.
  • No validation that the constructed IDs are valid or exist.

The previous structure:

  • Assumes the resources are created in the same order as var.keys. If the resource creation order doesn’t match, the mapping may be incorrect.
  • If the number of resources is less than length(var.keys), you may get errors or missing mappings.
  • If resources are created dynamically or filtered, the mapping may not be reliable.
  • More robust than the first method, as it uses actual resource IDs, but still depends on ordering.

I'm not sure I agree with the "more robust" claim because of the import issue, but I will concede that my method assumes the resource id instead of pulling it from the resource. I am struggling to think of a scenario where this would be detrimental though.

}

resource "google_kms_key_ring" "key_ring" {
Expand All @@ -25,8 +25,8 @@ resource "google_kms_key_ring" "key_ring" {
}

resource "google_kms_crypto_key" "key" {
count = var.prevent_destroy ? length(var.keys) : 0
name = var.keys[count.index]
for_each = var.prevent_destroy ? { for key in var.keys : key => key } : {}
name = each.key
key_ring = google_kms_key_ring.key_ring.id
rotation_period = var.key_rotation_period
purpose = var.purpose
Expand All @@ -49,8 +49,8 @@ resource "google_kms_crypto_key" "key" {
}

resource "google_kms_crypto_key" "key_ephemeral" {
count = var.prevent_destroy ? 0 : length(var.keys)
name = var.keys[count.index]
for_each = var.prevent_destroy ? {} : { for key in var.keys : key => key }
name = each.key
key_ring = google_kms_key_ring.key_ring.id
rotation_period = var.key_rotation_period
purpose = var.purpose
Expand All @@ -73,22 +73,22 @@ resource "google_kms_crypto_key" "key_ephemeral" {
}

resource "google_kms_crypto_key_iam_binding" "owners" {
count = length(var.set_owners_for)
for_each = toset(var.set_owners_for)
role = "roles/owner"
crypto_key_id = local.keys_by_name[var.set_owners_for[count.index]]
members = compact(split(",", var.owners[count.index]))
crypto_key_id = local.keys_by_name[each.key]
members = compact(split(",", var.owners[index(var.set_owners_for, each.key)]))
}

resource "google_kms_crypto_key_iam_binding" "decrypters" {
count = length(var.set_decrypters_for)
for_each = toset(var.set_decrypters_for)
role = "roles/cloudkms.cryptoKeyDecrypter"
crypto_key_id = local.keys_by_name[var.set_decrypters_for[count.index]]
members = compact(split(",", var.decrypters[count.index]))
crypto_key_id = local.keys_by_name[each.key]
members = compact(split(",", var.owners[index(var.decrypters, each.key)]))
}

resource "google_kms_crypto_key_iam_binding" "encrypters" {
count = length(var.set_encrypters_for)
for_each = toset(var.set_encrypters_for)
role = "roles/cloudkms.cryptoKeyEncrypter"
crypto_key_id = local.keys_by_name[element(var.set_encrypters_for, count.index)]
members = compact(split(",", var.encrypters[count.index]))
crypto_key_id = local.keys_by_name[each.key]
members = compact(split(",", var.owners[index(var.encrypters, each.key)]))
}