Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Added authz-casbin plugin and doc and tests for it #4710
feat: Added authz-casbin plugin and doc and tests for it #4710
Changes from 1 commit
cb37e5f
56668c0
38a1ff1
67a433e
2eb4cb5
22f76c5
2fa5b82
0aa4518
b95d9af
bc7981f
c15ecf9
8824fc9
e0858c8
842f9d1
a32d79e
ba0b0c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The plugin_metadata should be only set on the CP side. It's forbidden to let configuration flow from the DP to the CP.
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.
IMHO, it surprises me that the enforcer from metadata can override the one from the route. Normally, the route level configuration can override the global one.
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.
Multiple routes will use the same casbin enforcer? I think we should store different casbin via lrucache, like this:
apisix/apisix/plugins/error-log-logger.lua
Line 144 in 9db2dd2
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.
Yes, you are right. I have changed it now.
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.
Sorry, I rethought about it afternoon, and found it isn't a good idea.
It is more suitable to bind the casbin with the conf like:
apisix/apisix/plugins/uri-blocker.lua
Line 83 in 38561dc
We also need to ensure the global casbin is update-to-date with the metadata via #4710 (comment)
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.
@spacewander Sorry, I found this now. But is there any reason for us to not use lrucache? It worked well for two different routes with different casbin enforcers when I tested it.
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.
I have removed
lrucache
and replaced it by binding thecasbin_enforcer
to conf. Also we will use themodifiedIndex
to keep the casbin from metadata updated.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.
It works fine, but makes the life cycle complex.
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.
We don't need to check the existence for these three variables if we use them in the HTTP sub-system.
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.
Yes, right - this is redundant here.
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.
Since we already use metadata to store the configuration, there is no need to provide APIs on the DP side. Everything should be done with the plugin metadata's API on the CP side.
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.
@spacewander
Thanks for your review, so we will remove all the API functions and if the model/policy text in plugin metadata is updated (from the CP side), we will update the
casbin_enforcer
through that model/policy too. Is this correct?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.
Yes, you can use
metadata.modifiedIndex
to ensure casbin_enforcer is created with the latest model/policy, like this one:apisix/apisix/http/router/radixtree_uri.lua
Lines 31 to 36 in fd8f875
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.
And I think you can submit a minimum viable pull request so that we can check & merge it early.
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.
Sure I will do that.
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.
I added a condition that if the configuration changes, the
casbin_enforcer
will be recreated.