feat: add nversion support to azurekeyvault provider certificates and keys #1955

duffney · 2024-11-27T17:08:46Z

Description

What this PR does / why we need it:

Adds a configuration option to the Azure Key Vault provider of KMP called versionHistoryLimit that allows the user to define the number of versions of a certificate and or key to store in the Ratify KMP cache for artifact verification. Needed to fully implement the refresh interval feature proposed in design doc for KMP periodic retrieval.

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

Fixes #1751

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
- Note: Might be breaking, requires workload identities to have certificate & key List permission on AKV.
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Confirm that multiple certificates/keys are correctly stored in the KMP.
Ensure that the refresh process updates the KMP with the latest versions
Verify that the latest version is selected when versionHistoryLimit is not specified.
Prevent runtime errors by handling cases where versionHistoryLimit exceeds the total available versions.
The refresh process updates the total versions whenever new certificate or key versions are created, even if the specified limit exceeds the number of existing certificates.

apiVersion: config.ratify.deislabs.io/v1beta1
kind: KeyManagementProvider
metadata:
  name: akv
spec:
  type: azurekeyvault
  refreshInterval: 1m
  parameters:
    vaultURI: https://ratifyakvs5r.vault.azure.net/
    certificates:
      - name: ratify
        versionHistoryLimit: 2
    keys:
      - name: test
        versionHistoryLimit: 2
    tenantID: 
    clientID:

## KMP Status showing multiple versions stored
 properties:
    Certificates:
    - Enabled: "true"
      LastRefreshed: "2024-11-27T14:16:56Z"
      Name: ratify
      Version: 230b4bef7a2d435e80235f04310320df
    - Enabled: "true"
      LastRefreshed: "2024-11-27T14:16:56Z"
      Name: ratify
      Version: 67ae6208331e4ab8a03229a10bce9a6e
    Keys:
    - Enabled: "true"
      LastRefreshed: "2024-11-27T17:10:13Z"
      Name: test
      Version: 4f74798850e242c3914545961cdf595e
    - Enabled: "true"
      LastRefreshed: "2024-11-27T17:10:13Z"
      Name: test
      Version: 7df0412c291f46fc9112dc0df2df6a22

Note: Image showing latest versions of the certificate are stored in KMP

Note: Image showing latest versions of the key are stored in KMP

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

codecov · 2024-11-27T17:50:10Z

Codecov Report

Attention: Patch coverage is 95.89744% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...kg/keymanagementprovider/azurekeyvault/provider.go	95.89%	8 Missing ⚠️

Files with missing lines	Coverage Δ
...kg/keymanagementprovider/azurekeyvault/provider.go	`87.89% <95.89%> (+3.65%)`	⬆️

... and 5 files with indirect coverage changes

duffney · 2024-11-28T00:01:52Z

pkg/keymanagementprovider/azurekeyvault/types/types.go

