argo-cd icon indicating copy to clipboard operation
argo-cd copied to clipboard

docs(proposal): decoupling app sync from control plane user w/ impersonation

Open anandf opened this issue 1 year ago • 5 comments

Proposal for decoupling the application sync process from the Application controller service account.

Addresses issue https://github.com/argoproj/argo-cd/issues/7689 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:

  • [x] 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.
  • [x] The title of the PR states what changed and the related issues number (used for the release note).
  • [x] The title of the PR conforms to the Toolchain Guide
  • [ ] 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.
  • [x] Optional. My organization is added to USERS.md.
  • [x] 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).
  • [ ] My new feature complies with the feature status guidelines.
  • [ ] I have added a brief description of why this PR is necessary and/or what this PR solves.

Please see Contribution FAQs if you have questions about your pull-request.

anandf avatar Jun 28 '23 14:06 anandf

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (55713b3) 49.26% compared to head (d2c3cdf) 49.25%. Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14255      +/-   ##
==========================================
- Coverage   49.26%   49.25%   -0.02%     
==========================================
  Files         274      274              
  Lines       48158    48170      +12     
==========================================
- Hits        23727    23724       -3     
- Misses      22085    22099      +14     
- Partials     2346     2347       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 28 '23 15:06 codecov[bot]

@anandf the proposal as it stands is 👨‍🍳 💋, LGTM! Could you remove the docs/decouple-application-sync-user-using-impersonation doc since it was a bit confusing to follow and the actual proposal is located in docs/proposals/decouple-application-sync-user-using-impersonation?

blakepettersson avatar Jul 12 '23 15:07 blakepettersson

A question on the proposal, how would this help with fixing issues like https://github.com/argoproj/argo-cd/issues/15027, https://github.com/argoproj/argo-cd/pull/10897, https://github.com/argoproj/argo-cd/issues/9606 and https://github.com/argoproj/argo-cd/issues/9515. I see this proposal being mentioned as the fix, but I can't figure out how as all examples point to ArgoCD managing the same cluster where it is also deployed and not remote clusters. I think https://github.com/argoproj/argo-cd/pull/12755 goes into a better direction to fix the mentioned issues, as long as Argo can differentiate clusters with the same cluster url.

Isn't it also assuming that ArgoCD will have the permission to impersonate all the needed service accounts which might be difficult in multi-tenant clusters where the cluster is managed by a team different than the team managing ArgoCD. The examples in the docs do not really setup the permissions, so am I missing something here? Don't you have to grant the impersonate verb to ArgoCD so it can impersonate other SAs?

ljupchokotev avatar Aug 29 '23 15:08 ljupchokotev

A question on the proposal, how would this help with fixing issues like #15027, #10897, #9606 and #9515. I see this proposal being mentioned as the fix, but I can't figure out how as all examples point to ArgoCD managing the same cluster where it is also deployed and not remote clusters. I think #12755 goes into a better direction to fix the mentioned issues, as long as Argo can differentiate clusters with the same cluster url.

I see the issues #15027, #10897, #9606 and #9515 are all about managing multiple cluster URLs with different namespace. Probably one namespace per tenant and each tenant having its own set of cluster secrets.

Though this proposal might not directly address, it might help to remove the need for adding multiple cluster secrets per namespace. You can login to the remote cluster as cluster-admin user, for all the tenants, and for sync operations each tenant (cluster + namespace combination) would use a separate service account. That way you can manage with a single cluster secret and instead control the access to each application or tenant using appropriate RBAC policies. I can add an example of how to manage a remote cluster.

anandf avatar Sep 04 '23 00:09 anandf

You can login to the remote cluster as cluster-admin user

That is exactly the problem in our case and I assume for a lot of other people judging by the feedback on the linked issues. I do not own the multi-tenant cluster, so I am not able to get a cluster-admin service account to setup in Argo. Tenants get a service account that has privileges in their own namespace only. Due to this, I don't see this proposal as a solution for that particular issue at least. The proposal in https://github.com/argoproj/argo-cd/pull/12755 (or similar) should still be implemented as that allows for least-privilege principle to be implemented in an easy way. Impersonation can still be used as needed, whether with a cluster-admin SA or a namespace SA.

ljupchokotev avatar Sep 04 '23 07:09 ljupchokotev