Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cyrilgdn#321 replaces postgresql_grant all the time. #476

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

PabloAzNR
Copy link
Contributor

@PabloAzNR PabloAzNR commented Oct 2, 2024

Fixes #321

Fix based on doctolib's fork

version 1.19.0, with PR 135, the postgresql_grant resource gets re-created when there is a change.
Replacing the resource is not a good idea because the "destroy/create" operations are completely separate. i.e. they are not atomic which means (given the example in the "Steps to Reproduce" section above) for a short moment between the 2 operations the public role loses access to the public schema. If for any reason Terraform fails midway or it gets interrupted, users will end up not being able to access the objects in the public schema. This is what happens in the PostgreSQL log:

2023-07-11 14:50:05.989 UTC [1673] LOG:  statement: BEGIN READ WRITE
2023-07-11 14:50:06.000 UTC [1673] LOG:  statement: REVOKE ALL PRIVILEGES ON SCHEMA "public" FROM "public"
2023-07-11 14:50:06.001 UTC [1673] LOG:  statement: COMMIT
2023-07-11 14:50:06.033 UTC [1675] LOG:  statement: BEGIN READ WRITE
2023-07-11 14:50:06.043 UTC [1675] LOG:  statement: REVOKE ALL PRIVILEGES ON SCHEMA "public" FROM "public"
2023-07-11 14:50:06.044 UTC [1675] LOG:  statement: GRANT USAGE ON SCHEMA "public" TO "public"
2023-07-11 14:50:06.045 UTC [1675] LOG:  statement: COMMIT

In our case we're only removing ForceNew from privileges, as this fixes our use case, but the overall solution allows every schema to be updated instead of recreated.

Introduced a "getter" in order to fix PR 135 original issue that caused the introduction of "ForceNew".

Originally, the privileges argument did not force recreation of the resource.
This was a problem because it meant that when changing the privileges in a grant resource, the update function would be triggered and would receive only the new configuration. So the revocation would not revoke the old permissions, but the new one, which is not very useful.
....
I could not find a way to fetch the privilege stored in the state, & setting the argument to ForceNew solved this problem. So I did that.

Fetching the privilege stored in state is the job of our new getter, this way we don't have to "ForceNew" everything.

I think we might be able to keep a single "Create" function if we wanted, checking d.IsResourceNew() to decide if we should use the old one or new one, but the solution from doctolib seems robust enough.

@PabloAzNR PabloAzNR marked this pull request as ready for review October 2, 2024 13:58
@PabloAzNR PabloAzNR marked this pull request as draft October 2, 2024 14:07
@PabloAzNR PabloAzNR marked this pull request as ready for review October 2, 2024 14:19
@alhroub
Copy link

alhroub commented Oct 23, 2024

@cyrilgdn Could you please take a look at this PR?
This is blocking us from upgrading to the latest version.
Thanks.

Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing it here 👍

@cyrilgdn cyrilgdn merged commit 6d26b59 into cyrilgdn:main Oct 23, 2024
6 checks passed
@cyrilgdn
Copy link
Owner

cyrilgdn commented Dec 5, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.19.0 replaces postgresql_grant all the time
3 participants