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

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

Merged
merged 40 commits into from
Feb 21, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
39fb80f
Key Vault: support for soft delete
WodansSon Jan 8, 2020
beafb4c
fix lint errors
WodansSon Jan 8, 2020
70a6a18
Update website/docs/r/key_vault.html.markdown
WodansSon Jan 23, 2020
22701db
Update website/docs/r/key_vault.html.markdown
WodansSon Jan 23, 2020
50563f8
Update azurerm/internal/services/keyvault/resource_arm_key_vault.go
WodansSon Jan 23, 2020
872984b
Merge branch 'master' of https://github.com/terraform-providers/terra…
WodansSon Jan 23, 2020
68f55a9
WIP Get value from provider
WodansSon Feb 1, 2020
e424d9a
Merge branch 'master' of https://github.com/terraform-providers/terra…
WodansSon Feb 3, 2020
924c38a
Done except for purge on destroy
WodansSon Feb 4, 2020
0f89dbd
Working soft delete and purge
WodansSon Feb 6, 2020
023aa56
Complete
WodansSon Feb 7, 2020
7b9909c
updated or to and
WodansSon Feb 7, 2020
96a36df
Fix features test cases
WodansSon Feb 7, 2020
3912093
Removed sku support from datasource
WodansSon Feb 7, 2020
b02dd08
Add code to skip purge if not soft deleted
WodansSon Feb 7, 2020
d1dc823
Merge branch 'e_keyvault_puge_softdelete' of https://github.com/terra…
WodansSon Feb 7, 2020
96f858a
Remove depricated sku from tests
WodansSon Feb 8, 2020
b8c1418
Fix basic test
WodansSon Feb 8, 2020
abd8429
Fix test case lint issue
WodansSon Feb 8, 2020
38848cf
Merge branch 'master' into e_keyvault_puge_softdelete
WodansSon Feb 12, 2020
f00d8f4
Merge branch 'master' into e_keyvault_puge_softdelete
WodansSon Feb 18, 2020
7f48f3a
Update test cases
WodansSon Feb 18, 2020
94dfc85
Fix test case
WodansSon Feb 19, 2020
0cd0b77
Merge branch 'master' of https://github.com/terraform-providers/terra…
WodansSon Feb 19, 2020
6c5e904
validate: removing the bool validators
tombuildsstuff Feb 20, 2020
76babfa
(d|r)/key_vault: moving the name validation into that package
tombuildsstuff Feb 20, 2020
d38b743
r/key_vault: moving the migration code closer to the package
tombuildsstuff Feb 20, 2020
586901d
r/key_vault: handling conditionally updating the key vault resource
tombuildsstuff Feb 20, 2020
872de6b
r/key_vault: updating the docs
tombuildsstuff Feb 20, 2020
d2cd248
minor: committing some lingering test files
tombuildsstuff Feb 20, 2020
31f5a73
Start adding purge on destroy back in
WodansSon Feb 20, 2020
6a7b3e5
Added Purge on Destroy back in
WodansSon Feb 21, 2020
a349df8
r/key_vault: fixing up the tests
tombuildsstuff Feb 21, 2020
300e2c2
Merge branch 'e_keyvault_puge_softdelete' of github.com:terraform-pro…
tombuildsstuff Feb 21, 2020
3a222df
r/key_vault: outputting a warning rather than an error during the delete
tombuildsstuff Feb 21, 2020
e85e4d1
r/key_vault: updating the docs
tombuildsstuff Feb 21, 2020
5cc9062
r/key_vault: removing a superflurious if statement
tombuildsstuff Feb 21, 2020
dc7b871
r/keyvault: removing dead code
tombuildsstuff Feb 21, 2020
415d4fd
r/keyvault: fixing the soft delete disabled test
tombuildsstuff Feb 21, 2020
7bf15ef
linting
tombuildsstuff Feb 21, 2020
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
23 changes: 23 additions & 0 deletions azurerm/helpers/validate/bool.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package validate

