-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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: additional RBAC configmaps #9976
base: master
Are you sure you want to change the base?
feat: additional RBAC configmaps #9976
Conversation
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
0c7a474
to
826f031
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #9976 +/- ##
==========================================
- Coverage 45.84% 45.84% -0.01%
==========================================
Files 227 227
Lines 27095 27381 +286
==========================================
+ Hits 12422 12553 +131
- Misses 12974 13125 +151
- Partials 1699 1703 +4 ☔ View full report in Codecov by Sentry. |
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
e7413cc
to
577dea2
Compare
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
ba05ebb
to
17876a8
Compare
// newInformers returns an informer which watches updates on the rbac configmap | ||
func (e *Enforcer) newAdditionalInformer() cache.SharedIndexInformer { | ||
tweakConfigMap := func(options *metav1.ListOptions) { | ||
options.LabelSelector = "argocd.argoproj.io/cm-type=additional-rbac" |
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.
What happens if I remove the label? Will the policy be dropped from the combined policy?
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 are using v1.NewFilteredConfigMapInformer
with our specified labelselector, it will drop the policy from combined policy:
time="2022-11-21T23:04:23Z" level=info msg="RBAC Additional ConfigMap 'argocd-rbac-cm-extra' deleted" security=2
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
I thought the order of resolution of casbin rules was significant. How is this merging this in a predictable order? |
Hm, according to our Casbin model, we don't use any priority model. I think our model uses simple allow/deny patterns, with deny overriding any allow statement. But we should verify that. |
This change should also be reflected in the Probably
|
// newAdditionalInformer returns an informer which watches updates on the rbac configmap | ||
func (e *Enforcer) newAdditionalInformer() cache.SharedIndexInformer { | ||
tweakConfigMap := func(options *metav1.ListOptions) { | ||
options.LabelSelector = "argocd.argoproj.io/cm-type=additional-rbac" |
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.
Should additional-rbac
become a constant?
And also, I'm in favor for dropping additional
and just having it named rbac
@notfromstatefarm when is this expected to be merged? Thanks |
@notfromstatefarm sorry to ping again, what's needed here? |
@v1ctorrhs Jann's comments need to be addressed before this can be merged. I think @notfromstatefarm has been too busy to work on this PR. Do you think you'd be able to pick it up and finish off the last few items? |
This PR is just what we are looking for! 😍😍😍😍 |
@notfromstatefarm want to close this for now until you (or someone else) has time to pick it back up? |
greetings everyone, i am willing to pick the PR from the current state and try to complete requirements if that's ok for @notfromstatefarm. i will start working on your fork btw. |
@bilalcaliskan sounds great! Let me know if you need any help. |
by the way i am currently working on that PR on https://github.com/bilalcaliskan/argo-cd/tree/feat/additional-rbac-configmaps which is my own fork from @crenshaw-dev Is it OK to implement that feature on different PR by marking |
We are facing a similar issue at Intuit and we require something similar. However I don't understand the reason of adding an additional configmap for that. I implemented a similar feature in a different PR using additional predefined keys in the same configmap. Please take a look: #12511 The advantages of that approach are:
The cons are:
|
I like that approach, @leoluz. It's simple enough to be useful for me. As a con, I'd like to add: Requires some kind of config management tool such as Kustomize to leverage, as opposed to the "JBOCM" (Just a bunch of ConfigMaps) approach of the original PR. |
@leoluz Oh. And another one would maybe the etcd size limit for resources (thinking in terms of really huge amounts of RBAC rules to fit into a single ConfigMap resource). |
@jannfis Thank you! Updated my previous message with both cons. btw.. Is |
Hello, |
Signed-off-by: notfromstatefarm 86763948+notfromstatefarm@users.noreply.github.com
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:
Closes #8324
This PR implements 'additional configmaps' for RBAC. Any configmaps with the label
argocd.argoproj.io/cm-type=additional-rbac
will be used and watched for changes. Thepolicy.csv
key is merged from all of them. This allows users to easily deploy new RBAC rules by simply deploying additional ConfigMaps.This PR also introduces a new concept suggested by @crenshaw-dev: a standardized
security
field on logs where the value indicates severity/level of paranoia.I've tested this locally and it works like a charm!