Return error channel from PortForward#12512
Open
caseydavenport wants to merge 1 commit intoprojectcalico:masterfrom
Open
Return error channel from PortForward#12512caseydavenport wants to merge 1 commit intoprojectcalico:masterfrom
caseydavenport wants to merge 1 commit intoprojectcalico:masterfrom
Conversation
The port-forward goroutine previously swallowed errors silently. Return a channel so callers can detect when the port-forward process exits and react (e.g., restart it or fail fast with a clear message).
caseydavenport
added
docs-not-required
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the e2e Kubectl helper’s port-forward implementation so callers can observe when the underlying kubectl port-forward process exits (and why), instead of errors being silently swallowed.
Changes:
- Change
Kubectl.PortForwardto return an additional<-chan errorthat yields theExec()result when port-forward terminates. - Update staged policy e2e test to accommodate the new return value (ignore the channel for now).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| e2e/pkg/utils/kubectl.go | Extends PortForward to return an error channel and propagates the kubectl command exit result via that channel. |
| e2e/pkg/tests/policy/staged_policy.go | Updates the call site to the new PortForward signature by ignoring the new error channel. |
| // local port to avoid conflicts when tests run in parallel. It returns the local port | ||
| // and a channel that receives an error (or nil) when the port-forward process exits. | ||
| // Callers can select on the channel to detect unexpected port-forward termination. | ||
| func (k *Kubectl) PortForward(ns, pod, remotePort, user string, timeOut chan time.Time) (int, <-chan error, error) { |
There was a problem hiding this comment.
timeOut is declared as a bidirectional chan time.Time, but PortForward never sends on it (it only passes it to WithTimeout). Keeping it bidirectional unnecessarily restricts callers (e.g., you can’t pass <-chan time.Time values like time.After(...) directly) and weakens the API contract. Consider changing the parameter type to <-chan time.Time (and adjusting the call sites accordingly).
Suggested change
| func (k *Kubectl) PortForward(ns, pod, remotePort, user string, timeOut chan time.Time) (int, <-chan error, error) { | |
| func (k *Kubectl) PortForward(ns, pod, remotePort, user string, timeOut <-chan time.Time) (int, <-chan error, error) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The port-forward goroutine in the e2e test helpers previously swallowed errors silently - if the underlying
kubectl port-forwardprocess died (pod restart, network blip, etc.), callers had no way to know. Tests would just getconnection refusedon the next poll and time out with misleading errors.PortForwardnow returns a<-chan erroralongside the port. Callers that don't care can_it (like the whisker test does today), but callers that need resilience can select on it to detect failures and restart.This is the OSS half of a fix for a flaky Linseed port-forward in the enterprise e2e suite. The enterprise side adds a supervisor loop in the Linseed
PortForward()that watches this channel and auto-restarts.