azurerm_cosmosdb_account: Fix submitted key_vault_key_id values

[tism]

Fixes #10379 by removing the doubled // (left the trailing / on KeyVaultBaseUrl) and using a versionless ID for azurerm_cosmosdb_account.key_vault_key_id. Not sure how well LatestVersionID fits into the NewxxID()/ID() parse package pattern.

[tombuildsstuff]

hey @tism

Thanks for this PR - I've taken a look through and left some comments inline. On the whole this looks pretty good, but in retrospect perhaps we're being slightly misleading with the key_vault_key_id field, if we're requiring a versionless ID for the Key Vault Key, it'd be worth updating validation for this field to require this, rather than throwing away the version (and showing it in the plan) - WDYT?

Thanks!

[tombuildsstuff]

to be honest we could arguably change this to be:

Suggested change

	return formatID([]string{strings.TrimSuffix(n.KeyVaultBaseUrl, "/"), n.NestedItemType, n.Name, n.Version})
	segments := []string{
	strings.TrimSuffix(n.KeyVaultBaseUrl, "/"),
	n.Name,
	}
	if n.Version != "" {
	segments = append(segments, n.Version)
	}
	return strings.TrimPrefix(strings.Join(segments, "/"), "/")

[tism]

👍 That'll also highlight if the struct was somehow populated incorrectly with missing values like name

[tombuildsstuff]

whilst there's some use-cases for this, Terraform shouldn't be using this directly (unless a users specifying it) - since this means the apply value for this could drift from the plan value - as such consolidating this into ID above should handle this.

This means we could update the key_vault_key_id field within the CosmosDB Account resource to take a versionless ID, by adding a Versionless validation function in azurerm/internal/services/keyvault/validate/nested_item_id.go - meaning what users pass in (the Versionless ID is what gets sent, ensuring the Plan matches the Apply).

This'd also need the versionless_id added to the azurerm_key_vault_key resource to be fair, but would make this more consistent in general - WDYT?

[tism]

I like that, it makes the interface a lot clearer and the resource isn't making changes to the value under the covers.

[tism]

While running the key vault acceptance tests I couldn't get it to stop trying to purge the soft deleted keys, so anything that uses key_vault_uri will leave dangling vaults.

I thought purge_soft_delete_on_destroy = false would be enough to let the vault destroy pass and that is fine locally. Is there something different about how the acceptance tests are run?

[tombuildsstuff]

I thought purge_soft_delete_on_destroy = false would be enough to let the vault destroy pass and that is fine locally. Is there something different about how the acceptance tests are run?

This'll be due to a bug in the Plugin SDK where at this time the test framework doesn't honour the provider blocks - it's something we're working to fix - for the moment if this leaves some dangling resources during test cleanup this is fine, as they'll be caught by our Dalek (cleanup script).

[katbyte]

Thanks @tism - LGTM 👍

[ghost]

This has been released in version 2.47.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.47.0"
}
# ... other configuration ...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!