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

Remove parameter when retrieving the key vault id via Resource API #13409

Closed
wants to merge 1 commit into from

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Sep 18, 2021

The purpose of this PR:

  1. When investigating issue 11059, I found that using “top” parameter in Resource API will have performance issue, when retrieving key vault id. We could get the key vault directly by specifying the “resourceType” and “name” only. If it exists, since the key vault name is global unique in a sub, there will be only one match returned. To proof the theory, I did a test to set a secret for an existing key vault. Below are the results:

    • Prerequisites:

      • The number of resources under my test subscription: 183
      • The position of the existing key vault in the resource list: 91
    • Results:

      • Set “top” to 5 then get the key vault after 19 requests sent by calling “Next” URI
      • Set “top” to nil then get the key vault after one request
  2. Many customers have the similar issue with 11059, but we have tried many times and have not been able to reproduce it. Per the log and information provided by customer, I found:

    • When the secret is read again after setting it, the Key Vault cannot be found through the Resource API (but it did exist after the customer confirmed) ,which caused the error mentioned in issue 11059.
    • Retrieve the key vault ID by Resource API, the requests are hundreds and not found specified key vault finally.

So, this PR can improve the performance a lot ,reduce the chance to meet the issue dramatically and maybe fix issue 11059(cannot verify because it cannot be reproduced).

