New resources: postgresql_grant & postgresql_default_privileges #53

cyrilgdn · 2018-11-26T09:46:54Z

Add resources:

postgresql_grant: Allow to manage grant for tables or sequences for a specified role in a schema.
postgresql_default_privileges: Allow to manage default privileges for tables or sequences for a specified role in a schema.

sean- · 2018-12-29T19:09:28Z

@cyrilgdn this is a huge PR, thank you for submitting this.

I have one sizable structural change that I have a bias toward wanting to see, but want to ask you about it first. In your permissions blocks, you're using a slice of strings instead of a set of bools. I understand the desire to use the strings because these map to SQL DCL. In the work I'd done in the past, I have actually mapped these to booleans. Currently you'd use:

 	resource "postgresql_grant" "test_rw" {
 		database    = "%s"
 		role        = "%s"
 		schema      = "public"
 		object_type = "table"
 		privileges   = ["SELECT", "INSERT", "UPDATE"]
 	}

vs

 	resource "postgresql_grant" "test_rw" {
 		database    = "%s"
 		role        = "%s"
 		schema      = "public"
 		object_type = "table"
 		read = true
        insert = true
        update = true
        delete = false
 	}

The latter lets us easily do boolean algebra to assign or remove permissions and be explicit in making sure a permission isn't present.

Also, you may have an interest in: https://github.com/sean-/postgresql-acl which I wrote once upon a time as a prerequisite for adding grant support.

Thoughts?

cyrilgdn · 2018-12-31T16:25:11Z

this is a huge PR, thank you for submitting this.

@sean- Actually the PR is splitted in two:

Create new resource: postgresql_grant #51 for the postgresql_grant resource
This one for the postgresql_default_privileges resource which depends on Create new resource: postgresql_grant #51

So this diff also contains the postgresql_grant resource. I'm not sure if it's the right way to manage dependency between PRs?

But your question is valid for both :)

Currently, the resource postgresql_grant only manages table and sequence but the long term goal is to support all object types.
So if we want to manage all privileges, it means that we'll have around 12 booleans in the resource.

Even if I understand the advantage that can provide the postgresql-acl lib, in this case I'm pretty sure it will not be easier to write the logic with booleans.

Currently, thanks to the aclexplode Postgres function I can easily build a Terraform Set struct which helps me to compare existing vs expected privileges. And thanks to transactions, I don't have to compare privilege by privilege to know what to apply. I revoke all the existing ones and apply all the expected ones in the same transaction (with 2 queries only).

and be explicit in making sure a permission isn't present.

For me it's explicit enough as the goal of Terraform is to describe in which state we want our resources.
I don't feel like we have to precise in which state it's not (even if I can understand this concern is a bit different with the privileges case).

But maybe I'm not the most objective as the author of this code :) So I'm open to other opinions.

cyrilgdn · 2019-01-12T15:10:29Z

@sean- So what do you think about that ? I'm now a maintainer of this repo and I really want make it evolve.

There's multiple enhancements that I want/need to do on these new resources but as I don't want to stack lots of PRs, I would like to start merging those already open.

Is your remark a no-go for #51 and this one ?

I know you currently don't have much time for that so I'll probably ask other people to do reviews but I don't want to merge PRs against your will.

Thanks !

evgmoskalenko · 2019-01-30T09:17:51Z

@cyrilgdn, Hello, tell me please, when the release is expected?

paultyng

A few comments, some may be related to lack of experience with Postgres though, so please forgive me learning in real time here.

paultyng · 2019-02-14T21:30:50Z

postgresql/provider.go

+			"postgresql_database":           resourcePostgreSQLDatabase(),
+			"postgresql_extension":          resourcePostgreSQLExtension(),
+			"postgresql_schema":             resourcePostgreSQLSchema(),
+			"postgresql_role":               resourcePostgreSQLRole(),


minor nit, but mind alphabetizing these?

Will do that yes

postgresql/resource_postgresql_default_privileges.go

paultyng · 2019-02-14T21:37:19Z

postgresql/resource_postgresql_default_privileges.go

+	if err != nil {
+		return err
+	}
+	defer txn.Rollback()


I haven't worked much with sql, but I'm guessing calling Rollback after Commit doesn't cause any issues?

You're right 👍 !

I didn't see that as I don't manage the error of Rollback 🤦‍♂️

I'll see what I can do

This resource allow to grant privileges on all existing tables or sequences for a specified role in a specified schema.

This resource allow to manage default privileges for tables or sequences for a specified role in a schema. # Conflicts: # postgresql/resource_postgresql_grant.go # Conflicts: # CHANGELOG.md

cyrilgdn · 2019-03-08T16:52:52Z

@paultyng I updated the PR with, among others, your remarks.

I disable these new resources for Postgres < 9 as it uses function and syntax which does not exist prior this version.
I also add documentation page for the website.

voslartomas · 2019-04-01T09:20:15Z

@cyrilgdn @paultyng When probably are you going to create new release, please?

cyrilgdn · 2019-04-01T13:34:12Z

@voslartomas Normally this week, I'm just trying to merge a last PR before. I'll let you now here, also you can watch the repository with the new "Releases only" mode in order to alerted.

voslartomas · 2019-04-01T13:34:54Z

@cyrilgdn Perfect, thanks a lot.

zytek · 2019-04-01T15:21:28Z

@cyrilgdn could we use postgresql_grant to also manage role membership?

zytek · 2019-04-01T16:13:57Z

@cyrilgdn also, found one issue, similar to: hashicorp/terraform#11452

when using this on RDS:

resource "postgresql_default_privileges" "priv-sequence-for-user" {
  database    = "${var.db}"
  owner       = "${var.owner}"
  role        = "${var.user}"
  schema      = "public"
  object_type = "sequence"
  privileges  = ["ALL"]
  depends_on  = ["postgresql_database.db"]
}

