WebSocket Client and V5 RemoteCommand Subprotocol #119157

seans3 · 2023-07-07T17:30:02Z

Implements WebSocket client as WebSocketExecutor. The executor is not yet called by any client.
Creates new version 5 of remote command subprotocol
- RemoteCommand Version 5 adds a CLOSE signal, which is handled on the websocket server by closing the STDIN stream allowing other streams to complete transmission and close gracefully.
- doc.go in wsstream package describes every version of binary WebSockets RemoteCommand protocols.
Adds unit tests for new WebSocket client functionality
- Increases tools/remotecommand test coverage from 53.8% to 75.5%.
- File k8s.io/client-go/tools/remotecommmand/websocket.go has test coverage: 88.3%

  ok  	k8s.io/client-go/tools/remotecommand	coverage: 75.5% of statements
  ok  	k8s.io/client-go/transport/websocket	coverage: 71.4% of statements

Based on previous @ash2k PR: [WIP] Kubectl exec/attach/cp over WebSocket #110142

Example stress test for WebSocket client testing STDIN and STDOUT stream

This loopback test sends 1MB of random data onto the STDIN stream copying the data back to the client onto the STDOUT stream.

$ stress ./remotecommand.test -test.run TestWebSocketClient_LoopbackStdinToStdout
5s: 4964 runs so far, 0 failures
10s: 9952 runs so far, 0 failures
15s: 14929 runs so far, 0 failures
20s: 19879 runs so far, 0 failures
25s: 24866 runs so far, 0 failures
30s: 29862 runs so far, 0 failures
35s: 34858 runs so far, 0 failures
40s: 39869 runs so far, 0 failures
45s: 44853 runs so far, 0 failures
50s: 49852 runs so far, 0 failures
55s: 54858 runs so far, 0 failures
1m0s: 59861 runs so far, 0 failures

Example stress test for WebSocket client sending multiple write streams concurrently (STDIN, TTY Resize)

$ stress ./remotecommand.test -test.run TestWebSocketClient_MultipleWriteChannels
5s: 675 runs so far, 0 failures                                                                                                                                                                        
10s: 1351 runs so far, 0 failures                                                                                                                                                                      
15s: 2057 runs so far, 0 failures                                                                                                                                                                      
20s: 2742 runs so far, 0 failures                                                                                                                                                                      
25s: 3426 runs so far, 0 failures                                                                                                                                                                      
30s: 4126 runs so far, 0 failures                                                                                                                                                                      
35s: 4836 runs so far, 0 failures                                                                                                                                                                      
40s: 5534 runs so far, 0 failures                                                                                                                                                                      
45s: 6225 runs so far, 0 failures                                                                                                                                                                      
50s: 6917 runs so far, 0 failures                                                                                                                                                                      
55s: 7612 runs so far, 0 failures                                                                                                                                                                      
1m0s: 8300 runs so far, 0 failures                                                                                                                                                                     
1m5s: 9006 runs so far, 0 failures                                                                                                                                                                     
1m10s: 9681 runs so far, 0 failures                                                                                                                                                                    
1m15s: 10368 runs so far, 0 failures

/kind feature

Helps fix: #89163

NONE

KEP Transition from SPDY to WebSockets - 4006

NOTE: WebSocket client is implemented with the Gorilla WebSockets library

The other two possibilities are:
1. golang stdlib websocket - which is used in the current WebSocket server code
2. https://pkg.go.dev/nhooyr.io/websocket
All three libraries are very mature.
The Gorilla WebSockets library was used for the client because:
1. It has far better support for client creation. The gorilla library client creation functions fit well with Kubernetes use of RoundTripper, because they return an http response which is part of the RoundTripper interface when other libraries do not.
2. It has far better support for data messages other than binary (e.g. heartbeat ping/pong).
3. Gorilla has direct support for proxies in the client, while the golang stdlib websockets does not.
4. Until recently, the golang stdlib websocket library suggested using the Gorilla library as more feature rich.

seans3 · 2023-07-07T17:30:15Z

/sig api-machinery

seans3 · 2023-07-07T17:30:30Z

/priority important-soon

seans3 · 2023-07-07T17:39:06Z

/cc @aojea
/cc @ardaguclu

seans3 · 2023-07-07T18:50:30Z

/retest

seans3 · 2023-07-07T18:58:04Z

/retest

seans3 · 2023-07-07T19:36:41Z

/cc @jpbetz

aojea · 2023-09-01T22:29:35Z

LGTM

some questions that. may be follow ups if they make sense

aojea · 2023-09-02T20:28:13Z

/lgtm

k8s-ci-robot · 2023-09-02T20:28:20Z

LGTM label has been added.

Git tree hash: e63541a9b1e44c41bd81732a42f56e4a220c19c1

liggitt

/lgtm
/approve
/hold for one question about the websocket impl matching the spdy impl lack of thread safety

liggitt · 2023-09-05T17:13:54Z

staging/src/k8s.io/client-go/tools/remotecommand/websocket.go

