Key vault add soft delete #1517

janboll · 2018-07-09T09:47:44Z

Make it possible to enable soft delete via Terraform.
This would fix #1329.

tombuildsstuff

Thanks for this PR :)

I've taken a look through and this mostly LGTM - if we can fix up the merge conflicts / comments then we should be able to run the tests and get this merged :)

Thanks!

tombuildsstuff · 2018-07-09T10:00:23Z

azurerm/resource_arm_key_vault.go

+			"enabled_for_soft_delete": {
+				Type:         schema.TypeBool,
+				Optional:     true,
+				ValidateFunc: validate.BoolIsTrue(),


given the code below ignores this if it's not set to true - I think we can safely remove this validation function? This allows users to define this variable optionally using a variable e.g.

resource "azurerm_key_vault" "test" { count = 2 enabled_for_soft_delete = "${count.index == 0 ? true : false}" }

given the comment in the docs Once enabled you can not disable this setting anymore! can we add ForceNew: true here?

I tried to use ForceNew, but changing from soft delete true to soft delete false yields following error:
azurerm_key_vault.test: Error updating Key Vault "vault5353625459524355837" (Resource Group "acctestRG-5353625459524355837"): keyvault.VaultsClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="ConflictError" Message="Exist soft deleted vault with the same name. "
On the other hand, switching soft delete on works in place without issue.

I thought about adding the purge to the delete function, but the PurgeDeleted function also has some limitations: https://docs.microsoft.com/en-us/azure/key-vault/key-vault-ovw-soft-delete. So this would potentially break as well.

I suspect that removing the validation function and adding ForceNew: true could not work due to the way soft deleted key vaults are handled on Azure side.

I thought about adding the purge to the delete function, but the PurgeDeleted function also has some limitations: https://docs.microsoft.com/en-us/azure/key-vault/key-vault-ovw-soft-delete. So this would potentially break as well.

in which case I think we'd want to purge the Key Vault if it exists at deletion time, via an opt-in flag? If users don't opt-into that and delete a soft-deleted KeyVault then they'll get this error, at which point it's up to them to recover the vault and import it into Terraform's Statefile to be able to proceed. What do you think?

We have a similar opt-in property in the VM resource where we optionally delete the OS and Data Disks when the VM is deleted.

This sounds like a good idea 👍 I added an option to toggle the purge behaviour and also updated the tests support this.

tombuildsstuff · 2018-07-09T10:00:27Z

azurerm/resource_arm_key_vault.go

+			"enabled_for_purge_protection": {
+				Type:         schema.TypeBool,
+				Optional:     true,
+				ValidateFunc: validate.BoolIsTrue(),


given the code below ignores this if it's not set to true - I think we can safely remove this validation function?

given the comment in the docs Once enabled you can not disable this setting anymore! can we add ForceNew: true here?

tombuildsstuff · 2018-07-09T10:00:48Z

azurerm/resource_arm_key_vault_test.go

@@ -216,7 +220,7 @@ resource "azurerm_key_vault" "test" {

  sku {
    name = "premium"
-  }
+	}


minor can we revert this?

tombuildsstuff · 2018-07-09T10:01:08Z

azurerm/resource_arm_key_vault_test.go

@@ -295,6 +305,9 @@ resource "azurerm_key_vault" "test" {
  sku {
    name = "premium"
  }
+
+	enabled_for_soft_delete = "True"
+	enabled_for_purge_protection = "True"


minor can we remove the quotes around these values?

tombuildsstuff · 2018-07-09T10:01:14Z

azurerm/resource_arm_key_vault_test.go

-  }
+	}
+
+	enabled_for_soft_delete = "True"


minor can we remove the quotes around this value?

tombuildsstuff · 2018-07-09T10:02:06Z

website/docs/r/key_vault.html.markdown

+    the key vault is enabled for soft delete. Once enabled you can not disable this setting anymore!
+
+* `enabled_for_purge_protection` - (Optional) Boolean flag to specify whether
+    the key vault is enabled for purge protection. Once enabled you can not disable this setting anymore!


can we replace Once enabled you can not disable this setting anymore! with Changing this forces a new resource to be created.

tombuildsstuff · 2018-07-09T10:04:25Z

azurerm/resource_arm_key_vault_test.go

@@ -136,6 +138,8 @@ func TestAccAzureRMKeyVault_update(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "enabled_for_deployment", "true"),
 					resource.TestCheckResourceAttr(resourceName, "enabled_for_disk_encryption", "true"),
 					resource.TestCheckResourceAttr(resourceName, "enabled_for_template_deployment", "true"),
+					resource.TestCheckResourceAttr(resourceName, "enabled_for_soft_delete", "true"),
+					resource.TestCheckResourceAttr(resourceName, "enabled_for_purge_protection", "false"),


given changing this value will force a new resource, it'll break this test - can we check for this value in a new test?

tombuildsstuff · 2018-07-09T10:04:26Z

azurerm/resource_arm_key_vault_test.go

@@ -102,6 +102,8 @@ func TestAccAzureRMKeyVault_complete(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMKeyVaultExists(resourceName),
 					resource.TestCheckResourceAttrSet(resourceName, "access_policy.0.application_id"),
+					resource.TestCheckResourceAttr(resourceName, "enabled_for_soft_delete", "true"),
+					resource.TestCheckResourceAttr(resourceName, "enabled_for_purge_protection", "true"),


rather than modifying this test - can we check these values in a new test?

I created a seperate test for this: TestAccAzureRMKeyVault_enable_soft_delete

…_schedule` resource

…e `azurerm_automation_schedule` resource

…roperties of the `azurerm_automation_schedule` resource

…n_schedule` in case of state change to remove

…te of the `azurerm_automation_schedule` resource

Syncing from TF

There's an extra underscore in the header/title, and it doesn't match the actual name of the resource.

docs/kubernetes_cluster: Fix inaccurate kube_config_raw description

@tombuildsstuff

…icorp#1706) * tokens invalid when az and terraform run on different timezones * tokens invalid when az and terraform run on different timezones More specifically, - adapt @tombuildsstuff's (thanks, that's better) stylistic requests.

…hicorp#1713) * update VMSS docs to include ip_forwarding (fixes hashicorp#1709) * Updating to include the default value

The options for `require_sni` were not indented/formatted consistently with the rest of the page

…sCaseDiff cleaning up some package tech debt

WodansSon

@janboll thanks for the PR, I've looked it over an have a question I left you in the comments.

WodansSon · 2018-08-16T23:19:00Z

website/docs/d/key_vault.html.markdown

+* `enabled_for_soft_delete` -  Is soft delete enabled on this Key Vault?
+
+* `enabled_for_purge_protection` - Is purge protection enabled on this Key Vault?
+


You have updated the data source documentation but I did not see the data_source_key_vault.go in the PR, shouldn't also expose these new fields in the data source as well?

Fix some pointer references

Fix: Corrected regexp for eventhub name

* 🐛 (Network profile of AKS.) Remove client side validation for azure network plugin. Non-empty podCidr string validation for azure network plugin should be handled by ASK service. * 📝 (Network profile of AKS.) Add more explainations for network_profile. Add more explainations on default network_profile and `azure` specific validation requirement. * Updating to better match the other docs

…1789) * issues:1743 | Added support for EventHub compatible EndPoints and Paths * issues:1743 | Updated documentation for EventHub compatible EndPoints and Paths * Updating the documentation to list fields explicitly

janboll · 2018-08-21T13:20:36Z

I made a mistake while rebasing and broke the branch, will create a new one with reference to this one

janboll · 2018-08-21T14:25:48Z

Opened a new PR, I am sorry for the mess :-(

ghost · 2020-03-30T15:01:44Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

tombuildsstuff suggested changes Jul 9, 2018

View reviewed changes

tombuildsstuff added enhancement service/key-vault Key Vault labels Jul 9, 2018

Add property to enable soft delete feature

b7c8110

janboll force-pushed the KeyVault-Add-SoftDelete branch from c31abe7 to b7c8110 Compare July 9, 2018 10:14

Create seperate test for enabling soft delete feature, fix formatting

7628d8b

tombuildsstuff added the waiting-response label Jul 10, 2018

Jan-Hendrik Boll added 2 commits July 17, 2018 17:03

Add toggle to configure purge behaviour

ee7ca4f

Fix formating

a35b14e

tombuildsstuff removed the waiting-response label Jul 19, 2018

tombuildsstuff added this to the 1.11.0 milestone Jul 19, 2018

TFaga added 5 commits July 22, 2018 22:12

Added missing advanced schedule properties to the `azurerm_automation…

5e8bad9

…_schedule` resource

Added webside docs for the missing advanced schedule properties in th…

ab3ca15

…e `azurerm_automation_schedule` resource

Minor fixes to the code quality and diff behavior with the advanced p…

adab682

…roperties of the `azurerm_automation_schedule` resource

Updated handling of the advanced properties in the `azurerm_automatio…

22a9c1c

…n_schedule` in case of state change to remove

Minor fixes to the handling of the advanced properties on create/upda…

980066b

…te of the `azurerm_automation_schedule` resource

katbyte modified the milestones: 1.11.0, 1.12.0 Jul 25, 2018

Matt Betts and others added 4 commits July 27, 2018 22:44

Merge pull request hashicorp#1 from terraform-providers/master

e5e75f2

Syncing from TF

Fix typo in the azurerm_cosmosdb_account page heading (hashicorp#1702)

d11fe45

There's an extra underscore in the header/title, and it doesn't match the actual name of the resource.

docs/kubernetes_cluster: Fix inaccurate kube_config_raw description

bac8c25

Merge pull request hashicorp#1704 from terraform-providers/docs-aks-fix

60ede24

docs/kubernetes_cluster: Fix inaccurate kube_config_raw description

metacpp requested a review from WodansSon August 2, 2018 23:59

metacpp assigned WodansSon Aug 2, 2018

rrudduck and others added 4 commits August 3, 2018 05:48

Remove default from vmss priority property. (hashicorp#1586)

07ca122

Updating to include hashicorp#1586

84207b6

update VMSS docs to include ip_forwarding (fixes hashicorp#1709) (has…

6e4266c

…hicorp#1713) * update VMSS docs to include ip_forwarding (fixes hashicorp#1709) * Updating to include the default value

tombuildsstuff modified the milestones: 1.12.0, 1.13.0 Aug 3, 2018

tf-release-bot and others added 5 commits August 15, 2018 20:18

Cleanup after v1.13.0 release

ca79200

consolidate CaseDifference Supression functions

903bd52

folded subscription and [az]schema packages into azure

9dbcdf9

Fix indentation on application_gateway.html.markdown (hashicorp#1780)

8b7445d

The options for `require_sni` were not indented/formatted consistently with the rest of the page

Merge pull request hashicorp#1781 from terraform-providers/td-suppres…

6289f69

…sCaseDiff cleaning up some package tech debt

WodansSon requested changes Aug 16, 2018

View reviewed changes

hbuckle and others added 17 commits August 19, 2018 07:36

fix some pointer dereferences

c374aae

reset verb

f28bae6

Fix: Corrected regexp for eventhub name

90ac144

Merge pull request hashicorp#1791 from hiscox/pointer_dereference

2639057

Fix some pointer references

Merge pull request hashicorp#1795 from fabotronix/master

8d05709

Fix: Corrected regexp for eventhub name

Update CHAGELOG.md to include hashicorp#1795

86e5545

backport azure stack route_table PR review comments (hashicorp#1790)

552d613

Updating to include hashicorp#1798

2bc7ead

Updating to include hashicorp#1789

b11f631

Add property to enable soft delete feature

cb4e132

Create seperate test for enabling soft delete feature, fix formatting

e7b6a94

Add toggle to configure purge behaviour

a3d9e88

Fix formating

f708375

Add missing datasource code

b061f43

Merge

7f31d67

janboll closed this Aug 21, 2018

janboll mentioned this pull request Aug 21, 2018

Key Vault: support for soft delete #1805

Closed

WodansSon mentioned this pull request Jan 8, 2020

Enhancement: azurerm_key_vault: Add support for soft delete, purge protection, and purge on destroy #5344

Merged

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Key vault add soft delete #1517

Key vault add soft delete #1517

janboll commented Jul 9, 2018

tombuildsstuff left a comment

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

janboll Jul 10, 2018

tombuildsstuff Jul 10, 2018

janboll Jul 17, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

tombuildsstuff Jul 9, 2018

janboll Jul 10, 2018

WodansSon left a comment

WodansSon Aug 16, 2018

janboll commented Aug 21, 2018

janboll commented Aug 21, 2018

ghost commented Mar 30, 2020

		* `enabled_for_soft_delete` - Is soft delete enabled on this Key Vault?

		* `enabled_for_purge_protection` - Is purge protection enabled on this Key Vault?

Key vault add soft delete #1517

Key vault add soft delete #1517

Conversation

janboll commented Jul 9, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janboll commented Aug 21, 2018

janboll commented Aug 21, 2018

ghost commented Mar 30, 2020