fix: Optional revoke on postgresql_grant_role

[faymard]

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]

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]

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

[cyrilgdn]

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.

[faymard]

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 😄 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]

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]

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.

[faymard]

Hi, is it possible for you to review/merge this soon ? Thanks in advance

[cyrilgdn]

Hi @faymard ,

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.

Do you have some examples on some use case it could be useful.

Otherwise you can simply remove the revoke I think.

[faymard]

Hi,

Do you have some examples on some use case it could be useful

Well, I don't but I was just making assumptions 😅 Maybe it's just useful in case the role was granted outside of Terraform to avoid conflicts, but IMHO this should be resolved by imports.

If you're ok with simply removing the revoke part, I'll do it ASAP, no problems. Maybe even tomorrow so it's done quick.

[cyrilgdn]

If you're ok with simply removing the revoke part, I'll do it ASAP, no problems. Maybe even tomorrow so it's done quick.

I'm totally ok 👍 (it's the same than having the revoke disable by default, in the worse case of someone complain I'll do a small patch like you did here)

[faymard]

I made the change this morning. I still need to test it though

[cyrilgdn]

@faymard Is it ok for you, can we merge it ?

[faymard]

Yes, it's good for us and working as intended. Thanks :)