terraform-provider-azurerm icon indicating copy to clipboard operation
terraform-provider-azurerm copied to clipboard

`azurerm_app_configuration` - support for the `encrption`, `local_auth_enabled`, `public_network_access_enabled`, `purge_protection_enabled`,and `soft_delete_retention_days` properties

Open teowa opened this issue 2 years ago • 6 comments

Resolves https://github.com/hashicorp/terraform-provider-azurerm/issues/17408 , resolves https://github.com/hashicorp/terraform-provider-azurerm/issues/16915 azurerm_app_configuration support for encrption, local_auth_enabled, public_network_access_enabled, purge_protection_enabled,and soft_delete_retention_days properties.

For public_network_access_enabled, find a existing PR https://github.com/hashicorp/terraform-provider-azurerm/pull/17549, which can be merged before this one.

Reference:

Test Result
make acctests SERVICE='appconfiguration' TESTARGS='-run=TestAccAppConfiguration_' TESTTIMEOUT='60m'
==> 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 -run=TestAccAppConfiguration_ -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAppConfiguration_free
=== PAUSE TestAccAppConfiguration_free
=== RUN   TestAccAppConfiguration_standard
=== PAUSE TestAccAppConfiguration_standard
=== RUN   TestAccAppConfiguration_requiresImport
=== PAUSE TestAccAppConfiguration_requiresImport
=== RUN   TestAccAppConfiguration_complete
=== PAUSE TestAccAppConfiguration_complete
=== RUN   TestAccAppConfiguration_identity
=== PAUSE TestAccAppConfiguration_identity
=== RUN   TestAccAppConfiguration_identityUserAssigned
=== PAUSE TestAccAppConfiguration_identityUserAssigned
=== RUN   TestAccAppConfiguration_identityUpdated
=== PAUSE TestAccAppConfiguration_identityUpdated
=== RUN   TestAccAppConfiguration_update
=== PAUSE TestAccAppConfiguration_update
=== RUN   TestAccAppConfiguration_encryption
=== PAUSE TestAccAppConfiguration_encryption
=== RUN   TestAccAppConfiguration_encryptionUpdated
=== PAUSE TestAccAppConfiguration_encryptionUpdated
=== RUN   TestAccAppConfiguration_softDeleteRecovery
=== PAUSE TestAccAppConfiguration_softDeleteRecovery
=== RUN   TestAccAppConfiguration_softDeleteRecoveryDisabled
=== PAUSE TestAccAppConfiguration_softDeleteRecoveryDisabled
=== RUN   TestAccAppConfiguration_softDeletePurgeThenRecreate
=== PAUSE TestAccAppConfiguration_softDeletePurgeThenRecreate
=== RUN   TestAccAppConfiguration_purgeProtectionEnabled
=== PAUSE TestAccAppConfiguration_purgeProtectionEnabled
=== RUN   TestAccAppConfiguration_purgeProtectionAndSoftDeleteEnabled
=== PAUSE TestAccAppConfiguration_purgeProtectionAndSoftDeleteEnabled
=== RUN   TestAccAppConfiguration_purgeProtectionViaUpdate
=== PAUSE TestAccAppConfiguration_purgeProtectionViaUpdate
=== RUN   TestAccAppConfiguration_purgeProtectionAttemptToDisable
=== PAUSE TestAccAppConfiguration_purgeProtectionAttemptToDisable
=== CONT  TestAccAppConfiguration_free
=== CONT  TestAccAppConfiguration_encryptionUpdated
=== CONT  TestAccAppConfiguration_identityUserAssigned
=== CONT  TestAccAppConfiguration_complete
=== CONT  TestAccAppConfiguration_requiresImport
=== CONT  TestAccAppConfiguration_standard
=== CONT  TestAccAppConfiguration_update
=== CONT  TestAccAppConfiguration_encryption
--- PASS: TestAccAppConfiguration_requiresImport (150.57s)
=== CONT  TestAccAppConfiguration_identity
--- PASS: TestAccAppConfiguration_identityUserAssigned (195.42s)
=== CONT  TestAccAppConfiguration_identityUpdated
--- PASS: TestAccAppConfiguration_standard (202.92s)
=== CONT  TestAccAppConfiguration_purgeProtectionEnabled
=== CONT  TestAccAppConfiguration_purgeProtectionAttemptToDisable
--- PASS: TestAccAppConfiguration_free (203.58s)
--- PASS: TestAccAppConfiguration_identity (110.87s)
=== CONT  TestAccAppConfiguration_purgeProtectionViaUpdate
--- PASS: TestAccAppConfiguration_purgeProtectionEnabled (169.98s)
=== CONT  TestAccAppConfiguration_purgeProtectionAndSoftDeleteEnabled
--- PASS: TestAccAppConfiguration_complete (376.18s)
=== CONT  TestAccAppConfiguration_softDeleteRecoveryDisabled
--- PASS: TestAccAppConfiguration_encryption (377.90s)
=== CONT  TestAccAppConfiguration_softDeletePurgeThenRecreate
--- PASS: TestAccAppConfiguration_purgeProtectionAttemptToDisable (193.73s)
=== CONT  TestAccAppConfiguration_softDeleteRecovery
--- PASS: TestAccAppConfiguration_encryptionUpdated (429.24s)
--- PASS: TestAccAppConfiguration_purgeProtectionAndSoftDeleteEnabled (101.06s)
--- PASS: TestAccAppConfiguration_identityUpdated (285.05s)
--- PASS: TestAccAppConfiguration_purgeProtectionViaUpdate (219.24s)
--- PASS: TestAccAppConfiguration_update (505.31s)
--- PASS: TestAccAppConfiguration_softDeleteRecoveryDisabled (159.03s)
--- PASS: TestAccAppConfiguration_softDeletePurgeThenRecreate (214.60s)
--- PASS: TestAccAppConfiguration_softDeleteRecovery (270.63s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appconfiguration      669.153s

teowa avatar Jul 22 '22 05:07 teowa

I believe this also resolves #16915

jason-johnson avatar Aug 09 '22 08:08 jason-johnson

I find this is caused by service behaviour, If I PUT the same named resource right after the previous one is successfully purged, PUT will return a NameUnavailable error. There is gap (serveral minites) between purging the soft-deleted and creating a new resource with a same name, may releated to https://github.com/Azure/AppConfiguration/issues/677 https://github.com/Azure/AppConfiguration/issues/264. Service team has confirmed this gap is by design.

Now I am going to add a method in Create method to retry checkNameAvailability before PUT.

teowa avatar Aug 24 '22 09:08 teowa

I find this is caused by service behaviour, there is gap (serveral minites) between purging (POST method to purge soft-deleted) and creating a new resource with same name, may releated to Azure/AppConfiguration#264. If I PUT the same named resource right after the previous one is successfully purged, it will return a NameUnavailable error. Now I am going to add a method in Create method to retry checkNameAvailability before PUT.

Would this not mean that this case only occurs when one rapidly deletes and then creates the resource? Would that not mean it's mostly a problem with testing and unlikely to be an issue in actual practice?

jason-johnson avatar Aug 25 '22 12:08 jason-johnson

@jason-johnson thanks for the comment. Yes the error only occurs with creation right after the deleting. But if we cannot re-create the resource after purging the old one in an automated way, this means this resource will not support for:

  • Forcing Re-creation operations
  • Modifing some Create-Only properties, also called a ForceNew property in Terraform (if users want to modify properties that can only be set in initial create but cannot be modified later, Terraform will delete the old one first and then recreate the whole resource with same name as a replacement). For now the soft_delete_retention_days of App Configuration is a Create-Only property, and maybe there will be more in the future. And I should add a test case for this.

So I think it is neccessary to support such a capability.

teowa avatar Aug 26 '22 03:08 teowa

Blocked for this POST method checkNameAvailability is not in the generated SDK appconfiguration 2022-05-01. I have submitted an issue.

teowa avatar Aug 31 '22 05:08 teowa

Hi @katbyte , is it possible to merge this PR first and later add the retry checkNameAvailability after SDK is ready. As long as users don't recreate the resource shortly after the previous one is deleted, other functions works well. Actually the current provider also has the same issue (cannot recreate quickly).

teowa avatar Sep 23 '22 02:09 teowa

@teowa - if it returns unavailable then on delete can we not just do a state-wait on checkname until it returns free? thus ensuring the delete is complete by the time it returns?

katbyte avatar Sep 26 '22 17:09 katbyte

@teowa - if it returns unavailable then on delete can we not just do a state-wait on checkname until it returns free? thus ensuring the delete is complete by the time it returns?

Yes, checkname on delete can ensure the delete is complete. Instead, checkname before create can ensure the name used in create is available and no NameUnavailable error in PUT, this can cope with situations where user delete resource out of Terraform scope and right after that user want to recreate the same named one using Terraform. Maybe both checkname on delete and checkname before create should be added to ensure.

Apart from above, currently the checkname function is blocked by SDK, can we add the checkname later and merge this PR first? I believe features added in this PR can function well (except recreation and ForceNew properties, I should add this in doc).

teowa avatar Sep 27 '22 02:09 teowa

That makes sense to me @teowa - could you add a comment where that fix would go, and then remove the workarounds in the tests so they fail so we know they need to be fixed?

katbyte avatar Oct 12 '22 16:10 katbyte

This functionality has been released in v3.27.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[bot] avatar Oct 14 '22 01:10 github-actions[bot]

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[bot] avatar Nov 13 '22 02:11 github-actions[bot]