+}
+
+// NewWebSocketExecutor allows to execute commands via a WebSocket connection.
+func NewWebSocketExecutor(config *restclient.Config, method, url string) (Executor, error) {


the returned executor's Stream / StreamWithContext methods are not threadsafe, right? I think the spdy executor has the same issue (due to the coupled upgrader storing state during the connection setup) but can you verify that? would be good to explicitly document that here and on the spdy constructor, but it's fine if that goes into a follow up

Yes. You are correct that neither the SPDY nor WebSocket Stream/StreamWithContext are thread-safe for a single executor. As you mention, the UpgradeRoundTripper for both store state (the dialed/created connection). SPDY already has some documentation related to this when defining the SpdyRoundTripper struct. Above the connection field is the following comment

type SpdyRoundTripper struct { ... /* TODO according to http://golang.org/pkg/net/http/#RoundTripper, a RoundTripper must be safe for use by multiple concurrent goroutines. If this is absolutely necessary, we could keep a map from http.Request to net.Conn. In practice, a client will create an http.Client, set the transport to a new insteace of SpdyRoundTripper, and use it a single time, so this hopefully won't be an issue. */ // conn is the underlying network connection to the remote server. conn net.Conn

I have added documentation about the lack of thread safety to WebSocket.Stream/StreamWithContext

seans3 · 2023-09-05T19:46:30Z

/retest

unrelated required test failure

aojea · 2023-09-05T20:26:38Z

/lgtm

k8s-ci-robot · 2023-09-05T20:26:45Z

LGTM label has been added.

Git tree hash: 4cf19be17ea27119104412468df71489446462a9

aojea · 2023-09-05T20:28:24Z

/lgtm /approve /hold for one question about the websocket impl matching the spdy impl lack of thread safety

@liggitt it is on hold and the approve label was not added

liggitt · 2023-09-05T20:36:00Z

/hold cancel
/lgtm
/approve

k8s-ci-robot · 2023-09-05T20:36:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, seans3

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:

~~hack/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apimachinery/pkg/util/OWNERS~~ [liggitt]
~~staging/src/k8s.io/cli-runtime/OWNERS~~ [liggitt,seans3]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~staging/src/k8s.io/component-base/OWNERS~~ [liggitt]
~~staging/src/k8s.io/component-helpers/OWNERS~~ [liggitt]
~~staging/src/k8s.io/dynamic-resource-allocation/OWNERS~~ [liggitt]
~~staging/src/k8s.io/endpointslice/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kms/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kubectl/OWNERS~~ [liggitt,seans3]
~~staging/src/k8s.io/kubelet/OWNERS~~ [liggitt]
~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [liggitt]
~~staging/src/k8s.io/metrics/OWNERS~~ [liggitt]
~~staging/src/k8s.io/sample-cli-plugin/OWNERS~~ [liggitt,seans3]
~~staging/src/k8s.io/sample-controller/OWNERS~~ [liggitt]
~~vendor/OWNERS~~ [liggitt]

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 added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 7, 2023

k8s-ci-robot requested review from bart0sh and cheftako July 7, 2023 17:31

k8s-ci-robot requested review from aojea and ardaguclu July 7, 2023 17:39

seans3 force-pushed the websocket-executor branch from ae80a42 to ec34bf5 Compare July 7, 2023 18:28

k8s-ci-robot assigned aojea Sep 2, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2023

liggitt reviewed Sep 5, 2023

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 Sep 5, 2023

WebSocket Client and V5 RemoteCommand Subprotocol

a0d6a81

seans3 force-pushed the websocket-executor branch from 29bac76 to a0d6a81 Compare September 5, 2023 18:37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2023

k8s-ci-robot requested review from aojea, ardaguclu and liggitt September 5, 2023 18:37

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2023

k8s-ci-robot merged commit 6013381 into kubernetes:master Sep 5, 2023
17 of 18 checks passed

SIG Node PR Triage automation moved this from Needs Reviewer to Done Sep 5, 2023

k8s-ci-robot added this to the v1.29 milestone Sep 5, 2023

tigrato mentioned this pull request Sep 6, 2023

Support for Kubernetes Websocket V5 protocol gravitational/teleport#15474

Closed

pacoxu mentioned this pull request Sep 7, 2023

Track vendored status of unwanted deps, add more cloud dependencies #120374

Merged

pacoxu mentioned this pull request Sep 15, 2023

TestWebSocketClient_HeartbeatSucceeds ut flakes（also shown as remotecommand） #120684

Closed

jacobtomlinson mentioned this pull request Nov 28, 2023

Pod.exec stdin never closes kr8s-org/kr8s#212

Open

saschagrunert mentioned this pull request Dec 11, 2023

Support v5 websocket protocol in cri/streaming #122263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebSocket Client and V5 RemoteCommand Subprotocol #119157

WebSocket Client and V5 RemoteCommand Subprotocol #119157

seans3 commented Jul 7, 2023 •

edited

Loading

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

aojea commented Sep 1, 2023

aojea commented Sep 2, 2023

k8s-ci-robot commented Sep 2, 2023

liggitt left a comment

liggitt Sep 5, 2023

seans3 Sep 5, 2023 •

edited

Loading

seans3 commented Sep 5, 2023 •

edited

Loading

aojea commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

aojea commented Sep 5, 2023

liggitt commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

WebSocket Client and V5 RemoteCommand Subprotocol #119157

WebSocket Client and V5 RemoteCommand Subprotocol #119157

Conversation

seans3 commented Jul 7, 2023 • edited Loading

Example stress test for WebSocket client testing STDIN and STDOUT stream

Example stress test for WebSocket client sending multiple write streams concurrently (STDIN, TTY Resize)

NOTE: WebSocket client is implemented with the Gorilla WebSockets library

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

seans3 commented Jul 7, 2023

aojea commented Sep 1, 2023

aojea commented Sep 2, 2023

k8s-ci-robot commented Sep 2, 2023

liggitt left a comment

Choose a reason for hiding this comment

liggitt Sep 5, 2023

Choose a reason for hiding this comment

seans3 Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

seans3 commented Sep 5, 2023 • edited Loading

aojea commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

aojea commented Sep 5, 2023

liggitt commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

seans3 commented Jul 7, 2023 •

edited

Loading

seans3 Sep 5, 2023 •

edited

Loading

seans3 commented Sep 5, 2023 •

edited

Loading