Test Results (The failed testcases report the same error before modification):
--- PASS: TestAccKustoClusterCustomerManagedKey_userIdentity (1466.42s)
--- PASS: TestAccKustoClusterCustomerManagedKey_basic (1476.59s)
--- PASS: TestAccKustoClusterCustomerManagedKey_requiresImport (1613.76s)
--- PASS: TestAccKustoClusterCustomerManagedKey_updateKey (1665.89s)
--- PASS: TestAccDatabricksWorkspace_basic (649.91s)
--- PASS: TestAccDatabricksWorkspace_infrastructureEncryption (650.00s)
--- PASS: TestAccDatabricksWorkspace_loadBalancerSecureClusterConnectivity (807.88s)
--- PASS: TestAccDatabricksWorkspace_secureClusterConnectivity (824.62s)
--- PASS: TestAccDatabricksWorkspace_managedServicesAndDbfsCMK (840.12s)
--- PASS: TestAccDatabricksWorkspace_managedServices (852.94s)
--- PASS: TestAccDatabricksWorkspace_machineLearning (934.09s)
--- PASS: TestAccDatabricksWorkspace_complete (1041.71s)
--- PASS: TestAccDatabricksWorkspace_privateLink (1054.53s)
--- PASS: TestAccDatabricksWorkspace_updateSKU (1195.65s)
--- PASS: TestAccDatabricksWorkspace_requiresImport (614.18s)
--- PASS: TestAccDatabricksWorkspace_managedServicesDbfsCMKAndPrivateLink (1267.79s)
--- PASS: TestAccDatabricksWorkspace_sameName (615.21s)
--- PASS: TestAccDatabricksWorkspace_update (1096.10s)
--- PASS: TestAccKeyVaultCertificateIssuer_complete (336.53s)
--- PASS: TestAccKeyVaultCertificateIssuer_basic (362.05s)
--- PASS: TestAccKeyVaultCertificateIssuer_disappears (380.55s)
--- PASS: TestAccKeyVaultCertificateIssuer_requiresImport (436.91s)
--- PASS: TestAccKeyVaultCertificateIssuer_update (489.20s)
--- PASS: TestAccDataSourceKeyVaultKey_complete (433.25s)
--- PASS: TestAccKeyVaultKey_disappearsWhenParentKeyVaultDeleted (313.53s)
--- PASS: TestAccKeyVaultKey_purge (351.26s)
--- PASS: TestAccKeyVaultKey_complete (354.84s)
--- PASS: TestAccKeyVaultKey_basicEC (371.14s)
--- PASS: TestAccKeyVaultKey_basicECHSM (409.62s)
--- PASS: TestAccKeyVaultKey_requiresImport (430.33s)
--- PASS: TestAccKeyVaultKey_curveEC (433.03s)
--- PASS: TestAccKeyVaultKey_curveECDeprecated (433.14s)
--- PASS: TestAccKeyVaultKey_update (489.96s)
--- PASS: TestAccKeyVaultKey_updatedExternally (558.87s)
--- PASS: TestAccKeyVaultKey_basicRSAHSM (302.16s)
--- PASS: TestAccKeyVaultKey_disappears (327.10s)
--- PASS: TestAccKeyVaultKey_basicRSA (315.10s)
--- PASS: TestAccKeyVaultKey_withExternalAccessPolicy (426.86s)
--- PASS: TestAccKeyVaultKey_softDeleteRecovery (873.31s)
--- PASS: TestAccKeyVaultSecret_basic (349.26s)
--- PASS: TestAccKeyVaultSecret_complete (355.82s)
--- PASS: TestAccKeyVaultSecret_disappearsWhenParentKeyVaultDeleted (364.68s)
--- PASS: TestAccKeyVaultSecret_purge (367.13s)
--- PASS: TestAccKeyVaultSecret_disappears (392.22s)
--- PASS: TestAccKeyVaultSecret_requiresImport (440.20s)
--- PASS: TestAccKeyVaultSecret_withExternalAccessPolicy (454.11s)
--- PASS: TestAccKeyVaultSecret_update (468.02s)
--- PASS: TestAccKeyVaultSecret_updatingValueChangedExternally (471.92s)
--- PASS: TestAccKeyVaultSecret_recovery (849.96s)
--- PASS: TestAccDatabricksWorkspaceCustomerManagedKey_requiresImport (717.21s)
--- PASS: TestAccDatabricksWorkspaceCustomerManagedKey_basic (724.66s)
--- PASS: TestAccDatabricksWorkspaceCustomerManagedKey_remove (756.50s)
--- PASS: TestAccDatabricksWorkspaceCustomerManagedKey_noIp (898.99s)
--- PASS: TestAccKeyVaultCertificate_disappearsWhenParentKeyVaultDeleted (311.94s)
--- PASS: TestAccKeyVaultCertificate_basicGenerate (354.96s)
--- PASS: TestAccKeyVaultCertificate_emptyExtendedKeyUsage (366.03s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateSans (370.79s)
--- PASS: TestAccKeyVaultCertificate_basicImportPFX (379.70s)
--- PASS: TestAccKeyVaultCertificate_disappears (401.14s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateTags (412.31s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateEllipticCurveAutoKeySize (426.83s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateUnknownIssuer (432.10s)
--- PASS: TestAccKeyVaultCertificate_requiresImport (449.36s)
--- PASS: TestAccKeyVaultCertificate_basicExtendedKeyUsage (310.07s)
--- PASS: TestAccKeyVaultCertificate_purge (391.22s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateEllipticCurve (377.67s)
--- PASS: TestAccKeyVaultCertificate_withExternalAccessPolicy (511.66s)
--- PASS: TestAccKeyVaultCertificate_softDeleteRecovery (885.20s)
--- PASS: TestAccDataFactoryLinkedServiceKeyVault_basic (353.32s)
--- PASS: TestAccDataFactoryLinkedServiceKeyVault_update (362.12s)
--- PASS: TestAccPostgreSQLServerKey_basic (437.57s)
--- PASS: TestAccPostgreSQLServerKey_requiresImport (469.55s)
--- PASS: TestAccPostgreSQLServerKey_updateKey (548.71s)
--- PASS: TestAccStorageAccountCustomerManagedKey_userAssignedIdentity (363.23s)
--- PASS: TestAccStorageAccountCustomerManagedKey_requiresImport (448.50s)
--- PASS: TestAccStorageAccountCustomerManagedKey_updateKey (448.56s)
--- PASS: TestAccStorageAccountCustomerManagedKey_basic (492.95s)
--- PASS: TestAccStorageAccountCustomerManagedKey_testKeyVersion (509.63s)
--- PASS: TestAccAppServiceCertificate_PfxNoPassword (128.04s)
--- PASS: TestAccAppServiceCertificate_Pfx (181.94s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateEllipticCurve (332.87s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateTags (345.03s)
--- PASS: TestAccKeyVaultCertificate_emptyExtendedKeyUsage (387.03s)
--- PASS: TestAccKeyVaultCertificate_basicGenerate (392.98s)
--- PASS: TestAccKeyVaultCertificate_basicExtendedKeyUsage (398.25s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateSans (399.21s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateEllipticCurveAutoKeySize (411.52s)
--- PASS: TestAccKeyVaultCertificate_purge (415.09s)
--- PASS: TestAccKeyVaultCertificate_basicImportPFX (418.63s)
--- PASS: TestAccKeyVaultCertificate_withExternalAccessPolicy (539.30s)
--- PASS: TestAccKeyVaultCertificate_disappears (289.93s)
--- PASS: TestAccKeyVaultCertificate_basicGenerateUnknownIssuer (375.64s)
--- PASS: TestAccKeyVaultCertificate_disappearsWhenParentKeyVaultDeleted (324.03s)
--- PASS: TestAccKeyVaultCertificate_requiresImport (326.79s)
--- PASS: TestAccKeyVaultCertificate_softDeleteRecovery (832.37s)
--- PASS: TestAccKeyVaultCertificateIssuer_complete (285.79s)
--- PASS: TestAccKeyVaultCertificateIssuer_disappears (340.07s)
--- PASS: TestAccKeyVaultCertificateIssuer_basic (353.33s)
--- PASS: TestAccKeyVaultCertificateIssuer_requiresImport (374.66s)
--- PASS: TestAccKeyVaultCertificateIssuer_update (376.56s)
--- PASS: TestAccMySQLServerKey_basic (425.81s)
--- PASS: TestAccMySQLServerKey_updateKey (468.78s)
--- PASS: TestAccMySQLServerKey_requiresImport (486.28s)

--- FAIL: TestAccDiskEncryptionSet_keyRotate (499.69s)
--- FAIL: TestAccAppServiceCertificate_KeyVault (20.58s)

@@ -113,7 +113,7 @@ func (c *Client) KeyVaultIDFromBaseUrl(ctx context.Context, resourcesClient *res
}

filter := fmt.Sprintf("resourceType eq 'Microsoft.KeyVault/vaults' and name eq '%s'", *keyVaultName)
result, err := resourcesClient.ResourcesClient.List(ctx, filter, "", utils.Int32(5))
result, err := resourcesClient.ResourcesClient.List(ctx, filter, "", nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sinbai this query above is filtering for the resource type and exact name, with a maximum of 5 results - since this is an exact match there should only ever be a single result (or no results) returned - removing the value for the top parameter will instead return (I believe) 50-100 results by default.

As such I'm a little confused as to how this change will change things, given we're expecting a single result here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the behavior of the Resource API is different with or without the top parameter. The resource API that uses top+filter first top x from all the resources and then filter them in the top x resources, it will return {"value":[],"nextLink":"xxxxx"} if it does not exist in the first top x. The number of request depends on quantity of resources and the location of the retrieved resource. if there is no top parameter, it will return the value(or no results) directly, and only one request is required. Try the following link may be the proof of above:
https://docs.microsoft.com/en-us/rest/api/resources/resources/list#code-try-0

Choose a reason for hiding this comment

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

Totally agree with a @sinbai

query with a $top returns {{"value": []}, "nextLink": "url"}
query without $top returns {"value": "url" },

I will open a ticket to Azure, but I want to know why it's used.
We put a strict name of Azure Key Vault in the query
There is no chance to create multiple resources with the same name under the same subscription

This comment was marked as off-topic.

@bogdangrigg
Copy link

bogdangrigg commented Oct 8, 2021

I had a similar problem to the one described in #11059
And I can confirm that it was solved by custom build using @sinbai's fix.

I had also tried one without it (based on main / bba0cba) & it didn't help.

Terraform v1.0.7
AzureRM: terraform-provider-azurerm_v2.80.0_x5

@jawhites

This comment has been minimized.

@sinbai

This comment has been minimized.

@tombuildsstuff
Copy link
Contributor

@sinbai for the purposes of this example, since we have more of them I'll query for VirtualNetworks - although I'm seeing the same results regardless of the resource type (Key Vaults / API Management etc) - first listing all resources of the Resource Type with $type set:

Request:

GET /subscriptions/00000000-0000-0000-0000-000000000000/resources?api-version=2020-06-01&%24filter=resourceType%20eq%20%27Microsoft.Network%2FvirtualNetworks%27&%24top=5 HTTP/1.1
Host: management.azure.com
User-Agent: Charles/4.6.2

Response:

{
	"value": [{
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/aaacctest-tbamford/providers/Microsoft.Network/virtualNetworks/aadds-vnet-01",
		"name": "aadds-vnet-01",
		"type": "Microsoft.Network/virtualNetworks",
		"location": "westeurope"
	}, {
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG2-sql-211115020716086717/providers/Microsoft.Network/virtualNetworks/acctest-vnet2-211115020716086717",
		"name": "acctest-vnet2-211115020716086717",
		"type": "Microsoft.Network/virtualNetworks",
		"location": "eastus2",
		"tags": {}
	}, {
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-200217082404268777/providers/Microsoft.Network/virtualNetworks/acctestvn-200217082404268777",
		"name": "acctestvn-200217082404268777",
		"type": "Microsoft.Network/virtualNetworks",
		"location": "westeurope",
		"tags": {}
	}, {
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-210305010814773237/providers/Microsoft.Network/virtualNetworks/acctest-vnet-210305010814773237",
		"name": "acctest-vnet-210305010814773237",
		"type": "Microsoft.Network/virtualNetworks",
		"location": "westeurope",
		"tags": {}
	}, {
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-211115002130907613/providers/Microsoft.Network/virtualNetworks/acctest-vnet-211115002130907613",
		"name": "acctest-vnet-211115002130907613",
		"type": "Microsoft.Network/virtualNetworks",
		"location": "westeurope",
		"tags": {}
	}],
	"nextLink": "https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resources?api-version=2020-06-01&%24filter=resourceType+eq+%27Microsoft.Network%2fvirtualNetworks%27&%24top=5&%24skiptoken=XXXXXXX"
}

and then filtering using the Resource Type with the Resource Name:

Request:

GET /subscriptions/00000000-0000-0000-0000-000000000000/resources?api-version=2020-06-01&%24filter=resourceType%20eq%20%27Microsoft.Network%2FvirtualNetworks%27%20and%20name%20eq%20%27acctest-vnet-210128035907504856%27&%24top=5 HTTP/1.1
Host: management.azure.com
User-Agent: Charles/4.6.2

Response:

HTTP/1.1 200 OK
Cache-Control: no-cache
Pragma: no-cache
Content-Type: application/json; charset=utf-8
Expires: -1
x-ms-ratelimit-remaining-subscription-reads: 11999
x-ms-request-id: 712544b0-cd5e-4788-a7b8-6bf790046ef0
x-ms-correlation-request-id: 712544b0-cd5e-4788-a7b8-6bf790046ef0
x-ms-routing-request-id: GERMANYWESTCENTRAL:20211115T122347Z:712544b0-cd5e-4788-a7b8-6bf790046ef0
Strict-Transport-Security: max-age=31536000; includeSubDomains
X-Content-Type-Options: nosniff
Date: Mon, 15 Nov 2021 12:23:47 GMT
Content-Length: 314
Connection: keep-alive

{
	"value": [{
		"location": "westeurope",
		"type": "Microsoft.Network/virtualNetworks",
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-hsm-210128035907504856/providers/Microsoft.Network/virtualNetworks/acctest-vnet-210128035907504856",
		"name": "acctest-vnet-210128035907504856",
		"tags": {}
	}]
}

The behaviour remains the same without the $top set to, for example here's the list all operation:

Request

GET /subscriptions/00000000-0000-0000-0000-000000000000/resources?api-version=2020-06-01&%24filter=resourceType%20eq%20%27Microsoft.Network%2FvirtualNetworks%27 HTTP/1.1
Host: management.azure.com
User-Agent: Charles/4.6.2

Response

HTTP/1.1 200 OK
Cache-Control: no-cache
Pragma: no-cache
Content-Type: application/json; charset=utf-8
Expires: -1
x-ms-ratelimit-remaining-subscription-reads: 11999
x-ms-request-id: 7e5b9554-5374-4430-8c6a-989196cf4557
x-ms-correlation-request-id: 7e5b9554-5374-4430-8c6a-989196cf4557
x-ms-routing-request-id: GERMANYNORTH:20211115T123457Z:7e5b9554-5374-4430-8c6a-989196cf4557
Strict-Transport-Security: max-age=31536000; includeSubDomains
X-Content-Type-Options: nosniff
Date: Mon, 15 Nov 2021 12:34:57 GMT
Content-Length: 16830
Connection: keep-alive

{
	"value": [{
			"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/aaacctest-tbamford/providers/Microsoft.Network/virtualNetworks/aadds-vnet-01",
			"name": "aadds-vnet-01",
			"type": "Microsoft.Network/virtualNetworks",
			"location": "westeurope"
		},
		{
			"//": "lots more"
		},
		{
			"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/tbamford-auth-testing-donotdelete_group/providers/Microsoft.Network/virtualNetworks/tbamford-auth-testing-donotdelete_group-vnet",
			"name": "tbamford-auth-testing-donotdelete_group-vnet",
			"type": "Microsoft.Network/virtualNetworks",
			"location": "westeurope",
			"tags": {
				"Environment": "test"
			}
		}
	]
}

and when filtering for the Resource Type and Name specifically:

Request

GET /subscriptions/00000000-0000-0000-0000-000000000000/resources?api-version=2020-06-01&%24filter=resourceType%20eq%20%27Microsoft.Network%2FvirtualNetworks%27%20and%20name%20eq%20%27aadds-vnet-01%27 HTTP/1.1
Host: management.azure.com
User-Agent: Charles/4.6.2

Response

HTTP/1.1 200 OK
Cache-Control: no-cache
Pragma: no-cache
Content-Type: application/json; charset=utf-8
Expires: -1
x-ms-ratelimit-remaining-subscription-reads: 11999
x-ms-request-id: 285a74be-2bcc-4309-a4a1-5487ba883f4f
x-ms-correlation-request-id: 285a74be-2bcc-4309-a4a1-5487ba883f4f
x-ms-routing-request-id: GERMANYWESTCENTRAL:20211115T123934Z:285a74be-2bcc-4309-a4a1-5487ba883f4f
Strict-Transport-Security: max-age=31536000; includeSubDomains
X-Content-Type-Options: nosniff
Date: Mon, 15 Nov 2021 12:39:34 GMT
Content-Length: 254
Connection: keep-alive

{
	"value": [{
		"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/aaacctest-tbamford/providers/Microsoft.Network/virtualNetworks/aadds-vnet-01",
		"name": "aadds-vnet-01",
		"type": "Microsoft.Network/virtualNetworks",
		"location": "westeurope"
	}]
}

Again this behaviour is consistent regardless of the Resource Type being specified - as such if there's a single matching item in the API, this'll get returned without a nextLink (it can also be null meaning the same thing) - at which point internally the Azure SDK won't poll on the Future - meaning this'll get returned in a single hit.

Since this issue is intermittent by design - unfortunately the comments above are false positives - based on the comments in #11059 it appears that this is instead an issue with the API returning a 429 and the Azure SDK for Go not handling this correctly, as such this would need to be fixed there instead.

However for the moment since this contribution unfortunately won't fix this issue - whilst I'd like to thank you for this contribution, I'm going to close this issue for the moment.

Thanks!

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2021
@sinbai sinbai deleted the keyvault/removetopparameter branch January 7, 2022 01:40
@magodo
Copy link
Collaborator

magodo commented Sep 27, 2023

@tombuildsstuff I assume that is because you didn't specify the name in your filter, if you specified a name eq <some vnet>, then you'll observe the behavior as @sinbai described.

Update [Oct.12]: My above information is outdated, which is due to a ARM new feature introduced and later reverted..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants