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

managedhsm: introducing dedicated Resource ID Parsers for the Data Plane Versioned and Versionless Key IDs #25601

Merged
merged 24 commits into from
May 2, 2024

Conversation

tombuildsstuff
Copy link
Member

@tombuildsstuff tombuildsstuff commented Apr 12, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

This PR introduces dedicated Resource ID parsers for Managed HSM Data Plane Keys - it doesn't yet refactor the Role Assignments/Definitions to the same pattern since the current Resource IDs differ from those defined in the API - but that'll come in a bit.

This PR refactors the Managed HSM package to introduce a consistent set of Resource ID parsers which take into account the Domain Suffix.

Whilst this should be mostly complete, due to a number of issues with the existing resources - this isn’t quite as far as I wanted this to be before I headed out, and the following remains:

  1. Adding RequiresImport tests
  2. Updating the exists functions
  3. Update the documentation to account for managed_hsm_id being the new preference rather than vault_base_url
  4. Update all of the timeouts to 30m
  5. Investigate whether we should deprecate resource_manager_id?

@Botje
Copy link
Contributor

Botje commented Apr 16, 2024

Hi Tom,

I rebased #25088 on top of this PR and I found two small gaps this PR might want to address too:

  1. The Azure APIs return a triple of (base URI, key name, key version) when reading resources.
    In order to know whether a given base URI belongs to a key vault or a managed HSM you have to inspect the hostname.
    I abstracted that logic into internal/services/managedhsm/helpers/is_managed_hsm_uri.go

  2. Continuing from the above, reconstituting the HSM key ID from the parts returned by the Azure API requires checking whether the key version is empty or not. That could also be abstracted into a function like canonicalManagedHSMKeyIdFromParts(keyVaultURI, keyName, keyVersion string).

What do you think?

…ssignment

This switches to using the Resource ID the Resource actually uses rather than this apparent Terraform unique value?
This now uses `managed_hsm_id` to discover the Managed HSM rather than the Data Plane URI - which mirrors the pattern used elsewhere.
This is important for two reasons:

1. We don't support provisioning resources across Subscriptions - a unique Provider instance needs to be used for each Subscription
2. This allows us to determine when the Managed HSM in question has been removed out-of-band due to limitations in Go's networking layer
…he Parse function directly

This was tested via the validate, but was missing tests covering this directly
@manicminer manicminer marked this pull request as ready for review April 30, 2024 19:01
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.

LGTM 🥅

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@katbyte katbyte merged commit 97ac087 into main May 2, 2024
33 checks passed
@katbyte katbyte deleted the f/managed-hsm-nested-items branch May 2, 2024 16:23
@github-actions github-actions bot added this to the v3.102.0 milestone May 2, 2024
katbyte added a commit that referenced this pull request May 2, 2024
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request May 3, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.101.0&#34; to &#34;3.102.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.102.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.102.0&#xA;FEATURES:&#xA;&#xA;*
New Resource: `azurerm_storage_sync_server_endpoint`
([#25831](https://github.com/hashicorp/terraform-provider-azurerm/issues/25831))&#xA;*
New Resource: `azurerm_storage_container_immutability_policy`
([#25804](https://github.com/hashicorp/terraform-provider-azurerm/issues/25804))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
`azurerm_load_test` - add support for `encryption`
([#25759](https://github.com/hashicorp/terraform-provider-azurerm/issues/25759))&#xA;*
`azurerm_network_connection_monitor` - update validation for
`target_resource_type` and `target_resource_id`
([#25745](https://github.com/hashicorp/terraform-provider-azurerm/issues/25745))&#xA;*
`azurerm_mssql_managed_database` - support for a Restorable Database ID
to be used as the `source_database_id` for point in time restore
([#25568](https://github.com/hashicorp/terraform-provider-azurerm/issues/25568))&#xA;*
`azurerm_storage_account` - support for the `managed_hsm_key_id`
property
([#25088](https://github.com/hashicorp/terraform-provider-azurerm/issues/25088))&#xA;*
`azurerm_storage_account_customer_managed_key` - support for the
`managed_hsm_key_id` property
([#25088](https://github.com/hashicorp/terraform-provider-azurerm/issues/25088))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_linux_function_app` - now sets docker
registry url in `linux_fx_version` by default
([#23911](https://github.com/hashicorp/terraform-provider-azurerm/issues/23911))&#xA;*
`azurerm_resource_group` - work around sporadic eventual consistency
errors
([#25758](https://github.com/hashicorp/terraform-provider-azurerm/issues/25758))&#xA;&#xA;DEPRECATIONS:&#xA;&#xA;*
`azurerm_key_vault_managed_hardware_security_module_role_assignment` -
the `vault_base_url` property has been deprecated in favour of the
`managed_hsm_id` property
([#25601](https://github.com/hashicorp/terraform-provider-azurerm/issues/25601))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/148/">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>
@riemers
Copy link

riemers commented May 31, 2024

Think you missed the removal of the scope in the data example here https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/key_vault_managed_hardware_security_module_role_assignment
Also, if i dont have access to the read of the hsm but i do have read admin rights on the key the module now still wants me to have read rights on the hsm itself.

@manicminer
Copy link
Member

@riemers Could you open a new issue for this so we can track this? Thanks!

Copy link

github-actions bot commented Jul 1, 2024

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 Jul 1, 2024
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

6 participants