Skip to content

fix: add missing ClusterRole permissions to argo-cd-server#11210

Closed
EladDolev wants to merge 2 commits intoargoproj:masterfrom
EladDolev:2_5_clusterrole
Closed

fix: add missing ClusterRole permissions to argo-cd-server#11210
EladDolev wants to merge 2 commits intoargoproj:masterfrom
EladDolev:2_5_clusterrole

Conversation

@EladDolev
Copy link

@EladDolev EladDolev commented Nov 6, 2022

Add missing ClusterRole permissions to argo-cd-server to manage Application in all namespaces

Signed-off-by: Elad Dolev dolevelad@gmail.com

Following #9755, argo-cd server needs permissions to update Applications in every namespace

Steps to reproduce:

  • Create an application in a namespace outside of the controller's namespace
  • Press "Update" from the UI
  • Getting the following error:
Unable to deploy revision: error setting app operation: error updating application "<my_app>": applications.argoproj.io "<my_app>" is forbidden: User "system:serviceaccount:<controller_namespace>:argocd-server" cannot update resource "applications" in API group "argoproj.io" in the namespace "<app_namespace>"

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).

…cation in all namespaces

Signed-off-by: Elad Dolev <dolevelad@gmail.com>
@codecov
Copy link

codecov bot commented Nov 6, 2022

Codecov Report

Base: 45.60% // Head: 45.60% // No change to project coverage 👍

Coverage data is based on head (fd76b22) compared to base (cfb2470).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11210   +/-   ##
=======================================
  Coverage   45.60%   45.60%           
=======================================
  Files         239      239           
  Lines       28973    28973           
=======================================
  Hits        13214    13214           
  Misses      13940    13940           
  Partials     1819     1819           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Signed-off-by: Elad Dolev <dolevelad@gmail.com>
@nxf5025
Copy link

nxf5025 commented Nov 7, 2022

Don't you also need create for any new applications? I just hit:

time="2022-11-07T19:47:01Z" level=warning msg="finished unary call with code PermissionDenied" error="rpc error: code = PermissionDenied desc = error creating application: applications.argoproj.io is forbidden: User \"system:serviceaccount:argocd:argocd-server\" cannot create resource \"applications\" in API group \"argoproj.io\" in the namespace \"mynamespace\"" grpc.code=PermissionDenied grpc.method=Create grpc.service=application.ApplicationService grpc.start_time="2022-11-07T19:47:00Z" grpc.time_ms=1151.638 span.kind=server system=grpc

Check this out - argoproj/argo-helm#1625. Seems like create for events in required as well.

@iam-veeramalla
Copy link
Member

iam-veeramalla commented Nov 8, 2022

Reading through the proposal I agree with @nxf5025 , I think Argo CD Server should have permissions to create as well. WDYT @EladDolev
https://github.com/argoproj/argo-cd/blob/master/docs/proposals/003-applications-outside-argocd-namespace.md

@EladDolev
Copy link
Author

Well @iam-veeramalla it makes sense to me, but from my experience I am able to create applications without this permission

@nxf5025 can you maybe share some information on how to reproduce the issue ?

@nxf5025
Copy link

nxf5025 commented Nov 8, 2022

Linking my comment from argo-helm - argoproj/argo-helm#1625 (comment)

@EladDolev
Copy link
Author

Well I swear the example works for me out of the box 😄

Maybe @jannfis or one of the maintainers can help here with some insights

Is it argocd-server that creates the Application ? or is it another principal ?

@fengshunli
Copy link
Member

@crenshaw-dev @alexef can be merged

@roy-work
Copy link

roy-work commented Mar 29, 2023

I also appear to be running into exactly the error message in this bug report, also while trying to make changes to an Application where both the Application and its resources are in a namespace different from the one ArgoCD is in.

(I am wondering if perhaps Applications are created by the argocd-application-controller role, vs. the update here is coming from the argocd-server role?)

@jannfis
Copy link
Member

jannfis commented Mar 29, 2023

Oops. I'm sorry, I have not noticed this PR before. Apologies please for coming back here so late.

We decided to not supply the argocd-server with a writing cluster role by default. Instead, we supply an example clusterrole that people can install when opting into this feature.

@EladDolev EladDolev closed this Mar 30, 2023
@roy-work
Copy link

I dug through several of the files & associated changes. What I'm not getting is why not make that the default? As it stands, it seems like w.r.t. this feature, ArgoCD ships broken out of the box, and it's up to users to discover that, and then discover the fix you've linked to, and then apply it.

I.e., your linked ClusterRole is essentially a better/more comprehensive version of the workaround I'm currently using. (I have sort of something similar in a kustomize patch.) But I'd rather not have the workaround at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants