`azurerm_redis_cache` - Fix the issue about data persistence could not be switched #20286

sinbai · 2023-02-03T02:34:05Z

The rdb_backup_enabled or aof_backup_enabled is set to false in config when updating resource azurerm_redis_cache, they should be explicitly set value in terraform code so that data persistence could be switched.

Fix #20228 .

Test Results:
PASS: TestAccRedisCache_BackupEnabledDisabled (2173.38s)
PASS: TestAccRedisCache_AOFBackupEnabledDisabled (2631.76s)

tombuildsstuff · 2023-02-03T12:26:43Z

internal/services/redis/redis_cache_resource.go

-			return nil, fmt.Errorf("The rdb_storage_connection_string property must be set when rdb_backup_enabled is true")
+	// the "output.RdbBackupEnabled" should be set value when the rdb_backup_enabled is set to false in config since not setting false means it could not be disabled using patch to update the Redis Cache.
+	// d.GetOk cannot identify if user sets the property that is bool type and `rdb_backup_enabled` is set as `false`. So it has to identify it using `d.GetRawConfig()`
+	if v := d.GetRawConfig().AsValueMap()["redis_configuration"].AsValueSlice(); len(v) > 0 && !v[0].AsValueMap()["rdb_backup_enabled"].IsNull() {


rather than using GetRawConfig - we should update this to use d.GetOkExists - since this approach can be nil/crash at multiple points along this expression

@tombuildsstuff thanks for your time to review this PR. If we use if v, ok := d.GetOkExists("redis_configuration.0.rdb_backup_enabled"); ok{ instead of GetRawConfig, the vaule of ok is true when the rdb_backup_enabled is removed in tf config. But the GetRawConfig is null (meaning false) in this case.

Using GetOkExists means that if user specifies rdb_backup_enabled as true in creating a resource, and then removes rdb_backup_enabled in tf config to update the resource, the rdb_backup_enabled would be set to false. In fact, the behavior of API in this case, the rdb_backup_enabled should remain true. That's why I use GetRawConfig ranther than GetOkExists. BTW, if rdb_backup_enabled is not specified, API return nil.

So I assume that the GetRawConfig could not be replaced by GetOkExists. Please feel free let me know your feedback on this case.

@sinbai if you are going to do this you must be checking each conversion for a nil/crash. right now if anything changes/something unexpected happens this code will panic

tombuildsstuff · 2023-02-03T12:27:02Z

internal/services/redis/redis_cache_resource.go

-		output.AdditionalProperties["aof-backup-enabled"] = utils.String(strconv.FormatBool(v))
+	// the "output.AdditionalProperties["aof-backup-enabled"]" should be set value when the aof_backup_enabled is set to false in config since not setting false means it could not be disabled using patch to update the Redis Cache.
+	// d.GetOk cannot identify if user sets the property that is bool type and `aof_backup_enabled` is set as `false`. So it has to identify it using `d.GetRawConfig()`
+	if v := d.GetRawConfig().AsValueMap()["redis_configuration"].AsValueSlice(); len(v) > 0 && !v[0].AsValueMap()["aof_backup_enabled"].IsNull() {


(same here)

Same answer as above.

tombuildsstuff · 2023-02-03T12:27:28Z

internal/services/redis/redis_cache_resource_test.go

@@ -143,6 +143,7 @@ func TestAccRedisCache_BackupDisabled(t *testing.T) {
 			Check: acceptance.ComposeTestCheckFunc(
 				check.That(data.ResourceName).ExistsInAzure(r),
 			),
+			ExpectNonEmptyPlan: true,


doing this would signify there's a bug to fix?

Yes, there is a bug. I have added a comment to it.

tombuildsstuff

hi @sinbai

Thanks for this PR.

I've taken a look through and left some comments inline, unfortunately we need to take a different approach to what's used here, namely rather than leaning on GetRawConfig (which can return nil, so will crash in the current context), we can instead use GetOkExists to achieve the same thing in a safer manner.

Thanks!

tombuildsstuff · 2023-02-03T12:28:05Z

internal/services/redis/redis_cache_resource_test.go

+    aof_storage_connection_string_0 = "DefaultEndpointsProtocol=https;BlobEndpoint=${azurerm_storage_account.test.primary_blob_endpoint};AccountName=${azurerm_storage_account.test.name};AccountKey=${azurerm_storage_account.test.primary_access_key}"
+    aof_storage_connection_string_1 = "DefaultEndpointsProtocol=https;BlobEndpoint=${azurerm_storage_account.test.primary_blob_endpoint};AccountName=${azurerm_storage_account.test.name};AccountKey=${azurerm_storage_account.test.secondary_access_key}"


there's primary_ and secondary_ connection strings available on the storage account resource?

This shouldn't be the primary and secondary connection strings for the same storage account. This should be 2 separate storage accounts.

Ref: https://learn.microsoft.com/en-us/azure/azure-cache-for-redis/cache-how-to-premium-persistence#when-should-i-use-a-second-storage-account

@tombuildsstuff , @mclacore , thanks for your feedback. I have updated the test. Could you take another look?

@sinbai LGTM 👍

tombuildsstuff · 2023-02-03T12:28:11Z

internal/services/redis/redis_cache_resource_test.go

+    rdb_backup_enabled            = true
+    rdb_backup_frequency          = 60
+    rdb_backup_max_snapshot_count = 1
+    rdb_storage_connection_string = "DefaultEndpointsProtocol=https;BlobEndpoint=${azurerm_storage_account.test.primary_blob_endpoint};AccountName=${azurerm_storage_account.test.name};AccountKey=${azurerm_storage_account.test.primary_access_key}"


(same here)

katbyte · 2023-02-28T00:22:42Z

internal/services/redis/redis_cache_resource.go

-			return nil, fmt.Errorf("The rdb_storage_connection_string property must be set when rdb_backup_enabled is true")
+	// the "output.RdbBackupEnabled" should be set value when the rdb_backup_enabled is set to false in config since not setting false means it could not be disabled using patch to update the Redis Cache.
+	// d.GetOk cannot identify if user sets the property that is bool type and `rdb_backup_enabled` is set as `false`. So it has to identify it using `d.GetRawConfig()`
+	if v := d.GetRawConfig().AsValueMap()["redis_configuration"].AsValueSlice(); len(v) > 0 && !v[0].AsValueMap()["rdb_backup_enabled"].IsNull() {


@sinbai if you are going to do this you must be checking each conversion for a nil/crash. right now if anything changes/something unexpected happens this code will panic

sinbai · 2023-02-28T09:47:24Z

@katbyte thanks for your feedback. Code has been updated. Could you please take another look?

katbyte · 2023-03-03T01:30:34Z

@sinbai - could we fix the merge conflicts?

sinbai · 2023-03-03T05:25:17Z

@sinbai - could we fix the merge conflicts?

Sure, fixed.

katbyte · 2023-03-20T20:56:18Z

@sinbai - after discussing internally i was informed that GetRawConfig will not be directly available when we switch to framework. We think GetOkExists should be fine as there is a default, so we can just use the default?

sinbai · 2023-03-21T07:45:13Z

@sinbai - after discussing internally i was informed that GetRawConfig will not be directly available when we switch to framework. We think GetOkExists should be fine as there is a default, so we can just use the default?

@katbyte As I replied above , using GetOkExists means that if user specifies rdb_backup_enabled as true in creating a resource, and then removes rdb_backup_enabled in tf config to update the resource, the rdb_backup_enabled would be set to false. In fact, the behavior of API in this case, the rdb_backup_enabled should remain true. That's why I use GetRawConfig ranther than GetOkExists.

katbyte · 2023-04-02T18:19:49Z

@sinbai - the problem with us using get raw is that in the future when we move to framework, as i understand it, it won't be supported. so you will be introducing a future breaking change/behaviour we will not be able to mimic.

I'm not sure why you cannot just use GetOkExist? it sounds like the property should be defaulting to true to match the API behaviour? and that would be a breaking change that we can ship for 4.0?

sinbai · 2023-04-14T09:34:58Z

@sinbai - the problem with us using get raw is that in the future when we move to framework, as i understand it, it won't be supported. so you will be introducing a future breaking change/behaviour we will not be able to mimic.

I'm not sure why you cannot just use GetOkExist? it sounds like the property should be defaulting to true to match the API behaviour? and that would be a breaking change that we can ship for 4.0?

Hi @katbyte

The reason I previously thought that using GetOkExist would not work was because updating behavior of TF was inconsistent with the update behavior of the API. See below for details:

Using GetOkExist, if user specifies rdb_backup_enabled as true in creating a resource, and then removes rdb_backup_enabled in tf config to update the resource by TF, TF would set rdb_backup_enabled to false. In this case, the update API would not update the value of rdb_backup_enabled, keep it true.

But for now, I think that GetOkExist can fix issue #20228 as long as we state in TF doc that the default value of rdb_backup_enabled is false. With it, it is by design that TF updates rdb_backup_enabled to false in the above situation. So, I update the code by using GetOkExist , and all the tests passed in Teamcity, could you please take another look?

katbyte

Thanks @sinbai - i think that is fine as the default should already have been false.

LGTM 🌵

…t be switched (hashicorp#20286)

github-actions · 2023-04-28T13:30:09Z

This functionality has been released in v3.54.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 · 2024-04-28T02:10:33Z

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 added service/redis size/M labels Feb 3, 2023

sinbai mentioned this pull request Feb 3, 2023

Can't switch data persistence in azure_cache_redis #20228

Closed

1 task

tombuildsstuff reviewed Feb 3, 2023

View reviewed changes

github-actions bot added the waiting-response label Feb 3, 2023

tombuildsstuff reviewed Feb 3, 2023

View reviewed changes

tombuildsstuff suggested changes Feb 3, 2023

View reviewed changes

github-actions bot added size/L and removed waiting-response size/M labels Feb 6, 2023

katbyte requested changes Feb 28, 2023

View reviewed changes

katbyte added the waiting-response label Feb 28, 2023

github-actions bot removed the waiting-response label Feb 28, 2023

sinbai force-pushed the redis/fix_issue_20228 branch 2 times, most recently from 88e461f to f9f073f Compare March 3, 2023 03:28

github-actions bot added the dependencies label Mar 3, 2023

katbyte added the waiting-response label Mar 20, 2023

github-actions bot removed the waiting-response label Mar 21, 2023

katbyte added the waiting-response label Apr 2, 2023

sinbai added 2 commits April 11, 2023 18:03

fix issue 21363

07c9c35

Merge branch 'master' of github.com:sinbai/terraform-provider-azurerm

8cb43b8

github-actions bot added documentation and removed waiting-response labels Apr 14, 2023

github-actions bot added size/M and removed size/L labels Apr 14, 2023

sinbai added 3 commits April 14, 2023 14:56

fix issue 20228

ebee446

update code

511edc6

update code

73cdd78

sinbai force-pushed the redis/fix_issue_20228 branch from 27b63fe to 73cdd78 Compare April 14, 2023 07:02

update code

75808b3

github-actions bot removed the dependencies label Apr 14, 2023

sinbai added 2 commits April 14, 2023 15:37

update code

b0f88b9

update code

af36401

github-actions bot added size/L and removed size/M labels Apr 14, 2023

katbyte approved these changes Apr 28, 2023

View reviewed changes

katbyte merged commit f6dcbd9 into hashicorp:main Apr 28, 2023

github-actions bot added this to the v3.54.0 milestone Apr 28, 2023

xiaxyi pushed a commit to xiaxyi/terraform-provider-azurerm that referenced this pull request Apr 28, 2023

azurerm_redis_cache - Fix the issue about data persistence could no…

8d9638e

…t be switched (hashicorp#20286)

mbfrahry mentioned this pull request May 25, 2023

azurerm_redis_cache - check patch schedule existence before delete #21539

Closed

sinbai deleted the redis/fix_issue_20228 branch March 28, 2024 02:54

github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_redis_cache` - Fix the issue about data persistence could not be switched #20286

`azurerm_redis_cache` - Fix the issue about data persistence could not be switched #20286

sinbai commented Feb 3, 2023

tombuildsstuff Feb 3, 2023

sinbai Feb 6, 2023

katbyte Feb 28, 2023

tombuildsstuff Feb 3, 2023

sinbai Feb 6, 2023

tombuildsstuff Feb 3, 2023

sinbai Feb 6, 2023

tombuildsstuff left a comment

tombuildsstuff Feb 3, 2023

mclacore Feb 3, 2023

sinbai Feb 6, 2023

mclacore Feb 15, 2023 •

edited

Loading

tombuildsstuff Feb 3, 2023

sinbai Feb 6, 2023

katbyte Feb 28, 2023

sinbai commented Feb 28, 2023

katbyte commented Mar 3, 2023

sinbai commented Mar 3, 2023

katbyte commented Mar 20, 2023

sinbai commented Mar 21, 2023 •

edited

Loading

katbyte commented Apr 2, 2023

sinbai commented Apr 14, 2023 •

edited

Loading

katbyte left a comment

github-actions bot commented Apr 28, 2023

github-actions bot commented Apr 28, 2024

		aof_storage_connection_string_0 = "DefaultEndpointsProtocol=https;BlobEndpoint=${azurerm_storage_account.test.primary_blob_endpoint};AccountName=${azurerm_storage_account.test.name};AccountKey=${azurerm_storage_account.test.primary_access_key}"
		aof_storage_connection_string_1 = "DefaultEndpointsProtocol=https;BlobEndpoint=${azurerm_storage_account.test.primary_blob_endpoint};AccountName=${azurerm_storage_account.test.name};AccountKey=${azurerm_storage_account.test.secondary_access_key}"

azurerm_redis_cache - Fix the issue about data persistence could not be switched #20286

azurerm_redis_cache - Fix the issue about data persistence could not be switched #20286

Conversation

sinbai commented Feb 3, 2023

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

tombuildsstuff left a 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

mclacore Feb 15, 2023 • edited Loading

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

sinbai commented Feb 28, 2023

katbyte commented Mar 3, 2023

sinbai commented Mar 3, 2023

katbyte commented Mar 20, 2023

sinbai commented Mar 21, 2023 • edited Loading

katbyte commented Apr 2, 2023

sinbai commented Apr 14, 2023 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 28, 2023

github-actions bot commented Apr 28, 2024

`azurerm_redis_cache` - Fix the issue about data persistence could not be switched #20286

`azurerm_redis_cache` - Fix the issue about data persistence could not be switched #20286

mclacore Feb 15, 2023 •

edited

Loading

sinbai commented Mar 21, 2023 •

edited

Loading

sinbai commented Apr 14, 2023 •

edited

Loading