`azurerm_api_management` : fix API Management global policy is deleted temporarily issue #23474

sinbai · 2023-10-07T09:09:17Z

The purpose of this PR:

Removed useless "remove the existing policy" request in APIM service creation.
When updating the APIM service, update or delete the policy based on tf configuration instead of first deleting the existing policy and then adding the policy to fix API Management global policy is deleted temporarily when modifying them #23396.

Test result:
PASS: TestAccApiManagement_policy (2754.61s)

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

manicminer

@sinbai Thanks for working on this. We will need to conditionally delete the policy in the update func, otherwise it will be impossible to remove / nothing will happen if it is removed from configuration.

sinbai · 2023-10-11T07:29:01Z

@sinbai Thanks for working on this. We will need to conditionally delete the policy in the update func, otherwise it will be impossible to remove / nothing will happen if it is removed from configuration.

Hi @manicminer thanks for your feedback. The code has been updated. Could you please take another look?

manicminer · 2023-10-12T20:58:28Z

internal/services/apimanagement/api_management_resource.go

+		} else {
+			oldPolicy, newPolicy := d.GetChange("policy")
+			if len(oldPolicy.([]interface{})) > 0 && len(newPolicy.([]interface{})) == 0 {
+				if resp, err := policyClient.Delete(ctx, policyServiceId, policy.DeleteOperationOptions{}); err != nil {
+					if !response.WasNotFound(resp.HttpResponse) {
+						return fmt.Errorf("removing Policies from %s: %+v", id, err)
+					}
+				}
+			}


Rather than trying to determine whether the policy list has changed from >zero to zero elements, we should just check whether the current length is zero. Users can use ignore_changes if they do not wish to manage this property.

Thanks for your time, the code has been updated, could you please take a look again?

Hi @manicminer I removed the conditional policy deletion in the update func because I assume that if d.HasChange("policy") and policyContract == nil, there is only one possibility, which is to change the computed + optional property policy from being configured to being empty(policy = [] ) in TF configuration. So I assume that there is no need to add a conditional deletion here, any feedback feel free let me know, thank you.

stephybun

Thanks for this PR @sinbai.

It looks like this change may have introduced some test failures? It's difficult to tell given the number of failures in the acceptance tests but this error message shows up a couple of times and isn't found on the tests run against the main branch - could you please investigate?

------- Stdout: -------
=== RUN   TestAccApiManagement_softDeleteRecovery
=== PAUSE TestAccApiManagement_softDeleteRecovery
=== CONT  TestAccApiManagement_softDeleteRecovery
    testcase.go:113: Step 4/5 error: Error running apply: exit status 1
        Error: recovering Service (Subscription: "*******"
        Resource Group Name: "acctestRG-231030123449622871"
        Service Name: "acctestAM-231030123449622871"): unexpected status 409 with error: ServiceLocked: The API Service acctestAM-231030123449622871 is transitioning at this time. Please try the request again later.
          with azurerm_api_management.test,
          on terraform_plugin_test.tf line 26, in resource "azurerm_api_management" "test":
          26: resource "azurerm_api_management" "test" {
--- FAIL: TestAccApiManagement_softDeleteRecovery (329.46s)
FAIL

------- Stdout: -------
=== RUN   TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId
=== PAUSE TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId
=== CONT  TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId
    testing_new.go:90: Error running post-test destroy, there may be dangling resources: exit status 1
        Error: purging the deleted Service (Subscription: "*******"
        Resource Group Name: "acctestRG-231030122614304293"
        Service Name: "acctestAM-231030122614304293"): unexpected status 409 with error: ServiceLocked: The API Service acctestAM-231030122614304293 is transitioning at this time. Please try the request again later.
--- FAIL: TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId (1745.41s)
FAIL

sinbai · 2023-11-01T08:07:05Z

Thanks for this PR @sinbai.

It looks like this change may have introduced some test failures? It's difficult to tell given the number of failures in the acceptance tests but this error message shows up a couple of times and isn't found on the tests run against the main branch - could you please investigate?

------- Stdout: -------
=== RUN   TestAccApiManagement_softDeleteRecovery
=== PAUSE TestAccApiManagement_softDeleteRecovery
=== CONT  TestAccApiManagement_softDeleteRecovery
    testcase.go:113: Step 4/5 error: Error running apply: exit status 1
        Error: recovering Service (Subscription: "*******"
        Resource Group Name: "acctestRG-231030123449622871"
        Service Name: "acctestAM-231030123449622871"): unexpected status 409 with error: ServiceLocked: The API Service acctestAM-231030123449622871 is transitioning at this time. Please try the request again later.
          with azurerm_api_management.test,
          on terraform_plugin_test.tf line 26, in resource "azurerm_api_management" "test":
          26: resource "azurerm_api_management" "test" {
--- FAIL: TestAccApiManagement_softDeleteRecovery (329.46s)
FAIL

------- Stdout: -------
=== RUN   TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId
=== PAUSE TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId
=== CONT  TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId
    testing_new.go:90: Error running post-test destroy, there may be dangling resources: exit status 1
        Error: purging the deleted Service (Subscription: "*******"
        Resource Group Name: "acctestRG-231030122614304293"
        Service Name: "acctestAM-231030122614304293"): unexpected status 409 with error: ServiceLocked: The API Service acctestAM-231030122614304293 is transitioning at this time. Please try the request again later.
--- FAIL: TestAccApiManagement_identityUserAssignedHostnameConfigurationsKeyVaultId (1745.41s)
FAIL

Hi @stephybun thank you for your time. I don't think this change would cause the above tests to fail, as this change only affects the case where the policy block is set. However, the above test cases do not set policy block in config. BTW, I could not reproduce the error locally, the tests passed locally. Also, It seems that the main branch also has the same error, as shown below:

stephybun · 2023-11-01T08:18:56Z

@sinbai the large amount of eventual consistency failures in APIM is making it quite difficult to validate fixes or enhancements to resources within this service. Could you please investigate and open a separate PR to fix these? It's becoming problematic for all APIM PRs. cc. @mybayern1974

katbyte · 2023-12-07T04:41:22Z

@sinbai - any updates?

sinbai · 2023-12-07T07:18:01Z

@sinbai - any updates?

Hi @katbyte after investigation, it was found that while deleting the resourcegroup, the Resources_ListByResourceGroup API discovered that the APIM service existed. In fact, calling the ApiManagementService_Get API at this time found that the APIM service no longer exists. I assume that this is an API eventual consistency issue. I have contacted the service team and have received no valuable information so far. On the Terraform side, I think all I can do now is add prevent_deletion_if_contains_resources = false and I can submit a PR if necessary to fix the large number of Acctest failures. If you have any feedback please feel free to let me know, thank you:).

katbyte · 2023-12-08T01:33:09Z

I think all I can do now is add prevent_deletion_if_contains_resources = false

this doesn't fix it for the end user thou

I assume that this is an API eventual consistency issue.

so you have actually confirmed that the rg thinks the apim service is still within? What is that api call returning? but then a d.get does not show it?

sinbai · 2023-12-08T02:10:48Z

I think all I can do now is add prevent_deletion_if_contains_resources = false

this doesn't fix it for the end user thou

I assume that this is an API eventual consistency issue.

so you have actually confirmed that the rg thinks the apim service is still within? What is that api call returning? but then a d.get does not show it?

Hi @katbyte , thanks for your response. Yes, I added code in TF and when the eventual consistency issue occurs, I got the following logs, see below for details:

• The log of calling the ApiManagementService_Get API after APIM service deletion is complete, before calling Resources_ListByResourceGroup:

2023/11/02 12:50:46 AzureRM Request: GET https://management.azure.com/subscriptions/*******/resourceGroups/acctestRG-231102122242943723/providers/Microsoft.ApiManagement/service/acctestAM-231102122242943723?api-version=2022-08-01 HTTP/1.1
2023/11/02 12:50:47 Response: {"error":{"code":"ServiceNotFound","message":"Api service does not exist: acctestAM-231102122242943723","details":null,"innerError":null}}

• The log of calling the Resources_ListByResourceGroup API after getting the Api service does not exist:

2023/11/02 12:51:52 AzureRM Request: GET /subscriptions/*******/resourceGroups/acctestRG-231102122242943723/resources?%24expand=provisioningState&api-version=2020-06-01 HTTP/1.1
2023/11/02 12:51:52 AzureRM Response for https://management.azure.com/subscriptions/*******/resourceGroups/acctestRG-231102122242943723/resources?%24expand=provisioningState&api-version=2020-06-01:

{
   "value":[
      {
         "id":"/subscriptions/*******/resourceGroups/acctestRG-231102122242943723/providers/Microsoft.ApiManagement/service/acctestAM-231102122242943723",
         "name":"acctestAM-231102122242943723",
         "type":"Microsoft.ApiManagement/service",
         "sku":{
            "name":"Developer",
            "capacity":1
         },
         "location":"eastus2",
         "provisioningState":"Succeeded",
         "tags":{
            
         },
         "systemData":{
            "createdBy":"*******",
            "createdByType":"Application",
            "createdAt":"2023-11-02T12:22:53.6396659Z",
            "lastModifiedBy":"*******",
            "lastModifiedByType":"Application",
            "lastModifiedAt":"2023-11-02T12:22:53.6396659Z"
         }
      }
   ]
}

github-actions bot added service/api-management size/XS labels Oct 7, 2023

sinbai changed the title ~~azurerm_api_management : fix issue 23396~~ azurerm_api_management : fix API Management global policy is deleted temporarily issue Oct 7, 2023

manicminer suggested changes Oct 10, 2023

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Oct 10, 2023

manicminer suggested changes Oct 12, 2023

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Oct 12, 2023

stephybun requested changes Oct 31, 2023

View reviewed changes

github-actions bot added the waiting-response label Oct 31, 2023

github-actions bot removed the waiting-response label Nov 1, 2023

katbyte added the waiting-response label Nov 24, 2023

github-actions bot removed the waiting-response label Dec 7, 2023

katbyte added the waiting-response label Dec 8, 2023

github-actions bot removed the waiting-response label Dec 8, 2023

sinbai added 3 commits June 18, 2024 16:00

fix issue 23396

b39d48f

update code

2789d66

update code

a81b2a4

sinbai force-pushed the apim/fix_issue_23396 branch from ed4d809 to a81b2a4 Compare June 18, 2024 08:05

rcskosir added the bug label Oct 14, 2024

katbyte requested review from katbyte and a team as code owners November 14, 2024 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_api_management` : fix API Management global policy is deleted temporarily issue #23474

`azurerm_api_management` : fix API Management global policy is deleted temporarily issue #23474

sinbai commented Oct 7, 2023 •

edited by rcskosir

Loading

manicminer left a comment

sinbai commented Oct 11, 2023

manicminer Oct 12, 2023

sinbai Oct 13, 2023

sinbai Jun 18, 2024

stephybun left a comment

sinbai commented Nov 1, 2023

stephybun commented Nov 1, 2023

katbyte commented Dec 7, 2023

sinbai commented Dec 7, 2023

katbyte commented Dec 8, 2023

sinbai commented Dec 8, 2023

azurerm_api_management : fix API Management global policy is deleted temporarily issue #23474

Are you sure you want to change the base?

azurerm_api_management : fix API Management global policy is deleted temporarily issue #23474

Conversation

sinbai commented Oct 7, 2023 • edited by rcskosir Loading

manicminer left a comment

Choose a reason for hiding this comment

sinbai commented Oct 11, 2023

manicminer Oct 12, 2023

Choose a reason for hiding this comment

sinbai Oct 13, 2023

Choose a reason for hiding this comment

sinbai Jun 18, 2024

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

sinbai commented Nov 1, 2023

stephybun commented Nov 1, 2023

katbyte commented Dec 7, 2023

sinbai commented Dec 7, 2023

katbyte commented Dec 8, 2023

sinbai commented Dec 8, 2023

`azurerm_api_management` : fix API Management global policy is deleted temporarily issue #23474

`azurerm_api_management` : fix API Management global policy is deleted temporarily issue #23474

sinbai commented Oct 7, 2023 •

edited by rcskosir

Loading