azurerm_data_factory - customer-managed encryption key "cannot parse an empty string" bug fix by gerrytan · Pull Request #28621 · hashicorp/terraform-provider-azurerm

gerrytan · 2025-01-29T02:07:16Z

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 comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This PR fixes a bug where if customer_managed_key_id is set on an existing data factory, customer_managed_key_identity_id is assumed to be present, but it could be an empty string when Managed Service Identity (MSI) is used. As a result user see a parsing error when the provider attempted to parse the json payload back into state.

To reproduce the bug:

In Terraform, deploy a data factory without customer_managed_key_id set and with SystemAssigned identity
Setup a key vault and key for encryption (refer to detailed instruction here)
Set the customer_managed_key_id to the URL of the key created in step 2 and perform another deploy

Expected: Terraform deploy completed with no errors

Actual: Terraform deploy failed with error: Error: parsing "": parsing "": cannot parse an empty string

The reproduction step above reflects the TestAccDataFactory_keyVaultKeyEncryptionWithExistingDataFactoryAndManagedServiceIdentity I implemented in this PR.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_DATAFACTORY/531907

Unfortunately there are 34 pre-existing failures in main, but I have diffed and checked there are no new regressions introduced by this change

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_data_factory - customer-managed encryption key "cannot parse an empty string" bug fix [azurerm_data_factory changes and corrupts customer_managed_key_id implicitly #27717]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #27717, #28256

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

sreallymatt

Thanks @gerrytan, I've left some comments inline

website/docs/r/data_factory.html.markdown

sreallymatt · 2025-10-10T22:54:50Z

internal/services/datafactory/data_factory_resource.go

 				}

