# Deep Review: 20260330-155444-pr-198 | | | |---|---| | **Date** | 2026-03-30 15:54 | | **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) | | **Round** | 2 | | **Author** | [@mook-as](https://github.com/mook-as) | | **PR** | [#198](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/198) — mock controller: Add a mock logs endpoint | | **Branch** | `mook-as/container-api/mock-logs` | | **Commits** | `1b7060e` mock controller: Add a mock logs endpoint | | **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro | | **Verdict** | **Merge with fixes** — WebSocket upgrader rejects browser clients; error responses mask backend failures | | **Wall-clock time** | `13 min 29 s` | --- ## Executive Summary This PR adds a mock WebSocket logs endpoint (`/passthrough/mock/logs/{container}`) that streams container labels as log lines, intended for UI development. It wires the endpoint into the mock controller's passthrough discovery, extracts a reusable `curl_has_websocket_support` helper, and adds BATS tests covering valid containers, invalid IDs, and nonexistent containers. The main concern: the WebSocket upgrader's default origin check rejects browser connections — the primary use case this endpoint serves. --- ## Critical Issues None. --- ## Important Issues 1. **Browser clients rejected by default WebSocket origin check** — `pkg/controllers/mock/logs_passthrough.go:50-51` (important, regression) [Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro] ```go upgrader := websocket.Upgrader{} conn, err := upgrader.Upgrade(w, r, nil) ``` The zero-value `Upgrader` uses gorilla/websocket's default `checkSameOrigin`, which rejects requests whose `Origin` header does not match `Host`. Browser WebSocket requests always send `Origin`. When the UI runs on a separate dev server (e.g., Vite on port 5173) or in Electron with a custom origin, the origins diverge and the upgrade fails with 403. The existing demo controller at `pkg/controllers/app/demo/controller.go:90-92` explicitly bypasses this check. The BATS tests pass because `curl` omits the `Origin` header, so `checkSameOrigin` returns true unconditionally. Confirmed via `git blame`: introduced in commit `1b7060e`. Fix: Match the demo controller's pattern. ```diff - upgrader := websocket.Upgrader{} + upgrader := websocket.Upgrader{ + CheckOrigin: func(_ *http.Request) bool { return true }, + } ``` --- ## Suggestions 1. **All API errors return 404** — `pkg/controllers/mock/logs_passthrough.go:44-47` (suggestion, regression) [Claude Opus 4.6, Gemini 3.1 Pro] ```go if err != nil { log.V(5).Info("Failed to get container", "error", err) http.Error(w, "Failed to get container", http.StatusNotFound) return } ``` Any error from `GetClient().Get()` — not-found, kine database issue, context cancellation — returns HTTP 404. During UI development, this masks real backend problems as "container not found." Distinguishing not-found from internal errors improves debuggability. Fix: Use `apierrors.IsNotFound` to differentiate (already imported in `mock_controller.go`). ```diff + if apierrors.IsNotFound(err) { + log.V(5).Info("Container not found", "container", containerID) + http.Error(w, "Container not found", http.StatusNotFound) + return + } if err != nil { - log.V(5).Info("Failed to get container", "error", err) - http.Error(w, "Failed to get container", http.StatusNotFound) + log.V(5).Info("Failed to get container", "container", containerID, "error", err) + http.Error(w, "Failed to get container", http.StatusInternalServerError) return } ``` 2. **Trailing path segments silently ignored** — `pkg/controllers/mock/logs_passthrough.go:30` (suggestion, gap) [Gemini 3.1 Pro] ```go containerID, _, _ := strings.Cut(strings.TrimLeft(r.URL.Path, "/"), "/") ``` `strings.Cut` extracts the first path segment as `containerID` and discards the rest. A request to `/passthrough/mock/logs/1234/extra/stuff` succeeds with container ID `1234`. While harmless for a mock endpoint, it can mask malformed frontend requests during development. Fix: Check the `after` return value and reject requests with extra segments. ```diff - containerID, _, _ := strings.Cut(strings.TrimLeft(r.URL.Path, "/"), "/") + containerID, after, _ := strings.Cut(strings.TrimLeft(r.URL.Path, "/"), "/") + if after != "" { + http.Error(w, "Invalid path", http.StatusBadRequest) + return + } ``` 3. **WebSocket close frame may not reach client** — `pkg/controllers/mock/logs_passthrough.go:56-63` (suggestion, gap) [Gemini 3.1 Pro] ```go defer conn.Close() defer func() { message := websocket.FormatCloseMessage(websocket.CloseNormalClosure, "Closing connection") err := conn.WriteControl(websocket.CloseMessage, message, time.Now().Add(time.Second)) // ... }() ``` The deferred `conn.Close()` runs immediately after the close-frame write without waiting for the client to echo the close frame. Browser clients may see `1006 Abnormal Closure` instead of the clean `1000 Normal Closure`. Acceptable for a mock endpoint, but a brief read loop after sending the close message would produce cleaner client-side behavior. --- ## Design Observations **Strengths** - Reuses the `PassthroughController` interface and discovery ConfigMap pattern from the demo controller. Endpoint registration is clean and extensible. [Claude Opus 4.6, Codex GPT 5.4] - The WebSocket close sequence (lines 57-63) correctly uses LIFO defer ordering: send close message first, then close the connection. [Claude Opus 4.6] - Extracting `curl_has_websocket_support()` into `commands.bash` eliminates duplication across both WebSocket passthrough tests. [Claude Opus 4.6] - Test structure covers three distinct cases: valid container, invalid ID format, and nonexistent container. [Claude Opus 4.6] --- ## Testing Assessment Coverage is adequate for a mock endpoint: 1. No test exercises the endpoint with a browser-style `Origin` header. The BATS tests use `curl`, which omits `Origin`, so they miss the failure described in Important Finding 1. 2. Container with nil labels (`Status.Labels` is nil) is untested, but safe — `maps.Keys(nil)` returns an empty iterator. 3. Empty container ID is implicitly covered by the hex-character regex requiring at least one character (`+`). --- ## Documentation Assessment No documentation changes needed. This is a mock-only endpoint for internal development. --- ## Agent Performance Retro ### [Claude] - Duration: 8 min 34 s - Found 2 issues: Important 1, Suggestion 1 - Unique contributions: detailed analysis of when the `Origin` check would and would not fail (passthrough preserving `Host` header), thorough testing assessment with edge case enumeration (nil labels, empty ID) - Accuracy: no false positives - Depth: traced into demo controller for pattern comparison, examined defer ordering - Coverage: reviewed all 5 files, all accounted for in coverage summary - Signal-to-noise: high — every finding is actionable ### [Codex] - Duration: 2 min 36 s - Found 1 issue: Important 1 - Unique contributions: none — its single finding overlaps with the other two agents - Accuracy: no false positives - Depth: traced into `app/demo/controller.go` for comparison, ran `git blame` - Coverage: reviewed all 5 files, but missed the error masking and path segment issues - Coverage misses: `logs_passthrough.go` lines 44-47 (error masking) flagged by both other agents - Signal-to-noise: clean but shallow ### [Gemini] - Duration: 5 min 16 s - Found 4 issues: Important 2, Suggestion 2 - Unique contributions: trailing path segments (Suggestion 2), WebSocket close frame timing (Suggestion 3) - Accuracy: no false positives - Depth: examined error propagation, WebSocket protocol behavior, and path parsing - Coverage: reviewed all 5 files, all accounted for - Signal-to-noise: high — all findings are actionable, no noise ### Summary | Metric | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro | |---|---|---|---| | Duration | 8:34 | 2:36 | 5:16 | | Critical | 0 | 0 | 0 | | Important | 1 | 1 | 2 | | Suggestion | 1 | 0 | 2 | | Design observations | 4 | 1 | 0 | | False positives | 0 | 0 | 0 | | Unique insights | 0 | 0 | 2 | | Files reviewed | 5 | 5 | 5 | | Coverage misses | 0 | 1 | 0 | Gemini delivered the most comprehensive review, finding all four consolidated issues including two unique suggestions. Claude provided strong design observations and testing assessment. Codex was fast but shallow, contributing no unique findings. All three agents converged on the highest-value finding (WebSocket origin check), validating the multi-agent approach. --- ## Appendix: Original Reviews ### Claude Opus 4.6 Now I have all the context needed. Let me compile the review. --- ## PR #198: mock controller: Add a mock logs endpoint **Verdict:** Approve with minor suggestions ### Executive Summary This PR adds a WebSocket-based mock logs endpoint to the mock controller for UI development. It implements the `PassthroughController` interface, creates a `handleLogs` handler that returns container labels over WebSocket, adds a BATS integration test, and extracts the curl WebSocket version check into a reusable helper. The implementation follows established patterns from the demo controller. One functional concern around WebSocket origin checking and two minor suggestions. ### Findings #### Critical Issues None. #### Important Issues 1. **Default `CheckOrigin` may reject browser WebSocket connections** — `pkg/controllers/mock/logs_passthrough.go:50` (important, regression) ```go upgrader := websocket.Upgrader{} conn, err := upgrader.Upgrade(w, r, nil) ``` The zero-value `Upgrader` uses gorilla/websocket's default `checkSameOrigin`, which compares the `Origin` header against the `Host` header. The demo controller at `pkg/controllers/app/demo/controller.go:90-92` explicitly sets `CheckOrigin: func(_ *http.Request) bool { return true }` to allow cross-origin connections. Requests from browsers (the stated use case: "For working on the UI") include an `Origin` header set to the page's origin. Because the passthrough reverse proxy preserves the original `Host` header through to the controller, same-origin browser connections should work. However, if the UI is served from a separate development server (a common pattern, e.g., Vite on port 5173), the `Origin` won't match `Host` and the upgrade will be rejected with 403. The curl-based tests pass because curl doesn't send an `Origin` header, so `checkSameOrigin` returns true unconditionally. Fix: Match the demo controller's pattern: ```diff - upgrader := websocket.Upgrader{} + upgrader := websocket.Upgrader{ + CheckOrigin: func(_ *http.Request) bool { return true }, + } ``` #### Suggestions 2. **All API errors return 404** — `pkg/controllers/mock/logs_passthrough.go:44-47` (suggestion, regression) ```go if err != nil { log.V(5).Info("Failed to get container", "error", err) http.Error(w, "Failed to get container", http.StatusNotFound) return } ``` Any error from `c.mgr.GetClient().Get()` — not-found, serialization failure, kine database issue — returns HTTP 404. During UI development, this could mask real backend problems as "container not found." Distinguishing not-found from internal errors would improve debuggability. Fix: ```diff + if apierrors.IsNotFound(err) { + log.V(5).Info("Container not found", "container", containerID) + http.Error(w, "Container not found", http.StatusNotFound) + return + } if err != nil { - log.V(5).Info("Failed to get container", "error", err) - http.Error(w, "Failed to get container", http.StatusNotFound) + log.V(5).Info("Failed to get container", "container", containerID, "error", err) + http.Error(w, "Failed to get container", http.StatusInternalServerError) return } ``` This requires adding `apierrors "k8s.io/apimachinery/pkg/api/errors"` to the imports (already used in `mock_controller.go`). ### Design Observations **Strengths** - The passthrough map pattern in `newController()` is a clean, direct copy of the demo controller's approach. Endpoint registration is trivially extensible. - The WebSocket close sequence (line 57-63) correctly uses LIFO defer ordering: send close message first, then close the connection. - Extracting `curl_has_websocket_support()` into `commands.bash` avoids duplication across both WebSocket passthrough tests. The new regex `wss?` is more explicit than the old `ws` (though functionally equivalent since `ws` matched as a substring of `wss`). - The test structure is thorough: it validates preconditions (namespace, configmap, endpoint registration), tests the happy path with dynamic data, and tests two rejection cases (invalid format and nonexistent container). ### Testing Assessment Coverage is adequate for a mock endpoint: 1. Happy path: valid container ID returns expected log lines with container ID and labels — covered. 2. Invalid format (non-hex characters): returns error — covered via `run -22` on `invalid-container-id`. 3. Nonexistent container (valid hex, not in API): returns error — covered via `run -22` on `00000000`. 4. Empty container ID (`/passthrough/mock/logs/` with no ID): not explicitly tested, but handled by the regex validator since `+` requires at least one character. 5. Container with no labels (`Status.Labels` is nil): not tested, but safe — `maps.Keys(nil)` returns an empty iterator, so the loop is a no-op. ### Coverage Summary | File | Status | |------|--------| | `bats/helpers/commands.bash` | Reviewed, no issues | | `bats/tests/32-app-controllers/passthrough.bats` | Trivial — SPDX headers added, `curl_has_websocket_support` refactor | | `bats/tests/34-containers-controllers/containers-mock.bats` | Reviewed, no issues | | `pkg/controllers/mock/logs_passthrough.go` | **Finding 1, Finding 2** | | `pkg/controllers/mock/mock_controller.go` | Reviewed, no issues | ### Codex GPT 5.4 ### Executive Summary This PR adds a mock WebSocket logs passthrough under `/passthrough/mock/logs/{container}`, wires it into the mock controller’s discovery output, and adds BATS coverage for the happy path plus invalid IDs. The implementation is close, but the new WebSocket handler is configured in a way that works with `curl` and fails for browser clients, which is the main use case this PR is meant to unblock. ### Findings **Critical Issues** None. **Important Issues** 1. **Browser clients will be rejected by the new WebSocket endpoint** — [`pkg/controllers/mock/logs_passthrough.go:50`](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/mock\/logs_passthrough.go#L50) `(important, regression)` ```go upgrader := websocket.Upgrader{} conn, err := upgrader.Upgrade(w, r, nil) if err != nil { log.V(5).Info("Failed to upgrade to WebSocket", "error", err) return } ``` The new handler creates a zero-value `websocket.Upgrader` at line 50 and upgrades at line 51, which means gorilla/websocket applies its default origin check: reject any request whose `Origin` host does not match `Host`. That is fine for the new BATS coverage in [`bats/tests/34-containers-controllers/containers-mock.bats:67`](\/Users\/jan\/git\/rancher-desktop-daemon\/bats\/tests\/34-containers-controllers\/containers-mock.bats#L67), because `curl` does not send an `Origin` header here, but it breaks the stated frontend use case because browser WebSocket requests do send `Origin`. The existing demo passthrough explicitly handles this at [`pkg/controllers/app/demo/controller.go:90`](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/app\/demo\/controller.go#L90)-[`91`](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/app\/demo\/controller.go#L91), so this PR is introducing a stricter policy than the repo’s other passthrough WebSocket endpoint. I confirmed with `git blame` that the missing `CheckOrigin` was introduced in commit `1b7060e`. Fix: configure `CheckOrigin` deliberately for this internal single-user passthrough, and add coverage that sends an `Origin` header so the test matches browser behavior. ```diff - upgrader := websocket.Upgrader{} + upgrader := websocket.Upgrader{ + CheckOrigin: func(_ *http.Request) bool { return true }, + } ``` **Suggestions** None. ### Design Observations **Strengths** - Reusing the existing `base.PassthroughController` discovery path in [`pkg/controllers/mock/mock_controller.go:82`](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/mock\/mock_controller.go#L82)-[`87`](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/mock\/mock_controller.go#L87) is the right shape. It keeps the mock endpoint discoverable through the controller-manager config map instead of hard-coding a one-off route. ### Testing Assessment 1. No test exercises `/passthrough/mock/logs/{container}` with a browser-style `Origin` header. The current BATS case in [`bats/tests/34-containers-controllers/containers-mock.bats:67`](\/Users\/jan\/git\/rancher-desktop-daemon\/bats\/tests\/34-containers-controllers\/containers-mock.bats#L67)-[`76`](\/Users\/jan\/git\/rancher-desktop-daemon\/bats\/tests\/34-containers-controllers\/containers-mock.bats#L76) only covers `curl`’s origin-less handshake, so it misses the failure in Finding 1. ### Documentation Assessment No documentation changes look necessary for this mock-only endpoint. ### Coverage Summary - `bats/helpers/commands.bash` — Reviewed, no issues - `bats/tests/32-app-controllers/passthrough.bats` — Reviewed, no issues - `bats/tests/34-containers-controllers/containers-mock.bats` — Reviewed, no issues - `pkg/controllers/mock/logs_passthrough.go` — Finding 1 - `pkg/controllers/mock/mock_controller.go` — Reviewed, no issues ### Gemini 3.1 Pro ### Executive Summary This PR adds a mock logs endpoint (`/passthrough/mock/logs/{container}`) that returns static label data to simulate container logs over a WebSocket connection. The implementation and integration tests are generally solid, but the `websocket.Upgrader` initialization is missing an origin configuration, which will block the intended UI connections due to cross-origin restrictions. There are also a few areas where error mapping and connection termination could be improved. ### Findings #### Critical Issues None. #### Important Issues 1. **Missing Cross-Origin Check in WebSocket Upgrader** — `pkg/controllers/mock/logs_passthrough.go:50-51` (important, gap) ```go upgrader := websocket.Upgrader{} conn, err := upgrader.Upgrade(w, r, nil) ``` The `gorilla/websocket` `Upgrader` defaults to rejecting requests where the `Origin` header is present but does not match the `Host` header. The PR description explicitly states this endpoint is "for working on the UI". Because the UI runs in an Electron browser context with a custom protocol/origin (e.g., `app://...` or `http://localhost:...`) and connects to the daemon at `127.0.0.1:6443`, the origins will not match. The UI will receive a `403 Forbidden` error when it attempts to connect. Other passthrough endpoints intended for the UI (such as `app/demo/controller.go`) handle this by explicitly bypassing the check. Fix: Add a `CheckOrigin` function that allows the connection. ```diff - upgrader := websocket.Upgrader{} + upgrader := websocket.Upgrader{ + CheckOrigin: func(r *http.Request) bool { return true }, + } ``` 2. **Broad error masking on container lookup** — `pkg/controllers/mock/logs_passthrough.go:44-48` (important, gap) ```go if err != nil { log.V(5).Info("Failed to get container", "error", err) http.Error(w, "Failed to get container", http.StatusNotFound) return } ``` Any error returned by the Kubernetes client (such as the API server being unavailable, context cancellation, or RBAC issues) is unconditionally converted into an `http.StatusNotFound` (404) response. This masks the true cause of failures and complicates debugging if the embedded API server or storage encounters an error. Fix: Use `apierrors.IsNotFound` to differentiate missing resources from internal errors. You will need to add `"k8s.io/apimachinery/pkg/api/errors"` to the imports. ```diff if err != nil { log.V(5).Info("Failed to get container", "error", err) - http.Error(w, "Failed to get container", http.StatusNotFound) + if apierrors.IsNotFound(err) { + http.Error(w, "Failed to get container", http.StatusNotFound) + } else { + http.Error(w, "Internal server error", http.StatusInternalServerError) + } return } ``` #### Suggestions 1. **Trailing path segments silently ignored** — `pkg/controllers/mock/logs_passthrough.go:30` (suggestion, gap) ```go containerID, _, _ := strings.Cut(strings.TrimLeft(r.URL.Path, "/"), "/") ``` `strings.Cut` extracts the first segment as `containerID`, but any subsequent segments are ignored. For example, a request to `/passthrough/mock/logs/1234/extra/stuff` will successfully extract `1234` and serve the logs. While this is likely harmless for a mock endpoint, it can mask malformed requests from the frontend during development. Fix: Check the `after` return value from `strings.Cut` and reject the request if the path contains extra segments. ```diff - containerID, _, _ := strings.Cut(strings.TrimLeft(r.URL.Path, "/"), "/") + containerID, after, _ := strings.Cut(strings.TrimLeft(r.URL.Path, "/"), "/") + if after != "" { + http.Error(w, "Invalid path", http.StatusBadRequest) + return + } ``` 2. **Abrupt WebSocket closure before client can process close frame** — `pkg/controllers/mock/logs_passthrough.go:56-63` (suggestion, gap) ```go defer conn.Close() defer func() { message := websocket.FormatCloseMessage(websocket.CloseNormalClosure, "Closing connection") err := conn.WriteControl(websocket.CloseMessage, message, time.Now().Add(time.Second)) // ... }() ``` The handler writes the logs synchronously and then immediately returns, executing the deferred `WriteControl` to send the close frame followed directly by the deferred `conn.Close()`. Because `conn.Close()` immediately drops the underlying TCP connection without waiting for the client to echo the close frame, the client may experience an abrupt connection reset (`1006 Abnormal Closure` in browsers) or miss the final in-flight log messages. Fix: While acceptable for a simple mock, a cleaner standard approach is to wait briefly or perform a read loop after sending the close message until an error or close frame is received from the client. ### Coverage Summary - `bats/helpers/commands.bash` — Reviewed, no issues - `bats/tests/32-app-controllers/passthrough.bats` — Reviewed, no issues - `bats/tests/34-containers-controllers/containers-mock.bats` — Reviewed, no issues - `pkg/controllers/mock/logs_passthrough.go` — Findings 1, 2, 3, 4 - `pkg/controllers/mock/mock_controller.go` — Reviewed, no issues