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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants