Fix bug for azurerm_storage_management_policy #5803

neil-yechenwei · 2020-02-18T13:20:16Z

This PR is for fixing the issue #4580

jackofallops

Hi @neil-yechenwei
Thanks for the PR. There are few comments indicating changes needed at this stage.

jackofallops · 2020-03-06T08:58:38Z

azurerm/internal/services/storage/resource_arm_storage_management_policy.go

@@ -375,14 +377,20 @@ func flattenStorageManagementPolicyRules(armRules *[]storage.ManagementPolicyRul
 					if armActionBaseBlob.TierToCool != nil && armActionBaseBlob.TierToCool.DaysAfterModificationGreaterThan != nil {
 						intTemp := int(*armActionBaseBlob.TierToCool.DaysAfterModificationGreaterThan)
 						baseBlob["tier_to_cool_after_days_since_modification_greater_than"] = intTemp
+					} else {
+						baseBlob["tier_to_cool_after_days_since_modification_greater_than"] = -1


Setting a "-1" value here will break updates to the resource as this value won't pass validation.

Additionally, flatten should take the same approach as has been done with expand to prevent updates re-introducing the issue being addressed. (an update after creation will set values from state, in this case 0, but should only be added to the state if user configured)

No. I already tested. It wouldn't break updates. Could you give me a example?
Below is the test result after updated:

Here there are two issues.
First issue, the value of property in nested block would be set as "0" for TypeInt when tf config doesn't specify this property. But we actually don't want this is set as 0. We just want this is set as nil. So I have to use this way "d.GetOkExists(fmt.Sprintf("rule.%d.actions.0.base_blob.0.tier_to_archive_after_days_since_modification_greater_than", idx));" to fix this issue.

Second issue, d.set() would set the value of property in nested block as "0" for TypeInt after service api returns nil. Actually we just want this is set as nil. So I have to use this way "baseBlob["tier_to_cool_after_days_since_modification_greater_than"] = -1" to fix this issue.

The above example doesn't exercise your new code where the values are "-1" in the state (i.e. unset by config). If you extend your testing to do something like TestAccAzureRMStorageManagementPolicy_withOneActionWithUpdate and update the single value set in your new test, I believe you should see what I'm referring to. i.e. The update will read -1 from the state for the values not explicitly configured which will fail validation and you'll get something like:

ManagementPolicy rule rule1 is invalid. Invalid value for parameter : daysAfterModificationGreaterThan

To avoid this, we should not d.Set any value if it's not supplied in the config or read back from the API, meaning the approach for traversing the rules with an index value in flatten as you have done for expand is necessary. Does that make sense?

Yes. I repro the issue what you mentioned. But seems that I cannot remove d.Set for the properties since the properties would be set as "null" when the properties aren't specified in tfconfig after removed d.set for these properties. Then it always show the difference when I explicitly update the properties for several times. So I've updated code with other solution. Please help to have a look. If the solution is not better, could you share you sample code for the solution? Thanks.

tfstate result when the properties aren't specified in tfconfig after removed d.set for these properties:

"rule": [ { "actions": [ { "base_blob": [ { "delete_after_days_since_modification_greater_than": null, "tier_to_archive_after_days_since_modification_greater_than": null, "tier_to_cool_after_days_since_modification_greater_than": null } ], "snapshot": [ { "delete_after_days_since_creation_greater_than": 30 } ] } ], "enabled": true, "filters": [ { "blob_types": [ "blockBlob" ], "prefix_match": [ "container1/prefix1" ] } ], "name": "rule1" } ],

tfconfig for updating this resource several times:

provider "azurerm" { features {} } resource "azurerm_resource_group" "test" { name = "acctestRG-storage-neil" location = "westus" } resource "azurerm_storage_account" "test" { name = "unlikelyneiltest" resource_group_name = "${azurerm_resource_group.test.name}" location = "${azurerm_resource_group.test.location}" account_tier = "Standard" account_replication_type = "LRS" account_kind = "BlobStorage" } resource "azurerm_storage_management_policy" "test" { storage_account_id = "${azurerm_storage_account.test.id}" rule { name = "rule1" enabled = true filters { prefix_match = ["container1/prefix1"] blob_types = ["blockBlob"] } actions { base_blob { delete_after_days_since_modification_greater_than = 102 } snapshot { delete_after_days_since_creation_greater_than = 30 } } } }

Always show the difference while updating this resource several times:

jackofallops · 2020-03-06T08:58:51Z

azurerm/internal/services/storage/resource_arm_storage_management_policy.go

 					}
 					if armActionBaseBlob.TierToArchive != nil && armActionBaseBlob.TierToArchive.DaysAfterModificationGreaterThan != nil {
 						intTemp := int(*armActionBaseBlob.TierToArchive.DaysAfterModificationGreaterThan)
 						baseBlob["tier_to_archive_after_days_since_modification_greater_than"] = intTemp
+					} else {
+						baseBlob["tier_to_archive_after_days_since_modification_greater_than"] = -1


As above - setting a "-1" value here will break updates to the resource as this value won't pass validation.

Same as above reason

jackofallops · 2020-03-06T08:59:00Z

azurerm/internal/services/storage/resource_arm_storage_management_policy.go

 					}
 					if armActionBaseBlob.Delete != nil && armActionBaseBlob.Delete.DaysAfterModificationGreaterThan != nil {
 						intTemp := int(*armActionBaseBlob.Delete.DaysAfterModificationGreaterThan)
 						baseBlob["delete_after_days_since_modification_greater_than"] = intTemp
+					} else {
+						baseBlob["delete_after_days_since_modification_greater_than"] = -1


As above - setting a "-1" value here will break updates to the resource as this value won't pass validation.

Same as above reason

jackofallops · 2020-03-06T09:47:43Z

azurerm/internal/services/storage/tests/resource_arm_storage_management_policy_test.go

+					resource.TestCheckResourceAttr(data.ResourceName, "rule.0.actions.0.base_blob.0.tier_to_cool_after_days_since_modification_greater_than", "-1"),
+					resource.TestCheckResourceAttr(data.ResourceName, "rule.0.actions.0.base_blob.0.tier_to_archive_after_days_since_modification_greater_than", "-1"),


As mentioned above, these should not be set if not defined by the configuration.

Same as above reason.

neil-yechenwei · 2020-03-11T03:49:40Z

@jackofallops , I'v updated code. Please have a look. THanks.

jackofallops · 2020-03-17T08:25:47Z

Hi @neil-yechenwei
I hope you don't mind, but we've pushed a couple of updated files to this PR. I didn't seem to be explaining what was needed very well, which led you to revert some of the necessary steps, sorry about that. The changes we've pushed address the correct behaviours required, and still allow the use of 0 values, which are valid for the service. Additionally, when actions are removed from the resource configuration they'll be removed from the resource also. (see the singleActionUpdated test for an example).

Many thanks for your contribution 👍

neil-yechenwei · 2020-03-18T08:47:01Z

@jackofallops , Thanks for your commit. But I still found a bug from your commit. when the property tier_to_archive_after_days_since_modification_greater_than isn't set in tf config, this property in tfstate file is set as 0. Then I update this tf config and add property tier_to_archive_after_days_since_modification_greater_than = 0. You will see there is no difference after run command "terraform plan". Actually terraform should accept 0 since 0 is meaningful in service side. So it's a bug. Could you help to fix it or can you rollback to my last commit since my commit already fixed this issue?

tfconfig without tier_to_archive_after_days_since_modification_greater_than:

provider "azurerm" {
  features {}
}

resource "azurerm_resource_group" "test" {
  name     = "acctestRG-storage-neil"
  location = "westus"
}

resource "azurerm_storage_account" "test" {
  name                = "unlikely23exst2acctneil"
  resource_group_name = azurerm_resource_group.test.name

  location                 = azurerm_resource_group.test.location
  account_tier             = "Standard"
  account_replication_type = "LRS"
  account_kind             = "BlobStorage"
}

resource "azurerm_storage_management_policy" "test" {
  storage_account_id = azurerm_storage_account.test.id

  rule {
    name    = "singleActionRule"
    enabled = true
    filters {
      prefix_match = ["container1/prefix1"]
      blob_types   = ["blockBlob"]
    }
    actions {
      base_blob {
        delete_after_days_since_modification_greater_than = 30
      }
      snapshot {
        delete_after_days_since_creation_greater_than = 30
      }
    }
  }
}

tfstate with tier_to_archive_after_days_since_modification_greater_than:

{
  "version": 4,
  "terraform_version": "0.12.21",
  "serial": 4,
  "lineage": "xxxxx",
  "outputs": {},
  "resources": [
    {
      "mode": "managed",
      "type": "azurerm_resource_group",
      "name": "test",
      "provider": "provider.azurerm",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "id": "/subscriptions/xxx-xxx-xxx-xxx/resourceGroups/acctestRG-storage-neil",
            "location": "westus",
            "name": "acctestRG-storage-neil",
            "tags": null,
            "timeouts": null
          },
          "private": "xxxxx"
        }
      ]
    },
    {
      "mode": "managed",
      "type": "azurerm_storage_account",
      "name": "test",
      "provider": "provider.azurerm",
      "instances": [
        {
          "schema_version": 2,
          "attributes": {
            "access_tier": "Hot",
            "account_kind": "BlobStorage",
            "account_replication_type": "LRS",
            "account_tier": "Standard",
            "blob_properties": [],
            "custom_domain": [],
            "enable_https_traffic_only": true,
            "id": "/subscriptions/xxx-xxx-xxx-xxx/resourceGroups/acctestRG-storage-neil/providers/Microsoft.Storage/storageAccounts/unlikely23exst2acctneil",
            "identity": [],
            "is_hns_enabled": false,
            "location": "westus",
            "name": "unlikely23exst2acctneil",
            "network_rules": [
              {
                "bypass": [
                  "AzureServices"
                ],
                "default_action": "Allow",
                "ip_rules": [],
                "virtual_network_subnet_ids": []
              }
            ],
            "primary_access_key": "xxxxx",
            "primary_blob_connection_string": "xxxxxx",
            "primary_blob_endpoint": "https://xxxxx/",
            "primary_blob_host": "unxxxxxt",
            "primary_connection_string": "xxxxxxx",
            "primary_dfs_endpoint": "hxxxxxt/",
            "primary_dfs_host": "unlikely23xxxxx.net",
            "primary_file_endpoint": "",
            "primary_file_host": "",
            "primary_location": "westus",
            "primary_queue_endpoint": "",
            "primary_queue_host": "",
            "primary_table_endpoint": "https://unxxxx.net/",
            "primary_table_host": "unlikely2xxxxows.net",
            "primary_web_endpoint": "",
            "primary_web_host": "",
            "queue_properties": null,
            "resource_group_name": "acctestRG-storage-neil",
            "secondary_access_key": "xxxxxx",
            "secondary_blob_connection_string": "",
            "secondary_blob_endpoint": null,
            "secondary_blob_host": null,
            "secondary_connection_string": "xxxxxxt",
            "secondary_dfs_endpoint": null,
            "secondary_dfs_host": null,
            "secondary_file_endpoint": null,
            "secondary_file_host": null,
            "secondary_location": "",
            "secondary_queue_endpoint": null,
            "secondary_queue_host": null,
            "secondary_table_endpoint": null,
            "secondary_table_host": null,
            "secondary_web_endpoint": null,
            "secondary_web_host": null,
            "static_website": [],
            "tags": null,
            "timeouts": null
          },
          "private": "xxxxxx",
          "dependencies": [
            "azurerm_resource_group.test"
          ]
        }
      ]
    },
    {
      "mode": "managed",
      "type": "azurerm_storage_management_policy",
      "name": "test",
      "provider": "provider.azurerm",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "id": "/subscriptions/xxx-xxx-xxx-xxx/resourceGroups/acctestRG-storage-neil/providers/Microsoft.Storage/storageAccounts/unlikely23exst2acctneil/managementPolicies/default",
            "rule": [
              {
                "actions": [
                  {
                    "base_blob": [
                      {
                        "delete_after_days_since_modification_greater_than": 30,
                        "tier_to_archive_after_days_since_modification_greater_than": 0,
                        "tier_to_cool_after_days_since_modification_greater_than": 0
                      }
                    ],
                    "snapshot": [
                      {
                        "delete_after_days_since_creation_greater_than": 30
                      }
                    ]
                  }
                ],
                "enabled": true,
                "filters": [
                  {
                    "blob_types": [
                      "blockBlob"
                    ],
                    "prefix_match": [
                      "container1/prefix1"
                    ]
                  }
                ],
                "name": "singleActionRule"
              }
            ],
            "storage_account_id": "/subscriptions/xxx-xxx-xxx-xxx/resourceGroups/acctestRG-storage-neil/providers/Microsoft.Storage/storageAccounts/unlikely23exst2acctneil",
            "timeouts": null
          },
          "private": "xxxxxx",
          "dependencies": [
            "azurerm_resource_group.test",
            "azurerm_storage_account.test"
          ]
        }
      ]
    }
  ]
}

update tfconfig to add tier_to_archive_after_days_since_modification_greater_than = 0:

provider "azurerm" {
  features {}
}

resource "azurerm_resource_group" "test" {
  name     = "acctestRG-storage-neil"
  location = "westus"
}

resource "azurerm_storage_account" "test" {
  name                = "unlikely23exst2acctneil"
  resource_group_name = azurerm_resource_group.test.name

  location                 = azurerm_resource_group.test.location
  account_tier             = "Standard"
  account_replication_type = "LRS"
  account_kind             = "BlobStorage"
}

resource "azurerm_storage_management_policy" "test" {
  storage_account_id = azurerm_storage_account.test.id

  rule {
    name    = "singleActionRule"
    enabled = true
    filters {
      prefix_match = ["container1/prefix1"]
      blob_types   = ["blockBlob"]
    }
    actions {
      base_blob {
	    tier_to_archive_after_days_since_modification_greater_than = 0
            delete_after_days_since_modification_greater_than = 30
      }
      snapshot {
        delete_after_days_since_creation_greater_than = 30
      }
    }
  }
}

Result of "terraform plan":

jackofallops · 2020-03-18T14:23:29Z

Hi @neil-yechenwei
You are correct - there's a limitation of GetOk I was previously unaware of which does preclude this solution allowing the 0 values that should be allowed. Since the original issue this PR was logged against is addressed by the changes, I will open a new issue to look at addressing the valid zero-value bug.

