Skip to content

Add a custom resource for Key Vault access policies#2951

Merged
thomas11 merged 24 commits into
masterfrom
tkappler/kv-ap-2
Jan 3, 2024
Merged

Add a custom resource for Key Vault access policies#2951
thomas11 merged 24 commits into
masterfrom
tkappler/kv-ap-2

Conversation

@thomas11

@thomas11 thomas11 commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

This PR implements #594, allowing access policies (APs) to be managed as individual Pulumi resources.

This PR builds on top of #2950 and shares its commits.

It adds a new custom resource following the design of the existing ones: use Azure's KeyVault client to directly implement the AP CRUD operations. Note that access policies do not have standard CRUD semantics. Instead, the "add" operation merges if the AP already exists, and the "replace" operation creates if it does not exist. Therefore, the custom resource uses "replace" for both create and update, with a prior read to ensure that the AP does/does not exist yet.

In addition to the custom resource, some other changes were required:

  • The KV client authenticates via the newer azcore interfaces. Our existing tokens are valid, so I added a wrapper to implement the azcore interface.
  • When a resource has properties pointing to independent sub-resources, set them to their empty default value on create if not specified. This was required because Vault requires the accessPolicies property, but we don't want to explicitly set it to an empty [] and record this in Pulumi state. Otherwise, every update to the vault would clear out the APs.

@thomas11 thomas11 requested review from a team and danielrbradley December 18, 2023 09:43
@github-actions

github-actions Bot commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

Does the PR have any schema changes?

Looking good! No breaking changes found.

New resources:

  • keyvault.AccessPolicy

@thomas11 thomas11 force-pushed the tkappler/kv-ap-2 branch 2 times, most recently from d2b2418 to 10f5d17 Compare December 19, 2023 21:04
@thomas11 thomas11 changed the title Add a custom resource for independent maintenance of Key Vault access policies Add a custom resource for Key Vault access policies Dec 20, 2023
@codecov

codecov Bot commented Dec 20, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 150 lines in your changes are missing coverage. Please review.

Comparison is base (8fbb627) 60.52% compared to head (03bebe4) 60.09%.

Files Patch % Lines
...ider/pkg/resources/custom_keyvault_accesspolicy.go 47.05% 124 Missing and 2 partials ⚠️
provider/pkg/provider/auth.go 0.00% 15 Missing ⚠️
provider/pkg/gen/properties.go 0.00% 2 Missing and 1 partial ⚠️
provider/pkg/provider/provider.go 50.00% 2 Missing and 1 partial ⚠️
provider/pkg/resources/customresources.go 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2951      +/-   ##
==========================================
- Coverage   60.52%   60.09%   -0.43%     
==========================================
  Files          64       65       +1     
  Lines       10588    10852     +264     
==========================================
+ Hits         6408     6522     +114     
- Misses       3640     3785     +145     
- Partials      540      545       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomas11 thomas11 force-pushed the tkappler/kv-ap-2 branch 9 times, most recently from 7ba7c8f to b25f186 Compare December 21, 2023 14:11
@thomas11 thomas11 marked this pull request as ready for review December 21, 2023 14:49

@danielrbradley danielrbradley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Core implementation and integration tests all look good to go 👍

I think there's opportunity to add some basic unit tests around the simple funcitons like parseKeyVaultPathParams, sdkPolicyToMap, sdkPermissionsToMap and propertyPermissionsToSdk but happy for this to ship with the core integration tests for now if preferred.

@thomas11 thomas11 force-pushed the tkappler/kv-ap-2 branch 2 times, most recently from 6df5cf7 to 7fad6f6 Compare January 3, 2024 10:44
@thomas11 thomas11 merged commit 3db6e29 into master Jan 3, 2024
@thomas11 thomas11 deleted the tkappler/kv-ap-2 branch January 3, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants