From 3298f528f0d71766802b78472d1cd670cf1f983f Mon Sep 17 00:00:00 2001 From: Lukas Albani Date: Fri, 26 Jul 2024 16:17:38 +0200 Subject: [PATCH 1/8] Alter existing objects if owner changes --- postgresql/resource_postgresql_database.go | 80 ++++++++++++-- .../resource_postgresql_database_test.go | 103 ++++++++++++++++++ 2 files changed, 173 insertions(+), 10 deletions(-) diff --git a/postgresql/resource_postgresql_database.go b/postgresql/resource_postgresql_database.go index 9cc9da55..e7128bcc 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,54 @@ 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) { + 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 lockTxn.Commit() + + currentOwner, err := getDatabaseOwner(db, dbName) + if err != nil { + return fmt.Errorf("Error getting current database OWNER: %w", err) + } + + currentOwnerGranted, err := grantRoleMembership(db, currentOwner, currentUser) + if err != nil { + return err + } + if currentOwnerGranted { + defer func() { + _, err = revokeRoleMembership(db, currentOwner, currentUser) + }() + } + + newOwner := d.Get(dbOwnerAttr).(string) + + 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) + } + return nil + } + + 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..bb6061fc 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,71 @@ 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), + ) + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(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 +375,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 +497,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 +509,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" { From 2d4540efce8cf06269e1d87cc86e31a56a70be94 Mon Sep 17 00:00:00 2001 From: Lukas Albani Date: Mon, 29 Jul 2024 17:37:56 +0200 Subject: [PATCH 2/8] Skip test if not performed by a superuser --- postgresql/resource_postgresql_database_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/postgresql/resource_postgresql_database_test.go b/postgresql/resource_postgresql_database_test.go index bb6061fc..081aea8b 100644 --- a/postgresql/resource_postgresql_database_test.go +++ b/postgresql/resource_postgresql_database_test.go @@ -294,7 +294,10 @@ func TestAccPostgresqlDatabase_AlterObjectOwnership(t *testing.T) { } resource.Test(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, + PreCheck: func() { + testAccPreCheck(t) + testSuperuserPreCheck(t) + }, Providers: testAccProviders, CheckDestroy: testAccCheckPostgresqlDatabaseDestroy, Steps: []resource.TestStep{ From 11038f612cea016d8d94f94d265a32426f3329a7 Mon Sep 17 00:00:00 2001 From: Lukas Albani Date: Tue, 30 Jul 2024 08:17:33 +0200 Subject: [PATCH 3/8] Update documentation of database resource --- .../docs/r/postgresql_database.html.markdown | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/website/docs/r/postgresql_database.html.markdown b/website/docs/r/postgresql_database.html.markdown index 5110e0f5..13f98a8d 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 is a superuser. + ## Import Example `postgresql_database` supports importing resources. Supposing the following From 5ad83f88e5a91d1009cbcd91742dac75d90dc8bf Mon Sep 17 00:00:00 2001 From: Cyril Gaudin Date: Thu, 29 Aug 2024 22:25:43 +0200 Subject: [PATCH 4/8] fix: missing roles drop in tests --- postgresql/resource_postgresql_database_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/postgresql/resource_postgresql_database_test.go b/postgresql/resource_postgresql_database_test.go index 081aea8b..0ac97445 100644 --- a/postgresql/resource_postgresql_database_test.go +++ b/postgresql/resource_postgresql_database_test.go @@ -291,6 +291,10 @@ func TestAccPostgresqlDatabase_AlterObjectOwnership(t *testing.T) { 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{ From 43d570ae4dfb7a33afc999a98b68320557020c5d Mon Sep 17 00:00:00 2001 From: Cyril Gaudin Date: Thu, 29 Aug 2024 22:26:49 +0200 Subject: [PATCH 5/8] Update website/docs/r/postgresql_database.html.markdown --- website/docs/r/postgresql_database.html.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/r/postgresql_database.html.markdown b/website/docs/r/postgresql_database.html.markdown index 13f98a8d..d2715288 100644 --- a/website/docs/r/postgresql_database.html.markdown +++ b/website/docs/r/postgresql_database.html.markdown @@ -89,7 +89,7 @@ resource "postgresql_database" "my_db" { 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 is a superuser. + the username in the provider must be superuser. ## Import Example From cf1c0ce6c241e1df430c1a38fb10b65c408d43d4 Mon Sep 17 00:00:00 2001 From: Lukas Albani Date: Fri, 30 Aug 2024 14:25:09 +0200 Subject: [PATCH 6/8] Only execute sql queries if currentOwner does not equal newOwner --- postgresql/resource_postgresql_database.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/postgresql/resource_postgresql_database.go b/postgresql/resource_postgresql_database.go index e7128bcc..1ddb2416 100644 --- a/postgresql/resource_postgresql_database.go +++ b/postgresql/resource_postgresql_database.go @@ -514,6 +514,12 @@ func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { 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 @@ -523,9 +529,6 @@ func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { _, err = revokeRoleMembership(db, currentOwner, currentUser) }() } - - newOwner := d.Get(dbOwnerAttr).(string) - 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) From 0d380aa12a12728716899d84434a103c98944895 Mon Sep 17 00:00:00 2001 From: Lukas Albani Date: Tue, 3 Sep 2024 08:36:41 +0200 Subject: [PATCH 7/8] Reverse if-statements to reduce indentation --- postgresql/resource_postgresql_database.go | 75 +++++++++++----------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/postgresql/resource_postgresql_database.go b/postgresql/resource_postgresql_database.go index 1ddb2416..ffe53510 100644 --- a/postgresql/resource_postgresql_database.go +++ b/postgresql/resource_postgresql_database.go @@ -489,53 +489,52 @@ func setDBOwner(db *DBConnection, d *schema.ResourceData) error { } func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { - if d.HasChange(dbOwnerAttr) || d.HasChange(dbAlterObjectOwnership) { - owner := d.Get(dbOwnerAttr).(string) - if owner == "" { - return nil - } - - alterOwnership := d.Get(dbAlterObjectOwnership).(bool) - if !alterOwnership { - return nil - } - currentUser := db.client.config.getDatabaseUsername() + if !d.HasChange(dbOwnerAttr) && !d.HasChange(dbAlterObjectOwnership) { + return nil + } + owner := d.Get(dbOwnerAttr).(string) + if owner == "" { + return nil + } - dbName := d.Get(dbNameAttr).(string) + alterOwnership := d.Get(dbAlterObjectOwnership).(bool) + if !alterOwnership { + return nil + } + currentUser := db.client.config.getDatabaseUsername() - lockTxn, err := startTransaction(db.client, dbName) - if err := pgLockRole(lockTxn, currentUser); err != nil { - return err - } - defer lockTxn.Commit() + dbName := d.Get(dbNameAttr).(string) - currentOwner, err := getDatabaseOwner(db, dbName) - if err != nil { - return fmt.Errorf("Error getting current database OWNER: %w", err) - } + lockTxn, err := startTransaction(db.client, dbName) + if err := pgLockRole(lockTxn, currentUser); err != nil { + return err + } + defer lockTxn.Commit() - newOwner := d.Get(dbOwnerAttr).(string) + currentOwner, err := getDatabaseOwner(db, dbName) + if err != nil { + return fmt.Errorf("Error getting current database OWNER: %w", err) + } - if currentOwner == newOwner { - return nil - } + newOwner := d.Get(dbOwnerAttr).(string) - 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 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) + } return nil } From 7936df260573d0bfc5292e8956592c3b56a88211 Mon Sep 17 00:00:00 2001 From: Cyril Gaudin Date: Sun, 8 Sep 2024 18:00:54 +0200 Subject: [PATCH 8/8] fix lock transaction --- postgresql/resource_postgresql_database.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/postgresql/resource_postgresql_database.go b/postgresql/resource_postgresql_database.go index ffe53510..6cabadf7 100644 --- a/postgresql/resource_postgresql_database.go +++ b/postgresql/resource_postgresql_database.go @@ -509,7 +509,7 @@ func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { if err := pgLockRole(lockTxn, currentUser); err != nil { return err } - defer lockTxn.Commit() + defer deferredRollback(lockTxn) currentOwner, err := getDatabaseOwner(db, dbName) if err != nil { @@ -535,6 +535,10 @@ func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { 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 }