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

Allow writer user to create tables in schema #136

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

mfuhrmeisterDM
Copy link
Contributor

why

In postgres 15 they change the behaviour of the public schema. Now only the owner can create tables in this schema. And the user is in charge to configure the permissions.

proposed change

Grant the writer user to also create tables in a schema.

usage

add the public schema explicitly to the list of schemas to create, to force the schema privileges to be applied.

pkg/postgres/database.go Show resolved Hide resolved
@pcallewaert
Copy link
Contributor

👋 We tested this PR out as we are migrating to PG15, but we noticed this was only a part of the solution. We also had to call the new function for the public schema in the db creation logic when the ownership is transferred to the new owner, and also for the writer group.

@mfuhrmeisterDM are you still working on this? If not, I can try to work on this.

@mfuhrmeisterDM
Copy link
Contributor Author

@pcallewaert I updated the PR like requested. I must say I almost forgot this PR, thanks for the reminder.

@mfuhrmeisterDM
Copy link
Contributor Author

👋 We tested this PR out as we are migrating to PG15, but we noticed this was only a part of the solution. We also had to call the new function for the public schema in the db creation logic when the ownership is transferred to the new owner, and also for the writer group.

Can you explain a bit more what you mean, because in our case this worked fine.

@pcallewaert
Copy link
Contributor

👋 We tested this PR out as we are migrating to PG15, but we noticed this was only a part of the solution. We also had to call the new function for the public schema in the db creation logic when the ownership is transferred to the new owner, and also for the writer group.

Can you explain a bit more what you mean, because in our case this worked fine.

Of course. We are using a very basic Postgres resource:

apiVersion: db.movetokube.com/v1alpha1
kind: Postgres
metadata:
  name: postgresdb
spec:
  database: example-db
  dropOnDelete: true
  extensions:
    - fuzzystrmatch

We don't have schemas defined as we just use the public schema, and we use the owner permissions for our user.
The code changes here are made in the for loop for each schema. So we never hit the new code as this is an empty array.
In our patched version we re-used your method on L180 during the database creation logic, but specific for the public schema for the owner group.

@mfuhrmeisterDM
Copy link
Contributor Author

mfuhrmeisterDM commented Feb 14, 2024

Of course. We are using a very basic Postgres resource:

apiVersion: db.movetokube.com/v1alpha1
kind: Postgres
metadata:
  name: postgresdb
spec:
  database: example-db
  dropOnDelete: true
  extensions:
    - fuzzystrmatch

We don't have schemas defined as we just use the public schema, and we use the owner permissions for our user. The code changes here are made in the for loop for each schema. So we never hit the new code as this is an empty array. In our patched version we re-used your method on L180 during the database creation logic, but specific for the public schema for the owner group.

Got it, I will address this.
Edit: I double checked our code and we had the same problem. But we instead just added the public schema to the list of schemes. Is this also an option to you? (@pcallewaert )

@pcallewaert
Copy link
Contributor

pcallewaert commented Feb 14, 2024

Of course. We are using a very basic Postgres resource:

apiVersion: db.movetokube.com/v1alpha1
kind: Postgres
metadata:
  name: postgresdb
spec:
  database: example-db
  dropOnDelete: true
  extensions:
    - fuzzystrmatch

We don't have schemas defined as we just use the public schema, and we use the owner permissions for our user. The code changes here are made in the for loop for each schema. So we never hit the new code as this is an empty array. In our patched version we re-used your method on L180 during the database creation logic, but specific for the public schema for the owner group.

Got it, I will address this. Edit: I double checked our code and we had the same problem. But we instead just added the public schema to the list of schemes. Is this also an option to you? (@pcallewaert )

If it works it is certainly an option 😄 I'll have to test it out, I'm hoping to do it this week.

@mfuhrmeisterDM
Copy link
Contributor Author

@hitman99 can you please have a look?

@hitman99
Copy link
Member

Nice work, will check this out tomorrow. If there's no problems this will be merged and released as well

pcallewaert added a commit to pcallewaert/postgres-operator that referenced this pull request Feb 22, 2024
@pcallewaert
Copy link
Contributor

👋 I've finally found time to test it out, and the PR should indeed work for the writer user specific. But for the OWNER privilege it is missing a bit.
During the CreateDB we should also give the owner group the permissions to create in the database. Even as owner, you don't have the permissions to create without the explicit GRANT.
https://github.com/pcallewaert/postgres-operator/blob/pc/test-pg15-fix/pkg/postgres/database.go#L45-L53

pkg/controller/postgres/postgres_controller.go Outdated Show resolved Hide resolved
pkg/controller/postgres/postgres_controller.go Outdated Show resolved Hide resolved
Matthias Fuhrmeister added 4 commits February 27, 2024 11:15
In postgres 15 they change the behaviour of the public schema. Now only
the owner can create tables in this schema. And the user is in charge to
configure the permissions.

Grant the writer user to also create tables in a schema.

add the public schema explicitly to the list of schemas to create, to
force the schema privileges to be applied.
@mfuhrmeisterDM
Copy link
Contributor Author

👋 I've finally found time to test it out, and the PR should indeed work for the writer user specific. But for the OWNER privilege it is missing a bit. During the CreateDB we should also give the owner group the permissions to create in the database. Even as owner, you don't have the permissions to create without the explicit GRANT. https://github.com/pcallewaert/postgres-operator/blob/pc/test-pg15-fix/pkg/postgres/database.go#L45-L53

incorporated this

@hitman99
Copy link
Member

Hey @mfuhrmeisterDM woud you mind merging master or rebasing on master? I approve the changes but would like to base it on master. Once that's done I'll merge & release it

@hitman99 hitman99 merged commit bd11372 into movetokube:master Feb 29, 2024
2 checks passed
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.

3 participants