Skip to content

feat: import key handle on instance recreate #717

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

Closed
wants to merge 5 commits into from
Closed

feat: import key handle on instance recreate #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
391abc3
feat: import key handle on instance recreate
ps-occrp Apr 7, 2025
2e0cce8
Merge branch 'main' into autokey-import
imrannayer Apr 17, 2025
b5f03e0
chore: remove use of import
ps-occrp May 23, 2025
cff4748
Merge branch 'main' into autokey-import
ps-occrp May 26, 2025
a208351
Merge branch 'main' into autokey-import
ps-occrp Jun 16, 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
19 changes: 16 additions & 3 deletions modules/postgresql/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,13 @@ locals {

database_name = var.enable_default_db ? var.db_name : (length(var.additional_databases) > 0 ? var.additional_databases[0].name : "")

encryption_key = var.encryption_key_name != null ? var.encryption_key_name : var.use_autokey ? google_kms_key_handle.default[0].kms_key : null
encryption_key = var.encryption_key_name != null ? var.encryption_key_name : var.use_autokey ? local.autokey_kms_key : null
autokey_location = coalesce(var.region, join("-", slice(split("-", var.zone), 0, 2)))

// Get autokey kms key
existing_handle_key = var.use_existing_key_handle ? try([for handle in data.google_kms_key_handles.existing.key_handles : handle.kms_key if endswith(handle.name, "/${var.name}")][0], null) : null
create_handle = var.use_autokey && local.existing_handle_key == null
autokey_kms_key = local.create_handle ? google_kms_key_handle.default[0].kms_key : local.existing_handle_key
}

resource "random_id" "suffix" {
Expand Down Expand Up @@ -224,12 +230,19 @@ resource "google_sql_database_instance" "default" {
depends_on = [null_resource.module_depends_on]
}

data "google_kms_key_handles" "existing" {
Copy link
Collaborator

@imrannayer imrannayer Apr 8, 2025

Choose a reason for hiding this comment

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

@ps-occrp instead of searching and importing can we make google_kms_key_handle.name an optional variable (kms_key_handle_name) with default value null and also add create_kms_key_handle boolean variable which is true by default.

If use_autokey is true then:

Scenario1:
If user provides a kms_key_handle_name and create_kms_key_handle is false then we will assume it already exists and we will use google_kms_key_handle data source to find and use it.

Scenario2:
If kms_key_handle_name is provided and create_kms_key_handle is true then we will create key handle using that name

Scenario 3:
If kms_key_handle_name is not provided and create_kms_key_handle is true then we can just assume we need to create key handle using instance name

Copy link
Contributor Author

@ps-occrp ps-occrp Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@imrannayer In the current design you're proposing, the fact that google_kms_key_handle is not actually deleted or destroyed is exposed to the user—they need to be aware of this and act accordingly. While I understand and agree with this decision at the resource level, at the module level I personally prefer to abstract that behavior and keep it transparent to the user.

If you don’t agree with this approach, I’m happy to adopt the changes you suggested. However, if you do agree with the idea of keeping it transparent but would rather avoid using import, I believe I can implement a workaround to achieve that.

Copy link
Collaborator

@imrannayer imrannayer Apr 9, 2025

Choose a reason for hiding this comment

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

I am trying to keep non-standard behavior like import out of the modules. I think it will be a better to allow people to provide handle name. Majority of the customers use random suffix with database due to cloudSql limitation of reusing name within 7 days. Although that limitation is not there but there are lots of folks still deploying cloudsql with random suffix.
Also add this in the documentation and example documentation so users are aware of this limitation. It will be great if you can also add link to GCP/TF provider documentation with this detail.

provider = google-beta
project = var.project_id
location = local.autokey_location
resource_type_selector = "sqladmin.googleapis.com/Instance"
}

resource "google_kms_key_handle" "default" {
count = var.use_autokey ? 1 : 0
count = local.create_handle ? 1 : 0
provider = google-beta
project = var.project_id
name = local.instance_name
location = coalesce(var.region, join("-", slice(split("-", var.zone), 0, 2)))
location = local.autokey_location
resource_type_selector = "sqladmin.googleapis.com/Instance"
}

Expand Down
6 changes: 6 additions & 0 deletions modules/postgresql/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,3 +477,9 @@ variable "use_autokey" {
type = bool
default = false
}

variable "use_existing_key_handle" {
description = "kms_key_handle resource can not be delete from GCP (https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/kms_key_handle). If you recreate cloudsql instance with same name module will try to create kms_key_handle resource again. This will fail if you have existing key handle. Set this to true to use existing key handle with same name and fail. In that case make this variable true."
type = bool
default = false
}