From 88e461fe2a2b94f4ee5f986f35a709e6ccfe9c8a Mon Sep 17 00:00:00 2001 From: elena Date: Thu, 2 Feb 2023 17:28:57 +0800 Subject: [PATCH] fix issue 20228 --- .../services/redis/redis_cache_resource.go | 54 ++++++-- .../redis/redis_cache_resource_test.go | 126 ++++++++++++++++-- 2 files changed, 159 insertions(+), 21 deletions(-) diff --git a/internal/services/redis/redis_cache_resource.go b/internal/services/redis/redis_cache_resource.go index 7791a896396ed..63e50bb1eb58d 100644 --- a/internal/services/redis/redis_cache_resource.go +++ b/internal/services/redis/redis_cache_resource.go @@ -8,6 +8,9 @@ import ( "strings" "time" + "github.com/hashicorp/terraform-provider-azurerm/internal/services/redis/migration" + "github.com/hashicorp/terraform-provider-azurerm/internal/services/redis/validate" + "github.com/hashicorp/go-azure-helpers/lang/pointer" "github.com/hashicorp/go-azure-helpers/lang/response" "github.com/hashicorp/go-azure-helpers/resourcemanager/commonschema" @@ -17,6 +20,8 @@ import ( "github.com/hashicorp/go-azure-helpers/resourcemanager/zones" "github.com/hashicorp/go-azure-sdk/resource-manager/redis/2021-06-01/patchschedules" "github.com/hashicorp/go-azure-sdk/resource-manager/redis/2021-06-01/redis" + "github.com/hashicorp/go-cty/cty" + "github.com/hashicorp/go-cty/cty/gocty" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-provider-azurerm/helpers/tf" azValidate "github.com/hashicorp/terraform-provider-azurerm/helpers/validate" @@ -25,8 +30,6 @@ import ( "github.com/hashicorp/terraform-provider-azurerm/internal/services/network" networkParse "github.com/hashicorp/terraform-provider-azurerm/internal/services/network/parse" networkValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/network/validate" - "github.com/hashicorp/terraform-provider-azurerm/internal/services/redis/migration" - "github.com/hashicorp/terraform-provider-azurerm/internal/services/redis/validate" "github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk" "github.com/hashicorp/terraform-provider-azurerm/internal/tf/suppress" "github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" @@ -818,11 +821,28 @@ func expandRedisConfiguration(d *pluginsdk.ResourceData) (*redis.RedisCommonProp } // RDB Backup - if v := raw["rdb_backup_enabled"].(bool); v { - if connStr := raw["rdb_storage_connection_string"].(string); connStr == "" { - 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()` + rc := make(map[string]cty.Value, 0) + if err := gocty.FromCtyValue(d.GetRawConfig(), &rc); err != nil { + // RDB Backup + // 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 c := d.GetRawConfig().AsValueMap(); len(c) > 0 { + rc := cty.Value{} + if err := gocty.FromCtyValue(c["redis_configuration"], &rc); err != nil { + if v := c["redis_configuration"].AsValueSlice(); v != nil && len(v) > 0 && !v[0].AsValueMap()["rdb_backup_enabled"].IsNull() { + if v[0].AsValueMap()["rdb_backup_enabled"].True() { + if connStr := raw["rdb_storage_connection_string"].(string); connStr == "" { + return nil, fmt.Errorf("The rdb_storage_connection_string property must be set when rdb_backup_enabled is true") + } + output.RdbBackupEnabled = utils.String("true") + } else { + output.RdbBackupEnabled = utils.String("false") + } + } + } } - output.RdbBackupEnabled = utils.String(strconv.FormatBool(v)) } if v := raw["rdb_backup_frequency"].(int); v > 0 { @@ -842,8 +862,25 @@ func expandRedisConfiguration(d *pluginsdk.ResourceData) (*redis.RedisCommonProp } // AOF Backup - if v := raw["aof_backup_enabled"].(bool); v { - output.AofBackupEnabled = 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()` + rc = make(map[string]cty.Value, 0) + if err := gocty.FromCtyValue(d.GetRawConfig(), &rc); err != nil { + // AOF Backup + // 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 c := d.GetRawConfig().AsValueMap(); len(c) > 0 { + rc := cty.Value{} + if err := gocty.FromCtyValue(c["redis_configuration"], &rc); err != nil { + if v := d.GetRawConfig().AsValueMap()["redis_configuration"].AsValueSlice(); v != nil && len(v) > 0 && !v[0].AsValueMap()["aof_backup_enabled"].IsNull() { + if v[0].AsValueMap()["aof_backup_enabled"].True() { + output.AofBackupEnabled = utils.String("true") + } else { + output.AofBackupEnabled = utils.String("false") + } + } + } + } } if v := raw["aof_storage_connection_string_0"].(string); v != "" { @@ -853,7 +890,6 @@ func expandRedisConfiguration(d *pluginsdk.ResourceData) (*redis.RedisCommonProp if v := raw["aof_storage_connection_string_1"].(string); v != "" { output.AofStorageConnectionString1 = utils.String(v) } - authEnabled := raw["enable_authentication"].(bool) // Redis authentication can only be disabled if it is launched inside a VNET. if _, isPrivate := d.GetOk("subnet_id"); !isPrivate { diff --git a/internal/services/redis/redis_cache_resource_test.go b/internal/services/redis/redis_cache_resource_test.go index 550e8fb7068bb..6106ce3b1eb5a 100644 --- a/internal/services/redis/redis_cache_resource_test.go +++ b/internal/services/redis/redis_cache_resource_test.go @@ -143,6 +143,11 @@ func TestAccRedisCache_BackupDisabled(t *testing.T) { Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), ), + // `redis_configuration.0.aof_storage_connection_string_0` and `redis_configuration.0.aof_storage_connection_string_1` are returned as: + // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf" + // TODO: remove this once the Bug's been fixed: + // https://github.com/Azure/azure-rest-api-specs/issues/3037 + ExpectNonEmptyPlan: true, }, }) } @@ -178,7 +183,7 @@ func TestAccRedisCache_BackupEnabledDisabled(t *testing.T) { check.That(data.ResourceName).ExistsInAzure(r), ), // `redis_configuration.0.rdb_storage_connection_string` is returned as: - // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf" + // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf..." // TODO: remove this once the Bug's been fixed: // https://github.com/Azure/azure-rest-api-specs/issues/3037 ExpectNonEmptyPlan: true, @@ -188,8 +193,8 @@ func TestAccRedisCache_BackupEnabledDisabled(t *testing.T) { Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), ), - // `redis_configuration.0.rdb_storage_connection_string` is returned as: - // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf" + // `redis_configuration.0.aof_storage_connection_string_0` and `redis_configuration.0.aof_storage_connection_string_1` are returned as: + // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf..." // TODO: remove this once the Bug's been fixed: // https://github.com/Azure/azure-rest-api-specs/issues/3037 ExpectNonEmptyPlan: true, @@ -224,6 +229,9 @@ func TestAccRedisCache_AOFBackupEnabledDisabled(t *testing.T) { Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), ), + // `redis_configuration.0.aof_storage_connection_string_0` and `aof_storage_connection_string_1` are returned as: + // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf..." + // TODO: remove this once the Bug's been fixed: ExpectNonEmptyPlan: true, }, { @@ -231,6 +239,9 @@ func TestAccRedisCache_AOFBackupEnabledDisabled(t *testing.T) { Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), ), + // `redis_configuration.0.rdb_storage_connection_string` is returned as: + // "...;AccountKey=[key hidden]" rather than "...;AccountKey=fsjfvjnfnf..." + // TODO: remove this once the Bug's been fixed: ExpectNonEmptyPlan: true, }, }) @@ -710,6 +721,42 @@ resource "azurerm_resource_group" "test" { location = "%s" } +resource "azurerm_storage_account" "test" { + name = "acctestsa%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + +resource "azurerm_storage_account" "test2" { + name = "acctestsa2%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + +resource "azurerm_storage_account" "test3" { + name = "acctestsa3%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + resource "azurerm_redis_cache" "test" { name = "acctestRedis-%d" location = azurerm_resource_group.test.location @@ -720,10 +767,13 @@ resource "azurerm_redis_cache" "test" { enable_non_ssl_port = false redis_configuration { - rdb_backup_enabled = false + rdb_backup_enabled = false + aof_backup_enabled = true + aof_storage_connection_string_0 = azurerm_storage_account.test2.primary_connection_string + aof_storage_connection_string_1 = azurerm_storage_account.test3.primary_connection_string } } -`, data.RandomInteger, data.Locations.Primary, data.RandomInteger) +`, data.RandomInteger, data.Locations.Primary, data.RandomString, data.RandomString, data.RandomString, data.RandomInteger) } func (RedisCacheResource) backupEnabled(data acceptance.TestData) string { @@ -762,7 +812,7 @@ resource "azurerm_redis_cache" "test" { 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}" + rdb_storage_connection_string = azurerm_storage_account.test.primary_connection_string } } `, data.RandomInteger, data.Locations.Primary, data.RandomString, data.RandomInteger) @@ -779,6 +829,42 @@ resource "azurerm_resource_group" "test" { location = "%s" } +resource "azurerm_storage_account" "test" { + name = "acctestsa%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + +resource "azurerm_storage_account" "test2" { + name = "acctestsa2%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + +resource "azurerm_storage_account" "test3" { + name = "acctestsa3%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + resource "azurerm_redis_cache" "test" { name = "acctestRedis-%d" location = azurerm_resource_group.test.location @@ -789,10 +875,14 @@ resource "azurerm_redis_cache" "test" { enable_non_ssl_port = false redis_configuration { - aof_backup_enabled = false + aof_backup_enabled = false + rdb_backup_enabled = true + rdb_backup_frequency = 60 + rdb_backup_max_snapshot_count = 1 + rdb_storage_connection_string = azurerm_storage_account.test3.primary_connection_string } } -`, data.RandomInteger, data.Locations.Primary, data.RandomInteger) +`, data.RandomInteger, data.Locations.Primary, data.RandomString, data.RandomString, data.RandomString, data.RandomInteger) } func (RedisCacheResource) aofBackupEnabled(data acceptance.TestData) string { @@ -807,7 +897,19 @@ resource "azurerm_resource_group" "test" { } resource "azurerm_storage_account" "test" { - name = "unlikely23exst2acct%s" + name = "acctestsa%s" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + account_tier = "Standard" + account_replication_type = "GRS" + + tags = { + environment = "staging" + } +} + +resource "azurerm_storage_account" "test2" { + name = "acctestsa2%s" resource_group_name = azurerm_resource_group.test.name location = azurerm_resource_group.test.location account_tier = "Standard" @@ -829,11 +931,11 @@ resource "azurerm_redis_cache" "test" { redis_configuration { aof_backup_enabled = true - 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}" + aof_storage_connection_string_0 = azurerm_storage_account.test.primary_connection_string + aof_storage_connection_string_1 = azurerm_storage_account.test2.primary_connection_string } } -`, data.RandomInteger, data.Locations.Primary, data.RandomString, data.RandomInteger) +`, data.RandomInteger, data.Locations.Primary, data.RandomString, data.RandomString, data.RandomInteger) } func (RedisCacheResource) patchSchedule(data acceptance.TestData) string {