Close connection and stop listening when port forwarding errors occur so that kubectl can exit #103526

brianpursley · 2021-07-06T21:36:28Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, if port forwarding and the connection is lost and re-established, port forwarding gets into an unrecoverable state where it logs errors but does not fail. When this happens port forwarding no longer is working, but there is no way to handle this case except to manually terminate kubectl.

This PR changes the port forwarding code to capture errors that are received on the error stream and surface them to be returned so that kubectl can exit with a non-zero exit code instead of being "hung" in an unrecoverable error state.

This PR changes the port forwarding code to close the remote connection and stop listening for new connections when an error occurs while port forwarding.

Which issue(s) this PR fixes:

Related to kubernetes/kubectl#686

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubectl port-forward service will now properly exit when the attached pod dies

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

brianpursley · 2021-07-06T23:02:33Z

/cc @deads2k you had some comments on a different, unmerged PR a few months ago for this issue.

caesarxuchao · 2021-07-08T20:13:58Z

/assign
/triage accepted

caesarxuchao · 2021-07-08T20:30:11Z

This looks reasonable. Can you add a unit test?

brianpursley · 2021-07-12T17:49:21Z

This looks reasonable. Can you add a unit test?

Good idea. I added a couple unit tests.

brianpursley · 2021-07-12T19:03:05Z

/retest

brianpursley · 2021-11-16T19:34:42Z

/retest

deads2k · 2021-11-16T22:51:46Z

staging/src/k8s.io/client-go/tools/portforward/portforward.go

@@ -231,6 +233,8 @@ func (pf *PortForwarder) forward() error {
 	case <-pf.stopChan:
 	case <-pf.streamConn.CloseChan():
 		runtime.HandleError(errors.New("lost connection to pod"))
+	case err := <-pf.errorChan:


in the first case, a stop was called. portforward should probably shut down its ports on this for the library (pr wanted), but the intent is likely "process shutting down" when we wrote it.

in the second case, the streamconnection was lost

in this new case, the local handleConnection will stop https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/portforward/portforward.go#L356, but the callsite will restart it: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/portforward/portforward.go#L302-L312

if I'm reading this code correctly, this would return an error back and start listening on the port again. the tests are for the handleConnection, but miss the waitForConnection which will restart it indefinitely.

This error actually gets returned all the way up to the kubectl command itself and stops execution.

Return from forward()

Return from ForwardPorts

Return from ForwardPorts

Return from RunPortForward

I think that still races for whether the ForwardPort go route runs before the reconnect from waitForConnection.

I'm supportive of trying to return errors. I just want deterministic behavior when we make this change. Closing a stop channel (or even better a context) that is honored throughout the call chain is an approach that will make this deterministic and non-leaky.

deads2k · 2021-11-16T22:53:45Z

/hold

holding until #103526 (comment) is resolved.

…orwarding

brianpursley · 2021-11-17T03:06:29Z

/retest

deads2k · 2021-11-17T14:40:49Z

staging/src/k8s.io/client-go/tools/portforward/portforward.go

+				return
+			}
+			go pf.handleConnection(conn, port)


pre-existng and not blocking for this PR, but this code is so strange. it seems ripe for cleanup to have fewer go funcs launch go funcs and infinite loops.

deads2k · 2021-11-17T14:45:58Z

staging/src/k8s.io/client-go/tools/portforward/portforward.go