import (
"fmt"

"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
)

func BoolIsTrue() schema.SchemaValidateFunc {
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved
return func(i interface{}, k string) (_ []string, errors []error) {
v, ok := i.(bool)
if !ok {
errors = append(errors, fmt.Errorf("expected type of %q to be bool", k))
return
}

if !v {
errors = append(errors, fmt.Errorf("%q can only be set to true, if not required remove key", k))
return
}
return
}
}
35 changes: 35 additions & 0 deletions azurerm/helpers/validate/bool_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package validate

import "testing"

func TestBoolIsTrue(t *testing.T) {
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved
testCases := []struct {
Value bool
ShouldHaveError bool
}{
{
Value: true,
ShouldHaveError: false,
}, {
Value: false,
ShouldHaveError: true,
},
}

t.Run("TestBoolIsTrue", func(t *testing.T) {
for _, value := range testCases {
_, errors := BoolIsTrue()(value.Value, "dummy")
hasErrors := len(errors) > 0

if value.ShouldHaveError && !hasErrors {
t.Fatalf("Expected an error but didn't get one for %t", value.Value)
return
}

if !value.ShouldHaveError && hasErrors {
t.Fatalf("Expected %t to return no errors, but got some %+v", value.Value, errors)
return
}
}
})
}
12 changes: 12 additions & 0 deletions azurerm/internal/services/keyvault/data_source_key_vault.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,16 @@ func dataSourceArmKeyVault() *schema.Resource {
},
},

"enabled_for_soft_delete": {
Type: schema.TypeBool,
Computed: true,
},

"enabled_for_purge_protection": {
Type: schema.TypeBool,
Computed: true,
},

