Skip to content

Commit

Permalink
fix: ALL implicit privileges equality check (#339)
Browse files Browse the repository at this point in the history
When using resources `postgresql_default_privileges` or
`postgresql_grant` you have the option to define a fine grained set of
privileges (for example only `CONNECT` for object_type `database`) or
all at once, either by setting all privileges (for database: `["CREATE",
"CONNECT", "TEMPORARY"]` or simply `["ALL"]`.

The latter one is quite handy, since you do not have to find out which
privileges are part of it. When using `ALL`, the provider will grant the
privileges by using the set of the actual grants, so in this scenario by
granting `"CREATE", "CONNECT", "TEMPORARY"` and not `ALL`. Probably
because `ALL` is not known to postgres. 🤷🏼‍♂️

Anyways, when executing a `terraform plan` the next time after the apply
for `ALL`, the diff will find out, that not `ALL` is set as privilege,
but `"CREATE", "CONNECT", "TEMPORARY"`. Since these privileges are the
same or have the same semantic to it, there shouldn't be really a update
each time you use terraform, because the state on the DB is totally fine
and according to the resource.

To prevent this unnecessary modification, I implemented some more
detailed equality check for all `object_type`s except `column`.

I tested the changes locally with a dockerized postgres and also wrote
some unit tests for it.

Would love to hear some feedback about it, especially since it's my
first time working on a terraform provider..

Cheers


fix #32

---------

Co-authored-by: Cyril Gaudin <[email protected]>
Co-authored-by: Cyril Gaudin <[email protected]>
3 people authored Oct 24, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 6d26b59 commit fc5c40d
Showing 5 changed files with 203 additions and 12 deletions.
24 changes: 24 additions & 0 deletions postgresql/helpers.go
Original file line number Diff line number Diff line change
@@ -281,6 +281,30 @@ func validatePrivileges(d *schema.ResourceData) error {
return nil
}

func resourcePrivilegesEqual(granted *schema.Set, d *schema.ResourceData) bool {
objectType := d.Get("object_type").(string)
wanted := d.Get("privileges").(*schema.Set)

if granted.Equal(wanted) {
return true
}

if !wanted.Contains("ALL") {
return false
}

// implicit check: e.g. for object_type schema -> ALL == ["CREATE", "USAGE"]
log.Printf("The wanted privilege is 'ALL'. therefore, we will check if the current privileges are ALL implicitely")
implicits := []interface{}{}
for _, p := range allowedPrivileges[objectType] {
if p != "ALL" {
implicits = append(implicits, p)
}
}
wantedSet := schema.NewSet(schema.HashString, implicits)
return granted.Equal(wantedSet)
}

func pgArrayToSet(arr pq.ByteaArray) *schema.Set {
s := make([]interface{}, len(arr))
for i, v := range arr {
76 changes: 76 additions & 0 deletions postgresql/helpers_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package postgresql

import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"testing"

"github.com/stretchr/testify/assert"
@@ -45,3 +46,78 @@ func TestQuoteTableName(t *testing.T) {
})
}
}

func TestArePrivilegesEqual(t *testing.T) {

type PrivilegesTestObject struct {
d *schema.ResourceData
granted *schema.Set
wanted *schema.Set
assertion bool
}

tt := []PrivilegesTestObject{
{
buildResourceData("database", t),
buildPrivilegesSet("CONNECT", "CREATE", "TEMPORARY"),
buildPrivilegesSet("ALL"),
true,
},
{
buildResourceData("database", t),
buildPrivilegesSet("CREATE", "USAGE"),
buildPrivilegesSet("USAGE"),
false,
},
{
buildResourceData("table", t),
buildPrivilegesSet("SELECT", "INSERT", "UPDATE", "DELETE", "TRUNCATE", "REFERENCES", "TRIGGER"),
buildPrivilegesSet("ALL"),
true,
},
{
buildResourceData("table", t),
buildPrivilegesSet("SELECT"),
buildPrivilegesSet("SELECT, INSERT"),
false,
},
{
buildResourceData("schema", t),
buildPrivilegesSet("CREATE", "USAGE"),
buildPrivilegesSet("ALL"),
true,
},
{
buildResourceData("schema", t),
buildPrivilegesSet("CREATE"),
buildPrivilegesSet("ALL"),
false,
},
}

for _, configuration := range tt {
err := configuration.d.Set("privileges", configuration.wanted)
assert.NoError(t, err)
equal := resourcePrivilegesEqual(configuration.granted, configuration.d)
assert.Equal(t, configuration.assertion, equal)
}
}

func buildPrivilegesSet(grants ...interface{}) *schema.Set {
return schema.NewSet(schema.HashString, grants)
}

func buildResourceData(objectType string, t *testing.T) *schema.ResourceData {
var testSchema = map[string]*schema.Schema{
"object_type": {Type: schema.TypeString},
"privileges": {
Type: schema.TypeSet,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
}

m := make(map[string]any)
m["object_type"] = objectType
return schema.TestResourceDataRaw(t, testSchema, m)
}
6 changes: 5 additions & 1 deletion postgresql/resource_postgresql_default_privileges.go
Original file line number Diff line number Diff line change
@@ -268,7 +268,11 @@ func readRoleDefaultPrivileges(txn *sql.Tx, d *schema.ResourceData) error {
}

privilegesSet := pgArrayToSet(privileges)
d.Set("privileges", privilegesSet)
privilegesEqual := resourcePrivilegesEqual(privilegesSet, d)

if !privilegesEqual {
d.Set("privileges", privilegesSet)
}
d.SetId(generateDefaultPrivilegesID(d))

return nil
32 changes: 21 additions & 11 deletions postgresql/resource_postgresql_grant.go
Original file line number Diff line number Diff line change
@@ -262,7 +262,7 @@ func resourcePostgreSQLGrantDelete(db *DBConnection, d *schema.ResourceData) err
return nil
}

func readDatabaseRolePriviges(txn *sql.Tx, d *schema.ResourceData, roleOID uint32) error {
func readDatabaseRolePrivileges(txn *sql.Tx, d *schema.ResourceData, roleOID uint32) error {
dbName := d.Get("database").(string)
query := `
SELECT array_agg(privilege_type)
@@ -276,12 +276,14 @@ WHERE grantee = $2
if err := txn.QueryRow(query, dbName, roleOID).Scan(&privileges); err != nil {
return fmt.Errorf("could not read privileges for database %s: %w", dbName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
if !resourcePrivilegesEqual(granted, d) {
return d.Set("privileges", granted)
}
return nil
}

func readSchemaRolePriviges(txn *sql.Tx, d *schema.ResourceData, roleOID uint32) error {
func readSchemaRolePrivileges(txn *sql.Tx, d *schema.ResourceData, roleOID uint32) error {
dbName := d.Get("schema").(string)
query := `
SELECT array_agg(privilege_type)
@@ -296,7 +298,10 @@ WHERE grantee = $2
return fmt.Errorf("could not read privileges for schema %s: %w", dbName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
if !resourcePrivilegesEqual(granted, d) {
return d.Set("privileges", granted)
}
return nil
}

@@ -316,7 +321,10 @@ WHERE grantee = $2
return fmt.Errorf("could not read privileges for foreign data wrapper %s: %w", fdwName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
if !resourcePrivilegesEqual(granted, d) {
return d.Set("privileges", granted)
}
return nil
}

@@ -336,7 +344,10 @@ WHERE grantee = $2
return fmt.Errorf("could not read privileges for foreign server %s: %w", srvName, err)
}

d.Set("privileges", pgArrayToSet(privileges))
granted := pgArrayToSet(privileges)
if !resourcePrivilegesEqual(granted, d) {
return d.Set("privileges", granted)
}
return nil
}

@@ -433,10 +444,10 @@ func readRolePrivileges(txn *sql.Tx, d *schema.ResourceData) error {

switch objectType {
case "database":
return readDatabaseRolePriviges(txn, d, roleOID)
return readDatabaseRolePrivileges(txn, d, roleOID)

case "schema":
return readSchemaRolePriviges(txn, d, roleOID)
return readSchemaRolePrivileges(txn, d, roleOID)

case "foreign_data_wrapper":
return readForeignDataWrapperRolePrivileges(txn, d, roleOID)
@@ -509,8 +520,7 @@ GROUP BY pg_class.relname
}

privilegesSet := pgArrayToSet(privileges)

if !privilegesSet.Equal(d.Get("privileges").(*schema.Set)) {
if !resourcePrivilegesEqual(privilegesSet, d) {
// If any object doesn't have the same privileges as saved in the state,
// we return its privileges to force an update.
log.Printf(
77 changes: 77 additions & 0 deletions postgresql/resource_postgresql_grant_test.go
Original file line number Diff line number Diff line change
@@ -1139,6 +1139,83 @@ resource "postgresql_grant" "test" {
})
}

func TestAccPostgresqlImplicitGrants(t *testing.T) {
skipIfNotAcc(t)

dbSuffix, teardown := setupTestDatabase(t, true, true)
defer teardown()

testTables := []string{"test_schema.test_table"}
createTestTables(t, dbSuffix, testTables, "")

dbName, roleName := getTestDBNames(dbSuffix)

// create a TF config with placeholder for privileges
// it will be filled in each step.
var testGrant = fmt.Sprintf(`
resource "postgresql_grant" "test" {
database = "%s"
role = "%s"
schema = "test_schema"
object_type = "table"
objects = ["test_table"]
privileges = %%s
}
`, dbName, roleName)

var testCheckTableGrants = func(grants ...string) resource.TestCheckFunc {
return func(*terraform.State) error {
return testCheckTablesPrivileges(t, dbName, roleName, []string{testTables[0]}, grants)
}
}
resource.Test(t, resource.TestCase{
PreCheck: func() {
testAccPreCheck(t)
testCheckCompatibleVersion(t, featurePrivileges)
},
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: fmt.Sprintf(testGrant, `["ALL"]`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"postgresql_grant.test", "id", fmt.Sprintf("%s_%s_test_schema_table_test_table", roleName, dbName),
),
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.#", "1"),
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.0", "test_table"),
testCheckTableGrants("SELECT", "INSERT", "UPDATE", "DELETE"),
),
},
{
Config: fmt.Sprintf(testGrant, `["SELECT"]`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.#", "1"),
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.0", "test_table"),
testCheckTableGrants("SELECT"),
),
},
{
// Empty list means that privileges will be applied on all tables.
Config: fmt.Sprintf(testGrant, `["SELECT", "INSERT", "UPDATE", "DELETE"]`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.#", "1"),
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.0", "test_table"),
testCheckTableGrants("SELECT", "INSERT", "UPDATE", "DELETE"),
),
},
{
Config: fmt.Sprintf(testGrant, `[]`),
Destroy: true,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.#", "1"),
resource.TestCheckResourceAttr("postgresql_grant.test", "objects.0", "test_table"),
testCheckTableGrants(""),
),
},
},
})
}

func TestAccPostgresqlGrantSchema(t *testing.T) {
// create a TF config with placeholder for privileges
// it will be filled in each step.

0 comments on commit fc5c40d

Please sign in to comment.