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

azurerm_storage_account_customer_managed_key - support for cross-tenant customer-managed keys #20356

Merged
merged 16 commits into from
Sep 21, 2023

Conversation

Sewci0
Copy link
Contributor

@Sewci0 Sewci0 commented Feb 7, 2023

Adds support for Cross Tenant CMK

https://learn.microsoft.com/en-us/azure/storage/common/customer-managed-keys-configure-cross-tenant-new-account

Introduces two new properties:

  • key_vault_uri - used when the SP has no access to the vault that stores the CMK
  • federated_identity_client_id - points at the appID used for federated access

@Sewci0 Sewci0 changed the title azurerm_storage_account_customer_managed_key - support for cross-tenant customer-managed keys azurerm_storage_account_customer_managed_key - support for cross-tenant customer-managed keys Feb 7, 2023
@Sewci0 Sewci0 force-pushed the feature/federated_cmk branch from 04701eb to 78be7da Compare February 9, 2023 17:48
@github-actions github-actions bot added size/M and removed size/XL labels Feb 9, 2023
magodo
magodo previously requested changes Feb 15, 2023
Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
I've taken a look through and left some comments inline. Meanwhile, there needs one/more acctests to cover this new change.

@Sewci0
Copy link
Contributor Author

Sewci0 commented Feb 17, 2023

Thank you @magodo for looking into it, I've replied to your comment and fixed the rest of the issues.

I've also added acceptance tests but these are super tricky to run as you need to set up two separate tenants.

@magodo
Copy link
Collaborator

magodo commented Feb 21, 2023

@Sewci0 Thank you for the update! It now LGTM!

@Sewci0 Sewci0 requested a review from magodo February 22, 2023 13:42
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.

We have some test failures:

