azurerm_key_vault: use Resource Graph API to search keyvault resource from keyvault name
#23413
Conversation
…ource with a filter
There was a problem hiding this comment.
Hi @wuxu92, This is looking really good so far, however I have left a few comments and suggestions on the code. It LGTM more or less, we will have to see if @jackofallops and @tombuildsstuff agrees. 🚀
| @@ -111,6 +113,32 @@ func (c *Client) Exists(ctx context.Context, keyVaultId commonids.KeyVaultId) (b | |||
| return true, nil | |||
| } | |||
|
|
|||
| func (c *Client) KeyVaultIDFromBaseURLByGraph(ctx context.Context, client *graph.ResourcesClient, name string) (string, interface{}) { | |||
There was a problem hiding this comment.
should the return signature for this function be just *string?
| func (c *Client) KeyVaultIDFromBaseURLByGraph(ctx context.Context, client *graph.ResourcesClient, name string) (string, interface{}) { | |
| func (c *Client) KeyVaultIDFromBaseURLByGraph(ctx context.Context, client *graph.ResourcesClient, name string) *string { |
There was a problem hiding this comment.
I think (string, error) should be fine. I made a mistake here to define the second value as interface{}.
| @@ -111,6 +113,32 @@ func (c *Client) Exists(ctx context.Context, keyVaultId commonids.KeyVaultId) (b | |||
| return true, nil | |||
| } | |||
|
|
|||
| func (c *Client) KeyVaultIDFromBaseURLByGraph(ctx context.Context, client *graph.ResourcesClient, name string) (string, interface{}) { | |||
| if name == "" { | |||
| return "", nil | |||
There was a problem hiding this comment.
| return "", nil | |
| return nil |
| } | ||
| resp, err := client.Resources(ctx, req) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
| return "", err | |
| return nil |
| } | ||
| } | ||
| } | ||
| return "", fmt.Errorf("no such keyvault in resource graph by query: %s", req.Query) |
There was a problem hiding this comment.
| return "", fmt.Errorf("no such keyvault in resource graph by query: %s", req.Query) | |
| return nil |
| @@ -130,6 +158,15 @@ func (c *Client) KeyVaultIDFromBaseUrl(ctx context.Context, resourcesClient *res | |||
| return &v.keyVaultId, nil | |||
| } | |||
|
|
|||
| // try to get the keyVault id by resource graph API first to void the expensive ResourcesClient.List operation | |||
| // if it cannot fetch the keyvault from resource graph API, it will fall back to the resourceClient.List method | |||
| if idStr, err := c.KeyVaultIDFromBaseURLByGraph(ctx, resourcesClient.GraphResourceClient, *keyVaultName); err == nil && idStr != "" { | |||
There was a problem hiding this comment.
With the changes above idStr will always return nil if it was not successful for any reason. Should we only check for idStr not being nil here? Makes it a little easier to read maybe?
| if idStr, err := c.KeyVaultIDFromBaseURLByGraph(ctx, resourcesClient.GraphResourceClient, *keyVaultName); err == nil && idStr != "" { | |
| if idStr := c.KeyVaultIDFromBaseURLByGraph(ctx, resourcesClient.GraphResourceClient, *keyVaultName); idStr != nil { |
| // try to get the keyVault id by resource graph API first to void the expensive ResourcesClient.List operation | ||
| // if it cannot fetch the keyvault from resource graph API, it will fall back to the resourceClient.List method | ||
| if idStr, err := c.KeyVaultIDFromBaseURLByGraph(ctx, resourcesClient.GraphResourceClient, *keyVaultName); err == nil && idStr != "" { | ||
| if id, err := commonids.ParseKeyVaultID(idStr); err == nil && strings.EqualFold(id.VaultName, *keyVaultName) { |
There was a problem hiding this comment.
| if id, err := commonids.ParseKeyVaultID(idStr); err == nil && strings.EqualFold(id.VaultName, *keyVaultName) { | |
| if id, err := commonids.ParseKeyVaultID(pointer.From(idStr)); err == nil && strings.EqualFold(id.VaultName, pointer.From(keyVaultName)) { |
| // if it cannot fetch the keyvault from resource graph API, it will fall back to the resourceClient.List method | ||
| if idStr, err := c.KeyVaultIDFromBaseURLByGraph(ctx, resourcesClient.GraphResourceClient, *keyVaultName); err == nil && idStr != "" { | ||
| if id, err := commonids.ParseKeyVaultID(idStr); err == nil && strings.EqualFold(id.VaultName, *keyVaultName) { | ||
| c.AddToCache(*id, keyVaultBaseUrl) |
There was a problem hiding this comment.
| c.AddToCache(*id, keyVaultBaseUrl) | |
| c.AddToCache(pointer.From(id), keyVaultBaseUrl) |
| } | ||
|
|
||
| if resp.Model != nil && resp.Model.Data != nil { | ||
| if list, ok := resp.Model.Data.([]interface{}); ok && len(list) == 1 { |
There was a problem hiding this comment.
What if it returns 0 or 2? Is returning the no such keyvault in resource graph by query the correct behavior here?
There was a problem hiding this comment.
There shoud be 1 or 0 resource for keyvault resource of the same name.
| if res, ok := list[0].(map[string]interface{}); ok { | ||
| if id, ok := res["id"]; ok { | ||
| if idStr, ok := id.(string); ok { | ||
| return idStr, nil |
There was a problem hiding this comment.
| return idStr, nil | |
| return pointer.To(idStr) |
|
hey @wuxu92 Thanks for this PR - apologies for the delayed review here. Whilst we've investigated the That said, as a part of reviewing both the background for this PR, this PR and reviewing what's left to migrate off of As a part of that I've reviewed the caching setup and realised that we can take a different approach with this one, rather than loading the Key Vaults on demand when populating the cache (one at a time) - since we're hitting the Azure API to get the information for the specified Key Vault - we might as well pull the entire list into the cache at the same time prior to filtering the cache for the Key Vault in question. This means that we can use the more up to date As such whilst I'd like to thank you for this contribution I'm going to close this PR for the moment, as I believe that #24019 will solve #23404 (and the other issues) albeit in a different manner. Thanks! |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Currently the resourceClient.List API is low performance and with high probability of rasing a
context deadline exceededif the subscription has over thousands of resources.As there are several PRs try to migrate this logic to ARG before (like #13719 but closed at last), this PR is only use ARG as a high pri option and it will fall back to origin logic when ARG fails. And this PR is not to fix the issue of keyvault eventual consistant, but only to improve the performance of the
KeyVaultIDFromBaseUrlfunction, Because this function is prone to causing thecontext deadline exceederror on the production environment of our customers.To avoid breaking the
KeyVaultIDFromBaseUrlsignature, I placed the ResourceGraph Client underresourcesClient.Client. This way, we won't have to make changes to multiple files that depend on it.This should be a long-term fix to #23404.