feat: support pod exec terminal via websockets

[yeya24]

Signed-off-by: Ben Ye ben.ye@bytedance.com

fixes #4351

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

Screenshot:

[terrytangyuan]

This is pretty cool! Thanks for your contribution! Do you have a screenshot of what this looks like?

[yeya24]

This is pretty cool! Thanks for your contribution! Do you have a screenshot of what this looks like?

I have updated the screenshot. It is working in our env and we have been using it for weeks. Still some todos for me:

1. Add a new EXEC rbac to control terminal access
2. More tests and comments

[chenyuf4]

There are still issues of argo-ui (issue: argoproj/argo-ui#183). When I update latest version of argo-ui and it doesn't pass the front end check https://github.com/argoproj/argo-cd/runs/5732956547?check_suite_focus=true.

The reason is that xterm/FitAddon library need to access window object in its implementation. However, the test environment of Argocd is node.js, which has no window object. Here is a stack overflow post discussing the same issue https://stackoverflow.com/questions/66096260/why-am-i-getting-referenceerror-self-is-not-defined-when-i-import-a-client-side.

[alexmt]

@chenyuf4 , @yeya24 I've sent a small PR to your fork with a jest configuration change: yeya24#132

It introduces a self global variable while running unit tests and fixes the error.

[chenyuf4]

@alexmt Thanks for your update. I update your change locally, but it still doesn't pass the yarn test on argocd. Even I switch to master branch of argocd, the same error happens when I do yarn test.

[alexmt]

Sorry , looks like this error is intermittent. Working on next PR.

[alexmt]

@chenyuf4 can you please try to apply these changes: alexmt@997fbe7

[alexmt]

Looks like the tests were successful this time. You can fix some linter errors by running yarn lint --project . --fix

[codecov]

Codecov Report

Merging #8905 (a5f2236) into master (c7ff388) will decrease coverage by 0.30%.
The diff coverage is 11.81%.

@@            Coverage Diff             @@
##           master    #8905      +/-   ##
==========================================
- Coverage   45.63%   45.33%   -0.31%     
==========================================
  Files         217      219       +2     
  Lines       25696    25870     +174     
==========================================
+ Hits        11727    11728       +1     
- Misses      12337    12511     +174     
+ Partials     1632     1631       -1     

Impacted Files	Coverage Δ
cmd/argocd/commands/admin/settings_rbac.go	23.24% <ø> (ø)
server/application/terminal.go	3.60% <3.60%> (ø)
server/application/websocket.go	8.33% <8.33%> (ø)
server/application/application.go	30.50% <13.33%> (+0.37%)	⬆️
server/server.go	54.07% <43.33%> (-1.47%)	⬇️
server/rbacpolicy/rbacpolicy.go	82.35% <100.00%> (ø)
applicationset/services/scm_provider/github.go	63.52% <0.00%> (-17.65%)	⬇️
applicationset/services/scm_provider/utils.go	88.50% <0.00%> (+4.59%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ff388...a5f2236. Read the comment docs.

[yeya24]

@alexmt Thanks for the help. I think this pr should be ready for review now. All CI checks passed.

[crenshaw-dev]

@yeya24 will get back to this ASAP. Sorry, been a bit swamped. Definitely want this in 2.4 as well.

[crenshaw-dev]

My remaining thoughts:

1. It would be nice to have some unit test coverage on terminal.go and websocket.go.
2. I think the docs should probably point out that exec can be a powerful privilege - for example, exec grants access to any service account tokens living on accessible pods. Admins should already be aware of that, but it wouldn't hurt to remind them.
3. I'd love to have a UI expert review. I don't know how to write React decently well, much less securely. :-) Going to request a review from @rbreeze.

I'm really happy with this PR and don't think there's anything that would stop us from merging this before 2.4. But I'd love to get @yeya24 and @alexmt 's thoughts on those points.

[alexmt]

Just one nitpick about terminal package location. Not a blocker to merge. Thank you @yeya24 !

Extremely awesome PR !

[crenshaw-dev]

Adding my approval, will leave unmerged for the moment in case Remington gets a chance to review early this week. If merge without additional tests / security docs, I'll create follow-up issues for those. 🙂

[rbreeze]

LGTM! Exciting feature!

[crenshaw-dev]

@yeya24 it seems my attempted merge messed up your beautiful PR. Can you resolve conflicts? Might just want to reset HEAD~1 to get rid of my messed up commit. :-P

[yeya24]

@crenshaw-dev I just fixed it and thanks for the update. Are we good to go now? 😄

[alexmt]

Merging as soon as CI is green!

[jmateusousa]

Could this exec function be available in version 2.3.0? or is it really necessary to update to version 2.4...?

[crenshaw-dev]

@jmateusousa it's necessary to update. The feature is significant enough and introduces enough changes to how RBAC works that it wouldn't be a good idea to backport.

[sspreitzer]

I am running ArgoCD 2.4.2 via the helm chart and seem not be able to enable this feature. I am using the builtin rbac policy and assigned role:admin.
Is there any documentation needed about this topic?

[crenshaw-dev]

@sspreitzer you might still need to turn on the exec.enabled config flag. https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#exec-resource

[sspreitzer]

@crenshaw-dev, as you described, enabling the exec.enabled config flag did enable the feature. Thank you!
https://github.com/argoproj/argo-helm/blob/445b2757f5adb4c2144dee4b139ffe28c428f2fc/charts/argo-cd/values.yaml#L1236