From ac3f616784c0c0e52ad71e1ecedbeeb9314cfc22 Mon Sep 17 00:00:00 2001 From: omattsson Date: Fri, 30 Oct 2026 21:53:41 +0100 Subject: [PATCH 1/2] feat(stack): add stack exec (#179) stackctl stack exec [-c CONTAINER] -- COMMAND [ARGS...] runs one command in a container of the stack through the server (GET /api/v1/stack-instances/:id/exec, k8s-stack-manager v0.15.0 or later) and streams its stdout and stderr unchanged. No stdin, no TTY. - The command goes as an argument list in the first message, never in the URL. Header auth only; the stream dial never falls back to ?token=. - Exit codes: the remote exit code; 124 for the server time limit (4001); 130 for Ctrl-C or SIGTERM (a second Ctrl-C exits at once); 255 for an error of stackctl or the server; 1 for invalid flags or arguments. ExitError in cmd/exitcode.go, main.go exits with its code. - --insecure and an http:// URL to a non-loopback host are refused unless --allow-insecure-stream is set (shared with port-forward). - Server text is sanitized; --debug shows the started frame, never output. - A Ctrl-C during the WebSocket handshake now ends the dial at once (closeOnCancel), also for port-forward. - cluster create/update --allow-exec and --allow-port-forward set the per-cluster switches; cluster get shows them. - swagger.json refreshed with the exec route and the cluster switches. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 12 +- README.md | 16 + cli/cmd/agent_guide.md | 16 +- cli/cmd/agent_guide_test.go | 32 + cli/cmd/cluster.go | 32 +- cli/cmd/cluster_test.go | 113 +- cli/cmd/exitcode.go | 37 + cli/cmd/root.go | 27 +- cli/cmd/stack_exec.go | 287 +++++ cli/cmd/stack_exec_test.go | 889 ++++++++++++++ cli/cmd/stack_portforward.go | 54 +- cli/cmd/stack_portforward_test.go | 30 + cli/main.go | 10 + cli/pkg/client/exec.go | 543 ++++++++ cli/pkg/client/exec_test.go | 1498 +++++++++++++++++++++++ cli/pkg/client/websocket.go | 194 ++- cli/pkg/types/types.go | 26 + cli/test/contract/path_contract_test.go | 94 ++ cli/test/contract/testdata/swagger.json | 139 ++- cli/test/e2e/cli_e2e_test.go | 120 ++ 20 files changed, 4109 insertions(+), 60 deletions(-) create mode 100644 cli/cmd/exitcode.go create mode 100644 cli/cmd/stack_exec.go create mode 100644 cli/cmd/stack_exec_test.go create mode 100644 cli/pkg/client/exec.go create mode 100644 cli/pkg/client/exec_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 853997e..976c8e5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,6 +33,8 @@ cli/ # Go module root (github.com/omattsson/stackctl/cl stack_action.go # stack action list/run/log (custom actions of the hooks config; job log polling with backoff) stack_followers.go # stack follow/unfollow (in-app notifications; not the --follow log flag) stack_portforward.go # stack port-forward (parsePortSpec, --address, listeners with port fallback, one tunnel per local connection); addrinuse_*.go: EADDRINUSE per OS + stack_exec.go # stack exec (args after '--', -c, signals: execSignals/execForceExit, exit code mapping) + exitcode.go # ExitError{Code, Err}: main.go exits with Code (stack exec); exit codes 124, 130, 255 ttl.go # --ttl/--stop-at/--until parsing, TTL limit check (GET /api/v1/ui-config, 30-day fallback), expiry text server.go # server info (ping, ui-config; public routes, no credentials) template.go # template list/get/create/update/clone/publish/unpublish/versions/instantiate/quick-deploy/delete @@ -41,7 +43,7 @@ cli/ # Go module root (github.com/omattsson/stackctl/cl override.go # override list/get/set/unset/delete, branch overrides (list/get/set/delete), quota overrides (quota set merges with GET, --replace) bulk.go # bulk deploy/stop/clean/delete, bulk template publish/unpublish/delete (--ids flag or positional args) git.go # git providers/branches/validate - cluster.go # cluster list/get/create/update/delete/set-default/test-connection/health/nodes/namespaces/utilization, quota get/set/delete, shared-values list/set/update/delete (update: GET list, merge, PUT the full record) + cluster.go # cluster list/get/create/update/delete/set-default/test-connection/health/nodes/namespaces/utilization (create/update --allow-port-forward/--allow-exec: per-cluster stream switches; update sends false), quota get/set/delete, shared-values list/set/update/delete (update: GET list, merge, PUT the full record) analytics.go # analytics overview/templates/users apikey.go # apikey list/create/revoke audit.go # audit log list/export @@ -65,6 +67,7 @@ cli/ # Go module root (github.com/omattsson/stackctl/cl auth.go # Session renewal (refresh token, 401 retry), login/refresh/logout calls websocket.go # WebSocket client (watch, deploy log streaming); stream dial without ?token= (dialWSOpts), handshake error codes, close codes, keepalive portforward.go # DialPortForward and Tunnel (byte copy, half-close handling, keepalive) + exec.go # Exec (start message, stdout/stderr frames, exit frame, close codes), ValidateExecCommand types/ types.go # Client-side structs matching API responses config/ @@ -111,6 +114,8 @@ cli/ # Go module root (github.com/omattsson/stackctl/cl - `--quiet`: IDs only, one per line (pipeable to `xargs`) - `printer.PrintMessage` (text for people) writes to `printer.ErrWriter` (stderr) when `printer.MachineReadable()` (`-o json|yaml` or `-q`; not the table or a custom format), so stdout holds only data. An empty list prints `[]` in those modes, not a message. deploy/stop/clean/rollback end in `printOperation` (202 response for json/yaml, log ID for `-q`); with `--follow` in those modes `followOperation` sends the log to stderr (`followLogWriter`) and prints the instance (GET) or the log ID; `stack logs -f` prints the log as without `--follow` +**Exit codes**: an error exits 1 (`main.go`). An `*ExitError{Code, Err}` (`cmd/exitcode.go`, found with `errors.As`) exits with `Code`; when `Err` is not nil, `main.go` prints the returned error (the whole wrapped chain; `ExitError.Error()` is the text of `Err`). Only `stack exec` uses it: the remote exit code N (no message), 124 server time limit, 130 Ctrl-C or SIGTERM, 255 any other error after `RunE` starts (also the `--insecure`/`http://` refusal, an exit code outside 0..255, an exit frame without a valid `exit_code`); argument errors stay 1. A remote command can return any code, including 1, 124, 130 and 255; stackctl's own codes come with a stackctl message on stderr (remote stderr goes to the same stream, so read the message, not just its presence). stackctl itself uses 1 only for invalid flags or arguments. Ctrl-C closes the connection and exits 130 within a few seconds (a second Ctrl-C exits at once); when the command had already ended, stderr names its exit code. The remote process can keep running. Plugins exit with their own code (`plugins.go`). + **Destructive operations**: Commands that delete or clean resources must prompt for confirmation: `if err := confirmAction(cmd, msg); err != nil { return err }`. `--yes` flag skips the prompt. A declined prompt returns `errAborted` (exit 1, "aborted: not confirmed, nothing changed" on stderr), so scripts can tell that nothing changed. ## HTTP Client Pattern @@ -135,7 +140,7 @@ cli/ # Go module root (github.com/omattsson/stackctl/cl **Config-free commands**: `version`, `agent-guide` and `completion` skip config file loading and work even if the config is missing or corrupted. Mark such a command (and its subcommands) with the annotation `skipConfigAnnotation` (`skipsConfig` in `root.go`). -**Insecure mode**: When `--insecure` is active, a warning is printed to stderr. +**Insecure mode**: When `--insecure` (or the `insecure` setting of the context) is active, a warning is printed to stderr. `effectiveInsecure()` (`root.go`) decides it for the requests of stackctl itself (TLS, the warning, the stream refusals). The plugin environment has its own rule: `pluginEnv` sets `STACKCTL_INSECURE` from an explicit `--insecure[=false]`, and `contextEnv` (`plugins.go`) adds `1` for a context with `insecure: true` only when the variable is not set yet. **Base URL**: From `--api-url` flag, `STACKCTL_API_URL` env, or config file `api-url` key. @@ -191,10 +196,11 @@ All API calls go to `/api/v1/*`. Key route groups: - `GET /:id` returns `values_drift` (v0.6.0+, not on lists); the rollback response has `values_drift` (`*bool`, only for `target_log_id`) and `warning`; the clone response adds `warning` (quota override not copied; v0.7.0+). `stack rollback -o json|yaml` prints the response. `stack get`, `stack rollback` and `stack clone` write these warnings to stderr as `Warning: ` (`printServerWarning`, sanitized). `stack get` fills `warning` with `types.ValuesDriftWarning` when the server sends `values_drift` without text, so `-o json` has both fields. - `owner_username`, `cluster_name`, `definition_name` (k8s-stack-manager#470, v0.7.0+) are optional; tables show the name (`displayName`) or "name (id)" (`displayNameWithID`), else the ID. No extra API calls. - `PUT /:id/quota-overrides` replaces the whole override. `override quota set` does GET (only 404 "Instance quota override not found" = no override), applies the given flags (`""` clears a string field; pod_limit only via `--replace` or delete; `--pod-limit 0` = no limit) and PUTs; `--replace` skips the GET. An empty result is refused (points to `override quota delete`). GET and PUT are not atomic: a concurrent change is overwritten. Owners without admin/devops get 403 above the cluster quota (v0.7.0+); a value equal to the stored value passes. +- `GET /api/v1/stack-instances/:id/exec?chart=&container=` — `stack exec` (omattsson/stackctl#179, k8s-stack-manager#723, server PR #725, merged; k8s-stack-manager v0.15.0 or later, `client.ExecServerVersion`). Same dial, header auth and `{"error","code"}` refusals as port-forward (`WSHandshakeError` with `Op` "exec" and `Container`; exec hints in `execHint`: `container_not_allowed`, `cluster_exec_disabled`, `chart_not_found`, `container_not_found`, `no_ready_pod`, `exec_limit`, `service_account_token`, `token_check_forbidden`, 400 `invalid_chart` / `invalid_container` (the server checks the names; no DNS-1123 check in stackctl)). `container` is left out without `-c`. The command goes only in the first text frame `{"type":"start","command":[...]}` (`types.ExecStartMessage`; the server closes 4408 without it in 10 s). Fixed rules checked before the dial (`client.ValidateExecCommand`): 1 to 256 arguments, first not empty, no NUL; the length limit (`STACK_EXEC_MAX_COMMAND_LENGTH`, default 4096) is not exposed, so a long command gets close 4400. Server frames (`types.ExecServerMessage`): `started` (pod, container; the only thing `--debug` logs), binary frames with stream byte 1 stdout / 2 stderr (written unbuffered; unknown bytes, empty frames and unknown types ignored), `exit` (`exit_code`) + close 1000, or `error` (`message`) + 4502 (pod stream) / 1011 (internal). One reader goroutine, read limit 1 MiB, the keepalive of port-forward. Close codes: 4001 → `ErrExecTimeLimit` (no exit frame); others → `*ExecClosedError` (1008 unwraps to `WSClosedError` but has its own text: "token expired" never says "run the command again" while it can still run; 4400 "the server refused the command (too long or not valid)"; 1009 "the command is too long for the server"). The server sends `started` before it runs the command and its frames arrive in order, so a close frame without `started` or output (a binary frame) before it ends with "; the server did not start the command"; with `started` or output, and for a lost connection (code 0, gorilla 1006: plain "exec: connection lost; the command can still run on the server", no gorilla text) it ends with "; the command can still run on the server". A frame over the 1 MiB read limit (`websocket.ErrReadLimit`) has its own text. Ctx cancel: close 1000, wait up to 2 s (`CancelWait`) for an exit frame, else `*ExecCanceledError{Started}` (`errors.Is` matches `ctx.Err()`; `Started` false only for a cancel before, during or right after the dial (checked before the start frame, so the command is never sent) or a server close frame without `started` or output; `stack exec` then says "interrupted before the command started" (also for an interrupted run that ends with an `*ExecClosedError` with a close code and without `Started`, with the server text: `interruptedExit`), else "interrupted; the command can still run on the server", both exit 130). Stream dials (`dialStream`, also port-forward) close the connection when ctx ends during the handshake (`closeOnCancel`: gorilla v1.5.3 honours only the ctx deadline, so a held upgrade would wait for the 45 s handshake timeout). The second Ctrl-C writes "interrupted" from a goroutine to the raw stderr (never through the `syncWriter`, which the reader can hold in a blocked remote stderr write) and waits at most 100 ms (`writeNoteNoWait`) before `execForceExit(130)`. Remote output is passed through unchanged, terminal escape sequences included (help, README and agent guide say so). Never reconnects. The route is in the vendored swagger, so the normal path contract check covers it. `pendingRoutes` in `test/contract/path_contract_test.go` is empty; it is for a route that exists only in an open backend PR (keyed by the literal, with the PR and the expected swagger path; the literal must match that path, and the test fails as soon as the vendored swagger has it). - `/api/v1/audit-logs` — list/export; `--action` and `--entity-type` are exact matches (`deploy`, `stack_instance`, …; v0.7.0+, see `auditFilterValuesHelp`). Older servers: HTTP method + last route segment (`create` / `deploy`) - `/api/v1/users` — list/delete/disable/enable/password (admin). `PUT /:id/role` `{"role"}` (k8s-stack-manager v0.8.0+, `user set-role `, user by ID or username via `resolveUserID`): response `id`, `old_role`, `new_role`, `changed`; 403 own role / caller not admin, 409 SSO user / last admin, 400 invalid role; a plain 404 (no route) maps to the v0.8.0 upgrade hint (`setRoleError`) - `/ws` — the server closes a socket with code 1008 and reason `session revoked` or `token expired`. `Client.Watch` returns a `WatchStream`; `Err()` gives a `*WSClosedError` ("Connection closed by the server: . Log in again."). `stack watch` exits non-zero on it; for `token expired` it calls `Client.RenewSession` (forced renewal, false without refresh token) and reconnects, at most once per 30 s (`stackWatchMinReconnect`), then with `--id` GETs each pending instance (`watchCatchUp`) so a terminal status in the gap counts. Ctrl-C during renewal or dial exits 0. Another connection loss: `--id` with pending instances exits non-zero ("connection lost before all instances reached a terminal status"), else a stderr note and exit 0. `StreamDeploymentLogs` returns the same `WSClosedError`; `--follow` (`followLogsCtx`, op deploy/stop/clean/rollback, "" for `stack logs -f`) uses the same `renewAfterClose`; it reads the instance after the first connect (the server sets deploying/stopping/cleaning before the 202; not for `logs -f`) and after each reconnect (`StreamOptions.AfterSubscribe`). A normal close (code 1000, stream status `unknown`) reads the instance (`followEndError`): a final status counts, else it fails like a lost connection. When it cannot follow to the end it says that the operation continues on the server and points to `stack watch --id` / `stack status` (never "run again"); errors name the operation ("rollback failed (status error): ..."). Terminal statuses live in `client.IsTerminalStatus` (running, stopped, draft, error, partial, failed) and `client.IsFailedStatus` (error, partial, failed → exit 1) -- `GET /api/v1/stack-instances/:id/port-forward?service=&port=` — `stack port-forward` (omattsson/stackctl#178, k8s-stack-manager#722). WebSocket upgrade, auth only by header (`dialWSOpts` with `stream`: no `?token=` retry, a 401 gets one forced `RenewSession` and one more dial; no `Origin` header). Refusals are HTTP statuses with `{"error","code"}` (`WSHandshakeError.Code`, `StreamCode*`): `FeatureDisabled()` (404 `feature_disabled`), `NoRoute()` (404 without a code: older server; hint `client.PortForwardServerVersion`), `UpgradeDropped()` (2xx, or 400 without a code or `upgrade_required`: a proxy), `Fatal()` (400, 401, 403, 404, 409 stop the command; 429, 5xx and network errors close one connection; 3 handshake 429s in a row, with no successful upgrade between them (`portForwardMax429`, one counter for all tunnels), stop the command). After the upgrade: binary frames of at most 32 KiB both ways (server read limit 64 KiB), no half-close. `Client.Tunnel`: one reader and one writer goroutine, pings and the close through `WriteControl`; pings from their own goroutine every 30 s (a waiting ping cannot delay a cancel), read deadline 75 s moved by data, ping or pong; a local write deadline of 75 s; after a local EOF the tunnel copies server data to local until the server closes, a local write fails, or `DrainTimeout` (2 s) passes without server data, and only then sends close 1000; a server close 1000 does `CloseWrite` on the local socket. A 401 on the stream dial renews with the refused token (`renewSession(stale)`), so parallel 401s send one refresh. Close codes (`Close*` constants, `CloseCodeText`): 1001 shutdown, 1008 (`StreamClosedError.Fatal`, unwraps to `WSClosedError`; reason `session revoked` → login advice, `access revoked` (`CloseReasonAccessLost`) → stack stopped, deleted or cleaned, or access changed), 1011, 4001 time limit, 4002 idle, 4502 pod stream error. `--insecure`, and an `http://` API URL to a host that is not loopback (`plaintextRemoteURL`), are refused unless `--allow-insecure-stream`. At most 16 open tunnels (`portForwardConnLimit`); a further connection is closed at once. The non-loopback warning checks the bound address (`warnNonLoopback`). SIGTERM ends the command like Ctrl-C (`portForwardSignals`; `stack_portforward_signal_test.go` sends a real SIGTERM to a subprocess, not on Windows). A fatal tunnel error is not repeated in the per-connection close line. +- `GET /api/v1/stack-instances/:id/port-forward?service=&port=` — `stack port-forward` (omattsson/stackctl#178, k8s-stack-manager#722). WebSocket upgrade, auth only by header (`dialWSOpts` with `stream`: no `?token=` retry, a 401 gets one forced `RenewSession` and one more dial; no `Origin` header). Refusals are HTTP statuses with `{"error","code"}` (`WSHandshakeError.Code`, `StreamCode*`): `FeatureDisabled()` (404 `feature_disabled`), `NoRoute()` (404 without a code: older server; hint `client.PortForwardServerVersion`), `UpgradeDropped()` (2xx, or 400 without a code or `upgrade_required`: a proxy), `Fatal()` (400, 401, 403, 404, 409 stop the command; 429, 5xx and network errors close one connection; 3 handshake 429s in a row, with no successful upgrade between them (`portForwardMax429`, one counter for all tunnels), stop the command). After the upgrade: binary frames of at most 32 KiB both ways (server read limit 64 KiB), no half-close. `Client.Tunnel`: one reader and one writer goroutine, pings and the close through `WriteControl`; pings from their own goroutine every 30 s (a waiting ping cannot delay a cancel), read deadline 75 s moved by data, ping or pong; a local write deadline of 75 s; after a local EOF the tunnel copies server data to local until the server closes, a local write fails, or `DrainTimeout` (2 s) passes without server data, and only then sends close 1000; a server close 1000 does `CloseWrite` on the local socket. A 401 on the stream dial renews with the refused token (`renewSession(stale)`), so parallel 401s send one refresh. Close codes (`Close*` constants, `CloseCodeText`): 1001 shutdown, 1008 (`StreamClosedError.Fatal`, unwraps to `WSClosedError`; reason `session revoked` → login advice, `access revoked` (`CloseReasonAccessLost`) → stack stopped, deleted or cleaned, or access changed), 1011, 4001 time limit, 4002 idle, 4502 pod stream error. `--insecure`, and an `http://` API URL to a host that is not loopback (`plaintextRemoteURL`), are refused unless `--allow-insecure-stream` (`checkStreamTransport`, shared with `stack exec`; "insecure" is `effectiveInsecure()` in `root.go`, the same check as the TLS warning and `applyInsecureTLS`). At most 16 open tunnels (`portForwardConnLimit`); a further connection is closed at once. The non-loopback warning checks the bound address (`warnNonLoopback`). SIGTERM ends the command like Ctrl-C (`portForwardSignals`; `stack_portforward_signal_test.go` sends a real SIGTERM to a subprocess, not on Windows). A fatal tunnel error is not repeated in the per-connection close line. - `/api/v1/admin/orphaned-namespaces` — `orphaned list [--details]`; `DELETE /:ns?confirm=` is needed for a namespace without the label managed-by=k8s-stack-manager (409 without it; `orphaned delete --confirm`). `test/contract/path_contract_test.go` checks every `/api/v1` literal of `pkg/client` and `cmd` against the vendored swagger - `POST /api/v1/auth/logout-all` — `logout --all`: uses the session token of the context when a token file exists (also when an API key is set), else the API key; a 401 gets one forced `RenewSession` and one retry. On failure the command stops and keeps the token file; on success the normal logout follows. - `GET /api/v1/dashboard` — `dashboard`: covers all users; `failing_instances` has status error only, `expiring_soon` the next hour. For the role `user`, `clusters` has ID and name only (no health): the table hides clusters without `health_status`. diff --git a/README.md b/README.md index fdbe4ae..52d9f87 100644 --- a/README.md +++ b/README.md @@ -256,6 +256,10 @@ stackctl stack port-forward my-app mysql 3306 # then: mysql -h 127.0.0.1 -P stackctl stack port-forward my-app frontend 80 # local port 10080 stackctl stack port-forward my-app mysql 0:3306 -q # any free port; stdout: 127.0.0.1: +# Run one command in a container of a chart (exit code: the code of the command) +stackctl stack exec my-app backend -- bin/console cache:clear +stackctl stack exec my-app backend -c php -- sh -c 'ls -la /var/www | head' + # Custom actions of the hooks config (for example a database refresh) stackctl stack action list my-app stackctl stack action run my-app refresh-db --param market=dk --yes --wait @@ -310,6 +314,13 @@ stackctl stack rollback my-app --target-log - The server can refuse API keys for port-forward: then run `stackctl login` and remove the API key from the context or the environment. - Security: anyone who can reach the local port uses your access to the stack. On a shared machine, other local users can connect to it. Web pages in your browser can reach a forwarded HTTP service through DNS rebinding: stop the command when you do not need it. An address that is not loopback opens the port to the network (stackctl writes a warning). `--insecure` is refused, because a man in the middle could read the tunnel data. An `http://` API URL is refused for the same reason, except for `localhost`, `127.0.0.1` and `[::1]`. `--allow-insecure-stream` accepts that risk. +`stack exec [-c CONTAINER] -- COMMAND [ARGS...]` runs one command in a container of a chart of the stack through the server (`GET /api/v1/stack-instances/:id/exec`). You need no kubeconfig. The server chooses the pod (the first running and ready pod of the chart) and, without `-c`, the first container that allows exec. The `--` is required: the words after it are the command and its arguments, sent as a list without a shell (stackctl reads no flags after it). Use `-- sh -c '...'` for pipes, variables or globs. The command gets no stdin and no TTY; its stdout and stderr go unchanged to stdout and stderr. `-o` and `-q` are ignored. stackctl never connects again: a lost connection ends stackctl (exit 255). The server must enable exec, and the cluster of the stack must allow it: an admin runs `stackctl cluster update --allow-exec` (the same for port-forward with `--allow-port-forward`); `cluster get` shows both switches. + +- Exit codes: the exit code of the remote command (0 to 255, no message); 124 when the server time limit ends the command (close 4001); 130 for Ctrl-C or SIGTERM; 255 for an error of stackctl or the server (stack, chart or container not found, no access, no ready pod, a refused command, a lost connection, the refusal of `--insecure` or of an `http://` API URL, an exit code outside 0 to 255, an exit frame without a valid `exit_code`; the message is on stderr); 1 for invalid flags or arguments (no `--`, no command, more than 256 arguments, an empty program, a NUL byte). A remote command can return any code, including 1, 124, 130 and 255; stackctl's own codes come with a stackctl message on stderr (remote stderr goes to the same stream, so read the message, not just its presence). stackctl itself uses 1 only for invalid flags or arguments. +- Ctrl-C closes the connection and exits 130 within a few seconds (a second Ctrl-C exits at once); when the command had already ended, stderr names its exit code. The remote process can keep running. Ctrl-C before the server started the command says "interrupted before the command started". +- Errors: a 403 names the reason (the container does not allow exec, the cluster does not allow exec, the container holds a service-account token, the server may not check the pod for one, API keys not allowed: run `stackctl login`); 400 a chart or container name that is not valid; 404 an unknown chart or container, or exec turned off on the server; 503 no ready pod; 429 too many open exec sessions. A close of the server before the command started ends with "the server did not start the command". Close 4400: the server refused the command (too long or not valid; the length limit is a server setting). A 404 without an error code means that the server has no exec route: upgrade the server to k8s-stack-manager v0.15.0 or later. +- Security: the output can show secrets, for example from the environment of the container, and can end up in the terminal history or in CI logs. The output is passed through unchanged, so it can hold terminal escape sequences (colors, cursor moves, changes of the window title or the clipboard (OSC 52), hyperlinks); when you do not trust the container, pipe it through `| cat -v` or write it to a file. The server writes each command with its arguments to the audit log. `--insecure` is refused, because a man in the middle could read the commands and their output. An `http://` API URL is refused for the same reason, except for `localhost`, `127.0.0.1` and `[::1]`. `--allow-insecure-stream` accepts that risk. + `stack action list` and `stack action run` see the actions that apply to the stack: a server can limit an action to some templates, template categories, definitions or clusters (`applies_to`). An action of another stack gives "unknown action" with the actions of this stack. When the server could not check where an action applies, `stack action list` ends with "Some actions are not shown: the server could not check where they apply. Try again.", and `-o json` has `"scope_incomplete": true`. `stack action run`: @@ -635,6 +646,10 @@ stackctl stack list --cluster 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e -q | xargs -n stackctl cluster list stackctl cluster get 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e +# Per-cluster switches of stack port-forward and stack exec (admin; off by default) +stackctl cluster update 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e --allow-exec --allow-port-forward +stackctl cluster update 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e --allow-exec=false + # Cluster-level shared Helm values (applied to all deploys on a cluster) stackctl cluster shared-values list 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e stackctl cluster shared-values set 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e --name "local-dev-defaults" --file values.yaml @@ -682,6 +697,7 @@ These groups have no section above. Run `stackctl --help` for the flags. | `--stop-at`, `stack extend --until`, the TTL limit of `server info` and of the local check (an older server ignores `--stop-at` and `--until`: stackctl shows the result and exits with code 1; it uses 30 days as the limit) | v0.12.0 or later | | Actions limited to some stacks (`applies_to`) and the `scope_incomplete` hint of `stack action list` (an older server lists every action for every stack) | the release after v0.14.0 (omattsson/k8s-stack-manager#720) | | `stack port-forward` (an older server answers 404 without an error code: stackctl says "upgrade the server to k8s-stack-manager v0.15.0 or later") | v0.15.0 or later (omattsson/k8s-stack-manager#722) | +| `stack exec` (an older server answers 404 without an error code: stackctl says "upgrade the server to k8s-stack-manager v0.15.0 or later") | v0.15.0 or later (omattsson/k8s-stack-manager#723) | ## Output Formats diff --git a/cli/cmd/agent_guide.md b/cli/cmd/agent_guide.md index 1ea2661..607a3da 100644 --- a/cli/cmd/agent_guide.md +++ b/cli/cmd/agent_guide.md @@ -8,7 +8,7 @@ Run `stackctl --help` for the flags of a command. This guide covers wh 1. **Read with `-o json`.** stdout then holds only JSON: the resource, the list or the server response. Messages, warnings, notes and the deployment log of `--follow` go to stderr. A command without data (for example `stack delete` or `--dry-run`) leaves stdout empty. The default table output is for people and can change. 2. **`-q` prints IDs, one per line**, for pipes and `$(...)`. On `stack create`, `clone` and `template quick-deploy` it is the stack ID; on `stack deploy`, `stop`, `clean` and `rollback` it is the ID of the deployment log. -3. **Exit code 0 means no error, 1 means an error.** The error text is on stderr. Do not parse table output to detect errors. Exceptions: for operations that run on the server, exit 0 means that the server accepted the operation, not that it ended (see "Long-running operations"); `bulk` commands exit 0 even when some items fail (check `failed` and `results` in `-o json`); a plugin (`stackctl `) returns its own exit code. +3. **Exit code 0 means no error, 1 means an error.** The error text is on stderr. Do not parse table output to detect errors. Exceptions: for operations that run on the server, exit 0 means that the server accepted the operation, not that it ended (see "Long-running operations"); `bulk` commands exit 0 even when some items fail (check `failed` and `results` in `-o json`); a plugin (`stackctl `) returns its own exit code; `stack exec` returns the exit code of the remote command, 124 at the server time limit, 130 when interrupted and 255 for an error of stackctl or the server (also the refusal of `--insecure` or of an `http://` API URL, an exit code outside 0..255, and an exit frame without a valid `exit_code`). A remote command can return any code, including 1, 124, 130 and 255; stackctl's own codes come with a stackctl message on stderr (remote stderr goes to the same stream, so read the message, not just its presence). stackctl itself uses 1 only for invalid flags or arguments. 4. **Destructive commands ask for confirmation.** A command with a `--yes` flag (see its `--help`) reads the answer from stdin. Any answer other than `y` exits 1 with `aborted: not confirmed, nothing changed`; a closed stdin fails with `use --yes to confirm`; an open stdin without input waits forever. Pass `--yes` only when the user asked for that change, and run commands with `< /dev/null` so a missing `--yes` fails instead of hanging. 5. **`--dry-run` does not check anything.** It prints the action and the name you gave and contacts no server: a misspelled name still "succeeds". To check the target, run `stackctl stack get -o json` first. 6. **Do not retry an operation that is still running.** See "Long-running operations". @@ -75,6 +75,20 @@ stackctl stack port-forward my-app mysql 0:3306 -q # stdout: 127.0.0.1:; # Needs k8s-stack-manager v0.15.0 or later (an older server: "upgrade the server to k8s-stack-manager v0.15.0 or later"). # An http:// API URL to a host that is not loopback is refused (add --allow-insecure-stream to accept the risk). +# Run one command in a container of a chart (no stdin, no TTY, no shell; for a shell: -- sh -c '...') +stackctl stack exec my-app backend -c php -- bin/console cache:clear +# stdout and stderr are the output of the command, unchanged; -o and -q are ignored. The output can hold secrets +# and terminal escape sequences (colors, cursor moves, title or clipboard (OSC 52) changes, hyperlinks): when the +# container is not trusted, pipe it through `| cat -v` or write it to a file before you show it. +# Exit code: the code of the command; 124 server time limit; 130 interrupted; 255 a client or server error +# (not found, 403, 503 no ready pod, a lost connection ends stackctl: the message is on stderr). Ctrl-C closes +# the connection and exits 130 within a few seconds (a second Ctrl-C exits at once); when the command had already +# ended, stderr names its exit code. +# The remote process can keep running. Never retried: after 124, 130 or a lost connection the command can still +# run on the server, so check its effect before you run it again ("the server did not start the command" and +# "interrupted before the command started" mean that it did not run). +# Needs k8s-stack-manager v0.15.0 or later (an older server: "upgrade the server to k8s-stack-manager v0.15.0 or later"). + # Change values, then deploy again stackctl override set my-app my-chart --set image.tag=1.2.3 stackctl override branch set my-app my-chart feature/x diff --git a/cli/cmd/agent_guide_test.go b/cli/cmd/agent_guide_test.go index 4d72631..7f57dfc 100644 --- a/cli/cmd/agent_guide_test.go +++ b/cli/cmd/agent_guide_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "strings" "testing" @@ -101,6 +102,13 @@ func TestAgentGuide_CommandsExist(t *testing.T) { require.NoError(t, err) args, err := positionalArgs(cmd, rest) require.NoError(t, err) + if slices.Contains(rest, "--") { + // A validator that reads ArgsLenAtDash (stack exec) needs + // parsed flags. + require.NoError(t, cmd.ParseFlags(rest)) + t.Cleanup(func() { resetParsedFlags(cmd) }) + args = cmd.Flags().Args() + } assert.NoError(t, cmd.ValidateArgs(args), "arguments of %s", cmd.CommandPath()) }) } @@ -124,6 +132,7 @@ func TestPositionalArgs(t *testing.T) { {"value flag takes the next word", stackCreateCmd, []string{"x", "--definition", "d", "--branch=b"}, []string{"x"}, ""}, {"bool and global flags", stackDeleteCmd, []string{"--dry-run", "x", "-o", "json", "-q", "--help"}, []string{"x"}, ""}, {"unknown flag", stackCreateCmd, []string{"--nope"}, nil, "stackctl stack create has no flag --nope"}, + {"words after -- are arguments", stackExecCmd, []string{"x", "web", "-c", "php", "--", "ls", "-la", "--nope"}, []string{"x", "web", "ls", "-la", "--nope"}, ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -249,6 +258,10 @@ func positionalArgs(cmd *cobra.Command, args []string) ([]string, error) { var positional []string for i := 0; i < len(args); i++ { arg := args[i] + if arg == "--" { + // The words after "--" are arguments, not flags. + return append(positional, args[i+1:]...), nil + } if !strings.HasPrefix(arg, "-") || arg == "-" { positional = append(positional, arg) continue @@ -269,6 +282,25 @@ func positionalArgs(cmd *cobra.Command, args []string) ([]string, error) { return positional, nil } +// resetParsedFlags undoes cmd.ParseFlags: the flag values and the "--" +// position (pflag keeps ArgsLenAtDash from an earlier parse). +// +// Only scalar flags are reset correctly: Value.Set(DefValue) appends to a +// slice or array flag (StringSlice, StringArray, ...) and parses "[]" as a +// value, instead of clearing it. The commands reset here (stack exec and the +// root command) have no changed slice flags today; reset a slice flag +// through pflag.SliceValue.Replace if one is added. +func resetParsedFlags(cmd *cobra.Command) { + fs := cmd.Flags() + fs.VisitAll(func(f *pflag.Flag) { + if f.Changed { + _ = f.Value.Set(f.DefValue) + f.Changed = false + } + }) + fs.Init(fs.Name(), pflag.ContinueOnError) +} + // lookupFlag finds a local or inherited flag by name, or by shorthand for a // one-letter name. func lookupFlag(cmd *cobra.Command, name string) *pflag.Flag { diff --git a/cli/cmd/cluster.go b/cli/cmd/cluster.go index dc9f716..71e03b2 100644 --- a/cli/cmd/cluster.go +++ b/cli/cmd/cluster.go @@ -160,6 +160,8 @@ Examples: {Key: "Status", Value: printer.StatusColor(cluster.Status)}, {Key: "Default", Value: isDefault}, {Key: "Region", Value: cluster.Region}, + {Key: "Port-forward", Value: strconv.FormatBool(cluster.AllowPortForward)}, + {Key: "Exec", Value: strconv.FormatBool(cluster.AllowExec)}, } if health != nil { fields = append(fields, @@ -573,6 +575,8 @@ func printCluster(cluster *types.Cluster) error { {Key: "Status", Value: printer.StatusColor(cluster.Status)}, {Key: "Default", Value: isDefault}, {Key: "Region", Value: cluster.Region}, + {Key: "Port-forward", Value: strconv.FormatBool(cluster.AllowPortForward)}, + {Key: "Exec", Value: strconv.FormatBool(cluster.AllowExec)}, } return printer.PrintSingle(cluster, fields) } @@ -585,9 +589,15 @@ var clusterCreateCmd = &cobra.Command{ Provide either --from-file with a JSON/YAML file, or use --name and --kubeconfig-data / --kubeconfig-path. +--allow-port-forward and --allow-exec turn on the per-cluster switches of +'stackctl stack port-forward' and 'stackctl stack exec' (off by default; the +server must also enable the feature). Servers before k8s-stack-manager v0.15.0 +ignore them: check the Port-forward and Exec rows of the output. + Examples: stackctl cluster create --from-file cluster.json - stackctl cluster create --name prod --kubeconfig-path ~/.kube/config`, + stackctl cluster create --name prod --kubeconfig-path ~/.kube/config + stackctl cluster create --name dev --kubeconfig-path ~/.kube/config --allow-exec`, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { fromFile, _ := cmd.Flags().GetString("from-file") @@ -647,6 +657,8 @@ Examples: } req.Name = name req.Description, _ = cmd.Flags().GetString("description") + req.AllowPortForward, _ = cmd.Flags().GetBool("allow-port-forward") + req.AllowExec, _ = cmd.Flags().GetBool("allow-exec") req.KubeconfigData = kubeconfigData if kubeconfigPath != "" { content, err := os.ReadFile(kubeconfigPath) @@ -688,9 +700,11 @@ var clusterUpdateCmd = &cobra.Command{ kubeconfigDataChanged := cmd.Flags().Changed("kubeconfig-data") kubeconfigPathChanged := cmd.Flags().Changed("kubeconfig-path") defaultChanged := cmd.Flags().Changed("default") + portForwardChanged := cmd.Flags().Changed("allow-port-forward") + execChanged := cmd.Flags().Changed("allow-exec") - if fromFile == "" && !nameChanged && !descChanged && !kubeconfigDataChanged && !kubeconfigPathChanged && !defaultChanged { - return fmt.Errorf("at least one of --name, --description, --kubeconfig-data, --kubeconfig-path, --default, or --from-file must be specified") + if fromFile == "" && !nameChanged && !descChanged && !kubeconfigDataChanged && !kubeconfigPathChanged && !defaultChanged && !portForwardChanged && !execChanged { + return fmt.Errorf("at least one of --name, --description, --kubeconfig-data, --kubeconfig-path, --default, --allow-port-forward, --allow-exec, or --from-file must be specified") } var req types.UpdateClusterRequest @@ -771,6 +785,14 @@ var clusterUpdateCmd = &cobra.Command{ v, _ := cmd.Flags().GetBool("default") req.IsDefault = &v } + if portForwardChanged { + v, _ := cmd.Flags().GetBool("allow-port-forward") + req.AllowPortForward = &v + } + if execChanged { + v, _ := cmd.Flags().GetBool("allow-exec") + req.AllowExec = &v + } } c, err := newClient() @@ -1322,6 +1344,8 @@ func init() { clusterCreateCmd.Flags().String("description", "", "Cluster description") clusterCreateCmd.Flags().String("kubeconfig-data", "", "Raw kubeconfig data (base64 or YAML string)") clusterCreateCmd.Flags().String("kubeconfig-path", "", "Path to kubeconfig file") + clusterCreateCmd.Flags().Bool("allow-port-forward", false, "Allow 'stackctl stack port-forward' to stacks on this cluster (k8s-stack-manager v0.15.0 or later)") + clusterCreateCmd.Flags().Bool("allow-exec", false, "Allow 'stackctl stack exec' in stacks on this cluster (k8s-stack-manager v0.15.0 or later)") // cluster update flags clusterUpdateCmd.Flags().String("from-file", "", "JSON or YAML file with update payload") @@ -1330,6 +1354,8 @@ func init() { clusterUpdateCmd.Flags().String("kubeconfig-data", "", "New kubeconfig data") clusterUpdateCmd.Flags().String("kubeconfig-path", "", "New path to kubeconfig file") clusterUpdateCmd.Flags().Bool("default", false, "Mark cluster as default") + clusterUpdateCmd.Flags().Bool("allow-port-forward", false, "Allow 'stackctl stack port-forward' to stacks on this cluster (--allow-port-forward=false turns it off)") + clusterUpdateCmd.Flags().Bool("allow-exec", false, "Allow 'stackctl stack exec' in stacks on this cluster (--allow-exec=false turns it off)") // cluster delete flags clusterDeleteCmd.Flags().BoolP("yes", "y", false, flagDescSkipConfirm) diff --git a/cli/cmd/cluster_test.go b/cli/cmd/cluster_test.go index 7e56f9e..681c746 100644 --- a/cli/cmd/cluster_test.go +++ b/cli/cmd/cluster_test.go @@ -836,6 +836,8 @@ func resetClusterCreateFlags(t *testing.T) { clusterCreateCmd.Flags().Set("description", "") clusterCreateCmd.Flags().Set("kubeconfig-data", "") clusterCreateCmd.Flags().Set("kubeconfig-path", "") + clusterCreateCmd.Flags().Set("allow-port-forward", "false") + clusterCreateCmd.Flags().Set("allow-exec", "false") } func TestClusterCreateCmd_TableOutput(t *testing.T) { @@ -1001,6 +1003,10 @@ func resetClusterUpdateFlags(t *testing.T) { clusterUpdateCmd.Flags().Lookup("kubeconfig-data").Changed = false clusterUpdateCmd.Flags().Lookup("kubeconfig-path").Changed = false clusterUpdateCmd.Flags().Lookup("default").Changed = false + clusterUpdateCmd.Flags().Set("allow-port-forward", "false") + clusterUpdateCmd.Flags().Set("allow-exec", "false") + clusterUpdateCmd.Flags().Lookup("allow-port-forward").Changed = false + clusterUpdateCmd.Flags().Lookup("allow-exec").Changed = false } func TestClusterUpdateCmd_TableOutput(t *testing.T) { @@ -1077,6 +1083,112 @@ func TestClusterUpdateCmd_SetDefault(t *testing.T) { assert.Contains(t, buf.String(), "true") } +func TestClusterCreateCmd_StreamSwitches(t *testing.T) { + cl := sampleCluster() + cl.AllowExec = true + var body map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(cl) + })) + defer server.Close() + + buf := setupClusterTestCmd(t, server.URL) + clusterCreateCmd.Flags().Set("name", "dev-cluster") + clusterCreateCmd.Flags().Set("allow-exec", "true") + t.Cleanup(func() { resetClusterCreateFlags(t) }) + + err := clusterCreateCmd.RunE(clusterCreateCmd, nil) + require.NoError(t, err) + assert.Equal(t, true, body["allow_exec"]) + _, sent := body["allow_port_forward"] + assert.False(t, sent, "an unset switch is not sent") + assert.Regexp(t, `Exec:\s+true`, buf.String()) + assert.Regexp(t, `Port-forward:\s+false`, buf.String()) +} + +func TestClusterCreateCmd_FromFileStreamSwitches(t *testing.T) { + var body map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(sampleCluster()) + })) + defer server.Close() + + file := filepath.Join(t.TempDir(), "cluster.yaml") + require.NoError(t, os.WriteFile(file, []byte("name: dev\nuse_in_cluster: true\nallow_exec: true\nallow_port_forward: true\n"), 0o600)) + _ = setupClusterTestCmd(t, server.URL) + clusterCreateCmd.Flags().Set("from-file", file) + t.Cleanup(func() { resetClusterCreateFlags(t) }) + + require.NoError(t, clusterCreateCmd.RunE(clusterCreateCmd, nil)) + assert.Equal(t, true, body["allow_exec"]) + assert.Equal(t, true, body["allow_port_forward"]) +} + +func TestClusterUpdateCmd_StreamSwitches(t *testing.T) { + tests := []struct { + name string + flags map[string]string + want map[string]interface{} + }{ + {name: "exec on", flags: map[string]string{"allow-exec": "true"}, want: map[string]interface{}{"allow_exec": true}}, + {name: "exec off is sent", flags: map[string]string{"allow-exec": "false"}, want: map[string]interface{}{"allow_exec": false}}, + {name: "both", flags: map[string]string{"allow-exec": "true", "allow-port-forward": "false"}, want: map[string]interface{}{"allow_exec": true, "allow_port_forward": false}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var body map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPut, r.Method) + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(sampleCluster()) + })) + defer server.Close() + + _ = setupClusterTestCmd(t, server.URL) + for k, v := range tt.flags { + require.NoError(t, clusterUpdateCmd.Flags().Set(k, v)) + } + t.Cleanup(func() { resetClusterUpdateFlags(t) }) + + require.NoError(t, clusterUpdateCmd.RunE(clusterUpdateCmd, []string{"1"})) + assert.Equal(t, tt.want, body) + }) + } +} + +func TestClusterGetCmd_StreamSwitches(t *testing.T) { + cl := sampleCluster() + cl.AllowPortForward = true + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path != "/api/v1/clusters/1" { + w.WriteHeader(http.StatusNotFound) + return + } + json.NewEncoder(w).Encode(cl) + })) + defer server.Close() + + buf := setupClusterTestCmd(t, server.URL) + require.NoError(t, clusterGetCmd.RunE(clusterGetCmd, []string{"1"})) + assert.Regexp(t, `Port-forward:\s+true`, buf.String()) + assert.Regexp(t, `Exec:\s+false`, buf.String()) + + buf.Reset() + printer.Format = output.FormatJSON + require.NoError(t, clusterGetCmd.RunE(clusterGetCmd, []string{"1"})) + assert.Contains(t, buf.String(), `"allow_port_forward": true`) + assert.NotContains(t, buf.String(), `"allow_exec"`, "a switch that is off is left out, like the other bool fields") +} + func TestClusterUpdateCmd_PathTraversal(t *testing.T) { _ = setupClusterTestCmd(t, "http://unused") clusterUpdateCmd.Flags().Set("from-file", "../../etc/passwd") @@ -1295,7 +1407,6 @@ func TestClusterSetDefaultCmd_NotFound(t *testing.T) { assert.Contains(t, err.Error(), "cluster not found") } - // ---------- cluster test-connection ---------- func sampleTestConnectionResult() types.ClusterTestConnectionResult { diff --git a/cli/cmd/exitcode.go b/cli/cmd/exitcode.go new file mode 100644 index 0000000..0a40e0a --- /dev/null +++ b/cli/cmd/exitcode.go @@ -0,0 +1,37 @@ +package cmd + +import "fmt" + +// ExitError ends stackctl with exit code Code. When Err is not nil, main +// prints the returned error (the ExitError or a wrapper around it; Error() +// is the text of Err) to stderr; a nil Err prints nothing (for example the exit code of +// a remote command of stack exec). Errors of another type exit 1. +type ExitError struct { + Code int + Err error +} + +// Error returns the text of Err, or "exit status N" when Err is nil. +func (e *ExitError) Error() string { + if e.Err == nil { + return fmt.Sprintf("exit status %d", e.Code) + } + return e.Err.Error() +} + +// Unwrap returns Err. +func (e *ExitError) Unwrap() error { + return e.Err +} + +// Exit codes of stack exec besides the exit code of the remote command. +const ( + // exitTimeLimit: the server time limit ended the command (the GNU + // timeout convention). + exitTimeLimit = 124 + // exitInterrupted: Ctrl-C or SIGTERM (128 + SIGINT). + exitInterrupted = 130 + // exitStackctlError: an error of stackctl or the server, not of the + // remote command (the ssh convention). + exitStackctlError = 255 +) diff --git a/cli/cmd/root.go b/cli/cmd/root.go index c966ad9..7e4711a 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -70,11 +70,7 @@ AI agents and scripts: run 'stackctl agent-guide' first.`, } // Warn when TLS verification is disabled - insecure := flagInsecure - if !insecure && cfg.CurrentCtx() != nil { - insecure = cfg.CurrentCtx().Insecure - } - if insecure { + if effectiveInsecure() { fmt.Fprintln(cmd.ErrOrStderr(), "WARNING: TLS certificate verification is disabled") } @@ -237,13 +233,22 @@ func resolveAPIKey() string { return "" } -// applyInsecureTLS configures the client to skip TLS verification if the insecure flag is set. -func applyInsecureTLS(c *client.Client) { - insecure := flagInsecure - if !insecure && cfg.CurrentCtx() != nil { - insecure = cfg.CurrentCtx().Insecure +// effectiveInsecure reports whether TLS verification is off: --insecure, or +// the insecure setting of the current context. The warning, the transport +// (applyInsecureTLS) and the stream check (checkStreamTransport) all use it, +// so they cannot disagree. +func effectiveInsecure() bool { + if flagInsecure { + return true } - if insecure { + ctx := cfg.CurrentCtx() + return ctx != nil && ctx.Insecure +} + +// applyInsecureTLS configures the client to skip TLS verification when +// effectiveInsecure reports true (the flag or the context setting). +func applyInsecureTLS(c *client.Client) { + if effectiveInsecure() { // Clone the default transport to preserve proxies, timeouts, and keep-alives. if t, ok := http.DefaultTransport.(*http.Transport); ok { clone := t.Clone() diff --git a/cli/cmd/stack_exec.go b/cli/cmd/stack_exec.go new file mode 100644 index 0000000..e9a5731 --- /dev/null +++ b/cli/cmd/stack_exec.go @@ -0,0 +1,287 @@ +package cmd + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "os/signal" + "strings" + "sync" + "sync/atomic" + "time" + + "github.com/omattsson/stackctl/cli/pkg/client" + "github.com/spf13/cobra" +) + +var stackExecCmd = &cobra.Command{ + Use: "exec [-c CONTAINER] [flags] -- COMMAND [ARGS...]", + Short: "Run a command in a container of a stack", + Long: `Run one command in a container of a chart of a stack, through the +k8s-stack-manager server (GET /api/v1/stack-instances/:id/exec). You need +no kubeconfig and no cluster access: the server checks your access to the +stack and chooses the pod (the first running and ready pod of the chart). + +CHART is the chart (Helm release) name, as in the CHART column of +'stackctl stack pods'. -c selects the container; without it the server +uses the first container that allows exec. The server decides which +containers allow exec. + +The '--' is required. Everything after it is the command and its +arguments, passed as a list without a shell: stackctl does not read flags +after '--'. For pipes, variables or globs, run a shell: -- sh -c '...'. + +The command gets no stdin and no TTY. Its stdout goes to stdout and its +stderr to stderr, unchanged and as it arrives. -o and -q are ignored. +stackctl never connects again: a lost connection ends stackctl (exit 255). + +Exit codes: + N the exit code of the remote command (0 to 255) + 124 the server time limit ended the command + 130 interrupted (Ctrl-C or SIGTERM) + 255 an error of stackctl or the server: stack, chart or container not + found, no access, no ready pod, a lost connection, the refusal of + --insecure or of an http:// API URL, an exit code outside 0 to 255, + an exit frame without a valid exit_code + 1 invalid flags or arguments +A remote command can return any code, including 1, 124, 130 and 255; +stackctl's own codes come with a stackctl message on stderr (remote +stderr goes to the same stream, so read the message, not just its +presence). stackctl itself uses 1 only for invalid flags or arguments. + +Ctrl-C closes the connection and exits 130 within a few seconds (a second +Ctrl-C exits at once); when the command had already ended, stderr names its +exit code. The remote process can keep running. + +Security: + - The output can show secrets, for example from the environment of the + container, and can end up in the terminal history or in CI logs. + - The output of the remote command is passed through unchanged: it can + hold terminal escape sequences (colors, cursor moves, changes of the + window title or the clipboard (OSC 52), hyperlinks). When you do not + trust the container, pipe it through '| cat -v' or write it to a file. + - The server writes each command, with its arguments, to the audit log. + - --insecure is refused: without TLS verification, a man in the middle + can read the commands and their output. An http:// API URL is refused + for the same reason, except for localhost, 127.0.0.1 and [::1]. Add + --allow-insecure-stream to accept that risk. + - API keys are not allowed by default: run 'stackctl login'. + +Examples: + # Clear a cache + stackctl stack exec my-stack backend -- bin/console cache:clear + + # A shell command line, in a given container + stackctl stack exec my-stack backend -c php -- sh -c 'ls -la /var/www | head' + + # Flags after '--' belong to the remote command + stackctl stack exec my-stack backend -- ls -la --color=never /tmp`, + Args: stackExecArgs, + RunE: func(cmd *cobra.Command, args []string) error { + parent := cmd.Context() + if parent == nil { + parent = context.Background() + } + signals, stop := execSignals() + defer stop() + return runStackExec(parent, cmd, args, signals) + }, +} + +func init() { + stackExecCmd.Flags().StringP("container", "c", "", "Container name (default: the first container of the chart that allows exec)") + stackExecCmd.Flags().Bool("allow-insecure-stream", false, "Allow exec with --insecure or an http:// API URL (the commands and their output are not protected against a man in the middle)") + stackCmd.AddCommand(stackExecCmd) +} + +// execSignals returns the channel of the signals that interrupt stack exec +// (Ctrl-C, SIGTERM) and the function that stops their delivery. Tests +// replace it to inject signals. +var execSignals = func() (<-chan os.Signal, func()) { + ch := make(chan os.Signal, 2) + signal.Notify(ch, portForwardSignals...) + return ch, func() { signal.Stop(ch) } +} + +// execForceExit ends the process at the second interrupt. Tests replace it. +var execForceExit = os.Exit + +// execKeepalive and execCancelWait are the timings of the exec stream +// (zero: the client defaults). Tests shorten them. +var ( + execKeepalive client.KeepaliveTimings + execCancelWait time.Duration +) + +// stackExecArgs checks -- COMMAND [ARGS...] and the fixed +// command rules of the server. Its errors exit 1. +func stackExecArgs(cmd *cobra.Command, args []string) error { + const usage = "usage: stackctl stack exec [-c CONTAINER] -- COMMAND [ARGS...]" + dash := cmd.ArgsLenAtDash() + switch { + case dash < 0: + return errors.New("missing '--' before the command; " + usage) + case dash != 2: + return fmt.Errorf("expected and before '--', got %d argument(s); %s", dash, usage) + case len(args) == 2: + return errors.New("missing command after '--'; " + usage) + case strings.TrimSpace(args[0]) == "": + return errors.New("stack name or ID must not be empty") + case strings.TrimSpace(args[1]) == "": + return errors.New("chart name must not be empty") + } + if err := client.ValidateExecCommand(args[2:]); err != nil { + return fmt.Errorf("invalid command: %w", err) + } + return nil +} + +// runStackExec runs the command and maps the result to an *ExitError (see +// the exit codes in the help). The first signal on signals cancels the +// exec; a second one calls execForceExit(130). +func runStackExec(parent context.Context, cmd *cobra.Command, args []string, signals <-chan os.Signal) error { + chart := strings.TrimSpace(args[1]) + command := args[2:] + container, _ := cmd.Flags().GetString("container") + container = strings.TrimSpace(container) + stderr := &syncWriter{w: cmd.ErrOrStderr()} + + ctx, cancel := context.WithCancel(parent) + defer cancel() + var interrupted atomic.Bool + done := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + select { + case <-signals: + case <-done: + return + } + interrupted.Store(true) + cancel() + select { + case <-signals: + // Never through stderr: the reader can hold its lock in a + // blocked write of the remote stderr (for example 2>&1 | less, + // paused). The raw writer can block too, so wait at most + // execNoteWait. + writeNoteNoWait(cmd.ErrOrStderr(), "interrupted", execNoteWait) + execForceExit(exitInterrupted) + case <-done: + } + }() + defer func() { + close(done) + wg.Wait() + }() + + res, err := execOnce(ctx, cmd, args[0], chart, container, command, stderr) + if interrupted.Load() { + return interruptedExit(res, err) + } + if err != nil { + if errors.Is(err, client.ErrExecTimeLimit) { + return &ExitError{Code: exitTimeLimit, Err: err} + } + return &ExitError{Code: exitStackctlError, Err: err} + } + return remoteExit(res.ExitCode) +} + +// interruptedExit maps the result of an interrupted exec to exit 130 and +// says whether the command can have run. +func interruptedExit(res *client.ExecResult, err error) *ExitError { + var canceled *client.ExecCanceledError + var closed *client.ExecClosedError + switch { + case res != nil: + return &ExitError{Code: exitInterrupted, Err: fmt.Errorf("interrupted; the command ended with exit code %d", res.ExitCode)} + case errors.Is(err, errExecNotStarted), errors.As(err, &canceled) && !canceled.Started: + return &ExitError{Code: exitInterrupted, Err: errors.New("interrupted before the command started")} + case errors.As(err, &closed) && closed.Code != 0 && !closed.Started: + // A close frame of the server without started before it (it can + // come before the cancel): the command did not run. Keep the text + // of the server. + return &ExitError{Code: exitInterrupted, Err: fmt.Errorf("interrupted before the command started: %w", err)} + } + return &ExitError{Code: exitInterrupted, Err: errors.New("interrupted; the command can still run on the server")} +} + +// errExecNotStarted marks an interrupt before the dial. +var errExecNotStarted = errors.New("interrupted before the command started") + +// execNoteWait bounds the write of the note of the second interrupt. +const execNoteWait = 100 * time.Millisecond + +// writeNoteNoWait writes note and a newline to w from a goroutine and waits +// at most wait for it, so a blocked w cannot delay the caller. The +// goroutine stays blocked with w; the caller exits the process next. +func writeNoteNoWait(w io.Writer, note string, wait time.Duration) { + written := make(chan struct{}) + go func() { + defer close(written) + fmt.Fprintln(w, note) + }() + timer := time.NewTimer(wait) + defer timer.Stop() + select { + case <-written: + case <-timer.C: + } +} + +// execOnce resolves the stack and runs the command once. An error before +// the dial after an interrupt is errExecNotStarted. +func execOnce(ctx context.Context, cmd *cobra.Command, stack, chart, container string, command []string, stderr *syncWriter) (*client.ExecResult, error) { + beforeDial := func(err error) error { + if ctx.Err() != nil { + return errExecNotStarted + } + return err + } + c, err := newClient() + if err != nil { + return nil, beforeDial(err) + } + if err := checkStreamTransport(cmd, c, stderr, streamRisk{ + op: "exec", + data: "the commands and their output", + warning: "the commands and their output are not protected against a man in the middle", + }); err != nil { + return nil, beforeDial(err) + } + c.WarnWriter = stderr + if c.Debug { + // The same writer as the remote stderr, so the lines do not mix. + c.DebugWriter = stderr + } + id, err := resolveStackID(c, stack) + if err != nil { + return nil, beforeDial(err) + } + if ctx.Err() != nil { + return nil, errExecNotStarted + } + return c.Exec(ctx, id, client.ExecRequest{ + Chart: chart, Container: container, Command: command, + Keepalive: execKeepalive, CancelWait: execCancelWait, + }, cmd.OutOrStdout(), stderr) +} + +// remoteExit maps the exit code of the remote command to the exit code of +// stackctl: 0 is success, 1 to 255 pass through without a message. A code +// outside 0 to 255 (the operating system would cut it) exits 255 with a +// message. +func remoteExit(code int) error { + switch { + case code == 0: + return nil + case code > 0 && code <= 255: + return &ExitError{Code: code} + } + return &ExitError{Code: exitStackctlError, Err: fmt.Errorf("the command exited with code %d, which is outside 0 to 255", code)} +} diff --git a/cli/cmd/stack_exec_test.go b/cli/cmd/stack_exec_test.go new file mode 100644 index 0000000..6f8e9c8 --- /dev/null +++ b/cli/cmd/stack_exec_test.go @@ -0,0 +1,889 @@ +package cmd + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/gorilla/websocket" + "github.com/omattsson/stackctl/cli/pkg/client" + "github.com/omattsson/stackctl/cli/pkg/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" +) + +// Tests in this file are NOT parallelized: they mutate package globals +// (cfg, printer, flag variables, execSignals, execForceExit) and run +// rootCmd. + +const execCmdTestPath = "/api/v1/stack-instances/42/exec" + +// execCmdServer serves the exec route of instance 42 and the stack list +// (name resolution: "my-stack" is 42, every other name has no match). It +// counts all requests and the upgrades, and records the query and the +// command of the last exec. refuse, when set, answers the exec route +// without an upgrade; handler runs after the start message. +type execCmdServer struct { + *httptest.Server + requests atomic.Int32 + upgrades atomic.Int32 + lists atomic.Int32 + mu sync.Mutex + query url.Values + command []string + // listDelay delays the stack list answer. + listDelay time.Duration +} + +func (s *execCmdServer) lastQuery() url.Values { + s.mu.Lock() + defer s.mu.Unlock() + return s.query +} + +func (s *execCmdServer) lastCommand() []string { + s.mu.Lock() + defer s.mu.Unlock() + return s.command +} + +func newExecCmdServer(t *testing.T, refuse func(w http.ResponseWriter) bool, handler func(ws *websocket.Conn)) *execCmdServer { + t.Helper() + s := &execCmdServer{} + up := websocket.Upgrader{} + s.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + s.requests.Add(1) + switch r.URL.Path { + case "/api/v1/stack-instances": + s.lists.Add(1) + if s.listDelay > 0 { + time.Sleep(s.listDelay) + } + data := []map[string]any{} + if r.URL.Query().Get("name") == "my-stack" { + data = append(data, map[string]any{"id": "42", "name": "my-stack", "status": "running"}) + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"data": data, "total": len(data), "page": 1, "pageSize": 25}) + return + case execCmdTestPath: + default: + http.NotFound(w, r) + return + } + s.mu.Lock() + s.query = r.URL.Query() + s.mu.Unlock() + if refuse != nil && refuse(w) { + return + } + ws, err := up.Upgrade(w, r, nil) + if err != nil { + return + } + s.upgrades.Add(1) + defer ws.Close() + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + _, raw, err := ws.ReadMessage() + if err != nil { + return + } + _ = ws.SetReadDeadline(time.Time{}) + var start types.ExecStartMessage + if json.Unmarshal(raw, &start) != nil || start.Type != "start" { + return + } + s.mu.Lock() + s.command = start.Command + s.mu.Unlock() + if handler != nil { + handler(ws) + } + })) + t.Cleanup(s.Close) + return s +} + +func wsJSON(ws *websocket.Conn, v any) { + data, _ := json.Marshal(v) + _ = ws.WriteMessage(websocket.TextMessage, data) +} + +func wsStream(ws *websocket.Conn, stream byte, data string) { + _ = ws.WriteMessage(websocket.BinaryMessage, append([]byte{stream}, data...)) +} + +func wsCloseAndDrain(ws *websocket.Conn, code int, reason string) { + _ = ws.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(code, reason), time.Now().Add(time.Second)) + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + if _, _, err := ws.ReadMessage(); err != nil { + return + } + } +} + +// exitWith sends started, the output and the exit code. +func exitWith(code int, stdout, stderr string) func(*websocket.Conn) { + return func(ws *websocket.Conn) { + wsJSON(ws, map[string]any{"type": "started", "pod": "web-0", "container": "php"}) + if stdout != "" { + wsStream(ws, client.ExecStreamStdout, stdout) + } + if stderr != "" { + wsStream(ws, client.ExecStreamStderr, stderr) + } + wsJSON(ws, map[string]any{"type": "exit", "exit_code": code}) + wsCloseAndDrain(ws, websocket.CloseNormalClosure, "") + } +} + +// closeWith sends started, then the close code without an exit frame. +func closeWith(code int, reason string) func(*websocket.Conn) { + return func(ws *websocket.Conn) { + wsJSON(ws, map[string]any{"type": "started", "pod": "web-0", "container": "php"}) + wsCloseAndDrain(ws, code, reason) + } +} + +// waitClientClose sends started and "x", then waits for the close of the +// client without answering it, and sends the close code to gotClose. +// afterClose runs after the close; then the server waits until the client +// drops the connection. +func waitClientClose(gotClose chan<- int, afterClose func(ws *websocket.Conn)) func(*websocket.Conn) { + return func(ws *websocket.Conn) { + ws.SetCloseHandler(func(int, string) error { return nil }) + wsJSON(ws, map[string]any{"type": "started", "pod": "web-0", "container": "php"}) + wsStream(ws, client.ExecStreamStdout, "x") + _ = ws.SetReadDeadline(time.Now().Add(10 * time.Second)) + _, _, err := ws.ReadMessage() + code := 0 + var ce *websocket.CloseError + if errors.As(err, &ce) { + code = ce.Code + } + gotClose <- code + if afterClose != nil { + afterClose(ws) + } + _ = ws.NetConn().SetReadDeadline(time.Now().Add(10 * time.Second)) + _, _ = io.Copy(io.Discard, ws.NetConn()) + } +} + +// execTest is a run of stackctl stack exec through rootCmd. +type execTest struct { + stdout, stderr *lockedBuffer + signals chan os.Signal + forced chan int +} + +// setupExecTest prepares the globals for rootCmd runs of stack exec: the +// config directory, the API URL, the writers, an injected signal channel +// and a recording execForceExit. Everything is restored at the end. +func setupExecTest(t *testing.T, apiURL string) *execTest { + t.Helper() + setupStackTestCmd(t, apiURL) + e := &execTest{ + stdout: &lockedBuffer{}, stderr: &lockedBuffer{}, + signals: make(chan os.Signal, 2), forced: make(chan int, 2), + } + prevSignals, prevForce := execSignals, execForceExit + prevKeepalive, prevWait := execKeepalive, execCancelWait + execSignals = func() (<-chan os.Signal, func()) { return e.signals, func() {} } + execForceExit = func(code int) { e.forced <- code } + execCancelWait = 300 * time.Millisecond + t.Cleanup(func() { + execSignals, execForceExit = prevSignals, prevForce + execKeepalive, execCancelWait = prevKeepalive, prevWait + rootCmd.SetOut(nil) + rootCmd.SetErr(nil) + rootCmd.SetArgs(nil) + // pflag keeps the values, Changed and the "--" position of the last + // parse; reset them for the next Execute in this binary. + resetParsedFlags(stackExecCmd) + resetParsedFlags(rootCmd) + }) + rootCmd.SetOut(e.stdout) + rootCmd.SetErr(e.stderr) + return e +} + +// start runs "stackctl stack exec args..." in a goroutine. +func (e *execTest) start(args ...string) <-chan error { + rootCmd.SetArgs(append([]string{"stack", "exec"}, args...)) + done := make(chan error, 1) + go func() { done <- rootCmd.Execute() }() + return done +} + +func (e *execTest) run(t *testing.T, args ...string) error { + t.Helper() + return waitDone(t, e.start(args...)) +} + +// requireExit checks that err is an *ExitError with code and returns it. +func requireExit(t *testing.T, err error, code int) *ExitError { + t.Helper() + var ee *ExitError + require.ErrorAs(t, err, &ee, "error %v", err) + assert.Equal(t, code, ee.Code, "exit code; error %v", err) + return ee +} + +func TestStackExecCmd_UsageErrorsExit1WithoutDial(t *testing.T) { + many := []string{"42", "web", "--"} + for i := 0; i <= client.ExecMaxArgs; i++ { + many = append(many, "x") + } + tests := []struct { + name string + args []string + wantErr string + }{ + {"missing --", []string{"42", "web", "ls"}, "missing '--' before the command"}, + {"missing -- without a command", []string{"42", "web"}, "missing '--' before the command"}, + {"-- after the stack", []string{"42", "--", "web", "ls"}, "expected and before '--', got 1 argument(s)"}, + {"-- first", []string{"--", "42", "web", "ls"}, "expected and before '--', got 0 argument(s)"}, + {"-- after three words", []string{"42", "web", "extra", "--", "ls"}, "expected and before '--', got 3 argument(s)"}, + {"no command after --", []string{"42", "web", "--"}, "missing command after '--'"}, + {"empty stack", []string{" ", "web", "--", "ls"}, "stack name or ID must not be empty"}, + {"empty chart", []string{"42", "", "--", "ls"}, "chart name must not be empty"}, + {"empty program", []string{"42", "web", "--", "", "x"}, "invalid command: the first argument of the command (the program) is empty"}, + {"NUL byte", []string{"42", "web", "--", "ls", "a\x00b"}, "invalid command: argument 2 of the command contains a NUL byte"}, + {"too many arguments", many, "invalid command: the command has 257 arguments; the server allows at most 256"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "", "")) + e := setupExecTest(t, srv.URL) + err := e.run(t, tt.args...) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + var ee *ExitError + assert.False(t, errors.As(err, &ee), "a usage error exits 1, not with an ExitError") + assert.Equal(t, int32(0), srv.requests.Load(), "no request") + assert.Empty(t, e.stdout.String()) + }) + } +} + +func TestStackExecArgs_ResetBetweenParses(t *testing.T) { + // The "--" position of one parse must not leak into the next one. + require.NoError(t, stackExecCmd.ParseFlags([]string{"42", "web", "--", "ls"})) + require.NoError(t, stackExecArgs(stackExecCmd, stackExecCmd.Flags().Args())) + resetParsedFlags(stackExecCmd) + require.NoError(t, stackExecCmd.ParseFlags([]string{"42", "web", "ls"})) + t.Cleanup(func() { resetParsedFlags(stackExecCmd) }) + err := stackExecArgs(stackExecCmd, stackExecCmd.Flags().Args()) + require.Error(t, err) + assert.Contains(t, err.Error(), "missing '--'") +} + +func TestStackExecCmd_ContainerAndPassThrough(t *testing.T) { + tests := []struct { + name string + args []string + wantContainer string + wantCommand []string + }{ + {"no -c leaves out container", []string{"42", "web", "--", "ls"}, "", []string{"ls"}}, + {"-c sends container", []string{"42", "web", "-c", "php", "--", "ls"}, "php", []string{"ls"}}, + {"--container before the stack", []string{"--container=php", "42", "web", "--", "ls"}, "php", []string{"ls"}}, + {"flags after -- go to the remote command", []string{"42", "web", "--", "ls", "-la", "--help", "-c", "x"}, "", + []string{"ls", "-la", "--help", "-c", "x"}}, + {"global flags after -- are not parsed", []string{"42", "web", "-c", "php", "--", "sh", "-c", "echo hi", "-o", "json", "--debug", "--", "-q"}, "php", + []string{"sh", "-c", "echo hi", "-o", "json", "--debug", "--", "-q"}}, + {"name resolution", []string{"my-stack", "web", "--", "true"}, "", []string{"true"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "ok\n", "")) + e := setupExecTest(t, srv.URL) + require.NoError(t, e.run(t, tt.args...)) + assert.Equal(t, "ok\n", e.stdout.String(), "stdout is only the remote stdout") + assert.Empty(t, e.stderr.String()) + assert.Equal(t, tt.wantCommand, srv.lastCommand()) + q := srv.lastQuery() + assert.Equal(t, "web", q.Get("chart")) + _, has := q["container"] + assert.Equal(t, tt.wantContainer != "", has, "container in the query") + assert.Equal(t, tt.wantContainer, q.Get("container")) + assert.Len(t, q, map[bool]int{true: 2, false: 1}[has], "only chart and container in the query") + assert.Equal(t, int32(1), srv.upgrades.Load()) + assert.False(t, flagDebug, "--debug after -- is not a stackctl flag") + }) + } +} + +func TestStackExecCmd_ExitMapping(t *testing.T) { + tests := []struct { + name string + args []string + refuse func(w http.ResponseWriter) bool + handler func(ws *websocket.Conn) + wantNil bool + wantCode int + wantErrNil bool + wantMsg []string + wantDial bool + wantStdout string + check func(t *testing.T, err error) + }{ + {name: "exit 0", handler: exitWith(0, "out", "err"), wantStdout: "out", wantNil: true, wantDial: true}, + {name: "exit 7 passes through without a message", handler: exitWith(7, "out", "err"), wantStdout: "out", wantCode: 7, wantErrNil: true, wantDial: true}, + {name: "exit 1", handler: exitWith(1, "out", "err"), wantStdout: "out", wantCode: 1, wantErrNil: true, wantDial: true}, + {name: "exit 255", handler: exitWith(255, "out", "err"), wantStdout: "out", wantCode: 255, wantErrNil: true, wantDial: true}, + {name: "exit 256 is clamped to 255", handler: exitWith(256, "out", "err"), wantStdout: "out", wantCode: 255, wantDial: true, + wantMsg: []string{"the command exited with code 256, which is outside 0 to 255"}}, + {name: "exit -1 is clamped to 255", handler: exitWith(-1, "out", "err"), wantStdout: "out", wantCode: 255, wantDial: true, + wantMsg: []string{"the command exited with code -1, which is outside 0 to 255"}}, + {name: "time limit gives 124", handler: closeWith(client.CloseMaxDuration, "maximum duration reached"), wantCode: 124, wantDial: true, + wantMsg: []string{"server time limit reached"}, + check: func(t *testing.T, err error) { assert.ErrorIs(t, err, client.ErrExecTimeLimit) }}, + {name: "close without an exit frame gives 255", handler: closeWith(websocket.CloseNormalClosure, ""), wantCode: 255, wantDial: true, + wantMsg: []string{"connection lost; the command can still run on the server"}}, + {name: "refused command gives 255", handler: closeWith(client.CloseBadStart, "command too long"), wantCode: 255, wantDial: true, + wantMsg: []string{"the server refused the command (too long or not valid): command too long"}}, + {name: "revoked session gives 255", handler: closeWith(websocket.ClosePolicyViolation, "session revoked"), wantCode: 255, wantDial: true, + wantMsg: []string{"exec closed: session revoked (close 1008); log in again with 'stackctl login'; the command can still run on the server"}}, + {name: "a close before started says so", wantCode: 255, wantDial: true, + handler: func(ws *websocket.Conn) { wsCloseAndDrain(ws, websocket.CloseGoingAway, "server shutdown") }, + wantMsg: []string{"exec closed: the server is shutting down; the server did not start the command"}}, + {name: "403 container not allowed", args: []string{"42", "web", "-c", "php", "--", "ls"}, wantCode: 255, + refuse: jsonRefusal(http.StatusForbidden, "container_not_allowed", "Container does not allow exec"), + wantMsg: []string{"exec refused by the server (HTTP 403)", `Container "php" does not allow exec`}}, + {name: "404 feature disabled", wantCode: 255, + refuse: jsonRefusal(http.StatusNotFound, "feature_disabled", "This feature is disabled"), + wantMsg: []string{"The server has exec turned off"}}, + {name: "plain 404", wantCode: 255, + refuse: func(w http.ResponseWriter) bool { + http.NotFound(w, nil) + return true + }, + wantMsg: []string{"the server has no exec route (HTTP 404); upgrade the server to " + client.ExecServerVersion}}, + {name: "503 no ready pod", wantCode: 255, + refuse: jsonRefusal(http.StatusServiceUnavailable, "no_ready_pod", "No ready pod"), + wantMsg: []string{"HTTP 503", "no running and ready pod"}}, + {name: "unknown stack name", args: []string{"nope", "web", "--", "ls"}, wantCode: 255, + handler: exitWith(0, "", ""), + wantMsg: []string{`no stack found with name "nope"`}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := newExecCmdServer(t, tt.refuse, tt.handler) + e := setupExecTest(t, srv.URL) + args := tt.args + if args == nil { + args = []string{"42", "web", "--", "ls"} + } + err := e.run(t, args...) + if tt.wantNil { + require.NoError(t, err) + } else { + ee := requireExit(t, err, tt.wantCode) + if tt.wantErrNil { + assert.NoError(t, ee.Err, "no message for a remote exit code") + assert.Equal(t, fmt.Sprintf("exit status %d", tt.wantCode), ee.Error()) + } else { + require.Error(t, ee.Err) + } + for _, m := range tt.wantMsg { + assert.Contains(t, err.Error(), m) + } + } + assert.Equal(t, tt.wantStdout, e.stdout.String(), "stdout holds only the remote stdout") + if tt.check != nil { + tt.check(t, err) + } + if tt.wantDial { + assert.Equal(t, int32(1), srv.upgrades.Load()) + } else { + assert.Equal(t, int32(0), srv.upgrades.Load()) + } + }) + } +} + +func TestStackExecCmd_RemoteExitPrintsNothingExtra(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(7, "remote out\n", "remote err\n")) + e := setupExecTest(t, srv.URL) + err := e.run(t, "42", "web", "--", "false") + ee := requireExit(t, err, 7) + assert.Nil(t, ee.Err) + assert.Equal(t, "remote out\n", e.stdout.String()) + assert.Equal(t, "remote err\n", e.stderr.String(), "stderr holds only the remote stderr") +} + +func TestStackExecCmd_OutputFlagsDoNotChangeStdout(t *testing.T) { + const remote = "\x00binary\xff\n{\"not\": \"json\"}\n" + for _, flags := range [][]string{nil, {"-o", "json"}, {"-o", "yaml"}, {"-q"}, {"-o", "json", "-q"}} { + t.Run(strings.Join(append([]string{"default"}, flags...), " "), func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, remote, "remote err")) + e := setupExecTest(t, srv.URL) + args := append(append([]string{}, flags...), "42", "web", "--", "cat", "x") + require.NoError(t, e.run(t, args...)) + assert.Equal(t, remote, e.stdout.String(), "stdout is exactly the remote stdout") + assert.Equal(t, "remote err", e.stderr.String()) + }) + } +} + +func TestStackExecCmd_DebugShowsPodButNeverOutputOrCommand(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "SECRET-STDOUT", "remote-stderr")) + e := setupExecTest(t, srv.URL) + require.NoError(t, e.run(t, "--debug", "my-stack", "web", "--", "printenv", "SECRET-ARG")) + stderr := e.stderr.String() + assert.Contains(t, stderr, "exec: started in pod web-0, container php") + assert.NotContains(t, stderr, "SECRET-STDOUT", "remote stdout never on stderr") + assert.NotContains(t, stderr, "SECRET-ARG", "the argv is never logged") + assert.NotContains(t, stderr, "printenv") + assert.Equal(t, 1, strings.Count(stderr, "remote-stderr"), "remote stderr once, not logged again") + assert.Equal(t, "SECRET-STDOUT", e.stdout.String()) + assert.Equal(t, int32(1), srv.lists.Load(), "the name was resolved") +} + +func TestStackExecCmd_InsecureNeedsAllowFlag(t *testing.T) { + t.Run("--insecure is refused before any request", func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "out", "")) + e := setupExecTest(t, srv.URL) + err := e.run(t, "--insecure", "my-stack", "web", "--", "ls") + requireExit(t, err, 255) + assert.Equal(t, "exec refuses --insecure: without TLS verification a man in the middle can read the commands and their output; add --allow-insecure-stream to accept that risk", err.Error()) + assert.Equal(t, int32(0), srv.requests.Load(), "refused before any request") + assert.Empty(t, e.stdout.String()) + }) + t.Run("insecure context is refused", func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "out", "")) + e := setupExecTest(t, srv.URL) + dir := os.Getenv("STACKCTL_CONFIG_DIR") + require.NotEmpty(t, dir) + cfgYAML := "current-context: test\ncontexts:\n test:\n api-url: " + srv.URL + "\n insecure: true\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(cfgYAML), 0o600)) + err := e.run(t, "42", "web", "--", "ls") + requireExit(t, err, 255) + assert.Contains(t, err.Error(), "exec refuses --insecure") + assert.Equal(t, int32(0), srv.requests.Load()) + }) + t.Run("http to a remote host is refused", func(t *testing.T) { + // .invalid never resolves (RFC 2606): a dial would fail differently. + e := setupExecTest(t, "http://stacks.invalid") + err := e.run(t, "42", "web", "--", "ls") + requireExit(t, err, 255) + assert.Equal(t, "exec refuses an http:// API URL to a host that is not loopback: a man in the middle can read the commands and their output; use https, or add --allow-insecure-stream to accept that risk", err.Error()) + assert.Empty(t, e.stdout.String()) + }) + t.Run("--allow-insecure-stream warns and runs", func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "out", "")) + e := setupExecTest(t, srv.URL) + require.NoError(t, e.run(t, "--insecure", "--allow-insecure-stream", "42", "web", "--", "ls")) + assert.Contains(t, e.stderr.String(), "Warning: --allow-insecure-stream: the commands and their output are not protected against a man in the middle\n") + assert.Equal(t, "out", e.stdout.String()) + assert.Equal(t, int32(1), srv.upgrades.Load()) + }) +} + +// waitStdout waits until the buffer holds want. +func waitStdout(t *testing.T, b *lockedBuffer, want string, done <-chan error) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for b.String() != want { + select { + case err := <-done: + t.Fatalf("command ended early: %v", err) + default: + } + if time.Now().After(deadline) { + t.Fatalf("stdout %q, want %q", b.String(), want) + } + time.Sleep(5 * time.Millisecond) + } +} + +func waitCloseCode(t *testing.T, gotClose <-chan int) int { + t.Helper() + select { + case code := <-gotClose: + return code + case <-time.After(5 * time.Second): + t.Fatal("the server got no close") + return 0 + } +} + +func TestStackExecCmd_InterruptExits130(t *testing.T) { + t.Run("no exit frame", func(t *testing.T) { + gotClose := make(chan int, 1) + srv := newExecCmdServer(t, nil, waitClientClose(gotClose, nil)) + e := setupExecTest(t, srv.URL) + ignore := goleak.IgnoreCurrent() + + done := e.start("42", "web", "--", "sleep", "100") + waitStdout(t, e.stdout, "x", done) + e.signals <- os.Interrupt + assert.Equal(t, websocket.CloseNormalClosure, waitCloseCode(t, gotClose), "Ctrl-C sends close 1000") + err := waitDone(t, done) + ee := requireExit(t, err, 130) + assert.Equal(t, "interrupted; the command can still run on the server", ee.Err.Error()) + select { + case code := <-e.forced: + t.Fatalf("execForceExit(%d) after one signal", code) + default: + } + srv.Close() + goleak.VerifyNone(t, ignore) + }) + t.Run("exit frame inside the wait", func(t *testing.T) { + gotClose := make(chan int, 1) + srv := newExecCmdServer(t, nil, waitClientClose(gotClose, func(ws *websocket.Conn) { + wsJSON(ws, map[string]any{"type": "exit", "exit_code": 143}) + _ = ws.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseNormalClosure, ""), time.Now().Add(time.Second)) + })) + e := setupExecTest(t, srv.URL) + execCancelWait = 5 * time.Second + done := e.start("42", "web", "--", "sleep", "100") + waitStdout(t, e.stdout, "x", done) + start := time.Now() + e.signals <- syscall.SIGTERM + err := waitDone(t, done) + ee := requireExit(t, err, 130) + assert.Equal(t, "interrupted; the command ended with exit code 143", ee.Err.Error()) + assert.Less(t, time.Since(start), 3*time.Second, "returns at the exit frame") + assert.Equal(t, websocket.CloseNormalClosure, waitCloseCode(t, gotClose)) + }) + t.Run("during the dial", func(t *testing.T) { + // The server holds the upgrade until the end of the test. + dialing, release := make(chan struct{}), make(chan struct{}) + var once sync.Once + srv := newExecCmdServer(t, func(http.ResponseWriter) bool { + once.Do(func() { close(dialing) }) + select { + case <-release: + case <-time.After(5 * time.Second): + } + return true + }, exitWith(0, "out", "")) + t.Cleanup(func() { close(release) }) // before srv.Close + e := setupExecTest(t, srv.URL) + done := e.start("42", "web", "--", "rm", "-rf", "/tmp/x") + select { + case <-dialing: + case <-time.After(5 * time.Second): + t.Fatal("the dial did not reach the server") + } + start := time.Now() + e.signals <- os.Interrupt + ee := requireExit(t, waitDone(t, done), 130) + assert.Equal(t, "interrupted before the command started", ee.Err.Error()) + assert.Less(t, time.Since(start), 3*time.Second, "Ctrl-C ends the dial") + assert.Nil(t, srv.lastCommand(), "no start message") + }) + t.Run("after the dial, before the started frame", func(t *testing.T) { + gotStart := make(chan struct{}) + srv := newExecCmdServer(t, nil, func(ws *websocket.Conn) { + close(gotStart) + // No started frame; the default close handler answers the close + // of the client. + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + _, _, _ = ws.ReadMessage() + }) + e := setupExecTest(t, srv.URL) + execCancelWait = 5 * time.Second + done := e.start("42", "web", "--", "rm", "-rf", "/tmp/x") + select { + case <-gotStart: + case <-time.After(5 * time.Second): + t.Fatal("the server got no start message") + } + start := time.Now() + e.signals <- os.Interrupt + ee := requireExit(t, waitDone(t, done), 130) + assert.Equal(t, "interrupted before the command started", ee.Err.Error()) + assert.Less(t, time.Since(start), 3*time.Second, "the close of the server ends the wait") + assert.Equal(t, int32(1), srv.upgrades.Load()) + }) + t.Run("before the command started", func(t *testing.T) { + srv := newExecCmdServer(t, nil, exitWith(0, "out", "")) + srv.listDelay = 500 * time.Millisecond // the signal is handled during the name resolution + e := setupExecTest(t, srv.URL) + e.signals <- os.Interrupt + err := e.run(t, "my-stack", "web", "--", "ls") + ee := requireExit(t, err, 130) + assert.Equal(t, "interrupted before the command started", ee.Err.Error()) + assert.Equal(t, int32(0), srv.upgrades.Load(), "no exec") + assert.Empty(t, e.stdout.String()) + }) +} + +func TestStackExecCmd_SecondInterruptForcesExit(t *testing.T) { + gotClose := make(chan int, 1) + srv := newExecCmdServer(t, nil, waitClientClose(gotClose, nil)) + e := setupExecTest(t, srv.URL) + const cancelWait = 3 * time.Second + execCancelWait = cancelWait + done := e.start("42", "web", "--", "sleep", "100") + waitStdout(t, e.stdout, "x", done) + + e.signals <- os.Interrupt + require.Equal(t, websocket.CloseNormalClosure, waitCloseCode(t, gotClose)) + second := time.Now() + e.signals <- os.Interrupt + select { + case code := <-e.forced: + assert.Equal(t, 130, code) + assert.Less(t, time.Since(second), cancelWait/2, "the second signal exits at once, not after the wait") + case <-time.After(cancelWait / 2): + t.Fatal("execForceExit was not called after the second signal") + } + assert.Contains(t, e.stderr.String(), "interrupted\n") + // The replaced execForceExit returns: the command still ends after the + // wait, with 130. + requireExit(t, waitDone(t, done), 130) +} + +// blockingWriter passes writes to buf until a write contains block; from +// then on every write waits for release (a paused 2>&1 | less). blocked is +// closed at the first such write. +type blockingWriter struct { + buf *lockedBuffer + block string + on atomic.Bool + once sync.Once + blocked chan struct{} + release chan struct{} + freed sync.Once +} + +func newBlockingWriter(buf *lockedBuffer, block string) *blockingWriter { + return &blockingWriter{buf: buf, block: block, blocked: make(chan struct{}), release: make(chan struct{})} +} + +func (w *blockingWriter) Write(p []byte) (int, error) { + if strings.Contains(string(p), w.block) { + w.on.Store(true) + } + if w.on.Load() { + w.once.Do(func() { close(w.blocked) }) + <-w.release + } + return w.buf.Write(p) +} + +func (w *blockingWriter) free() { w.freed.Do(func() { close(w.release) }) } + +func TestStackExecCmd_SecondInterruptWithABlockedStderr(t *testing.T) { + // The reader holds the lock of the stderr writer in a blocked write of + // the remote stderr. The second signal must still reach execForceExit. + gotClose := make(chan int, 1) + srv := newExecCmdServer(t, nil, func(ws *websocket.Conn) { + ws.SetCloseHandler(func(int, string) error { return nil }) + wsJSON(ws, map[string]any{"type": "started", "pod": "web-0", "container": "php"}) + wsStream(ws, client.ExecStreamStderr, "BLOCK\n") + _ = ws.SetReadDeadline(time.Now().Add(10 * time.Second)) + _, _, err := ws.ReadMessage() + code := 0 + var ce *websocket.CloseError + if errors.As(err, &ce) { + code = ce.Code + } + gotClose <- code + _ = ws.NetConn().SetReadDeadline(time.Now().Add(10 * time.Second)) + _, _ = io.Copy(io.Discard, ws.NetConn()) + }) + e := setupExecTest(t, srv.URL) + stderr := newBlockingWriter(e.stderr, "BLOCK") + var done <-chan error + ended := false + // Runs before the restore of setupExecTest: free the writer and wait for + // the command, so a failed run never calls the real execForceExit. + t.Cleanup(func() { + stderr.free() + if done != nil && !ended { + select { + case <-done: + case <-time.After(10 * time.Second): + } + } + }) + rootCmd.SetErr(stderr) + ignore := goleak.IgnoreCurrent() + + done = e.start("42", "web", "--", "sleep", "100") + select { + case <-stderr.blocked: + case err := <-done: + t.Fatalf("command ended early: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("the remote stderr write did not block") + } + e.signals <- os.Interrupt + require.Equal(t, websocket.CloseNormalClosure, waitCloseCode(t, gotClose), "the first signal sends close 1000") + second := time.Now() + e.signals <- os.Interrupt + select { + case code := <-e.forced: + assert.Equal(t, 130, code) + assert.Less(t, time.Since(second), time.Second, "the note waits at most execNoteWait") + case <-time.After(3 * time.Second): + t.Fatal("execForceExit was not called: the second signal blocked on stderr") + } + select { + case err := <-done: + t.Fatalf("command ended while stderr was blocked: %v", err) + default: + } + + // The real execForceExit ends the process; the test one returns, so free + // the writer and let the command end. + stderr.free() + err := waitDone(t, done) + ended = true + ee := requireExit(t, err, 130) + assert.Equal(t, "interrupted; the command can still run on the server", ee.Err.Error()) + // Both blocked writes complete after free, in either order. + assert.Eventually(t, func() bool { + out := e.stderr.String() + return strings.Contains(out, "BLOCK\n") && strings.Contains(out, "interrupted\n") + }, 5*time.Second, 5*time.Millisecond, "stderr %q", e.stderr.String()) + srv.Close() + goleak.VerifyNone(t, ignore) +} + +func TestWriteNoteNoWait(t *testing.T) { + t.Run("a free writer gets the note", func(t *testing.T) { + var b lockedBuffer + writeNoteNoWait(&b, "interrupted", time.Second) + assert.Equal(t, "interrupted\n", b.String()) + }) + t.Run("a blocked writer delays at most wait", func(t *testing.T) { + var b lockedBuffer + w := newBlockingWriter(&b, "") + w.on.Store(true) + defer w.free() + start := time.Now() + writeNoteNoWait(w, "interrupted", 50*time.Millisecond) + assert.Less(t, time.Since(start), time.Second) + assert.Empty(t, b.String()) + }) +} + +func TestStackExecCmd_Registered(t *testing.T) { + found, _, err := rootCmd.Find([]string{"stack", "exec"}) + require.NoError(t, err) + assert.Equal(t, stackExecCmd, found) + for _, s := range []string{"124", "130", "255", "--allow-insecure-stream", "sh -c", "secrets", "stackctl login", "escape", "OSC 52", "cat -v"} { + assert.Contains(t, found.Long, s) + } + f := found.Flags().ShorthandLookup("c") + require.NotNil(t, f) + assert.Equal(t, "container", f.Name) +} + +func TestRemoteExit(t *testing.T) { + t.Parallel() + tests := []struct { + code int + wantNil bool + wantCode int + wantMsg bool + }{ + {0, true, 0, false}, + {1, false, 1, false}, + {7, false, 7, false}, + {124, false, 124, false}, + {255, false, 255, false}, + {256, false, 255, true}, + {-1, false, 255, true}, + {1 << 20, false, 255, true}, + } + for _, tt := range tests { + t.Run(fmt.Sprint(tt.code), func(t *testing.T) { + t.Parallel() + err := remoteExit(tt.code) + if tt.wantNil { + assert.NoError(t, err) + return + } + var ee *ExitError + require.ErrorAs(t, err, &ee) + assert.Equal(t, tt.wantCode, ee.Code) + assert.Equal(t, tt.wantMsg, ee.Err != nil) + }) + } +} + +// TestInterruptedExit covers the exit mapping of an interrupted exec. A +// close frame of the server can win the race with the cancel; without +// started or output before it the command did not run. +func TestInterruptedExit(t *testing.T) { + t.Parallel() + tests := []struct { + name string + res *client.ExecResult + err error + want string + }{ + {name: "exit frame", res: &client.ExecResult{ExitCode: 143}, + want: "interrupted; the command ended with exit code 143"}, + {name: "before the dial", err: errExecNotStarted, + want: "interrupted before the command started"}, + {name: "canceled before started", err: &client.ExecCanceledError{Err: context.Canceled}, + want: "interrupted before the command started"}, + {name: "canceled after started", err: &client.ExecCanceledError{Started: true, Err: context.Canceled}, + want: "interrupted; the command can still run on the server"}, + {name: "server close before started keeps the server text", + err: &client.ExecClosedError{Code: client.CloseGoingAway, Reason: "server shutdown"}, + want: "interrupted before the command started: exec closed: the server is shutting down; the server did not start the command"}, + {name: "server refused the command", + err: &client.ExecClosedError{Code: client.CloseBadStart, Reason: "command too long"}, + want: "interrupted before the command started: exec failed: the server refused the command (too long or not valid): command too long"}, + {name: "server close after started", err: &client.ExecClosedError{Code: client.CloseGoingAway, Reason: "server shutdown", Started: true}, + want: "interrupted; the command can still run on the server"}, + {name: "lost connection (no close frame)", err: &client.ExecClosedError{}, + want: "interrupted; the command can still run on the server"}, + {name: "time limit", err: client.ErrExecTimeLimit, + want: "interrupted; the command can still run on the server"}, + {name: "other error", err: errors.New("boom"), + want: "interrupted; the command can still run on the server"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ee := interruptedExit(tt.res, tt.err) + require.NotNil(t, ee) + assert.Equal(t, exitInterrupted, ee.Code) + assert.Equal(t, tt.want, ee.Err.Error()) + var ce *client.ExecClosedError + if errors.As(tt.err, &ce) && ce.Code != 0 && !ce.Started { + assert.ErrorAs(t, ee, &ce, "the close error stays in the chain") + } + }) + } +} + +func TestExitError(t *testing.T) { + t.Parallel() + assert.Equal(t, "exit status 7", (&ExitError{Code: 7}).Error()) + assert.Nil(t, (&ExitError{Code: 7}).Unwrap()) + cause := errors.New("boom") + wrapped := fmt.Errorf("outer: %w", &ExitError{Code: 255, Err: cause}) + var ee *ExitError + require.ErrorAs(t, wrapped, &ee) + assert.Equal(t, 255, ee.Code) + assert.Equal(t, "boom", ee.Error()) + assert.ErrorIs(t, wrapped, cause) +} diff --git a/cli/cmd/stack_portforward.go b/cli/cmd/stack_portforward.go index 91f72cd..d5fa342 100644 --- a/cli/cmd/stack_portforward.go +++ b/cli/cmd/stack_portforward.go @@ -286,13 +286,38 @@ func (s *syncWriter) Write(p []byte) (int, error) { return s.w.Write(p) } -// streamInsecure reports whether TLS verification is off (flag or context). -func streamInsecure() bool { - if flagInsecure { - return true +// streamRisk describes what a stream command (port-forward, exec) exposes +// without TLS, for checkStreamTransport. +type streamRisk struct { + // op names the command in the error, for example "port-forward". + op string + // data is what a man in the middle can read, for example "the tunnel + // data". + data string + // warning is the text of the warning when --allow-insecure-stream is set. + warning string +} + +// checkStreamTransport refuses a stream command when TLS verification is off +// (--insecure or the insecure setting of the context) or the API URL is +// http:// to a host that is not loopback, unless the command has +// --allow-insecure-stream; then it writes a warning to stderr. Call it before +// any request. +func checkStreamTransport(cmd *cobra.Command, c *client.Client, stderr io.Writer, risk streamRisk) error { + insecure, plaintext := effectiveInsecure(), plaintextRemoteURL(c.BaseURL) + if !insecure && !plaintext { + return nil + } + allow, _ := cmd.Flags().GetBool("allow-insecure-stream") + switch { + case allow: + fmt.Fprintln(stderr, "Warning: --allow-insecure-stream: "+risk.warning) + return nil + case insecure: + return fmt.Errorf("%s refuses --insecure: without TLS verification a man in the middle can read %s; add --allow-insecure-stream to accept that risk", risk.op, risk.data) + default: + return fmt.Errorf("%s refuses an http:// API URL to a host that is not loopback: a man in the middle can read %s; use https, or add --allow-insecure-stream to accept that risk", risk.op, risk.data) } - ctx := cfg.CurrentCtx() - return ctx != nil && ctx.Insecure } // portForwardFatal reports whether err stops the command: every later @@ -341,17 +366,12 @@ func runPortForward(ctx context.Context, cmd *cobra.Command, args []string) erro if err != nil { return err } - insecure, plaintext := streamInsecure(), plaintextRemoteURL(c.BaseURL) - if insecure || plaintext { - allow, _ := cmd.Flags().GetBool("allow-insecure-stream") - switch { - case allow: - fmt.Fprintln(stderr, "Warning: --allow-insecure-stream: the tunnel data is not protected against a man in the middle") - case insecure: - return errors.New("port-forward refuses --insecure: without TLS verification a man in the middle can read the tunnel data; add --allow-insecure-stream to accept that risk") - default: - return errors.New("port-forward refuses an http:// API URL to a host that is not loopback: a man in the middle can read the tunnel data; use https, or add --allow-insecure-stream to accept that risk") - } + if err := checkStreamTransport(cmd, c, stderr, streamRisk{ + op: "port-forward", + data: "the tunnel data", + warning: "the tunnel data is not protected against a man in the middle", + }); err != nil { + return err } c.WarnWriter = stderr id, err := resolveStackID(c, args[0]) diff --git a/cli/cmd/stack_portforward_test.go b/cli/cmd/stack_portforward_test.go index 8a9600e..718694b 100644 --- a/cli/cmd/stack_portforward_test.go +++ b/cli/cmd/stack_portforward_test.go @@ -724,3 +724,33 @@ func TestPortForwardCmd_SuccessResets429Count(t *testing.T) { assert.Equal(t, int32(2), srv.upgrades.Load()) assert.Equal(t, 4, strings.Count(stderr.String(), "HTTP 429")) } + +func TestPortForwardCmd_InsecureTextsUnchanged(t *testing.T) { + // The check moved to checkStreamTransport; the port-forward texts stay. + t.Run("--insecure", func(t *testing.T) { + srv := newPFTestServer(t, nil) + _, _ = setupPortForwardTest(t, srv.URL) + flagInsecure = true + err := waitDone(t, startPortForward(context.Background(), "42", "mysql", "0:3306")) + require.Error(t, err) + assert.Equal(t, "port-forward refuses --insecure: without TLS verification a man in the middle can read the tunnel data; add --allow-insecure-stream to accept that risk", err.Error()) + }) + t.Run("http remote", func(t *testing.T) { + _, _ = setupPortForwardTest(t, "http://stacks.invalid") + err := waitDone(t, startPortForward(context.Background(), "42", "mysql", "0:3306")) + require.Error(t, err) + assert.Equal(t, "port-forward refuses an http:// API URL to a host that is not loopback: a man in the middle can read the tunnel data; use https, or add --allow-insecure-stream to accept that risk", err.Error()) + }) + t.Run("warning", func(t *testing.T) { + srv := newPFTestServer(t, nil) + stdout, stderr := setupPortForwardTest(t, srv.URL) + flagInsecure = true + require.NoError(t, stackPortForwardCmd.Flags().Set("allow-insecure-stream", "true")) + ctx, cancel := context.WithCancel(context.Background()) + done := startPortForward(ctx, "42", "mysql", "0:3306") + waitForwarding(t, stdout, done) + cancel() + require.NoError(t, waitDone(t, done)) + assert.Contains(t, stderr.String(), "Warning: --allow-insecure-stream: the tunnel data is not protected against a man in the middle\n") + }) +} diff --git a/cli/main.go b/cli/main.go index 916a12d..c8f703c 100644 --- a/cli/main.go +++ b/cli/main.go @@ -1,6 +1,7 @@ package main import ( + "errors" "fmt" "os" @@ -17,6 +18,15 @@ var ( func main() { cmd.SetVersionInfo(version, commit, date) if err := cmd.Execute(); err != nil { + var exitErr *cmd.ExitError + if errors.As(err, &exitErr) { + if exitErr.Err != nil { + // The whole chain: a wrapper around the ExitError adds its + // context. ExitError.Error() is the text of Err. + fmt.Fprintln(os.Stderr, err) + } + os.Exit(exitErr.Code) + } fmt.Fprintln(os.Stderr, err) os.Exit(1) } diff --git a/cli/pkg/client/exec.go b/cli/pkg/client/exec.go new file mode 100644 index 0000000..bfc62d2 --- /dev/null +++ b/cli/pkg/client/exec.go @@ -0,0 +1,543 @@ +package client + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net" + "net/url" + "strings" + "sync" + "time" + + "github.com/gorilla/websocket" + "github.com/omattsson/stackctl/cli/pkg/types" +) + +// execOp names the exec route in handshake errors. +const execOp = "exec" + +// Message types of the exec WebSocket (types.ExecStartMessage, +// types.ExecServerMessage). +const ( + ExecMessageStart = "start" + ExecMessageStarted = "started" + ExecMessageExit = "exit" + ExecMessageError = "error" +) + +// Stream bytes: the first byte of a binary frame of the exec WebSocket. +const ( + ExecStreamStdout byte = 1 + ExecStreamStderr byte = 2 +) + +const ( + // ExecMaxArgs is the largest number of arguments of a command (a fixed + // rule of the server). The total length limit is a server setting + // (STACK_EXEC_MAX_COMMAND_LENGTH) that clients cannot read: a command + // that is too long gets close 4400. + ExecMaxArgs = 256 + // execReadLimit bounds a server frame (the server sends at most 32 KiB). + execReadLimit = 1 << 20 + // DefaultExecCancelWait is how long Exec waits for the exit frame after + // a cancel. + DefaultExecCancelWait = 2 * time.Second + // execExitCloseTimeout bounds the close frame after the exit frame. + execExitCloseTimeout = time.Second + // stillRuns ends the message of a stream that ended without an exit + // code: an exec without a TTY does not always stop the process. + stillRuns = "; the command can still run on the server" +) + +// ErrExecTimeLimit reports that the server closed the exec session at its +// maximum duration (close 4001). The server sends no exit code then. +var ErrExecTimeLimit = errors.New("exec closed: server time limit reached" + stillRuns) + +// ExecRequest is the command of Exec. +type ExecRequest struct { + // Chart is the chart (Helm release name) of the stack. + Chart string + // Container is the container name; empty lets the server choose the + // first allowed container. + Container string + // Command is the program and its arguments. It goes only in the first + // message, never in the URL. The server never uses a shell. + Command []string + // Keepalive changes the ping and read timings (zero fields: defaults). + Keepalive KeepaliveTimings + // CancelWait is how long Exec waits for the exit frame after ctx ends + // (default DefaultExecCancelWait). + CancelWait time.Duration + // OnStarted, when set, runs once when the server sends the started + // frame, with the sanitized pod and container names. It runs on the + // reader goroutine, before any output. + OnStarted func(pod, container string) +} + +// ExecResult is the end of a command that reported an exit code. +type ExecResult struct { + ExitCode int + // Pod and Container are from the started frame (sanitized; empty when + // the server sent none). + Pod string + Container string +} + +// ExecClosedError reports that the exec stream ended without an exit code +// and without a cancel: a close frame of the server (other than 4001, see +// ErrExecTimeLimit), or a lost connection. +type ExecClosedError struct { + // Code is the close code of the server; 0 when the connection was lost + // without a close frame (also a gorilla 1006). + Code int + // Reason is the sanitized close reason. + Reason string + // Message is the sanitized message of the error frame of the server + // (4502, 1011), or "". + Message string + // Started is true when the server sent the started frame or output (a + // binary frame). + Started bool + // APIKey is true when the stream used an API key. + APIKey bool + err error +} + +// notStarted ends the message of a close frame that came before the started +// frame: the server sends started before it runs the command, and its frames +// arrive in order. +const notStarted = "; the server did not start the command" + +// Error returns a message for the user, for example "exec failed: the server +// refused the command (too long or not valid): command is longer than 4096 +// bytes". +func (e *ExecClosedError) Error() string { + // A close frame without the started frame before it: the command did not + // run. Without a close frame (code 0) a started frame can be lost. + end := stillRuns + if !e.Started { + end = notStarted + } + switch e.Code { + case 0: + const lost = "exec: connection lost" + stillRuns + var ne net.Error + var ce *websocket.CloseError + switch { + case errors.Is(e.err, websocket.ErrReadLimit): + return "exec failed: the server sent a frame larger than 1 MiB; stackctl closed the connection" + stillRuns + case errors.As(e.err, &ce) && ce.Code == websocket.CloseAbnormalClosure: + // gorilla's text ("close 1006 (abnormal closure): unexpected + // EOF") says nothing more. + return lost + case errors.As(e.err, &ne) && ne.Timeout(): + return lost + " (no response from the server)" + case e.err != nil: + return lost + " (" + e.err.Error() + ")" + } + return lost + case CloseNormal, closeNoStatus: + if !e.Started { + return "exec: the server closed the connection without an exit code" + notStarted + } + return "exec: connection lost" + stillRuns + " (the server closed the connection without an exit code)" + case ClosePolicy: + return "exec closed: " + e.policyText() + end + case CloseGoingAway: + return "exec closed: the server is shutting down" + end + case CloseBadStart: + return "exec failed: " + CloseCodeText(e.Code, e.Reason, e.APIKey) + case CloseStartTimeout: + return "exec failed: the server received no command in time (close 4408); try again" + case CloseMessageTooBig: + // stackctl sends only the start message. + return "exec failed: the command is too long for the server (close 1009)" + case CloseInternalError: + msg := "exec failed: internal server error" + if e.Message != "" && !strings.EqualFold(e.Message, "internal server error") { + msg += ": " + e.Message + } + return msg + end + case ClosePodStreamError: + msg := "exec failed: pod stream error" + if detail := e.Message; detail != "" { + msg += ": " + detail + } else if e.Reason != "" { + msg += ": " + e.Reason + } + return msg + end + } + return "exec closed: " + CloseCodeText(e.Code, e.Reason, e.APIKey) + end +} + +// policyText is the text of close 1008. It never says "run the command +// again" while the command can still run: exec is not idempotent. +func (e *ExecClosedError) policyText() string { + switch e.Reason { + case CloseReasonAccessLost, CloseReasonAPIKeyExpired: + return CloseCodeText(ClosePolicy, e.Reason, e.APIKey) + case CloseReasonTokenExpired: + if e.Started { + return "token expired (close 1008); check the effect of the command before you run it again, and run 'stackctl login' if a new command fails" + } + return "token expired (close 1008); run 'stackctl login' if a new command fails" + } + reason := e.Reason + if reason == "" { + reason = "policy violation" + } + if e.APIKey { + return reason + " (close 1008); the API key was deleted or its user changed; check it with 'stackctl apikey list'" + } + return reason + " (close 1008); log in again with 'stackctl login'" +} + +// Unwrap returns a *WSClosedError for close code 1008 (so callers that +// handle a revoked /ws connection handle a revoked exec too), the read error +// for a lost connection, else nil. +func (e *ExecClosedError) Unwrap() error { + if e.Code == ClosePolicy { + return &WSClosedError{Code: e.Code, Reason: e.Reason, APIKey: e.APIKey} + } + return e.err +} + +// ExecCanceledError reports that the context of Exec ended (Ctrl-C) before +// an exit frame. errors.Is matches the error of the context +// (context.Canceled or context.DeadlineExceeded). +type ExecCanceledError struct { + // Started is false only when the command surely did not start: the + // context ended before, during or right after the dial (before the + // command was sent), or the server closed the connection (a close + // frame) without a started frame or output before it. It is true when + // the started frame or output came, and also when the stream ended + // without a close frame of the server (a started frame can be unread). + Started bool + // Err is ctx.Err(). + Err error +} + +// Error returns "exec canceled; the command can still run on the server", +// or "exec canceled before the command started". +func (e *ExecCanceledError) Error() string { + if !e.Started { + return "exec canceled before the command started" + } + return "exec canceled" + stillRuns +} + +// Unwrap returns the error of the context. +func (e *ExecCanceledError) Unwrap() error { + return e.Err +} + +// ValidateExecCommand checks the fixed rules of the server for a command: 1 +// to ExecMaxArgs arguments, the first one not empty, no NUL byte. The length +// limit is a server setting and is not checked. +func ValidateExecCommand(command []string) error { + switch { + case len(command) == 0: + return errors.New("the command is empty") + case len(command) > ExecMaxArgs: + return fmt.Errorf("the command has %d arguments; the server allows at most %d", len(command), ExecMaxArgs) + case command[0] == "": + return errors.New("the first argument of the command (the program) is empty") + } + for i, a := range command { + if strings.IndexByte(a, 0) >= 0 { + return fmt.Errorf("argument %d of the command contains a NUL byte", i+1) + } + } + return nil +} + +// execOutcome is the end of the reader goroutine. +type execOutcome struct { + exit *int + pod, container string + // started is true after the started frame or any binary frame (output + // proves that the command ran). + started bool + // startedFrame is true after the started frame (pod and container set). + startedFrame bool + // errMsg is the sanitized message of an error frame. + errMsg string + // err is the read error that ended the stream (nil after an exit frame). + err error + // localErr is a failed write to stdout or stderr. + localErr error +} + +// Exec runs req.Command in a container of the chart req.Chart of a stack +// instance (GET /api/v1/stack-instances/:id/exec?chart=&container=) and +// copies its output: stdout frames to stdout, stderr frames to stderr, each +// write unbuffered and in the order of the server. It never reads stdin and +// never reconnects. +// +// The route accepts only header auth (no ?token= retry). A refused upgrade +// gives a *WSHandshakeError with Op "exec" and the error code of the server. +// +// Results: +// - an exit frame gives the ExecResult (also for a non-zero exit code); +// - close 4001 gives ErrExecTimeLimit; +// - another close or a lost connection gives a *ExecClosedError; +// - a failed write to stdout or stderr gives an error; +// - when ctx ends, Exec sends close 1000 and waits up to req.CancelWait +// for an exit frame: it returns that result with a nil error, else a +// *ExecCanceledError (errors.Is matches ctx.Err()); its Started field +// tells whether the command can have started. +// +// Exec leaves no goroutine behind; it waits for a write to stdout or stderr +// that blocks. It never logs the command or the output; with Debug it logs +// only the pod and the container of the started frame. +func (c *Client) Exec(ctx context.Context, instanceID string, req ExecRequest, stdout, stderr io.Writer) (*ExecResult, error) { + if strings.TrimSpace(req.Chart) == "" { + return nil, errors.New("the chart name must not be empty") + } + if err := ValidateExecCommand(req.Command); err != nil { + return nil, err + } + start, err := json.Marshal(types.ExecStartMessage{Type: ExecMessageStart, Command: req.Command}) + if err != nil { + return nil, fmt.Errorf("encoding the command: %w", err) + } + if stdout == nil { + stdout = io.Discard + } + if stderr == nil { + stderr = io.Discard + } + k := req.Keepalive.withDefaults() + cancelWait := req.CancelWait + if cancelWait <= 0 { + cancelWait = DefaultExecCancelWait + } + + q := url.Values{} + q.Set("chart", req.Chart) + if req.Container != "" { + q.Set("container", req.Container) + } + path := fmt.Sprintf("/api/v1/stack-instances/%s/exec", url.PathEscape(instanceID)) + "?" + q.Encode() + // stream: true: header auth only. dialWSOpts returns before the ?token= + // fallback for a stream route, so the token never goes in the URL. + ws, err := c.dialWSOpts(ctx, path, c.WarnWriter, wsDialOptions{stream: true, op: execOp}) + if err != nil { + if ctx.Err() != nil { + // The command goes in the first message, after the dial. + return nil, &ExecCanceledError{Err: ctx.Err()} + } + var hs *WSHandshakeError + if errors.As(err, &hs) { + hs.Container = req.Container + } + return nil, err + } + if ctx.Err() != nil { + // A cancel right after the dial: never send the command. + _ = ws.Close() + return nil, &ExecCanceledError{Err: ctx.Err()} + } + ws.SetReadLimit(execReadLimit) + extend := setKeepalive(ws, k) + + readDone := make(chan execOutcome, 1) + stopPing := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + readDone <- c.execRead(ws, extend, stdout, stderr, req.OnStarted) + }() + go func() { + defer wg.Done() + pingLoop(ws, k, stopPing) + }() + teardown := func() { + close(stopPing) + _ = ws.Close() + wg.Wait() + } + + // The only data write of the connection. Pings and closes use + // WriteControl, which is safe next to it. + _ = ws.SetWriteDeadline(time.Now().Add(k.WriteTimeout)) + if werr := ws.WriteMessage(websocket.TextMessage, start); werr != nil { + // The reader usually holds the close frame that explains it. + select { + case o := <-readDone: + teardown() + return c.execEnd(o) + case <-ctx.Done(): + teardown() + return nil, &ExecCanceledError{Started: (<-readDone).mayHaveStarted(), Err: ctx.Err()} + case <-time.After(interruptCloseTimeout): + teardown() + return nil, fmt.Errorf("exec: sending the command: %w", werr) + } + } + + select { + case o := <-readDone: + if o.exit != nil || o.localErr != nil { + _ = sendClose(ws, CloseNormal, execExitCloseTimeout) + } + teardown() + return c.execEnd(o) + case <-ctx.Done(): + _ = sendClose(ws, CloseNormal, interruptCloseTimeout) + timer := time.NewTimer(cancelWait) + defer timer.Stop() + var o execOutcome + select { + case o = <-readDone: + teardown() + case <-timer.C: + teardown() + // teardown waited for the reader: its outcome is buffered. + o = <-readDone + } + if o.exit != nil { + return &ExecResult{ExitCode: *o.exit, Pod: o.pod, Container: o.container}, nil + } + return nil, &ExecCanceledError{Started: o.mayHaveStarted(), Err: ctx.Err()} + } +} + +// mayHaveStarted reports whether the command can have started: the reader +// saw the started frame, or it ended without a close frame of the server +// (a lost connection, a read error, or the teardown of stackctl), so a +// started frame can have been lost or unread. Frames of the server arrive in +// order, and the server sends started before it runs the command. +func (o execOutcome) mayHaveStarted() bool { + if o.started { + return true + } + var ce *websocket.CloseError + return !errors.As(o.err, &ce) || ce.Code == websocket.CloseAbnormalClosure +} + +// execEnd classifies the end of the reader goroutine. +func (c *Client) execEnd(o execOutcome) (*ExecResult, error) { + if o.localErr != nil { + return nil, fmt.Errorf("exec: writing the output: %w"+stillRuns, o.localErr) + } + if o.exit != nil { + return &ExecResult{ExitCode: *o.exit, Pod: o.pod, Container: o.container}, nil + } + apiKey := c.APIKey != "" + var ce *websocket.CloseError + // gorilla reports a lost connection as close code 1006; no frame came. + if errors.As(o.err, &ce) && ce.Code != websocket.CloseAbnormalClosure { + if ce.Code == CloseMaxDuration { + return nil, ErrExecTimeLimit + } + return nil, &ExecClosedError{ + Code: ce.Code, Reason: sanitizeServerMessage(ce.Text), Message: o.errMsg, + Started: o.started, APIKey: apiKey, + } + } + return nil, &ExecClosedError{Message: o.errMsg, Started: o.started, APIKey: apiKey, err: o.err} +} + +// execRead is the only reader of ws. It writes the binary frames to stdout +// and stderr and handles the text frames, until the exit frame or a read +// error. Unknown stream bytes, empty binary frames, malformed text frames +// and unknown message types are ignored (a binary frame still counts as +// proof that the command started); no frame is ever logged. +func (c *Client) execRead(ws *websocket.Conn, extend func(), stdout, stderr io.Writer, onStarted func(pod, container string)) (o execOutcome) { + buf := make([]byte, maxTunnelFrame) + for { + extend() + mt, r, err := ws.NextReader() + if err != nil { + o.err = err + return o + } + switch mt { + case websocket.BinaryMessage: + // The server sends output only after it started the command, + // also when the started frame was malformed. + o.started = true + rerr, werr := copyExecFrame(r, buf, extend, stdout, stderr) + if werr != nil { + o.localErr = werr + return o + } + if rerr != nil { + o.err = rerr + return o + } + case websocket.TextMessage: + data, err := io.ReadAll(r) + if err != nil { + o.err = err + return o + } + var msg types.ExecServerMessage + if json.Unmarshal(data, &msg) != nil { + continue + } + switch msg.Type { + case ExecMessageStarted: + if o.startedFrame { + continue + } + o.started, o.startedFrame = true, true + o.pod, o.container = sanitizeServerMessage(msg.Pod), sanitizeServerMessage(msg.Container) + c.debugf("exec: started in pod %s, container %s\n", o.pod, o.container) + if onStarted != nil { + onStarted(o.pod, o.container) + } + case ExecMessageExit: + if msg.ExitCode == nil { + continue + } + code := *msg.ExitCode + o.exit = &code + return o + case ExecMessageError: + o.errMsg = sanitizeServerMessage(msg.Message) + } + } + } +} + +// copyExecFrame writes one binary frame without its stream byte to stdout +// (1) or stderr (2), in chunks as they arrive. A frame without data or with +// another stream byte is read and dropped. rerr is a read error of ws, werr a +// failed local write. +func copyExecFrame(r io.Reader, buf []byte, extend func(), stdout, stderr io.Writer) (rerr, werr error) { + var stream [1]byte + if _, err := io.ReadFull(r, stream[:]); err != nil { + if errors.Is(err, io.EOF) { + return nil, nil // an empty frame + } + return err, nil + } + w := io.Discard + switch stream[0] { + case ExecStreamStdout: + w = stdout + case ExecStreamStderr: + w = stderr + } + for { + n, err := r.Read(buf) + if n > 0 { + extend() + if _, werr := w.Write(buf[:n]); werr != nil { + return nil, werr + } + } + if errors.Is(err, io.EOF) { + return nil, nil + } + if err != nil { + return err, nil + } + } +} diff --git a/cli/pkg/client/exec_test.go b/cli/pkg/client/exec_test.go new file mode 100644 index 0000000..e3b5ed3 --- /dev/null +++ b/cli/pkg/client/exec_test.go @@ -0,0 +1,1498 @@ +package client + +import ( + "bytes" + "context" + "crypto/tls" + "encoding/json" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "sort" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/gorilla/websocket" + "github.com/omattsson/stackctl/cli/pkg/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Leaks: the package TestMain runs goleak.VerifyTestMain, which covers the +// goroutines of Exec (reader, pinger) in every test of this file. The tests +// run in parallel, so a per-test goleak.VerifyNone would see the goroutines +// of other tests. + +const execPath = "/api/v1/stack-instances/42/exec" + +// Secrets of the test clients: no error text may contain them. +const ( + execTestToken = "eyJ.exec-secret-token.sig" + execTestAPIKey = "sk_exec_secret_key" +) + +// execBuf is a bytes.Buffer that the reader goroutine of Exec and the test +// can use at the same time. notify, when set, gets a signal after each write. +type execBuf struct { + mu sync.Mutex + b bytes.Buffer + notify chan struct{} +} + +func (e *execBuf) Write(p []byte) (int, error) { + e.mu.Lock() + n, err := e.b.Write(p) + e.mu.Unlock() + if e.notify != nil { + select { + case e.notify <- struct{}{}: + default: + } + } + return n, err +} + +func (e *execBuf) Bytes() []byte { + e.mu.Lock() + defer e.mu.Unlock() + return bytes.Clone(e.b.Bytes()) +} + +func (e *execBuf) String() string { return string(e.Bytes()) } + +// execSrv serves the exec route with handler after the upgrade. It counts +// all requests and keeps the last request of the route. +type execSrv struct { + *httptest.Server + requests atomic.Int32 + mu sync.Mutex + last *http.Request +} + +func (s *execSrv) lastRequest() *http.Request { + s.mu.Lock() + defer s.mu.Unlock() + return s.last +} + +func newExecSrv(t *testing.T, handler func(r *http.Request, ws *websocket.Conn)) *execSrv { + t.Helper() + s := &execSrv{} + up := websocket.Upgrader{} + s.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + s.requests.Add(1) + if r.URL.Path != execPath { + http.NotFound(w, r) + return + } + s.mu.Lock() + s.last = r.Clone(context.Background()) + s.mu.Unlock() + ws, err := up.Upgrade(w, r, nil) + if err != nil { + return + } + defer ws.Close() + handler(r, ws) + })) + t.Cleanup(s.Close) + return s +} + +// refusingSrv answers every request with status and body, without an +// upgrade, and counts the requests. +func refusingSrv(t *testing.T, status int, body string) (*httptest.Server, *atomic.Int32) { + t.Helper() + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + return srv, &hits +} + +// Server side helpers. They run on the handler goroutine and never call +// t.FailNow. + +// readStart reads the first client message. +func readStart(ws *websocket.Conn) (mt int, raw []byte, err error) { + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + mt, raw, err = ws.ReadMessage() + _ = ws.SetReadDeadline(time.Time{}) + return mt, raw, err +} + +func sendJSON(ws *websocket.Conn, v any) error { + data, err := json.Marshal(v) + if err != nil { + return err + } + return ws.WriteMessage(websocket.TextMessage, data) +} + +func sendStarted(ws *websocket.Conn, pod, container string) error { + return sendJSON(ws, map[string]any{"type": "started", "pod": pod, "container": container}) +} + +// sendStream writes a binary frame: the stream byte, then data. +func sendStream(ws *websocket.Conn, stream byte, data []byte) error { + return ws.WriteMessage(websocket.BinaryMessage, append([]byte{stream}, data...)) +} + +// closeAndDrain writes a close frame and reads until the client answers or +// drops the connection. +func closeAndDrain(ws *websocket.Conn, code int, reason string) { + _ = ws.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(code, reason), time.Now().Add(time.Second)) + drain(ws) +} + +// drain reads until the first read error. +func drain(ws *websocket.Conn) { + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + if _, _, err := ws.ReadMessage(); err != nil { + return + } + } +} + +// sendExit writes the exit frame and close 1000, like the server. +func sendExit(ws *websocket.Conn, code int) { + _ = sendJSON(ws, map[string]any{"type": "exit", "exit_code": code}) + closeAndDrain(ws, websocket.CloseNormalClosure, "") +} + +// serveFrames reads the start message, sends started, runs frames and sends +// exit code. +func serveFrames(code int, frames func(ws *websocket.Conn)) func(*http.Request, *websocket.Conn) { + return func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "web-0", "php") + if frames != nil { + frames(ws) + } + sendExit(ws, code) + } +} + +type execRun struct { + res *ExecResult + err error + stdout, stderr *execBuf +} + +// runExec runs Exec with a 10 s guard, so a hang fails the test. +func runExec(t *testing.T, ctx context.Context, c *Client, req ExecRequest) execRun { + t.Helper() + return waitExec(t, startExec(ctx, c, req, &execBuf{}, &execBuf{})) +} + +func startExec(ctx context.Context, c *Client, req ExecRequest, stdout, stderr *execBuf) <-chan execRun { + done := make(chan execRun, 1) + go func() { + var so, se io.Writer + if stdout != nil { + so = stdout + } + if stderr != nil { + se = stderr + } + res, err := c.Exec(ctx, "42", req, so, se) + done <- execRun{res: res, err: err, stdout: stdout, stderr: stderr} + }() + return done +} + +func waitExec(t *testing.T, done <-chan execRun) execRun { + t.Helper() + select { + case r := <-done: + return r + case <-time.After(10 * time.Second): + t.Fatal("Exec did not return") + return execRun{} + } +} + +func execReq(command ...string) ExecRequest { + if len(command) == 0 { + command = []string{"bin/console", "cache:clear"} + } + return ExecRequest{Chart: "backend", Command: command} +} + +// assertNoSecret checks that an error text holds no credential. +func assertNoSecret(t *testing.T, err error) { + t.Helper() + if err == nil { + return + } + assert.NotContains(t, err.Error(), execTestToken) + assert.NotContains(t, err.Error(), execTestAPIKey) +} + +// terminalControls are texts that must never reach the terminal from the +// server: ESC (CSI), a C1 CSI, bidi overrides and isolates. +var terminalControls = []string{"\x1b", "\u009b", "\u202e", "\u2066", "\u200f"} + +func assertNoTerminalControls(t *testing.T, s string) { + t.Helper() + for _, c := range terminalControls { + assert.NotContains(t, s, c, "terminal control %q in %q", c, s) + } +} + +func TestExec_QueryStartMessageAndAuth(t *testing.T) { + t.Parallel() + tests := []struct { + name string + container string + token, apiKey string + wantHeader string + wantValue string + noHeader string + wantQueryKeys []string + }{ + {name: "bearer without container", token: execTestToken, + wantHeader: "Authorization", wantValue: "Bearer " + execTestToken, noHeader: "X-API-Key", + wantQueryKeys: []string{"chart"}}, + {name: "api key with container", container: "php", apiKey: execTestAPIKey, + wantHeader: "X-API-Key", wantValue: execTestAPIKey, noHeader: "Authorization", + wantQueryKeys: []string{"chart", "container"}}, + {name: "api key wins over a token", container: "php", token: execTestToken, apiKey: execTestAPIKey, + wantHeader: "X-API-Key", wantValue: execTestAPIKey, noHeader: "Authorization", + wantQueryKeys: []string{"chart", "container"}}, + } + command := []string{"bin/console", "cache:clear", "--env=prod", "a b&c=d"} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + starts := make(chan []byte, 1) + frameTypes := make(chan int, 1) + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + mt, raw, err := readStart(ws) + if err != nil { + return + } + frameTypes <- mt + starts <- raw + sendExit(ws, 0) + }) + c := New(srv.URL) + c.Token, c.APIKey = tt.token, tt.apiKey + req := ExecRequest{Chart: "back end&x=1", Container: tt.container, Command: command} + r := runExec(t, context.Background(), c, req) + require.NoError(t, r.err) + assert.Equal(t, 0, r.res.ExitCode) + + hr := srv.lastRequest() + require.NotNil(t, hr) + assert.Equal(t, http.MethodGet, hr.Method) + assert.Equal(t, execPath, hr.URL.Path) + q := hr.URL.Query() + var keys []string + for k := range q { + keys = append(keys, k) + } + sort.Strings(keys) + assert.Equal(t, tt.wantQueryKeys, keys, "only chart and container in the query") + assert.Equal(t, "back end&x=1", q.Get("chart"), "chart is escaped") + if tt.container != "" { + assert.Equal(t, tt.container, q.Get("container")) + } + for _, a := range command { + assert.NotContains(t, hr.URL.RawQuery, strings.ReplaceAll(a, " ", "+"), "the command is not in the URL") + } + assert.NotContains(t, hr.URL.String(), "cache") + assert.Empty(t, q.Get("token"), "no ?token=") + assert.Equal(t, tt.wantValue, hr.Header.Get(tt.wantHeader)) + assert.Empty(t, hr.Header.Get(tt.noHeader)) + assert.True(t, strings.HasPrefix(hr.Header.Get("User-Agent"), "stackctl/")) + + assert.Equal(t, websocket.TextMessage, <-frameTypes, "the start message is a text frame") + raw := <-starts + assert.JSONEq(t, `{"type":"start","command":["bin/console","cache:clear","--env=prod","a b&c=d"]}`, string(raw)) + }) + } +} + +func TestExec_NoQueryTokenAfter401(t *testing.T) { + t.Parallel() + var hits, withToken atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + if r.URL.Query().Has("token") || strings.Contains(r.URL.RawQuery, execTestToken) { + withToken.Add(1) + } + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "Invalid or expired token"}) + })) + defer srv.Close() + + c := New(srv.URL) + c.Token = execTestToken + r := runExec(t, context.Background(), c, execReq()) + var hs *WSHandshakeError + require.ErrorAs(t, r.err, &hs) + assert.Equal(t, http.StatusUnauthorized, hs.StatusCode) + assert.Equal(t, "exec", hs.Op) + assert.Contains(t, r.err.Error(), "exec refused by the server (HTTP 401)") + assert.Contains(t, r.err.Error(), "stackctl login") + assert.Equal(t, int32(1), hits.Load(), "exactly one dial, no ?token= fallback") + assert.Equal(t, int32(0), withToken.Load(), "the JWT never goes into the URL") + assertNoSecret(t, r.err) +} + +func TestExec_StreamsRawBytesToStdoutAndStderr(t *testing.T) { + t.Parallel() + big := bytes.Repeat([]byte("0123456789abcdef"), 6<<10) // 96 KiB, more than one read chunk + srv := newExecSrv(t, serveFrames(0, func(ws *websocket.Conn) { + _ = sendStream(ws, ExecStreamStdout, []byte{0x00, 'a', 0x00}) + _ = sendStream(ws, ExecStreamStderr, []byte{0xff, 0xfe, 'e', 0x80}) + _ = sendStream(ws, ExecStreamStdout, []byte("\xc3\x28 bad utf-8 \r\n")) + _ = sendStream(ws, ExecStreamStdout, big) + _ = sendStream(ws, ExecStreamStderr, []byte("\x1b[31mred\x1b[0m\n")) + })) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.NoError(t, r.err) + wantOut := append([]byte{0x00, 'a', 0x00}, []byte("\xc3\x28 bad utf-8 \r\n")...) + wantOut = append(wantOut, big...) + assert.True(t, bytes.Equal(wantOut, r.stdout.Bytes()), "stdout bytes differ") + assert.Equal(t, []byte("\xff\xfee\x80\x1b[31mred\x1b[0m\n"), r.stderr.Bytes(), "stderr bytes unchanged") +} + +func TestExec_NilWritersDiscard(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, serveFrames(4, func(ws *websocket.Conn) { + _ = sendStream(ws, ExecStreamStdout, []byte("out")) + _ = sendStream(ws, ExecStreamStderr, []byte("err")) + })) + r := waitExec(t, startExec(context.Background(), New(srv.URL), execReq(), nil, nil)) + require.NoError(t, r.err) + assert.Equal(t, 4, r.res.ExitCode) +} + +func TestExec_StreamsBeforeExit(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "web-0", "php") + _ = sendStream(ws, ExecStreamStdout, []byte("first line\n")) + select { + case <-release: + case <-time.After(5 * time.Second): + return // the client never showed the frame: the test fails below + } + _ = sendStream(ws, ExecStreamStdout, []byte("second line\n")) + sendExit(ws, 0) + }) + stdout := &execBuf{notify: make(chan struct{}, 1)} + done := startExec(context.Background(), New(srv.URL), execReq(), stdout, &execBuf{}) + + deadline := time.After(5 * time.Second) + for stdout.String() != "first line\n" { + select { + case <-stdout.notify: + case <-deadline: + t.Fatalf("the first frame did not reach stdout before the exit frame; stdout %q", stdout.String()) + } + } + select { + case <-done: + t.Fatal("Exec returned before the exit frame") + default: + } + close(release) + r := waitExec(t, done) + require.NoError(t, r.err) + assert.Equal(t, "first line\nsecond line\n", stdout.String()) +} + +func TestExec_ExitCodes(t *testing.T) { + t.Parallel() + // The client reports the code of the server as is; cmd maps a code + // outside 0 to 255. + for _, code := range []int{0, 3, 124, 255, 256, -1} { + t.Run(fmt.Sprint(code), func(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, serveFrames(code, nil)) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.NoError(t, r.err) + require.NotNil(t, r.res) + assert.Equal(t, code, r.res.ExitCode) + assert.Equal(t, "web-0", r.res.Pod) + assert.Equal(t, "php", r.res.Container) + }) + } +} + +func TestExec_ExitWithoutStartedFrame(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + sendExit(ws, 2) + }) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.NoError(t, r.err) + assert.Equal(t, &ExecResult{ExitCode: 2}, r.res) +} + +func TestExec_HandshakeErrors(t *testing.T) { + t.Parallel() + tests := []struct { + name string + status int + body string + container string + apiKey bool + wantMsg []string + notMsg []string + check func(t *testing.T, hs *WSHandshakeError) + }{ + {name: "container not allowed names the container", status: 403, container: "php", + body: `{"error":"Container does not allow exec","code":"container_not_allowed"}`, + wantMsg: []string{"exec refused by the server (HTTP 403): Container does not allow exec", `Container "php" does not allow exec`}}, + {name: "container not allowed without -c", status: 403, + body: `{"error":"No container allows exec","code":"container_not_allowed"}`, + wantMsg: []string{"No container of the chart allows exec"}}, + {name: "api key not allowed", status: 403, apiKey: true, + body: `{"error":"API keys cannot open this stream","code":"api_key_not_allowed"}`, + wantMsg: []string{"HTTP 403", "API keys are not allowed for this command", "stackctl login"}}, + {name: "forbidden", status: 403, + body: `{"error":"You do not have permission","code":"forbidden"}`, + wantMsg: []string{"exec refused by the server (HTTP 403): You do not have permission"}}, + {name: "service account token", status: 403, + body: `{"error":"Container holds a token","code":"service_account_token"}`, + wantMsg: []string{"service-account token"}}, + {name: "cluster exec disabled", status: 403, + body: `{"error":"Exec is disabled for the cluster","code":"cluster_exec_disabled"}`, + wantMsg: []string{"The cluster of the stack does not allow exec"}}, + {name: "feature disabled", status: 404, + body: `{"error":"This feature is disabled","code":"feature_disabled"}`, + wantMsg: []string{"This feature is disabled", "The server has exec turned off"}, + check: func(t *testing.T, hs *WSHandshakeError) { + assert.True(t, hs.FeatureDisabled()) + assert.False(t, hs.NoRoute()) + }}, + {name: "plain 404 gives the upgrade hint", status: 404, body: "404 page not found", + wantMsg: []string{"exec failed: the server has no exec route (HTTP 404); upgrade the server to " + ExecServerVersion}, + notMsg: []string{"port-forward"}, + check: func(t *testing.T, hs *WSHandshakeError) { assert.True(t, hs.NoRoute()) }}, + {name: "chart not found", status: 404, + body: `{"error":"Chart not found","code":"chart_not_found"}`, + wantMsg: []string{"Chart not found", "CHART column of 'stackctl stack pods'"}}, + {name: "container not found", status: 404, container: "nope", + body: `{"error":"Container not found","code":"container_not_found"}`, + wantMsg: []string{`runs container "nope"`}}, + {name: "container not found without -c", status: 404, + body: `{"error":"No allowed container","code":"container_not_found"}`, + wantMsg: []string{"exec refused by the server (HTTP 404): No allowed container. No ready pod of the chart runs an allowed container."}, + notMsg: []string{`container ""`}}, + {name: "invalid chart", status: 400, + body: `{"error":"Invalid chart name","code":"invalid_chart"}`, + wantMsg: []string{"exec refused by the server (HTTP 400): Invalid chart name. Check the chart name (the Helm release name): the CHART column of 'stackctl stack pods'."}, + check: func(t *testing.T, hs *WSHandshakeError) { + assert.False(t, hs.UpgradeDropped(), "a 400 with a code is a refusal") + }}, + {name: "invalid container", status: 400, container: "PHP_", + body: `{"error":"Invalid container name","code":"invalid_container"}`, + wantMsg: []string{"exec refused by the server (HTTP 400): Invalid container name. Check the container name."}}, + {name: "token check forbidden", status: 403, + body: `{"error":"Cannot check the pod","code":"token_check_forbidden"}`, + wantMsg: []string{"exec refused by the server (HTTP 403): Cannot check the pod. The server may not check the pod for a service-account token; ask an administrator."}}, + {name: "no ready pod", status: 503, + body: `{"error":"No ready pod","code":"no_ready_pod"}`, + wantMsg: []string{"exec refused by the server (HTTP 503): No ready pod", "no running and ready pod"}}, + {name: "exec limit", status: 429, + body: `{"error":"Too many exec sessions","code":"exec_limit"}`, + wantMsg: []string{"Too many of your exec sessions"}}, + {name: "401 with an api key", status: 401, apiKey: true, + body: `{"error":"Invalid API key"}`, + wantMsg: []string{"HTTP 401", "Check the API key"}}, + {name: "terminal escapes in the JSON body are sanitized", status: 403, container: "php\x1b[2J\u202e", + body: `{"error":"bad \u001b[31mthing\u009b2J\u202eevil\u2066","code":"container_not_allowed"}`, + wantMsg: []string{"HTTP 403", "bad", "evil"}}, + {name: "200 without upgrade", status: 200, body: "ok", + wantMsg: []string{"exec failed: the server answered HTTP 200 without a WebSocket upgrade"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv, hits := refusingSrv(t, tt.status, tt.body) + c := New(srv.URL) + c.Token = execTestToken + if tt.apiKey { + c.APIKey = execTestAPIKey + } + req := execReq() + req.Container = tt.container + r := runExec(t, context.Background(), c, req) + require.Nil(t, r.res) + var hs *WSHandshakeError + require.ErrorAs(t, r.err, &hs) + assert.Equal(t, tt.status, hs.StatusCode) + assert.Equal(t, "exec", hs.Op) + assert.Equal(t, tt.container, hs.Container) + for _, m := range tt.wantMsg { + assert.Contains(t, r.err.Error(), m) + } + for _, m := range tt.notMsg { + assert.NotContains(t, r.err.Error(), m) + } + assertNoTerminalControls(t, r.err.Error()) + assertNoSecret(t, r.err) + assert.Equal(t, int32(1), hits.Load(), "one dial") + if tt.check != nil { + tt.check(t, hs) + } + }) + } +} + +func TestStreamRoutes_PlainNotFoundNamesTheirOwnServerVersion(t *testing.T) { + t.Parallel() + srv, _ := refusingSrv(t, http.StatusNotFound, "404 page not found") + c := New(srv.URL) + + _, err := c.DialPortForward(context.Background(), "42", "mysql", 3306) + require.Error(t, err) + assert.Equal(t, "port-forward failed: the server has no port-forward route (HTTP 404); upgrade the server to "+PortForwardServerVersion, err.Error()) + + r := runExec(t, context.Background(), c, execReq()) + require.Error(t, r.err) + assert.Equal(t, "exec failed: the server has no exec route (HTTP 404); upgrade the server to "+ExecServerVersion, r.err.Error()) +} + +func TestExec_ServerCloses(t *testing.T) { + t.Parallel() + tests := []struct { + name string + code int + reason string + errFrame string + started bool + apiKey bool + wantMsg string + wantTimeout bool + wantWSClosed bool + }{ + {name: "4400 bad start with reason", code: CloseBadStart, reason: "command is longer than 4096 bytes", + wantMsg: "exec failed: the server refused the command (too long or not valid): command is longer than 4096 bytes"}, + {name: "4400 bad start without reason", code: CloseBadStart, + wantMsg: "exec failed: the server refused the command (too long or not valid)"}, + {name: "4408 start timeout", code: CloseStartTimeout, + wantMsg: "exec failed: the server received no command in time (close 4408); try again"}, + {name: "4001 time limit", code: CloseMaxDuration, reason: "maximum duration reached", started: true, wantTimeout: true}, + {name: "1008 session revoked", code: ClosePolicy, reason: CloseReasonSessionRevoked, started: true, wantWSClosed: true, + wantMsg: "exec closed: session revoked (close 1008); log in again with 'stackctl login'; the command can still run on the server"}, + {name: "1008 session revoked before start", code: ClosePolicy, reason: CloseReasonSessionRevoked, wantWSClosed: true, + wantMsg: "exec closed: session revoked (close 1008); log in again with 'stackctl login'; the server did not start the command"}, + {name: "1008 token expired", code: ClosePolicy, reason: CloseReasonTokenExpired, started: true, wantWSClosed: true, + wantMsg: "exec closed: token expired (close 1008); check the effect of the command before you run it again, and run 'stackctl login' if a new command fails; the command can still run on the server"}, + {name: "1008 token expired before start", code: ClosePolicy, reason: CloseReasonTokenExpired, wantWSClosed: true, + wantMsg: "exec closed: token expired (close 1008); run 'stackctl login' if a new command fails; the server did not start the command"}, + {name: "1008 api key", code: ClosePolicy, reason: "api key revoked", apiKey: true, started: true, wantWSClosed: true, + wantMsg: "exec closed: api key revoked (close 1008); the API key was deleted or its user changed; check it with 'stackctl apikey list'; the command can still run on the server"}, + {name: "1008 api key expired", code: ClosePolicy, reason: CloseReasonAPIKeyExpired, apiKey: true, started: true, wantWSClosed: true, + wantMsg: "exec closed: the API key expired; create a new key with 'stackctl apikey create'; the command can still run on the server"}, + {name: "1008 without reason", code: ClosePolicy, started: true, wantWSClosed: true, + wantMsg: "exec closed: policy violation (close 1008); log in again with 'stackctl login'; the command can still run on the server"}, + {name: "1008 access revoked", code: ClosePolicy, reason: CloseReasonAccessLost, started: true, wantWSClosed: true, + wantMsg: "exec closed: access revoked: the stack was stopped, deleted or cleaned, or your access to it changed; the command can still run on the server"}, + {name: "1008 access revoked before start", code: ClosePolicy, reason: CloseReasonAccessLost, wantWSClosed: true, + wantMsg: "exec closed: access revoked: the stack was stopped, deleted or cleaned, or your access to it changed; the server did not start the command"}, + {name: "4502 with an error frame after start", code: ClosePodStreamError, reason: "stream error", errFrame: "container exited unexpectedly", started: true, + wantMsg: "exec failed: pod stream error: container exited unexpectedly; the command can still run on the server"}, + {name: "4502 reason only before start", code: ClosePodStreamError, reason: "exec not found", + wantMsg: "exec failed: pod stream error: exec not found; the server did not start the command"}, + {name: "1011 with an error frame", code: CloseInternalError, errFrame: "audit unavailable", + wantMsg: "exec failed: internal server error: audit unavailable; the server did not start the command"}, + {name: "1011 with the generic message", code: CloseInternalError, errFrame: "Internal server error", + wantMsg: "exec failed: internal server error; the server did not start the command"}, + {name: "1011 after start", code: CloseInternalError, errFrame: "Internal server error", started: true, + wantMsg: "exec failed: internal server error; the command can still run on the server"}, + {name: "1001 server shutdown", code: CloseGoingAway, reason: "server shutdown", started: true, + wantMsg: "exec closed: the server is shutting down; the command can still run on the server"}, + {name: "1001 server shutdown before start", code: CloseGoingAway, reason: "server shutdown", + wantMsg: "exec closed: the server is shutting down; the server did not start the command"}, + {name: "1000 without an exit frame", code: CloseNormal, started: true, + wantMsg: "exec: connection lost; the command can still run on the server (the server closed the connection without an exit code)"}, + {name: "1000 before start", code: CloseNormal, + wantMsg: "exec: the server closed the connection without an exit code; the server did not start the command"}, + {name: "1005 close without a code", code: closeNoStatus, started: true, + wantMsg: "exec: connection lost; the command can still run on the server (the server closed the connection without an exit code)"}, + {name: "1009 message too big", code: CloseMessageTooBig, + wantMsg: "exec failed: the command is too long for the server (close 1009)"}, + {name: "4002 idle timeout", code: CloseIdleTimeout, started: true, + wantMsg: "exec closed: idle timeout (no data in either direction); the command can still run on the server"}, + {name: "unknown code", code: 4999, reason: "odd", started: true, + wantMsg: "exec closed: closed by the server (code 4999): odd; the command can still run on the server"}, + {name: "unknown code before start", code: 4999, reason: "odd", + wantMsg: "exec closed: closed by the server (code 4999): odd; the server did not start the command"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + if tt.started { + _ = sendStarted(ws, "web-0", "php") + _ = sendStream(ws, ExecStreamStdout, []byte("partial")) + } + if tt.errFrame != "" { + _ = sendJSON(ws, map[string]any{"type": "error", "message": tt.errFrame}) + } + if tt.code == closeNoStatus { + _ = ws.WriteControl(websocket.CloseMessage, []byte{}, time.Now().Add(time.Second)) + drain(ws) + return + } + closeAndDrain(ws, tt.code, tt.reason) + }) + c := New(srv.URL) + c.Token = execTestToken + if tt.apiKey { + c.APIKey = execTestAPIKey + } + r := runExec(t, context.Background(), c, execReq()) + require.Error(t, r.err) + assert.Nil(t, r.res) + assertNoSecret(t, r.err) + if tt.started { + assert.Equal(t, "partial", r.stdout.String(), "output before the close is kept") + } + if tt.wantTimeout { + assert.ErrorIs(t, r.err, ErrExecTimeLimit) + assert.Equal(t, "exec closed: server time limit reached; the command can still run on the server", r.err.Error()) + return + } + assert.NotErrorIs(t, r.err, ErrExecTimeLimit) + var ce *ExecClosedError + require.ErrorAs(t, r.err, &ce) + wantCode := tt.code + assert.Equal(t, wantCode, ce.Code) + assert.Equal(t, tt.started, ce.Started) + assert.Equal(t, tt.apiKey, ce.APIKey) + assert.Equal(t, tt.wantMsg, r.err.Error()) + var wsClosed *WSClosedError + assert.Equal(t, tt.wantWSClosed, errors.As(r.err, &wsClosed), "1008 also gives a WSClosedError") + }) + } +} + +func TestExec_ConnectionDropWithoutCloseFrame(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "web-0", "php") + _ = sendStream(ws, ExecStreamStdout, []byte("before the drop")) + _ = ws.NetConn().Close() // abrupt TCP close, no close frame + }) + c := New(srv.URL) + c.Token = execTestToken + r := runExec(t, context.Background(), c, execReq()) + var ce *ExecClosedError + require.ErrorAs(t, r.err, &ce) + assert.Equal(t, 0, ce.Code) + assert.True(t, ce.Started) + // gorilla reports the drop as close 1006; its text is not shown. + assert.Equal(t, "exec: connection lost; the command can still run on the server", r.err.Error()) + assert.NotContains(t, r.err.Error(), "1006") + assert.Equal(t, "before the drop", r.stdout.String()) + assertNoSecret(t, r.err) +} + +func TestExec_SilentServerTripsReadDeadline(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "web-0", "php") + // No reads: the pings of the client get no pong. + select { + case <-release: + case <-time.After(5 * time.Second): + } + }) + t.Cleanup(func() { close(release) }) // runs before srv.Close + req := execReq() + req.Keepalive = KeepaliveTimings{PingInterval: 50 * time.Millisecond, ReadTimeout: 300 * time.Millisecond, WriteTimeout: time.Second} + r := runExec(t, context.Background(), New(srv.URL), req) + var ce *ExecClosedError + require.ErrorAs(t, r.err, &ce) + assert.Equal(t, 0, ce.Code) + assert.Equal(t, "exec: connection lost; the command can still run on the server (no response from the server)", r.err.Error()) +} + +func TestExec_PingsKeepASilentCommandAlive(t *testing.T) { + t.Parallel() + var pings atomic.Int32 + // Generous for a loaded -race runner: 10 pings per read timeout. + const readTimeout = 500 * time.Millisecond + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + ws.SetPingHandler(func(data string) error { + pings.Add(1) + return ws.WriteControl(websocket.PongMessage, []byte(data), time.Now().Add(time.Second)) + }) + readerDone := make(chan struct{}) + go func() { + defer close(readerDone) + for { + if _, _, err := ws.ReadMessage(); err != nil { + return + } + } + }() + _ = sendStarted(ws, "web-0", "php") + time.Sleep(3 * readTimeout) // silent, longer than the read timeout + _ = sendJSON(ws, map[string]any{"type": "exit", "exit_code": 0}) + _ = ws.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(CloseNormal, ""), time.Now().Add(time.Second)) + select { + case <-readerDone: + case <-time.After(5 * time.Second): + } + }) + req := execReq() + req.Keepalive = KeepaliveTimings{PingInterval: 50 * time.Millisecond, ReadTimeout: readTimeout, WriteTimeout: time.Second} + r := runExec(t, context.Background(), New(srv.URL), req) + require.NoError(t, r.err) + assert.Equal(t, 0, r.res.ExitCode) + assert.Greater(t, pings.Load(), int32(1), "the client pings") +} + +// cancelServer reads the start message, sends started and "x", then waits +// for the close of the client without answering it. afterClose runs after +// the close frame; then the server waits until the client drops the TCP +// connection. +func cancelServer(gotClose chan<- int, afterClose func(ws *websocket.Conn)) func(*http.Request, *websocket.Conn) { + return func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + ws.SetCloseHandler(func(int, string) error { return nil }) // no echo + _ = sendStarted(ws, "web-0", "php") + _ = sendStream(ws, ExecStreamStdout, []byte("x")) + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + _, _, err := ws.ReadMessage() + code := 0 + var ce *websocket.CloseError + if errors.As(err, &ce) { + code = ce.Code + } + gotClose <- code + if afterClose != nil { + afterClose(ws) + } + _ = ws.NetConn().SetReadDeadline(time.Now().Add(5 * time.Second)) + _, _ = io.Copy(io.Discard, ws.NetConn()) + } +} + +// waitOutput waits until w holds want. +func waitOutput(t *testing.T, w *execBuf, want string) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for w.String() != want { + if time.Now().After(deadline) { + t.Fatalf("output %q, want %q", w.String(), want) + } + time.Sleep(5 * time.Millisecond) + } +} + +func TestExec_CancelWithoutExitFrameWaitsCancelWait(t *testing.T) { + t.Parallel() + gotClose := make(chan int, 1) + srv := newExecSrv(t, cancelServer(gotClose, nil)) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + req := execReq() + const wait = 300 * time.Millisecond + req.CancelWait = wait + stdout := &execBuf{} + done := startExec(ctx, New(srv.URL), req, stdout, &execBuf{}) + waitOutput(t, stdout, "x") + + start := time.Now() + cancel() + select { + case code := <-gotClose: + assert.Equal(t, websocket.CloseNormalClosure, code, "the client sends close 1000") + case <-time.After(5 * time.Second): + t.Fatal("the server got no close") + } + r := waitExec(t, done) + elapsed := time.Since(start) + assert.ErrorIs(t, r.err, context.Canceled) + assertCanceled(t, r.err, true) + assert.Equal(t, "exec canceled; the command can still run on the server", r.err.Error()) + assert.Nil(t, r.res) + assert.GreaterOrEqual(t, elapsed, wait-50*time.Millisecond, "waits for the exit frame") + assert.Less(t, elapsed, wait+2*time.Second, "but not much longer than CancelWait") +} + +func TestExec_CancelReturnsTheExitFrameInsideCancelWait(t *testing.T) { + t.Parallel() + gotClose := make(chan int, 1) + srv := newExecSrv(t, cancelServer(gotClose, func(ws *websocket.Conn) { + _ = sendJSON(ws, map[string]any{"type": "exit", "exit_code": 143}) + _ = ws.WriteControl(websocket.CloseMessage, websocket.FormatCloseMessage(CloseNormal, ""), time.Now().Add(time.Second)) + })) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + req := execReq() + req.CancelWait = 5 * time.Second + stdout := &execBuf{} + done := startExec(ctx, New(srv.URL), req, stdout, &execBuf{}) + waitOutput(t, stdout, "x") + + start := time.Now() + cancel() + r := waitExec(t, done) + require.NoError(t, r.err) + require.NotNil(t, r.res) + assert.Equal(t, 143, r.res.ExitCode) + assert.Equal(t, "web-0", r.res.Pod) + assert.Less(t, time.Since(start), 3*time.Second, "returns at the exit frame, not at the end of CancelWait") + assert.Equal(t, websocket.CloseNormalClosure, <-gotClose) +} + +func TestExec_CancelWhileTheServerEchoesTheClose(t *testing.T) { + t.Parallel() + // The default close handler of the server answers the close at once: + // Exec returns ctx.Err() without waiting for CancelWait. + gotClose := make(chan int, 1) + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStream(ws, ExecStreamStdout, []byte("x")) + _, _, err := ws.ReadMessage() + var ce *websocket.CloseError + if errors.As(err, &ce) { + gotClose <- ce.Code + } + }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + req := execReq() + req.CancelWait = 5 * time.Second + stdout := &execBuf{} + done := startExec(ctx, New(srv.URL), req, stdout, &execBuf{}) + waitOutput(t, stdout, "x") + start := time.Now() + cancel() + r := waitExec(t, done) + assert.ErrorIs(t, r.err, context.Canceled) + // No started frame, but the output proves that the command ran. + assertCanceled(t, r.err, true) + assert.Less(t, time.Since(start), 3*time.Second) + assert.Equal(t, websocket.CloseNormalClosure, <-gotClose) +} + +func TestExec_CancelledBeforeTheDial(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, serveFrames(0, nil)) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + r := runExec(t, ctx, New(srv.URL), execReq()) + assert.ErrorIs(t, r.err, context.Canceled) + assertCanceled(t, r.err, false) + assert.Nil(t, r.res) +} + +// assertCanceled checks that err is a *ExecCanceledError with started. +func assertCanceled(t *testing.T, err error, started bool) { + t.Helper() + var ce *ExecCanceledError + require.ErrorAs(t, err, &ce, "error %v", err) + assert.Equal(t, started, ce.Started, "Started; error %v", err) + assert.ErrorIs(t, err, context.Canceled) +} + +// holdingSrv accepts every request and holds it without an answer until +// the client gives up (at most 5 s). dialing is closed at the first request. +func holdingSrv(t *testing.T) (srv *httptest.Server, dialing <-chan struct{}) { + t.Helper() + return holdingSrvWith(t, httptest.NewServer) +} + +// holdingSrvWith is holdingSrv with a server constructor, for example +// httptest.NewTLSServer. +func holdingSrvWith(t *testing.T, newServer func(http.Handler) *httptest.Server) (srv *httptest.Server, dialing <-chan struct{}) { + t.Helper() + ch := make(chan struct{}) + var once sync.Once + srv = newServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + once.Do(func() { close(ch) }) + select { + case <-r.Context().Done(): + case <-time.After(5 * time.Second): + } + })) + t.Cleanup(srv.Close) + return srv, ch +} + +func TestExec_CancelDuringTheDial(t *testing.T) { + t.Parallel() + // The server holds the upgrade: gorilla alone would wait for its + // handshake timeout (45 s). + srv, dialing := holdingSrv(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := startExec(ctx, New(srv.URL), execReq(), &execBuf{}, &execBuf{}) + select { + case <-dialing: + case <-time.After(5 * time.Second): + t.Fatal("the dial did not reach the server") + } + start := time.Now() + cancel() + r := waitExec(t, done) + assert.Nil(t, r.res) + assertCanceled(t, r.err, false) + assert.Equal(t, "exec canceled before the command started", r.err.Error()) + assert.Less(t, time.Since(start), 3*time.Second, "the cancel ends the dial") +} + +func TestExec_CancelDuringTheTLSDial(t *testing.T) { + t.Parallel() + // wss: the TLS handshake is done and the server holds the upgrade + // request. The cancel closes the TCP connection under the TLS one. + srv, dialing := holdingSrvWith(t, httptest.NewTLSServer) + require.True(t, strings.HasPrefix(srv.URL, "https://")) + c := New(srv.URL) + c.HTTPClient = &http.Client{Transport: &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // test server certificate + }} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := startExec(ctx, c, execReq(), &execBuf{}, &execBuf{}) + select { + case <-dialing: + case <-time.After(5 * time.Second): + t.Fatal("the dial did not reach the server") + } + start := time.Now() + cancel() + r := waitExec(t, done) + assert.Nil(t, r.res) + assertCanceled(t, r.err, false) + assert.Equal(t, "exec canceled before the command started", r.err.Error()) + assert.Less(t, time.Since(start), 3*time.Second, "the cancel ends the wss dial") +} + +func TestCloseOnCancel_RefusesDialersThatSkipTheWrapper(t *testing.T) { + t.Parallel() + ctx := context.Background() + netDial := func(string, string) (net.Conn, error) { return nil, errors.New("unused") } + tlsDial := func(context.Context, string, string) (net.Conn, error) { return nil, errors.New("unused") } + for name, d := range map[string]*websocket.Dialer{ + "NetDial": {NetDial: netDial}, + "NetDialTLSContext": {NetDialTLSContext: tlsDial}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + got, release, err := closeOnCancel(ctx, d) + require.Error(t, err) + assert.Contains(t, err.Error(), "not supported for stream routes") + assert.Nil(t, got) + assert.Nil(t, release) + }) + } + t.Run("default dialer", func(t *testing.T) { + t.Parallel() + got, release, err := closeOnCancel(ctx, websocket.DefaultDialer) + require.NoError(t, err) + defer release() + assert.NotNil(t, got.NetDialContext) + assert.Nil(t, websocket.DefaultDialer.NetDialContext, "the dialer is copied, not changed") + }) +} + +func TestDialPortForward_CancelDuringTheDial(t *testing.T) { + t.Parallel() + // The same stream dial as exec. + srv, dialing := holdingSrv(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + type result struct{ err error } + done := make(chan result, 1) + go func() { + ws, err := New(srv.URL).DialPortForward(ctx, "42", "mysql", 3306) + if ws != nil { + _ = ws.Close() + } + done <- result{err} + }() + select { + case <-dialing: + case <-time.After(5 * time.Second): + t.Fatal("the dial did not reach the server") + } + start := time.Now() + cancel() + select { + case r := <-done: + assert.ErrorIs(t, r.err, context.Canceled) + assert.Less(t, time.Since(start), 3*time.Second, "the cancel ends the dial") + case <-time.After(10 * time.Second): + t.Fatal("DialPortForward did not return after the cancel") + } +} + +func TestExec_CancelBeforeTheStartedFrame(t *testing.T) { + t.Parallel() + tests := []struct { + name string + // answer: the server answers the close (a close frame); else it + // sends nothing, and the client ends at CancelWait. + answer bool + sendStarted bool + wantStarted bool + }{ + {name: "the server closes without started", answer: true, wantStarted: false}, + {name: "the server closes after started", answer: true, sendStarted: true, wantStarted: true}, + {name: "no answer: started can be unread", wantStarted: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + gotStart := make(chan struct{}) + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + if tt.sendStarted { + _ = sendStarted(ws, "web-0", "php") + } + if !tt.answer { + ws.SetCloseHandler(func(int, string) error { return nil }) + } + close(gotStart) + // The default close handler answers the close of the client. + drain(ws) + }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + req := execReq() + req.CancelWait = 300 * time.Millisecond + var started atomic.Bool + req.OnStarted = func(string, string) { started.Store(true) } + done := startExec(ctx, New(srv.URL), req, &execBuf{}, &execBuf{}) + select { + case <-gotStart: + case <-time.After(5 * time.Second): + t.Fatal("the server got no start message") + } + if tt.sendStarted { + deadline := time.Now().Add(5 * time.Second) + for !started.Load() && time.Now().Before(deadline) { + time.Sleep(5 * time.Millisecond) + } + require.True(t, started.Load(), "the client did not see started") + } + cancel() + r := waitExec(t, done) + assert.Nil(t, r.res) + assertCanceled(t, r.err, tt.wantStarted) + }) + } +} + +func TestExec_OutputProvesTheStart(t *testing.T) { + t.Parallel() + // The started frame is malformed or missing, but output came: the + // command ran, so a close never says that it did not start. + tests := []struct { + name string + started func(ws *websocket.Conn) + stream byte + }{ + {"malformed started frame", func(ws *websocket.Conn) { + _ = ws.WriteMessage(websocket.TextMessage, []byte(`{"type":"started","pod":`)) + }, ExecStreamStdout}, + {"no started frame", func(*websocket.Conn) {}, ExecStreamStderr}, + {"unknown stream byte", func(*websocket.Conn) {}, 9}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + tt.started(ws) + _ = sendStream(ws, tt.stream, []byte("partial")) + closeAndDrain(ws, CloseGoingAway, "server shutdown") + }) + var onStarted atomic.Int32 + req := execReq() + req.OnStarted = func(string, string) { onStarted.Add(1) } + r := runExec(t, context.Background(), New(srv.URL), req) + require.Error(t, r.err) + var ce *ExecClosedError + require.ErrorAs(t, r.err, &ce) + assert.True(t, ce.Started) + assert.Equal(t, "exec closed: the server is shutting down; the command can still run on the server", r.err.Error()) + assert.NotContains(t, r.err.Error(), "did not start") + assert.Zero(t, onStarted.Load(), "OnStarted needs the started frame") + }) + } + t.Run("a started frame after output still sets the pod", func(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStream(ws, ExecStreamStdout, []byte("early")) + _ = sendStarted(ws, "web-0", "php") + sendExit(ws, 0) + }) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.NoError(t, r.err) + assert.Equal(t, "early", r.stdout.String()) + assert.Equal(t, "web-0", r.res.Pod) + assert.Equal(t, "php", r.res.Container) + }) +} + +func TestExec_IgnoresUnknownFrames(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, serveFrames(0, func(ws *websocket.Conn) { + _ = sendStream(ws, 9, []byte("unknown stream")) + _ = sendStream(ws, 0, []byte("stream zero")) + _ = ws.WriteMessage(websocket.BinaryMessage, nil) // empty binary frame + _ = ws.WriteMessage(websocket.TextMessage, []byte("not json")) + _ = ws.WriteMessage(websocket.TextMessage, []byte(`{"type":`)) + _ = sendJSON(ws, map[string]any{"type": "resize", "rows": 40}) + _ = sendJSON(ws, map[string]any{"type": "exit"}) // no exit_code + _ = sendJSON(ws, map[string]any{"type": "exit", "exit_code": "7"}) + _ = sendStarted(ws, "other-pod", "other") // a second started frame + _ = sendStream(ws, ExecStreamStdout, []byte("ok")) + _ = sendStream(ws, ExecStreamStderr, nil) // stream byte only + })) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.NoError(t, r.err) + assert.Equal(t, 0, r.res.ExitCode) + assert.Equal(t, "ok", r.stdout.String()) + assert.Empty(t, r.stderr.String()) + assert.Equal(t, "web-0", r.res.Pod, "the first started frame counts") + assert.Equal(t, "php", r.res.Container) +} + +func TestExec_OnStartedRunsBeforeOutput(t *testing.T) { + t.Parallel() + var mu sync.Mutex + var events []string + record := func(e string) { + mu.Lock() + events = append(events, e) + mu.Unlock() + } + srv := newExecSrv(t, serveFrames(0, func(ws *websocket.Conn) { + _ = sendStream(ws, ExecStreamStdout, []byte("out")) + })) + req := execReq() + var calls atomic.Int32 + req.OnStarted = func(pod, container string) { + calls.Add(1) + record("started " + pod + "/" + container) + } + stdout := writerFunc(func(p []byte) (int, error) { + record("stdout " + string(p)) + return len(p), nil + }) + res, err := New(srv.URL).Exec(context.Background(), "42", req, stdout, io.Discard) + require.NoError(t, err) + assert.Equal(t, 0, res.ExitCode) + assert.Equal(t, int32(1), calls.Load()) + mu.Lock() + defer mu.Unlock() + assert.Equal(t, []string{"started web-0/php", "stdout out"}, events) +} + +type writerFunc func(p []byte) (int, error) + +func (f writerFunc) Write(p []byte) (int, error) { return f(p) } + +func TestExec_DebugLogsOnlyPodAndContainer(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, serveFrames(0, func(ws *websocket.Conn) { + _ = sendStream(ws, ExecStreamStdout, []byte("SECRET-STDOUT")) + _ = sendStream(ws, ExecStreamStderr, []byte("SECRET-STDERR")) + })) + var debug execBuf + c := New(srv.URL) + c.Token = execTestToken + c.Debug = true + c.DebugWriter = &debug + r := runExec(t, context.Background(), c, execReq("printenv", "SECRET-ARG")) + require.NoError(t, r.err) + out := debug.String() + assert.Contains(t, out, "exec: started in pod web-0, container php") + for _, s := range []string{"SECRET-STDOUT", "SECRET-STDERR", "SECRET-ARG", "printenv", execTestToken} { + assert.NotContains(t, out, s) + } +} + +func TestExec_ReadLimit(t *testing.T) { + t.Parallel() + t.Run("a 1 MiB frame passes", func(t *testing.T) { + t.Parallel() + data := bytes.Repeat([]byte{'a'}, execReadLimit-1) // plus the stream byte: 1 MiB + srv := newExecSrv(t, serveFrames(0, func(ws *websocket.Conn) { + _ = sendStream(ws, ExecStreamStdout, data) + })) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.NoError(t, r.err) + assert.Equal(t, len(data), len(r.stdout.Bytes())) + }) + t.Run("a larger frame is refused", func(t *testing.T) { + t.Parallel() + data := bytes.Repeat([]byte{'a'}, execReadLimit) // plus the stream byte: 1 MiB + 1 + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "web-0", "php") + _ = sendStream(ws, ExecStreamStdout, data) + drain(ws) + }) + r := runExec(t, context.Background(), New(srv.URL), execReq()) + require.Error(t, r.err) + assert.ErrorIs(t, r.err, websocket.ErrReadLimit) + var ce *ExecClosedError + require.ErrorAs(t, r.err, &ce) + assert.Equal(t, "exec failed: the server sent a frame larger than 1 MiB; stackctl closed the connection; the command can still run on the server", r.err.Error()) + assert.Empty(t, r.stdout.Bytes(), "nothing of the refused frame is written") + }) +} + +func TestExec_LocalWriteErrorClosesTheStream(t *testing.T) { + t.Parallel() + gotClose := make(chan int, 1) + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStream(ws, ExecStreamStdout, []byte("x")) + _ = ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + _, _, err := ws.ReadMessage() + var ce *websocket.CloseError + if errors.As(err, &ce) { + gotClose <- ce.Code + } + }) + broken := writerFunc(func([]byte) (int, error) { return 0, errors.New("broken pipe") }) + res, err := New(srv.URL).Exec(context.Background(), "42", execReq(), broken, io.Discard) + assert.Nil(t, res) + require.Error(t, err) + assert.Equal(t, "exec: writing the output: broken pipe; the command can still run on the server", err.Error()) + select { + case code := <-gotClose: + assert.Equal(t, websocket.CloseNormalClosure, code) + case <-time.After(5 * time.Second): + t.Fatal("the server got no close") + } +} + +func TestExec_InvalidRequestsDoNotDial(t *testing.T) { + t.Parallel() + tests := []struct { + name string + req ExecRequest + wantErr string + }{ + {"empty chart", ExecRequest{Command: []string{"ls"}}, "the chart name must not be empty"}, + {"blank chart", ExecRequest{Chart: " ", Command: []string{"ls"}}, "the chart name must not be empty"}, + {"no command", ExecRequest{Chart: "backend"}, "the command is empty"}, + {"empty program", ExecRequest{Chart: "backend", Command: []string{"", "x"}}, "the program) is empty"}, + {"NUL byte", ExecRequest{Chart: "backend", Command: []string{"ls", "a\x00b"}}, "argument 2 of the command contains a NUL byte"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, serveFrames(0, nil)) + r := runExec(t, context.Background(), New(srv.URL), tt.req) + require.Error(t, r.err) + assert.Contains(t, r.err.Error(), tt.wantErr) + assert.Equal(t, int32(0), srv.requests.Load(), "no request") + }) + } +} + +func TestExec_SanitizesServerText(t *testing.T) { + t.Parallel() + const dirty = "a\x1b[31mb\u009b2Jc\u202ed\u2066e\u200f" + tests := []struct { + name string + serve func(ws *websocket.Conn) + }{ + {"error frame", func(ws *websocket.Conn) { + _ = sendJSON(ws, map[string]any{"type": "error", "message": dirty}) + closeAndDrain(ws, ClosePodStreamError, "x") + }}, + {"close reason 4400", func(ws *websocket.Conn) { closeAndDrain(ws, CloseBadStart, dirty) }}, + {"close reason 4502", func(ws *websocket.Conn) { closeAndDrain(ws, ClosePodStreamError, dirty) }}, + {"close reason 1008", func(ws *websocket.Conn) { closeAndDrain(ws, ClosePolicy, dirty) }}, + {"close reason of an unknown code", func(ws *websocket.Conn) { closeAndDrain(ws, 4999, dirty) }}, + {"error frame of 1011", func(ws *websocket.Conn) { + _ = sendJSON(ws, map[string]any{"type": "error", "message": dirty}) + closeAndDrain(ws, CloseInternalError, "") + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "pod"+dirty, "ctr"+dirty) + tt.serve(ws) + }) + c := New(srv.URL) + c.Token = execTestToken + r := runExec(t, context.Background(), c, execReq()) + require.Error(t, r.err) + assert.Contains(t, r.err.Error(), "a [31mb 2Jcde", "the text stays, the controls go") + assertNoTerminalControls(t, r.err.Error()) + assertNoSecret(t, r.err) + }) + } + + t.Run("started pod and container", func(t *testing.T) { + t.Parallel() + var gotPod, gotContainer string + var debug execBuf + srv := newExecSrv(t, func(_ *http.Request, ws *websocket.Conn) { + if _, _, err := readStart(ws); err != nil { + return + } + _ = sendStarted(ws, "pod"+dirty, "ctr"+dirty) + sendExit(ws, 0) + }) + c := New(srv.URL) + c.Debug, c.DebugWriter = true, &debug + req := execReq() + req.OnStarted = func(pod, container string) { gotPod, gotContainer = pod, container } + r := runExec(t, context.Background(), c, req) + require.NoError(t, r.err) + for _, s := range []string{gotPod, gotContainer, r.res.Pod, r.res.Container, debug.String()} { + assertNoTerminalControls(t, s) + } + assert.Equal(t, "poda [31mb 2Jcde", r.res.Pod) + assert.Equal(t, "ctra [31mb 2Jcde", r.res.Container) + assert.Equal(t, r.res.Pod, gotPod) + }) +} + +func TestExecClosedError_Unwrap(t *testing.T) { + t.Parallel() + var ws *WSClosedError + e := &ExecClosedError{Code: ClosePolicy, Reason: CloseReasonSessionRevoked} + require.ErrorAs(t, e, &ws) + assert.True(t, ws.SessionRevoked()) + + cause := errors.New("read tcp: reset") + lost := &ExecClosedError{err: cause} + assert.ErrorIs(t, lost, cause) + assert.Equal(t, "exec: connection lost; the command can still run on the server (read tcp: reset)", lost.Error()) + assert.Equal(t, "exec: connection lost; the command can still run on the server", (&ExecClosedError{}).Error()) + + assert.Nil(t, (&ExecClosedError{Code: CloseGoingAway}).Unwrap()) + + abnormal := &ExecClosedError{Started: true, err: &websocket.CloseError{Code: websocket.CloseAbnormalClosure, Text: io.ErrUnexpectedEOF.Error()}} + assert.Equal(t, "exec: connection lost; the command can still run on the server", abnormal.Error(), "no gorilla text for 1006") + abnormal.Started = false + assert.Equal(t, "exec: connection lost; the command can still run on the server", abnormal.Error(), "without a close frame, started can be lost in flight") + assert.Equal(t, "exec: connection lost; the command can still run on the server", (&ExecClosedError{}).Error()) +} + +func TestExecCanceledError(t *testing.T) { + t.Parallel() + before := &ExecCanceledError{Err: context.Canceled} + assert.Equal(t, "exec canceled before the command started", before.Error()) + assert.ErrorIs(t, before, context.Canceled) + after := &ExecCanceledError{Started: true, Err: context.DeadlineExceeded} + assert.Equal(t, "exec canceled; the command can still run on the server", after.Error()) + assert.ErrorIs(t, after, context.DeadlineExceeded) + assert.NotErrorIs(t, after, context.Canceled) +} + +func TestValidateExecCommand(t *testing.T) { + t.Parallel() + args := func(n int) []string { + a := make([]string, n) + for i := range a { + a[i] = "x" + } + return a + } + tests := []struct { + name string + command []string + wantErr string + }{ + {"nil", nil, "the command is empty"}, + {"empty", []string{}, "the command is empty"}, + {"empty first argument", []string{""}, "the first argument of the command (the program) is empty"}, + {"empty later argument is fine", []string{"echo", ""}, ""}, + {"one argument", []string{"ls"}, ""}, + {"256 arguments", args(ExecMaxArgs), ""}, + {"257 arguments", args(ExecMaxArgs + 1), "the command has 257 arguments; the server allows at most 256"}, + {"NUL in the program", []string{"l\x00s"}, "argument 1 of the command contains a NUL byte"}, + {"NUL in an argument", []string{"ls", "-la", "\x00"}, "argument 3 of the command contains a NUL byte"}, + {"spaces and unicode", []string{"sh", "-c", "echo 'hä llo' | wc"}, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := ValidateExecCommand(tt.command) + if tt.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + }) + } +} + +func TestExecWireTypes(t *testing.T) { + t.Parallel() + data, err := json.Marshal(types.ExecStartMessage{Type: ExecMessageStart, Command: []string{"ls"}}) + require.NoError(t, err) + assert.JSONEq(t, `{"type":"start","command":["ls"]}`, string(data)) + + var msg types.ExecServerMessage + require.NoError(t, json.Unmarshal([]byte(`{"type":"exit","exit_code":0}`), &msg)) + require.NotNil(t, msg.ExitCode, "exit code 0 is present") + assert.Equal(t, 0, *msg.ExitCode) +} diff --git a/cli/pkg/client/websocket.go b/cli/pkg/client/websocket.go index 233b493..6c0d95d 100644 --- a/cli/pkg/client/websocket.go +++ b/cli/pkg/client/websocket.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" "github.com/gorilla/websocket" @@ -304,31 +305,13 @@ func (c *Client) dialWSOpts(ctx context.Context, path string, warnWriter io.Writ } } + if opts.stream { + return c.dialStream(ctx, dialer, wsURL, header, token, opts.op) + } conn, resp, err := dialer.DialContext(ctx, wsURL, header) if err == nil { return conn, nil } - if opts.stream { - if resp != nil && resp.StatusCode == http.StatusUnauthorized && c.APIKey == "" && token != "" { - renewed, rerr := c.renewSession(token) - if rerr != nil { - return nil, rerr - } - if renewed { - c.debugf("↻ session renewed after a WebSocket 401; dialing %s again\n", opts.op) - header.Set("Authorization", "Bearer "+c.currentToken()) - conn, resp, err = dialer.DialContext(ctx, wsURL, header) - if err == nil { - return conn, nil - } - } - } - if hsErr := handshakeError(resp, err, c.APIKey != ""); hsErr != nil { - hsErr.Op = opts.op - return nil, hsErr - } - return nil, fmt.Errorf("connecting to the %s WebSocket: %w", opts.op, err) - } // 401 → fall back to ?token= query param when a JWT is configured. // The retry is gated on c.Token because an API-key-only caller has no // token to fall back to. When BOTH APIKey and Token are set, this @@ -370,6 +353,110 @@ const wsAuthRequiredMessage = "Authentication required" // port-forward route (k8s-stack-manager#722). const PortForwardServerVersion = "k8s-stack-manager v0.15.0 or later" +// ExecServerVersion is the first server release with the exec route +// (k8s-stack-manager#723, PR #725), the same release as port-forward. +const ExecServerVersion = "k8s-stack-manager v0.15.0 or later" + +// streamServerVersion returns the server release that has the stream route +// of op. +func streamServerVersion(op string) string { + if op == execOp { + return ExecServerVersion + } + return PortForwardServerVersion +} + +// dialStream dials a stream route (port-forward, exec): header auth only, +// one renewal and one more dial after a 401, and a cancel of ctx ends the +// dial. +func (c *Client) dialStream(ctx context.Context, dialer *websocket.Dialer, wsURL string, header http.Header, token, op string) (*websocket.Conn, error) { + // Ctrl-C must end a dial that a server or proxy holds. + dialer, release, err := closeOnCancel(ctx, dialer) + if err != nil { + return nil, err + } + defer release() + dialed := func(conn *websocket.Conn) (*websocket.Conn, error) { + release() + if ctx.Err() != nil { + // The cancel can have closed the connection. + _ = conn.Close() + return nil, ctx.Err() + } + return conn, nil + } + conn, resp, err := dialer.DialContext(ctx, wsURL, header) + if err == nil { + return dialed(conn) + } + if resp != nil && resp.StatusCode == http.StatusUnauthorized && c.APIKey == "" && token != "" { + renewed, rerr := c.renewSession(token) + if rerr != nil { + return nil, rerr + } + if renewed { + c.debugf("↻ session renewed after a WebSocket 401; dialing %s again\n", op) + header.Set("Authorization", "Bearer "+c.currentToken()) + conn, resp, err = dialer.DialContext(ctx, wsURL, header) + if err == nil { + return dialed(conn) + } + } + } + if ctx.Err() != nil { + // Not the text of the connection that the cancel closed. + return nil, ctx.Err() + } + if hsErr := handshakeError(resp, err, c.APIKey != ""); hsErr != nil { + hsErr.Op = op + return nil, hsErr + } + return nil, fmt.Errorf("connecting to the %s WebSocket: %w", op, err) +} + +// closeOnCancel returns a copy of d whose connections are closed when ctx +// ends, until release is called. gorilla/websocket v1.5.3 uses only the +// deadline of ctx during the handshake, not its cancel: without this, a +// Ctrl-C waits up to the handshake timeout (45 s) while a server or a proxy +// holds the upgrade. Call release when the dial has returned; after a +// successful dial, check ctx.Err(): the cancel can have closed the +// connection. +// +// Only NetDialContext is wrapped. gorilla uses NetDialTLSContext instead of +// it for wss, and the wrapper would hide a NetDial of d, so a dialer with +// either set is refused: the cancel must never be skipped silently. +func closeOnCancel(ctx context.Context, d *websocket.Dialer) (dialer *websocket.Dialer, release func(), err error) { + if d.NetDial != nil || d.NetDialTLSContext != nil { + return nil, nil, errors.New("websocket dialer: NetDial and NetDialTLSContext are not supported for stream routes (the cancel of the dial would be skipped)") + } + cp := *d + base := d.NetDialContext + if base == nil { + base = (&net.Dialer{}).DialContext + } + var mu sync.Mutex + var stops []func() bool + cp.NetDialContext = func(dctx context.Context, network, addr string) (net.Conn, error) { + conn, err := base(dctx, network, addr) + if err != nil { + return nil, err + } + stop := context.AfterFunc(ctx, func() { _ = conn.Close() }) + mu.Lock() + stops = append(stops, stop) + mu.Unlock() + return conn, nil + } + return &cp, func() { + mu.Lock() + defer mu.Unlock() + for _, stop := range stops { + stop() + } + stops = nil + }, nil +} + // maxHandshakeBody is the number of response body bytes that // handshakeError reads. gorilla/websocket keeps at most 1024 bytes. const maxHandshakeBody = 4096 @@ -391,7 +478,10 @@ type WSHandshakeError struct { // APIKey is true when the refused request used an API key (no JWT // fallback). APIKey bool - err error + // Container is the container that an exec asked for; empty when the + // server chooses it. Only used in the message. + Container string + err error } // Error codes of the stream routes (port-forward, exec) of @@ -418,6 +508,17 @@ const ( StreamCodeShuttingDown = "shutting_down" StreamCodeDialFailed = "dial_failed" StreamCodeInternal = "internal" + + // Error codes of the exec route only. + StreamCodeInvalidChart = "invalid_chart" + StreamCodeInvalidContainer = "invalid_container" + StreamCodeClusterExecDisabled = "cluster_exec_disabled" + StreamCodeContainerNotAllowed = "container_not_allowed" + StreamCodeServiceAccountToken = "service_account_token" // #nosec G101 -- an error code, not a credential + StreamCodeTokenCheckForbidden = "token_check_forbidden" // #nosec G101 -- an error code, not a credential + StreamCodeChartNotFound = "chart_not_found" + StreamCodeContainerNotFound = "container_not_found" + StreamCodeExecLimit = "exec_limit" ) // Error returns a message for the user, for example "WebSocket refused by @@ -445,7 +546,7 @@ func (e *WSHandshakeError) streamError() string { return fmt.Sprintf("%s failed: the server answered HTTP %d without a WebSocket upgrade; a proxy between stackctl and the server probably dropped the upgrade", e.Op, e.StatusCode) } if e.NoRoute() { - return fmt.Sprintf("%s failed: the server has no %s route (HTTP 404); upgrade the server to %s", e.Op, e.Op, PortForwardServerVersion) + return fmt.Sprintf("%s failed: the server has no %s route (HTTP 404); upgrade the server to %s", e.Op, e.Op, streamServerVersion(e.Op)) } msg := fmt.Sprintf("%s refused by the server (HTTP %d): %s", e.Op, e.StatusCode, e.Message) switch { @@ -466,9 +567,47 @@ func (e *WSHandshakeError) streamError() string { case StreamCodeTunnelLimit: return msg + ". Close other connections, then try again." } + if e.Op == execOp { + return msg + e.execHint() + } return msg } +// execHint is the advice for a refused exec upgrade, or "". +func (e *WSHandshakeError) execHint() string { + container := sanitizeServerMessage(e.Container) + switch e.Code { + case StreamCodeContainerNotAllowed: + if container != "" { + return fmt.Sprintf(". Container %q does not allow exec; choose another container, or ask an administrator.", container) + } + return ". No container of the chart allows exec; ask an administrator." + case StreamCodeServiceAccountToken: + return ". The container holds a service-account token, so the server refuses exec into it; ask an administrator." + case StreamCodeTokenCheckForbidden: + return ". The server may not check the pod for a service-account token; ask an administrator." + case StreamCodeClusterExecDisabled: + return ". The cluster of the stack does not allow exec; ask an administrator." + case StreamCodeInvalidChart: + // The server checks the name (DNS-1123); stackctl does not repeat it. + return ". Check the chart name (the Helm release name): the CHART column of 'stackctl stack pods'." + case StreamCodeInvalidContainer: + return ". Check the container name." + case StreamCodeChartNotFound: + return ". Check the chart name: the CHART column of 'stackctl stack pods'." + case StreamCodeContainerNotFound: + if container != "" { + return fmt.Sprintf(". No ready pod of the chart runs container %q; check the container name.", container) + } + return ". No ready pod of the chart runs an allowed container." + case StreamCodeNoReadyPod: + return ". The chart has no running and ready pod; check 'stackctl stack pods', then try again." + case StreamCodeExecLimit: + return ". Too many of your exec sessions are open; wait until one ends, then try again." + } + return "" +} + // Unwrap returns the dialer error (websocket.ErrBadHandshake). func (e *WSHandshakeError) Unwrap() error { return e.err @@ -772,6 +911,8 @@ const ( CloseMaxDuration = 4001 // server time limit CloseIdleTimeout = 4002 // no data in either direction ClosePodStreamError = 4502 // the reason holds the error + CloseBadStart = 4400 // exec: the start message is not valid + CloseStartTimeout = 4408 // exec: no start message in time closeNoStatus = websocket.CloseNoStatusReceived // 1005: close frame without a code ) @@ -838,6 +979,13 @@ func CloseCodeText(code int, reason string, apiKey bool) string { return "server time limit reached" case CloseIdleTimeout: return "idle timeout (no data in either direction)" + case CloseBadStart: + if reason == "" { + return "the server refused the command (too long or not valid)" + } + return "the server refused the command (too long or not valid): " + reason + case CloseStartTimeout: + return "the server received no command in time" case ClosePodStreamError: if reason == "" { return "pod stream error" diff --git a/cli/pkg/types/types.go b/cli/pkg/types/types.go index 23a0433..67c2f3f 100644 --- a/cli/pkg/types/types.go +++ b/cli/pkg/types/types.go @@ -359,6 +359,8 @@ type Cluster struct { MaxNamespaces int `json:"max_namespaces,omitempty" yaml:"max_namespaces,omitempty"` MaxInstancesPerUser int `json:"max_instances_per_user,omitempty" yaml:"max_instances_per_user,omitempty"` UseInCluster bool `json:"use_in_cluster,omitempty" yaml:"use_in_cluster,omitempty"` + AllowPortForward bool `json:"allow_port_forward,omitempty" yaml:"allow_port_forward,omitempty"` + AllowExec bool `json:"allow_exec,omitempty" yaml:"allow_exec,omitempty"` RegistryURL string `json:"registry_url,omitempty" yaml:"registry_url,omitempty"` RegistryUsername string `json:"registry_username,omitempty" yaml:"registry_username,omitempty"` ImagePullSecretName string `json:"image_pull_secret_name,omitempty" yaml:"image_pull_secret_name,omitempty"` @@ -376,6 +378,8 @@ type CreateClusterRequest struct { MaxInstancesPerUser int `json:"max_instances_per_user,omitempty" yaml:"max_instances_per_user,omitempty"` IsDefault bool `json:"is_default,omitempty" yaml:"is_default,omitempty"` UseInCluster bool `json:"use_in_cluster,omitempty" yaml:"use_in_cluster,omitempty"` + AllowPortForward bool `json:"allow_port_forward,omitempty" yaml:"allow_port_forward,omitempty"` + AllowExec bool `json:"allow_exec,omitempty" yaml:"allow_exec,omitempty"` RegistryURL string `json:"registry_url,omitempty" yaml:"registry_url,omitempty"` RegistryUsername string `json:"registry_username,omitempty" yaml:"registry_username,omitempty"` RegistryPassword string `json:"registry_password,omitempty" yaml:"registry_password,omitempty"` @@ -395,6 +399,8 @@ type UpdateClusterRequest struct { MaxInstancesPerUser *int `json:"max_instances_per_user,omitempty" yaml:"max_instances_per_user,omitempty"` IsDefault *bool `json:"is_default,omitempty" yaml:"is_default,omitempty"` UseInCluster *bool `json:"use_in_cluster,omitempty" yaml:"use_in_cluster,omitempty"` + AllowPortForward *bool `json:"allow_port_forward,omitempty" yaml:"allow_port_forward,omitempty"` + AllowExec *bool `json:"allow_exec,omitempty" yaml:"allow_exec,omitempty"` RegistryURL *string `json:"registry_url,omitempty" yaml:"registry_url,omitempty"` RegistryUsername *string `json:"registry_username,omitempty" yaml:"registry_username,omitempty"` RegistryPassword *string `json:"registry_password,omitempty" yaml:"registry_password,omitempty"` @@ -1551,6 +1557,26 @@ type WSDeploymentStatus struct { ErrorMessage string `json:"error_message,omitempty"` } +// ExecStartMessage is the first client message of an exec WebSocket +// (GET /api/v1/stack-instances/:id/exec): {"type":"start","command":[...]}. +// The type field leaves room for later client messages (stdin, resize). +type ExecStartMessage struct { + Type string `json:"type"` + Command []string `json:"command"` +} + +// ExecServerMessage is a text frame of the server on an exec WebSocket: +// "started" (Pod, Container), "exit" (ExitCode) or "error" (Message). +// ExitCode is a pointer so that an exit frame without exit_code is +// detectable. +type ExecServerMessage struct { + Type string `json:"type"` + Pod string `json:"pod,omitempty"` + Container string `json:"container,omitempty"` + ExitCode *int `json:"exit_code,omitempty"` + Message string `json:"message,omitempty"` +} + // StreamResult is the outcome of a WebSocket log-streaming session. type StreamResult struct { Status string diff --git a/cli/test/contract/path_contract_test.go b/cli/test/contract/path_contract_test.go index 63380f2..2b9448b 100644 --- a/cli/test/contract/path_contract_test.go +++ b/cli/test/contract/path_contract_test.go @@ -2,6 +2,7 @@ package contract import ( "encoding/json" + "fmt" "os" "path/filepath" "regexp" @@ -32,6 +33,28 @@ var pathPrefixesNotInSwagger = map[string]struct{}{ // builder; every other literal must match a swagger path exactly. var twoStepPaths = map[string]string{} +// pendingRoute is a route that exists only in an open k8s-stack-manager PR. +type pendingRoute struct { + // issue names the PR, for example "k8s-stack-manager#725". + issue string + // swagger is the swagger path that the PR adds, for example + // "/api/v1/stack-instances/{id}/exec". The literal must match it. + swagger string +} + +// pendingRoutes are literals of routes that exist only in an open +// k8s-stack-manager PR: the vendored swagger.json comes from the backend +// main branch, which does not have them yet. Each entry names the PR and the +// swagger path that the PR adds; the literal must match that path (a %s, %d +// or %v verb for each {param}). As soon as the vendored swagger has the +// path, the test fails until the entry is removed: then the normal check +// runs, so a wrong literal cannot stay hidden after a refresh. Do not add +// the route to the vendored file by hand: it must stay a copy of a backend +// revision. +// The map is empty when no route is pending; the checks below then do +// nothing. +var pendingRoutes = map[string]pendingRoute{} + // TestClientPaths_ExistInBackend checks that every /api/v1 path in the // non-test source of pkg/client and cmd exists in the vendored swagger. // A wrong path gives a 404 only against a real server (stackctl#141: @@ -58,7 +81,15 @@ func TestClientPaths_ExistInBackend(t *testing.T) { literals := collectPathLiterals(t, "../../pkg/client", "../../cmd") require.NotEmpty(t, literals, "no /api/v1 literals found; did the source layout change?") + used := map[string]bool{} for _, lit := range literals { + if pr, ok := pendingRoutes[lit]; ok { + used[lit] = true + for _, p := range pendingRouteProblems(lit, pr, raw.Paths, swaggerPaths) { + t.Error(p) + } + continue + } if _, ok := pathPrefixesNotInSwagger[lit]; ok { assert.Truef(t, isSwaggerPrefix(lit, raw.Paths), "prefix %q is not the prefix of a swagger path", lit) continue @@ -71,6 +102,69 @@ func TestClientPaths_ExistInBackend(t *testing.T) { assert.Truef(t, matchesSwaggerPath(lit, swaggerPaths), "path %q is not a route in the vendored swagger.json (wrong path, or refresh swagger.json)", lit) } + for lit, pr := range pendingRoutes { + assert.Truef(t, used[lit], "pending route %q (%s) is not used by pkg/client or cmd; remove it from pendingRoutes", lit, pr.issue) + } +} + +// pendingRouteProblems checks a pendingRoutes entry and returns what is +// wrong: the literal does not match the swagger path of the entry, or the +// vendored swagger has that path or another path that the literal matches. +func pendingRouteProblems(lit string, pr pendingRoute, paths map[string]json.RawMessage, swaggerPaths [][]string) []string { + var problems []string + if !strings.HasPrefix(pr.swagger, "/api/v1/") { + problems = append(problems, fmt.Sprintf("pending route %q: swagger path %q must start with /api/v1/", lit, pr.swagger)) + } + if !matchesSwaggerPath(lit, [][]string{strings.Split(strings.Trim(pr.swagger, "/"), "/")}) { + problems = append(problems, fmt.Sprintf("pending route %q (%s) does not match its swagger path %q", lit, pr.issue, pr.swagger)) + } + if _, exists := paths[pr.swagger]; exists { + problems = append(problems, fmt.Sprintf("pending route %q (%s): %q is in the vendored swagger.json now; remove the entry from pendingRoutes", lit, pr.issue, pr.swagger)) + } else if matchesSwaggerPath(lit, swaggerPaths) { + problems = append(problems, fmt.Sprintf("pending route %q (%s) matches a path of the vendored swagger.json now; remove the entry from pendingRoutes", lit, pr.issue)) + } + return problems +} + +// TestPendingRouteProblems covers the checks of a pendingRoutes entry. +func TestPendingRouteProblems(t *testing.T) { + t.Parallel() + exec := pendingRoute{issue: "k8s-stack-manager#725", swagger: "/api/v1/stack-instances/{id}/exec"} + without := map[string]json.RawMessage{"/api/v1/stack-instances/{id}": nil} + with := map[string]json.RawMessage{"/api/v1/stack-instances/{id}": nil, "/api/v1/stack-instances/{id}/exec": nil} + split := func(paths map[string]json.RawMessage) [][]string { + var out [][]string + for p := range paths { + out = append(out, strings.Split(strings.Trim(p, "/"), "/")) + } + return out + } + tests := []struct { + name string + lit string + pr pendingRoute + paths map[string]json.RawMessage + want []string + }{ + {"pending and matching", "/api/v1/stack-instances/%s/exec", exec, without, nil}, + {"literal does not match the template", "/api/v1/stack-instance/%s/exec", exec, without, []string{"does not match its swagger path"}}, + {"fixed segment in a param place", "/api/v1/stack-instances/42/exec", exec, without, []string{"does not match its swagger path"}}, + {"template in the swagger now", "/api/v1/stack-instances/%s/exec", exec, with, []string{"is in the vendored swagger.json now"}}, + {"wrong literal after the refresh", "/api/v1/stack-instance/%s/exec", exec, with, + []string{"does not match its swagger path", "is in the vendored swagger.json now"}}, + {"template outside /api/v1", "/api/v1/stack-instances/%s/exec", pendingRoute{issue: "x", swagger: "/stack-instances/{id}/exec"}, without, + []string{"must start with /api/v1/", "does not match its swagger path"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := pendingRouteProblems(tt.lit, tt.pr, tt.paths, split(tt.paths)) + require.Len(t, got, len(tt.want), "problems: %v", got) + for i, w := range tt.want { + assert.Contains(t, got[i], w) + } + }) + } } // collectPathLiterals returns the sorted unique /api/v1 literals of the diff --git a/cli/test/contract/testdata/swagger.json b/cli/test/contract/testdata/swagger.json index e939c72..ef8d42b 100644 --- a/cli/test/contract/testdata/swagger.json +++ b/cli/test/contract/testdata/swagger.json @@ -6973,6 +6973,128 @@ } } }, + "/api/v1/stack-instances/{id}/exec": { + "get": { + "security": [ + { + "BearerAuth": [] + }, + { + "ApiKeyAuth": [] + } + ], + "description": "Upgrades the connection to a WebSocket that runs one command in a container of the chart (Helm release) of the instance and streams its output. The command runs without stdin and without a TTY. Authentication: the Authorization header (JWT) or, when STACK_STREAM_ALLOW_API_KEYS is true, the X-API-Key header; the route never accepts a token in the query string. The caller must be the owner of the instance, a devops user or an admin, exec must be on (STACK_EXEC_ENABLED) and the cluster must allow it (allow_exec). Only containers in STACK_EXEC_ALLOWED_CONTAINERS can be used, and a container that holds a service-account token is refused unless STACK_EXEC_ALLOW_SERVICE_ACCOUNT_TOKEN is true. The pod is always chosen by the server: the first Running and Ready pod of the release, by name. All checks run before the upgrade and give an HTTP status with a stable code in the body ({\"error\",\"code\"}). Protocol after the upgrade: the client sends one text frame within 10 s, {\"type\":\"start\",\"command\":[\"bin/console\",\"cache:clear\"]} (1 to 256 arguments, the first not empty, no NUL byte, at most STACK_EXEC_MAX_COMMAND_LENGTH bytes in total; the server never uses a shell). The server answers with the text frame {\"type\":\"started\",\"pod\":\"...\",\"container\":\"...\"}, then sends the output as binary frames: the first byte is the stream (1 stdout, 2 stderr), the rest is raw output, at most 32 KiB per frame. The command ends with the text frame {\"type\":\"exit\",\"exit_code\":N} and the close code 1000. A Kubernetes stream error ends with {\"type\":\"error\",\"message\":\"...\"} and 4502; an internal error with {\"type\":\"error\",\"message\":\"Internal server error\"} and 1011. Later client frames are ignored. The server pings every 30 s. Close codes: 1000 the command ended, 1001 server shutdown, 1008 access revoked, 1009 client frame larger than the read limit (64 KiB, or 6 times STACK_EXEC_MAX_COMMAND_LENGTH plus 4 KiB when that is larger), 1011 internal error (also when the audit entry cannot be written; the command does not run then), 4001 maximum duration (no exit frame), 4400 invalid start message, 4408 no start message in 10 s, 4502 Kubernetes stream error. The remote process can keep running after the stream closes. Exec shows the container environment and its secrets: turn it on only where the stack owners may see them. The start and the end of each command are audited (exec, exec_end).", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "stack-instances" + ], + "summary": "Run a command in a container of a stack instance", + "parameters": [ + { + "type": "string", + "description": "Instance ID", + "name": "id", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "Chart name (the Helm release name, DNS-1123 subdomain, at most 53 characters)", + "name": "chart", + "in": "query", + "required": true + }, + { + "type": "string", + "description": "Container name (DNS-1123 label). Omitted: the first allowed container of the first pod that has one", + "name": "container", + "in": "query" + } + ], + "responses": { + "101": { + "description": "Switching Protocols" + }, + "400": { + "description": "invalid_chart, invalid_container, upgrade_required", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "401": { + "description": "Unauthorized", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "403": { + "description": "api_key_not_allowed, forbidden, cluster_exec_disabled, namespace_not_allowed, container_not_allowed, service_account_token, token_check_forbidden, origin_not_allowed", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "404": { + "description": "feature_disabled, instance_not_found, chart_not_found, container_not_found", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "409": { + "description": "instance_not_deployed", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "429": { + "description": "exec_limit", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "500": { + "description": "Internal Server Error", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + }, + "503": { + "description": "no_ready_pod, kube_timeout, shutting_down", + "schema": { + "type": "object", + "additionalProperties": { + "type": "string" + } + } + } + } + } + }, "/api/v1/stack-instances/{id}/extend": { "post": { "description": "Extend the expiry time of a stack instance. The extend never makes the expiry earlier and never changes ttl_minutes.\n- {\"minutes\": N}: adds N minutes (N \u003e 0) to the current expires_at, or to now when the instance already expired or has no expiry time. The new expiry is capped at now + MAX_INSTANCE_TTL_DAYS (default 30 days).\n- {\"until\": \"\u003cRFC 3339\u003e\"}: sets expires_at to this time. It must be after the current expiry (or after now when the expiry has passed) and not later than now + MAX_INSTANCE_TTL_DAYS (400). It works for a TTL and for a fixed stop time. Do not send it with minutes or ttl_minutes (400).\n- Empty body: adds the instance's ttl_minutes in the same way (400 when ttl_minutes is 0).\n- Deprecated {\"ttl_minutes\": N} (without minutes): the old behaviour for older clients (stackctl 0.4.0 and earlier). It sets ttl_minutes to N and expires_at to now + N, which can make the expiry earlier. The response has a Warning header; the server logs a deprecation warning.\nAn instance without TTL and without expiry time has nothing to extend (400).", @@ -9823,7 +9945,7 @@ }, "/api/v1/ui-config": { "get": { - "description": "Returns the display values of the web UI: product title, logo URL and favicon URL (APP_TITLE, APP_LOGO_URL, APP_FAVICON_URL), the start value of the TTL field in the create forms (default_instance_ttl_minutes, DEFAULT_INSTANCE_TTL_MINUTES; 0 means no expiry), the maximum TTL and stop-time distance in days (max_instance_ttl_days, MAX_INSTANCE_TTL_DAYS), and whether port-forward is on (port_forward_enabled, STACK_PORT_FORWARD_ENABLED). Public: no authentication, so the login page can use it. An empty logo_url means the built-in logo. A URL is a same-origin path or an https URL. Cached for 5 minutes.", + "description": "Returns the display values of the web UI: product title, logo URL and favicon URL (APP_TITLE, APP_LOGO_URL, APP_FAVICON_URL), the start value of the TTL field in the create forms (default_instance_ttl_minutes, DEFAULT_INSTANCE_TTL_MINUTES; 0 means no expiry), the maximum TTL and stop-time distance in days (max_instance_ttl_days, MAX_INSTANCE_TTL_DAYS), whether port-forward is on (port_forward_enabled, STACK_PORT_FORWARD_ENABLED), and whether exec is on (exec_enabled, STACK_EXEC_ENABLED). Public: no authentication, so the login page can use it. An empty logo_url means the built-in logo. A URL is a same-origin path or an https URL. Cached for 5 minutes.", "produces": [ "application/json" ], @@ -11195,6 +11317,9 @@ "name" ], "properties": { + "allow_exec": { + "type": "boolean" + }, "allow_port_forward": { "type": "boolean" }, @@ -11920,6 +12045,11 @@ "type": "integer", "example": 480 }, + "exec_enabled": { + "description": "ExecEnabled is true when exec is on (STACK_EXEC_ENABLED). The UI shows\nthe stackctl hint then.", + "type": "boolean", + "example": false + }, "favicon_url": { "description": "FaviconURL is the browser tab icon (APP_FAVICON_URL).", "type": "string", @@ -11950,6 +12080,9 @@ "handlers.UpdateClusterRequest": { "type": "object", "properties": { + "allow_exec": { + "type": "boolean" + }, "allow_port_forward": { "type": "boolean" }, @@ -13813,6 +13946,10 @@ "models.Cluster": { "type": "object", "properties": { + "allow_exec": { + "description": "AllowExec lets users run commands in the containers of the stacks of\nthis cluster. Off by default; only an admin sets it. Exec shows the\ncontainer environment and its secrets.", + "type": "boolean" + }, "allow_port_forward": { "description": "AllowPortForward lets users open port-forward tunnels to the stacks of\nthis cluster. Off by default; only an admin sets it.", "type": "boolean" diff --git a/cli/test/e2e/cli_e2e_test.go b/cli/test/e2e/cli_e2e_test.go index be1a634..30da744 100644 --- a/cli/test/e2e/cli_e2e_test.go +++ b/cli/test/e2e/cli_e2e_test.go @@ -2996,3 +2996,123 @@ func TestE2E_PluginGlobalFlags(t *testing.T) { assert.Contains(t, out, "api_url=http://plugin.example\n") assert.Contains(t, out, "config_dir="+dir+"\n") } + +// startE2EExecMockServer serves the stack list (the name my-stack is +// instance 42) and the exec WebSocket of instance 42. refuse, when set, +// answers the exec route without an upgrade. Otherwise the server sends +// started, one stdout and one stderr frame and exit code 7. +func startE2EExecMockServer(t *testing.T, refuse func(w http.ResponseWriter)) (*httptest.Server, func() []string) { + t.Helper() + var mu sync.Mutex + var command []string + upgrader := websocket.Upgrader{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/stack-instances": + data := []map[string]interface{}{} + if r.URL.Query().Get("name") == "my-stack" { + data = append(data, map[string]interface{}{"id": "42", "name": "my-stack", "status": "running"}) + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{"data": data, "total": len(data), "page": 1, "pageSize": 25}) + return + case "/api/v1/stack-instances/42/exec": + default: + http.NotFound(w, r) + return + } + if refuse != nil { + refuse(w) + return + } + ws, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return + } + defer ws.Close() + _, raw, err := ws.ReadMessage() + if err != nil { + return + } + var start types.ExecStartMessage + if json.Unmarshal(raw, &start) != nil { + return + } + mu.Lock() + command = start.Command + mu.Unlock() + send := func(v interface{}) { + b, _ := json.Marshal(v) + _ = ws.WriteMessage(websocket.TextMessage, b) + } + send(map[string]interface{}{"type": "started", "pod": "backend-0", "container": "php"}) + _ = ws.WriteMessage(websocket.BinaryMessage, append([]byte{1}, "remote stdout\n"...)) + _ = ws.WriteMessage(websocket.BinaryMessage, append([]byte{2}, "remote stderr\n"...)) + send(map[string]interface{}{"type": "exit", "exit_code": 7}) + _ = ws.WriteMessage(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseNormalClosure, "")) + for { + if _, _, err := ws.ReadMessage(); err != nil { + return + } + } + })) + t.Cleanup(srv.Close) + return srv, func() []string { + mu.Lock() + defer mu.Unlock() + return command + } +} + +// TestE2E_StackExecExitCodes runs the binary: main.go exits with the code +// of an ExitError (stackctl#179). +func TestE2E_StackExecExitCodes(t *testing.T) { + if testing.Short() { + t.Skip("skipping e2e test in short mode") + } + + t.Run("remote exit code 7", func(t *testing.T) { + server, command := startE2EExecMockServer(t, nil) + dir := t.TempDir() + setupE2EStackContext(t, dir, server.URL) + + stdout, stderr, err := runStackctl(t, dir, "stack", "exec", "my-stack", "backend", "--", "bin/console", "-v", "--help") + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr, "stderr=%q", stderr) + assert.Equal(t, 7, exitErr.ExitCode()) + assert.Equal(t, "remote stdout\n", stdout) + assert.Equal(t, "remote stderr\n", stderr, "no message of stackctl for a remote exit code") + assert.Equal(t, []string{"bin/console", "-v", "--help"}, command()) + }) + + t.Run("403 exits 255 with the message", func(t *testing.T) { + server, _ := startE2EExecMockServer(t, func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(map[string]string{"error": "Container does not allow exec", "code": "container_not_allowed"}) + }) + dir := t.TempDir() + setupE2EStackContext(t, dir, server.URL) + + stdout, stderr, err := runStackctl(t, dir, "stack", "exec", "my-stack", "backend", "-c", "php", "--", "ls") + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + assert.Equal(t, 255, exitErr.ExitCode()) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "exec refused by the server (HTTP 403): Container does not allow exec") + assert.Contains(t, stderr, `Container "php" does not allow exec`) + }) + + t.Run("usage error exits 1", func(t *testing.T) { + server, _ := startE2EExecMockServer(t, nil) + dir := t.TempDir() + setupE2EStackContext(t, dir, server.URL) + + stdout, stderr, err := runStackctl(t, dir, "stack", "exec", "my-stack", "backend", "ls") + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + assert.Equal(t, 1, exitErr.ExitCode()) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "missing '--' before the command") + }) +} From 7f2dfc18bc95307224055dd85f0e7c23114e721a Mon Sep 17 00:00:00 2001 From: omattsson Date: Fri, 30 Oct 2026 21:57:19 +0100 Subject: [PATCH 2/2] feat(cluster): add --api-server-url and --use-in-cluster to cluster create/update The server needs api_server_url for a cluster with a kubeconfig, and use_in_cluster for the cluster it runs in, but the flags of cluster create could send neither: only --from-file could register a cluster. - create and update: --api-server-url and --use-in-cluster. - create refuses a kubeconfig flag without --api-server-url, and --use-in-cluster with a kubeconfig flag, before any request. - update sends --use-in-cluster=false (the server removes the stored kubeconfig on true). Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- README.md | 5 ++ cli/cmd/cluster.go | 43 +++++++++++++-- cli/cmd/cluster_test.go | 114 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 976c8e5..7bd5d25 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,7 +43,7 @@ cli/ # Go module root (github.com/omattsson/stackctl/cl override.go # override list/get/set/unset/delete, branch overrides (list/get/set/delete), quota overrides (quota set merges with GET, --replace) bulk.go # bulk deploy/stop/clean/delete, bulk template publish/unpublish/delete (--ids flag or positional args) git.go # git providers/branches/validate - cluster.go # cluster list/get/create/update/delete/set-default/test-connection/health/nodes/namespaces/utilization (create/update --allow-port-forward/--allow-exec: per-cluster stream switches; update sends false), quota get/set/delete, shared-values list/set/update/delete (update: GET list, merge, PUT the full record) + cluster.go # cluster list/get/create/update/delete/set-default/test-connection/health/nodes/namespaces/utilization (create/update --allow-port-forward/--allow-exec: per-cluster stream switches; update sends false. --api-server-url is required with a kubeconfig flag; --use-in-cluster excludes the kubeconfig flags), quota get/set/delete, shared-values list/set/update/delete (update: GET list, merge, PUT the full record) analytics.go # analytics overview/templates/users apikey.go # apikey list/create/revoke audit.go # audit log list/export diff --git a/README.md b/README.md index 52d9f87..8fa6744 100644 --- a/README.md +++ b/README.md @@ -646,6 +646,11 @@ stackctl stack list --cluster 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e -q | xargs -n stackctl cluster list stackctl cluster get 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e +# Register a cluster: with a kubeconfig and the API server URL as the server reaches it, +# or the cluster that the server runs in (its service account, no kubeconfig) +stackctl cluster create --name prod --api-server-url https://prod.example.com:6443 --kubeconfig-path ~/.kube/prod.yaml +stackctl cluster create --name local --use-in-cluster + # Per-cluster switches of stack port-forward and stack exec (admin; off by default) stackctl cluster update 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e --allow-exec --allow-port-forward stackctl cluster update 5b6c7d8e-9f0a-4b1c-8d2e-3f4a5b6c7d8e --allow-exec=false diff --git a/cli/cmd/cluster.go b/cli/cmd/cluster.go index 71e03b2..9194cbd 100644 --- a/cli/cmd/cluster.go +++ b/cli/cmd/cluster.go @@ -587,7 +587,12 @@ var clusterCreateCmd = &cobra.Command{ Short: "Register a new cluster", Long: `Register a new Kubernetes cluster. -Provide either --from-file with a JSON/YAML file, or use --name and --kubeconfig-data / --kubeconfig-path. +Provide either --from-file with a JSON/YAML file, or use --name with one of: + - --api-server-url and --kubeconfig-data / --kubeconfig-path: a cluster that + the server reaches with that kubeconfig. --api-server-url is the address of + the Kubernetes API server as the server reaches it. + - --use-in-cluster: the cluster that the server runs in (its service + account); no kubeconfig and no API server URL. --allow-port-forward and --allow-exec turn on the per-cluster switches of 'stackctl stack port-forward' and 'stackctl stack exec' (off by default; the @@ -596,8 +601,8 @@ ignore them: check the Port-forward and Exec rows of the output. Examples: stackctl cluster create --from-file cluster.json - stackctl cluster create --name prod --kubeconfig-path ~/.kube/config - stackctl cluster create --name dev --kubeconfig-path ~/.kube/config --allow-exec`, + stackctl cluster create --name prod --api-server-url https://prod.example.com:6443 --kubeconfig-path ~/.kube/prod.yaml + stackctl cluster create --name local --use-in-cluster --allow-exec`, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { fromFile, _ := cmd.Flags().GetString("from-file") @@ -655,8 +660,19 @@ Examples: if kubeconfigData != "" && kubeconfigPath != "" { return fmt.Errorf("--kubeconfig-data and --kubeconfig-path are mutually exclusive") } + apiServerURL, _ := cmd.Flags().GetString("api-server-url") + useInCluster, _ := cmd.Flags().GetBool("use-in-cluster") + hasKubeconfig := kubeconfigData != "" || kubeconfigPath != "" + if useInCluster && hasKubeconfig { + return fmt.Errorf("--use-in-cluster cannot be combined with --kubeconfig-data or --kubeconfig-path") + } + if hasKubeconfig && apiServerURL == "" { + return fmt.Errorf("--api-server-url is required with --kubeconfig-data or --kubeconfig-path (or use --use-in-cluster)") + } req.Name = name req.Description, _ = cmd.Flags().GetString("description") + req.APIServerURL = apiServerURL + req.UseInCluster = useInCluster req.AllowPortForward, _ = cmd.Flags().GetBool("allow-port-forward") req.AllowExec, _ = cmd.Flags().GetBool("allow-exec") req.KubeconfigData = kubeconfigData @@ -702,9 +718,11 @@ var clusterUpdateCmd = &cobra.Command{ defaultChanged := cmd.Flags().Changed("default") portForwardChanged := cmd.Flags().Changed("allow-port-forward") execChanged := cmd.Flags().Changed("allow-exec") + apiServerURLChanged := cmd.Flags().Changed("api-server-url") + inClusterChanged := cmd.Flags().Changed("use-in-cluster") - if fromFile == "" && !nameChanged && !descChanged && !kubeconfigDataChanged && !kubeconfigPathChanged && !defaultChanged && !portForwardChanged && !execChanged { - return fmt.Errorf("at least one of --name, --description, --kubeconfig-data, --kubeconfig-path, --default, --allow-port-forward, --allow-exec, or --from-file must be specified") + if fromFile == "" && !nameChanged && !descChanged && !kubeconfigDataChanged && !kubeconfigPathChanged && !defaultChanged && !portForwardChanged && !execChanged && !apiServerURLChanged && !inClusterChanged { + return fmt.Errorf("at least one of --name, --description, --api-server-url, --kubeconfig-data, --kubeconfig-path, --use-in-cluster, --default, --allow-port-forward, --allow-exec, or --from-file must be specified") } var req types.UpdateClusterRequest @@ -760,6 +778,17 @@ var clusterUpdateCmd = &cobra.Command{ if kubeconfigDataChanged && kubeconfigPathChanged { return fmt.Errorf("--kubeconfig-data and --kubeconfig-path are mutually exclusive") } + if inClusterChanged { + v, _ := cmd.Flags().GetBool("use-in-cluster") + if v && (kubeconfigDataChanged || kubeconfigPathChanged) { + return fmt.Errorf("--use-in-cluster cannot be combined with --kubeconfig-data or --kubeconfig-path") + } + req.UseInCluster = &v + } + if apiServerURLChanged { + v, _ := cmd.Flags().GetString("api-server-url") + req.APIServerURL = &v + } if nameChanged { v, _ := cmd.Flags().GetString("name") req.Name = &v @@ -1344,6 +1373,8 @@ func init() { clusterCreateCmd.Flags().String("description", "", "Cluster description") clusterCreateCmd.Flags().String("kubeconfig-data", "", "Raw kubeconfig data (base64 or YAML string)") clusterCreateCmd.Flags().String("kubeconfig-path", "", "Path to kubeconfig file") + clusterCreateCmd.Flags().String("api-server-url", "", "Kubernetes API server URL as the server reaches it (required with --kubeconfig-data or --kubeconfig-path)") + clusterCreateCmd.Flags().Bool("use-in-cluster", false, "Use the cluster that the server runs in (its service account; no kubeconfig)") clusterCreateCmd.Flags().Bool("allow-port-forward", false, "Allow 'stackctl stack port-forward' to stacks on this cluster (k8s-stack-manager v0.15.0 or later)") clusterCreateCmd.Flags().Bool("allow-exec", false, "Allow 'stackctl stack exec' in stacks on this cluster (k8s-stack-manager v0.15.0 or later)") @@ -1354,6 +1385,8 @@ func init() { clusterUpdateCmd.Flags().String("kubeconfig-data", "", "New kubeconfig data") clusterUpdateCmd.Flags().String("kubeconfig-path", "", "New path to kubeconfig file") clusterUpdateCmd.Flags().Bool("default", false, "Mark cluster as default") + clusterUpdateCmd.Flags().String("api-server-url", "", "New Kubernetes API server URL") + clusterUpdateCmd.Flags().Bool("use-in-cluster", false, "Use the cluster that the server runs in (the server removes the stored kubeconfig; --use-in-cluster=false needs a kubeconfig)") clusterUpdateCmd.Flags().Bool("allow-port-forward", false, "Allow 'stackctl stack port-forward' to stacks on this cluster (--allow-port-forward=false turns it off)") clusterUpdateCmd.Flags().Bool("allow-exec", false, "Allow 'stackctl stack exec' in stacks on this cluster (--allow-exec=false turns it off)") diff --git a/cli/cmd/cluster_test.go b/cli/cmd/cluster_test.go index 681c746..d949dd4 100644 --- a/cli/cmd/cluster_test.go +++ b/cli/cmd/cluster_test.go @@ -838,6 +838,8 @@ func resetClusterCreateFlags(t *testing.T) { clusterCreateCmd.Flags().Set("kubeconfig-path", "") clusterCreateCmd.Flags().Set("allow-port-forward", "false") clusterCreateCmd.Flags().Set("allow-exec", "false") + clusterCreateCmd.Flags().Set("api-server-url", "") + clusterCreateCmd.Flags().Set("use-in-cluster", "false") } func TestClusterCreateCmd_TableOutput(t *testing.T) { @@ -1007,6 +1009,10 @@ func resetClusterUpdateFlags(t *testing.T) { clusterUpdateCmd.Flags().Set("allow-exec", "false") clusterUpdateCmd.Flags().Lookup("allow-port-forward").Changed = false clusterUpdateCmd.Flags().Lookup("allow-exec").Changed = false + clusterUpdateCmd.Flags().Set("api-server-url", "") + clusterUpdateCmd.Flags().Set("use-in-cluster", "false") + clusterUpdateCmd.Flags().Lookup("api-server-url").Changed = false + clusterUpdateCmd.Flags().Lookup("use-in-cluster").Changed = false } func TestClusterUpdateCmd_TableOutput(t *testing.T) { @@ -1189,6 +1195,114 @@ func TestClusterGetCmd_StreamSwitches(t *testing.T) { assert.NotContains(t, buf.String(), `"allow_exec"`, "a switch that is off is left out, like the other bool fields") } +func TestClusterCreateCmd_ConnectionFlags(t *testing.T) { + kubeconfig := filepath.Join(t.TempDir(), "kubeconfig") + require.NoError(t, os.WriteFile(kubeconfig, []byte("apiVersion: v1\nkind: Config\n"), 0o600)) + + tests := []struct { + name string + flags map[string]string + want map[string]interface{} + wantErr string + }{ + { + name: "kubeconfig with api server url", + flags: map[string]string{"api-server-url": "https://k8s.example.com:6443", "kubeconfig-path": kubeconfig}, + want: map[string]interface{}{"name": "dev", "api_server_url": "https://k8s.example.com:6443", "kubeconfig_data": "apiVersion: v1\nkind: Config\n"}, + }, + { + name: "in cluster", + flags: map[string]string{"use-in-cluster": "true"}, + want: map[string]interface{}{"name": "dev", "use_in_cluster": true}, + }, + { + name: "kubeconfig without api server url", + flags: map[string]string{"kubeconfig-path": kubeconfig}, + wantErr: "--api-server-url is required with --kubeconfig-data or --kubeconfig-path", + }, + { + name: "in cluster with kubeconfig", + flags: map[string]string{"use-in-cluster": "true", "kubeconfig-data": "x", "api-server-url": "https://k8s.example.com"}, + wantErr: "--use-in-cluster cannot be combined", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var body map[string]interface{} + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(sampleCluster()) + })) + defer server.Close() + + _ = setupClusterTestCmd(t, server.URL) + require.NoError(t, clusterCreateCmd.Flags().Set("name", "dev")) + for k, v := range tt.flags { + require.NoError(t, clusterCreateCmd.Flags().Set(k, v)) + } + t.Cleanup(func() { resetClusterCreateFlags(t) }) + + err := clusterCreateCmd.RunE(clusterCreateCmd, nil) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Zero(t, requests, "refused before any request") + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, body) + }) + } +} + +func TestClusterUpdateCmd_ConnectionFlags(t *testing.T) { + tests := []struct { + name string + flags map[string]string + want map[string]interface{} + wantErr string + }{ + {name: "api server url", flags: map[string]string{"api-server-url": "https://new.example.com:6443"}, want: map[string]interface{}{"api_server_url": "https://new.example.com:6443"}}, + {name: "in cluster", flags: map[string]string{"use-in-cluster": "true"}, want: map[string]interface{}{"use_in_cluster": true}}, + {name: "in cluster off is sent", flags: map[string]string{"use-in-cluster": "false", "kubeconfig-data": "x"}, want: map[string]interface{}{"use_in_cluster": false, "kubeconfig_data": "x"}}, + {name: "in cluster with kubeconfig", flags: map[string]string{"use-in-cluster": "true", "kubeconfig-data": "x"}, wantErr: "--use-in-cluster cannot be combined"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var body map[string]interface{} + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(sampleCluster()) + })) + defer server.Close() + + _ = setupClusterTestCmd(t, server.URL) + for k, v := range tt.flags { + require.NoError(t, clusterUpdateCmd.Flags().Set(k, v)) + } + t.Cleanup(func() { resetClusterUpdateFlags(t) }) + + err := clusterUpdateCmd.RunE(clusterUpdateCmd, []string{"1"}) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Zero(t, requests, "refused before any request") + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, body) + }) + } +} + func TestClusterUpdateCmd_PathTraversal(t *testing.T) { _ = setupClusterTestCmd(t, "http://unused") clusterUpdateCmd.Flags().Set("from-file", "../../etc/passwd")