Skip to content
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

Merged
merged 5 commits into from
Aug 24, 2021
Merged

Feature/add cmek support 114 #115

merged 5 commits into from
Aug 24, 2021

Conversation

imrannayer
Copy link
Contributor

Fixed #114

@comment-bot-dev
Copy link

comment-bot-dev commented Aug 9, 2021

Thanks for the PR! 🚀
✅ Lint checks have passed.

main.tf Outdated
Comment on lines 87 to 107
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}"
}
Copy link
Member

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}",
  ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. Leme update.

bharathkkb
bharathkkb previously approved these changes Aug 10, 2021
Copy link
Member

@bharathkkb bharathkkb left a 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
Copy link
Member

Choose a reason for hiding this comment

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

nit: comment

Copy link
Contributor Author

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.

@bharathkkb
Copy link
Member

/gcbrun

variables.tf Outdated
default = null
}

variable "prevent_destroy" {
Copy link
Contributor

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_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rjerrems
Copy link
Contributor

rjerrems commented Aug 23, 2021

LGTM, just one minor nit on variable naming.

rjerrems
rjerrems previously approved these changes Aug 23, 2021
@rjerrems
Copy link
Contributor

@imrannayer can you also please rebase from master as your other change has been merged.

Copy link
Member

@bharathkkb bharathkkb left a 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?

@rjerrems rjerrems merged commit 2fea4be into terraform-google-modules:master Aug 24, 2021
@rjerrems
Copy link
Contributor

Thanks @imrannayer - I have created a follow up issue here #119 for add an example and tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CMEK support for state file GCS bucket
4 participants