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.

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

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

Merged
merged 19 commits into from Dec 1, 2022

Conversation

omaraibrahim
Copy link
Contributor

@omaraibrahim omaraibrahim commented Oct 18, 2022

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:

  1. Create and delete their logical replication slots for databases-for-postgresql.
  2. The ability to change the password of the replication (repl) user upon creation, which is a prereq for creating a logical replication slot.
  3. 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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Took your recommendation and added it here:

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :D

@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

PG only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :D

@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Smart!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and did it!

…This is done to support backups and special users
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove 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.

loool thanks!

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")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

updatePass, err := waitForDatabaseTaskComplete(taskID, d, meta, d.Timeout(schema.TimeoutUpdate))

// user exists but task failed to complete
if oldUserExists && err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// Updating the password has failed but an old user does not exist
if !updatePass {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should call CreateDatabaseUser if ChangeUserPassword password failed

return diag.FromErr(fmt.Errorf("[ERROR] Error getting database client settings: %s", err))
}

oldSlots, newSlots := d.GetChange("logical_replication_slot")
Copy link
Contributor

Choose a reason for hiding this comment

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

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{})
		
		// ...
	}
}

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}


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

if logicalReplicationList == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@alexhemard alexhemard left a comment

Choose a reason for hiding this comment

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

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

@omaraibrahim
Copy link
Contributor Author

Ran more IT :D

@obai-1 obai-1 merged commit ad38584 into IBM-Cloud:master Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants