feat: port forwarding for pods, services, and workloads (#1) - #29
feat: port forwarding for pods, services, and workloads (#1)#29lingdie wants to merge 2 commits into
Conversation
zjy365
left a comment
zjy365
left a comment
There was a problem hiding this comment.
Reviewed both axes (repo standards + spec fidelity). Verified locally: go -C core test ./... and pnpm typecheck pass on this branch, but gofmt -l flags forward_resolve.go, portforward_resolve_test.go, forward_resolve_test.go, and this PR has no CI checks reported — the AGENTS.md verification suite evidently wasn't run.
Blocking issues (details as inline comments):
- Named
targetPortresolution is functionally wrong —portMatchesServicePortcompares the EndpointSlice port name (which mirrors the Service port name) againsttargetPort.StrVal(the container port name). The new test hides this by using the same string for both. Common configs likeport: http, targetPort: webfail with "no ready endpoints". - No lifecycle ownership: forwards outlive context switches, view close, and session teardown.
StopAllPortForwardshas zero production callers;server.gopassescontext.Background(); the renderer'sensureContextdrops entries without calling stop. Local binds leak until process exit. Forwards are also unmonitored after start (errChanabandoned), so a rolled/deleted pod kills the listener while the UI still showslocalhost:PORTas active. - UI offers forwarding for Jobs, but core's
forwardKinds/validate.goreject Job — every Job forward fails. ReplicaSet is the reverse (core supports, UI never surfaces). Kind set is duplicated in 5 places. - Pod selection ignores the "pick the newest" comment — no
creationTimestampin the sort, so during a rollout a forward can land on an old pod.
Also: dead code (forwardWorkloadKinds, unused OverviewTab imports, no-op localPort: 0 spread, unused "scale" dialog type), gofmt, and the PR body is empty — the (#1) reference points at an unrelated closed issue (kubeconfig context collisions). Please link the real spec/issue.
| return port.Port != nil && int64(*port.Port) == int64(servicePort.TargetPort.IntValue()) | ||
| } | ||
| return port.Name != nil && *port.Name == servicePort.TargetPort.StrVal | ||
| } |
There was a problem hiding this comment.
[P1] Named targetPort matching is wrong. The EndpointSlice port.Name mirrors the Service port name, not the container target port name, so *port.Name == servicePort.TargetPort.StrVal fails for the common pattern port: http, targetPort: web — the forward errors with "no ready endpoints" even though the service works fine. The new test (forward_resolve_test.go:27-38) uses the same string (named = "http") for both the Service port name and the targetPort, which papers over the bug. To resolve a named targetPort you need the backing Pod's container spec (match containerPort.Name), then intersect with the endpoint's numeric port. The legacy Endpoints path has the same flaw. Please add a test where the service port name and targetPort differ.
| } | ||
| for portIndex := range subset.Ports { | ||
| port := subset.Ports[portIndex] | ||
| if port.Name == servicePort.TargetPort.StrVal || (servicePort.TargetPort.Type == intstr.Int && int64(port.Port) == int64(servicePort.TargetPort.IntValue())) { |
There was a problem hiding this comment.
[P2] gofmt: this line broke indentation (the if is outdented into the loop body). gofmt -l also flags portforward_resolve_test.go and forward_resolve_test.go. Please run gofmt -w and the AGENTS.md verification suite — no CI checks are reported on this PR.
| return "", 0, fmt.Errorf("no pods match %q %q", request.Kind, request.Name) | ||
| } | ||
| // Prefer Running and ready, then Running, then anything else; among | ||
| // equals, pick the newest so a rollout lands on the current pods. |
There was a problem hiding this comment.
[P2] Comment/implementation mismatch. "among equals, pick the newest so a rollout lands on the current pods" — but podForwardRank only scores phase/ready, and SliceStable keeps API list order for equal ranks. During a rollout, newest-pod preference doesn't exist here. Either add creationTimestamp as a tiebreaker or fix the comment.
| // forwardWorkloadKinds are the selector-based kinds a forward can resolve to | ||
| // a single backing pod. Every one of them exposes spec.selector of | ||
| // metav1.LabelSelector shape, so the same code path serves all. | ||
| var forwardWorkloadKinds = map[string]bool{ |
There was a problem hiding this comment.
[P3] Dead code: forwardWorkloadKinds is never referenced anywhere in the repo. Remove it, or use it as the single source of truth — right now the supported-kind set is duplicated in 5 places: this map, resources/portforward.go forwardKinds, ResolveForwardTarget's switch, resolveWorkloadForwardTarget's switch, validate.go:260, plus the renderer's isWorkloadLogKind. Adding a kind is shotgun surgery; one shared table would be better.
| // the pod port over SPDY. The returned stop function tears the listener down; | ||
| // the forward also ends when the context is cancelled. | ||
| func (m *Manager) PortForward(ctx context.Context, contextID, namespace, name string, podPort int64) (func(), int, error) { | ||
| func (m *Manager) PortForward(ctx context.Context, contextID, namespace, name string, podPort, localPort int64) (func(), int, error) { |
There was a problem hiding this comment.
[P1] Stale doc comment + no lifecycle backstop. "The returned stop function tears the listener down; the forward also ends when the context is cancelled" — but server.go:467 now passes context.Background(), so the go func(){ <-ctx.Done(); stop() }() guard at the end of this function is permanently parked, and "random free port" above is no longer true either (localPort may be explicit). The claim in portforward.go:12 that forwards are "reclaimed on stop or context teardown" has no mechanism: StopAllPortForwards has zero non-test callers. Please tie forward lifetime to something real (a context owned by the target kube-context in the registry, or a teardown hook).
| // request that started it: the request context dies as soon as the | ||
| // response is written, while the forward must survive until it is | ||
| // stopped explicitly or the sidecar process exits. | ||
| result, err := s.resources.StartPortForward(context.Background(), value) |
There was a problem hiding this comment.
[P1] context.Background() detaches the forward from every owner. The comment explains why the request ctx can't be used (fair), but then the forward has no parent at all: it survives context switching, view close, and cluster removal until process exit. Pass a context tied to the target kube-context's lifetime in the service registry (or register with a teardown hook that calls StopAllPortForwards), so local loopback binds are reclaimed deterministically.
| /** Resets the store when the active context changes: a forward belongs to the context it was started in. */ | ||
| function ensureContext(contextId: string) { | ||
| if (state.contextId === contextId) return; | ||
| state = { contextId, entries: new Map() }; |
There was a problem hiding this comment.
[P1] Context switch orphans core-side forwards. ensureContext discards the entry map — including each entry's core-assigned id — without calling desktop.resources.portForwardStop. Combined with server.go:467 (context.Background()), the local ports stay bound indefinitely and become uncontrollable from the UI. Stop each tracked forward (or at least keep ids retrievable) before wiping the map.
| const isService = row?.kind === "Service"; | ||
| // The Ports tab serves every kind whose forward the core can resolve; it | ||
| // stays visible even without declared ports (manual input covers those). | ||
| const canForward = Boolean(row && (isPod || isService || isWorkloadLogKind(row.kind))); |
There was a problem hiding this comment.
[P2] UI/core kind mismatch. isWorkloadLogKind includes "Job", so the Ports tab is offered on Job details — but forwardKinds and validate.go reject Job, so every attempt fails with "Job" cannot be port-forwarded. Conversely ReplicaSet is core-supported but never offered here. Either add Job/CronJob support in core or gate the tab on the actual forwardable set; a shared kind list (see my note on forwardWorkloadKinds) would prevent this class of drift.
| import type { RelatedResource, ResourceEvent, ResourceRow } from "../../shared/types"; | ||
| import { Button } from "../components/ui/button"; | ||
| import { StatusDot } from "../components/ResourceTable"; | ||
| import { PortForwardSection } from "./PortForwardSection"; |
There was a problem hiding this comment.
[P3] Unused imports. PortForwardSection and ForwardPort are imported here but never used in this file — dead wiring (or an unfinished refactor). Remove them.
| return PortForwardResponse{ID: id, LocalPort: boundPort, Pod: resolved}, nil | ||
| } | ||
|
|
||
| var forwardKinds = map[string]bool{ |
There was a problem hiding this comment.
[P2] forwardKinds duplicates the RPC caps (validate.go:260) and the resolver's switch — re-validation at the core boundary is the documented standard, so the RPC copy stays, but the semantic kind set should come from one place to avoid drift (the Job mismatch I flagged in ResourceDetailView.tsx:183 is exactly this drift). Also: StopAllPortForwards below has no production callers — see my notes on manager.go / server.go.
2 participants
No description provided.