-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix handling of creating an Access Policy for a non-existent Key Vault #2922
Fix handling of creating an Access Policy for a non-existent Key Vault #2922
Conversation
This commit adds an acceptance test reproducing the scenario of attempting to create an `azurerm_key_vault_access_policy` resource when specifying the ID of a Key Vault which does not exist, but is in the correct URI-path-style format. It generates these by appending the string "NOPE" to the ID of a valid Key Vault (which the tests creates), but could equally truncate the name (which is how this was encountered in the first place). Applying this particular configuration, I expect an error - probably of the form "Error retrieving Key Vault" (but the exact text is arbitrary). Instead, we see that no error is produced. If this scenario is run via the CLI, the CLI marks the resource as successfully created, but no state is created for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jen20,
Thank you for the bugfix. While this works great on creation, on update we still need to check for a not found response as the keyvault the policy was created with no longer exists (and thus by proxy the resource has been deleted)
This will also need to be done for the other key vault resources but i'll open a PR for that.
This fixes the test added in the previous commit, while retaining the ability to reflect access policy deletion by deleting the key vault itself.
Thanks @katbyte - after e99279b, the related tests all run OK: $ make testacc TESTARGS='-run=TestAccAzureRMKeyVaultAccessPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMKeyVaultAccessPolicy -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
? github.com/terraform-providers/terraform-provider-azurerm [no test files]
=== RUN TestAccAzureRMKeyVaultAccessPolicy_basic
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_basic
=== RUN TestAccAzureRMKeyVaultAccessPolicy_basicClassic
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_basicClassic
=== RUN TestAccAzureRMKeyVaultAccessPolicy_requiresImport
--- SKIP: TestAccAzureRMKeyVaultAccessPolicy_requiresImport (0.00s)
resource_arm_key_vault_access_policy_test.go:72: Skipping since resources aren't required to be imported
=== RUN TestAccAzureRMKeyVaultAccessPolicy_multiple
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_multiple
=== RUN TestAccAzureRMKeyVaultAccessPolicy_update
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_update
=== RUN TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault
=== CONT TestAccAzureRMKeyVaultAccessPolicy_basic
=== CONT TestAccAzureRMKeyVaultAccessPolicy_update
=== CONT TestAccAzureRMKeyVaultAccessPolicy_multiple
=== CONT TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault
=== CONT TestAccAzureRMKeyVaultAccessPolicy_basicClassic
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault (193.92s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_basicClassic (203.56s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_multiple (206.81s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_basic (210.38s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_update (221.80s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 221.840s
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure 0.025s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/kubernetes 0.014s [no tests to run]
? github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response [no test files]
? github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/set [no test files]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress 0.031s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf 0.020s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate 0.020s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils 0.031s [no tests to run]
? github.com/terraform-providers/terraform-provider-azurerm/version [no test files] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @jen20, LGTM 👍
This has been released in version 1.23.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 = "~> 1.23.0"
}
# ... other configuration ... |
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! |
This pull request adds an acceptance test demonstrating the case where a key vault access policy resource is created with a bad key vault ID. Currently, the Terraform CLI gives the following output for this configuration:
However, state shows:
The test reproduces this workflow, and the subsequent commit makes it pass by removing the branch at which a nil state and nil error is returned during creation.