immediately close the other side of the connection when proxying

[MHBauer]

What this PR does / why we need it:
close the other side of the connection by exiting once one side closes. This code is used in both apiserver and kubelet.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57992

Special notes for your reviewer:
unit tests passed locally. something similar is done in client-go/tools/portforward. doesn't seem to break the tests.

Release note:

Immediately close the other side of the connection when proxying.

[MHBauer]

/test pull-kubernetes-integration

[MHBauer]

/assign @deads2k

[duglin]

/LGTM

[MHBauer]

FYI @mikebrow @Random-Liu Not sure if there is a better node based fix from CRI pushing events to close upwards. Thanks for talking through this with me.
/sig api-machinery

[MHBauer]

/kind bug

[mikebrow]

NIce!

nit.. since we have log for creating the connection, maybe we could use log on closing the connection in the defer?

[MHBauer]

/assign @caesarxuchao

[MHBauer]

@mikebrow before and after the wait? One side or the other specifically?

[mikebrow]

Well yes, but I was specifically looking at these "Connecting to backend proxy" log entires: https://github.com/kubernetes/kubernetes/pull/67288/files?utf8=✓&diff=unified#diff-16a26bb0d342318c6eb3d03e6c5756e9R257

and thinking we should have a reciprocal closing message.

So I suppose after they are closed to balance the "before" it's opened message would be the minimum. And yeah it would also be nice to have a before wait and after wait message. But that might be overkill..

[MHBauer]

/assign @lavalamp

[caesarxuchao]

I have a stupid question, from #57992 (comment),

After closing a port-forward from kubectl, I see immediately the reader close on the apiserver, and the writer close on the kubelet.

Why is that? Shouldn't both the reader and writer close on both apiserver and kubelet?

[MHBauer]

#67288 (comment)

@caesarxuchao I agree, but what mechanism would do that?
My logic for this solution is that no more progress can be made either way and the code is prepared to handle the hangup, so just hang up.

Nothing seems to be broken, and my scenario is fixed during my testing.

[caesarxuchao]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, duglin, MHBauer

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/apimachinery/pkg/OWNERS [caesarxuchao]

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

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[MHBauer]

/test pull-kubernetes-e2e-gce-100-performance

[k8s-github-robot]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

[anguslees]

So ... Let's say I have this exchange:

• client sends request
• client closes the write direction (ie: shutdown() syscall, ie: not http-keepalive)
• server does some non-trivial processing
• server sends reply

Will the connection still be open for the reply? I haven't built it and tried yet, but I gather from the description and the patch that this will kill both directions of the connection when either closes - breaking the above use of not-HTTP-keepalive. Is that correct?

Fwiw, in an ssh / kubectl exec context, the common equivalent of the above scenario is piping to sort on the remote side.

[caesarxuchao]

@MHBauer I thought the io.Copy would only return after after the request connection or the backend connection is closed, in that case, the PR was right. But io.Copy returns at EOF, so @anguslees's comment makes sense.

[MHBauer]

@anguslees
not sure what what the case is.

sort pipe like this? Something else?

     Running `genrand 10000000`
➜  ls -lh
total 195M
-rw-rw-r-- 1 mhb mhb 195M Aug 17 09:40 nums
➜  sort nums | wc -l
10000000
➜  sort nums | wc -l
➜  docker run -i -a stdin -a stdout busybox sort - < nums | wc -l
10000000
➜  kubectl get nodes                                             
NAME          STATUS    ROLES     AGE       VERSION
kube-master   Ready     master    15m       v1.13.0-alpha.0.234+b9544382baf4a0-dirty
kube-node-1   Ready     <none>    14m       v1.13.0-alpha.0.234+b9544382baf4a0-dirty
kube-node-2   Ready     <none>    14m       v1.13.0-alpha.0.234+b9544382baf4a0-dirty
➜  kubectl version
Client Version: version.Info{Major:"1", Minor:"11", GitVersion:"v1.11.1", GitCommit:"b1b29978270dc22fecc592ac55d903350454310a", GitTreeState:"clean", BuildDate:"2018-08-07T04:27:44Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"13+", GitVersion:"v1.13.0-alpha.0.234+b9544382baf4a0-dirty", GitCommit:"b9544382baf4a0a366dd01e05e585f8556993754", GitTreeState:"dirty", BuildDate:"2018-08-17T16:23:52Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"linux/amd64"}

➜  time kubectl run bb --image busybox --restart=Never -i --rm sort - < nums | wc -l
If you don't see a command prompt, try pressing enter.
10000001
kubectl run bb --image busybox --restart=Never -i --rm sort - < nums  1.33s user 1.23s system 3% cpu 1:11.84 total
wc -l  0.13s user 0.16s system 0% cpu 1:11.84 total
➜  
     Running `genrand 10`
➜  cat nums 
196211602033574003
5523453742303018414
15316494326757210401
5642514980812386917
6611172744505202147
17001739917637352584
5020503333694379813
9999606963224618426
18295755340485735596
8929362997318480641
➜  time sort nums                                                
15316494326757210401
17001739917637352584
18295755340485735596
196211602033574003
5020503333694379813
5523453742303018414
5642514980812386917
6611172744505202147
8929362997318480641
9999606963224618426
sort nums  0.00s user 0.00s system 83% cpu 0.003 total
➜  docker run -i -a stdin -a stdout busybox sort - < nums        
15316494326757210401
17001739917637352584
18295755340485735596
196211602033574003
5020503333694379813
5523453742303018414
5642514980812386917
6611172744505202147
8929362997318480641
9999606963224618426
➜  kubectl run bb --image busybox --restart=Never -i --rm sort - < nums        
If you don't see a command prompt, try pressing enter.
15316494326757210401
17001739917637352584
18295755340485735596
196211602033574003
5020503333694379813
5523453742303018414
5642514980812386917
6611172744505202147
8929362997318480641
9999606963224618426
pod "bb" deleted

[MHBauer]

just re-read and saw the exec. that also works with the busybox pod running with an open sh in another terminal.

➜  kubectl exec -i bb  sort - < nums                                   
15316494326757210401
17001739917637352584
18295755340485735596
196211602033574003
5020503333694379813
5523453742303018414
5642514980812386917
6611172744505202147
8929362997318480641
9999606963224618426
➜
     Running `genrand 10000000`
     
➜  kubectl exec -i bb  sort - < nums | wc -l
10000000

[MHBauer]

Do you have a test case? I'm not sure I fully understand.

[anguslees]

Do you have a test case?

Sorry for the delay, it took me a while to find a good test client that actually made a TLS HTTP/1.0 request, with half-close (it appears curl can't be made to do this).

I constructed a full test scenario that generated an HTTP/1.0 TLS request with an early close to a /api/.../proxy' URL for a job that introduced a delay before responding ... and found that my nearby k8s 1.8 (old!) test cluster already "failed" my test. Then I went back and re-read the HTTP/1.0 spec and found that this was acceptable behaviour ("the closing of the connection by either or both parties always terminates the current request, regardless of its status.")

So: My concern about doing this at the HTTP-level was invalid, and your testing above confirms that there's no problem at the tunneled-over-websocket level.

TL;DR: lgtm! And sorry for the distraction.

[mbohlool]

@MHBauer I added a release note to this PR because we need one for the cherrypick. please see if that looks good.

[MHBauer]

@anguslees thanks for the followup.

@mbohlool release note looks fine. Sorry for my hold up in responding.

[MHBauer]

Reading the cherry-pick discussions, this isn't a user facing change. There should be nothing different. It changes how server components interact with each other.