@@ -37,4 +37,6 @@ type KeyVaultValue struct {
 	Name string `json:"name" yaml:"name"`
 	// the version of the Azure Key Vault certificate/key
 	Version string `json:"version" yaml:"version"`
+	// the number of versions to keep in the cache
+	VersionHistoryLimit int `json:"versionHistoryLimit" yaml:"versionHistoryLimit"`


@akashsinghal @binbin-li what are your thoughts on adding a VersionHistory field here with the sortVersionHistory and GetSortedVersions as methods?

if those 2 methods takes KeyVaultValue as receiver or operates on it, I feel they can be added here.

+1 to binbin's comment

binbin-li · 2024-12-02T04:42:21Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+			keyVaultCert.VersionHistoryLimit = versionHistoryLimitDefault
+		}
+
+		var versionHistory []struct {


nit: we can define the struct on top of the file as I saw it's defined multiple times in the file.

Changes made: 9f42567

binbin-li · 2024-12-02T05:41:20Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+		sortedVersionHistory := GetSortedVersions(versionHistory)
+
+		// if versionHistoryLimit is greater than the number of versions, set it to the number of versions
+		if keyVaultKey.VersionHistoryLimit > len(sortedVersionHistory) {


we can combine both checking on versionHistoryLimit in the same place. keyVaultKey.VersionHistoryLimit == 0

Changes made: a3828c9

I moved the logic together and added another if to check the length of the sorted versions to output a info message and continue when no versions were returned from the pager.

// if no versions found, log and continue if len(sortedVersionHistory) == 0 { logger.GetLogger(ctx, logOpt).Infof("no versions found for key %s", keyVaultKey.Name) continue } // if versionHistoryLimit isn't set default to 1 to avoid out of bounds error if keyVaultKey.VersionHistoryLimit == 0 { keyVaultKey.VersionHistoryLimit = versionHistoryLimitDefault } // if versionHistoryLimit is greater than the number of versions, set it to the number of versions if keyVaultKey.VersionHistoryLimit > len(sortedVersionHistory) { keyVaultKey.VersionHistoryLimit = len(sortedVersionHistory) }

I know, it's a lot of if statements... I'm working on refactoring it with the backwards compatibility suggestions from @susanshi.

binbin-li · 2024-12-02T05:50:00Z

pkg/keymanagementprovider/azurekeyvault/types/types.go

@@ -37,4 +37,6 @@ type KeyVaultValue struct {
 	Name string `json:"name" yaml:"name"`
 	// the version of the Azure Key Vault certificate/key
 	Version string `json:"version" yaml:"version"`
+	// the number of versions to keep in the cache
+	VersionHistoryLimit int `json:"versionHistoryLimit" yaml:"versionHistoryLimit"`


if those 2 methods takes KeyVaultValue as receiver or operates on it, I feel they can be added here.

binbin-li · 2024-12-02T05:50:26Z

pkg/keymanagementprovider/azurekeyvault/types/types.go

@@ -37,4 +37,6 @@ type KeyVaultValue struct {
 	Name string `json:"name" yaml:"name"`
 	// the version of the Azure Key Vault certificate/key
 	Version string `json:"version" yaml:"version"`
+	// the number of versions to keep in the cache
+	VersionHistoryLimit int `json:"versionHistoryLimit" yaml:"versionHistoryLimit"`


wonder if the VersionHistoryLimit should be set per key or per provider

Having it at the key/cert level provides more granularity. And it's likely each key/cert will have different requirements for the number of versions to be kept. Imo, having it at this level gives a better user experience.

binbin-li · 2024-12-02T06:02:15Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+}
+
+// GetSortedVersions returns the sorted versions of the object
+func GetSortedVersions(versionHistory []struct {


nit: func GetSortedVersions(sortedVersionHistory

changes: fc8a17ceb14f6358e43225b8627805aec89421b3

shahramk64 · 2024-12-02T21:52:25Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+}
+
+// GetSortedVersions returns the sorted versions of the object
+func GetSortedVersions(versionHistory []struct {


nit: The function name is a bit misleading, the function doesn't do any sorting. When it's called, it's passed with the sorted versions struct. I suggest you change the name to GetVersions.

Changes: fc8a17c#diff-05a95e8187612728112ae655adb4dd1e3e692659bdc8fd6cd6ac5d518f982462R597

shahramk64 · 2024-12-02T21:53:39Z

pkg/keymanagementprovider/azurekeyvault/provider.go

 // GetKey retrieves a key from the keyvault
 func (c *keyKVClientImpl) GetKey(ctx context.Context, keyName string, keyVersion string) (azkeys.GetKeyResponse, error) {
 	return c.Client.GetKey(ctx, keyName, keyVersion, nil)
 }

+func (c *keyKVClientImpl) NewListKeyVersionsPager(name string, options *azkeys.ListKeyVersionsOptions) *runtime.Pager[azkeys.ListKeyVersionsResponse] {


To keep it consistent with the signature of NewListCertificateVersionsPager, rename the first parameter to keyName.

changes: fc8a17c#diff-05a95e8187612728112ae655adb4dd1e3e692659bdc8fd6cd6ac5d518f982462R597

shahramk64 · 2024-12-02T22:06:10Z

pkg/keymanagementprovider/azurekeyvault/provider.go

-				mapKey := keymanagementprovider.KMPMapKey{Name: keyVaultCert.Name, Version: keyVaultCert.Version, Enabled: isEnabled}
-				keymanagementprovider.DeleteCertificateFromMap(s.resource, mapKey)
-				continue
+		// if versionHistoryLimit is not set, set it to default value 1


nit: It'll be neater if you move this logic to line 233 so that setting the limit is done in one place:
// if versionHistoryLimit is not set, set it to default value 1
if keyVaultCert.VersionHistoryLimit == 0 {
keyVaultCert.VersionHistoryLimit = versionHistoryLimitDefault
}
// if versionHistoryLimit is greater than the number of versions, set it to the number of versions
if keyVaultCert.VersionHistoryLimit > len(sortedVersionHistory) {
keyVaultCert.VersionHistoryLimit = len(sortedVersionHistory)
}

#1955 (comment)

shahramk64 · 2024-12-02T22:06:51Z

pkg/keymanagementprovider/azurekeyvault/provider.go

-		keyResponse, err := s.keyKVClient.GetKey(ctx, keyVaultKey.Name, keyVaultKey.Version)
-		if err != nil {
-			return nil, nil, fmt.Errorf("failed to get key objectName:%s, objectVersion:%s, error: %w", keyVaultKey.Name, keyVaultKey.Version, err)
+		// if versionHistoryLimit is not set, set it to default value 1


akashsinghal · 2024-12-03T05:22:49Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+				versionHistory = append(versionHistory, struct {
+					Version string
+					Created time.Time
+				}{Version: cert.ID.Version(), Created: *cert.Attributes.Created})


nit: nil check for cert ?

changes: fc8a17c

pkg/keymanagementprovider/azurekeyvault/provider.go

akashsinghal · 2024-12-03T05:27:35Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+				versionHistory = append(versionHistory, struct {
+					Version string
+					Created time.Time
+				}{Version: key.KID.Version(), Created: *key.Attributes.Created})


nit: nil check for key?

changes: fc8a17c

pkg/keymanagementprovider/azurekeyvault/provider.go

akashsinghal · 2024-12-03T05:30:51Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+				return nil, nil, fmt.Errorf("failed to get key objectName:%s, objectVersion:%s, error: %w", keyVaultKey.Name, version, err)
+			}
+			keyBundle := keyResponse.KeyBundle
+			isEnabled := *keyBundle.Attributes.Enabled


nit: is keyBundle a pointer? if so we might consider a nil check

keyBundle isn't a pointer but the Attributes is. I can add a nil check for that field. Should I also add a nil check for the SecretBundle field in the GetCertificates method?

Added changes for the SecretBundle as well. :) fc8a17c

akashsinghal · 2024-12-03T05:34:08Z

pkg/keymanagementprovider/azurekeyvault/provider_test.go

+		},
+		Fetcher: func(_ context.Context, _ *azkeys.ListKeyVersionsResponse) (azkeys.ListKeyVersionsResponse, error) {
+			var resp azkeys.ListKeyVersionsResponse
+			var keyID azkeys.ID = "https://testkv.vault.azure.net/keys/key1/c1f03df1113d460491d970737dfdc35d"


General comment not specific to this PR: I was reading on some security best practices and it's recommended not to use valid resource IDs or http urls that you don't own unless it's absolutely necessary to testing the functionality

Good to know. Thank you for sharing your learnings. :)

akashsinghal · 2024-12-03T05:34:28Z

pkg/keymanagementprovider/azurekeyvault/provider_test.go

+		},
+		Fetcher: func(_ context.Context, _ *azcertificates.ListCertificateVersionsResponse) (azcertificates.ListCertificateVersionsResponse, error) {
+			var resp azcertificates.ListCertificateVersionsResponse
+			var certID azcertificates.ID = "https://testkv.vault.azure.net/certificates/cert1/c1f03df1113d460491d970737dfdc35d"


akashsinghal · 2024-12-03T05:37:23Z

pkg/keymanagementprovider/azurekeyvault/provider.go

-		secretBundle := secretResponse.SecretBundle
-		isEnabled := *secretBundle.Attributes.Enabled
+		// sort the version history by created time
+		sortVersionHistory(versionHistory)


can you leave a comment in code explaining why a separate method is needed called GetSortedVersions?

akashsinghal · 2024-12-03T05:40:00Z

pkg/keymanagementprovider/azurekeyvault/types/types.go

@@ -37,4 +37,6 @@ type KeyVaultValue struct {
 	Name string `json:"name" yaml:"name"`
 	// the version of the Azure Key Vault certificate/key
 	Version string `json:"version" yaml:"version"`
+	// the number of versions to keep in the cache
+	VersionHistoryLimit int `json:"versionHistoryLimit" yaml:"versionHistoryLimit"`


+1 to binbin's comment

shizhMSFT · 2024-12-04T05:12:03Z

pkg/keymanagementprovider/azurekeyvault/provider.go

@@ -82,12 +85,19 @@ type akvKMProvider struct {
 	certificateKVClient certificateKVClient
 }

+type VersionInfo struct {


quality: request doc for exported struct

shizhMSFT · 2024-12-04T05:12:33Z

pkg/keymanagementprovider/azurekeyvault/provider.go

 // GetKey retrieves a key from the keyvault
 func (c *keyKVClientImpl) GetKey(ctx context.Context, keyName string, keyVersion string) (azkeys.GetKeyResponse, error) {
 	return c.Client.GetKey(ctx, keyName, keyVersion, nil)
 }

+func (c *keyKVClientImpl) NewListKeyVersionsPager(keyName string, options *azkeys.ListKeyVersionsOptions) *runtime.Pager[azkeys.ListKeyVersionsResponse] {


quality: request doc for exported method

pkg/keymanagementprovider/azurekeyvault/provider.go

shizhMSFT · 2024-12-04T05:16:01Z

pkg/keymanagementprovider/azurekeyvault/provider.go

-					return nil, nil, fmt.Errorf("failed to get certificate objectName:%s, objectVersion:%s, error: %w", keyVaultCert.Name, keyVaultCert.Version, err)
+		versionHistory := []VersionInfo{}
+
+		certVersionPager := s.certificateKVClient.NewListCertificateVersionsPager(keyVaultCert.Name, nil)


Question: is the returned page already sorted?
The question behind is that what if we have thousands of versions and we only need the recent 2? It is better to make it O(1) instead of O(n).

Unfortunately, the versions are not already sorted. At least not by created date. I reached out to the azure-sdk-go team and they informed me that the sorted is done by the service itself and currently there isn't any options that can be passed in to change out the pager returns them.

@shizhMSFT also pointed out the latest version is v1.16. https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime. Do we know if the latest API /SDK have improved behaviors?

After looking into the latest version of the runtime package I didn't find any options that can be passed to sort the results of the pager. But there were some comments that indicated that it might be available in future releases. There are other azure-sdk-for-go packages that are deprecated and should be updated as well. Here's a link to an issue tracking that work.

shizhMSFT · 2024-12-04T05:33:39Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+		for _, version := range sortedVersionHistory[len(sortedVersionHistory)-keyVaultCert.VersionHistoryLimit:] {
+			secretReponse, err := s.secretKVClient.GetSecret(ctx, keyVaultCert.Name, version)
+			if err != nil {
+				if isSecretDisabledError(err) {


The info of the Enabled attribute is already returned by NewListCertificateVersionsPager and we can fail fast earlier. Meanwhile, when we say fetch the first n certs, do we mean n certs or n enabled certs?

Excellent call out @shizhMSFT! The NewListCertificateVersionPager does have the Enabled attribute, which can be used instead of the isSecretDisabledError func. However, I don't think we can fail much sooner. n certs means enabled and disabled certs. Meaning we won't know which disabled certs to write to the status until after the pager results are returned and sorted.

I updated the fetchVersionHistory to use the enabled field from the pager to fail faster. However, the single fetch logic remains the same since it doesn't have access to the pager attributes.

susanshi

thanks for much for the PR @duffney. Left some comments, happy to go through the comments and discuss next steps at the community meeting tomorrow.

susanshi · 2024-12-04T05:18:54Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+	ProviderName               string = "azurekeyvault"
+	PKCS12ContentType          string = "application/x-pkcs12"
+	PEMContentType             string = "application/x-pem-file"
+	versionHistoryLimitDefault int    = 1


just a reminder to update CRD doc and sample to capture the default value

@susanshi are there any other places besides the following you'd like updated?

doc

samples

susanshi · 2024-12-04T05:19:42Z

pkg/keymanagementprovider/azurekeyvault/provider.go

@@ -82,12 +85,19 @@ type akvKMProvider struct {
 	certificateKVClient certificateKVClient
 }

+type VersionInfo struct {


suggestion to move VersionInfo to types.go

I've moved this into types.go with the refactor changes and also added methods for sorting. Those changes aren't reflected in the PR just yet waiting for approval from the team on the existing changes and then we can discuss whether or not it makes sense to include the refactor in this PR or in a separate one.

susanshi · 2024-12-04T05:21:33Z

pkg/keymanagementprovider/azurekeyvault/provider.go

 // GetKey retrieves a key from the keyvault
 func (c *keyKVClientImpl) GetKey(ctx context.Context, keyName string, keyVersion string) (azkeys.GetKeyResponse, error) {
 	return c.Client.GetKey(ctx, keyName, keyVersion, nil)
 }

+func (c *keyKVClientImpl) NewListKeyVersionsPager(keyName string, options *azkeys.ListKeyVersionsOptions) *runtime.Pager[azkeys.ListKeyVersionsResponse] {


add description for public methods

susanshi · 2024-12-04T05:31:48Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+		versionHistory := []VersionInfo{}
+
+		certVersionPager := s.certificateKVClient.NewListCertificateVersionsPager(keyVaultCert.Name, nil)
+		for certVersionPager.More() {


Suggestion to pass in empty &azcertificates.ListCertificateVersionsOptions{} , so the reader know that this can be configured

susanshi · 2024-12-04T05:35:52Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+		versionHistory := []VersionInfo{}
+
+		certVersionPager := s.certificateKVClient.NewListCertificateVersionsPager(keyVaultCert.Name, nil)
+		for certVersionPager.More() {


the logic to fetch version seem contained, suggestion to extract into a "getVersion" method so its more unit testable.

for certVersionPager.More() { }

Agreed. The GetCertificate and GetKeys methods are getting a bit unwieldy and the tests are over a hundred lines. I'll work on refactoring the logic and add additional methods to make it easier to understand and test.

susanshi · 2024-12-04T05:40:07Z

pkg/keymanagementprovider/azurekeyvault/provider.go

-			return nil, nil, fmt.Errorf("failed to get key from key bundle:%w", err)
+		// if versionHistoryLimit isn't set default to 1 to avoid out of bounds error
+		if keyVaultKey.VersionHistoryLimit == 0 {
+			keyVaultKey.VersionHistoryLimit = versionHistoryLimitDefault


suggestion to add a log that we have updated the VersionHistoryLimit

Instead of using this if for setting the default it's now used for triggering the backwards compatibility logic to fetch a single version.

susanshi · 2024-12-04T05:41:25Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+
+		// Pager results are not sorted by created time, so we sort them here
+		// in ascending order (oldest to newest)
+		sortVersionHistory(versionHistory)


should we combine sortVersionHistory and GetVersions

I think so, I'd like to move the type into types.go and then make the sorting a method of that new type.

susanshi · 2024-12-04T05:47:41Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+		}
+
+		// get the latest version of the certificate up to the limit
+		for _, version := range sortedVersionHistory[len(sortedVersionHistory)-keyVaultKey.VersionHistoryLimit:] {


how do we handle the scenario where keyVaultKey.VersionHistoryLimit and keyVaultKey.Version are both provided in the CR? especially if Version points to a older version that is not within the limit of the VersionHistory range

how do we handle the scenario where keyVaultKey.VersionHistoryLimit and keyVaultKey.Version are both provided in the CR? especially if Version points to a older version that is not within the limit of the VersionHistory range

It depends. Do we want to support the following options?

Store multiple specific versions:

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify versions: - "1" - "2" - "5"

Store specified version and multiple previous versions

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify versions: - "10" versionHistoryLimit: 2

Store latest version and multiple previous versions

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify versionHistoryLimit: 2

Or a single specified version with the version field or latest plus nversions with the limit specified.

Store a single version:

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify versions: "1"

Store latest and nversions:

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify versionHistoryLimit: 3

We want to support both:

when no version is specified, get 2 version including the latest

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify versionHistoryLimit: 2

spec: type: azurekeyvault refreshInterval: 1m parameters: vaultURI: https://ratifyakvs5r.vault.azure.net/ certificates: - name: ratify version: "10" # single version that can be manually specified as the "tail" and goback n versionHistoryLimit: 2

In this example, if Dec9 is the specified version , with a limit of 2, we will fetch Dec8 and Dec9

Dec10
Dec9
Dec8

susanshi · 2024-12-04T05:50:53Z

pkg/keymanagementprovider/azurekeyvault/provider.go

+
+		// get the latest version of the certificate up to the limit
+		for _, version := range sortedVersionHistory[len(sortedVersionHistory)-keyVaultCert.VersionHistoryLimit:] {
+			secretReponse, err := s.secretKVClient.GetSecret(ctx, keyVaultCert.Name, version)


To maintain backward cap, if CR only specified a single version, we could do a direct Get avoiding thte List permission required for the pager.

susanshi · 2024-12-04T06:16:44Z

pkg/keymanagementprovider/azurekeyvault/provider_test.go

 	}{
+		{
+			name:                "Key enabled with multiple versions",
+			versionHistoryLimit: 3,


Given the method now have additional logic around pager, sorting etc, the test should validate the length of the response and which versions are returned. Suggestion to update the current validation. If there is a error thrown, we should also validate if its the expected error msg.

_, _, err := provider.GetKeys(context.Background())

Hey @susanshi I addressed this in a different branch as part of a refactor. Do you want those changes pulled into this PR? Or is it okay to include them in a follow up PR with the refactoring?

duffney · 2024-12-04T19:40:59Z

Discussion: Should the user be able to define multiple specific version of a certificate?

spec:
  type: azurekeyvault
  refreshInterval: 1m
  parameters:
    vaultURI: https://ratifyakvs5r.vault.azure.net/
    certificates:
      - name: ratify
        versions:
          - "1"
          - "2"
          - "5"

Signed-off-by: Josh Duffney <[email protected]> Update pkg/keymanagementprovider/azurekeyvault/provider.go Co-authored-by: Akash Singhal <[email protected]> Signed-off-by: Josh Duffney <[email protected]> Update pkg/keymanagementprovider/azurekeyvault/provider.go Co-authored-by: Akash Singhal <[email protected]> Signed-off-by: Josh Duffney <[email protected]> Update pkg/keymanagementprovider/azurekeyvault/provider.go Co-authored-by: Akash Singhal <[email protected]> Signed-off-by: Josh Duffney <[email protected]> mod: replace literal struct with VersionInfo mod: combine versionHistory len and value checking fix: handle nil pointers Update pkg/keymanagementprovider/azurekeyvault/provider.go Co-authored-by: Shiwei Zhang <[email protected]> Signed-off-by: Josh Duffney <[email protected]> mod: add desc to public methods fix: backwards compatability for single cert fetching without list permissions fix: const for rawResponse mod: support version as tail in versionHistory mod: rm trailing whitespace Signed-off-by: Josh Duffney <[email protected]>

duffney · 2024-12-10T21:20:06Z

@susanshi @binbin-li @akashsinghal the PR is ready for a final round of review. :)

akashsinghal · 2024-12-11T19:43:55Z

@susanshi @binbin-li @akashsinghal the PR is ready for a final round of review. :)

Thanks for addressing my comments. PR looks good to me once other's comments are addressed.

duffney · 2024-12-13T16:24:52Z

Just a side note: I completed the refactoring work in a separate branch. If everyone is good with the changes in this PR, I'll submit the refactoring as a different PR for review. The functionality doesn't change at all. I simply added additional methods and functions to improve readability and unit testing.

duffney force-pushed the issue-1751/nversion branch from ccf970c to 7fbc3e2 Compare November 27, 2024 20:47

duffney commented Nov 28, 2024

View reviewed changes

binbin-li reviewed Dec 2, 2024

View reviewed changes

duffney marked this pull request as ready for review December 2, 2024 14:09

duffney requested review from akashsinghal, jimmyraywv, luisdlp, susanshi and toddysm as code owners December 2, 2024 14:09

shahramk64 reviewed Dec 2, 2024

View reviewed changes

akashsinghal reviewed Dec 3, 2024

View reviewed changes

shizhMSFT reviewed Dec 4, 2024

View reviewed changes

susanshi reviewed Dec 4, 2024

View reviewed changes

duffney force-pushed the issue-1751/nversion branch from 3672ca0 to 562b5be Compare December 10, 2024 21:07

feat: add nversion support to azurekeyvault provider certificates and keys #1955

Are you sure you want to change the base?

feat: add nversion support to azurekeyvault provider certificates and keys #1955

Conversation

duffney commented Nov 27, 2024 • edited Loading

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Type of change

How Has This Been Tested?

Checklist:

Post Merge Requirements

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

duffney Nov 28, 2024 • edited Loading

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

duffney Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duffney Dec 3, 2024 • edited Loading

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

duffney Dec 4, 2024 • edited Loading

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

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

susanshi 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

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

duffney commented Dec 4, 2024 • edited Loading

duffney commented Nov 27, 2024 •

edited

Loading

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Nov 27, 2024 •

edited

Loading

duffney Nov 28, 2024 •

edited

Loading

duffney Dec 3, 2024 •

edited

Loading

duffney Dec 3, 2024 •

edited

Loading

duffney Dec 4, 2024 •

edited

Loading

duffney commented Dec 4, 2024 •

edited

Loading