From 070e6eb135649966763c56ca77ad0f300023b7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arthur=20Woimb=C3=A9e?= Date: Wed, 5 Feb 2025 10:35:15 +0100 Subject: [PATCH 1/2] original work from @kylejohnson --- examples/issues/178/locals.tf | 4 + examples/issues/178/main.tf | 161 +++++++++++++++++++++++++++++++ examples/issues/178/test.sh | 10 ++ examples/issues/178/variables.tf | 67 +++++++++++++ postgresql/helpers.go | 66 +++---------- 5 files changed, 254 insertions(+), 54 deletions(-) create mode 100644 examples/issues/178/locals.tf create mode 100644 examples/issues/178/main.tf create mode 100755 examples/issues/178/test.sh create mode 100644 examples/issues/178/variables.tf diff --git a/examples/issues/178/locals.tf b/examples/issues/178/locals.tf new file mode 100644 index 00000000..21c56fbc --- /dev/null +++ b/examples/issues/178/locals.tf @@ -0,0 +1,4 @@ +locals { + read_only_users = ["fu", "bar"] + read_write_users = ["blah", "bzz"] +} diff --git a/examples/issues/178/main.tf b/examples/issues/178/main.tf new file mode 100644 index 00000000..0579bb22 --- /dev/null +++ b/examples/issues/178/main.tf @@ -0,0 +1,161 @@ +terraform { + required_providers { + docker = { + source = "kreuzwerker/docker" + version = ">= 3.0.2" + } + postgresql = { + source = "cyrilgdn/postgresql" + version = "1.21" + } + # postgresql = { + # source = "terraform-fu.bar/terraform-provider-postgresql/postgresql" + # version = ">= 1.20" + # } + } +} + +provider "docker" { + host = "unix:///var/run/docker.sock" +} + +resource "docker_image" "postgres" { + name = var.postgres_image + keep_locally = var.keep_image +} + +resource "docker_container" "postgres" { + image = docker_image.postgres.image_id + name = var.container_name + wait = true + ports { + internal = var.POSTGRES_PORT + external = var.POSTGRES_PORT + } + env = [ + "POSTGRES_PASSWORD=${var.POSTGRES_PASSWORD}" + ] + healthcheck { + test = ["CMD-SHELL", "pg_isready"] + interval = "5s" + timeout = "5s" + retries = 5 + start_period = "2s" + } +} + +provider "postgresql" { + scheme = "postgres" + host = var.POSTGRES_HOST + port = docker_container.postgres.ports[0].external + database = var.POSTGRES_PASSWORD + username = var.POSTGRES_PASSWORD + password = var.POSTGRES_PASSWORD + sslmode = "disable" + superuser = var.superuser +} + +resource "postgresql_database" "this" { + name = var.database_name + template = var.database_template + owner = var.POSTGRES_USER +} + +resource "postgresql_role" "readonly_role" { + name = "readonly" + login = false + superuser = false + create_database = false + create_role = false + inherit = false + replication = false + connection_limit = -1 +} + +resource "postgresql_role" "readwrite_role" { + name = "readwrite" + login = false + superuser = false + create_database = false + create_role = false + inherit = false + replication = false + connection_limit = -1 +} + +resource "postgresql_grant" "readonly_role" { + database = postgresql_database.this.name + role = postgresql_role.readonly_role.name + object_type = "table" + schema = "public" + privileges = ["SELECT"] + with_grant_option = false +} + +resource "postgresql_grant" "readwrite_role" { + database = postgresql_database.this.name + role = postgresql_role.readwrite_role.name + object_type = "table" + schema = "public" + privileges = ["SELECT", "INSERT", "UPDATE", "DELETE"] + with_grant_option = false +} + +resource "postgresql_role" "readonly_users" { + for_each = toset(local.read_only_users) + name = each.key + roles = [postgresql_role.readonly_role.name] + login = true + superuser = false + create_database = false + create_role = false + inherit = true + replication = false + connection_limit = -1 +} + +resource "postgresql_role" "readwrite_users" { + for_each = toset(local.read_write_users) + name = each.key + roles = [postgresql_role.readonly_role.name] + login = true + superuser = false + create_database = false + create_role = false + inherit = true + replication = false + connection_limit = -1 +} + +resource "postgresql_grant" "connect_db_readonly_role" { + database = postgresql_database.this.name + object_type = "database" + privileges = ["CREATE", "CONNECT"] + role = postgresql_role.readonly_role.name +} + +resource "postgresql_grant" "connect_db_readwrite_role" { + database = postgresql_database.this.name + object_type = "database" + privileges = ["CREATE", "CONNECT"] + role = postgresql_role.readwrite_role.name +} + +resource "postgresql_grant" "usage_readonly_role" { + database = postgresql_database.this.name + role = postgresql_role.readonly_role.name + object_type = "schema" + schema = "public" + privileges = ["USAGE"] + with_grant_option = false +} + +resource "postgresql_grant" "usage_readwrite_role" { + database = postgresql_database.this.name + role = postgresql_role.readwrite_role.name + object_type = "schema" + schema = "public" + privileges = ["USAGE"] + with_grant_option = false +} + diff --git a/examples/issues/178/test.sh b/examples/issues/178/test.sh new file mode 100755 index 00000000..0d341965 --- /dev/null +++ b/examples/issues/178/test.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +set -eou pipefail +i=0 + +while [ $i -lt 10 ]; do + terraform apply -auto-approve + terraform destroy -auto-approve + i=$((i+1)) +done \ No newline at end of file diff --git a/examples/issues/178/variables.tf b/examples/issues/178/variables.tf new file mode 100644 index 00000000..4bcb422c --- /dev/null +++ b/examples/issues/178/variables.tf @@ -0,0 +1,67 @@ +variable "postgres_image" { + description = "Which postgres docker image to use." + default = "postgres:15" + type = string + sensitive = false +} + +variable "POSTGRES_USER" { + default = "postgres" + type = string + sensitive = false +} + +variable "POSTGRES_PASSWORD" { + description = "Password for docker POSTGRES_USER" + default = "postgres" + type = string + sensitive = false +} + +variable "POSTGRES_HOST" { + default = "127.0.0.1" + type = string + sensitive = false +} + +variable "POSTGRES_PORT" { + description = "Which port postgres should listen on." + default = 5432 + type = number + sensitive = false +} + +variable "keep_image" { + description = "If true, then the Docker image won't be deleted on destroy operation. If this is false, it will delete the image from the docker local storage on destroy operation." + default = true + type = bool + sensitive = false +} + +variable "database_name" { + description = "Name for the database to be created." + default = "issue178" + type = string + sensitive = false +} + +variable "database_template" { + description = "The name of the template database from which to create the database." + default = "template0" + type = string + sensitive = false +} + +variable "superuser" { + description = "Whether the POSTGRES_USER is a PostgreSQL superuser." + default = false + type = bool + sensitive = false +} + +variable "container_name" { + description = "The name for the docker container." + default = "postgres" + type = string + sensitive = false +} diff --git a/postgresql/helpers.go b/postgresql/helpers.go index 54e462d9..94dea63f 100644 --- a/postgresql/helpers.go +++ b/postgresql/helpers.go @@ -56,25 +56,11 @@ func pqQuoteLiteral(in string) string { func isMemberOfRole(db QueryAble, role, member string) (bool, error) { var _rez int - setOption := true - err := db.QueryRow( - "SELECT 1 FROM information_schema.columns WHERE table_name='pg_auth_members' AND column_name = 'set_option'", + "SELECT 1 FROM pg_auth_members WHERE pg_get_userbyid(roleid) = $1 AND pg_get_userbyid(member) = $2", + role, member, ).Scan(&_rez) - switch { - case err == sql.ErrNoRows: - setOption = false - case err != nil: - return false, fmt.Errorf("could not read setOption column: %w", err) - } - - query := "SELECT 1 FROM pg_auth_members WHERE pg_get_userbyid(roleid) = $1 AND pg_get_userbyid(member) = $2" - if setOption { - query += " AND set_option" - } - - err = db.QueryRow(query, role, member).Scan(&_rez) switch { case err == sql.ErrNoRows: return false, nil @@ -179,7 +165,7 @@ func withRolesGranted(txn *sql.Tx, roles []string, fn func() error) error { // in order to manipulate its objects/privileges. // But PostgreSQL prevents `foo` to be a member of the role `postgres`, // and for `postgres` to be a member of the role `foo`, at the same time. - // In this case we will temporarily revoke this privilege. + // In this case we will temporary revoke this privilege. // So, the following queries will happen (in the same transaction): // - REVOKE postgres FROM foo // - GRANT foo TO postgres @@ -281,30 +267,6 @@ 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 implicitly") - 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 { @@ -586,6 +548,15 @@ func pgLockRole(txn *sql.Tx, role string) error { return nil } +// Lock a schema avoid concurrent updates during revoke query. +func pgLockSchema(txn *sql.Tx, schema string) error { + if _, err := txn.Exec("SELECT pg_advisory_xact_lock(oid::bigint) FROM pg_catalog.pg_namespace WHERE nspname = $1", schema); err != nil { + return fmt.Errorf("could not get advisory lock for schema %s: %w", schema, err) + } + + return nil +} + // Lock a database and all his members to avoid concurrent updates on some resources func pgLockDatabase(txn *sql.Tx, database string) error { // Disable statement timeout for this connection otherwise the lock could fail @@ -644,16 +615,3 @@ func findStringSubmatchMap(expression string, text string) map[string]string { func defaultDiffSuppressFunc(k, old, new string, d *schema.ResourceData) bool { return old == new } - -// quoteTable can quote a table name with or without a schema prefix -// Example: -// -// my_table -> "my_table" -// public.my_table -> "public"."my_table" -func quoteTableName(tableName string) string { - parts := strings.Split(tableName, ".") - for i := range parts { - parts[i] = pq.QuoteIdentifier(parts[i]) - } - return strings.Join(parts, ".") -} From 0ebb82774349b31bd8d5be5b2727b065b7caa61c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arthur=20Woimb=C3=A9e?= Date: Wed, 5 Feb 2025 10:43:48 +0100 Subject: [PATCH 2/2] fixes & merge with main --- examples/issues/178/main.tf | 5 -- postgresql/helpers.go | 62 +++++++++++++++++++++++-- postgresql/resource_postgresql_grant.go | 4 ++ 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/examples/issues/178/main.tf b/examples/issues/178/main.tf index 0579bb22..79f21220 100644 --- a/examples/issues/178/main.tf +++ b/examples/issues/178/main.tf @@ -8,10 +8,6 @@ terraform { source = "cyrilgdn/postgresql" version = "1.21" } - # postgresql = { - # source = "terraform-fu.bar/terraform-provider-postgresql/postgresql" - # version = ">= 1.20" - # } } } @@ -158,4 +154,3 @@ resource "postgresql_grant" "usage_readwrite_role" { privileges = ["USAGE"] with_grant_option = false } - diff --git a/postgresql/helpers.go b/postgresql/helpers.go index 94dea63f..c7967748 100644 --- a/postgresql/helpers.go +++ b/postgresql/helpers.go @@ -56,11 +56,25 @@ func pqQuoteLiteral(in string) string { func isMemberOfRole(db QueryAble, role, member string) (bool, error) { var _rez int + setOption := true + err := db.QueryRow( - "SELECT 1 FROM pg_auth_members WHERE pg_get_userbyid(roleid) = $1 AND pg_get_userbyid(member) = $2", - role, member, + "SELECT 1 FROM information_schema.columns WHERE table_name='pg_auth_members' AND column_name = 'set_option'", ).Scan(&_rez) + switch { + case err == sql.ErrNoRows: + setOption = false + case err != nil: + return false, fmt.Errorf("could not read setOption column: %w", err) + } + + query := "SELECT 1 FROM pg_auth_members WHERE pg_get_userbyid(roleid) = $1 AND pg_get_userbyid(member) = $2" + if setOption { + query += " AND set_option" + } + + err = db.QueryRow(query, role, member).Scan(&_rez) switch { case err == sql.ErrNoRows: return false, nil @@ -165,7 +179,7 @@ func withRolesGranted(txn *sql.Tx, roles []string, fn func() error) error { // in order to manipulate its objects/privileges. // But PostgreSQL prevents `foo` to be a member of the role `postgres`, // and for `postgres` to be a member of the role `foo`, at the same time. - // In this case we will temporary revoke this privilege. + // In this case we will temporarily revoke this privilege. // So, the following queries will happen (in the same transaction): // - REVOKE postgres FROM foo // - GRANT foo TO postgres @@ -267,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 implicitly") + 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 { @@ -548,8 +586,11 @@ func pgLockRole(txn *sql.Tx, role string) error { return nil } -// Lock a schema avoid concurrent updates during revoke query. +// Lock a schema to avoid concurrent updates func pgLockSchema(txn *sql.Tx, schema string) error { + if _, err := txn.Exec("SET statement_timeout = 0"); err != nil { + return fmt.Errorf("could not disable statement_timeout: %w", err) + } if _, err := txn.Exec("SELECT pg_advisory_xact_lock(oid::bigint) FROM pg_catalog.pg_namespace WHERE nspname = $1", schema); err != nil { return fmt.Errorf("could not get advisory lock for schema %s: %w", schema, err) } @@ -615,3 +656,16 @@ func findStringSubmatchMap(expression string, text string) map[string]string { func defaultDiffSuppressFunc(k, old, new string, d *schema.ResourceData) bool { return old == new } + +// quoteTable can quote a table name with or without a schema prefix +// Example: +// +// my_table -> "my_table" +// public.my_table -> "public"."my_table" +func quoteTableName(tableName string) string { + parts := strings.Split(tableName, ".") + for i := range parts { + parts[i] = pq.QuoteIdentifier(parts[i]) + } + return strings.Join(parts, ".") +} diff --git a/postgresql/resource_postgresql_grant.go b/postgresql/resource_postgresql_grant.go index fb93c479..d347c771 100644 --- a/postgresql/resource_postgresql_grant.go +++ b/postgresql/resource_postgresql_grant.go @@ -184,6 +184,10 @@ func resourcePostgreSQLGrantCreateOrUpdate(db *DBConnection, d *schema.ResourceD if err := pgLockDatabase(txn, database); err != nil { return err } + } else if objectType == "schema" { + if err := pgLockSchema(txn, database); err != nil { + return err + } } owners, err := getRolesToGrant(txn, d)