-				runtime.HandleError(fmt.Errorf("error accepting connection on port %d: %v", port.Local, err))
-			}
+		select {
+		case <-pf.streamConn.CloseChan():


good improvement.

deads2k · 2021-11-17T14:46:42Z

I think that overall the existing code could benefit from simplification and context wiring, but this PR now looks predictable to me.

/approve
/hold cancel

deads2k · 2021-11-17T14:47:21Z

bug fix

/milestone v1.23

eddiezane

/lgtm

k8s-ci-robot · 2021-11-17T14:58:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, deads2k, eddiezane, justinmchase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from liggitt and sttts July 6, 2021 21:37

brianpursley changed the title ~~Surface port forwarding errors so that kubectl can exit with error~~ Return port forwarding errors so that kubectl can exit with error Jul 7, 2021

k8s-ci-robot assigned caesarxuchao Jul 8, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 8, 2021

brianpursley force-pushed the kubectl-686 branch from 1a19be1 to 33cda18 Compare July 12, 2021 17:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 12, 2021

brianpursley changed the title ~~Return port forwarding errors so that kubectl can exit with error~~ [WIP] Return port forwarding errors so that kubectl can exit with error Jul 13, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2021

brianpursley force-pushed the kubectl-686 branch 4 times, most recently from 6e41199 to 9596c48 Compare July 15, 2021 19:16

brianpursley changed the title ~~[WIP] Return port forwarding errors so that kubectl can exit with error~~ Return port forwarding errors so that kubectl can exit with error Jul 15, 2021

deads2k reviewed Nov 16, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2021

CatherineF-dev mentioned this pull request Nov 17, 2021

Fix metrics AlreadyRegisteredError on TestSampler unit test #105886

Closed

brianpursley force-pushed the kubectl-686 branch from bd06c69 to aa8652b Compare November 17, 2021 02:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2021

brianpursley changed the title ~~Return port forwarding errors so that kubectl can exit with error~~ Close connection and stop listening when port forwarding errors occur so that kubectl can exit Nov 17, 2021

close streamConn and stop listening when an error occurs while port f…

4a35e6c

…orwarding

brianpursley force-pushed the kubectl-686 branch from aa8652b to 4a35e6c Compare November 17, 2021 02:06

deads2k reviewed Nov 17, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 17, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2021

eddiezane approved these changes Nov 17, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2021

k8s-ci-robot merged commit cd6ffff into kubernetes:master Nov 17, 2021

eddiezane mentioned this pull request Feb 2, 2022

Port-forward drops connection to pod after first connection kubernetes/kubectl#1169

Closed

rm3l mentioned this pull request Jun 27, 2022

odo dev port forward timeouts if app is not ready redhat-developer/odo#5877

Closed

ghost mentioned this pull request Jul 25, 2022

Stop port-forwarding after errors occured - let the user decide. kubernetes/kubectl#1249

Open

panteparak mentioned this pull request Aug 15, 2022

port-forward closed on specific postgres command #111825

Open

brianpursley mentioned this pull request Aug 15, 2022

client-go/tools/portforward: Add ErrorHandler field #111860

Closed

brianpursley deleted the kubectl-686 branch February 2, 2023 01:21

chobostar mentioned this pull request Mar 29, 2023

Port-forward dropped after disconnection CrunchyData/postgres-operator#3547

Closed

brianpursley mentioned this pull request Apr 18, 2023

Port forwarding: preserve streamConn on error if any bytes sent/received #117442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close connection and stop listening when port forwarding errors occur so that kubectl can exit #103526

Close connection and stop listening when port forwarding errors occur so that kubectl can exit #103526

brianpursley commented Jul 6, 2021 •

edited

brianpursley commented Jul 6, 2021

caesarxuchao commented Jul 8, 2021

caesarxuchao commented Jul 8, 2021

brianpursley commented Jul 12, 2021

brianpursley commented Jul 12, 2021

brianpursley commented Nov 16, 2021

deads2k Nov 16, 2021

eddiezane Nov 16, 2021

deads2k Nov 17, 2021

deads2k commented Nov 16, 2021

brianpursley commented Nov 17, 2021

deads2k Nov 17, 2021

deads2k Nov 17, 2021

deads2k commented Nov 17, 2021

deads2k commented Nov 17, 2021

eddiezane left a comment

k8s-ci-robot commented Nov 17, 2021

Close connection and stop listening when port forwarding errors occur so that kubectl can exit #103526

Close connection and stop listening when port forwarding errors occur so that kubectl can exit #103526

Conversation

brianpursley commented Jul 6, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

brianpursley commented Jul 6, 2021

caesarxuchao commented Jul 8, 2021

caesarxuchao commented Jul 8, 2021

brianpursley commented Jul 12, 2021

brianpursley commented Jul 12, 2021

brianpursley commented Nov 16, 2021

deads2k Nov 16, 2021

Choose a reason for hiding this comment

eddiezane Nov 16, 2021

Choose a reason for hiding this comment

deads2k Nov 17, 2021

Choose a reason for hiding this comment

deads2k commented Nov 16, 2021

brianpursley commented Nov 17, 2021

deads2k Nov 17, 2021

Choose a reason for hiding this comment

deads2k Nov 17, 2021

Choose a reason for hiding this comment

deads2k commented Nov 17, 2021

deads2k commented Nov 17, 2021

eddiezane left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 17, 2021

brianpursley commented Jul 6, 2021 •

edited