fix: ALL implicit privileges equality check (cyrilgdn#339)

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 <cyril.gaudin@gmail.com>
Co-authored-by: Cyril Gaudin <cgaudin@edgelab.ch>

Author: 3 people

postgresql/helpers.go

@@ -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 {

postgresql/helpers_test.go

@@ -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)
+}

postgresql/resource_postgresql_default_privileges.go

@@ -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

postgresql/resource_postgresql_grant.go

@@ -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(

postgresql/resource_postgresql_grant_test.go

@@ -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.