ghost · 2020-03-19T00:00:46Z

This has been released in version 2.2.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.2.0"
}
# ... other configuration ...

ghost · 2020-04-16T14:50:13Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost added size/XS size/M and removed size/XS labels Feb 18, 2020

neil-yechenwei requested a review from mbfrahry February 20, 2020 02:11

tombuildsstuff removed the request for review from mbfrahry February 20, 2020 09:15

jackofallops requested changes Mar 6, 2020

View reviewed changes

jackofallops added service/storage waiting-response labels Mar 9, 2020

ghost added size/L and removed size/M labels Mar 11, 2020

ghost removed the waiting-response label Mar 11, 2020

neil-yechenwei requested a review from jackofallops March 11, 2020 03:49

fixed rule actions behaviour and added sinngle action tests

c27728b

katbyte force-pushed the fix_storage_mgmt_policy branch from 18238ba to c27728b Compare March 17, 2020 08:09

jackofallops added this to the v2.2.0 milestone Mar 17, 2020

jackofallops merged commit 9350439 into hashicorp:master Mar 17, 2020

jackofallops added a commit that referenced this pull request Mar 17, 2020

Updated for #5803

34de670

hashicorp locked and limited conversation to collaborators Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug for azurerm_storage_management_policy #5803

Fix bug for azurerm_storage_management_policy #5803

