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

feat: basic support OPA plugin #5734

Merged
merged 22 commits into from
Dec 13, 2021
Merged

feat: basic support OPA plugin #5734

merged 22 commits into from
Dec 13, 2021

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Dec 8, 2021

What this PR does / why we need it:

Add OPA plugin to support API access control using OPA services.
implement #5714

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first
@bzp2010 bzp2010 self-assigned this Dec 8, 2021
@bzp2010 bzp2010 force-pushed the feat-opa branch 2 times, most recently from 217c921 to 56f52cd Compare December 8, 2021 16:00
@bzp2010 bzp2010 marked this pull request as ready for review December 9, 2021 08:07
@bzp2010 bzp2010 added the enhancement New feature or request label Dec 9, 2021
@bzp2010 bzp2010 linked an issue Dec 9, 2021 that may be closed by this pull request
apisix/plugins/opa.lua Outdated Show resolved Hide resolved
apisix/plugins/opa.lua Outdated Show resolved Hide resolved
apisix/core/request.lua Outdated Show resolved Hide resolved
apisix/plugins/opa/helper.lua Outdated Show resolved Hide resolved
t/plugin/opa.t Outdated Show resolved Hide resolved
Copy link

@anderseknert anderseknert left a 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! 👏

@bzp2010
Copy link
Contributor Author

bzp2010 commented Dec 9, 2021

Hi, @anderseknert. Glad to see your comments.

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.

Do you mean to merge host, package, decision together and use a single field (eg. url)?

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?

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.

@anderseknert
Copy link

Do you mean to merge host, package, decision together and use a single field (eg. url)?

Yeah. Either way is fine though :)

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.

Oh, nice! I had missed the roadmap section. Thanks for bringing that to my attention! Sounds like a good plan 👍

@bzp2010
Copy link
Contributor Author

bzp2010 commented Dec 9, 2021

Ok, In that case, I would keeping the current way of combining URLs, which seems to provide better flexibility. XD

@anderseknert
Copy link

anderseknert commented Dec 9, 2021

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:

http://localhost:8181/v1/data/policy

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 http://localhost:8181/v1/data/policy, would respond with the result from evaluating both rules:

{
    "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.

@bzp2010
Copy link
Contributor Author

bzp2010 commented Dec 9, 2021

Oh, I see, I will change it later. I don't know OPA very well, thank you for pointing out my problem.

apisix/plugins/opa.lua Outdated Show resolved Hide resolved
apisix/plugins/opa.lua Outdated Show resolved Hide resolved
t/plugin/opa.t Outdated Show resolved Hide resolved
@bzp2010
Copy link
Contributor Author

bzp2010 commented Dec 13, 2021

ping @tokers @tzssangglass @shuaijinchao PTAL, thanks

@bzp2010 bzp2010 changed the title feat: support OPA plugin Dec 13, 2021
@bzp2010 bzp2010 merged commit 1e53ccb into apache:master Dec 13, 2021
@moonming
Copy link
Member

@bzp2010 please add OPA into the README, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
7 participants