cyrilgdn / terraform-provider-postgresql

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

Bypass role grant logic for `pg_database_owner` role #316

Closed joshsouza closed 10 months ago

joshsouza commented 1 year ago

See issue #301. This appears to solve the issue, but may have unanticipated side-effects.

This is my incredibly naive attempt at fixing the issue here. I don't understand enough of what is being done here and why to understand if this is a terrible idea, or what limitations this will introduce. I imagine that without this, if Terraform is using a role that doesn't have proper permissions to grant permissions, it will fail (but for a different reason), which may be why this code was originally in place.

I have tried this out by debugging locally with this change in place on a state impacted by the failure in https://github.com/cyrilgdn/terraform-provider-postgresql/issues/301 and it was able to apply for my state without a problem.

I would love to get any feedback on what this PR would need to be up to necessary standards to be pulled in.

romanstingler commented 1 year ago

@cyrilgdn can we prioritize that, as it is a blocker for POSTGRES15

mattthaber commented 11 months ago

@cyrilgdn (and @kylejohnson, not sure if you are a new maintainer?) any idea when this would get in? Or when supporting pg_database_owner will be implemented?

This is a pretty major blocker if people cant upgrade their PG versions

cyrilgdn commented 11 months ago

Hi,

I had a look last Sunday week-end but I'm don't think thix fix is enough, I'll try to confirm that next week-end and to enhance the fix if needed.

fjlopezs commented 11 months ago

Hi @cyrilgdn. Just for curiosity, have you had a chance to take a look at this?

ToniRib commented 11 months ago

Also following along on this thread. This is currently a blocker for us to stand up postgres 15 databases on AWS Aurora. Basically it means teams who need a new database must use 14.x, which just means we're going to have to deal with more upgrades in the future.

tarvip commented 11 months ago

I tested this fix with Google Cloud SQL PostgreSQL 15 and it seems to work fine.

ThomasKasene commented 11 months ago

I'm no expert on this, but should the change introduced in this PR only apply when PostgreSQL versions >= 14? (Or 15? Although I think the role was introduced in PostgreSQL 14.)

That way it won't introduce any (unlikely) side-effects on earlier versions.

joshsouza commented 10 months ago

I'm no expert on this, but should the change introduced in this PR only apply when PostgreSQL versions >= 14? (Or 15? Although I think the role was introduced in PostgreSQL 14.)

That way it won't introduce any (unlikely) side-effects on earlier versions.

This change would only apply to databases with the pg_database_owner role. Which is any database PSQL 14 and up (since it's created by default), or any older database where someone chose to create a role by that name (possible). So, side-effects are possible, though I would hope that creating a role with the same name on a lower version would mean that bypassing the logic that this does would remain valid. Obviously that's an assumption though.

I am also not sure that this is actually the ideal "fix" to the problem, it's my best attempt to work around an issue with no knowledge on what's really happening or intended here.

So short answer: Yeah, that is probably a reasonable check that could be added here, but rather than add more complex logic to the fix I'm proposing, I'd like to give @cyrilgdn time/space to assess the root cause and determine if this is the right approach, or if there's a more complete solution.

jindrichskupa commented 10 months ago

@cyrilgdn Please, we are blocked on this. We can't manage new databases or migrate to 15.

andyhuynh3 commented 10 months ago

Running into this issue as well when trying to create Aurora Postgres 15.x instances on AWS.

cyrilgdn commented 10 months ago

Replaced by #348