New Resource: Key Vault Access Policy #1149

monkey-jeff · 2018-04-23T03:53:46Z

Hello everyone.

This is to address #754 which allows for separating our Key Vault Policies from the Key Vault Resource. This is also a feature I wanted as it allows me to pass in N number of policies for a Key Vault where N is not a constant. I can now add 'count' to the policy resource to create multiple at once.

The resource looks like below allowing you to update the policy of an existing policy.

resource "azurerm_key_vault_policy" "test" {
  vault_name                   = "NAME_OF_VAULT"
  vault_resource_group  = "RESOURCE_GROUP_OF_VAULT"

  object_id 		= "OBJECT_ID of service principal"
  tenant_id		= "TENANT_ID of service principal"
  application_id 	= "APPLICATION_ID if granting access to an application"

  key_permissions = [
    "create",
    "get"
  ]

  secret_permissions = [
    "get",
    "delete"
  ]
  certificate_permissions = [
    "create", 
    "delete"
  ]
}

This PR changes some behavior of the existing resrouce

Minimum Access Policy blocks in a key vault resource has been reduced to 0
Checking the access policies has been removed from the refresh of the Key Vault Resource (this is to prevent it from removing policies added by a policy resource object)

The way the new Key Vault resource interacts with these is as follows

If the object id / tenant id pair already exists in the access policy there will be no failure and in effect that resource will be imported into the terraform state (Hence the lacking of the ability to import since it already works seamlessly) //Question. Should I add something similar to this to the doc section explaining why there isn't an import?
If the access policy is removed outside of the terraform plan the refresh will want to add it back on a plan

I also moved the access policy definitions into its own file so there is a single place to update this.

Let me know what changes if any you guys would like me to make here. I do apologize if my go code is sloppy, this is my first time contributing to terraform and my first time writing in go, so I am sure there will be some issues to resolve.

-Jeff

day-jeff · 2018-04-23T23:18:20Z

Reviewed and tested this code. The implementation is solid, and everything worked perfectly in my real-world test. Thanks for this!

monkey-jeff · 2018-04-25T17:57:38Z

azurerm/resource_arm_key_vault.go

@@ -364,50 +298,11 @@ func flattenKeyVaultSku(sku *keyvault.Sku) []interface{} {
 	return []interface{}{result}
 }

