Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove forceNew for zone and standby_availability_zone of azurerm_postgresql_flexible_server #13507

Merged
merged 12 commits into from
Oct 21, 2021
Merged

Remove forceNew for zone and standby_availability_zone of azurerm_postgresql_flexible_server #13507

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ed8d646
update code
Sep 25, 2021
2b0775c
update code
Sep 25, 2021
0c6814a
update code
Sep 25, 2021
7197737
update code
Sep 25, 2021
6d6126b
update code
Sep 25, 2021
3779bda
update code
Sep 25, 2021
a3670b6
update code
Sep 25, 2021
b47c5c6
Merge remote-tracking branch 'upstream/main' into fixpostgresqlfsha2
Sep 26, 2021
62f5bcf
update code
Sep 26, 2021
5efba83
update code
Sep 26, 2021
351c327
update code
Sep 26, 2021
bc1a669
update code
Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 76 additions & 7 deletions internal/services/postgres/postgresql_flexible_server_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,16 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource {
}, false),
},

"zone": azure.SchemaZoneComputed(),
"zone": {
Type: pluginsdk.TypeString,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be Computed, since we should highlight the change when this occurs/isn't specified:

Suggested change
Computed: true,

Similarly, is there a reason this isn't Required? In other clouds it's the convention to have to specify a zone when creating resources

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TF would cause diff when it isn't specified. Because service backend would set it with random zone when it isn't specified in request body. So suggest to keep "Computed: true".

Service team confirmed generally user doesn't care where zone is. Service backend would optimize according to the actual situation and the success rate of the creation would be higher. So it isn't required property.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neil-yechenwei as above this needs to become un-computed here

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tombuildsstuff , as service team confirmed user doesn't care where zone is, so I think we shouldn't highlight the change when it isn't specified. Does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neil-yechenwei this is a core tenant of Terraform that we'll show you the drift - so this has to be uncomputed - I've opened #13843

ValidateFunc: validation.StringInSlice([]string{
"1",
"2",
"3",
}, false),
},

"create_mode": {
Type: pluginsdk.TypeString,
Expand Down Expand Up @@ -194,7 +203,17 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource {
string(postgresqlflexibleservers.HighAvailabilityModeZoneRedundant),
}, false),
},
"standby_availability_zone": azure.SchemaZoneComputed(),

"standby_availability_zone": {
Type: pluginsdk.TypeString,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same here)

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TF would cause diff when it isn't specified. Because service backend would set it with random zone when it isn't specified in request body. So suggest to keep "Computed: true".

Service team confirmed generally user doesn't care where standby_availability_zone is. Service backend would optimize according to the actual situation and the success rate of the creation would be higher. So it isn't required property.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neil-yechenwei as above this needs to become un-computed here

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as service team confirmed user doesn't care where zone is, so I think we shouldn't highlight the change when it isn't specified. Does it make sense?

ValidateFunc: validation.StringInSlice([]string{
"1",
"2",
"3",
}, false),
},
},
},
},
Expand All @@ -219,6 +238,7 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource {
},
}
}

