feat: Make Casbin matcher configurable on runtime(globMatch(default) or RegexMatch) #7165
Conversation
…ces argoproj#2338 Signed-off-by: cezhang <c1zhang.dev@gmail.com>
…or RegexMatch) Signed-off-by: cezhang <c1zhang.dev@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #7165 +/- ##
==========================================
+ Coverage 41.24% 41.34% +0.09%
==========================================
Files 161 161
Lines 21525 21581 +56
==========================================
+ Hits 8878 8922 +44
- Misses 11384 11399 +15
+ Partials 1263 1260 -3
Continue to review full report at Codecov.
|
Signed-off-by: cezhang <c1zhang.dev@gmail.com>
|
Hello @alexmt , any suggestion of moving next? |
util/rbac/rbac_test.go
Outdated
| _ = enf.SetUserPolicy(policy) | ||
|
|
||
| assert.True(t, enf.Enforce("alice", "clusters", "get", "https://github.com/argoproj/argo-cd.git")) | ||
| assert.False(t, enf.Enforce("alice", "repositories", "get", "https://github.com/argoproj/argo-cd.git")) |
There was a problem hiding this comment.
In this test, the policy is for cluster, p, alice, clusters, get, "https://github.com/*/*.git", allow, wonder if you want to verify for repositories? Should it be testing for a cluster which does not match with the expression?
There was a problem hiding this comment.
My point is to test policy only for clusters, other case should fail on this scenario. So what's your suggestion, only verify test cases for clusters policy?
There was a problem hiding this comment.
Since this is to test GlobMatch, I am thinking of focusing the test data on which fails the pattern https://github.com/*/*.git. If the test data is using repository, then it failed not due to it failed the GlobMatch.
util/rbac/rbac_test.go
Outdated
| _ = enf.SetUserPolicy(policy) | ||
|
|
||
| assert.True(t, enf.Enforce("alice", "clusters", "get", "https://github.com/argoproj/argo-cd.git")) | ||
| assert.False(t, enf.Enforce("alice", "repositories", "get", "https://github.com/argoproj/1argo-cd.git")) |
There was a problem hiding this comment.
I have the same question as above.
|
@jessesuen Wonder if you could help provide further review of this PR? Thank you! |
Signed-off-by: cezhang <c1zhang.dev@gmail.com>
There was a problem hiding this comment.
Thank you for the feature @cezhang !
The only concern is that we had another request to make the whole model configurable. However, after looking at the code, I've realized that the matchMode config and globOrRegexMatch function don't introduce any conflict. The globOrRegexMatch in future can be used in user provide policy model.
Added just one minor comment about renaming matchMode to policy.matchMode. LGTM after we agree about the name.
| # matchMode configures the matchers function for casbin. | ||
| # There are two options for this, 'glob' for glob matcher or 'regex' for regex matcher. If omitted or mis-configured, | ||
| # will be set to 'glob' as default. | ||
| matchMode: 'glob' |
There was a problem hiding this comment.
I feel that matchMode key name is a little ambiguous: Argo CD matches different things and it is good to scope it a little. Can we rename it to policy.matchMode to make it more explicit? WDYT @cezhang ?
There was a problem hiding this comment.
Good point. Commit again, please check.
Signed-off-by: cezhang <c1zhang.dev@gmail.com>
…or RegexMatch) (argoproj#7165) * feat: Make Casbin matcher configurable on runtime(globMatch(default) or RegexMatch) Signed-off-by: cezhang <c1zhang.dev@gmail.com> Signed-off-by: ciiay <yicai@redhat.com>
Related topic: #3717 #3870 #4719
As @jannfis mentioned in #3870, I implement the feature of making the casbin matcher configurable on runtime. Default mode is globMatch, so completely compatible with current code. Anyone who want to use regex mode needs to set "match.mode" in argocd-rbac-cm.
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: