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

enhancement(refactor)!: Refactor policy Admin API endpoints #516

Merged
merged 14 commits into from Dec 31, 2021

Conversation

oguzhand95
Copy link
Member

Description

Fixes #487

Checklist

  • The PR title has the correct prefix
  • PR is linked to the corresponding issue
  • All commits are signed-off (git commit -s ...) to provide the DCO

@oguzhand95 oguzhand95 self-assigned this Dec 21, 2021
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

I think that instead of the FQN, we should let the human readable identifier be the tuple (kind, name, version) which can be represented as a dot separated string (resource.leave_request.default). That shields the users from being exposed to internal implementation details and allow them to work with the concrete data they deal with in the policy files.

Also, I believe the cerbosctl commands need to be updated to handle this change as well. I think we are now losing the ability to filter and sort policies by fields.

api/public/cerbos/request/v1/request.proto Outdated Show resolved Hide resolved
api/public/cerbos/request/v1/request.proto Outdated Show resolved Hide resolved
api/public/cerbos/response/v1/response.proto Outdated Show resolved Hide resolved
client/admin.go Outdated Show resolved Hide resolved
client/admin_test.go Outdated Show resolved Hide resolved
internal/storage/db/internal/db.go Outdated Show resolved Hide resolved
internal/storage/db/mysql/schema.sql Outdated Show resolved Hide resolved
internal/storage/index/builder.go Outdated Show resolved Hide resolved
@charithe charithe marked this pull request as draft December 22, 2021 10:26
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
…tore

Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
api/public/cerbos/request/v1/request.proto Outdated Show resolved Hide resolved
api/public/cerbos/request/v1/request.proto Outdated Show resolved Hide resolved
api/public/cerbos/svc/v1/svc.proto Outdated Show resolved Hide resolved
client/admin.go Outdated Show resolved Hide resolved
internal/storage/blob/store.go Outdated Show resolved Hide resolved
internal/storage/db/internal/db.go Outdated Show resolved Hide resolved
internal/storage/db/internal/db.go Outdated Show resolved Hide resolved
internal/storage/db/internal/db.go Outdated Show resolved Hide resolved
internal/svc/admin_svc.go Outdated Show resolved Hide resolved
internal/storage/index/index.go Outdated Show resolved Hide resolved
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
@oguzhand95 oguzhand95 changed the title enhancement(refactor): Refactor policy Admin API endpoints [DRAFT/WIP] enhancement(refactor): Refactor policy Admin API endpoints Dec 29, 2021
@oguzhand95
Copy link
Member Author

oguzhand95 commented Dec 29, 2021

@charithe
The table of support for concatenation using || and CONCAT() look like this;

SQLite MySQL PostgresSQL
|   -   |   +   |   +   | CONCAT()
|   +   |   -   |   +   | ||

MySQL documentation suggests executing the following makes MySQL recognize || as concatenation operator. (By default, MySQL treats || as OR.)
SET sql_mode = 'PIPES_AS_CONCAT';

Let's discuss whether this could be an option or not.
One other option may be using goqu to handle the case.

Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
client/admin.go Outdated Show resolved Hide resolved
cmd/cerbosctl/list/list.go Outdated Show resolved Hide resolved
internal/storage/db/internal/db.go Outdated Show resolved Hide resolved
@charithe
Copy link
Contributor

It's a shame that MySQL doesn't conform to ANSI SQL. We'll have to figure out a way to generate the correct CONCAT SQL based on the database dialect. Goqu should have a way of doing that but it's not very obvious. I'll do a bit more digging and discuss the options with you.

Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽 Just a couple of small nits to address.
Will do a bit of testing tomorrow morning and we can merge this afterwards.

internal/storage/db/postgres/schema.sql Outdated Show resolved Hide resolved
internal/svc/list.go Outdated Show resolved Hide resolved
@charithe charithe marked this pull request as ready for review December 30, 2021 18:31
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
@charithe charithe changed the title enhancement(refactor): Refactor policy Admin API endpoints enhancement(refactor)!: Refactor policy Admin API endpoints Dec 31, 2021
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@charithe charithe merged commit f8aadf6 into cerbos:main Dec 31, 2021
@oguzhand95 oguzhand95 deleted the api/policy branch December 31, 2021 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Admin API
2 participants