-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: basic support OPA plugin #5734
Conversation
217c921
to
56f52cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I can't comment on the Lua bits, but what I read looks sensible. Some random thoughts:
- I would probably just let the user configure the full OPA URL rather than separating host, package and decision. In some cases you'll actually query a whole package, which is fine for OPA.
- The policy example (and from what I understand, the Lua code) only deals with boolean decisions. Maybe good to keep it simple, but could also be useful to allow OPA to return for example a reason for denying a request, and surface that to the caller.
- I don't see much in terms of documentation. Will that be added in a later PR?
But all things considered, great work on this! 👏
Hi, @anderseknert. Glad to see your comments.
Do you mean to merge host, package, decision together and use a single field (eg.
Features such as custom response codes, response headers and error causes will be added later in the PR. It will basically follow the roadmap section in #5714 to implement it. |
Yeah. Either way is fine though :)
Oh, nice! I had missed the roadmap section. Thanks for bringing that to my attention! Sounds like a good plan 👍 |
Ok, In that case, I would keeping the current way of combining URLs, which seems to provide better flexibility. XD |
I'm not sure that it's more flexible. It seems that currently all the fields are required to build the URL: required = {"host", "package", "decision"} But while not super common, it is perfectly legitimate to query OPA for only a package, like:
When OPA is queried on the package level, it will evaluate all the rules under that package and return the result. So a policy that looks like the below: package policy
default allow = false
allow {
input.user.roles[_] == "admin"
}
reason = "User needs to be admin" {
not allow
} When queried at {
"allow": false,
"reason": "User needs to be admin"
} In this case, we don't query a rule (or decision as it's called in this code) directly, but rather all the rules in the package. |
Oh, I see, I will change it later. I don't know OPA very well, thank you for pointing out my problem. |
ping @tokers @tzssangglass @shuaijinchao PTAL, thanks |
@bzp2010 please add OPA into the README, thanks. |
What this PR does / why we need it:
Add OPA plugin to support API access control using OPA services.
implement #5714
Pre-submission checklist: