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.

Sign up for GitHub

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

Jump to bottom

feat: RBAC csv policy can be composed by multiple configmap keys #12511

Merged
merged 4 commits into from
Jun 1, 2023
Merged

feat: RBAC csv policy can be composed by multiple configmap keys #12511

merged 4 commits into from
Jun 1, 2023

Conversation

leoluz
Copy link
Collaborator

@leoluz leoluz commented Feb 17, 2023 •
edited
Loading

This PR adds support to provide multiple RBAC csv entries in argocd-rbac-cm.
Example:

apiVersion: v1
kind: ConfigMap
metadata:
  name: argocd-rbac-cm
data:
  policy.default: role:readonly
  policy.csv: |
    g, group-admins, role:admin
    g, kubernetes:argocd-readonly, role:readonly
  policy.overlay1.csv: |
    p, role:tester, applications, *, */*, allow
    p, role:tester, projects, *, *, allow
    g, kubernetes:argocd-readonly, role:tester
  policy.overlay2.csv: |
    p, role:devops, applications, *, */*, allow
    p, role:devops, projects, *, *, allow
    g, group-devops, role:devops

This is useful to allow composing RBAC policies with multiple Kustomize overlays.

fix #8324

Signed-off-by: Leonardo Luz Almeida leonardo_almeida@intuit.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:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@codecov
Copy link

codecov bot commented Feb 17, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (60b7a31) 49.25% compared to head (6a6845e) 49.28%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #12511      +/-   ##
==========================================
+ Coverage   49.25%   49.28%   +0.02%     
==========================================
  Files         251      251              
  Lines       43521    43541      +20     
==========================================
+ Hits        21438    21458      +20     
  Misses      19949    19949              
  Partials     2134     2134
Impacted Files Coverage Δ
util/rbac/rbac.go 79.54% <100.00%> (+1.42%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leoluz leoluz marked this pull request as draft February 17, 2023 15:52
@leoluz leoluz requested a review from crenshaw-dev February 17, 2023 16:10
This was referenced Feb 17, 2023
Open
Open
@leoluz leoluz requested a review from jannfis February 17, 2023 19:46
jannfis
jannfis reviewed Feb 17, 2023
View reviewed changes
util/rbac/rbac.go Outdated
strBuilder.WriteString(p)
}
// append additional policies at the end of the csv
csvRegex := regexp.MustCompile(fmt.Sprintf("^%s.+$", ConfigMapPolicyCSVKey))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to make the suffix for the entries explicit, and then use strings.HasSuffix(s, ".csv") instead of a regexp matcher.

So what is now policy.csv.overlay1 would become policy.overlay1.csv. At least to me, this would be more straight forward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I will make the change. To be safe I will make 2 checks:
strings.HasSuffix(s, ".csv") && strings.HasPrefix(s, "policy.")
WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed the changes. Can you please take another look?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannfis ping :)

@leoluz leoluz marked this pull request as ready for review February 17, 2023 22:37
@leoluz leoluz marked this pull request as draft March 1, 2023 22:07
@JorTurFer
Copy link
Contributor

Hello,
Any update about this? This feature would be nice for our use case

@leoluz leoluz marked this pull request as ready for review May 31, 2023 15:29
@leoluz
Copy link
Collaborator Author

leoluz commented May 31, 2023 •
edited
Loading

@JorTurFer Hopefully it will be available in the next release.

jannfis
jannfis reviewed May 31, 2023
View reviewed changes
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got just something up for discussion

util/rbac/rbac.go Outdated
strBuilder.WriteString(p)
}
// append additional policies at the end of the csv
for key, value := range data {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought here: The order of keys in a map is non-deterministic afaik. Policy order may be important.

Would it make sense to:

  1. Handle entry for policy.csv specially (e.g. first to process)
  2. Process the rest of the entries sorted by keys, not randomly/non-deterministic

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle entry for policy.csv specially (e.g. first to process)

This is already the case. See line 399-400

Process the rest of the entries sorted by keys, not randomly/non-deterministic

This is a good idea. Will update the code and let you know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already the case. See line 399-400

D'oh. I would blame it on not having had enough coffee, but that would be a lie. Stupid me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all! :D
Updated the code. Please take a look.

leoluz added 4 commits May 31, 2023 16:09
@leoluz
feat: RBAC csv policy can be composed by multiple configmap keys
1f9af6f 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@leoluz
implement RBAC csv match by suffix and prefix
cd98b9e 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@leoluz
add documentation
4879084 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@leoluz
add policies in deterministic order
6a6845e 
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
jannfis
jannfis approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@leoluz leoluz merged commit 187c7d0 into argoproj:master Jun 1, 2023
@leoluz leoluz deleted the composed.rbac branch June 1, 2023 13:43
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
@leoluz
feat: RBAC csv policy can be composed by multiple configmap keys (arg…
f26a85a 
…oproj#12511)

* feat: RBAC csv policy can be composed by multiple configmap keys

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* implement RBAC csv match by suffix and prefix

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* add documentation

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* add policies in deterministic order

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

---------

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
@slammajamma28 slammajamma28 mentioned this pull request Aug 17, 2023
Open
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
@leoluz @tesla59
feat: RBAC csv policy can be composed by multiple configmap keys (arg…
7e2dc13 
…oproj#12511)

* feat: RBAC csv policy can be composed by multiple configmap keys

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* implement RBAC csv match by suffix and prefix

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* add documentation

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

* add policies in deterministic order

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

---------

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannfis jannfis jannfis approved these changes

@crenshaw-dev crenshaw-dev Awaiting requested review from crenshaw-dev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support for multiple rbac configmaps
3 participants
@leoluz @JorTurFer @jannfis