"tags": tags.SchemaDataSource(),
},
}
Expand Down Expand Up @@ -190,6 +200,8 @@ func dataSourceArmKeyVaultRead(d *schema.ResourceData, meta interface{}) error {
d.Set("enabled_for_deployment", props.EnabledForDeployment)
d.Set("enabled_for_disk_encryption", props.EnabledForDiskEncryption)
d.Set("enabled_for_template_deployment", props.EnabledForTemplateDeployment)
d.Set("enabled_for_soft_delete", props.EnableSoftDelete)
d.Set("enabled_for_purge_protection", props.EnablePurgeProtection)
d.Set("vault_uri", props.VaultURI)

if sku := props.Sku; sku != nil {
Expand Down
49 changes: 49 additions & 0 deletions azurerm/internal/services/keyvault/resource_arm_key_vault.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,25 @@ func resourceArmKeyVault() *schema.Resource {
Optional: true,
},

"enabled_for_soft_delete": {
Type: schema.TypeBool,
Optional: true,
ValidateFunc: validate.BoolIsTrue(),
WodansSon marked this conversation as resolved.
Show resolved Hide resolved
},

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

Choose a reason for hiding this comment

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

(same here)

Suggested change
ValidateFunc: validate.BoolIsTrue(),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

},

"purge_on_delete": {
Type: schema.TypeBool,
Optional: true,
Default: false,
ConflictsWith: []string{"enabled_for_purge_protection"},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


"network_acls": {
Type: schema.TypeList,
Optional: true,
Expand Down Expand Up @@ -252,6 +271,8 @@ func resourceArmKeyVaultCreateUpdate(d *schema.ResourceData, meta interface{}) e
enabledForDeployment := d.Get("enabled_for_deployment").(bool)
enabledForDiskEncryption := d.Get("enabled_for_disk_encryption").(bool)
enabledForTemplateDeployment := d.Get("enabled_for_template_deployment").(bool)
enabledForSoftDelete := d.Get("enabled_for_soft_delete").(bool)
enabledForPurgeProtection := d.Get("enabled_for_purge_protection").(bool)
t := d.Get("tags").(map[string]interface{})

networkAclsRaw := d.Get("network_acls").([]interface{})
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

if enabledForSoftDelete {
parameters.Properties.EnableSoftDelete = &enabledForSoftDelete
}

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

if enabledForPurgeProtection {
parameters.Properties.EnablePurgeProtection = &enabledForPurgeProtection
}

// Locking this resource so we don't make modifications to it at the same time if there is a
// key vault access policy trying to update it as well
locks.ByName(name, keyVaultResourceName)
Expand Down Expand Up @@ -375,6 +406,8 @@ func resourceArmKeyVaultRead(d *schema.ResourceData, meta interface{}) error {
d.Set("enabled_for_deployment", props.EnabledForDeployment)
d.Set("enabled_for_disk_encryption", props.EnabledForDiskEncryption)
d.Set("enabled_for_template_deployment", props.EnabledForTemplateDeployment)
d.Set("enabled_for_soft_delete", props.EnableSoftDelete)
d.Set("enabled_for_purge_protection", props.EnablePurgeProtection)
d.Set("vault_uri", props.VaultURI)

if sku := props.Sku; sku != nil {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

log.Printf("[DEBUG] KeyVault %s marked for purge, executing purge", name)
location := d.Get("location").(string)

future, err := client.PurgeDeleted(ctx, name, location)
if err != nil {
return err
}

log.Printf("[DEBUG] Waiting for purge of KeyVault %s", name)
err = future.WaitForCompletionRef(ctx, client.Client)
if err != nil {
return fmt.Errorf("Error purging Key Vault %q (Resource Group %q): %+v", name, resourceGroup, err)
}
}
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved

return nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,26 @@ func TestAccDataSourceAzureRMKeyVault_networkAcls(t *testing.T) {
})
}

func TestAccDataSourceAzureRMKeyVault_enable_soft_delete(t *testing.T) {
data := acceptance.BuildTestData(t, "data.azurerm_key_vault", "test")

resource.Test(t, resource.TestCase{
PreCheck: func() { acceptance.PreCheck(t) },
Providers: acceptance.SupportedProviders,
CheckDestroy: testCheckAzureRMKeyVaultDestroy,
Steps: []resource.TestStep{
{
Config: testAccDataSourceAzureRMKeyVault_enable_soft_delete(data),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMKeyVaultExists(data.ResourceName),
resource.TestCheckResourceAttr(data.ResourceName, "enabled_for_soft_delete", "true"),
resource.TestCheckResourceAttr(data.ResourceName, "enabled_for_purge_protection", "false"),
),
},
},
})
}

func testAccDataSourceAzureRMKeyVault_basic(data acceptance.TestData) string {
r := testAccAzureRMKeyVault_basic(data)
return fmt.Sprintf(`
Expand Down Expand Up @@ -146,3 +166,15 @@ data "azurerm_key_vault" "test" {
}
`, r)
}

func testAccDataSourceAzureRMKeyVault_enable_soft_delete(data acceptance.TestData) string {
r := testAccAzureRMKeyVault_enable_soft_delete(data)
return fmt.Sprintf(`
%s

data "azurerm_key_vault" "test" {
name = "${azurerm_key_vault.test.name}"
resource_group_name = "${azurerm_key_vault.test.resource_group_name}"
}
`, r)
}
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,55 @@ func TestAccAzureRMKeyVault_justCert(t *testing.T) {
})
}