-				if encIdentity := enc.Identity; encIdentity != nil && encIdentity.UserAssignedIdentity != nil {
+				if encIdentity := enc.Identity; encIdentity != nil && encIdentity.UserAssignedIdentity != nil && *encIdentity.UserAssignedIdentity != "" {


We can make use of pointer.From here to simplify the condition

Suggested change

if encIdentity := enc.Identity; encIdentity != nil && encIdentity.UserAssignedIdentity != nil && *encIdentity.UserAssignedIdentity != "" {

if encIdentity := enc.Identity; encIdentity != nil && pointer.From(encIdentity.UserAssignedIdentity) != "" {

Thanks, will fix.

sreallymatt · 2025-10-10T22:54:53Z

internal/services/datafactory/data_factory_resource.go

 					parsed, err := commonids.ParseUserAssignedIdentityIDInsensitively(*encIdentity.UserAssignedIdentity)
 					if err != nil {
-						return fmt.Errorf("parsing %q: %+v", *encIdentity.UserAssignedIdentity, err)
+						return fmt.Errorf("parsing %q: %+v", "customer_managed_key_identity_id", err)


While here, we can remove the wrapped error, for ID parsing errors we generally just return the error directly

Suggested change

return fmt.Errorf("parsing %q: %+v", "customer_managed_key_identity_id", err)

return err

Thanks, will fix

github-actions · 2025-11-17T06:05:01Z

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

…fmt.Sprintf

gerrytan · 2025-11-18T06:45:07Z

@sreallymatt I have addressed all your feedbacks, and acctest has been run against latest commit (see description). Please have another look.

sreallymatt

Thanks for adding the CustomizeDiff @gerrytan, I do think for safety it needs minor adjustments, would you mind taking a look at the comment I've left inline?

sreallymatt

Forgot to submit comment... 🙃

sreallymatt · 2025-12-09T17:17:46Z

internal/services/datafactory/validate/datafactory.go

+func CMKIdentityIdRequiredAtCreation(ctx context.Context, d *pluginsdk.ResourceDiff, meta interface{}) error {
+	if d.Id() == "" &&
+		d.Get("customer_managed_key_id").(string) != "" &&
+		d.Get("customer_managed_key_identity_id").(string) == "" {


Since these 2 CMK properties are IDs, it's likely these will be unknown at initial plan time, while it won't cause issues if both are unknown/computed it will be problematic if only customer_managed_key_identity_id is.

To be safe, we should probably use RawConfig and .IsNull checks, e.g.

if d.Id() == "" { rawConfig := d.GetRawConfig().AsValueMap() rawCMK := rawConfig["customer_managed_key_id"] rawCMKIdentity := rawConfig["customer_managed_key_identity_id"] if !rawCMK.IsNull() && rawCMKIdentity.IsNull() { return fmt.Errorf("`customer_managed_key_identity_id` is required when creating a new Data Factory with `customer_managed_key_id`") } }

What do you think?

Hey that is a good trick, thank you @sreallymatt I have pushed this change. Also I will document this in the best-practices.md in a separate PR.

@sreallymatt can you please have a look at this follow up PR where I documented this in the best-practices.md: #31355 🙏

gerrytan · 2025-12-14T23:06:18Z

Thanks for the feedback @sreallymatt , good for you to take another look.

I have triggered another acctest run: https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_DATAFACTORY/556638

Failures have been checked pre-existing on main

sreallymatt

Thanks @gerrytan - LGTM ✅

github-actions · 2026-01-15T02:29:52Z

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.

azurerm_data_factory: customer_managed_key_identity_id bug fix

0718a7a

gerrytan requested a review from a team as a code owner January 29, 2025 02:07

github-actions bot added bug documentation service/data-factory size/M waiting-response labels Jan 29, 2025

gerrytan mentioned this pull request Jan 29, 2025

azurerm_data_factory changes and corrupts customer_managed_key_id implicitly #27717

Closed

1 task

tflint fix

10ccc1b

github-actions bot removed the waiting-response label Jan 29, 2025

gerrytan mentioned this pull request Jan 29, 2025

Getting parsing "": parsing "" cannot parsing an empty string in azurerm_data_factory; I am getting this parsing error post imported the CMK #28256

Open

1 task

sreallymatt requested changes Oct 10, 2025

View reviewed changes

github-actions bot assigned sreallymatt Oct 10, 2025

github-actions bot added the waiting-response label Oct 10, 2025

github-actions bot added the stale label Nov 17, 2025

gerrytan removed the stale label Nov 17, 2025

gerrytan added 3 commits November 18, 2025 10:57

Merge branch 'main' into gerrytan/datafactory-cmk-bugfix

003ae81

CustomizeDiff: added validate.CMKIdentityIdRequiredAtCreation

051f2d6

resourceDataFactoryRead: use pointerFrom and avoid wrapping error

35e5df5

gerrytan requested review from WodansSon and magodo as code owners November 18, 2025 04:30

github-actions bot added size/L waiting-response and removed waiting-response size/M labels Nov 18, 2025

data_factory_resource_test.go: fix golangci-lint - avoid unnecessary …

c876f46

…fmt.Sprintf

github-actions bot removed the waiting-response label Nov 18, 2025

gerrytan requested a review from sreallymatt November 18, 2025 06:45

sreallymatt requested changes Dec 9, 2025

View reviewed changes

github-actions bot added the waiting-response label Dec 9, 2025

sreallymatt reviewed Dec 9, 2025

View reviewed changes

gerrytan added 3 commits December 12, 2025 16:48

Merge branch 'main' into gerrytan/datafactory-cmk-bugfix

128d623

Use GetRawConfig in CustomizeDiff to handle known-after-apply values

4082b65

Merge branch 'main' into gerrytan/datafactory-cmk-bugfix

578e4e5

github-actions bot removed the waiting-response label Dec 14, 2025

gerrytan requested a review from sreallymatt December 14, 2025 23:04

gerrytan mentioned this pull request Dec 15, 2025

best-practices.md: Using GetRawConfig() in CustomizeDiff #31355

Open

10 tasks

sreallymatt approved these changes Dec 15, 2025

View reviewed changes

sreallymatt merged commit c3b45ca into hashicorp:main Dec 15, 2025
35 checks passed

github-actions bot added this to the v4.57.0 milestone Dec 15, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2026

	if encIdentity := enc.Identity; encIdentity != nil && encIdentity.UserAssignedIdentity != nil && *encIdentity.UserAssignedIdentity != "" {
	if encIdentity := enc.Identity; encIdentity != nil && pointer.From(encIdentity.UserAssignedIdentity) != "" {

	return fmt.Errorf("parsing %q: %+v", "customer_managed_key_identity_id", err)
	return err

Conversation

gerrytan commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sreallymatt Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

gerrytan Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

sreallymatt Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

gerrytan Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 17, 2025

Uh oh!

gerrytan commented Nov 18, 2025

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

sreallymatt Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

gerrytan Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

gerrytan Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

gerrytan commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sreallymatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gerrytan commented Jan 29, 2025 •

edited

Loading

gerrytan commented Dec 14, 2025 •

edited

Loading