------- Stdout: -------
=== RUN   TestAccStorageAccountCustomerManagedKey_updateKey
=== PAUSE TestAccStorageAccountCustomerManagedKey_updateKey
=== CONT  TestAccStorageAccountCustomerManagedKey_updateKey
    testcase.go:110: Step 1/4 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azurerm_storage_account_customer_managed_key.test will be updated in-place
          ~ resource "azurerm_storage_account_customer_managed_key" "test" {
                id                 = "/subscriptions/*******/resourceGroups/acctestRG-230224173716578224/providers/Microsoft.Storage/storageAccounts/acctestsai40tk"
              - key_vault_uri      = "https://acctestkvi40tk.vault.azure.net/" -> null
                # (4 unchanged attributes hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccStorageAccountCustomerManagedKey_updateKey (380.72s)
FAIL

@vaishakhbn
Copy link

Hi, do you know if this PR is being actively worked on?

@Sewci0 Sewci0 force-pushed the feature/federated_cmk branch from 3671c14 to 5bb43e2 Compare May 4, 2023 10:46
@Sewci0
Copy link
Contributor Author

Sewci0 commented May 4, 2023

Hi @magodo, apologies it has taken so long but I've just fixed the tests and rerun them locally. It should be ready to go. Thanks!

@Sewci0 Sewci0 requested a review from katbyte May 4, 2023 12:37
@msmygit
Copy link

msmygit commented Sep 18, 2023

@tombuildsstuff @magodo @manicminer -- any idea if this PR is being looked at activtely and will be incorporated? This has been pending for quite a long time now with more and more people awaiting its release. Thank you in advance!

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@Sewci0 Thank you for keeping the branch updated and for your patience whilst we update our testing infrastructure. I've pushed a few minor changes to the test config and it LGTM!

Since I've updated our TeamCity configuration, I'll let another maintainer look over those changes before merging.

Screenshot 2023-09-21 at 00 43 16 Screenshot 2023-09-21 at 02 14 25

@manicminer
Copy link
Contributor

TeamCity config changes for additional review: 707a94b

@manicminer manicminer requested a review from a team September 21, 2023 01:03
@manicminer manicminer dismissed stale reviews from magodo, tombuildsstuff, and katbyte September 21, 2023 01:04

Changes made

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Tests look good. Thanks for this @Sewci0 and @manicminer! LGTM 🎉

@manicminer manicminer merged commit 38e711c into hashicorp:main Sep 21, 2023
25 checks passed
@github-actions github-actions bot added this to the v3.74.0 milestone Sep 21, 2023
manicminer added a commit that referenced this pull request Sep 21, 2023
@Sewci0
Copy link
Contributor Author

Sewci0 commented Sep 21, 2023

@manicminer This is very exciting. Thank you!

@Sewci0 Sewci0 deleted the feature/federated_cmk branch September 22, 2023 12:04
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Sep 23, 2023
<Actions>
<action
id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.73.0&#34; to
&#34;3.74.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.74.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.74.0&#xA;NOTES:&#xA;&#xA;*
`azurerm_synapse_sql_pool` - users that have imported
`azurerm_synapse_sql_pool` resources that were created outside of
Terraform using an `LRS` storage account type will need to use
`ignore_changes` to avoid the resource from being destroyed and
recreated.&#xA;&#xA;FEATURES:&#xA;&#xA;* **New Resource**:
`azurerm_arc_resource_bridge_appliance`
([#23108](hashicorp/terraform-provider-azurerm#23108
**New Resource**: `azurerm_data_factory_dataset_azure_sql_table`
([#23264](hashicorp/terraform-provider-azurerm#23264
**New Resource**: `azurerm_function_app_connection`
([#23127](https://github.com/hashicorp/terraform-provider-azurerm/issues/23127))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20230918.1115907` of
`github.com/hashicorp/go-azure-sdk`
([#23337](hashicorp/terraform-provider-azurerm#23337
dependencies: downgrading to `v1.12.5` of `github.com/rickb777/date`
([#23296](hashicorp/terraform-provider-azurerm#23296
`mysql`: updating to use API Version `2022-01-01`
([#23320](hashicorp/terraform-provider-azurerm#23320
`azurerm_app_configuration` - support for the `replica` block
([#22452](hashicorp/terraform-provider-azurerm#22452
`azurerm_bot_channel_directline` - support for `user_upload_enabled`,
`endpoint_parameters_enabled`, and `storage_enabled`
([#23149](hashicorp/terraform-provider-azurerm#23149
`azurerm_container_app` - support for scale rules
([#23294](hashicorp/terraform-provider-azurerm#23294
`azurerm_container_app_environment` - support for zone redundancy
([#23313](hashicorp/terraform-provider-azurerm#23313
`azurerm_container_group` - support for the `key_vault_user_identity_id`
property for Customer Managed Keys
([#23332](hashicorp/terraform-provider-azurerm#23332
`azurerm_cosmosdb_account` - support for MongoDB connection strings
([#23331](hashicorp/terraform-provider-azurerm#23331
`azurerm_data_factory_dataset_delimited_text` - support for the
`dynamic_file_system_enabled`, `dynamic_path_enabled`, and
`dynamic_filename_enabled` properties
([#23261](hashicorp/terraform-provider-azurerm#23261
`azurerm_data_factory_dataset_parquet` - support for the
`azure_blob_fs_location` block
([#23261](hashicorp/terraform-provider-azurerm#23261
`azurerm_monitor_diagnostic_setting` - validation to ensure either
`category` or `category_group` are supplied in `enabled_log` and `log`
blocks
([#23308](hashicorp/terraform-provider-azurerm#23308
`azurerm_network_interface` - support for the `auxiliary_mode` and
`auxiliary_sku` properties
([#22979](hashicorp/terraform-provider-azurerm#22979
`azurerm_postgresql_flexible_server` - increased the maximum supported
value for `storage_mb`
([#23277](hashicorp/terraform-provider-azurerm#23277
`azurerm_shared_image_version` - support for the
`replicated_region_deletion_enabled` and
`target_region.exclude_from_latest_enabled` properties
([#23147](hashicorp/terraform-provider-azurerm#23147
`azurerm_storage_account` - support for setting `domain_name` and
`domain_guid` for `AADKERB`
([#22833](hashicorp/terraform-provider-azurerm#22833
`azurerm_storage_account_customer_managed_key` - support for
cross-tenant customer-managed keys with the
`federated_identity_client_id`, and `key_vault_uri` properties
([#20356](hashicorp/terraform-provider-azurerm#20356
`azurerm_web_application_firewall_policy` - support for the
`rate_limit_duration`, `rate_limit_threshold`, `group_rate_limit_by`,
and `request_body_inspect_limit_in_kb` properties
([#23239](https://github.com/hashicorp/terraform-provider-azurerm/issues/23239))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_container_app_environment`: fix
`log_analytics_workspace_name` output to correct value
([#23298](hashicorp/terraform-provider-azurerm#23298
`azurerm_api_management_api` - set the `service_url` property when
importing the resource
([#23011](hashicorp/terraform-provider-azurerm#23011
`azurerm_app_configuration` - prevent crash by nil checking the
encryption configuration
([#23302](hashicorp/terraform-provider-azurerm#23302
`azurerm_app_configuration_feature` - update `percentage_filter_value`
to accept correct type of float
([#23263](hashicorp/terraform-provider-azurerm#23263
`azurerm_container_app` - fix an issue with `commands` and `args` being
overwritten when using multiple containers
([#23338](hashicorp/terraform-provider-azurerm#23338
`azurerm_key_vault_certificate` - fix issue where certificates
couldn&#39;t be recovered anymore
([#23204](hashicorp/terraform-provider-azurerm#23204
`azurerm_key_vault_key` - the ForceNew when `expiration_date` is removed
from the config file
([#23327](hashicorp/terraform-provider-azurerm#23327
`azurerm_linux_function_app` - fix a bug in setting the storage settings
when using Elastic Premium plans
([#21212](hashicorp/terraform-provider-azurerm#21212
`azurerm_linux_web_app` - fix docker app stack update
([#23303](hashicorp/terraform-provider-azurerm#23303
`azurerm_linux_web_app` - fix crash in auto heal expansion
([#21328](hashicorp/terraform-provider-azurerm#21328
`azurerm_linux_web_app_slot` - fix docker app stack update
([#23303](hashicorp/terraform-provider-azurerm#23303
`azurerm_linux_web_app_slot` - fix crash in auto heal expansion
([#21328](hashicorp/terraform-provider-azurerm#21328
`azurerm_log_analytics_solution` - fix bug where the resource wasn&#39;t
handling successful creation on subsequent applies
([#23312](hashicorp/terraform-provider-azurerm#23312
`azurerm_management_group_subscription_association` - fix bug to
correctly mark resource as gone if not found during read
([#23335](hashicorp/terraform-provider-azurerm#23335
`azurerm_mssql_elasticpool` - remove check that prevents `license_type`
from being set for certain skus
([#23262](hashicorp/terraform-provider-azurerm#23262
`azurerm_servicebus_queue` - fixing an issue where `auto_delete_on_idle`
couldn&#39;t be set to `P10675199DT2H48M5.4775807S`
([#23296](hashicorp/terraform-provider-azurerm#23296
`azurerm_servicebus_topic` - fixing an issue where `auto_delete_on_idle`
couldn&#39;t be set to `P10675199DT2H48M5.4775807S`
([#23296](hashicorp/terraform-provider-azurerm#23296
`azurerm_storage_account` - prevent sending unsupported blob properties
in payload for `Storage` account kind
([#23288](hashicorp/terraform-provider-azurerm#23288
`azurerm_synapse_sql_pool` - expose `storage_account_type`
([#23217](hashicorp/terraform-provider-azurerm#23217
`azurerm_windows_function_app` - fix a bug in setting the storage
settings when using Elastic Premium plans
([#21212](hashicorp/terraform-provider-azurerm#21212
`azurerm_windows_web_app` - fix docker app stack update
([#23303](hashicorp/terraform-provider-azurerm#23303
`azurerm_windows_web_app_slot` - fix docker app stack update
([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))&#xA;&#xA;DEPRECATIONS:&#xA;&#xA;*
`azurerm_application_gateway` - deprecate `Standard` and `WAF` skus
([#23310](hashicorp/terraform-provider-azurerm#23310
`azurerm_bot_channel_web_chat` - deprecate `site_names` in favour of
`site` block
([#23161](hashicorp/terraform-provider-azurerm#23161
`azurerm_monitor_diagnostic_setting` - deprecate `retention_policy` in
favour of `azurerm_storage_management_policy`
([#23260](https://github.com/hashicorp/terraform-provider-azurerm/issues/23260))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </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>
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 13, 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.

10 participants