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

kms_key_v1: Please check state before trying to create a key #1804

Closed
rramge opened this issue Jul 8, 2022 · 10 comments · Fixed by #1806
Closed

kms_key_v1: Please check state before trying to create a key #1804

rramge opened this issue Jul 8, 2022 · 10 comments · Fixed by #1806
Assignees
Labels
Projects
Milestone

Comments

@rramge
Copy link

rramge commented Jul 8, 2022

Terraform provider version

Terraform v1.3.0-alpha20220608
on linux_amd64
+ provider registry.terraform.io/hashicorp/random v3.3.2
+ provider registry.terraform.io/opentelekomcloud/opentelekomcloud v1.29.9

Affected Resource(s)

opentelekomcloud_kms_key_v1

Terraform Configuration Files

module "kms" {
  source = "../modules/terraform-opentelekomcloud-kms"

  kms_keys = {
    key1 = { alias = "test" }
    key2 = { alias = "test-2", description = "A test key", is_rotation_enabled = true, rotation_interval = 90 }
  }
}
resource "opentelekomcloud_kms_key_v1" "this" {
  for_each = var.kms_keys != null ? var.kms_keys : {}

  key_alias = each.value["alias"]
  key_description = each.value["description"]
  realm = each.value["realm"]
  pending_days = each.value["pending_days"]
  is_enabled = each.value["is_enabled"]
  rotation_interval = each.value["rotation_interval"]
  rotation_enabled = each.value["is_rotation_enabled"]
  tags = var.tags
}

Debug Output/Panic Output

