From d0842249d3b92ea67c446fe273f84fe74ebaed9f Mon Sep 17 00:00:00 2001
From: Brian Pursley <bpursley@cinlogic.com>
Date: Tue, 13 Dec 2022 13:32:42 -0500
Subject: [PATCH 1/2] portforward: return error on lost connection to pod

Currently, when the remote connection is unexpected closed, forward() prints an error message saying "lost connection to pod" via runtime.HandleError, but then it returns nil for the error.

This prevents the caller from being able to handle this error differently.

This commit changes forward() to return the "lost connection to pod" error so that it can be handled by the caller.

Making this change enables kubectl port-forward to exit with code 1, instead of 0, which is the expected behavior for a command that has failed.

Kubernetes-commit: a9f04103854893056237a09250ad3335867b0391
---
 tools/portforward/portforward.go      |  4 +++-
 tools/portforward/portforward_test.go | 30 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/portforward/portforward.go b/tools/portforward/portforward.go
index 6b5e3076..b581043f 100644
--- a/tools/portforward/portforward.go
+++ b/tools/portforward/portforward.go
@@ -37,6 +37,8 @@ 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")
+
 // PortForwarder knows how to listen for local connections and forward them to
 // a remote pod via an upgraded HTTP request.
 type PortForwarder struct {
@@ -230,7 +232,7 @@ func (pf *PortForwarder) forward() error {
 	select {
 	case <-pf.stopChan:
 	case <-pf.streamConn.CloseChan():
-		runtime.HandleError(errors.New("lost connection to pod"))
+		return ErrLostConnectionToPod
 	}
 
 	return nil
diff --git a/tools/portforward/portforward_test.go b/tools/portforward/portforward_test.go
index ada70339..070c8904 100644
--- a/tools/portforward/portforward_test.go
+++ b/tools/portforward/portforward_test.go
@@ -567,3 +567,33 @@ func TestWaitForConnectionExitsOnStreamConnClosed(t *testing.T) {
 	port := ForwardedPort{}
 	pf.waitForConnection(&listener, port)
 }
+
+func TestForwardPortsReturnsErrorWhenConnectionIsLost(t *testing.T) {
+	dialer := &fakeDialer{
+		conn: newFakeConnection(),
+	}
+
+	stopChan := make(chan struct{})
+	readyChan := make(chan struct{})
+	errChan := make(chan error)
+
+	pf, err := New(dialer, []string{":5000"}, stopChan, readyChan, os.Stdout, os.Stderr)
+	if err != nil {
+		t.Fatalf("faile to create new PortForwarder: %s", err)
+	}
+
+	go func() {
+		errChan <- pf.ForwardPorts()
+	}()
+
+	<-pf.Ready
+
+	pf.streamConn.Close()
+
+	err = <-errChan
+	if err == nil {
+		t.Fatalf("unexpected non-error from pf.ForwardPorts()")
+	} else if err != ErrLostConnectionToPod {
+		t.Fatalf("unexpected error from pf.ForwardPorts(): %s", err)
+	}
+}

From b0a6a6f777230d621ca668149cd0788edd9b3022 Mon Sep 17 00:00:00 2001
From: Brian Pursley <bpursley@cinlogic.com>
Date: Fri, 23 Dec 2022 10:11:21 -0500
Subject: [PATCH 2/2] portforward: Add unit test to cover stopChan usage

Kubernetes-commit: 6f08ab013c6ce54de239aab46b4086c09464f977
---
 tools/portforward/portforward_test.go | 33 ++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/portforward/portforward_test.go b/tools/portforward/portforward_test.go
index 070c8904..3c90a3fd 100644
--- a/tools/portforward/portforward_test.go
+++ b/tools/portforward/portforward_test.go
@@ -579,7 +579,7 @@ func TestForwardPortsReturnsErrorWhenConnectionIsLost(t *testing.T) {
 
 	pf, err := New(dialer, []string{":5000"}, stopChan, readyChan, os.Stdout, os.Stderr)
 	if err != nil {
-		t.Fatalf("faile to create new PortForwarder: %s", err)
+		t.Fatalf("failed to create new PortForwarder: %s", err)
 	}
 
 	go func() {
@@ -588,6 +588,7 @@ func TestForwardPortsReturnsErrorWhenConnectionIsLost(t *testing.T) {
 
 	<-pf.Ready
 
+	// Simulate lost pod connection by closing streamConn, which should result in pf.ForwardPorts() returning an error.
 	pf.streamConn.Close()
 
 	err = <-errChan
@@ -597,3 +598,33 @@ func TestForwardPortsReturnsErrorWhenConnectionIsLost(t *testing.T) {
 		t.Fatalf("unexpected error from pf.ForwardPorts(): %s", err)
 	}
 }
+
+func TestForwardPortsReturnsNilWhenStopChanIsClosed(t *testing.T) {
+	dialer := &fakeDialer{
+		conn: newFakeConnection(),
+	}
+
+	stopChan := make(chan struct{})
+	readyChan := make(chan struct{})
+	errChan := make(chan error)
+
+	pf, err := New(dialer, []string{":5000"}, stopChan, readyChan, os.Stdout, os.Stderr)
+	if err != nil {
+		t.Fatalf("failed to create new PortForwarder: %s", err)
+	}
+
+	go func() {
+		errChan <- pf.ForwardPorts()
+	}()
+
+	<-pf.Ready
+
+	// Closing (or sending to) stopChan indicates a stop request by the caller, which should result in pf.ForwardPorts()
+	// returning nil.
+	close(stopChan)
+
+	err = <-errChan
+	if err != nil {
+		t.Fatalf("unexpected error from pf.ForwardPorts(): %s", err)
+	}
+}