-func flattenKeyVaultAccessPolicies(policies *[]keyvault.AccessPolicyEntry) []interface{} {


This was removed due to it only being used in a refresh. Refresh the access policies on the key vault resource causes a fight between key vault and key vault policy over ownership of the resource (basically doing this during a keyvault refresh it wants to remove resources created by a key vault policy

I note this because the merge conflict is now due to content that has been changes in the function that I had removed.

that's intentional - we can instead make the field access_policy Optional + Computed - which means that the value will be used from the API unless it's set locally (in which case diff's will be detected)

tiwood · 2018-05-02T18:02:30Z

Any updates on this? I have some use cases for this new resource type.

Thanks for your contribution.

pixelicous · 2018-05-03T16:20:14Z

This is cool! Looking forward to seeing it implemented as well

…t need to wait for keyvaults creationg as we aren't creating keyvaults

JunyiYi · 2018-05-08T03:09:18Z

azurerm/resource_arm_key_vault.go

@@ -76,7 +76,7 @@ func resourceArmKeyVault() *schema.Resource {
 			"access_policy": {
 				Type:     schema.TypeList,
 				Optional: true,
-				MinItems: 1,
+				MinItems: 0,


Since it is optional, you might just remove this line (MinItems: 0).

JunyiYi · 2018-05-08T03:12:10Z

azurerm/resource_arm_key_vault_policy.go

+	log.Printf("[INFO] preparing arguments for Azure ARM Policy: %s.", action)
+
+	name := d.Get("vault_name").(string)
+	//location := azureRMNormalizeLocation(d.Get("vault_location").(string))


If it is not used, remove this line.

JunyiYi · 2018-05-08T03:14:30Z

azurerm/resource_arm_key_vault_policy.go

+
+	d.SetId(*read.ID)
+
+	return resourceArmKeyVaultRead(d, meta)


Typically we don't read terraform resources after it is deleted, and we don't set its ID either.

Added a check that we aren't deleting it.

JunyiYi · 2018-05-09T00:56:32Z

azurerm/resource_arm_key_vault_policy.go

+	}
+
+	if action != keyvault.Remove {
+		d.SetId(*read.ID)


if action != keyvault.Remove { d.SetId(*read.ID) return resourceArmKeyVaultRead(d, meta) } else { return nil }

Good point, fixed

we can actually make this:

if d.IsNewResource() { d.SetID(*read.ID) }

since the ID won't change post-creation

monkey-jeff · 2018-05-16T15:18:21Z

Is there anything I need to do here right now? Have 2 approvals and a couple of pending. Sorry this is my first PR against terraform. Just curious as to what the normal process is from here as the CONTRIBUTING.md is a bit vague around this point of the process.

Also thanks for looking this over as well as the great platform/product that is terraform!

-Jeff

achandmsft · 2018-05-17T17:36:18Z

@tombuildsstuff could you please merge?

tiwood · 2018-07-04T11:22:03Z

I'm really desperate to get this functionality, is there a way to help or do we just need to wait for the reviews?

monkey-jeff · 2018-07-05T01:41:28Z

I think at this point it is waiting for reviews. It is not a small PR so it would take a little bit longer...hopefully tom and kat can get to it soon, but they are both very busy and have a lot to review....best think you can do is upvote this PR in my opinion.

day-jeff · 2018-07-05T16:41:03Z

@tiwood: Another option is to clone and build Jeff's branch: sophos-jeff:feature/key_vault_policy

tombuildsstuff · 2018-07-06T11:35:59Z

@sophos-jeff apologies for the delayed re-review here; I'm still trying to catch up from a few weeks AFK - I'll be taking another look through this shortly :)

Ensuring we only lock on the Key Vault name, since the access policy doesn't matter for locking purposes if we lock on the entire KeyVault ``` $ acctests azurerm TestAccAzureRMKeyVaultAccessPolicy_ === RUN TestAccAzureRMKeyVaultAccessPolicy_basic --- PASS: TestAccAzureRMKeyVaultAccessPolicy_basic (222.29s) === RUN TestAccAzureRMKeyVaultAccessPolicy_multiple --- PASS: TestAccAzureRMKeyVaultAccessPolicy_multiple (228.28s) === RUN TestAccAzureRMKeyVaultAccessPolicy_update --- PASS: TestAccAzureRMKeyVaultAccessPolicy_update (238.59s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 689.209s ```

Tests pass: ``` $ acctests azurerm TestAccAzureRMKeyVaultAccessPolicy_import === RUN TestAccAzureRMKeyVaultAccessPolicy_importBasic --- PASS: TestAccAzureRMKeyVaultAccessPolicy_importBasic (225.81s) === RUN TestAccAzureRMKeyVaultAccessPolicy_importMultiple --- PASS: TestAccAzureRMKeyVaultAccessPolicy_importMultiple (218.71s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 444.557s ```

tombuildsstuff · 2018-07-06T16:28:06Z

@sophos-jeff I've been spending some time on this resource this afternoon testing this/working out what's needed to get this merged and have made a few commits (mostly fixing the locking/refactoring/tests) - I hope you don't mind but I'm going to push these so that we can run the tests.

Once the tests pass (which the Access Policy tests have locally) - we'll do some edge case testing (probably Monday) and then we should be good to merge this :)

changes have been pushed

tombuildsstuff · 2018-07-06T16:30:55Z

Key Vault Access Policy Tests pass:

$ acctests azurerm TestAccAzureRMKeyVaultAccessPolicy_

=== RUN   TestAccAzureRMKeyVaultAccessPolicy_basic
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_basic (222.29s)
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_multiple
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_multiple (228.28s)
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_update
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_update (238.59s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	689.209s

$ acctests azurerm TestAccAzureRMKeyVaultAccessPolicy_import

=== RUN   TestAccAzureRMKeyVaultAccessPolicy_importBasic
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_importBasic (225.81s)
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_importMultiple
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_importMultiple (218.71s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	444.557s

…ing tests

tombuildsstuff · 2018-07-09T07:58:19Z

Ignoring an existing failing test in Key Vault - the tests pass:

tombuildsstuff

This now LGTM / seems fine with some edge-case testing 👍🏻

Thanks for this @sophos-jeff (hope you don't mind me pushing a few commits to finish this off!) :)

monkey-jeff · 2018-07-09T12:18:58Z

Not at all Tom. Thanks for getting this merged :D Glad to see this feature available soon :D

jmeisner3 · 2018-07-10T02:57:49Z

@sophos-jeff thanks a lot for working on this!

pixelicous · 2018-07-29T15:05:59Z

@tombuildsstuff Great work! This would be very useful to us and anyone else using azure, finally this is merged!! 😋 👍 🥇

Initial support for Azure Key Vault Policy resource

223a5d8

tombuildsstuff added new-resource service/key-vault Key Vault labels Apr 23, 2018

monkey-jeff mentioned this pull request Apr 23, 2018

Add stand-alone resource to edit Key Vault access policies #1052

Closed

tombuildsstuff self-assigned this Apr 24, 2018

monkey-jeff commented Apr 25, 2018

View reviewed changes

metacpp requested review from tombuildsstuff, metacpp, JunyiYi and WodansSon May 4, 2018 18:27

metacpp assigned tombuildsstuff and JunyiYi and unassigned tombuildsstuff May 4, 2018

metacpp added this to the Future milestone May 4, 2018

sophos-jeff and others added 2 commits May 4, 2018 14:40

Merge branch 'master' into feature/key_vault_policy

f8b19a5

Updated to fix the test broken from updating the source code. We don'…

aae8844

…t need to wait for keyvaults creationg as we aren't creating keyvaults

WodansSon previously approved these changes May 4, 2018

View reviewed changes

JunyiYi reviewed May 8, 2018

View reviewed changes

Addressed PR comments

24df15d

JunyiYi suggested changes May 9, 2018

View reviewed changes

Return nil on a delete for the shared delete/update function

000963e

JunyiYi previously approved these changes May 10, 2018

View reviewed changes

sophos-jeff and others added 2 commits May 13, 2018 16:25

Merge branch 'master' into feature/key_vault_policy

981dff0

Removed reference to old code

6490dad

tombuildsstuff unassigned JunyiYi May 23, 2018

Ran make fmt

17f46df

tombuildsstuff removed the request for review from metacpp July 6, 2018 11:38

tombuildsstuff removed the waiting-response label Jul 6, 2018

tombuildsstuff modified the milestones: Future, Soon Jul 6, 2018

tombuildsstuff added 3 commits July 6, 2018 13:53

Merge branch 'master' into feature/key_vault_policy

f500bab

Handling the case of an AppID and no AppID being specified / refactor…

0ea420a

…ing tests

tombuildsstuff modified the milestones: Soon, 1.9.0 Jul 9, 2018

tombuildsstuff added 2 commits July 9, 2018 11:30

Fixing the error message

56ae66b

Documentation fixes

aa2d2b0

tombuildsstuff approved these changes Jul 9, 2018

View reviewed changes

tombuildsstuff merged commit 1e9f067 into hashicorp:master Jul 9, 2018

tombuildsstuff added a commit that referenced this pull request Jul 9, 2018

Updating to include #1149

e481842

tombuildsstuff mentioned this pull request Jul 9, 2018

Feature Request: Allow access control modifications to an existing keyvault, without import. #754

Closed

hashicorp locked and limited conversation to collaborators Jul 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: Key Vault Access Policy #1149

New Resource: Key Vault Access Policy #1149

monkey-jeff commented Apr 23, 2018 •

edited

Loading

day-jeff commented Apr 23, 2018

monkey-jeff Apr 25, 2018 •

edited

Loading

tombuildsstuff May 23, 2018

tiwood commented May 2, 2018

pixelicous commented May 3, 2018

JunyiYi May 8, 2018

monkey-jeff May 8, 2018

JunyiYi May 8, 2018

monkey-jeff May 8, 2018

JunyiYi May 8, 2018

monkey-jeff May 8, 2018

JunyiYi May 9, 2018

monkey-jeff May 9, 2018

tombuildsstuff May 23, 2018

monkey-jeff commented May 16, 2018 •

edited

Loading

achandmsft commented May 17, 2018

tiwood commented Jul 4, 2018

monkey-jeff commented Jul 5, 2018

day-jeff commented Jul 5, 2018

tombuildsstuff commented Jul 6, 2018

tombuildsstuff commented Jul 6, 2018

tombuildsstuff commented Jul 6, 2018

tombuildsstuff commented Jul 9, 2018

tombuildsstuff left a comment •

edited

Loading

monkey-jeff commented Jul 9, 2018

jmeisner3 commented Jul 10, 2018

pixelicous commented Jul 29, 2018

New Resource: Key Vault Access Policy #1149

New Resource: Key Vault Access Policy #1149

Conversation

monkey-jeff commented Apr 23, 2018 • edited Loading

day-jeff commented Apr 23, 2018

monkey-jeff Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiwood commented May 2, 2018

pixelicous commented May 3, 2018

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

monkey-jeff commented May 16, 2018 • edited Loading

achandmsft commented May 17, 2018

tiwood commented Jul 4, 2018

monkey-jeff commented Jul 5, 2018

day-jeff commented Jul 5, 2018

tombuildsstuff commented Jul 6, 2018

tombuildsstuff commented Jul 6, 2018

tombuildsstuff commented Jul 6, 2018

tombuildsstuff commented Jul 9, 2018

tombuildsstuff left a comment • edited Loading

Choose a reason for hiding this comment

monkey-jeff commented Jul 9, 2018

jmeisner3 commented Jul 10, 2018

pixelicous commented Jul 29, 2018

monkey-jeff commented Apr 23, 2018 •

edited

Loading

monkey-jeff Apr 25, 2018 •

edited

Loading

monkey-jeff commented May 16, 2018 •

edited

Loading

tombuildsstuff left a comment •

edited

Loading