Skip to content

Rdb: Allow RDB instance upgrade to HA without replacing the resource #518

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

Merged
merged 7 commits into from
Jul 23, 2020
Merged

Rdb: Allow RDB instance upgrade to HA without replacing the resource #518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c72261
Allow RDB instance upgrade to HA without replacing the resource.
Jul 3, 2020
5128fb6
Add RDB conversion to HA into acceptance tests.
Jul 7, 2020
f5bf723
RDB user_name and password attributes cannot be updated from the
Jul 7, 2020
1b6696f
RDB: rework instance upgrade logic to avoid code duplication.
Jul 7, 2020
07ff615
Syntax fix.
Jul 22, 2020
0422c04
RDB: allow updating initial user password.
Jul 22, 2020
5a8aea5
RDB: Sleep between instance upgrades to prevent inconsistent state.
Jul 22, 2020
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
48 changes: 41 additions & 7 deletions scaleway/resource_rdb_instance_beta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package scaleway

import (
"io/ioutil"
"time"

"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/scaleway/scaleway-sdk-go/api/rdb/v1"
Expand Down Expand Up @@ -39,7 +40,6 @@ func resourceScalewayRdbInstanceBeta() *schema.Resource {
"is_ha_cluster": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is out of this MR but when reading it this field name seems strange, IMHO enable_ha_cluster would make more sense don't you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would make more sense indeed. is_ha_cluster is what is being used currently in the CreateInstance API.

So I am balancing using a better name vs keeping the attribute name in sync with the API and dealing with renaming an attribute without breaking compatibility.

I am more inclined to keep it as is for now, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it makes more sense to write enable_ha_cluster = true in the HCL:is_ha_cluster is the read only name of the property through the API, whereas enable_ha_cluster is an actionnable attribute in the terraform config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll update this in a separate PR.

Type: schema.TypeBool,
Optional: true,
ForceNew: true,
Default: false,
Description: "Enable or disable high availability for the database instance",
},
Expand All @@ -52,11 +52,13 @@ func resourceScalewayRdbInstanceBeta() *schema.Resource {
"user_name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

imho we should add a separate user resource, and if db creation with no user is not on the table, we should at least handle the update here, not a new resource

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that splitting the user to a separate resource makes more sens in TF. This would imply allowing the creation of the database without any user in the API or silently deleting it in the TF create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am planning on adding a separate user resource in an upcoming PR.

I am concerned about allowing user changes in the instance resource as it could potentially conflict with definition in the user resource.

For example in this template definition (after adding a user object):

resource scaleway_rdb_instance_beta db {
  name = "test-terraform"
  node_type = "db-dev-m"
  engine = "PostgreSQL-11"
  is_ha_cluster = true
  user_name = "admin"
  password = "azerty"
}

resource scaleway_rdb_user_beta db_admin {
  instance_id = scaleway_rdb_instance_beta.db.id
  name = "toto"
  password = "R34lP4sSw#Rd"
  is_admin = true
}

We don’t want to have a confusion on which password is used, so I want to set the expectation that the password in the user definition "always win", which is only possible if we don’t allow updates on the instance resource. As such the user password is always updated to the user object definition after the instance is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the PR to add a specific resource for database users #520

Description: "Identifier for the first user of the database instance",
},
"password": {
Type: schema.TypeString,
Required: true,
Sensitive: true,
Description: "Password for the first user of the database instance",
},
"tags": {
Expand Down Expand Up @@ -234,13 +236,26 @@ func resourceScalewayRdbInstanceBetaUpdate(d *schema.ResourceData, m interface{}
if err != nil {
return err
}

upgradeInstanceRequests := []rdb.UpgradeInstanceRequest(nil)
if d.HasChange("node_type") {
_, err = rdbAPI.UpgradeInstance(&rdb.UpgradeInstanceRequest{
Region: region,
InstanceID: ID,
NodeType: scw.StringPtr(d.Get("node_type").(string)),
})
upgradeInstanceRequests = append(upgradeInstanceRequests,
rdb.UpgradeInstanceRequest{
Region: region,
InstanceID: ID,
NodeType: scw.StringPtr(d.Get("node_type").(string)),
})
}

if d.HasChange("is_ha_cluster") {
upgradeInstanceRequests = append(upgradeInstanceRequests,
rdb.UpgradeInstanceRequest{
Region: region,
InstanceID: ID,
EnableHa: scw.BoolPtr(d.Get("is_ha_cluster").(bool)),
})
}
for _, request := range upgradeInstanceRequests {
_, err = rdbAPI.UpgradeInstance(&request)
if err != nil {
return err
}
Expand All @@ -253,7 +268,26 @@ func resourceScalewayRdbInstanceBetaUpdate(d *schema.ResourceData, m interface{}
if err != nil {
return err
}

// Wait for the instance to settle after upgrading
time.Sleep(30 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

What issue did you encounter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an issue on RDB side. I will remove this when it is resolved.


}

if d.HasChange("password") {
req := &rdb.UpdateUserRequest{
Region: region,
InstanceID: ID,
Name: d.Get("user_name").(string),
Password: scw.StringPtr(d.Get("password").(string)),
}

_, err = rdbAPI.UpdateUser(req)
if err != nil {
return err
}
}

return resourceScalewayRdbInstanceBetaRead(d, m)
}

Expand Down
8 changes: 4 additions & 4 deletions scaleway/resource_rdb_instance_beta_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestAccScalewayRdbInstanceBeta(t *testing.T) {
name = "test-rdb"
node_type = "db-dev-s"
engine = "PostgreSQL-11"
is_ha_cluster = true
is_ha_cluster = false
disable_backup = true
user_name = "my_initial_user"
password = "thiZ_is_v&ry_s3cret"
Expand All @@ -65,7 +65,7 @@ func TestAccScalewayRdbInstanceBeta(t *testing.T) {
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "name", "test-rdb"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "node_type", "db-dev-s"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "engine", "PostgreSQL-11"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "is_ha_cluster", "true"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "is_ha_cluster", "false"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "disable_backup", "true"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "user_name", "my_initial_user"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "password", "thiZ_is_v&ry_s3cret"),
Expand All @@ -86,7 +86,7 @@ func TestAccScalewayRdbInstanceBeta(t *testing.T) {
is_ha_cluster = true
disable_backup = false
user_name = "my_initial_user"
password = "thiZ_is_v&ry_s3cret"
password = "thiZ_is_v&ry_s8cret"
tags = [ "terraform-test", "scaleway_rdb_instance_beta", "minimal" ]
}
`,
Expand All @@ -98,7 +98,7 @@ func TestAccScalewayRdbInstanceBeta(t *testing.T) {
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "is_ha_cluster", "true"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "disable_backup", "false"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "user_name", "my_initial_user"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "password", "thiZ_is_v&ry_s3cret"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "password", "thiZ_is_v&ry_s8cret"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "tags.0", "terraform-test"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "tags.1", "scaleway_rdb_instance_beta"),
resource.TestCheckResourceAttr("scaleway_rdb_instance_beta.main", "tags.2", "minimal"),
Expand Down