we need to first run `GRANT ${var.owner} to ${provider.username}"

can / should this be implemented ?

cyrilgdn · 2019-04-01T16:18:53Z

@zytek

could we use postgresql_grant to also manage role membership?

No, but it will be managed by the PR #52 that I would like to merge for the next release too (just looking for review).

also, found one issue, similar to: hashicorp/terraform#11452

Yes we have this issue too at my work. It can be implemented.
Could you create an issue for that please ?

zytek · 2019-04-01T20:10:18Z

@zytek

could we use postgresql_grant to also manage role membership?

No, but it will be managed by the PR #52 that I would like to merge for the next release too (just looking for review).

Ah, didn't notice #52, gonna give it a shot tomorrow.

also, found one issue, similar to: hashicorp/terraform#11452

Yes we have this issue too at my work. It can be implemented.
Could you create an issue for that please ?

Yes, along with one concern that I have plus I might try to implement it.

cyrilgdn · 2019-04-16T17:14:40Z

@voslartomas v0.3.0 has just been released.

ghost added the size/XXL label Nov 26, 2018

cyrilgdn force-pushed the upstream-postgresql_default_privileges branch from dd7076f to 1bbda4c Compare November 26, 2018 09:48

cyrilgdn mentioned this pull request Dec 29, 2018

Is this repository being maintained? #56

Closed

cyrilgdn mentioned this pull request Jan 14, 2019

Create new resource: postgresql_grant #51

Closed

cyrilgdn requested a review from sean- January 14, 2019 21:47

cyrilgdn added the enhancement label Jan 30, 2019

paultyng reviewed Feb 14, 2019

View reviewed changes

cyrilgdn force-pushed the upstream-postgresql_default_privileges branch from 1bbda4c to 47a0e8d Compare February 20, 2019 18:33

ghost added the dependencies label Feb 20, 2019

cyrilgdn force-pushed the master branch 2 times, most recently from 9c127f8 to 62ab8f1 Compare February 20, 2019 21:43

cyrilgdn force-pushed the upstream-postgresql_default_privileges branch from 47a0e8d to 80255dd Compare February 27, 2019 23:10

cyrilgdn added the WIP label Mar 1, 2019

cyrilgdn force-pushed the upstream-postgresql_default_privileges branch from 80255dd to e8dfd9f Compare March 4, 2019 23:37

cyrilgdn changed the title ~~New resource: posgresql_default_privileges~~ New resources: postgresql_grant & postgresql_default_privileges Mar 4, 2019

cyrilgdn force-pushed the upstream-postgresql_default_privileges branch from e8dfd9f to f8e91e1 Compare March 8, 2019 15:30

cyrilgdn added 2 commits March 8, 2019 17:37

Create new resource: postgresql_grant

d9f18f3

This resource allow to grant privileges on all existing tables or sequences for a specified role in a specified schema.

New resource: posgresql_default_privileges

7dccb18

This resource allow to manage default privileges for tables or sequences for a specified role in a schema. # Conflicts: # postgresql/resource_postgresql_grant.go # Conflicts: # CHANGELOG.md

cyrilgdn force-pushed the upstream-postgresql_default_privileges branch from f8e91e1 to 7dccb18 Compare March 8, 2019 16:39

ghost added the documentation label Mar 8, 2019

cyrilgdn removed the WIP label Mar 8, 2019

paultyng approved these changes Mar 26, 2019

View reviewed changes

cyrilgdn merged commit e709d75 into hashicorp:master Mar 31, 2019

zytek mentioned this pull request Apr 1, 2019

Temporarily add role membership when managing permissions #70

Closed

multani deleted the upstream-postgresql_default_privileges branch December 10, 2019 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resources: postgresql_grant & postgresql_default_privileges #53

New resources: postgresql_grant & postgresql_default_privileges #53

cyrilgdn commented Nov 26, 2018 •

edited

Loading

sean- commented Dec 29, 2018

cyrilgdn commented Dec 31, 2018

cyrilgdn commented Jan 12, 2019

evgmoskalenko commented Jan 30, 2019

paultyng left a comment

paultyng Feb 14, 2019

cyrilgdn Feb 20, 2019 •

edited

Loading

paultyng Feb 14, 2019

cyrilgdn Feb 20, 2019

cyrilgdn commented Mar 8, 2019

voslartomas commented Apr 1, 2019 •

edited

Loading

cyrilgdn commented Apr 1, 2019

voslartomas commented Apr 1, 2019

zytek commented Apr 1, 2019

zytek commented Apr 1, 2019

cyrilgdn commented Apr 1, 2019

zytek commented Apr 1, 2019

cyrilgdn commented Apr 16, 2019

New resources: postgresql_grant & postgresql_default_privileges #53

New resources: postgresql_grant & postgresql_default_privileges #53

Conversation

cyrilgdn commented Nov 26, 2018 • edited Loading

sean- commented Dec 29, 2018

cyrilgdn commented Dec 31, 2018

cyrilgdn commented Jan 12, 2019

evgmoskalenko commented Jan 30, 2019

paultyng left a comment

Choose a reason for hiding this comment

paultyng Feb 14, 2019

Choose a reason for hiding this comment

cyrilgdn Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

paultyng Feb 14, 2019

Choose a reason for hiding this comment

cyrilgdn Feb 20, 2019

Choose a reason for hiding this comment

cyrilgdn commented Mar 8, 2019

voslartomas commented Apr 1, 2019 • edited Loading

cyrilgdn commented Apr 1, 2019

voslartomas commented Apr 1, 2019

zytek commented Apr 1, 2019

zytek commented Apr 1, 2019

cyrilgdn commented Apr 1, 2019

zytek commented Apr 1, 2019

cyrilgdn commented Apr 16, 2019

cyrilgdn commented Nov 26, 2018 •

edited

Loading

cyrilgdn Feb 20, 2019 •

edited

Loading

voslartomas commented Apr 1, 2019 •

edited

Loading