search

cyrilgdn / terraform-provider-postgresql

Terraform PostgreSQL provider
https://www.terraform.io/docs/providers/postgresql/
Mozilla Public License 2.0
356 stars 181 forks source link

fix: Optional revoke on postgresql_grant_role #384

Open faymard opened 6 months ago

faymard commented 6 months ago

Since PostgreSQL 16.0, there is a dependency check made when revoking grants on roles.

Specifically, if role A has a grant with admin rights on role B, and uses these rights to grant role B to role C, then it can't be revoked without cascading.

This PR allows to use postgresql_grant_role without doing an initial REVOKE query. Our use case for this feature is applying two different DB stacks (one for development and one for unit testing) which share grants on common roles.

faymard commented 4 months ago

Hi, could we had a quick review and feedback on this one please ? A few of our teams are starting their upgrades to PG16 and they start experiencing errors due to the systematic revoke.

faymard commented 4 months ago

Hi, I would like some view on when this PR could be checked (if it ever could be). Thank you very much :)

cyrilgdn commented 4 months ago

Hi,

Thank you for opening this PR and sorry for the response delay.

Actually I wonder why it revokes the role before granting him again :thinking:

I know in postgresql_grant we do that to be sure to revoke potential extra permission, but in this we want the role to be granted eventually so :thinking:

As it's possible to grant the same role twice, it will just print a NOTICE log saying that the role is already a member, I think we could get rid of this revoke. I'll just quickly check if it's the same behavior in all supported versions.

faymard commented 4 months ago

Hi,

Thank you for opening this PR and sorry for the response delay.

Actually I wonder why it revokes the role before granting him again 🤔

I know in postgresql_grant we do that to be sure to revoke potential extra permission, but in this we want the role to be granted eventually so 🤔

As it's possible to grant the same role twice, it will just print a NOTICE log saying that the role is already a member, I think we could get rid of this revoke. I'll just quickly check if it's the same behavior in all supported versions.

Hi, thank you very much for your feedback :smile: I also had the same feeling about this potentially-useless revoke, but I was not confident enough in my DBA skills to assume it in the first place. Thank you for comforting this point of view.

I'm waiting for your feedback then.

Have a nice day/evening

faymard commented 2 months ago

Hi, any news ? We're being sollicitated on this issue, we have a work-around to provide (we built a custom version of the provider, and we tell people to change the version temporarily) but it's not easy to understand for our colleagues.

faymard commented 1 month ago

Hi,

I did a small change to disable the REVOKE query by default. For me it makes more sense since its usage is not trivial in all cases, but I do get that it could be useful for some. Hence it should be opt-in for me.