Azure / terraform-provider-azapi

Terraform provider for Azure Resource Manager Rest API
https://registry.terraform.io/providers/Azure/azapi/latest
Mozilla Public License 2.0
168 stars 44 forks source link

`ignore_body_changes` does not work if referenced element is null in body config #385

Open felixstorm opened 6 months ago

felixstorm commented 6 months ago

Example:

resource "azapi_update_resource" "log_analytics_workspace_audit_table" {
  type      = "Microsoft.OperationalInsights/workspaces/tables@2022-10-01"
  name      = "AzureDiagnostics"
  parent_id = azurerm_log_analytics_workspace.main_log.id

  body = jsonencode(
    {
      properties = {
        schema               = null
        retentionInDays      = 730
        totalRetentionInDays = 1826
      }
    }
  )

  ignore_body_changes = ["properties.schema"]
}

In this specific example, the Graph does not allow to update the property schema but due to the way updating resources with PUT works, setting schema to null results in it being ignored by the Graph and hence the update will work.
To avoid permanent change notifications, the contents of properties.schema being returned from the Graph (a map with the full schema of the table) should be ignored for comparison afterwards, but this is not the case. This is probably due to null (nil) not being deemed an appropriate replacement for a map here. Instead OverrideWithPaths will fall trough to here und return the unchanged value.

I crafted a very simple patch (without any safeguards, tests or similar) which seems to make it work:

diff --git a/utils/json.go b/utils/json.go
index 9f09580c..ff4795a1 100644
--- a/utils/json.go
+++ b/utils/json.go
@@ -138,6 +138,9 @@ func OverrideWithPaths(old interface{}, new interface{}, path string, pathSet ma
        if len(pathSet) == 0 || old == nil {
                return old, nil
        }
+       if new == nil {
+               return new, nil
+       }
        switch oldValue := old.(type) {
        case map[string]interface{}:
                if newMap, ok := new.(map[string]interface{}); ok {
ms-henglu commented 6 months ago

Hi @felixstorm ,

Thank you for taking time to report this issue and provide a bug fix!

I'll investigate and fix it, thanks!

ms-henglu commented 6 months ago

Hi @felixstorm ,

Sorry for late update.

In this case, I think it's easier to make it work if you remove the "schema = null" from the config, the azapi_update_resource will combine the config with the remote state, then send it in the PUT request.

felixstorm commented 6 months ago

Hi @ms-henglu ,

the reason to set schema to null in the first place is that MS Graph does not allow to update schema in any way in this case, i.e. if schema contains even the contents that have previously been read from MS Graph the PUT request will fail. That is the reason why setting schema to null (and therefore avoiding sending any merged remote state) is required here.

ms-henglu commented 6 months ago

Hi @felixstorm ,

Thanks for the explanation!

I think the "ignore_body_changes" feature doesn't work in this case. Because this feature is used when the config and remote state are different, and once set, it will override the config with remote state, and send it in the PUT. But in this case, it needs to send the config in the PUT.

I have another workaround that might help, which uses azapi_resource_action to make a PUT request.

data "azapi_resource_id" "table" {
  type      = "Microsoft.OperationalInsights/workspaces/tables@2022-10-01"
  name      = "AzureDiagnostics"
  parent_id = azurerm_log_analytics_workspace.main_log.id
}

resource "azapi_resource_action" "log_analytics_workspace_audit_table" {
  type        = "Microsoft.OperationalInsights/workspaces/tables@2022-10-01"
  resource_id = data.azapi_resource_id.table.id
  method      = "PUT"
  // the PUT request body only contains the content in the config, and there'll be no plan-diff for the action resource.
  body = jsonencode({
    properties = {
      schema               = null
      retentionInDays      = 730
      totalRetentionInDays = 1826
    }
  })
}
felixstorm commented 6 months ago

Hi @ms-henglu,

thanks for taking the time to come up with a potential workaround, but honestly I do not think that azapi_resource_action will work as required here since AFAIK it does not compare any remote config (i.e. actual MS Graph state) but is just a one-time action (either on create or destroy), isn't it?
In this case here we need to compare the actual remote MS Graph state with the local config, e.g. retentionInDays should be compared to 730 and adjusted if it differs for any reason (see full example in the issue description at the top).
I am happy to use any other means to achieve this, I just don't see how azapi_resource_action would help here.

Also, it's not clear to me why setting a property to null within body would invalidate it's use within ignore_body_changes? Is there any specific reasoning behind it?
Or is there another way that a fix/patch should work (I just thought that null might be a valid replacement for any value type)? I will be happy to file a regular PR using a different strategy if you give me a hint on what approach you would like me to take to make it work.

As you can see from this other issue in the regular Terraform AzureRM provider I am not the only one with the original problem: https://github.com/hashicorp/terraform-provider-azurerm/issues/15858

ms-henglu commented 6 months ago

Hi @felixstorm ,

Thank you for the details!

The azapi_resource_action doesn't not compare config with remote state, so it won't be automatically triggered if the resource is modified outside terraform. But this resource could be triggered multiple times, for example, if the retentionInDays in config changes from 730 to 731, it will send another PUT request to update the resource.

So I think it could solve the issue, though it's not ideal.

Also, it's not clear to me why setting a property to null within body would invalidate it's use within ignore_body_changes? Is there any specific reasoning behind it?

Because the ignore_body_changes is used to simulate how lifecycle.ignore_changes works. When using this feature, the property schema in Put request body is set to the value in remote, to avoid modifying the resource. However this API doesn't allow it, though the values are exactly the same(I believe it's an API bug, in other APIs setting a field to null may remove the values).