feat(pgsql): extend Grant kind specification with schema, tables, columns, sequences, routines, foreign data wrappers, foreign servers

[Sabotaz]

Description of your changes

Fixes #217

This PR is a rework of #218 who is still a draft and will probably be closed soon by his owner (cf. comments).

This PR create more fields in the forProvider part of the Grant kind for managing grants on different objects.

I refactored #218 by removing the defined objectType and objects fields, and using specific fields for each type of grant. I believe it follows the original philosophy of the Grant kind better.

This PR does not cover the GRANT xxx ON ALL xxx IN SCHEMA yyy case.

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

I tested this PR using both make test and make e2e.
I tested the raw queries on my local database.

e2e tests & unit tests have been updated with more Grant configurations :

• grant some privileges on schema
• grant some privileges on 1 table
• grant some privileges on 1 column of 1 table
• grant some privileges on 1 routine
• grant some privileges on 1 sequence
• grant some privileges on 1 foreign data wrapper
• grant some privileges on 1 foreign server

However, I have only tested requests for multiple objets (i.e: grant all on table1, table2 to user ) on my local database.

I did not test this PR against sql injections, as I believe it is already a problem for the actual Grant feature (see #18 , #22 for mysql, and I would say #22 did not fully fix the problem for mysql as the injection can still be performed using some other fields: database, table, user).

[Sabotaz]

@Duologic, can anyone among the maintainers review this PR?
@Bastichou as you already worked on this issue, perhaps you can review it too? :D

[dawidmalina]

Adding @chlunde to the loop - this change is really needed by many Postgres users

[dawidmalina]

Just tested and I can confirmed that I can achieve GRANT USAGE, CREATE ON SCHEMA public TO <ROLE>; with crossplane:

---
apiVersion: postgresql.sql.crossplane.io/v1alpha1
kind: Grant
metadata:
  name: public
spec:
  forProvider:
    databaseRef:
      name: <DATABASE>
    privileges:
      - CREATE
      - USAGE
    roleRef:
      name: <ROLE>
    schema: public
  providerConfigRef:
    name: admin

Looking forward to use official provider not custom build.

[chlunde]

@dawidmalina thanks for testing!

If we have consensus on the schema design, I think that (and testing) is most important.

Linewise, the change is huge (+2000 lines), so getting a few eyes on this would be very helpful.

[dawidmalina]

Any updates on this? I would really like to see that to be part of the official release.

[chlunde]

@Bastichou could you take a look at this version?

I've read through all the issues and closed duplicates, and I think this solves a lot of them. I'd like to make this part of the next release, so thanks a lot for everyone working on this!

Two things I would like to test:

• That this is 100% compatible with existing grants
• That we're sure this is the right design

This PR does not cover the GRANT xxx ON ALL xxx IN SCHEMA yyy case.

@Sabotaz do you think this could be handled in the future, could you expand a bit on this? Thanks!

[Kidswiss]

That we're sure this is the right design

Can you elaborate what design you mean? The CRD design? Code design?

[Kidswiss]

FWIW: I've tested the implementation and so far seems to be doing what it should.

[Sabotaz]

This PR does not cover the GRANT xxx ON ALL xxx IN SCHEMA yyy case.

@Sabotaz do you think this could be handled in the future, could you expand a bit on this? Thanks!

An example :
given table_1 and table_2
we can make a grant like this one :

apiVersion: postgresql.sql.crossplane.io/v1alpha1
kind: Grant
metadata:
  name: example-grant-role-1-on-table
spec:
  forProvider:
    privileges:
      - SELECT
      - INSERT
    roleRef:
      name: {role}
    databaseRef:
      name: {db}
    schemaRef:
      name: {schema}
    tables:
      - table_1
      - table_2

if someone create a table_3, we have to add table_3 in this list.

Some users would like to directly grant privileges on all tables, something like :

spec:
  forProvider:
    tables:
      - ALL

The thing is, in the same way the "ALL" privileges is expanded on the real privileges list, we would need to expand the table list by listing all the tables on the schema every time this resource is reconciled, because if a new table is created, grants are not given by default, and the "ALL" on the past is not always the same as the "ALL" of the future.

Another technique is to just count the correct grants and assert it's the same length as the tables count, so no expand is needed.

This issue could also be addressed by resolving #117 (default privileges).

About if it can be handled in the future, yes probably. The SQL requests are already quite obscure, but if someone love SQL, it should be easy.

I don't like the - ALL spec, but i don't have a better solution now.

[Sabotaz]

That we're sure this is the right design

Can you elaborate what design you mean? The CRD design? Code design?

I think the CRD design.

The initial PR #218 had another design :

spec:
  forProvider:
    database: {db}
    schema: {schema}
    objectType: table
    objects:
      - table_1
      - table_2

But it had 2 drawbacks :

• when granting on schema or database, the initial PR spec is :

spec:
  forProvider:
    database: {db}
    schema: {schema}
    objectType: schema

• when granting on columns, the initial PR spec is :

spec:
  forProvider:
    database: {db}
    schema: {schema}
    objectType: column
    objects:
      - table_1
    columns:
      - col_1
      - col_2

For some values of objectTypes, the objects field is not used
For some values of objectTypes, the objects field contains objects of another type than objectTypes

These problems does not exist with the proposed CRD.

Another good point with this design is that for tables, the spec is the same as the MySQL Grant kind, so I think having one field for each kind of objects is more in the initial philosophy of the Grant CRD.

apiVersion: mysql.sql.crossplane.io/v1alpha1
kind: Grant
metadata:
  name: example-grant-table
spec:
  forProvider:
    privileges:
      - DROP
      - INSERT
      - SELECT
    table: example-table
    userRef:
      name: example-user
    databaseRef:
      name: example-db

[dawidmalina]

Are we going in direction where this change can be accepted and merged?

[dawidmalina]

Adding @ulrich-giraud who added capability to grant on all sequences and tables https://github.com/Evaneos/provider-sql maybe we could join forces under one project.

[ulrich-giraud]

I was waiting for this change to be accepted and avoid adding another layer of complexity (and it also needs a little bit more work ).
In fact we needed the "ALL TABLES" and "ALL SEQUENCES", and we are currently using our fork that extends this design, meaning we also tested this change, and so far, everything works correctly.

I don't like the - ALL spec, but i don't have a better solution now.

I played with the idea of a boolean property allTables but imho, the - ALL fits better the way grants are written and is easier to understand for the user.

[ulrich-giraud]

@Sabotaz @chlunde Apart from testing and contributing our opinions on the design, is there any other way I can help with the approval of this PR?

[Sabotaz]

@Sabotaz @chlunde Apart from testing and contributing our opinions on the design, is there any other way I can help with the approval of this PR?

Slack is probably the best way, but no luck for me there...

[dawidmalina]

I wonder if there is still something missing in this PR before it can be merged? I think we all agree and there have been some tests done to prove it is correct. I am pretty sure that the current shape is good enough to start with and all other enhancements like ALL support can be delivered later. What do you think?

[sandpwr]

My two cents:

In most workflows, tables, sequences, functions, etc. are created by Flyway or Liquibase, so you can’t know their names up front. It would be great to have an ALL option like GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA in the Grant CRD. That way you can bootstrap a database, schema, and permissions in one go without needing a separate post-migration step.

Hope this makes sense!

[fnicolelli-ls]

Hi @Sabotaz would it be possible to include altering the default permissions as well? i.e:
ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE, TRUNCATE ON TABLES TO ;
ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT EXECUTE ON FUNCTIONS TO ;

[tomitesh]

this is very useful change. waiting for this to approve and merge. are we publishing any pre-release version which i can test and give feedback?

[BradMoulderAlethea]

What is that status of this? This would be extremely useful for us as well.

[zorocloud]

+1 for this being a feature that would be very useful.

[Sabotaz]

Hi @Sabotaz would it be possible to include altering the default permissions as well? i.e: ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE, TRUNCATE ON TABLES TO ; ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT EXECUTE ON FUNCTIONS TO ;

Hello fnicolelli-ls :)
Here is the PR you are looking for : #199

[dawidmalina]

Hi @chlunde :) any update on this topic? Is there anything we can do to move this forward?