-
Notifications
You must be signed in to change notification settings - Fork 453
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.
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
base: main
Are you sure you want to change the base?
Conversation
@imrannayer what do you think about this? |
@@ -214,12 +217,25 @@ resource "google_sql_database_instance" "default" { | |||
depends_on = [null_resource.module_depends_on] | |||
} | |||
|
|||
data "google_kms_key_handles" "existing" { |
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.
@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
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 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.
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.
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.
@ps-occrp can u plz review my comments about not using import. |
@ps-occrp r u still working on this fix? Would be better to get this fix in before next release which will have autikey. |
Keyhandle resource cannot be deleted from project so when using autokey if you delete and recreate an instance you should import older keyhandle. This PR implements this functionality in Postgresql module.
Using data block first we check if keyhandle exists or not, if it exists we import it else it will be created.
https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/kms_key_handle