From f8caf6472640d5088ab255f653774448cd5fc29c Mon Sep 17 00:00:00 2001 From: id Date: Mon, 13 May 2024 15:00:19 +0000 Subject: [PATCH 1/4] azurerm_postgresql_flexible_server: fix storage_tier calculation --- .../postgresql_flexible_server_resource.go | 9 +++++- .../validate/flexible_server_storage_tier.go | 15 ++++++++++ .../flexible_server_storage_tier_test.go | 30 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 internal/services/postgres/validate/flexible_server_storage_tier_test.go diff --git a/internal/services/postgres/postgresql_flexible_server_resource.go b/internal/services/postgres/postgresql_flexible_server_resource.go index 139892eaf68f..fd8b5ac70c6f 100644 --- a/internal/services/postgres/postgresql_flexible_server_resource.go +++ b/internal/services/postgres/postgresql_flexible_server_resource.go @@ -382,7 +382,6 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { } newMb = newStorageMbRaw.(int) - newTier = newTierRaw.(string) // storage_mb can only be scaled up... if newMb < oldStorageMbRaw.(int) { @@ -400,8 +399,16 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { // storage_mb size... storageTiers := storageTierMappings[newMb] + newTier = "" + + // set new tier only when tier is changed or when value is faster than default + if oldTierRaw.(string) != newTierRaw.(string) || validate.CompareAzureManagedDiskPerformance(servers.AzureManagedDiskPerformanceTiers(newTierRaw.(string)), storageTiers.DefaultTier) { + newTier = newTierRaw.(string) + } + if newTier == "" { newTier = string(storageTiers.DefaultTier) + diff.SetNew("storage_tier", newTier) } // verify that the storage_tier is valid diff --git a/internal/services/postgres/validate/flexible_server_storage_tier.go b/internal/services/postgres/validate/flexible_server_storage_tier.go index 689d47de8e1d..56105c44eddb 100644 --- a/internal/services/postgres/validate/flexible_server_storage_tier.go +++ b/internal/services/postgres/validate/flexible_server_storage_tier.go @@ -4,6 +4,9 @@ package validate import ( + "strconv" + "strings" + "github.com/hashicorp/go-azure-sdk/resource-manager/postgresql/2023-06-01-preview/servers" ) @@ -88,3 +91,15 @@ func InitializeFlexibleServerStorageTierDefaults() map[int]StorageTiers { return storageTiersMappings } + +// CompareAzureManagedDiskPerformancef returns an bool comparing two AzureManagedDiskPerformanceTiers. +// The result will be false if disk == compareDisk, false if disk < compareDisk, and true if disk > compareDisk. +func CompareAzureManagedDiskPerformance(disk, compareDisk servers.AzureManagedDiskPerformanceTiers) bool { + diskP := strings.TrimLeft(strings.ToUpper(string(disk)), "P") + compareDiskP := strings.TrimLeft(strings.ToUpper(string(compareDisk)), "P") + + dPn, _ := strconv.Atoi(diskP) + cPn, _ := strconv.Atoi(compareDiskP) + + return dPn > cPn +} diff --git a/internal/services/postgres/validate/flexible_server_storage_tier_test.go b/internal/services/postgres/validate/flexible_server_storage_tier_test.go new file mode 100644 index 000000000000..e364e5060f0f --- /dev/null +++ b/internal/services/postgres/validate/flexible_server_storage_tier_test.go @@ -0,0 +1,30 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package validate + +import ( + "testing" + + "github.com/hashicorp/go-azure-sdk/resource-manager/postgresql/2023-06-01-preview/servers" +) + +func TestCompareAzureManagedDiskPerformance(t *testing.T) { + p10 := servers.AzureManagedDiskPerformanceTiers("p10") + p20 := servers.AzureManagedDiskPerformanceTiers("p20") + + notFaster := CompareAzureManagedDiskPerformance(p10, p20) + if notFaster { + t.Errorf("expected no error, %s is slower than %s - got: %v", p10, p20, notFaster) + } + + notFaster = CompareAzureManagedDiskPerformance(p20, p20) + if notFaster { + t.Errorf("expected no error, %s is slower/equal than %s - got: %v", p20, p20, notFaster) + } + + isFaster := CompareAzureManagedDiskPerformance(p20, p10) + if !isFaster { + t.Errorf("expected no error, %s is slower than %s - got: %v", p20, p10, isFaster) + } +} From 955ca45efd4b1e27bf52c1d585bcb81e74187c31 Mon Sep 17 00:00:00 2001 From: id Date: Tue, 14 May 2024 08:44:03 +0000 Subject: [PATCH 2/4] Add feedback --- .../postgresql_flexible_server_resource.go | 12 +++--- ...ostgresql_flexible_server_resource_test.go | 38 +++++++++++++++++++ .../validate/flexible_server_storage_tier.go | 15 -------- .../flexible_server_storage_tier_test.go | 30 --------------- 4 files changed, 44 insertions(+), 51 deletions(-) delete mode 100644 internal/services/postgres/validate/flexible_server_storage_tier_test.go diff --git a/internal/services/postgres/postgresql_flexible_server_resource.go b/internal/services/postgres/postgresql_flexible_server_resource.go index fd8b5ac70c6f..ded3274e47c5 100644 --- a/internal/services/postgres/postgresql_flexible_server_resource.go +++ b/internal/services/postgres/postgresql_flexible_server_resource.go @@ -382,6 +382,7 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { } newMb = newStorageMbRaw.(int) + newTier = newTierRaw.(string) // storage_mb can only be scaled up... if newMb < oldStorageMbRaw.(int) { @@ -399,16 +400,15 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { // storage_mb size... storageTiers := storageTierMappings[newMb] - newTier = "" - - // set new tier only when tier is changed or when value is faster than default - if oldTierRaw.(string) != newTierRaw.(string) || validate.CompareAzureManagedDiskPerformance(servers.AzureManagedDiskPerformanceTiers(newTierRaw.(string)), storageTiers.DefaultTier) { - newTier = newTierRaw.(string) + // set default tier for storage when there is now configuration available + if v := diff.GetRawConfig().AsValueMap()["storage_tier"]; v.IsNull() { + newTier = string(storageTiers.DefaultTier) + // set the new value that the update function will pickup and deploy new new storage tier. + diff.SetNew("storage_tier", newTier) } if newTier == "" { newTier = string(storageTiers.DefaultTier) - diff.SetNew("storage_tier", newTier) } // verify that the storage_tier is valid diff --git a/internal/services/postgres/postgresql_flexible_server_resource_test.go b/internal/services/postgres/postgresql_flexible_server_resource_test.go index 37414e552b1b..17ae1c14e7e5 100644 --- a/internal/services/postgres/postgresql_flexible_server_resource_test.go +++ b/internal/services/postgres/postgresql_flexible_server_resource_test.go @@ -524,6 +524,27 @@ func TestAccPostgresqlFlexibleServer_invalidStorageTierScalingStorageMbStorageTi }) } +func TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_postgresql_flexible_server", "test") + r := PostgresqlFlexibleServerResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.invalidStorageTierScaling(data, "P4", "32768"), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + data.ImportStep("administrator_password", "create_mode"), + { + Config: r.updateOnlyWithStorageMb(data, "65536"), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + }) +} + func (PostgresqlFlexibleServerResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { id, err := servers.ParseFlexibleServerID(state.ID) if err != nil { @@ -551,6 +572,23 @@ resource "azurerm_resource_group" "test" { `, data.RandomInteger, data.Locations.Primary) } +func (r PostgresqlFlexibleServerResource) updateOnlyWithStorageMb(data acceptance.TestData, storageMb string) string { + return fmt.Sprintf(` +%s +resource "azurerm_postgresql_flexible_server" "test" { + name = "acctest-fs-%d" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + administrator_login = "adminTerraform" + administrator_password = "QAZwsx123" + storage_mb = %s + version = "12" + sku_name = "GP_Standard_D2s_v3" + zone = "2" +} +`, r.template(data), data.RandomInteger, storageMb) +} + func (r PostgresqlFlexibleServerResource) basic(data acceptance.TestData) string { return fmt.Sprintf(` %s diff --git a/internal/services/postgres/validate/flexible_server_storage_tier.go b/internal/services/postgres/validate/flexible_server_storage_tier.go index 56105c44eddb..689d47de8e1d 100644 --- a/internal/services/postgres/validate/flexible_server_storage_tier.go +++ b/internal/services/postgres/validate/flexible_server_storage_tier.go @@ -4,9 +4,6 @@ package validate import ( - "strconv" - "strings" - "github.com/hashicorp/go-azure-sdk/resource-manager/postgresql/2023-06-01-preview/servers" ) @@ -91,15 +88,3 @@ func InitializeFlexibleServerStorageTierDefaults() map[int]StorageTiers { return storageTiersMappings } - -// CompareAzureManagedDiskPerformancef returns an bool comparing two AzureManagedDiskPerformanceTiers. -// The result will be false if disk == compareDisk, false if disk < compareDisk, and true if disk > compareDisk. -func CompareAzureManagedDiskPerformance(disk, compareDisk servers.AzureManagedDiskPerformanceTiers) bool { - diskP := strings.TrimLeft(strings.ToUpper(string(disk)), "P") - compareDiskP := strings.TrimLeft(strings.ToUpper(string(compareDisk)), "P") - - dPn, _ := strconv.Atoi(diskP) - cPn, _ := strconv.Atoi(compareDiskP) - - return dPn > cPn -} diff --git a/internal/services/postgres/validate/flexible_server_storage_tier_test.go b/internal/services/postgres/validate/flexible_server_storage_tier_test.go deleted file mode 100644 index e364e5060f0f..000000000000 --- a/internal/services/postgres/validate/flexible_server_storage_tier_test.go +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 - -package validate - -import ( - "testing" - - "github.com/hashicorp/go-azure-sdk/resource-manager/postgresql/2023-06-01-preview/servers" -) - -func TestCompareAzureManagedDiskPerformance(t *testing.T) { - p10 := servers.AzureManagedDiskPerformanceTiers("p10") - p20 := servers.AzureManagedDiskPerformanceTiers("p20") - - notFaster := CompareAzureManagedDiskPerformance(p10, p20) - if notFaster { - t.Errorf("expected no error, %s is slower than %s - got: %v", p10, p20, notFaster) - } - - notFaster = CompareAzureManagedDiskPerformance(p20, p20) - if notFaster { - t.Errorf("expected no error, %s is slower/equal than %s - got: %v", p20, p20, notFaster) - } - - isFaster := CompareAzureManagedDiskPerformance(p20, p10) - if !isFaster { - t.Errorf("expected no error, %s is slower than %s - got: %v", p20, p10, isFaster) - } -} From a0b06cd04a9ef38d8748df991201435f79435894 Mon Sep 17 00:00:00 2001 From: id Date: Tue, 14 May 2024 09:45:17 +0000 Subject: [PATCH 3/4] Add storage tier test --- ...ostgresql_flexible_server_resource_test.go | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/internal/services/postgres/postgresql_flexible_server_resource_test.go b/internal/services/postgres/postgresql_flexible_server_resource_test.go index 17ae1c14e7e5..b8fc78ad49ea 100644 --- a/internal/services/postgres/postgresql_flexible_server_resource_test.go +++ b/internal/services/postgres/postgresql_flexible_server_resource_test.go @@ -545,6 +545,27 @@ func TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb(t *testing.T) { }) } +func TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_postgresql_flexible_server", "test") + r := PostgresqlFlexibleServerResource{} + + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.invalidStorageTierScaling(data, "P4", "32768"), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + data.ImportStep("administrator_password", "create_mode"), + { + Config: r.updateOnlyWithStorageTier(data, "P10"), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + ), + }, + }) +} + func (PostgresqlFlexibleServerResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { id, err := servers.ParseFlexibleServerID(state.ID) if err != nil { @@ -572,6 +593,24 @@ resource "azurerm_resource_group" "test" { `, data.RandomInteger, data.Locations.Primary) } +func (r PostgresqlFlexibleServerResource) updateOnlyWithStorageTier(data acceptance.TestData, storageTier string) string { + return fmt.Sprintf(` +%s +resource "azurerm_postgresql_flexible_server" "test" { + name = "acctest-fs-%d" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + administrator_login = "adminTerraform" + administrator_password = "QAZwsx123" + storage_mb = 65536 + storage_tier = "%s" + version = "12" + sku_name = "GP_Standard_D2s_v3" + zone = "2" +} +`, r.template(data), data.RandomInteger, storageTier) +} + func (r PostgresqlFlexibleServerResource) updateOnlyWithStorageMb(data acceptance.TestData, storageMb string) string { return fmt.Sprintf(` %s From 33d93df64ce1386ac1eec39afa500f8b185f26d8 Mon Sep 17 00:00:00 2001 From: id Date: Wed, 15 May 2024 10:07:32 +0000 Subject: [PATCH 4/4] Add feedback and more tests --- .../postgresql_flexible_server_resource.go | 21 ++++++++---- ...ostgresql_flexible_server_resource_test.go | 34 +++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/internal/services/postgres/postgresql_flexible_server_resource.go b/internal/services/postgres/postgresql_flexible_server_resource.go index ded3274e47c5..2eec141b97b3 100644 --- a/internal/services/postgres/postgresql_flexible_server_resource.go +++ b/internal/services/postgres/postgresql_flexible_server_resource.go @@ -400,13 +400,6 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { // storage_mb size... storageTiers := storageTierMappings[newMb] - // set default tier for storage when there is now configuration available - if v := diff.GetRawConfig().AsValueMap()["storage_tier"]; v.IsNull() { - newTier = string(storageTiers.DefaultTier) - // set the new value that the update function will pickup and deploy new new storage tier. - diff.SetNew("storage_tier", newTier) - } - if newTier == "" { newTier = string(storageTiers.DefaultTier) } @@ -421,6 +414,20 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { } if !isValid { + if strings.EqualFold(oldTierRaw.(string), newTier) { + // The tier value did not change, so we need to determin if they are + // using the default value for the tier, or they actually defined the + // tier in the config or not... If they did not define + // the tier in the config we need to assign a new valid default + // tier for the newMb value. However, if the tier is in the config + // this is a valid error and should be returned... + if v := diff.GetRawConfig().AsValueMap()["storage_tier"]; v.IsNull() { + diff.SetNew("storage_tier", string(storageTiers.DefaultTier)) + log.Printf("[DEBUG]: 'storage_tier' was not valid and was not in the config assigning new default 'storage_tier' %q -> %q\n", newTier, storageTiers.DefaultTier) + return nil + } + } + return fmt.Errorf("invalid 'storage_tier' %q for defined 'storage_mb' size '%d', expected one of [%s]", newTier, newMb, azure.QuotedStringSlice(*storageTiers.ValidTiers)) } diff --git a/internal/services/postgres/postgresql_flexible_server_resource_test.go b/internal/services/postgres/postgresql_flexible_server_resource_test.go index b8fc78ad49ea..17ff0b543179 100644 --- a/internal/services/postgres/postgresql_flexible_server_resource_test.go +++ b/internal/services/postgres/postgresql_flexible_server_resource_test.go @@ -563,6 +563,23 @@ func TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier(t *testing.T) { check.That(data.ResourceName).ExistsInAzure(r), ), }, + data.ImportStep("administrator_password", "create_mode"), + { + Config: r.updateStorageTierWithoutProperty(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + // the storage tier is not changed to the default value, because p10 is still valid. + check.That(data.ResourceName).Key("storage_tier").HasValue("P10"), + ), + }, + data.ImportStep("administrator_password", "create_mode"), + { + Config: r.updateOnlyWithStorageMb(data, "262144"), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("storage_tier").HasValue("P15"), + ), + }, }) } @@ -611,6 +628,23 @@ resource "azurerm_postgresql_flexible_server" "test" { `, r.template(data), data.RandomInteger, storageTier) } +func (r PostgresqlFlexibleServerResource) updateStorageTierWithoutProperty(data acceptance.TestData) string { + return fmt.Sprintf(` +%s +resource "azurerm_postgresql_flexible_server" "test" { + name = "acctest-fs-%d" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + administrator_login = "adminTerraform" + administrator_password = "QAZwsx123" + storage_mb = 65536 + version = "12" + sku_name = "GP_Standard_D2s_v3" + zone = "2" +} +`, r.template(data), data.RandomInteger) +} + func (r PostgresqlFlexibleServerResource) updateOnlyWithStorageMb(data acceptance.TestData, storageMb string) string { return fmt.Sprintf(` %s