search

berenddeboer / cdk-rds-sql

A CDK construct that allows creating roles and databases an on Aurora Serverless Postgresql cluster.
Apache License 2.0
23 stars 11 forks source link

feat: Grant access to schema for role (#36) #37

Closed stephanpelikan closed 2 months ago

stephanpelikan commented 2 months ago

Fixes #36

Tests are missing since I don't know how to test this, write them and unfortunately my time is limited. I've tested the code for creating and deleting a schema manually in my project successfully.

berenddeboer commented 2 months ago

Yeah, this needs to come with a test. Just look at the handler.test.ts. Not merging code without an actual test that runs that code. And yes, everyone's time is limited.

stephanpelikan commented 2 months ago

The tests you did are quite easy to achieve. It is hard to figure out about those grants. I still searching for those queries which can be used for tests...

stephanpelikan commented 2 months ago

I found a way to test for those grants on stackoverflow. I added the test and I hope it is sufficient.

berenddeboer commented 2 months ago

I have another idea: should we introduce a concept like schema access or so? That can do what your role property does. Would make it easier to grant access to the schema to more roles, as this just handles only the one role case.

What do you think?

Will have a look at your changes later.

stephanpelikan commented 2 months ago

Well yes. But this also applies to databases. I use one RDS serverless instance for all app in my Kubernetes cluster. Within that instance I create a database per application. I only use the public schema, but roles created for and applied to those databases are not allowed to use public. Actually, whatever is done needs to be applies to databases and schemas. One uses multiple databases and another multiple schemas and others multiple databases and multiple schemas.

What I often do is: Keep it simple for the simple case and provide a more flexible way for more complex scenarios. For that simple scenario I would change the role properties of database and schema to roles: Role | Role[]. But if you provide a separate construct for granting and it's that easy for the simple cases then it would be fine as well. Maybe something like this:

interface RoleGrantProps {
  provider: Provider
  role: Role | Role[]
  target: Database | Schema
  grants: 'all' | 'readonly' | string | string[]
}

For the custom grants I don't know enough about what is typically needed. Maybe it is the best the simply allow grant-SQLs here.

Hower, I build CDK stacks for a customer as a draft and would need something released. If you don't want to release the current version in preference of a separate construct, then please let me know so I can replace the new role property by a SQL construct.

berenddeboer commented 2 months ago

Merged, thanks for all the work @stephanpelikan. I agree, we can improve things later.