module.kms.opentelekomcloud_kms_key_v1.this["key2"]: Still creating... [10s elapsed]
module.kms.opentelekomcloud_kms_key_v1.this["key1"]: Still creating... [10s elapsed]
╷
│ Error: error waiting for key (f043636f-79d9-4fc0-9d5d-8f491dc13065) to become ready: unexpected state '4', wanted target '2'. last error: %!s(<nil>)
│ 
│   with module.kms.opentelekomcloud_kms_key_v1.this["key2"],
│   on ../modules/terraform-opentelekomcloud-kms/main.tf line 1, in resource "opentelekomcloud_kms_key_v1" "this":
│    1: resource "opentelekomcloud_kms_key_v1" "this" {
│ 
╵
╷
│ Error: error waiting for key (b6654ceb-3325-4d75-ae91-3e37be5e9538) to become ready: unexpected state '4', wanted target '2'. last error: %!s(<nil>)
│ 
│   with module.kms.opentelekomcloud_kms_key_v1.this["key1"],
│   on ../modules/terraform-opentelekomcloud-kms/main.tf line 1, in resource "opentelekomcloud_kms_key_v1" "this":
│    1: resource "opentelekomcloud_kms_key_v1" "this" {
│ 

Steps to Reproduce

  1. terraform apply
  2. terraform destroy
  3. terraform apply

Expected Behavior

After a destroy, another apply on the same terraform code should work.

Actual Behavior

During the destroy, the kms resource gets removed from the state, even though it still exists in OTC for a minimum of 7 and a maximum of 1096 days. During this period, which in production environments can pretty much effectively feel like "forever", other applyruns will loead into this error since the KMS is within OTC in the state "Pending deletion".

The only workaround on terraform level which I see at the moment would be appending random values to the key_alias to make the keys effectively unique, but this is visible for the customer and thus pretty ugly.

So, to avoid breaking customer's production environment, I see two possibilities:

  1. (Preferred) The provider should check if a key already exists, and if the state of the kms key is "Pending deletion" the creation should be skipped and optionally a Warning be generated on stdout. Or any other kind of error handling which seems more appropriate. Other cloud providers like Oracle already implement this in the backend and check this during refresh in the planning stage, so it should somehow be possible here, too I think.

  2. The quick & dirty solution which comes to my mind would be the introduction of a display_name and turning the key_alias unique.

Important Factoids

References

@anton-sidelnikov anton-sidelnikov self-assigned this Jul 11, 2022
@anton-sidelnikov anton-sidelnikov added this to To do in Upstream Jul 11, 2022
@anton-sidelnikov anton-sidelnikov moved this from To do to In progress in Upstream Jul 11, 2022
@anton-sidelnikov
Copy link
Member

@rramge Hi, we can add smth like "desired_state" option and reenable the key if it exist (https://docs.otc.t-systems.com/en-us/api/kms/kms_02_0016.html)

@anton-sidelnikov
Copy link
Member

Will continue after: opentelekomcloud/gophertelekomcloud#369

@rramge
Copy link
Author

rramge commented Jul 12, 2022

Sounds okay to me. Important thing is that a "terraform apply" does not lead into an error, and re-enabling the key would work.

otc-zuul bot pushed a commit that referenced this issue Jul 13, 2022
[KMS] Add `desired_state` in schema of `kms_key_v1`

Summary of the Pull Request
Added ability to canceling the Scheduled Deletion of a CMK
PR Checklist

 Refers to: #1804
 Tests added/passed.
 Documentation updated.
 Schema updated.
 Release notes added.

Acceptance Steps Performed
=== RUN   TestAccKmsKeyV1_basic
--- PASS: TestAccKmsKeyV1_basic (83.71s)
=== RUN   TestAccKmsKey_isEnabled
--- PASS: TestAccKmsKey_isEnabled (115.14s)
=== RUN   TestAccKmsKey_rotation
--- PASS: TestAccKmsKey_rotation (50.18s)
=== RUN   TestAccKmsKey_cancelDeletion
--- PASS: TestAccKmsKey_cancelDeletion(51.07s)
PASS

Process finished with the exit code 0

Reviewed-by: Artem Lifshits <None>
Reviewed-by: Vladimir Vshivkov <None>
Reviewed-by: Anton Sidelnikov <None>
@anton-sidelnikov anton-sidelnikov added this to the v1.30.0 milestone Jul 13, 2022
@vladimirvshivkov vladimirvshivkov linked a pull request Jul 14, 2022 that will close this issue
5 tasks
@vladimirvshivkov vladimirvshivkov moved this from In progress to Done in Upstream Jul 14, 2022
@anton-sidelnikov
Copy link
Member

Released, please check.

@rramge
Copy link
Author

rramge commented Jul 18, 2022

Released, please check.

You did something, yes, but the problem is still not solved. 'terraform apply' still breaks, the given example is still valid.

It's nice you added a desired_state flag or something, but you also need to validate it and act accordingly during the apply or update phase.

Notable difference is, that now even a manual "terraform import" doesn't work anymore:

╷

│ Error: Cannot import non-existent remote object
│ 
│ While attempting to import an existing object to
│ "module.kms.opentelekomcloud_kms_key_v1.this[\"key1\"]", the provider detected that no
│ object exists with the given id. Only pre-existing objects can be imported; check that
│ the id is correct and that it is associated with the provider's configured region or
│ endpoint, or use "terraform apply" to create a new remote object for this resource.
╵

Upstream automation moved this from Done to In progress Jul 18, 2022
@anton-sidelnikov
Copy link
Member

@rramge Did you add allow_cancel_deletion=true key into your code?

@rramge
Copy link
Author

rramge commented Jul 18, 2022

@anton-sidelnikov

I was not aware of allow_cancel_deletion, it's not in the documentation (yet). But now I am, so I hardcoded it in the module.

Effect is that it works with key1 in the following example, but not with key2.

module "kms" {
  source = "../modules/terraform-opentelekomcloud-kms"

  kms_keys = {
    key1 = { alias = "test" }
    key2 = { alias = "test-2", description = "A test key", is_rotation_enabled = true, rotation_interval = 90 }
  }
}

This leads to a follow-up error: {"error":{"error_msg":"The rotation state of key is not disabled.","error_code":"KMS.2901"}}.

@anton-sidelnikov
Copy link
Member

Depends-on: #1821

@anton-sidelnikov anton-sidelnikov modified the milestones: v1.30.0, v1.30.1 Jul 19, 2022
otc-zuul bot pushed a commit that referenced this issue Jul 19, 2022
[KMS] fix kms deletion with  enabled rotation

Summary of the Pull Request
When reenable key, which was created with rotation: {"error":{"error_msg":"The rotation state of key is not disabled.","error_code":"KMS.2901"}}.
PR Checklist

 Refers to: #1804 #1821
 Tests added/passed.
 Documentation updated.
 Schema updated.
 Release notes added.

Acceptance Steps Performed
=== RUN   TestAccKmsKeyV1_basic
--- PASS: TestAccKmsKeyV1_basic (83.19s)
=== RUN   TestAccKmsKey_isEnabled
--- PASS: TestAccKmsKey_isEnabled (116.61s)
=== RUN   TestAccKmsKey_rotation
--- PASS: TestAccKmsKey_rotation (51.10s)
=== RUN   TestAccKmsKey_cancelDeletion
--- PASS: TestAccKmsKey_cancelDeletion (50.83s)
=== RUN   TestAccKmsKey_cancelDeletionWithRotation
--- PASS: TestAccKmsKey_cancelDeletionWithRotation (52.38s)
PASS


Process finished with the exit code 0

Reviewed-by: Vladimir Vshivkov <None>
Reviewed-by: Artem Lifshits <None>
@anton-sidelnikov
Copy link
Member

@rramge Please check on new version

@rramge
Copy link
Author

rramge commented Jul 25, 2022

@anton-sidelnikov I tested with 1.30.1 and it looks good now.

Thank you very much :-)

@rramge rramge closed this as completed Jul 25, 2022
Upstream automation moved this from In progress to Done Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Upstream
  
Done
2 participants