-
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
New Resource: azurerm_key_vault_managed_hardware_security_module_role_definition
and azurerm_key_vault_managed_hardware_security_module_role_assignment
for managed HSM
#22332
Conversation
Hi @wuxu92 Thanks! |
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 @wuxu92 - Thanks for this PR, and thanks for bearing with us while we worked on the upstream issue.
There's a number of comments to take a look at below if you could please, then I'll be able to continue review. I believe there are some problems around the IDs for these resources and their parsing that we need to resolve/work out before this can be merged, but I'll take a look into that while you work through the comments here and we'll come back to that later in the next round of review.
Thanks!
func lockForHSM(hsmURI string) { | ||
locks.ByName(hsmURI, "azurerm_key_vault_managed_hardware_security_module") | ||
} | ||
|
||
func unlockForHSM(hsmURI string) { | ||
locks.UnlockByName(hsmURI, "azurerm_key_vault_managed_hardware_security_module") | ||
} |
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.
I feel this causes more obfuscation than provides convenience, and should be removed and used long-hand in the code.
@@ -15,6 +15,7 @@ import ( | |||
|
|||
type KeyVaultManagedHardwareSecurityModuleResource struct{} | |||
|
|||
// https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/multi-region-replication |
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.
what's the purpose of this comment?
// https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/multi-region-replication |
var param keyvault.RoleAssignmentCreateParameters | ||
param.Properties = &keyvault.RoleAssignmentProperties{} | ||
prop := param.Properties | ||
prop.PrincipalID = pointer.FromString(model.PrincipalId) | ||
prop.RoleDefinitionID = pointer.FromString(model.RoleDefinitionId) |
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.
This doesn't create object and populate the param object, what was the intention here? i.e. param
will be empty in the client.Create
below.
lockForHSM(model.VaultBaseUrl) | ||
defer unlockForHSM(model.VaultBaseUrl) |
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.
Locking should be established before starting any work on the resource, can we move this to the top? (before the requires import check at least)
func (k *KeyVaultRoleDefinitionDataSourceModel) ToSDKPermissions() *[]keyvault.Permission { | ||
var res []keyvault.Permission | ||
for _, p := range k.Permission { | ||
ins := keyvault.Permission{} | ||
if p.Actions != nil { | ||
ins.Actions = pointer.To(p.Actions) | ||
} | ||
if p.NotActions != nil { | ||
ins.NotActions = pointer.To(p.NotActions) | ||
} | ||
ins.DataActions, ins.NotDataActions = p.toSDKDataAction() | ||
res = append(res, ins) | ||
} | ||
return &res | ||
} | ||
|
||
func (k *KeyVaultRoleDefinitionDataSourceModel) LoadSDKPermission(perms *[]keyvault.Permission) { | ||
if perms != nil { | ||
k.Permission = []Permission{} | ||
for _, p := range *perms { | ||
k.Permission = append(k.Permission, (Permission{}).loadSDKDataAction(p)) | ||
} | ||
} | ||
} |
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.
Can we have these as normal flatten and expand functions rather than receiver functions?
* `actions` - (Optional) Specifies a list of action permission to granted. | ||
|
||
* `not_actions` - (Optional) Specifies a list of action permission excluded (but not denied). |
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.
Can we get the valid values for these properties to help users out?
MHSMRoleClient *dataplane.RoleDefinitionsClient | ||
MHSMSDClient *dataplane.HSMSecurityDomainClient | ||
MHSMRoleClient *dataplane.RoleDefinitionsClient | ||
MHSMRoleAssignClient *dataplane.RoleAssignmentsClient |
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.
MHSMRoleAssignClient *dataplane.RoleAssignmentsClient | |
MHSMRoleAssignmntsClient *dataplane.RoleAssignmentsClient |
roleAssignClient := dataplane.NewRoleAssignmentsClient() | ||
o.ConfigureClient(&roleAssignClient.Client, o.ManagedHSMAuthorizer) |
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.
For consistency with the existing mhsm client name:
roleAssignClient := dataplane.NewRoleAssignmentsClient() | |
o.ConfigureClient(&roleAssignClient.Client, o.ManagedHSMAuthorizer) | |
mhsmRoleAssignClient := dataplane.NewRoleAssignmentsClient() | |
o.ConfigureClient(&mhsmRoleAssignClient.Client, o.ManagedHSMAuthorizer) |
VaultsClient: &vaultsClient, | ||
MHSMSDClient: &sdClient, | ||
MHSMRoleClient: &mhsmRoleDefineClient, | ||
MHSMRoleAssignClient: &roleAssignClient, |
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.
MHSMRoleAssignClient: &roleAssignClient, | |
MHSMRoleAssignClient: &mhsmRoleAssignClient, |
type KeyvaultRoleDefinitionDataResource struct{} | ||
|
||
var _ sdk.DataSource = (*KeyvaultRoleDefinitionDataResource)(nil) |
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.
type KeyvaultRoleDefinitionDataResource struct{} | |
var _ sdk.DataSource = (*KeyvaultRoleDefinitionDataResource)(nil) | |
type KeyvaultRoleDefinitionDataSource struct{} | |
var _ sdk.DataSource = KeyvaultRoleDefinitionDataSource{} |
@jackofallops Thank you for your comments! I have addressed them and the Tests pass now. Would you please take another look when you have a chance. |
_, err = client.Create(ctx, model.VaultBaseUrl, model.Scope, model.Name, param) | ||
if err != nil { |
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.
lines can be combined
|
||
type KeyVaultRoleAssignmentResource struct{} | ||
|
||
var _ sdk.Resource = (*KeyVaultRoleAssignmentResource)(nil) |
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.
@wuxu92 you missed this
"github.com/tombuildsstuff/kermit/sdk/keyvault/7.4/keyvault" | ||
) | ||
|
||
type ManagedHSMRoleAssignmentModel struct { |
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.
shoudln't these all be
type ManagedHSMRoleAssignmentModel struct { | |
type KeyVaultManagedHSMRoleAssignmentModel struct { |
"github.com/hashicorp/terraform-provider-azurerm/utils" | ||
) | ||
|
||
type KeyVaultRoleAssignmentResource struct{} |
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.
should this be something like
type KeyVaultRoleAssignmentResource struct{} | |
type KeyVaultManagedHSMRoleAssignmentResource struct{} |
"actions": { | ||
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
Elem: &pluginsdk.Schema{ | ||
Type: pluginsdk.TypeString, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, | ||
}, | ||
|
||
"not_actions": { | ||
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
Elem: &pluginsdk.Schema{ | ||
Type: pluginsdk.TypeString, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, | ||
}, |
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.
@wuxu92 - please address this comment and add some validation here
_, err = client.CreateOrUpdate(ctx, model.VaultBaseUrl, roleDefinitionScope, model.Name, param) | ||
if err != nil { |
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.
these lines can be combined
model.Name = pointer.From(result.Name) | ||
model.Scope = id.Scope | ||
model.VaultBaseUrl = id.VaultBaseUrl | ||
model.ResourceManagerId = pointer.From(result.ID) |
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.
@wuxu92 - please address this comment. we should always be parsing IDs and normalizing them to prevent casing issues
model.Description = pointer.ToString(prop.Description) | ||
model.RoleType = string(prop.RoleType) | ||
model.RoleName = pointer.From(prop.RoleName) | ||
|
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.
|
||
* `vault_base_url` - (Required) Specify the base URL of the Managed HSM resource. | ||
|
||
* `scope` - (Optional) Specify the scope to retrieve the role definition. Defaults to `/`. |
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.
@wuxu92 - comments?
|
||
* `role_name` - (Optional) Specify a name for this KeyVault Role Definition. | ||
|
||
* `scope` - (Optional) Specify the scope of this role definition. Possible value is `/`. Defaults to `/`. Changing this forces a new KeyVault to be created. |
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.
if you deleted it why is it still in the comments?
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.
LGTM 🔐
<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>"hashicorp/azurerm" updated from "3.82.0" to "3.83.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.83.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.83.0
UPGRADE NOTES

* Key Vaults are now loaded using [the `ListBySubscription` API within the Key Vault Resource Provider](https://learn.microsoft.com/en-us/rest/api/keyvault/keyvault/vaults/list-by-subscription?view=rest-keyvault-keyvault-2022-07-01&tabs=HTTP) rather than [the Resources API](https://learn.microsoft.com/en-us/rest/api/keyvault/keyvault/vaults/list?view=rest-keyvault-keyvault-2022-07-01&tabs=HTTP). This change means that the Provider now caches the list of Key Vaults available within a Subscription, rather than loading these piecemeal to workaround stale data returned from the Resources API ([#24019](https://github.com/hashicorp/terraform-provider-azurerm/issues/24019))

FEATURES:

* New Data Source: `azurerm_stack_hci_cluster` ([#24032](https://github.com/hashicorp/terraform-provider-azurerm/issues/24032))

ENHANCEMENTS:

* dependencies: updating to `v0.20231129.1103252` of `github.com/hashicorp/go-azure-sdk` ([#24063](hashicorp/terraform-provider-azurerm#24063 `automation`: updating to API Version `2023-11-01` ([#24017](hashicorp/terraform-provider-azurerm#24017 `keyvault`: the cache is now populated using the `ListBySubscription` endpoint on the KeyVault Resource Provider rather than via the `Resources` API ([#24019](hashicorp/terraform-provider-azurerm#24019 `keyvault`: updating the cache to populate all Key Vaults available within the Subscription to reduce the number of API calls ([#24019](hashicorp/terraform-provider-azurerm#24019 Data Source `azurerm_private_dns_zone`: refactoring to use the `ListBySubscription` API rather than the Resources API when `resource_group_name` is omitted ([#24024](hashicorp/terraform-provider-azurerm#24024 `azurerm_dashboard_grafana` - support for `grafana_major_version` ([#24014](hashicorp/terraform-provider-azurerm#24014 `azurerm_linux_web_app` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_linux_web_app_slot` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_media_transform` - deprecate `face_detector_preset` and `video_analyzer_preset` ([#24002](hashicorp/terraform-provider-azurerm#24002 `azurerm_postgresql_database` - update the validation of `collation` to include `Norwegian_Norway.1252` ([#24070](hashicorp/terraform-provider-azurerm#24070 `azurerm_postgresql_flexible_server` - updating to API Version `2023-06-01-preview` ([#24016](hashicorp/terraform-provider-azurerm#24016 `azurerm_redis_cache` - support for the `active_directory_authentication_enabled` property ([#23976](hashicorp/terraform-provider-azurerm#23976 `azurerm_windows_web_app` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_windows_web_app_slot` - add support for dotnet 8 ([#23893](hashicorp/terraform-provider-azurerm#23893 `azurerm_storage_account` - add `name` validation in custom diff ([#23799](https://github.com/hashicorp/terraform-provider-azurerm/issues/23799))

BUG FIXES:

* authentication: fix a bug where auxiliary tenants were not correctly authorized ([#24063](hashicorp/terraform-provider-azurerm#24063 `azurerm_app_configuration` - normalize location in `replica` block ([#24074](hashicorp/terraform-provider-azurerm#24074 `azurerm_cosmosdb_account` - cosmosdb version and capabilities can now be updated at the same time ([#24029](hashicorp/terraform-provider-azurerm#24029 `azurerm_data_factory_flowlet_data_flow` - `source` and `sink` properties are now optional ([#23987](hashicorp/terraform-provider-azurerm#23987 `azurerm_datadog_monitor_tag_rule` - correctly handle default rule ([#22806](hashicorp/terraform-provider-azurerm#22806 `azurerm_ip_group`: fixing a crash when `firewall_ids` and `firewall_policy_ids` weren't parsed correctly from the API Response ([#24031](hashicorp/terraform-provider-azurerm#24031 `azurerm_nginx_deployment` - add default value of `20` for `capacity` ([#24033](https://github.com/hashicorp/terraform-provider-azurerm/issues/24033))


</pre> </details> <details> <summary>3.84.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.84.0
FEATURES:

* **New Data Source:** `azurerm_storage_containers` ([#24061](hashicorp/terraform-provider-azurerm#24061 **New Resource:** `azurerm_elastic_san` ([#23619](hashicorp/terraform-provider-azurerm#23619 **New Resource:** `azurerm_key_vault_managed_hardware_security_module_role_assignment` ([#22332](hashicorp/terraform-provider-azurerm#22332 **New Resource:** `azurerm_key_vault_managed_hardware_security_module_role_definition` ([#22332](https://github.com/hashicorp/terraform-provider-azurerm/issues/22332))

ENHANCEMENTS:

* dependencies: updating mssql elasticpools from `v5.0` to `2023-05-01-preview`
* dependencies: updating to `v0.20231207.1122031` of `github.com/hashicorp/go-azure-sdk` ([#24149](hashicorp/terraform-provider-azurerm#24149 Data Source: `azurerm_storage_account` - export the primary and secondary internet and microsoft hostnames for blobs, dfs, files, queues, tables and web ([#23517](hashicorp/terraform-provider-azurerm#23517 Data Source: `azurerm_cosmosdb_account` - export the `connection_strings`, `primary_sql_connection_string`, `secondary_sql_connection_string`, `primary_readonly_sql_connection_string`, `secondary_readonly_sql_connection_string`, `primary_mongodb_connection_string`, `secondary_mongodb_connection_string`, `primary_readonly_mongodb_connection_string`, and `secondary_readonly_mongodb_connection_string` attributes ([#24129](hashicorp/terraform-provider-azurerm#24129 `azurerm_bot_service_azure_bot` - support for the `public_network_access_enabled` property ([#24125](hashicorp/terraform-provider-azurerm#24125 `azurerm_container_app_environment` - support for the `workload_profile` property ([#23478](hashicorp/terraform-provider-azurerm#23478 `azurerm_cosmosdb_cassandra_datacenter` - support for the `seed_node_ip_addresses` property ([#24076](hashicorp/terraform-provider-azurerm#24076 `azurerm_firewall` - support for the `dns_proxy_enabled` property ([#20519](hashicorp/terraform-provider-azurerm#20519 `azurerm_kubernetes_cluster` - support for the `support_plan` property and the `sku_tier` `Premium` ([#23970](hashicorp/terraform-provider-azurerm#23970 `azurerm_mssql_database` - support for `enclave_type` field ([#24054](hashicorp/terraform-provider-azurerm#24054 `azurerm_mssql_elasticpool` - support for `enclave_type` field ([#24054](hashicorp/terraform-provider-azurerm#24054 `azurerm_mssql_managed_instance` - support for more `vcores`: `6`, `10`, `12`, `20`, `48`, `56`, `96`, `128` ([#24085](hashicorp/terraform-provider-azurerm#24085 `azurerm_redis_linked_server` - support for the property `geo_replicated_primary_host_name` ([#23984](hashicorp/terraform-provider-azurerm#23984 `azurerm_storage_account` - expose the primary and secondary internet and microsoft hostnames for blobs, dfs, files, queues, tables and web ([#23517](hashicorp/terraform-provider-azurerm#23517 `azurerm_synapse_role_assignment` - support for the `principal_type` property ([#24089](hashicorp/terraform-provider-azurerm#24089 `azurerm_spring_cloud_build_deployment` - support for the `application_performance_monitoring_ids` property ([#23969](hashicorp/terraform-provider-azurerm#23969 `azurerm_virtual_network_gateway` - support for the `bgp_route_translation_for_nat_enabled`, `dns_forwarding_enabled`, `ip_sec_replay_protection_enabled`, `remote_vnet_traffic_enabled`, `virtual_wan_traffic_enabled`, `radius_server`, `virtual_network_gateway_client_connection`, `policy_group`, and `ipsec_policy` property ([#23220](https://github.com/hashicorp/terraform-provider-azurerm/issues/23220))

BUG FIXES:

* `azurerm_application_insights_api_key` - prevent a bug where multiple keys couldn't be created for an Application Insights instance ([#23463](hashicorp/terraform-provider-azurerm#23463 `azurerm_container_registry` - the `network_rule_set.virtual_network` property has been deprecated ([#24140](hashicorp/terraform-provider-azurerm#24140 `azurerm_hdinsight_hadoop_cluster` - set `roles.edge_node.install_script_action.parameters` into state by retrieving the value provided in the user config since this property isn't returned by the API ([#23971](hashicorp/terraform-provider-azurerm#23971 `azurerm_kubernetes_cluster` - prevent a bug where maintenance window start date was always recalculated and sent to the API ([#23985](hashicorp/terraform-provider-azurerm#23985 `azurerm_mssql_database` - will no longer send all long retention values in payload unless set ([#24124](hashicorp/terraform-provider-azurerm#24124 `azurerm_mssql_managed_database` - will no longer send all long retention values in payload unless set ([#24124](hashicorp/terraform-provider-azurerm#24124 `azurerm_mssql_server_microsoft_support_auditing_policy` - only include storage endpoint in payload if set ([#24122](hashicorp/terraform-provider-azurerm#24122 `azurerm_mobile_network_packet_core_control_plane` - prevent a panic if the HTTP Response is nil ([#24083](hashicorp/terraform-provider-azurerm#24083 `azurerm_storage_account` - revert plan time name validation `(#23799)` ([#24142](hashicorp/terraform-provider-azurerm#24142 `azurerm_web_application_firewall_policy` - split create and update function to fix lifecycle - ignore changes ([#23412](https://github.com/hashicorp/terraform-provider-azurerm/issues/23412))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/931/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> --------- Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com> Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
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. |
AccTest failure nto caused by this PR.