From fc5c40d25110e0780805026dd32e2972c59d3bca Mon Sep 17 00:00:00 2001 From: Tom Albrecht Date: Thu, 24 Oct 2024 17:00:48 +0200 Subject: [PATCH] fix: ALL implicit privileges equality check (#339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Cyril Gaudin --- postgresql/helpers.go | 24 ++++++ postgresql/helpers_test.go | 76 ++++++++++++++++++ .../resource_postgresql_default_privileges.go | 6 +- postgresql/resource_postgresql_grant.go | 32 +++++--- postgresql/resource_postgresql_grant_test.go | 77 +++++++++++++++++++ 5 files changed, 203 insertions(+), 12 deletions(-) diff --git a/postgresql/helpers.go b/postgresql/helpers.go index 058da49d..2eb93e88 100644 --- a/postgresql/helpers.go +++ b/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 { diff --git a/postgresql/helpers_test.go b/postgresql/helpers_test.go index 6b60d806..11538618 100644 --- a/postgresql/helpers_test.go +++ b/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) +} diff --git a/postgresql/resource_postgresql_default_privileges.go b/postgresql/resource_postgresql_default_privileges.go index 30be8314..124b3511 100644 --- a/postgresql/resource_postgresql_default_privileges.go +++ b/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 diff --git a/postgresql/resource_postgresql_grant.go b/postgresql/resource_postgresql_grant.go index 608570f6..ecdae0f3 100644 --- a/postgresql/resource_postgresql_grant.go +++ b/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( diff --git a/postgresql/resource_postgresql_grant_test.go b/postgresql/resource_postgresql_grant_test.go index 611986a0..4b62180b 100644 --- a/postgresql/resource_postgresql_grant_test.go +++ b/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.