feat(Cloud Databases): Create and Delete Logical Replication Slots for databases-for-postgresql #4116

omaraibrahim · 2022-10-18T17:00:24Z

We support the creation and deletion of logical replication slots in our API and CLI as seen here: https://cloud.ibm.com/apidocs/cloud-databases-api/cloud-databases-api-v5#createlogicalreplicationslot

However, we do not support it in our current Terraform integration.

This PR gives users the ability to:

Create and delete their logical replication slots for databases-for-postgresql.
The ability to change the password of the replication (repl) user upon creation, which is a prereq for creating a logical replication slot.
The ability to change the passwords of all pre-existing users upon update.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Output from acceptance testing:

make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseInstancePostgresBasic'
--- PASS: TestAccIBMDatabaseInstancePostgresBasic (1794.68s)
PASS
ok      github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database        1795.827s
...

make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseInstanceMongodbBasic'
--- PASS: TestAccIBMDatabaseInstanceMongodbBasic (1835.40s)
PASS
ok      github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database        1836.821s

make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseInstance_Redis_Basic'
--- PASS: TestAccIBMDatabaseInstance_Redis_Basic (1324.40s)
PASS
ok      github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database        1325.324s

alexhemard · 2022-10-25T15:08:31Z

ibm/service/database/resource_ibm_database.go

@@ -467,6 +467,29 @@ func ResourceIBMDatabaseInstance() *schema.Resource {
 					},
 				},
 			},
