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_app_configuration - support for the encrption, local_auth_enabled, public_network_access_enabled, purge_protection_enabled,and soft_delete_retention_days properties #17714

Merged
merged 9 commits into from
Oct 13, 2022

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Jul 22, 2022

Resolves #17408 , resolves #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 #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

@jason-johnson
Copy link

I believe this also resolves #16915

@teowa teowa changed the title azurerm_app_configuration support for more properties azurerm_app_configuration support for encrption, local_auth_enabled, public_network_access_enabled, purge_protection_enabled,and soft_delete_retention_days Aug 15, 2022
@teowa teowa changed the title azurerm_app_configuration support for encrption, local_auth_enabled, public_network_access_enabled, purge_protection_enabled,and soft_delete_retention_days azurerm_app_configuration - support for the encrption, local_auth_enabled, public_network_access_enabled, purge_protection_enabled,and soft_delete_retention_days properties Aug 15, 2022
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.

failing test:

------- Stdout: -------
=== RUN   TestAccAppConfiguration_softDeletePurgeThenRecreate
=== PAUSE TestAccAppConfiguration_softDeletePurgeThenRecreate
=== CONT  TestAccAppConfiguration_softDeletePurgeThenRecreate
    testcase.go:110: Step 4/5 error: Error running apply: exit status 1
        
        Error: creating Configuration Store (Subscription: "*******"
        Resource Group Name: "acctestRG-appconfig-220818172108466617"
        Config Store Name: "testaccappconf220818172108466617"): polling after Create: Code="NameUnavailable" Message="The specified name is already in use." AdditionalInfo=[{"info":{"activityId":"97d8f3d1-da74-427f-a5a2-be22e2ffdcd9"},"type":"ActivityId"}]
        
          with azurerm_app_configuration.test,
          on terraform_plugin_test.tf line 15, in resource "azurerm_app_configuration" "test":
          15: resource "azurerm_app_configuration" "test" {
        
    testing_new.go:84: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: deleting Resource Group "acctestRG-appconfig-220818172108466617": the Resource Group still contains Resources.
        
        Terraform is configured to check for Resources within the Resource Group when deleting the Resource Group - and
        raise an error if nested Resources still exist to avoid unintentionally deleting these Resources.
        
        Terraform has detected that the following Resources still exist within the Resource Group:
        
        * `/subscriptions/*******/resourceGroups/acctestRG-appconfig-220818172108466617/providers/Microsoft.AppConfiguration/configurationStores/testaccappconf220818172108466617`
        
        This feature is intended to avoid the unintentional destruction of nested Resources provisioned through some
        other means (for example, an ARM Template Deployment) - as such you must either remove these Resources, or
        disable this behaviour using the feature flag `prevent_deletion_if_contains_resources` within the `features`
        block when configuring the Provider, for example:
        
        provider "azurerm" {
          features {
            resource_group {
              prevent_deletion_if_contains_resources = false
            }
          }
        }
        
        When that feature flag is set, Terraform will skip checking for any Resources within the Resource Group and
        delete this using the Azure API directly (which will clear up any nested resources).
        
        More information on the `features` block can be found in the documentation:
        https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#features
        
        
--- FAIL: TestAccAppConfiguration_softDeletePurgeThenRecreate (814.75s)
FAIL

likely this is due to the delete returning too early?

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.

still have a test failure

------- Stdout: -------
=== RUN   TestAccAppConfiguration_softDeletePurgeThenRecreate
=== PAUSE TestAccAppConfiguration_softDeletePurgeThenRecreate
=== CONT  TestAccAppConfiguration_softDeletePurgeThenRecreate
    testcase.go:110: Step 4/5 error: Error running apply: exit status 1
        
        Error: creating Configuration Store (Subscription: "*******"
        Resource Group Name: "acctestRG-appconfig-220823153556433279"
        Config Store Name: "testaccappconf220823153556433279"): polling after Create: Code="NameUnavailable" Message="The specified name is already in use." AdditionalInfo=[{"info":{"activityId":"adca45ed-5098-4946-9a48-0276995f957f"},"type":"ActivityId"}]
        
          with azurerm_app_configuration.test,
          on terraform_plugin_test.tf line 15, in resource "azurerm_app_configuration" "test":
          15: resource "azurerm_app_configuration" "test" {
        
    testing_new.go:84: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: deleting Resource Group "acctestRG-appconfig-220823153556433279": the Resource Group still contains Resources.
        
        Terraform is configured to check for Resources within the Resource Group when deleting the Resource Group - and
        raise an error if nested Resources still exist to avoid unintentionally deleting these Resources.
        
        Terraform has detected that the following Resources still exist within the Resource Group:
        
        * `/subscriptions/*******/resourceGroups/acctestRG-appconfig-220823153556433279/providers/Microsoft.AppConfiguration/configurationStores/testaccappconf220823153556433279`
        
        This feature is intended to avoid the unintentional destruction of nested Resources provisioned through some
        other means (for example, an ARM Template Deployment) - as such you must either remove these Resources, or
        disable this behaviour using the feature flag `prevent_deletion_if_contains_resources` within the `features`
        block when configuring the Provider, for example:
        
        provider "azurerm" {
          features {
            resource_group {
              prevent_deletion_if_contains_resources = false
            }
          }
        }
        
        When that feature flag is set, Terraform will skip checking for any Resources within the Resource Group and
        delete this using the Azure API directly (which will clear up any nested resources).
        
        More information on the `features` block can be found in the documentation:
        https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs#features
        
        
--- FAIL: TestAccAppConfiguration_softDeletePurgeThenRecreate (797.64s)
FAIL

@teowa
Copy link
Contributor Author

teowa commented Aug 24, 2022

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 Azure/AppConfiguration#677 Azure/AppConfiguration#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.

@jason-johnson
Copy link

jason-johnson commented Aug 25, 2022

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?

@teowa
Copy link
Contributor Author

teowa commented Aug 26, 2022

@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 teowa marked this pull request as draft August 29, 2022 02:51
@teowa
Copy link
Contributor Author

teowa commented Aug 31, 2022

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

@teowa
Copy link
Contributor Author

teowa commented Sep 23, 2022

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).

ForceNew: true,
ValidateFunc: validation.IntBetween(1, 7),
DiffSuppressFunc: func(_, old, new string, _ *pluginsdk.ResourceData) bool {
return old == "0"
Copy link
Contributor Author

@teowa teowa Sep 23, 2022

Choose a reason for hiding this comment

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

Here this is because service default value is 0 for free sku, but 7 for standard sku.

@teowa teowa marked this pull request as ready for review September 23, 2022 02:51
@teowa teowa requested a review from katbyte September 23, 2022 03:02
@katbyte
Copy link
Collaborator

katbyte commented Sep 26, 2022

@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?

@teowa
Copy link
Contributor Author

teowa commented Sep 27, 2022

@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).

@katbyte
Copy link
Collaborator

katbyte commented Oct 12, 2022

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?

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.

Thanks @teowa ! LGTM 🍰

@katbyte katbyte merged commit 31c094c into hashicorp:main Oct 13, 2022
@github-actions github-actions bot added this to the v3.27.0 milestone Oct 13, 2022
katbyte added a commit that referenced this pull request Oct 13, 2022
@github-actions
Copy link

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
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 Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants