-
Notifications
You must be signed in to change notification settings - Fork 148
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.
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
Feature/add cmek support 114 #115
Feature/add cmek support 114 #115
Conversation
Thanks for the PR! 🚀 |
main.tf
Outdated
resource "google_kms_key_ring" "key_ring" { | ||
count = var.encrypt_gcs_bucket_tfstate ? 1 : 0 | ||
name = "${var.project_prefix}-keyring" | ||
project = module.seed_project.project_id | ||
location = var.default_region | ||
} | ||
|
||
resource "google_kms_crypto_key" "gcs_key" { | ||
count = var.encrypt_gcs_bucket_tfstate ? 1 : 0 | ||
name = "${var.project_prefix}-key" | ||
key_ring = google_kms_key_ring.key_ring[0].self_link | ||
rotation_period = var.key_rotation_period | ||
|
||
lifecycle { | ||
prevent_destroy = false | ||
} | ||
|
||
version_template { | ||
algorithm = "GOOGLE_SYMMETRIC_ENCRYPTION" | ||
protection_level = var.key_protection_level | ||
} | ||
|
||
} | ||
|
||
resource "google_kms_crypto_key_iam_member" "gcs_key_iam" { | ||
count = var.encrypt_gcs_bucket_tfstate ? 1 : 0 | ||
crypto_key_id = google_kms_crypto_key.gcs_key[0].id | ||
role = "roles/cloudkms.cryptoKeyEncrypterDecrypter" | ||
member = "serviceAccount:${data.google_storage_project_service_account.gcs_account.email_address}" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use https://github.com/terraform-google-modules/terraform-google-kms?
module "kms" {
source = "terraform-google-modules/kms/google"
version = "~> 1.2"
project_id = module.seed_project.project_id
location = var.default_region
keyring = "${var.project_prefix}-keyring"
keys = [ "${var.project_prefix}-key"]
key_rotation_period = var.key_rotation_period
key_protection_level = var. key_protection_level
set_decrypters_for = ["${var.project_prefix}-key"]
set_encrypters_for = ["${var.project_prefix}-key"]
decrypters = [
"serviceAccount:${data.google_storage_project_service_account.gcs_account.email_address}",
]
encrypters = [
"serviceAccount:${data.google_storage_project_service_account.gcs_account.email_address}",
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. Leme update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/cc @rjerrems
main.tf
Outdated
dynamic "encryption" { | ||
for_each = var.encrypt_gcs_bucket_tfstate ? ["encryption"] : [] | ||
content { | ||
# default_kms_key_name = google_kms_crypto_key.gcs_key[0].id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Leme fix it.
/gcbrun |
variables.tf
Outdated
default = null | ||
} | ||
|
||
variable "prevent_destroy" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If this only applies to KMS, it should probably be prefixed with kms_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
LGTM, just one minor nit on variable naming. |
@imrannayer can you also please rebase from master as your other change has been merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imrannayer looks like there are changes from #116 in this PR. Could you rebase?
Thanks @imrannayer - I have created a follow up issue here #119 for add an example and tests |
Fixed #114