docs: Add doc for web-based terminal#891
Conversation
jparsai
requested review from
chetan-rns,
jannfis,
jgwest and
mikeshng
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds a new user-guide page and navigation entry documenting argocd-agent’s web-based terminal: end-to-end session flow (browser → Argo CD Server/principal → agent → Kubernetes exec), prerequisites, control-plane and workload RBAC setup, UI steps, and shell fallback configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ArgoCD_Server as "Argo CD Server / Principal"
participant Agent
participant KubeAPI as "Kubernetes (Pod)"
Browser->>ArgoCD_Server: Open Terminal (WebSocket)
ArgoCD_Server->>Agent: gRPC proxy session (resource-proxy)
Agent->>KubeAPI: Create pods/exec into target Pod
KubeAPI-->>Agent: Exec stream
Agent-->>ArgoCD_Server: Stream proxy over gRPC
ArgoCD_Server-->>Browser: WebSocket stream
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jparsai
force-pushed
the
web-terminal-doc
branch
from
a68d7bc to
00a569c
Compare
jparsai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/user-guide/web-terminal.md (1)
71-77: Consider documenting current TTY behavior as a terminal limitation.A short note that sessions are TTY-mode terminal sessions (with merged stdout/stderr behavior) would better align operator expectations for command output/debugging behavior.
Based on learnings: In the current implementation, TTY is always enabled for web terminal sessions and Kubernetes merges stdout/stderr streams.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-guide/web-terminal.md` around lines 71 - 77, Update the "Using the Terminal" section by adding a brief limitation note stating that the web terminal always opens sessions in TTY mode and that Kubernetes merges stdout and stderr in this configuration (so stdout/stderr are not separated); reference the "Terminal" tab/pod details flow already described and place the note immediately after step 4 so operators see the TTY/merged-stream behavior before using the shell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user-guide/web-terminal.md`:
- Around line 9-11: Two fenced code blocks are missing language identifiers;
update the blocks containing "Browser ──WebSocket──► Argo CD Server ──HTTPS──►
Principal ──gRPC──► Agent ──K8s Exec──► Pod" and "p, role:myrole, exec, create,
*/*, allow" (and the duplicate at lines ~52-54) by adding a language tag (e.g.,
text) after the opening triple backticks so the blocks become ```text ... ``` to
satisfy markdownlint MD040.
- Around line 13-17: Add a prominent security prerequisite to the Prerequisites
section warning that when using header-based auth the principal must NOT be
directly reachable; instruct readers to bind the principal to localhost (use
--listen-host=127.0.0.1) or, if localhost binding is not possible, to protect
traffic with Istio mTLS (STRICT/ISTIO_MUTUAL) and reference the detailed
networking/service-mesh guidance link for configuration steps and examples.
---
Nitpick comments:
In `@docs/user-guide/web-terminal.md`:
- Around line 71-77: Update the "Using the Terminal" section by adding a brief
limitation note stating that the web terminal always opens sessions in TTY mode
and that Kubernetes merges stdout and stderr in this configuration (so
stdout/stderr are not separated); reference the "Terminal" tab/pod details flow
already described and place the note immediately after step 4 so operators see
the TTY/merged-stream behavior before using the shell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4ff1da2-90cb-4755-b591-58405de476e3
📒 Files selected for processing (2)
docs/user-guide/web-terminal.mdmkdocs.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/user-guide/web-terminal.md (2)
13-17:⚠️ Potential issue | 🟠 MajorAdd a prominent header-based auth security prerequisite.
Prerequisites should explicitly warn that principal must not be directly reachable when header-based auth is used, and should include
--listen-host=127.0.0.1plus Istio STRICT/ISTIO_MUTUAL as the documented alternative.Suggested patch
## Prerequisites - The agent is running in managed mode. - The resource-proxy is enabled on both the principal and agent. +- **Security prerequisite (header-based auth):** Do **not** expose principal directly. + - Prefer binding principal to localhost only: `--listen-host=127.0.0.1` + - If localhost binding is incompatible with Istio sidecar redirection, enforce mesh mTLS with `PeerAuthentication` (`STRICT`) and `DestinationRule` (`tls.mode: ISTIO_MUTUAL`) so identity is established via sidecar mTLS. + - Also enforce network policies to prevent bypass paths to principal.Based on learnings: When using header-based authentication in argocd-agent principal, the service MUST listen on localhost and must not be directly reachable; if localhost binding is incompatible with Istio, document STRICT/ISTIO_MUTUAL guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-guide/web-terminal.md` around lines 13 - 17, Insert a prominent security prerequisite under the existing "Prerequisites" that warns: when using header-based authentication the principal must NOT be directly reachable (bind service to localhost), and document the required flag "--listen-host=127.0.0.1"; also add the Istio alternative guidance to use STRICT/ISTIO_MUTUAL if localhost binding is incompatible. Place this as a clearly labeled warning or header (e.g., "Header-based auth security") near the top of the Prerequisites, reference "header-based authentication", "principal", the "--listen-host=127.0.0.1" flag, and the "STRICT/ISTIO_MUTUAL" Istio mode so readers see the constraint and the alternative.
9-11:⚠️ Potential issue | 🟡 MinorAdd fenced code block languages to satisfy markdownlint (MD040).
Line 9 and Line 52 still use unlabeled fenced blocks. Please mark them as
text.Suggested patch
-``` +```text Browser ──WebSocket──► Argo CD Server ──HTTPS──► Principal ──gRPC──► Agent ──K8s Exec──► Pod@@
-
+text
p, role:myrole, exec, create, /, allowAlso applies to: 52-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-guide/web-terminal.md` around lines 9 - 11, Label the two unlabeled fenced code blocks in docs/user-guide/web-terminal.md as text by adding the language identifier "text" after the opening triple backticks for the block containing "Browser ──WebSocket──► Argo CD Server ──HTTPS──► Principal ──gRPC──► Agent ──K8s Exec──► Pod" and the block containing "p, role:myrole, exec, create, */*, allow" so they satisfy markdownlint MD040.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user-guide/web-terminal.md`:
- Line 7: Add a short note to the "web-based terminal provides `kubectl
exec`-style access to pods..." paragraph indicating that terminal sessions run
with TTY enabled (TTY is hardcoded to true in the argocd-agent web terminal
implementation), and that Kubernetes therefore merges stdout and stderr into a
single stream so users should not expect separate stdout/stderr streams when
debugging.
---
Duplicate comments:
In `@docs/user-guide/web-terminal.md`:
- Around line 13-17: Insert a prominent security prerequisite under the existing
"Prerequisites" that warns: when using header-based authentication the principal
must NOT be directly reachable (bind service to localhost), and document the
required flag "--listen-host=127.0.0.1"; also add the Istio alternative guidance
to use STRICT/ISTIO_MUTUAL if localhost binding is incompatible. Place this as a
clearly labeled warning or header (e.g., "Header-based auth security") near the
top of the Prerequisites, reference "header-based authentication", "principal",
the "--listen-host=127.0.0.1" flag, and the "STRICT/ISTIO_MUTUAL" Istio mode so
readers see the constraint and the alternative.
- Around line 9-11: Label the two unlabeled fenced code blocks in
docs/user-guide/web-terminal.md as text by adding the language identifier "text"
after the opening triple backticks for the block containing "Browser
──WebSocket──► Argo CD Server ──HTTPS──► Principal ──gRPC──► Agent ──K8s Exec──►
Pod" and the block containing "p, role:myrole, exec, create, */*, allow" so they
satisfy markdownlint MD040.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c29eb8c-8151-4c38-bb57-bb064f71c255
📒 Files selected for processing (2)
docs/user-guide/web-terminal.mdmkdocs.yml
✅ Files skipped from review due to trivial changes (1)
- mkdocs.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #891 +/- ##
==========================================
- Coverage 46.55% 46.54% -0.02%
==========================================
Files 122 122
Lines 17456 17456
==========================================
- Hits 8127 8125 -2
- Misses 8585 8587 +2
Partials 744 744 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jparsai
force-pushed
the
web-terminal-doc
branch
from
00a569c to
e8e124a
Compare
|
|
||
| ## Prerequisites | ||
|
|
||
| - The agent is running in managed mode. |
There was a problem hiding this comment.
Is this actually true? Does the pod logs only work for agents in managed mode, not in autonomous mode?
There was a problem hiding this comment.
Yes, as of now running web terminal from control-plane is not allowed. It is blocked by this condition https://github.com/argoproj-labs/argocd-agent/blob/main/principal/terminal.go#L63.
Logs do work for both modes as they are read only. The reason to now allow web terminal for autonomous mode was that control-plane then would have write/exec access of app pod running in autonomous cluster. Which was not in line with our architecture. If needed I can remove the condition and it should work for both.
docs/user-guide/web-terminal.md
Outdated
| exec.enabled: "true" | ||
| ``` | ||
|
|
||
| ### Step 2: Grant `pods/exec` Permission to `argocd-server` (Control Plane) |
There was a problem hiding this comment.
I would assume the argocd-server only needs that in a hybrid mode, for pod terminals accessed the "old" way (not through the agent). When accessing pods through the resource proxy, the argocd-server on the hub doesn't need any particular RBAC on the control plane cluster.
Assisted by: Cursor Signed-off-by: Jayendra Parsai <jparsai@redhat.com>
Signed-off-by: Jayendra Parsai <jparsai@redhat.com>
jparsai
force-pushed
the
web-terminal-doc
branch
from
e8e124a to
0fb193c
Compare
3 participants
This PR is to add user guide document for using web-based terminal in argocd-agent.
Assisted by: Cursor
Summary by CodeRabbit