azurerm_key_vault_managed_hardware_security_module - support for activate_config
#20855
Conversation
There was a problem hiding this comment.
we need to fix the tests up:
testcase.go:117: Step 1/2 error: Error running apply: exit status 1
Error: creating Managed H S M: (Name "kvHsm230401192111680619" / Resource Group "acctestRG-KV-230401192111680619"): keyvault.ManagedHsmsClient#CreateOrUpdate: Failure sending request: StatusCode=503 -- Original Error: Code="503" Message="Pool creation is disabled in this region"
with azurerm_key_vault_managed_hardware_security_module.test,
on terraform_plugin_test.tf line 27, in resource "azurerm_key_vault_managed_hardware_security_module" "test":
27: resource "azurerm_key_vault_managed_hardware_security_module" "test" {
=== RUN TestAccKeyVaultManagedHardwareSecurityModule/resource/update
testcase.go:117: Step 1/2 error: Error running apply: exit status 1
Error: creating Managed H S M: (Name "kvHsm230401192433135766" / Resource Group "acctestRG-KV-230401192433135766"): keyvault.ManagedHsmsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ManagedHsmInsufficentQuota" Message="You may only create only 5 HSM pool(s) per subscription in a region."
with azurerm_key_vault_managed_hardware_security_module.test,
on terraform_plugin_test.tf line 27, in resource "azurerm_key_vault_managed_hardware_security_module" "test":
27: resource "azurerm_key_vault_managed_hardware_security_module" "test" {
=== RUN TestAccKeyVaultManagedHardwareSecurityModule/resource/complete
=== RUN TestAccKeyVaultManagedHardwareSecurityModule/resource/download
testcase.go:117: Step 1/6 error: Error running apply: exit status 1
Error: creating Managed H S M: (Name "kvHsm230401195642032955" / Resource Group "acctestRG-KV-230401195642032955"): keyvault.ManagedHsmsClient#CreateOrUpdate: Failure sending request: StatusCode=503 -- Original Error: Code="503" Message="Pool creation is disabled in this region"
with azurerm_key_vault_managed_hardware_security_module.test,
on terraform_plugin_test.tf line 27, in resource "azurerm_key_vault_managed_hardware_security_module" "test":
27: resource "azurerm_key_vault_managed_hardware_security_module" "test" {
|
acc local run pass now, I'll run in TC later. |
wuxu92
force-pushed
the
mhsm2/active
branch
2 times, most recently
from
2dd42a6 to
ffe7ae1
Compare
|
Hi @mbfrahry I have rebased origin/main and the LRO issue exists in the new sdk too. |
internal/services/keyvault/key_vault_managed_hardware_security_module_resource.go
Outdated
Show resolved
Hide resolved
internal/services/keyvault/key_vault_managed_hardware_security_module_resource.go
Outdated
Show resolved
Hide resolved
internal/services/keyvault/key_vault_managed_hardware_security_module_resource.go
Outdated
Show resolved
Hide resolved
| @@ -130,20 +142,53 @@ func resourceKeyVaultManagedHardwareSecurityModule() *pluginsdk.Resource { | |||
| }, | |||
| }, | |||
|
|
|||
| "activate_config": { | |||
There was a problem hiding this comment.
Because this isn't ForceNew can we add a test that adds certificates after already activating the hsm and also another step that removes certificates/removes them entirely?
| Elem: &pluginsdk.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "security_domain_certificate": { | ||
| Type: pluginsdk.TypeSet, |
There was a problem hiding this comment.
is the ordering important here?
| }, | ||
| }, | ||
|
|
||
| "security_domain_enc_data": { |
There was a problem hiding this comment.
| "security_domain_enc_data": { | |
| "security_domain_encrypted_data": { |
There was a problem hiding this comment.
In retrospect/per the documentation below, this probably shouldn't be part of this resource, but instead a separate resource, what's the reasoning for in-lining this?
There was a problem hiding this comment.
this is an activate action rather than a resource. it has no update/delete methods so I think it should be a part of the managed HSM itself.
| originResponse := future.Response() | ||
| data, err := io.ReadAll(originResponse.Body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| var encData struct { | ||
| Value string `json:"value"` | ||
| } | ||
|
|
||
| err = json.Unmarshal(data, &encData) | ||
| if err != nil { | ||
| return "", fmt.Errorf("unmarshal EncData: %v", err) | ||
| } | ||
|
|
||
| err = waitHSMPendingStatus(ctx, vaultBaseURL, sdClient) |
There was a problem hiding this comment.
this should be done/exposed by go-azure-sdk - is there an issue for this?
|
|
||
| // if isUpload is false, then check the download pending | ||
| // the generated SDK of `future.WaitForCompletionRef` not work, see: | ||
| // https://github.com/Azure/azure-rest-api-specs/issues/23035 |
There was a problem hiding this comment.
this shouldn't be needed with go-azure-sdk?
There was a problem hiding this comment.
this is a data-plane API. Does go-azure-sdk support data-plane now?
| if err != nil || resp.Model == nil || resp.Model.Properties == nil || resp.Model.Properties.HsmUri == nil { | ||
| return fmt.Errorf("get nil HSMUri for %s: %+v", id, err) | ||
| } else { | ||
| encData, err := securityDomainDownload(ctx, | ||
| kvClient, | ||
| *resp.Model.Properties.HsmUri, | ||
| certs[0].(map[string]interface{}), | ||
| ) | ||
| if err == nil { | ||
| d.Set("security_domain_enc_data", encData) | ||
| } else { | ||
| log.Printf("security domain download: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
this becomes a lot simpler/less crash-prone if we pull hsmUri out into a variable:
| if err != nil || resp.Model == nil || resp.Model.Properties == nil || resp.Model.Properties.HsmUri == nil { | |
| return fmt.Errorf("get nil HSMUri for %s: %+v", id, err) | |
| } else { | |
| encData, err := securityDomainDownload(ctx, | |
| kvClient, | |
| *resp.Model.Properties.HsmUri, | |
| certs[0].(map[string]interface{}), | |
| ) | |
| if err == nil { | |
| d.Set("security_domain_enc_data", encData) | |
| } else { | |
| log.Printf("security domain download: %v", err) | |
| } | |
| } | |
| hsmUri := "" | |
| if model := resp.Model; model != nil && model.Properties != nil && model.Properties.HsmUri != nil { | |
| hsmUri = *model.Properties.HsmUri | |
| } | |
| if hsmUri == "" { | |
| return fmt.Errorf("retrieving %s: `properties.HsmUri` was nil", id) | |
| } | |
| encryptedData, err := securityDomainDownload(ctx, kvClient, hsmUri, certs) | |
| if err != nil { | |
| return fmt.Errorf("downloading security domain for %s: %+v", id, err) | |
| } | |
| d.Set("security_domain_enc_data", encryptedData) |
it's also worth calling out that we're assuming the length on certs here, which is a potential crash point - we should instead pass that into the other function
There was a problem hiding this comment.
The length of certs has been checked in the if statement before.
|
|
||
| A `activate_config` block supports the following: | ||
|
|
||
| * `security_domain_certificate` - (Required) A list of KeyVault certificates resource ID(minimum of three and up to a maximum of 10) to activate this Managed HSM. More information see [activate-your-managed-hsm](https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/quick-create-cli#activate-your-managed-hsm) |
There was a problem hiding this comment.
we don't use the phrase Resource ID, since it's ambiguous - this should be:
| * `security_domain_certificate` - (Required) A list of KeyVault certificates resource ID(minimum of three and up to a maximum of 10) to activate this Managed HSM. More information see [activate-your-managed-hsm](https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/quick-create-cli#activate-your-managed-hsm) | |
| * `security_domain_certificate` - (Required) A list of Key Vault Certificate IDs which should be used to activate this Managed HSM. More information on [activating the Managed HSM can be found in the Microsoft documentation](https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/quick-create-cli#activate-your-managed-hsm). |
Also this name isn't matching what this is doing, so perhaps this field wants renaming?
There was a problem hiding this comment.
how do you think of renaming as certificate_ids?
There was a problem hiding this comment.
security_domain_certificate_ids or acrivation_key_vault_certificat_ids ?
There was a problem hiding this comment.
renamed to activation_key_vault_certificate_ids
website/docs/r/key_vault_managed_hardware_security_module.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/key_vault_managed_hardware_security_module.html.markdown
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
we have some test failures
Error: creating Managed H S M (Subscription: "*******"
Resource Group Name: "acctestRG-KV-230601191510354687"
Managed H S M Name: "kvHsm230601191510354687"): performing CreateOrUpdate: managedhsms.ManagedHsmsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="GatewayTimeout" Message="The gateway did not receive a response from 'Microsoft.KeyVault' within the specified time period."
with azurerm_key_vault_managed_hardware_security_module.test,
on terraform_plugin_test.tf line 28, in resource "azurerm_key_vault_managed_hardware_security_module" "test":
28: resource "azurerm_key_vault_managed_hardware_security_module" "test" {
=== RUN TestAccKeyVaultManagedHardwareSecurityModule/resource/basic
testcase.go:117: Step 1/2 error: Error running apply: exit status 1
Error: creating Managed H S M (Subscription: "*******"
Resource Group Name: "acctestRG-KV-230601192459875707"
Managed H S M Name: "kvHsm230601192459875707"): performing CreateOrUpdate: managedhsms.ManagedHsmsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="503" Message="Capacity check failed"
|
@katbyte Only some regions support creating Managed HSM resources and there is a quota limitation (only 5 in my experience) for a subscription in each region. so it's easy to failed. This case can get pass easier if set
|
mbfrahry
changed the title
azurerm_key_vault_managed_hardware_security_module - support activate ManagedHSM in terraformazurerm_key_vault_managed_hardware_security_module - support for activate_config
|
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. |
Draft for download pending SDK issue
the managed HSM has to be activated before using it. This PR add this function by using the certificates generated from keyvault reosurce.
this function is needed for creating keys under managed HSM.
reference doc: https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/quick-create-cli#activate-your-managed-hsm
docs of az cli: https://learn.microsoft.com/en-us/cli/azure/keyvault/security-domain?view=azure-cli-latest
(related pr #20150)