PostgreSQL backend does not properly respect schema_name configuration parameter

[caseyandgina]

https://www.terraform.io/docs/language/settings/backends/pg.html#schema_name

I am using the following backend configuration:

terraform {
        backend "pg" {
                conn_str = "postgres://app_terraform:<password>@<host>/dba?sslmode=disable"
                schema_name = "terraform"
                skip_schema_creation = true
        }
}

I have created the schema as the database owner, using:

create schema "terraform" authorization "app_terraform";

...which sets the app_terraform user as the owner of the terraform schema, so that it can create whatever it wants in it. However, when I terraform init, I get the following error:

│ Error: pq: permission denied for schema public

Looking in the PostgreSQL log, this is because Terraform is explicitly trying to create a sequence in the public schema instead, which I do not permit:

2021-06-25 00:09:35.745 UTC,"app_terraform","dba",365766,"10.124.250.228:54729",60d51ebf.594c6,3,"CREATE SEQUENCE",2021-06-25 00:09:35 UTC,5/54951,0,ERROR,42501,"permission denied for schema public",,,,,,"CREATE SEQUENCE IF NOT EXISTS public.global_states_id_seq AS bigint",,,"","client backend"

It should use the schema_name specified in the backend configuration instead of "public" in the above statement. It's worth noting that if it is able to create the sequence in the public schema, it goes on to create the table that depends on the sequence in the schema specified in the configuration.

Also, the following from the documentation indicates a misunderstanding of how permissions work in PostgreSQL:

skip_index_creation - If set to true, the Postgres index must already exist. Terraform won't try to create the index. Useful when the Postgres user does not have "create index" permission on the database.

One cannot separately grant/revoke create table and create index. Either the user is able to create (anything) in the target schema, or it is not. Also, looking at the table that Terraform creates, there is no index aside from the ones required by the table's primary key and unique constraint definition. The table will allow duplicate name values to exist if the unique index is not added. So I believe this should be combined with the skip_table_creation configuration parameter.

Terraform attempts execute the following SQL statements:

CREATE SEQUENCE IF NOT EXISTS public.global_states_id_seq AS bigint;
CREATE TABLE IF NOT EXISTS "<schema_name>".states (
                        id bigint NOT NULL DEFAULT nextval('public.global_states_id_seq') PRIMARY KEY,
                        name text,
                        data text
                        )
CREATE UNIQUE INDEX IF NOT EXISTS states_by_name ON ""terraform"".states (name);

This can be simplified into a single statement, which makes it easier to keep everything together in one schema:

create table if not exists "<schema_name>".states (
	id bigserial not null,
	name text,
	data text,
	primary key (id),
	unique (name)
)

This will result in the sequence being named states_id_seq instead of global_states_id_seq, and the index being named states_name_key instead of states_by_name, but if the table already exists as created by the old method, it will not make any changes since a table of that name already exists.

[remilapeyre]

Hi @caseyandgina, thanks for taking the time to look into the issue thoroughly, there is multiple things that interact badly here and stems from the way we handle workspace locking and how it interacts when multiple projects store their states in the same database but in different schema.

Because the pg backend uses pg_try_advisory_lock() to lock the workspace based on its ID, locking one workspace would lock all unrelated workspaces that happen to share the same ID. This means that locking the default workspace of any project would lock all default workspaces (the pg backend has always used pg_try_advisory_lock() since the start). This was reported as an issue in #22833 and would make it difficult for multiple persons to work at the same time.

Perhaps locking a given workspace should have used SELECT FOR UPDATE and pg_try_advisory_lock() should have only been used as a global lock during workspace creation, but now that some versions of Terraform are using pg_try_advisory_lock() it is very difficult to switch: if we did then a workspace locked by Terraform 1.0 using SELECT FOR UPDATE could be concurrently changed by another user on Terraform 0.15 which uses pg_try_advisory_lock(). This is something that could result in loss of information and we must absolutely avoid it.

When faced with this issue I tried to fix the locking so that locking one project would not lock the other projects but I still needed this to be backward compatible, the reasons that made me settle on using a global sequence rather than one per schema are explained in d81d521, basically I need to continue to use pg_try_advisory_lock() so that previous versions of Terraform would see that a workspace is currently locked.

I'm not sure if it is possible to solve this issue properly without using a global sequence. I'm not sure why the pg backend is using schema to separate projects, I certainly wish that it just used SELECT FOR UPDATE to handle locking, or put all states in a single table with a column with the project name and RLS to manage the security but now that it works like this, only a backward compatible solution can be used.

Do this makes sense?

I agree that the wording regarding the skip_create_index is not accurate and will need to be updated.

Regarding the last issue you raise:

The table will allow duplicate name values to exist if the unique index is not added.

skip_create_index was added for situations where the index could not be created by the Terraform postgres user, but it is still expected to be created by a database administrator, even when skip_create_index = true. As a defensive measure for the case where the DBA forgets to create it, it's indeed a good idea to add a constraint to make sure two workspaces don't accidentally share the same name.

[jbardin]

Closed via #29157

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.