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

new resource and data source "azurerm_key_vault_managed_hardware_security_module" #10873

Merged
merged 8 commits into from
Apr 22, 2021

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented Mar 8, 2021

first pr to fix issue: #10209

  1. this resource is only supported in latest preview version: 2020-04-01-preview, and it's going to be GA soon
  2. this resource is almost the same with key vault, doc refers to here: https://docs.microsoft.com/en-us/azure/key-vault/managed-hsm/overview
  3. api issue that needs to pay attention:
    3.1 soft_delete is always true. If setting to false in the create rest api, it will report error
    3.2 soft_delete_retention_days could not be updated, the backend service will report error
    3.3 purge_protection_enabled could be set in the update rest api, but the response remain the same. this is an issue and I have reported to the service team
    3.4 I have made the above fields forcenew
  4. During public preview, the rest api limits we can create only 1 HSM pool(s) per subscription in a region. The acctest is made to run in sequence.

image

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @njuCZ

Thanks for this PR - I've taken a look through and left some comments inline, but if we can fix those up then we should be able to take another look through and run the tests.

Thanks!

azurerm/internal/services/keyvault/resourceids.go Outdated Show resolved Hide resolved

* `purge_protection_enabled` - (Optional) Is Purge Protection enabled for this Key Vault Managed Hardware Security Module? Defaults to `false`. Changing this forces a new resource to be created.

* `soft_delete_retention_days` - (Optional) The number of days that items should be retained for once soft-deleted. This value can be between `7` and `90` days. Defaults to `90`. Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

this isn't defaulted in the schema - and from memory Terraform intentionally defaults to 7 for the other resource, so we should match that 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.

@njuCZ
Copy link
Contributor Author

njuCZ commented Mar 10, 2021

it seems the CI error is because of temporary network break

@njuCZ
Copy link
Contributor Author

njuCZ commented Mar 18, 2021

rebase and run the acctest in sequence

@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 6, 2021

thanks for the commit

@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 13, 2021

@katbyte I have made the acctest run in sequence. Every test has used the func data.ResourceSequentialTest.

@njuCZ njuCZ force-pushed the hsm branch 3 times, most recently from ed1fe9e to 3129f77 Compare April 20, 2021 02:49
@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 20, 2021

@katbyte thanks for the suggestions. I have changed the way to run test case. Now all acctest have passed in teamcity

@njuCZ njuCZ requested a review from katbyte April 20, 2021 05:21
@katbyte katbyte added this to the v2.57.0 milestone Apr 21, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @njuCZ - LGTM and once the merge conflict is resolved we can get this merged

@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 22, 2021

@katbyte Have rebased and fix the conflicts

@katbyte katbyte merged commit 8a62e76 into hashicorp:master Apr 22, 2021
katbyte added a commit that referenced this pull request Apr 22, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 2.57.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.57.0"
}
# ... other configuration ...

@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 May 30, 2021
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

3 participants