terraform-google-modules / terraform-google-kms

Allows managing a keyring, zero or more keys in the keyring, and IAM role bindings on individual keys
https://registry.terraform.io/modules/terraform-google-modules/kms/google
Apache License 2.0
45 stars 94 forks source link

KMS Module causes Build Pipeline to fail due to unmet dependencies #39

Open rutalreja-deloitte opened 3 years ago

rutalreja-deloitte commented 3 years ago

GCS-CMEK

When the above linked GCS-CMEK is run in the terraform-example-foundation Cloud Build pipeline, it creates Keyrings, Keys, and GCS buckets encrypted with the generated keys in multiple projects parallelly; this produces an error.

Error: googleapi: Error 403: Permission denied on Cloud KMS key. Please ensure that your Cloud Storage service account has been authorized to use this key., forbidden

Causality: The GCS module only requires the key as input, creating the implicit dependency only to the key self-link being available, ignoring the IAM bindings.

As such, Terraform tries to create the GCS bucket before the IAM binding for Role: Encrypter/Decrypter is linked to the KMS Key in many cases, the test even goes further after the failure point and completes the IAM Binding, thus leaving no trace that it was the root cause, if not looking closely at the output. It can only be detected by comparing the line numbers of successfully deployed buckets vs. unsuccessful buckets; in cases where the bucket randomly deploys after the binding has been set, it is successful and fails with the above error when the opposite occurs.

In the linked module, the above was resolved by adding an explicit dependency on the entire KMS module (Only possible in Terraform 0.13+) depends_on = [module.kms]

Suggested Improvement to KMS module until a better solution is ascertained

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

morgante commented 3 years ago

I think it would be a good idea to make the outputs in this module dependent on the IAM bindings.

bharathkkb commented 3 years ago

@daniel-cit This should let us remove the explicit module level depends on DWH, PTAL

meons commented 1 year ago

Same issue here