azurerm_app_configuration_key, azurerm_app_configuration_feature - change resource ID to Data Plane URL format

[teowa]

Resolves #20038
Resolves #20849

Context:

The error is because of using /AppConfigurationKey/SAMPLE/Label as label, which is sourced from the App Configuration Key ID format in Azurerm: /subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resourceGroup1/providers/Microsoft.AppConfiguration/configurationStores/appConf1/AppConfigurationKey/{appConfKey1}/Label/{label1}, currently we support / in the {appConfKey1} and {label1} segments and stores the / directly in the resource ID (e.g. /AppConfigurationKey/key:name/test/Label/test:label/name" is a ID with key:name/test as the key and test:label/name as the label). This can ease the use of import feature with straight forward resource ID, but has some limitations like using /Label/ or /AppConfigurationKey/ literal in label will cause some conflict in ID parsing. To copy with using /Label/ in label property, two possible solutions are:

1. Escape the configuration Key and the label (e.g. transform /Label/ to %2FLabel%2F) and then stores to the ID, but this makes the resource ID not so straight forward. 
2. Keep the current logic to store the /Label/ directly to ID and forbidden using /Label/ literal in label property.

This PR implements the second of above, keeps the current ID format (store the / instead of escaped %2F), but returns the error message telling users not to use /Label/ literal label property, and since current used regex is greedy, we can use the /AppConfigurationKey/ in label. what do you think?

---

Update

change resource ID to valid data plane URL format:

• azurerm_app_configuration_feature
  • https://appconfname1.azconfig.io/kv/.appconfig.featureflag%2FkeyName?label=%00 (for no label)
  • https://appconfname1.azconfig.io/kv/.appconfig.featureflag%2FkeyName?label=labelName (for when a label is present)
• azurerm_app_configuration_key
  • https://appconfname1.azconfig.io/kv/keyName?label=labelName?label=%00 (for no label)
  • https://appconfname1.azconfig.io/kv/keyName?label=labelName (for when a label is present)

the keyName in URL path should be escaped with Golang url.PathEscape method and the labelName in URL query is escaped with url.QueryEscape method.

Test:

make acctests SERVICE="appconfiguration" TESTARGS="-parallel 20 -test.run=TestAccAppConfigurationKey_compli" TESTTIMEOUT='1440m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/appconfiguration -parallel 20 -test.run=TestAccAppConfigurationKey_compli -timeout 1440m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAppConfigurationKey_complicatedKeyLabel
=== PAUSE TestAccAppConfigurationKey_complicatedKeyLabel
=== CONT  TestAccAppConfigurationKey_complicatedKeyLabel
--- PASS: TestAccAppConfigurationKey_complicatedKeyLabel (356.43s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appconfiguration      359.890s

[katbyte]

@teowa - i'm not sure this is the correct solution, given if it IS valid to have label in the name we should support it. This might be something we need to write a custom id parser for that is a little smarter.

@tombuildsstuff any ideas/comments?

[tombuildsstuff]

@teowa thanks for this PR.

So whilst this approach solves this issue to some extent - I think the other question here is why are these Data Plane resources using a Resource Manager ID at all? Whilst some of the functionality for App Configuration is available within the Resource Manager API - the primary interface for configuration is the Data Plane API which comes with it's own set of Resource IDs.

Whilst it's possible to use Resource Manager IDs for these resources, since the validation rules are different between Data Plane and Resource Manager (insofar as Resource Manager requires Key-Value pairs in the URI, whereas Data Plane allows any arbitrary expression) - so whilst URI-encoding the key would work - I think we'd be better to switch the Resource ID being used for these resources to match what the API's expecting here.

Doing so would allow the use of a more bespoke parser as @katbyte has suggested, which I'd agree would remove the problem here entirely - so I think would be the better approach here, and whilst this would require a State Migration to do, I think would probably make more sense than trying to shoe-horn this into the Resource Manager ID, since this is ultimately using the Data Plane SDK/API.

As such would you be able to take a look into updating the Resource ID format being used for these resources to match:

1. azurerm_app_configuration_feature - https://{store-name}.{dns-suffix-from-azure-environment}/keys?name=.appconfig.featureflag/{featureName} (for no label) and https://{store-name}.{dns-suffix-from-azure-environment/keys?name=.appconfig.featureflag/{featureName}&label={label} (for when a label is present)
2. azurerm_app_configuration_key - https://{store-name}.{dns-suffix-from-azure-environment}/keys?name={key} (for no label) and https://{store-name}.{dns-suffix-from-azure-environment}/keys?name={key}&label={label} (for when a label is present)

Whilst that may seem like an odd choice - particularly for azurerm_app_configuration_feature - it both matches how the API behaves and means that we can use Go's URI parsing logic to handle that - meaning we should be able to use the same approach as we do for the Key Vault Nested Item parsers, rather than the generic ParseResourceID function which is intended for Azure Resource IDs.

As such, would you mind taking a look into adding the ID parsers, and then adding state migrations to update these?

Thanks!

[teowa]

Current parser accept either of below format ID for no label:

1. https://{store-name}.azconfig.io/kv/{key}?label=
2. https://{store-name}.azconfig.io/kv/{key}?label=%00
3. https://{store-name}.azconfig.io/kv/{key}

And ID string in state file uses the 2nd. Should there be a strict parser? Thanks.

[katbyte]

@teowa - i think 1 or 2 is likely the best option, doesn't seem to be any point in making it more complex for a user by requiring %00. if i was a user i'd expect https://{store-name}.azconfig.io/kv/{key}?label= to give me an empty label.

could we also please update the PR title to reflect the id change? thanks

[teowa]

@katbyte The parser accept all of above format, but to keep consistence with current ID format, %00 is dumped to state for empty label.

[katbyte]

@teowa - discussing internally yes it would be best if the parser would only accept 1 format to prevent confusion with users in what format to use and potentially issues later on - lets go with #1 as expecting users to know/handle %00 isn't useful?

[teowa]

--- PASS: TestAccAppConfigurationKeysDataSource_key (166.86s)
--- PASS: TestAccAppConfigurationKeysDataSource_label (167.86s)
--- PASS: TestAccAppConfigurationKeyDataSource_basic (171.86s)
--- PASS: TestAccAppConfigurationKeysDataSource_allkeys (172.08s)
--- PASS: TestAccAppConfigurationKeyDataSource_basicNoLabel (172.80s)
--- PASS: TestAccAppConfigurationKey_basicNoLabel (185.61s)
--- PASS: TestAccAppConfigurationKey_slash (185.98s)
--- PASS: TestAccAppConfigurationKey_basicNoLabel_afterLabel (188.28s)
--- PASS: TestAccAppConfigurationKey_complicatedKeyLabel (188.65s)
--- PASS: TestAccAppConfigurationKey_basic (189.41s)
--- PASS: TestAccAppConfigurationKey_requiresImport (200.84s)
--- PASS: TestAccAppConfigurationKey_lockUpdate (238.31s)
--- PASS: TestAccAppConfigurationKeyDataSource_basicVault (315.91s)
--- PASS: TestAccAppConfigurationKey_basicVault (328.21s)
--- PASS: TestAccAppConfigurationKey_KVToVault (427.07s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appconfiguration      427.141s

--- PASS: TestAccAppConfigurationFeature_basicNoLabel (172.77s)
--- PASS: TestAccAppConfigurationFeature_basicWithSlash (173.35s)
--- PASS: TestAccAppConfigurationFeature_basic (175.58s)
--- PASS: TestAccAppConfigurationFeature_complicatedKeyLabel (176.20s)
--- PASS: TestAccAppConfigurationFeature_basicNoFilters (178.01s)
--- PASS: TestAccAppConfigurationFeature_requiresImport (195.56s)
--- PASS: TestAccAppConfigurationFeature_lockUpdate (227.71s)
--- PASS: TestAccAppConfigurationFeature_enabledUpdate (229.46s)
--- PASS: TestAccAppConfigurationFeature_noLabelUpdate (256.69s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appconfiguration      256.747s

[katbyte]

aside from 2 docs comments this LGTM 🧪

[teowa]

Hi @katbyte, thanks for reviewing this. I have also enhanced the state migration to fix cases without label in issue #20849, please kindly take another look.

[github-actions]

This functionality has been released in v3.49.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

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.