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

WodansSon · 2020-01-08T20:34:34Z

Porting features from closed #1805 to new code base
Original: #1517
Dependent PR: #4366 #5668

(fixes #1329)

tombuildsstuff

Thanks for porting this PR over to the new structure.

Taking a look though this mostly LGTM - if we can fix up the comments then this should otherwise be good to go out in 2.0 👍

Thanks!

tombuildsstuff · 2020-01-09T06:01:26Z

azurerm/internal/services/keyvault/resource_arm_key_vault.go

+				Optional:      true,
+				Default:       false,
+				ConflictsWith: []string{"enabled_for_purge_protection"},
+			},


this can't go in the resource unfortunately, since the config isn't necessarily available during a destroy (e.g. if the users removed the code, this property won't be accessible) - instead it'll need to be in the provider block (which wants a little design work) - probably something like this:

provider "azurerm" { # ... purge_on_delete { key_vault = true } }

the reason for doing this as a nested block is this also applies to some other resources (recovery services) so we may as well use the same (design) approach for both

Fixed, moved it into the features section of the provider block.

azurerm/internal/services/keyvault/resource_arm_key_vault.go

tombuildsstuff · 2020-01-09T06:07:20Z

azurerm/internal/services/keyvault/resource_arm_key_vault.go

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


(same here)

Suggested change

ValidateFunc: validate.BoolIsTrue(),

tombuildsstuff · 2020-01-09T06:08:49Z

azurerm/internal/services/keyvault/resource_arm_key_vault.go

+		parameters.Properties.EnableSoftDelete = &enabledForSoftDelete
+	}
+
+	// This setting can only be set if it is true, if set when value is false API returns errors


there's an issue tracking disabling purge protection: Azure/azure-rest-api-specs#8075

...it still throws an error, maybe in future they will fix this and we can re-address the behavior.

tombuildsstuff · 2020-01-09T06:09:02Z

azurerm/internal/services/keyvault/resource_arm_key_vault.go

@@ -277,6 +298,16 @@ func resourceArmKeyVaultCreateUpdate(d *schema.ResourceData, meta interface{}) e
 		Tags: tags.Expand(t),
 	}

+	// This setting can only be set if it is true, if set when value is false API returns errors


I believe this is no longer true - soft delete can be enabled/disabled?

Nope... it still throws an error, maybe in future they will fix this and we can re-address the behavior.

tombuildsstuff · 2020-01-09T06:10:37Z

azurerm/internal/services/keyvault/resource_arm_key_vault.go

@@ -461,6 +494,22 @@ func resourceArmKeyVaultDelete(d *schema.ResourceData, meta interface{}) error {
 		}
 	}

+	if d.Get("purge_on_delete").(bool) && d.Get("enabled_for_soft_delete").(bool) && err == nil {


(as above) we'll need to pull this from the provider block instead unfortunately

website/docs/r/key_vault.html.markdown

tombuildsstuff · 2020-01-09T06:15:24Z

website/docs/r/key_vault.html.markdown

@@ -84,6 +84,12 @@ The following arguments are supported:

 * `enabled_for_template_deployment` - (Optional) Boolean flag to specify whether Azure Resource Manager is permitted to retrieve secrets from the key vault. Defaults to `false`.

+* `enabled_for_soft_delete` - (Optional) Boolean flag to specify whether the key vault is enabled for soft delete. Once enabled you can not disable this setting.
+
+* `purge_on_delete` - (Optional) Boolean flag to specify if the KeyVault should be purged on delete. This purges KeyVaults enabled for soft delete on resource deletition.


(as above) we'll need to move this into the provider block

website/docs/r/key_vault.html.markdown

Co-Authored-By: Tom Harvey <[email protected]>

…form-provider-azurerm into e_keyvault_puge_softdelete

…form-providers/terraform-provider-azurerm into e_keyvault_puge_softdelete

tombuildsstuff

hey @WodansSon

Thanks for porting this PR over to the refactored structure - I've taken a look through and left some comments inline but this otherwise LGTM; as discussed offline I'm going to push a few commits to resolve the issues highlighted below, but this otherwise looks good to me 👍

Thanks!

azurerm/internal/services/keyvault/tests/resource_arm_key_vault_test.go

website/docs/r/key_vault.html.markdown

azurerm/internal/services/keyvault/tests/resource_arm_key_vault_test.go

azurerm/helpers/validate/bool_test.go

azurerm/internal/services/keyvault/resource_arm_key_vault.go

azurerm/helpers/azure/key_vault.go

WodansSon · 2020-02-21T03:03:02Z

I'm not sure what to do for the test cases where soft delete is enabled, when the test finishes and it destroys the test resources, the key vault will still be there just soft deleted. My workaround for this was to add an extra test step pointing the config to the testAccAzureRMKeyVault_softDeletePurge config which then forces the resource to destroy the key vault by triggering the purge on destroy code path, else we will have a bunch of key vaults hanging out in the soft delete state for 90 days.

I also think we need to update the exists function in the tests to verify that the key vault is:

Deleted, cannot be found with a normal client.Get
Not in the soft delete state, cannot be found with a client.GetDeleted

WDYT?

…viders/terraform-provider-azurerm into e_keyvault_puge_softdelete

azurerm/internal/services/keyvault/resource_arm_key_vault.go

WodansSon

There is one change that I think needs to be made, other than that I think this is good to go!

tombuildsstuff · 2020-02-21T15:55:09Z

Tests pass:

ghost · 2020-02-24T16:47:57Z

This has been released in version 2.0.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.0.0"
}
# ... other configuration ...