func resourcePostgresqlFlexibleServerCreate(d *pluginsdk.ResourceData, meta interface{}) error {
subscriptionId := meta.(*clients.Client).Account.SubscriptionId
client := meta.(*clients.Client).Postgres.FlexibleServersClient
Expand Down Expand Up @@ -281,7 +301,7 @@ func resourcePostgresqlFlexibleServerCreate(d *pluginsdk.ResourceData, meta inte
Network: expandArmServerNetwork(d),
Version: postgresqlflexibleservers.ServerVersion(d.Get("version").(string)),
Storage: expandArmServerStorage(d),
HighAvailability: expandFlexibleServerHighAvailability(d.Get("high_availability").([]interface{})),
HighAvailability: expandFlexibleServerHighAvailability(d.Get("high_availability").([]interface{}), true),
Backup: expandArmServerBackup(d),
},
Sku: sku,
Expand Down Expand Up @@ -426,6 +446,35 @@ func resourcePostgresqlFlexibleServerUpdate(d *pluginsdk.ResourceData, meta inte
ServerPropertiesForUpdate: &postgresqlflexibleservers.ServerPropertiesForUpdate{},
}

var requireFailover bool
// failover is only supported when `zone` and `standby_availability_zone` is exchanged
switch {
case d.HasChange("zone") && d.HasChange("high_availability.0.standby_availability_zone"):
resp, err := client.Get(ctx, id.ResourceGroup, id.Name)
if err != nil {
return err
}

if props := resp.ServerProperties; props != nil {
zone := d.Get("zone").(string)
standbyZone := d.Get("high_availability.0.standby_availability_zone").(string)

if props.AvailabilityZone != nil && props.HighAvailability != nil && props.HighAvailability.StandbyAvailabilityZone != nil {
if zone == *props.HighAvailability.StandbyAvailabilityZone && standbyZone == *props.AvailabilityZone {
requireFailover = true
} else {
return fmt.Errorf("failover only supports exchange between `zone` and `standby_availability_zone`")
}
} else {
return fmt.Errorf("`standby_availability_zone` cannot be added after creation")
}
}
case !d.HasChange("zone") && !d.HasChange("high_availability.0.standby_availability_zone"):
requireFailover = false
default:
return fmt.Errorf("`zone` and `standby_availability_zone` should be changed or not changed together")
}

if d.HasChange("administrator_password") {
parameters.ServerPropertiesForUpdate.AdministratorLoginPassword = utils.String(d.Get("administrator_password").(string))
}
Expand Down Expand Up @@ -455,7 +504,7 @@ func resourcePostgresqlFlexibleServerUpdate(d *pluginsdk.ResourceData, meta inte
}

if d.HasChange("high_availability") {
parameters.HighAvailability = expandFlexibleServerHighAvailability(d.Get("high_availability").([]interface{}))
parameters.HighAvailability = expandFlexibleServerHighAvailability(d.Get("high_availability").([]interface{}), false)
}

future, err := client.Update(ctx, id.ResourceGroup, id.Name, parameters)
Expand All @@ -466,6 +515,23 @@ func resourcePostgresqlFlexibleServerUpdate(d *pluginsdk.ResourceData, meta inte
if err := future.WaitForCompletionRef(ctx, client.Client); err != nil {
return fmt.Errorf("waiting for the update of the Postgresql Flexible Server %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err)
}

if requireFailover {
restartParameters := &postgresqlflexibleservers.RestartParameter{
RestartWithFailover: utils.Bool(true),
FailoverMode: postgresqlflexibleservers.FailoverModePlannedFailover,
}

future, err := client.Restart(ctx, id.ResourceGroup, id.Name, restartParameters)
if err != nil {
return fmt.Errorf("failing over %s: %+v", *id, err)
}

if err := future.WaitForCompletionRef(ctx, client.Client); err != nil {
return fmt.Errorf("waiting for failover of %s: %+v", *id, err)
}
}

return resourcePostgresqlFlexibleServerRead(d, meta)
}

Expand Down Expand Up @@ -613,7 +679,7 @@ func flattenArmServerMaintenanceWindow(input *postgresqlflexibleservers.Maintena
}
}

func expandFlexibleServerHighAvailability(inputs []interface{}) *postgresqlflexibleservers.HighAvailability {
func expandFlexibleServerHighAvailability(inputs []interface{}, isCreate bool) *postgresqlflexibleservers.HighAvailability {
if len(inputs) == 0 || inputs[0] == nil {
return &postgresqlflexibleservers.HighAvailability{
Mode: postgresqlflexibleservers.HighAvailabilityModeDisabled,
Expand All @@ -626,8 +692,11 @@ func expandFlexibleServerHighAvailability(inputs []interface{}) *postgresqlflexi
Mode: postgresqlflexibleservers.HighAvailabilityMode(input["mode"].(string)),
}

if v, ok := input["standby_availability_zone"]; ok && v.(string) != "" {
result.StandbyAvailabilityZone = utils.String(v.(string))
// service team confirmed it doesn't support to update `standby_availability_zone` after creation
if isCreate {
if v, ok := input["standby_availability_zone"]; ok && v.(string) != "" {
result.StandbyAvailabilityZone = utils.String(v.(string))
}
}

return &result
Expand Down
54 changes: 53 additions & 1 deletion internal/services/postgres/postgresql_flexible_server_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,28 @@ func TestAccPostgresqlflexibleServer_pitr(t *testing.T) {
})
}

func TestAccPostgresqlflexibleServer_failover(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_postgresql_flexible_server", "test")
r := PostgresqlFlexibleServerResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.failover(data, "1", "2"),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep("administrator_password", "create_mode"),
{
Config: r.failover(data, "2", "1"),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep("administrator_password", "create_mode"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add a step to fail-back here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

})
}

func (PostgresqlFlexibleServerResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := parse.FlexibleServerID(state.ID)
if err != nil {
Expand Down Expand Up @@ -319,7 +341,7 @@ resource "azurerm_postgresql_flexible_server" "test" {

high_availability {
mode = "ZoneRedundant"
standby_availability_zone = "1"
standby_availability_zone = "2"
}

maintenance_window {
Expand Down Expand Up @@ -483,3 +505,33 @@ resource "azurerm_postgresql_flexible_server" "pitr" {
}
`, r.basic(data), data.RandomInteger, time.Now().Add(time.Duration(15)*time.Minute).UTC().Format(time.RFC3339))
}

func (r PostgresqlFlexibleServerResource) failover(data acceptance.TestData, parimaryZone string, standbyZone 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
version = "12"
administrator_login = "adminTerraform"
administrator_password = "QAZwsx123"
zone = "%s"
backup_retention_days = 10
storage_mb = 131072
sku_name = "GP_Standard_D2s_v3"

maintenance_window {
day_of_week = 0
start_hour = 0
start_minute = 0
}

high_availability {
mode = "ZoneRedundant"
standby_availability_zone = "%s"
}
}
`, r.template(data), data.RandomInteger, parimaryZone, standbyZone)
}
2 changes: 1 addition & 1 deletion website/docs/r/postgresql_flexible_server.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ The following arguments are supported:

* `administrator_password` - (Optional) The Password associated with the `administrator_login` for the PostgreSQL Flexible Server. Required when `create_mode` is `Default`.

* `zone` - (Optional) The availability zone of the PostgreSQL Flexible Server. Possible values are `1`, `2` and `3`. Changing this forces a new PostgreSQL Flexible Server to be created.
* `zone` - (Optional) The availability zone of the PostgreSQL Flexible Server. Possible values are `1`, `2` and `3`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Availability Zones available change per-region - some support No Zones, some support 1 & 2, others support 1, 2 and 3. We should add a callout to explain this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


* `backup_retention_days` - (Optional) The backup retention days for the PostgreSQL Flexible Server. Possible values are between `7` and `35` days.

Expand Down