func TestAccAzureRMKeyVault_soft_delete(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_key_vault", "test")
expectedError := fmt.Sprintf("^Check failed: Check 1/1 error: Not found: %s", data.ResourceName)
errorRegEx, _ := regexp.Compile(expectedError)

resource.Test(t, resource.TestCase{
PreCheck: func() { acceptance.PreCheck(t) },
Providers: acceptance.SupportedProviders,
CheckDestroy: testCheckAzureRMKeyVaultDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMKeyVault_basic(data),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMKeyVaultExists(data.ResourceName),
resource.TestCheckResourceAttr(data.ResourceName, "access_policy.0.key_permissions.0", "create"),
resource.TestCheckResourceAttr(data.ResourceName, "access_policy.0.secret_permissions.0", "set"),
resource.TestCheckResourceAttr(data.ResourceName, "tags.environment", "Production"),
),
},
{
Config: testAccAzureRMKeyVault_enable_soft_delete(data),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(data.ResourceName, "access_policy.0.key_permissions.0", "get"),
resource.TestCheckResourceAttr(data.ResourceName, "access_policy.0.secret_permissions.0", "get"),
resource.TestCheckResourceAttr(data.ResourceName, "enabled_for_soft_delete", "true"),
resource.TestCheckResourceAttr(data.ResourceName, "enabled_for_purge_protection", "false"),
resource.TestCheckResourceAttr(data.ResourceName, "tags.environment", "Production"),
),
},
{
Config: testAccAzureRMKeyVault_purge_vault(data),
ExpectError: errorRegEx,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMKeyVaultExists(data.ResourceName),
),
},
{
Config: testAccAzureRMKeyVault_basic(data),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMKeyVaultExists(data.ResourceName),
resource.TestCheckResourceAttr(data.ResourceName, "access_policy.0.key_permissions.0", "create"),
resource.TestCheckResourceAttr(data.ResourceName, "access_policy.0.secret_permissions.0", "set"),
resource.TestCheckResourceAttr(data.ResourceName, "tags.environment", "Production"),
),
},
},
})
}

func testCheckAzureRMKeyVaultDestroy(s *terraform.State) error {
client := acceptance.AzureProvider.Meta().(*clients.Client).KeyVault.VaultsClient
ctx := acceptance.AzureProvider.Meta().(*clients.Client).StopContext
Expand Down Expand Up @@ -446,6 +495,10 @@ resource "azurerm_key_vault" "test" {
"set",
]
}

tags = {
environment = "Production"
}
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger)
}
Expand Down Expand Up @@ -787,6 +840,59 @@ resource "azurerm_key_vault" "test" {
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger)
}

func testAccAzureRMKeyVault_enable_soft_delete(data acceptance.TestData) string {
return fmt.Sprintf(`
data "azurerm_client_config" "current" {}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_key_vault" "test" {
name = "vault%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
tenant_id = "${data.azurerm_client_config.current.tenant_id}"

sku {
name = "premium"
}

access_policy {
tenant_id = "${data.azurerm_client_config.current.tenant_id}"
object_id = "${data.azurerm_client_config.current.client_id}"

key_permissions = [
"get",
]

secret_permissions = [
"get",
]
}

enabled_for_soft_delete = true
purge_on_delete = true

tags = {
environment = "Production"
}
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger)
}

func testAccAzureRMKeyVault_purge_vault(data acceptance.TestData) string {
return fmt.Sprintf(`
data "azurerm_client_config" "current" {}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}
`, data.RandomInteger, data.Locations.Primary)
}

func testAccAzureRMKeyVault_complete(data acceptance.TestData) string {
return fmt.Sprintf(`
data "azurerm_client_config" "current" {}
Expand Down
4 changes: 4 additions & 0 deletions website/docs/d/key_vault.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ The following attributes are exported:

* `enabled_for_template_deployment` - Can Azure Resource Manager retrieve secrets from the Key Vault?

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

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

* `tags` - A mapping of tags assigned to the Key Vault.

A `sku` block exports the following:
Expand Down
6 changes: 6 additions & 0 deletions website/docs/r/key_vault.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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.
WodansSon marked this conversation as resolved.
Show resolved Hide resolved

* `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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


* `enabled_for_purge_protection` - (Optional) Boolean flag to specify whether the key vault is enabled for purge protection, this conflicts with `purge_on_delete`. Once enabled you can not disable this setting.
WodansSon marked this conversation as resolved.
Show resolved Hide resolved

* `network_acls` - (Optional) A `network_acls` block as defined below.

* `tags` - (Optional) A mapping of tags to assign to the resource.
Expand Down