ghost · 2020-03-28T14:55:32Z

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 👉 [email protected]. Thanks!

Key Vault: support for soft delete

39fb80f

WodansSon added enhancement documentation service/key-vault Key Vault labels Jan 8, 2020

WodansSon added this to the v2.0.0 milestone Jan 8, 2020

ghost added the size/L label Jan 8, 2020

WodansSon mentioned this pull request Jan 8, 2020

Key Vault: support for soft delete #1805

Closed

fix lint errors

beafb4c

WodansSon requested a review from katbyte January 8, 2020 23:30

tombuildsstuff suggested changes Jan 9, 2020

View reviewed changes

tombuildsstuff added the waiting-response label Jan 9, 2020

WodansSon and others added 5 commits January 23, 2020 14:16

Update website/docs/r/key_vault.html.markdown

70a6a18

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/key_vault.html.markdown

22701db

Co-Authored-By: Tom Harvey <[email protected]>

Update azurerm/internal/services/keyvault/resource_arm_key_vault.go

50563f8

Co-Authored-By: Tom Harvey <[email protected]>

Merge branch 'master' of https://github.com/terraform-providers/terra…

872984b

…form-provider-azurerm into e_keyvault_puge_softdelete

WIP Get value from provider

68f55a9

ghost added size/XL and removed size/L labels Feb 1, 2020

WodansSon added 2 commits February 3, 2020 12:28

Merge branch 'master' of https://github.com/terraform-providers/terra…

e424d9a

…form-provider-azurerm into e_keyvault_puge_softdelete

Done except for purge on destroy

924c38a

WodansSon removed the waiting-response label Feb 4, 2020

WodansSon added 4 commits February 5, 2020 16:24

Working soft delete and purge

0f89dbd

Complete

023aa56

updated or to and

7b9909c

Fix features test cases

96a36df

WodansSon changed the title ~~Key Vault: support for soft delete~~ Enhancement: azurerm_key_vault: Add support for soft delete, purge protection, and purge on destroy Feb 7, 2020

WodansSon requested a review from tombuildsstuff February 7, 2020 05:52

WodansSon added 3 commits February 6, 2020 22:05

Removed sku support from datasource

3912093

Add code to skip purge if not soft deleted

b02dd08

Merge branch 'e_keyvault_puge_softdelete' of https://github.com/terra…

d1dc823

…form-providers/terraform-provider-azurerm into e_keyvault_puge_softdelete

tombuildsstuff added 5 commits February 20, 2020 12:59

validate: removing the bool validators

6c5e904

(d|r)/key_vault: moving the name validation into that package

76babfa

r/key_vault: moving the migration code closer to the package

d38b743

r/key_vault: handling conditionally updating the key vault resource

586901d

r/key_vault: updating the docs

872de6b

tombuildsstuff approved these changes Feb 20, 2020

View reviewed changes

ghost added size/XXL and removed size/XL labels Feb 20, 2020

minor: committing some lingering test files

d2cd248

tombuildsstuff removed the request for review from katbyte February 20, 2020 19:32

WodansSon added 2 commits February 20, 2020 14:21

Start adding purge on destroy back in

31f5a73

Added Purge on Destroy back in

6a7b3e5

tombuildsstuff added 2 commits February 21, 2020 09:29

r/key_vault: fixing up the tests

a349df8

Merge branch 'e_keyvault_puge_softdelete' of github.com:terraform-pro…

300e2c2

…viders/terraform-provider-azurerm into e_keyvault_puge_softdelete

WodansSon commented Feb 21, 2020

View reviewed changes

azurerm/internal/services/keyvault/resource_arm_key_vault.go Outdated Show resolved Hide resolved

WodansSon commented Feb 21, 2020

View reviewed changes

tombuildsstuff added 5 commits February 21, 2020 14:36

r/key_vault: outputting a warning rather than an error during the delete

3a222df

r/key_vault: updating the docs

e85e4d1

r/key_vault: removing a superflurious if statement

5cc9062

r/keyvault: removing dead code

dc7b871

r/keyvault: fixing the soft delete disabled test

415d4fd

linting

7bf15ef

tombuildsstuff merged commit 2e17f74 into master Feb 21, 2020

tombuildsstuff deleted the e_keyvault_puge_softdelete branch February 21, 2020 17:46

tombuildsstuff added a commit that referenced this pull request Feb 21, 2020

updating to include #5344

aa72687

ghost locked and limited conversation to collaborators Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

WodansSon commented Jan 8, 2020 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Jan 9, 2020

WodansSon Feb 7, 2020

tombuildsstuff Jan 9, 2020

WodansSon Feb 7, 2020

tombuildsstuff Jan 9, 2020

WodansSon Feb 7, 2020

tombuildsstuff Jan 9, 2020

WodansSon Feb 7, 2020

tombuildsstuff Jan 9, 2020

WodansSon Feb 7, 2020

tombuildsstuff Jan 9, 2020

WodansSon Feb 7, 2020

tombuildsstuff left a comment

WodansSon commented Feb 21, 2020

WodansSon left a comment

tombuildsstuff commented Feb 21, 2020

ghost commented Feb 24, 2020

ghost commented Mar 28, 2020

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

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

Conversation

WodansSon commented Jan 8, 2020 • edited Loading

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

tombuildsstuff left a comment

Choose a reason for hiding this comment

WodansSon commented Feb 21, 2020

WodansSon left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Feb 21, 2020

ghost commented Feb 24, 2020

ghost commented Mar 28, 2020

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

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

WodansSon commented Jan 8, 2020 •

edited

Loading