+			"logical_replication_slot": {


This should have validation to enforce only postgresql deployments can use logical_replication_slot

Thanks! Took your recommendation and added it here:

terraform-provider-ibm/ibm/service/database/resource_ibm_database.go

Line 2301 in 7a2fac0

if service != "databases-for-postgresql" {

:D

I think we might want to do this in a CustomizeDiff function since this will blow up during runtime rather than during the plan stage

alexhemard · 2022-10-25T15:09:04Z

ibm/service/database/resource_ibm_database.go

@@ -2209,6 +2237,14 @@ func resourceIBMDatabaseInstanceUpdate(context context.Context, d *schema.Resour
 				change.Old = nil
 			}

+			newName, ok := change.New["name"]
+			isPgOrEdb := strings.Contains(service, "postgresql") || strings.Contains(service, "enterprisedb")


Logical Replication is PG only :D But the repl user exists in both edb and pg. So we need to handle it for both cases.

alexhemard · 2022-10-25T15:18:51Z

ibm/service/database/resource_ibm_database.go

+			newName, ok := change.New["name"]
+			isPgOrEdb := strings.Contains(service, "postgresql") || strings.Contains(service, "enterprisedb")
+			if ok && isPgOrEdb && newName.(string) == "repl" {
+				updateReplUser(change.New["password"].(string), instanceID, change.New["type"].(string), meta, d)


if this method is necessary can it be a generic changeUserPassword method?

alexhemard · 2022-10-25T15:33:46Z

ibm/service/database/resource_ibm_database.go

@@ -2209,6 +2237,14 @@ func resourceIBMDatabaseInstanceUpdate(context context.Context, d *schema.Resour
 				change.Old = nil
 			}

+			newName, ok := change.New["name"]
+			isPgOrEdb := strings.Contains(service, "postgresql") || strings.Contains(service, "enterprisedb")


Rather than create a special case for the repl user seems like we should always attempt to update the password first and then create the user if it does not exist.

terraform-provider-ibm/ibm/service/database/resource_ibm_database.go

Line 2201 in 4f61e10

if change.Old != nil && change.New != nil {

Went ahead and did it!

…This is done to support backups and special users

alexhemard · 2022-10-27T20:11:00Z

ibm/service/database/resource_ibm_database.go

@@ -2144,6 +2171,7 @@ func resourceIBMDatabaseInstanceUpdate(context context.Context, d *schema.Resour
 		}

 		oldUsers, newUsers := d.GetChange("users")
+		log.Printf("oldUsers %v newUsers %v", oldUsers, newUsers)


Remove this

loool thanks!

alexhemard · 2022-10-27T20:15:33Z

ibm/service/database/resource_ibm_database.go

-				Username: core.StringPtr(userEl["name"].(string)),
-				Password: core.StringPtr(userEl["password"].(string)),
-			}
+			if userEl["name"].(string) == "repl" && (strings.Contains(serviceName, "postgresql") || strings.Contains(serviceName, "enterprisedb")) {


Instead of a special case for the repl user, we should attempt to update the password if user creation fails in case it already exists i.e. restoring from a backup

alexhemard · 2022-10-27T20:17:52Z

ibm/service/database/resource_ibm_database.go

@@ -467,6 +467,29 @@ func ResourceIBMDatabaseInstance() *schema.Resource {
 					},
 				},
 			},
+			"logical_replication_slot": {


I think we might want to do this in a CustomizeDiff function since this will blow up during runtime rather than during the plan stage

…cation for non-PG databases on plan

alexhemard · 2022-11-01T14:23:19Z

ibm/service/database/resource_ibm_database.go

+	updatePass, err := waitForDatabaseTaskComplete(taskID, d, meta, d.Timeout(schema.TimeoutUpdate))
+
+	// user exists but task failed to complete
+	if oldUserExists && err != nil {


Do we need this oldUserExists boolean? Couldn't the password update fail if the user was deleted outside of terraform?

Yup that is true :) I guess we should try creating the user afterwards regardless.

alexhemard · 2022-11-08T18:01:58Z

ibm/service/database/resource_ibm_database.go

+	}
+
+	// Updating the password has failed but an old user does not exist
+	if !updatePass {


We should call CreateDatabaseUser if ChangeUserPassword password failed

alexhemard · 2022-11-08T18:18:55Z

ibm/service/database/resource_ibm_database.go

+			return diag.FromErr(fmt.Errorf("[ERROR] Error getting database client settings: %s", err))
+		}
+
+		oldSlots, newSlots := d.GetChange("logical_replication_slot")


Since we can't update anything, we don't need to track changes to the state. Something like this may be more straightforwardL

oldList, newList := d.GetChange("logical_replication_slot") if oldList == nil { oldList = new(schema.Set) } if newList == nil { newList = new(schema.Set) } os := oldList.(*schema.Set) ns := newList.(*schema.Set) remove := os.Difference(ns).List() add := ns.Difference(os).List() if len(add) > 0 { for _, slot := range add { addSlot := slot.(map[string]interface{}) logicalReplicationSlot := &clouddatabasesv5.LogicalReplicationSlot{ Name: core.StringPtr(addSlot["name"].(string)), DatabaseName: core.StringPtr(addSlot["database_name"].(string)), PluginType: core.StringPtr(addSlot["plugin_type"].(string)), } // ... } } if len(remove) > 0 { for _, slot := range remove { removeSlot := slot.(map[string]interface{}) // ... } }

alexhemard · 2022-11-09T15:04:36Z

ibm/service/database/resource_ibm_database.go

@@ -3139,3 +3159,67 @@ func checkV5Groups(_ context.Context, diff *schema.ResourceDiff, meta interface{

 	return nil
 }
+
+// Updates and creates users. Because we cannot get users, we first attempt to update the users, then create them
+func userUpdateCreate(userData map[string]interface{}, instanceID string, meta interface{}, d *schema.ResourceData) diag.Diagnostics {


It doesn't look like we do anything with value returned by this method. I'd recommend changing the return value to (err error) and check and return the error when calling it

err := userUpdateCreate(userEl, instanceID, meta, d) if err != nil { return diag.FromErr(err) }

alexhemard · 2022-11-28T18:09:40Z

ibm/service/database/resource_ibm_database.go

+
+		_, logicalReplicationList := d.GetChange("logical_replication_slot")
+
+		if logicalReplicationList == nil {


I don't think we need this nil check. We've already checked that logical_replication_slot is set.

alexhemard

LGTM - can you run more ITs before merging on other deployment types?
i.e. make testacc TESTARGS='-run=TestAccIBMDatabaseInstance'

omaraibrahim · 2022-11-29T17:14:38Z

Ran more IT :D

omaraibrahim added 4 commits October 3, 2022 13:58

added logical replication slot to schema and create function

7f636ce

added logical replication creation and deletion. it works :D

dc7ed24

finished writing logical replication code, testing, and documenting

349dccb

updated secrets.baseline

4f61e10

alexhemard reviewed Oct 25, 2022

View reviewed changes

reformated code so that we attempt updating the user password first. …

7a2fac0

…This is done to support backups and special users

alexhemard reviewed Oct 27, 2022

View reviewed changes

omaraibrahim and others added 2 commits October 28, 2022 11:18

update users first and then create them. prevent use of logical repli…

44dd3dc

…cation for non-PG databases on plan

Merge branch 'master' into logical_replication

6257092

alexhemard reviewed Nov 8, 2022

View reviewed changes

refactored logical replication update code according to review

23c3ce8

alexhemard reviewed Nov 9, 2022

View reviewed changes

omaraibrahim and others added 9 commits November 9, 2022 10:38

updated error messages

2b78695

Merge branch 'master' into logical_replication

700ae1c

fixed bug. changed to getok from getokexists

e299e4c

Merge branch 'IBM-Cloud:master' into logical_replication

84cf939

implemented logical replication on create

06aafc4

modified tests

1a7625d

Update database.html.markdown

866c817

Merge branch 'master' into logical_replication

7a099a4

removed accidental duplication

a8bcaf1

alexhemard reviewed Nov 28, 2022

View reviewed changes

removed unnessary code block

99334ca

alexhemard approved these changes Nov 28, 2022

View reviewed changes

fixed description typo

8a74dbb

obai-1 merged commit ad38584 into IBM-Cloud:master Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Cloud Databases): Create and Delete Logical Replication Slots for databases-for-postgresql #4116

feat(Cloud Databases): Create and Delete Logical Replication Slots for databases-for-postgresql #4116

omaraibrahim commented Oct 18, 2022 •

edited

Loading

alexhemard Oct 25, 2022

omaraibrahim Oct 27, 2022

alexhemard Oct 27, 2022 •

edited

Loading

omaraibrahim Oct 28, 2022

alexhemard Oct 25, 2022

omaraibrahim Oct 25, 2022

alexhemard Oct 25, 2022

omaraibrahim Oct 27, 2022

alexhemard Oct 25, 2022

omaraibrahim Oct 25, 2022

omaraibrahim Oct 27, 2022

alexhemard Oct 27, 2022

omaraibrahim Oct 27, 2022

alexhemard Oct 27, 2022

alexhemard Oct 27, 2022 •

edited

Loading

alexhemard Nov 1, 2022

omaraibrahim Nov 8, 2022

alexhemard Nov 8, 2022

alexhemard Nov 8, 2022

alexhemard Nov 9, 2022

alexhemard Nov 28, 2022

alexhemard left a comment

omaraibrahim commented Nov 29, 2022


		_, logicalReplicationList := d.GetChange("logical_replication_slot")

		if logicalReplicationList == nil {

feat(Cloud Databases): Create and Delete Logical Replication Slots for databases-for-postgresql #4116

feat(Cloud Databases): Create and Delete Logical Replication Slots for databases-for-postgresql #4116

Conversation

omaraibrahim commented Oct 18, 2022 • edited Loading

Community Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexhemard Oct 27, 2022 • 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

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

Choose a reason for hiding this comment

alexhemard Oct 27, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexhemard left a comment

Choose a reason for hiding this comment

omaraibrahim commented Nov 29, 2022

omaraibrahim commented Oct 18, 2022 •

edited

Loading

alexhemard Oct 27, 2022 •

edited

Loading

alexhemard Oct 27, 2022 •

edited

Loading