From 59a36ae2831c73fa73903f85576fb7a63f2e0f49 Mon Sep 17 00:00:00 2001 From: lukaalba <44895214+lukaalba@users.noreply.github.com> Date: Sun, 8 Sep 2024 18:18:33 +0200 Subject: [PATCH] postgresql_database: Reassign objects owners if database owner changes (#458) Hi, this is my first Pull Request, so if something is missing or wrong, let me know. To resolve [439](https://github.com/cyrilgdn/terraform-provider-postgresql/issues/439) I suggest to add an additional parameter to the database resource called `alter_object_ownership`. If set to true, it will issue a REASSIGN OWNED BY query on the database when the owner changes, transfer the whole ownership in the database from the previous owner to the new one. As far as I can tell this is only possible if the user in the provider configuration is a superuser or a user which has grants for both the previous owner role and the new one. So I skipped the test in the rds-like test suite. --------- Co-authored-by: Lukas Albani Co-authored-by: Cyril Gaudin --- postgresql/resource_postgresql_database.go | 86 ++++++++++++-- .../resource_postgresql_database_test.go | 110 ++++++++++++++++++ .../docs/r/postgresql_database.html.markdown | 21 +++- 3 files changed, 201 insertions(+), 16 deletions(-) diff --git a/postgresql/resource_postgresql_database.go b/postgresql/resource_postgresql_database.go index 9cc9da55..6cabadf7 100644 --- a/postgresql/resource_postgresql_database.go +++ b/postgresql/resource_postgresql_database.go @@ -14,16 +14,17 @@ import ( ) const ( - dbAllowConnsAttr = "allow_connections" - dbCTypeAttr = "lc_ctype" - dbCollationAttr = "lc_collate" - dbConnLimitAttr = "connection_limit" - dbEncodingAttr = "encoding" - dbIsTemplateAttr = "is_template" - dbNameAttr = "name" - dbOwnerAttr = "owner" - dbTablespaceAttr = "tablespace_name" - dbTemplateAttr = "template" + dbAllowConnsAttr = "allow_connections" + dbCTypeAttr = "lc_ctype" + dbCollationAttr = "lc_collate" + dbConnLimitAttr = "connection_limit" + dbEncodingAttr = "encoding" + dbIsTemplateAttr = "is_template" + dbNameAttr = "name" + dbOwnerAttr = "owner" + dbTablespaceAttr = "tablespace_name" + dbTemplateAttr = "template" + dbAlterObjectOwnership = "alter_object_ownership" ) func resourcePostgreSQLDatabase() *schema.Resource { @@ -102,6 +103,12 @@ func resourcePostgreSQLDatabase() *schema.Resource { Computed: true, Description: "If true, then this database can be cloned by any user with CREATEDB privileges", }, + dbAlterObjectOwnership: { + Type: schema.TypeBool, + Optional: true, + Default: false, + Description: "If true, the owner of already existing objects will change if the owner changes", + }, }, } } @@ -393,6 +400,10 @@ func resourcePostgreSQLDatabaseUpdate(db *DBConnection, d *schema.ResourceData) return err } + if err := setAlterOwnership(db, d); err != nil { + return err + } + if err := setDBOwner(db, d); err != nil { return err } @@ -468,6 +479,7 @@ func setDBOwner(db *DBConnection, d *schema.ResourceData) error { } dbName := d.Get(dbNameAttr).(string) + sql := fmt.Sprintf("ALTER DATABASE %s OWNER TO %s", pq.QuoteIdentifier(dbName), pq.QuoteIdentifier(owner)) if _, err := db.Exec(sql); err != nil { return fmt.Errorf("Error updating database OWNER: %w", err) @@ -476,6 +488,60 @@ func setDBOwner(db *DBConnection, d *schema.ResourceData) error { return err } +func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { + if !d.HasChange(dbOwnerAttr) && !d.HasChange(dbAlterObjectOwnership) { + return nil + } + owner := d.Get(dbOwnerAttr).(string) + if owner == "" { + return nil + } + + alterOwnership := d.Get(dbAlterObjectOwnership).(bool) + if !alterOwnership { + return nil + } + currentUser := db.client.config.getDatabaseUsername() + + dbName := d.Get(dbNameAttr).(string) + + lockTxn, err := startTransaction(db.client, dbName) + if err := pgLockRole(lockTxn, currentUser); err != nil { + return err + } + defer deferredRollback(lockTxn) + + currentOwner, err := getDatabaseOwner(db, dbName) + if err != nil { + return fmt.Errorf("Error getting current database OWNER: %w", err) + } + + newOwner := d.Get(dbOwnerAttr).(string) + + if currentOwner == newOwner { + return nil + } + + currentOwnerGranted, err := grantRoleMembership(db, currentOwner, currentUser) + if err != nil { + return err + } + if currentOwnerGranted { + defer func() { + _, err = revokeRoleMembership(db, currentOwner, currentUser) + }() + } + sql := fmt.Sprintf("REASSIGN OWNED BY %s TO %s", pq.QuoteIdentifier(currentOwner), pq.QuoteIdentifier(newOwner)) + if _, err := lockTxn.Exec(sql); err != nil { + return fmt.Errorf("Error reassigning objects owned by '%s': %w", currentOwner, err) + } + + if err := lockTxn.Commit(); err != nil { + return fmt.Errorf("error committing reassign: %w", err) + } + return nil +} + func setDBTablespace(db QueryAble, d *schema.ResourceData) error { if !d.HasChange(dbTablespaceAttr) { return nil diff --git a/postgresql/resource_postgresql_database_test.go b/postgresql/resource_postgresql_database_test.go index bf8d255f..0ac97445 100644 --- a/postgresql/resource_postgresql_database_test.go +++ b/postgresql/resource_postgresql_database_test.go @@ -43,6 +43,8 @@ func TestAccPostgresqlDatabase_Basic(t *testing.T) { "postgresql_database.default_opts", "connection_limit", "-1"), resource.TestCheckResourceAttr( "postgresql_database.default_opts", "is_template", "false"), + resource.TestCheckResourceAttr( + "postgresql_database.default_opts", "alter_object_ownership", "false"), resource.TestCheckResourceAttr( "postgresql_database.modified_opts", "owner", "myrole"), @@ -62,6 +64,8 @@ func TestAccPostgresqlDatabase_Basic(t *testing.T) { "postgresql_database.modified_opts", "connection_limit", "10"), resource.TestCheckResourceAttr( "postgresql_database.modified_opts", "is_template", "true"), + resource.TestCheckResourceAttr( + "postgresql_database.modified_opts", "alter_object_ownership", "true"), resource.TestCheckResourceAttr( "postgresql_database.pathological_opts", "owner", "myrole"), @@ -266,6 +270,78 @@ resource postgresql_database "test_db" { }) } +// Test the case where the owned objects by the previous database owner are altered. +func TestAccPostgresqlDatabase_AlterObjectOwnership(t *testing.T) { + skipIfNotAcc(t) + + const ( + databaseSuffix = "ownership" + tableName = "testtable1" + previous_owner = "previous_owner" + new_owner = "new_owner" + ) + + databaseName := fmt.Sprintf("%s_%s", dbNamePrefix, databaseSuffix) + + config := getTestConfig(t) + dsn := config.connStr("postgres") + + for _, role := range []string{previous_owner, new_owner} { + dbExecute( + t, dsn, + fmt.Sprintf("CREATE ROLE %s;", role), + ) + defer func(role string) { + dbExecute(t, dsn, fmt.Sprintf("DROP ROLE %s", role)) + }(role) + + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + testAccPreCheck(t) + testSuperuserPreCheck(t) + }, + Providers: testAccProviders, + CheckDestroy: testAccCheckPostgresqlDatabaseDestroy, + Steps: []resource.TestStep{ + { + Config: ` +resource postgresql_database "test_db" { + name = "tf_tests_db_ownership" + owner = "previous_owner" + alter_object_ownership = true +} +`, + Check: func(*terraform.State) error { + // To test default privileges, we need to create a table + // after having apply the state. + _ = createTestTables(t, databaseSuffix, []string{tableName}, previous_owner) + return nil + }, + }, + { + Config: ` +resource postgresql_database "test_db" { + name = "tf_tests_db_ownership" + owner = "new_owner" + alter_object_ownership = true +} +`, + Check: resource.ComposeTestCheckFunc( + testAccCheckPostgresqlDatabaseExists("postgresql_database.test_db"), + resource.TestCheckResourceAttr("postgresql_database.test_db", "name", databaseName), + resource.TestCheckResourceAttr("postgresql_database.test_db", "owner", new_owner), + resource.TestCheckResourceAttr("postgresql_database.test_db", "alter_object_ownership", "true"), + + checkTableOwnership(t, config.connStr(databaseName), new_owner, tableName), + ), + }, + }, + }) + +} + func checkUserMembership( t *testing.T, dsn, member, role string, shouldHaveRole bool, ) resource.TestCheckFunc { @@ -306,6 +382,38 @@ func checkUserMembership( } } +func checkTableOwnership( + t *testing.T, dsn, owner, tableName string, +) resource.TestCheckFunc { + return func(s *terraform.State) error { + db, err := sql.Open("postgres", dsn) + if err != nil { + t.Fatalf("could not create connection pool: %v", err) + } + defer db.Close() + + var _rez int + + err = db.QueryRow(` + SELECT 1 FROM pg_tables + WHERE tablename = $1 AND tableowner = $2 + `, tableName, owner).Scan(&_rez) + + switch { + case err == sql.ErrNoRows: + return fmt.Errorf( + "User %s should be owner of %s but is not", owner, tableName, + ) + case err != nil: + t.Fatalf("Error checking table ownership. %v", err) + + } + + return nil + + } +} + func testAccCheckPostgresqlDatabaseDestroy(s *terraform.State) error { client := testAccProvider.Meta().(*Client) @@ -396,6 +504,7 @@ resource "postgresql_database" "default_opts" { lc_ctype = "C" connection_limit = -1 is_template = false + alter_object_ownership = false } resource "postgresql_database" "modified_opts" { @@ -407,6 +516,7 @@ resource "postgresql_database" "modified_opts" { lc_ctype = "en_US.UTF-8" connection_limit = 10 is_template = true + alter_object_ownership = true } resource "postgresql_database" "pathological_opts" { diff --git a/website/docs/r/postgresql_database.html.markdown b/website/docs/r/postgresql_database.html.markdown index 5110e0f5..d2715288 100644 --- a/website/docs/r/postgresql_database.html.markdown +++ b/website/docs/r/postgresql_database.html.markdown @@ -17,12 +17,13 @@ within a PostgreSQL server instance. ```hcl resource "postgresql_database" "my_db" { - name = "my_db" - owner = "my_role" - template = "template0" - lc_collate = "C" - connection_limit = -1 - allow_connections = true + name = "my_db" + owner = "my_role" + template = "template0" + lc_collate = "C" + connection_limit = -1 + allow_connections = true + alter_object_ownership = true } ``` @@ -82,6 +83,14 @@ resource "postgresql_database" "my_db" { force the creation of a new resource as this value can only be changed when a database is created. +* `alter_object_ownership` - (Optional) If `true`, the change of the database + `owner` will also include a reassignment of the ownership of preexisting + objects like tables or sequences from the previous owner to the new one. + If set to `false` (the default), then the previous database `owner` will still + hold the ownership of the objects in that database. To alter existing objects in + the database, you must be a direct or indirect member of the specified role, or + the username in the provider must be superuser. + ## Import Example `postgresql_database` supports importing resources. Supposing the following