From d6404c8d76d6891022a33e91f5d5879c3a883cf6 Mon Sep 17 00:00:00 2001 From: Nic Date: Fri, 4 Oct 2024 14:48:15 +0800 Subject: [PATCH 1/2] fix: draining remote stream after port-forward connection broken Signed-off-by: Nic Kubernetes-commit: dbe6b6657bacc846656f4009ee869ca996dde1da --- tools/portforward/portforward.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/portforward/portforward.go b/tools/portforward/portforward.go index 83ef3e92..ec26efdd 100644 --- a/tools/portforward/portforward.go +++ b/tools/portforward/portforward.go @@ -406,6 +406,11 @@ func (pf *PortForwarder) handleConnection(conn net.Conn, port ForwardedPort) { case <-remoteDone: case <-localError: } + /* + reset dataStream to discard any unsent data, preventing port forwarding from being blocked. + we must reset dataStream before waiting on errorChan, otherwise, the blocking data will affect errorStream and cause <-errorChan to block indefinitely. + */ + _ = dataStream.Reset() // always expect something on errorChan (it may be nil) err = <-errorChan From bf414551df60c6e9d1f0e37ac1879778ba6b9aea Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Wed, 16 Oct 2024 14:02:01 +0200 Subject: [PATCH 2/2] Clean error handling in port-forward This commit introduces: 1. Cleanups in port-forwarding error handling code, which ensures that we only compare lowercased text always. 2. E2E verifying that when a pod is removed a port-forward is stopped. Signed-off-by: Maciej Szulik Kubernetes-commit: 0b1617ccefbc6ea61c0e7c2b0b4052703f11c51c --- tools/portforward/portforward.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/tools/portforward/portforward.go b/tools/portforward/portforward.go index ec26efdd..126c14e8 100644 --- a/tools/portforward/portforward.go +++ b/tools/portforward/portforward.go @@ -37,7 +37,13 @@ import ( // TODO move to API machinery and re-unify with kubelet/server/portfoward const PortForwardProtocolV1Name = "portforward.k8s.io" -var ErrLostConnectionToPod = errors.New("lost connection to pod") +var ( + // error returned whenever we lost connection to a pod + ErrLostConnectionToPod = errors.New("lost connection to pod") + + // set of error we're expecting during port-forwarding + networkClosedError = "use of closed network connection" +) // PortForwarder knows how to listen for local connections and forward them to // a remote pod via an upgraded HTTP request. @@ -312,7 +318,7 @@ func (pf *PortForwarder) waitForConnection(listener net.Listener, port Forwarded conn, err := listener.Accept() if err != nil { // TODO consider using something like https://github.com/hydrogen18/stoppableListener? - if !strings.Contains(strings.ToLower(err.Error()), "use of closed network connection") { + if !strings.Contains(strings.ToLower(err.Error()), networkClosedError) { runtime.HandleError(fmt.Errorf("error accepting connection on port %d: %v", port.Local, err)) } return @@ -381,7 +387,7 @@ func (pf *PortForwarder) handleConnection(conn net.Conn, port ForwardedPort) { go func() { // Copy from the remote side to the local port. - if _, err := io.Copy(conn, dataStream); err != nil && !strings.Contains(err.Error(), "use of closed network connection") { + if _, err := io.Copy(conn, dataStream); err != nil && !strings.Contains(strings.ToLower(err.Error()), networkClosedError) { runtime.HandleError(fmt.Errorf("error copying from remote stream to local connection: %v", err)) } @@ -394,7 +400,7 @@ func (pf *PortForwarder) handleConnection(conn net.Conn, port ForwardedPort) { defer dataStream.Close() // Copy from the local port to the remote side. - if _, err := io.Copy(dataStream, conn); err != nil && !strings.Contains(err.Error(), "use of closed network connection") { + if _, err := io.Copy(dataStream, conn); err != nil && !strings.Contains(strings.ToLower(err.Error()), networkClosedError) { runtime.HandleError(fmt.Errorf("error copying from local connection to remote stream: %v", err)) // break out of the select below without waiting for the other copy to finish close(localError) @@ -406,10 +412,10 @@ func (pf *PortForwarder) handleConnection(conn net.Conn, port ForwardedPort) { case <-remoteDone: case <-localError: } - /* - reset dataStream to discard any unsent data, preventing port forwarding from being blocked. - we must reset dataStream before waiting on errorChan, otherwise, the blocking data will affect errorStream and cause <-errorChan to block indefinitely. - */ + + // reset dataStream to discard any unsent data, preventing port forwarding from being blocked. + // we must reset dataStream before waiting on errorChan, otherwise, + // the blocking data will affect errorStream and cause <-errorChan to block indefinitely. _ = dataStream.Reset() // always expect something on errorChan (it may be nil)