Fix #321 replaces postgresql_grant all the time. (#476)

Fixes #321

Fix based on [doctolib's
fork](doctolib@1caee37)

version 1.19.0, with [PR
135](#135),
the postgresql_grant resource gets re-created when there is a change.
Replacing the resource is not a good idea because the "destroy/create"
operations are completely separate. i.e. they are not atomic which means
(given the example in the "Steps to Reproduce" section above) for a
short moment between the 2 operations the public role loses access to
the public schema. If for any reason Terraform fails midway or it gets
interrupted, users will end up not being able to access the objects in
the public schema. This is what happens in the PostgreSQL log:

```
2023-07-11 14:50:05.989 UTC [1673] LOG:  statement: BEGIN READ WRITE
2023-07-11 14:50:06.000 UTC [1673] LOG:  statement: REVOKE ALL PRIVILEGES ON SCHEMA "public" FROM "public"
2023-07-11 14:50:06.001 UTC [1673] LOG:  statement: COMMIT
2023-07-11 14:50:06.033 UTC [1675] LOG:  statement: BEGIN READ WRITE
2023-07-11 14:50:06.043 UTC [1675] LOG:  statement: REVOKE ALL PRIVILEGES ON SCHEMA "public" FROM "public"
2023-07-11 14:50:06.044 UTC [1675] LOG:  statement: GRANT USAGE ON SCHEMA "public" TO "public"
2023-07-11 14:50:06.045 UTC [1675] LOG:  statement: COMMIT
```

In our case we're only removing ForceNew from privileges, as this fixes
our use case, but the overall solution allows every schema to be updated
instead of recreated.

Introduced a "getter" in order to fix [PR
135](#135)
original issue that caused the introduction of "ForceNew".

> Originally, the privileges argument did not force recreation of the
resource.
This was a problem because it meant that when changing the privileges in
a grant resource, the update function would be triggered and would
receive only the new configuration. So the revocation would not revoke
the old permissions, but the new one, which is not very useful.
....
I could not find a way to fetch the privilege stored in the state, &
setting the argument to ForceNew solved this problem. So I did that.

Fetching the privilege stored in state is the job of our new getter,
this way we don't have to "ForceNew" everything.

I think we might be able to keep a single "Create" function if we
wanted, checking d.IsResourceNew() to decide if we should use the old
one or new one, but the solution from doctolib seems robust enough.

Author: PabloAzNR

postgresql/resource_postgresql_grant.go

@@ -34,12 +34,12 @@ var objectTypes = map[string]string{
 	"schema":   "n",
 }
 
+type ResourceSchemeGetter func(string) interface{}
+
 func resourcePostgreSQLGrant() *schema.Resource {
 	return &schema.Resource{
 		Create: PGResourceFunc(resourcePostgreSQLGrantCreate),
-		// Since all of this resource's arguments force a recreation
-		// there's no need for an Update function
-		// Update:
+		Update: PGResourceFunc(resourcePostgreSQLGrantUpdate),
 		Read:   PGResourceFunc(resourcePostgreSQLGrantRead),
 		Delete: PGResourceFunc(resourcePostgreSQLGrantDelete),
 
@@ -88,7 +88,6 @@ func resourcePostgreSQLGrant() *schema.Resource {
 			"privileges": {
 				Type:        schema.TypeSet,
 				Required:    true,
-				ForceNew:    true,
 				Elem:        &schema.Schema{Type: schema.TypeString},
 				Set:         schema.HashString,
 				Description: "The list of privileges to grant",
@@ -129,6 +128,14 @@ func resourcePostgreSQLGrantRead(db *DBConnection, d *schema.ResourceData) error
 }
 
 func resourcePostgreSQLGrantCreate(db *DBConnection, d *schema.ResourceData) error {
+	return resourcePostgreSQLGrantCreateOrUpdate(db, d, false)
+}
+
+func resourcePostgreSQLGrantUpdate(db *DBConnection, d *schema.ResourceData) error {
+	return resourcePostgreSQLGrantCreateOrUpdate(db, d, true)
+}
+
+func resourcePostgreSQLGrantCreateOrUpdate(db *DBConnection, d *schema.ResourceData, usePrevious bool) error {
 	if err := validateFeatureSupport(db, d); err != nil {
 		return fmt.Errorf("feature is not supported: %v", err)
 	}
@@ -187,7 +194,7 @@ func resourcePostgreSQLGrantCreate(db *DBConnection, d *schema.ResourceData) err
 		// Revoke all privileges before granting otherwise reducing privileges will not work.
 		// We just have to revoke them in the same transaction so the role will not lost its
 		// privileges between the revoke and grant statements.
-		if err := revokeRolePrivileges(txn, d); err != nil {
+		if err := revokeRolePrivileges(txn, d, usePrevious); err != nil {
 			return err
 		}
 		if err := grantRolePrivileges(txn, d); err != nil {
@@ -243,7 +250,7 @@ func resourcePostgreSQLGrantDelete(db *DBConnection, d *schema.ResourceData) err
 	}
 
 	if err := withRolesGranted(txn, owners, func() error {
-		return revokeRolePrivileges(txn, d)
+		return revokeRolePrivileges(txn, d, false)
 	}); err != nil {
 		return err
 	}
@@ -589,40 +596,40 @@ func createGrantQuery(d *schema.ResourceData, privileges []string) string {
 	return query
 }
 
-func createRevokeQuery(d *schema.ResourceData) string {
+func createRevokeQuery(getter ResourceSchemeGetter) string {
 	var query string
 
-	switch strings.ToUpper(d.Get("object_type").(string)) {
+	switch strings.ToUpper(getter("object_type").(string)) {
 	case "DATABASE":
 		query = fmt.Sprintf(
 			"REVOKE ALL PRIVILEGES ON DATABASE %s FROM %s",
-			pq.QuoteIdentifier(d.Get("database").(string)),
-			pq.QuoteIdentifier(d.Get("role").(string)),
+			pq.QuoteIdentifier(getter("database").(string)),
+			pq.QuoteIdentifier(getter("role").(string)),
 		)
 	case "SCHEMA":
 		query = fmt.Sprintf(
 			"REVOKE ALL PRIVILEGES ON SCHEMA %s FROM %s",
-			pq.QuoteIdentifier(d.Get("schema").(string)),
-			pq.QuoteIdentifier(d.Get("role").(string)),
+			pq.QuoteIdentifier(getter("schema").(string)),
+			pq.QuoteIdentifier(getter("role").(string)),
 		)
 	case "FOREIGN_DATA_WRAPPER":
-		fdwName := d.Get("objects").(*schema.Set).List()[0]
+		fdwName := getter("objects").(*schema.Set).List()[0]
 		query = fmt.Sprintf(
 			"REVOKE ALL PRIVILEGES ON FOREIGN DATA WRAPPER %s FROM %s",
 			pq.QuoteIdentifier(fdwName.(string)),
-			pq.QuoteIdentifier(d.Get("role").(string)),
+			pq.QuoteIdentifier(getter("role").(string)),
 		)
 	case "FOREIGN_SERVER":
-		srvName := d.Get("objects").(*schema.Set).List()[0]
+		srvName := getter("objects").(*schema.Set).List()[0]
 		query = fmt.Sprintf(
 			"REVOKE ALL PRIVILEGES ON FOREIGN SERVER %s FROM %s",
 			pq.QuoteIdentifier(srvName.(string)),
-			pq.QuoteIdentifier(d.Get("role").(string)),
+			pq.QuoteIdentifier(getter("role").(string)),
 		)
 	case "COLUMN":
-		objects := d.Get("objects").(*schema.Set)
-		columns := d.Get("columns").(*schema.Set)
-		privileges := d.Get("privileges").(*schema.Set)
+		objects := getter("objects").(*schema.Set)
+		columns := getter("columns").(*schema.Set)
+		privileges := getter("privileges").(*schema.Set)
 		if privileges.Len() == 0 || columns.Len() == 0 {
 			// No privileges to revoke, so don't revoke anything
 			query = "SELECT NULL"
@@ -631,38 +638,38 @@ func createRevokeQuery(d *schema.ResourceData) string {
 				"REVOKE %s (%s) ON TABLE %s FROM %s",
 				setToPgIdentSimpleList(privileges),
 				setToPgIdentListWithoutSchema(columns),
-				setToPgIdentList(d.Get("schema").(string), objects),
-				pq.QuoteIdentifier(d.Get("role").(string)),
+				setToPgIdentList(getter("schema").(string), objects),
+				pq.QuoteIdentifier(getter("role").(string)),
 			)
 		}
 	case "TABLE", "SEQUENCE", "FUNCTION", "PROCEDURE", "ROUTINE":
-		objects := d.Get("objects").(*schema.Set)
-		privileges := d.Get("privileges").(*schema.Set)
+		objects := getter("objects").(*schema.Set)
+		privileges := getter("privileges").(*schema.Set)
 		if objects.Len() > 0 {
 			if privileges.Len() > 0 {
 				// Revoking specific privileges instead of all privileges
 				// to avoid messing with column level grants
 				query = fmt.Sprintf(
 					"REVOKE %s ON %s %s FROM %s",
 					setToPgIdentSimpleList(privileges),
-					strings.ToUpper(d.Get("object_type").(string)),
-					setToPgIdentList(d.Get("schema").(string), objects),
-					pq.QuoteIdentifier(d.Get("role").(string)),
+					strings.ToUpper(getter("object_type").(string)),
+					setToPgIdentList(getter("schema").(string), objects),
+					pq.QuoteIdentifier(getter("role").(string)),
 				)
 			} else {
 				query = fmt.Sprintf(
 					"REVOKE ALL PRIVILEGES ON %s %s FROM %s",
-					strings.ToUpper(d.Get("object_type").(string)),
-					setToPgIdentList(d.Get("schema").(string), objects),
-					pq.QuoteIdentifier(d.Get("role").(string)),
+					strings.ToUpper(getter("object_type").(string)),
+					setToPgIdentList(getter("schema").(string), objects),
+					pq.QuoteIdentifier(getter("role").(string)),
 				)
 			}
 		} else {
 			query = fmt.Sprintf(
 				"REVOKE ALL PRIVILEGES ON ALL %sS IN SCHEMA %s FROM %s",
-				strings.ToUpper(d.Get("object_type").(string)),
-				pq.QuoteIdentifier(d.Get("schema").(string)),
-				pq.QuoteIdentifier(d.Get("role").(string)),
+				strings.ToUpper(getter("object_type").(string)),
+				pq.QuoteIdentifier(getter("schema").(string)),
+				pq.QuoteIdentifier(getter("role").(string)),
 			)
 		}
 	}
@@ -687,8 +694,21 @@ func grantRolePrivileges(txn *sql.Tx, d *schema.ResourceData) error {
 	return err
 }
 
-func revokeRolePrivileges(txn *sql.Tx, d *schema.ResourceData) error {
-	query := createRevokeQuery(d)
+func revokeRolePrivileges(txn *sql.Tx, d *schema.ResourceData, usePrevious bool) error {
+	getter := d.Get
+
+	if usePrevious {
+		getter = func(name string) interface{} {
+			if d.HasChange(name) {
+				old, _ := d.GetChange(name)
+				return old
+			}
+
+			return d.Get(name)
+		}
+	}
+
+	query := createRevokeQuery(getter)
 	if len(query) == 0 {
 		// Query is empty, don't run anything
 		return nil

postgresql/resource_postgresql_grant_test.go

@@ -293,7 +293,7 @@ func TestCreateRevokeQuery(t *testing.T) {
 	}
 
 	for _, c := range cases {
-		out := createRevokeQuery(c.resource)
+		out := createRevokeQuery(c.resource.Get)
 		if out != c.expected {
 			t.Fatalf("Error matching output and expected: %#v vs %#v", out, c.expected)
 		}