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

postgresql_database: Reassign objects owners if database owner changes #458

Merged
83 changes: 73 additions & 10 deletions postgresql/resource_postgresql_database.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
},
},
}
}
Expand Down Expand Up @@ -393,6 +400,10 @@ func resourcePostgreSQLDatabaseUpdate(db *DBConnection, d *schema.ResourceData)
return err
}

if err := setAlterOwnership(db, d); err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

This does more than setting a database "parameter", probably this function could have better name
(something like reassignOwnership or similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I update the whole name for the flag then? I don't want to break the naming conventions of the functions called in the resourcePostgreSQLDatabaseUpdate, so I'm not sure if it might be misleading if the function name changes, but the flag itself not

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm I get the point, we can keep this function name to be consistent with the rest of the functions then 👍

return err
}

if err := setDBOwner(db, d); err != nil {
return err
}
Expand Down Expand Up @@ -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)
Expand All @@ -476,6 +488,57 @@ 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) {
Copy link
Owner

Choose a reason for hiding this comment

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

if only dbAlterObjectOwnership changes, it seems you will execute the REASSIGN with the same previous and new owner so I'm not sure to see the point on this case 🤔

At least you should check if currentOwner and newOwner are different below.

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 for the review. If updated the code so it checks if currentOwner and newOwner are equal before executing the sql statements

cyrilgdn marked this conversation as resolved.
Show resolved Hide resolved
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)
}

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

return nil
}

func setDBTablespace(db QueryAble, d *schema.ResourceData) error {
if !d.HasChange(dbTablespaceAttr) {
return nil
Expand Down
110 changes: 110 additions & 0 deletions postgresql/resource_postgresql_database_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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"),
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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" {
Expand All @@ -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" {
Expand Down
21 changes: 15 additions & 6 deletions website/docs/r/postgresql_database.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
```

Expand Down Expand Up @@ -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
Expand Down