[CMCSMACD-4695] Add a function for syncing users and roles

[hundt-corbalt]

This will be used instead of migrations for making sure that a database has the right users and roles.

Testing: pulled in to dso-metrics repo and added a call to the migrate binary. Verified that it appeared to add users and fix roles as necessary.

PR Checklist

• New automated tests have been written to the extent possible.
• The code has been checked for structural/syntactic validity.
  • AMI/application: a build was performed
  • terraform changes: "terraform plan" checked on every affected environment
• (If applicable) the code has been manually tested on our infrastructure.
  • AMI/application: deployed an a test or dev environment
  • terraform changes: applied to test or dev environment
  • script: run against test or dev environment
• Likely failure points and new functionality have been identified and tested manually.
  Examples:
  • Application manually run in a way that triggers any new branches
  • AMI logged into and changes verified from login shell
• Pull request description includes a description of all the manual steps performed to accomplish the above.

To provide feedback on this template, visit https://docs.google.com/document/d/1YfTv7Amyop5G_8w1c2GJ_Mu-70L0KkZHhm9f9umDi3U/edit

[jonah-corbalt]

Looks good, I left a few comments.

[jonah-corbalt]

I think here we will also need to grant the rds_iam role per docs.

[ben-harvey]

I think the caller will pass rds_iam as one of the GrantRoles when they want to use RDS IAM auth

[ben-harvey]

Here's how it looks on my branch. UsePasswordAuth is false if we detect the Lambda runtime

func ensureUsersAndMigrate(usePasswordAuth bool) error {
	users := []migrations.PostgreSQLUser{
		{
			Username: "reader",
			GrantRoles: []string{
				postgresReadAllRole,
			},
		},
		{
			Username: "writer",
			GrantRoles: []string{
				postgresReadAllRole,
				postgresWriteAllRole,
			},
		},
	}

	// When using RDS IAM auth instead of passwords, users also need the rds_iam role
	if !usePasswordAuth {
		for i := range users {
			users[i].GrantRoles = append(users[i].GrantRoles, rdsIAMRole)
		}
	}

	err := migrations.EnsureUsersWithRoles(db, users, usePasswordAuth)
	if err != nil {
		return fmt.Errorf("error ensuring users with roles: %w", err)
	}
	return migrations.Migrate(db, schema.AllMigrations())
}

[jonahb]

Ah, okay, so that logic is in the caller — makes sense.

[jonahb]

Actually, on second thought, maybe we do want to include that in the function since everyone will want it and it simplifies the caller.

[ben-harvey]

That would work too

[hundt-corbalt]

Check it out now and let me know what you all think.

[jonah-corbalt]

Do we need the committed flag? This example suggests doing tx.Rollback in all cases since it will be a noop if the transaction has been committed. If we want to print the error from tx.Rollback, we could check that it's not sql.ErrTxDone.

[hundt-corbalt]

Huh. The API docs don't really make abundantly clear that this is a good idea, but since this section of the docs suggests it I guess it's fine! Done.

[jonah-corbalt]

Another option would be to create optimistically and catch an exception (static, less interpolation):

createUserSQL := fmt.Sprintf(`
  DO $$
  BEGIN
    CREATE USER %s;
  EXCEPTION
    WHEN duplicate_object THEN
      NULL;
  END
  $$;
`, pq.QuoteIdentifier(user.Username),
)

[hundt-corbalt]

I do kind of like that, but also reading the docs about exception handling makes me a little nervous. They recommend not doing it if you can avoid it (although I think just for performance reasons?), and there are some semantics regarding transactions that I don't think would matter here but I'm not 100% confident. I'm inclined to leave it as is.

[hundt-corbalt]

Thanks for the reviews!

[hundt-corbalt]

Huh. The API docs don't really make abundantly clear that this is a good idea, but since this section of the docs suggests it I guess it's fine! Done.

[hundt-corbalt]

I do kind of like that, but also reading the docs about exception handling makes me a little nervous. They recommend not doing it if you can avoid it (although I think just for performance reasons?), and there are some semantics regarding transactions that I don't think would matter here but I'm not 100% confident. I'm inclined to leave it as is.