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 2 commits
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.
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.
Please specify the precedence between this one and the path way.
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 make it more clear there.