neil-yechenwei commented Feb 18, 2020 •

edited by tombuildsstuff

jackofallops left a comment

jackofallops Mar 6, 2020

neil-yechenwei Mar 10, 2020 •

edited

jackofallops Mar 10, 2020

neil-yechenwei Mar 11, 2020 •

edited

jackofallops Mar 6, 2020

neil-yechenwei Mar 10, 2020

jackofallops Mar 6, 2020

neil-yechenwei Mar 10, 2020

jackofallops Mar 6, 2020

neil-yechenwei Mar 10, 2020

neil-yechenwei commented Mar 11, 2020

jackofallops commented Mar 17, 2020

neil-yechenwei commented Mar 18, 2020 •

edited

jackofallops commented Mar 18, 2020

ghost commented Mar 19, 2020

ghost commented Apr 16, 2020

		resource.TestCheckResourceAttr(data.ResourceName, "rule.0.actions.0.base_blob.0.tier_to_cool_after_days_since_modification_greater_than", "-1"),
		resource.TestCheckResourceAttr(data.ResourceName, "rule.0.actions.0.base_blob.0.tier_to_archive_after_days_since_modification_greater_than", "-1"),

Fix bug for azurerm_storage_management_policy #5803

Fix bug for azurerm_storage_management_policy #5803

Conversation

neil-yechenwei commented Feb 18, 2020 • edited by tombuildsstuff

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Mar 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Mar 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Mar 11, 2020

jackofallops commented Mar 17, 2020

neil-yechenwei commented Mar 18, 2020 • edited

jackofallops commented Mar 18, 2020

ghost commented Mar 19, 2020

ghost commented Apr 16, 2020

neil-yechenwei commented Feb 18, 2020 •

edited by tombuildsstuff

neil-yechenwei Mar 10, 2020 •

edited

neil-yechenwei Mar 11, 2020 •

edited

neil-yechenwei commented Mar 18, 2020 •

edited