Resource: postgresql_schema privilege support

[sean-]

Updates to the PostgreSQL provider.

• Update postgresql_role to include the skip_drop_role and skip_reassign_owned attributes, which are required when deleting a PostgreSQL database where a ROLE is used across multiple database instances within a single PostgreSQL cluster.
• Update the postgresql_schema resource to include a policy block that allows for setting permissions on a per-schema basis to zero or more roles (one per block).

Note: due to the way that locking is handled within PostgreSQL, it is required to run these tests with a parallelism of 1 because concurrent access for updating permissions results in error messages about concurrent updates to system catalogs.

[sean-]

@stack72 e.g. This is what should be required, but only on destroy. ?

[apparentlymart]

@sean- The dependency edges in the DAG are dealt with in the opposite order during the destroy phase, so if a postgresql_schema_policy depends on a postgresql_role then the latter will precede the former on create, but the former will be destroyed before the latter on destroy.

This means we don't need to explicitly draw the opposing dependency to deal with the destroy case, and so the need for a cycle is avoided. As long as all of the relevant objects were created by Terraform, Terraform should be able to unwind them all in an appropriate order automatically.

A tricky situation is if a role and a policy are created by two separate Terraform configs or the role policy was created manually outside of Terraform. In that case Terraform can't know it needs to delete the policy first because it doesn't know the policy exists. But this is generally desirable behavior because Terraform does its best to guarantee that it will never interfere with a resource it didn't create.

It seems like you've already been working with @stack72 on this, so I'll leave him to do a full review but I hope the above info is helpful in answering your question about how to deal with the destroy process.

[sean-]

@apparentlymart Yup, understood. The problem I'm trying to figure out is how to deal with a quirk in PostgreSQL where the create/destroy order of objects is asymmetric. By that I mean:

postgres@grit=# CREATE SCHEMA foo;
CREATE SCHEMA
postgres@grit=# CREATE ROLE bar;
CREATE ROLE
postgres@grit=# GRANT USAGE ON SCHEMA foo TO bar;
GRANT
postgres@grit=# DROP ROLE bar;
ERROR:  2BP01: role "bar" cannot be dropped because some objects depend on it
DETAIL:  privileges for schema foo
LOCATION:  DropRole, user.c:1045

Which in Terraform is written out like:

resource "postgresql_schema" "foo" {
  name = "foo"
}

resource "postgresql_role" "bar" {
  name = "bar"
}

resource "postgresql_schema_policy" "foo" {
  create = true

  schema = "${postgresql_schema.foo.name}"
  role = "${postgresql_role.bar.name}"
}

I've been chewing on this a bit and started out thinking I wanted a postgresql_schema_policy resource that would be defined like:

resource "postgresql_schema_policy" "foo_allow_res" {
  create_with_grant = true
  usage_with_grant = true

  schema = "${postgresql_schema.foo_res.name}"
  role = "${postgresql_role.app1_res.name}"
}

Which creates a DAG roughly like: database -> schema -> role -> schema_policy, which is how you would normally construct a database. It's possible to preserve the correct ordering of a DAG using something like:

resource "postgresql_schema" "foo" {
  name = "foo"
  owner = "${postgresql_role.dba.name}"
  policies = [
    "${postgresql_schema_policy.foo_allow}",
    "${postgresql_schema_policy.foo_deny}",
  ]
}

resource "postgresql_schema_policy" "foo_allow" {
  create_with_grant = true
  usage_with_grant = true

  role = "${postgresql_role.app1.name}"
}

resource "postgresql_schema_policy" "foo_deny" {
  role = "${postgresql_role.app2.name}"
}

But I don't like the aesthetics of that. What I'm going to do instead, and this resolves my own initial question above, is to move forward toward the following:

resource "postgresql_schema" "foo" {
  name = "foo"
  owner = "${postgresql_role.dba.name}"

  policy {
    role = "${postgresql_role.app1.name}"
    create = true
    usage_with_grant = true
  }

  policy {
    roles = ["${postgresql_role.app2.name}"]
    usage = true
  }
}

Which I like a lot more.

[sean-]

There's another quirk with PostgreSQL and Terraform: PostgreSQL has its own dependency graph and resolves things just fine on its own! Said differently in Terraform-speak:

resource "postgresql_role" "myrole" {
  name = "myrole"
}

resource "postgresql_schema" "test_schema" {
  name = "test_schema"

  policy {
    create = true
    usage = true
    role = "${postgresql_role.myrole.name}"
  }

When you go to drop the psotgresql_schema via TF, because myrole is dropped first, the revocation of the GRANTs fails:

LOG:  statement: DROP ROLE "all_with_grantdrop"
LOG:  statement: REVOKE CREATE ON SCHEMA "test_schema" FROM "myrole"
ERROR:  role "myrole" does not exist

Which is correct, but irritating from Terraform's perspective because you have no way of knowing that myrole no longer exists without testing first before each and every DCL. This is really only a problem when the PostgreSQL ROLE has been removed first, which in theory shouldn't happen too often. I've added a guard to prevent this from happening by querying the DB to test and see if the ROLE exists or not, but I wish there was something more elegant that was possible since this check will run for every role. Not the end of the world.

[sean-]

@stack72 whenever you're back and have time. The only thing that I'm not happy about with this is the need to individually spell out the policies, one per role. For instance:

resource "postgresql_schema" "test_schema" {
  name = "test_schema"

  policy {
    create = true
    usage = true
    role = "${postgresql_role.myrole.name}"
  }
}

What I was originally aiming to do was:

resource "postgresql_schema" "test_schema" {
  name = "test_schema"

  policy {
    create = true
    usage = true
    roles = ["${postgresql_role.myrole.name}"]
  }
}

so that an array of roles could receive the same target permission. Internally this was very problematic because PostgreSQL stores this per-policy. What I attempted to do was flatten the input of a list of roles into synthesized list of policies, however there isn't a delete method that allows for easy rewriting of the resource data. Anyway, for another day. Regardless, PTAL. Thanks in advance!

[jen20]

Which is correct, but irritating from Terraform's perspective because you have no way of knowing that myrole no longer exists without testing first before each and every DCL

@sean- This is a fairly common case where things which are logically subresources are modelled as top level resources. It is also needed to deal with divergence caused by operator intervention - there is a specific operation for Exists which can be used on refresh for this.

[jen20]

Few minor changes, but otherwise LGTM! Thanks @sean-!

[jen20]

We should switch all of these to use the err wrap.Wrapf pattern as we have been rolling through the rest of the code base.

[sean-]

There were only two call sites where fmt.Errorf() was still being used. Fixed the remaining two sites.

[jen20]

s/who/that/ or s/who/which/? "Who" seems unusual for a non-person.

[jen20]

s/a// or s/objects/object

[jen20]

We usually give the command line to import rather than the configuration code for import examples.

[sean-]

I started there but added the examples because the import without a config is opaque to new users who want to move to TF. I can remove the import sample if you'd like but think it's a net win for users for the time being. Happy to go either way, however.

[jen20]

I think we at least need the import command line as well as the example code.

[jen20]

LGTM, with exception of last comment on import documentation.

[jen20]

I think we at least need the import command line as well as the example code.

[ghost]

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.