support websocket for exec and attach #1305

duguhaotian · 2023-12-05T11:36:12Z

What type of PR is this?

Add one of the following kinds:
/kind feature

Optionally add one or more of the following kinds if applicable:
/kind api-change

What this PR does / why we need it:

Which issue(s) this PR fixes:

#1296

Special notes for your reviewer:

Does this PR introduce a user-facing change?

add new option for exec/attach, manual is: crictl exec/attach -w -it ${container-id} ls

test

i use container engine is iSulad which implement stream server is websocket.

$ export CRICTL_REMOTE_COMMAND_WEBSOCKETS=true
$ mycrictl exec -w -it b277916602c7 ls
bin   dev   etc   home  proc  root  sys   tmp   usr   var

# tcpdump logs
08:53:15.674128 IP localhost.10350 > localhost.40462: Flags [.], ack 303, win 510, options [nop,nop,TS val 1395824017 ecr 1395824017], length 0
E..4o.@.@..,........(n...       ..T=.......(.....
S2..S2..
08:53:15.675140 IP localhost.10350 > localhost.40462: Flags [P.], seq 1:173, ack 303, win 512, options [nop,nop,TS val 1395824018 ecr 1395824017], length 172
E...o.@.@...........(n...       ..T=.............
S2..S2..HTTP/1.1 101 Switching Protocols
Upgrade: WebSocket
Connection: Upgrade
Sec-WebSocket-Accept: mvvvKmgjEhY8040cH4xt8YSYr+Y=
Sec-WebSocket-Protocol: v5.channel.k8s.io


08:53:15.675156 IP localhost.40462 > localhost.10350: Flags [.], ack 173, win 511, options [nop,nop,TS val 1395824018 ecr 1395824018], length 0
E..4|w@.@..J..........(nT=...   .......(.....
S2..S2..
08:53:15.675527 IP localhost.40462 > localhost.10350: Flags [P.], seq 303:336, ack 173, win 512, options [nop,nop,TS val 1395824018 ecr 1395824018], length 33
E..U|x@.@..(..........(nT=...   .......I.....
S2..S2....S..mW..::...q..^g..%6...'..^c..
08:53:15.716065 IP localhost.10350 > localhost.40462: Flags [.], ack 336, win 512, options [nop,nop,TS val 1395824059 ecr 1395824018], length 0
E..4o.@.@..*........(n...       ..T=.......(.....
S2..S2..
08:53:15.755820 IP localhost.10350 > localhost.40462: Flags [P.], seq 173:337, ack 336, win 512, options [nop,nop,TS val 1395824099 ecr 1395824018], length 164
E...o.@.@...........(n...       ..T=.............
S2..S2...~....[1;34mbin.[m   .[1;34mdev.[m   .[1;34metc.[m   .[1;34mhome.[m  .[1;34mproc.[m  .[1;34mroot.[m  .[1;34msys.[m   .[1;34mtmp.[m   .[1;34musr.[m   .[1;34mvar.[m

08:53:15.796061 IP localhost.40462 > localhost.10350: Flags [.], ack 337, win 512, options [nop,nop,TS val 1395824139 ecr 1395824099], length 0
E..4|y@.@..H..........(nT=...   .2.....(.....
S2..S2..
08:53:15.856882 IP localhost.10350 > localhost.40462: Flags [P.], seq 337:360, ack 336, win 512, options [nop,nop,TS val 1395824200 ecr 1395824139], length 23
E..Ko.@.@...........(n...       .2T=.......?.....
S2.HS2.....{"status":"Success"}
08:53:15.856891 IP localhost.40462 > localhost.10350: Flags [.], ack 360, win 512, options [nop,nop,TS val 1395824200 ecr 1395824200], length 0

k8s-ci-robot · 2023-12-05T11:36:22Z

Welcome @duguhaotian!

It looks like this is your first PR to kubernetes-sigs/cri-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cri-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

SergeyKanzhelev · 2023-12-06T20:39:17Z

cmd/crictl/exec.go

+		if err != nil {
+			return err
+		}
+		executor, err = remoteclient.NewFallbackExecutor(websocketExecutor, executor, httpstream.IsUpgradeFailure)


is it the intention of the crictl? Should we just use websocket executor if requested without the fallback? It may be better for the end user to be able to explicitly ask for websocket.

SergeyKanzhelev · 2023-12-06T20:40:07Z

cmd/crictl/attach.go

+			id:        id,
+			tty:       c.Bool("tty"),
+			stdin:     c.Bool("stdin"),
+			websocket: c.Bool("websocket"),


would the name use-websocket be better? Or maybe have something with three options: -- transport=spdy|websocket|default where default will do websocket with the fallback to spdy?

duguhaotian · 2023-12-07T09:36:20Z

i donot know why check of runc and cri-o critest failed?

the report error is spdy upgrade failed? why?

saschagrunert · 2023-12-07T10:13:53Z

pkg/validate/streaming.go

+	we, err := remoteclient.NewWebSocketExecutor(&rest.Config{TLSClientConfig: rest.TLSClientConfig{Insecure: true}}, "GET", url.String())
+	framework.ExpectNoError(err, "failed to create websocket executor for %q", execServerURL)
+
+	e, err = remoteclient.NewFallbackExecutor(we, e, httpstream.IsUpgradeFailure)
+	framework.ExpectNoError(err, "failed to create fallback executor for %q", execServerURL)
+


This is making the tests fail in the CI. Do runtimes have to support that?

If websocket failed, will fallback to spdy that is same to before. Why make ci failed?
I think that IsUpgradeFailure will ignore error of websocket upgrade

and error : unable to upgrade connection: 404 page not found is reported by spdy(vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go#NewConnection)

just remove this first, wait runtimes support it

saschagrunert

How can we test this? Using CRI-O main always returns an error on exec:

sudo ./build/bin/linux/amd64/crictl exec -p websocket 20168cc1a0aefa251fed echo hi
FATA[0000] execing command in container: unable to upgrade streaming request: websocket: bad handshake

CRI-O logs:

ERRO[2023-12-08 09:28:57.322412994+01:00] unable to upgrade websocket connection: websocket server finished before becoming ready

The error is coming from:

https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/vendor/k8s.io/apimachinery/pkg/util/httpstream/wsstream/conn.go#L223-L225

We're vendoring Kubernetes v1.29.0-rc.1 right now, so I'm wondering which part is missing what 🤔

saschagrunert · 2023-12-08T08:24:47Z

cmd/crictl/exec.go

+			Usage:   "transport protocol, One of: spdy|websocket|default",
+			Value:   "default",


I think we should keep spdy as default value and remove default.

duguhaotian · 2023-12-08T10:49:54Z

How can we test this? Using CRI-O main always returns an error on exec:
sudo ./build/bin/linux/amd64/crictl exec -p websocket 20168cc1a0aefa251fed echo hi
FATA[0000] execing command in container: unable to upgrade streaming request: websocket: bad handshake
CRI-O logs:
ERRO[2023-12-08 09:28:57.322412994+01:00] unable to upgrade websocket connection: websocket server finished before becoming ready
The error is coming from:

https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/vendor/k8s.io/apimachinery/pkg/util/httpstream/wsstream/conn.go#L223-L225

We're vendoring Kubernetes v1.29.0-rc.1 right now, so I'm wondering which part is missing what 🤔

I think CRI-O need update code of websocket, like support 'v5.channel.k8s.io'

saschagrunert · 2023-12-08T11:04:31Z

I think CRI-O need update code of websocket, like support 'v5.channel.k8s.io'

I may miss something here, but we use the code vendored from Kubernetes: https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/go.mod#L75-L80

duguhaotian · 2023-12-11T01:28:03Z

I think CRI-O need update code of websocket, like support 'v5.channel.k8s.io'

I may miss something here, but we use the code vendored from Kubernetes: https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/go.mod#L75-L80

can we get more logging of cri-o ?

why http server failed? code:
https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/vendor/k8s.io/apimachinery/pkg/util/httpstream/wsstream/conn.go#L214C3-L214C3

saschagrunert · 2023-12-11T08:11:38Z

@duguhaotian it returns there: https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/vendor/golang.org/x/net/websocket/server.go#L84

With the error message:

requested protocol(s) are not supported: [v5.channel.k8s.io]; supports [ channel.k8s.io base64.channel.k8s.io v4.channel.k8s.io v4.base64.channel.k8s.io]

@duguhaotian is it intentional that runtimes only support v1-v4? Ref: https://github.com/kubernetes/client-go/blob/84a6fe7e4032ae1b8bc03b5208e771c5f7103549/tools/remotecommand/websocket.go#L89-L92

duguhaotian · 2023-12-11T10:50:36Z

@duguhaotian it returns there: https://github.com/cri-o/cri-o/blob/f40da54704c5c32fb24b2b0e204424afae5b0dc2/vendor/golang.org/x/net/websocket/server.go#L84

With the error message:
requested protocol(s) are not supported: [v5.channel.k8s.io]; supports [ channel.k8s.io base64.channel.k8s.io v4.channel.k8s.io v4.base64.channel.k8s.io]
@duguhaotian is it intentional that runtimes only support v1-v4? Ref: https://github.com/kubernetes/client-go/blob/84a6fe7e4032ae1b8bc03b5208e771c5f7103549/tools/remotecommand/websocket.go#L89-L92

cri-o not support v5.channel.k8s.io;
i think runtimes just haven't done a good job of supporting v5.channel.k8s.io, and runtimes should support it.

saschagrunert · 2023-12-11T11:10:34Z

cmd/crictl/exec.go

+	case "spdy":
+		exec, err = remoteclient.NewSPDYExecutor(config, "POST", url)
+	case "websocket":
+		exec, err = remoteclient.NewWebSocketExecutor(config, "GET", url.String())


Can we use remoteclient.NewWebSocketExecutorForProtocols and make the protocols CLI configurable?

Ref: kubernetes/kubernetes#122263 (comment)

ofcourse, default use StreamProtocolV5Name?

@duguhaotian yeah, this way we'll not require to flip the default again in the future once kubernetes/kubernetes#122263 got resolved.

change option to crictl --websocket="x..channel.k8s.io",

if not set --websocket, will use spdy;

if websocket failed, fallback to spdy;

@duguhaotian yeah, this way we'll not require to flip the default again in the future once kubernetes/kubernetes#122263 got resolved.

fixed

aojea · 2023-12-11T12:17:46Z

There is a KEP and a plane to gradually move on to SPDY https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4006-transition-spdy-to-websockets , we should coordinate, @seans3 is leading this effort and if we implement different bits at different times we'll risk to drift

k8s-ci-robot · 2023-12-15T02:23:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: duguhaotian
Once this PR has been reviewed and has the lgtm label, please ask for approval from saschagrunert. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

aojea · 2023-12-16T16:47:33Z

cmd/crictl/attach.go

+			Usage:   "protocol of transport, One of: spdy|websocket",
+			Value:   "spdy",
+		},
+		&cli.BoolFlag{


why a flag and not using fall back , as we are doing with kubectl and is defined in the KEP?

flags are hard to remove in the future and become APIs for users, I also prefer an environment variable instead

thk you reply. fixed

aojea · 2023-12-16T16:51:12Z

I think this should follow the same flow that the kubectl implementation designed in the KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4006-transition-spdy-to-websockets#proposal-kubectl-websocket-executor-and-fallback-executor

use the legacy by default and have an environment variable to opt in to websockets by default but fallback to the legacy https://github.com/kubernetes/kubernetes/pull/119186/files#diff-62da7750fb41c9d53934dd9706ba719f0c0efdf931b6b36eb46498505e4d3520

Signed-off-by: haozi007 <duguhaotian@gmail.com>

duguhaotian · 2023-12-17T06:51:57Z

I think this should follow the same flow that the kubectl implementation designed in the KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4006-transition-spdy-to-websockets#proposal-kubectl-websocket-executor-and-fallback-executor

use the legacy by default and have an environment variable to opt in to websockets by default but fallback to the legacy https://github.com/kubernetes/kubernetes/pull/119186/files#diff-62da7750fb41c9d53934dd9706ba719f0c0efdf931b6b36eb46498505e4d3520

I gree with you, my first version which use fallback executor.
But, current some runtime(like crio) do not support v5, so will cause CI testcase failed.

aojea · 2023-12-17T10:27:46Z

But, current some runtime(like crio) do not support v5, so will cause CI testcase failed.

Please check more in detail how this is implemented in Kubernetes, I linked the corresponding code and KEP for context

v5 will be only used when a special env variable is set (feature flag here is an env variable)
v5 will fallback to v4 if it fails because the container runtime does not implement it, so is not possible CI fails because the fallback has to work, otherwise is a bug in the implementation
if no env variable is set, no new code path is executed

k8s-triage-robot · 2024-03-16T11:26:56Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

saschagrunert · 2024-03-20T14:17:25Z

I think we could at least support the websocket executor in crictl for users to play around with the feature and runtimes.

saschagrunert · 2024-03-21T07:30:30Z

cmd/crictl/util.go

+func IsEnabledWebsockets() bool {
+	return strings.ToLower(os.Getenv("CRICTL_REMOTE_COMMAND_WEBSOCKETS")) == "true"
+}


Let's remove this

saschagrunert · 2024-03-21T07:30:37Z

cmd/crictl/exec.go

+	if !IsEnabledWebsockets() {
+		return
+	}


Suggested change

if !IsEnabledWebsockets() {

return

}

saschagrunert · 2024-03-21T07:31:10Z

cmd/crictl/exec.go

@@ -163,8 +164,27 @@ func Exec(ctx context.Context, client internalapi.RuntimeService, opts execOptio
 	return stream(ctx, opts.stdin, opts.tty, URL)
 }

+func getExecutor(url *url.URL) (exec remoteclient.Executor, err error) {


I'd say we reintroduce the flag to select the executor.

This allows end users to choose the transport to be used as well as runtime developers to experiment on feature development. Supersedes: kubernetes-sigs#1305 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert · 2024-03-21T21:20:00Z

Thank you for the effort on this topic @duguhaotian, we really appreciate it. 🙏

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 5, 2023

k8s-ci-robot requested review from Random-Liu and yujuhong December 5, 2023 11:36

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 5, 2023

duguhaotian force-pushed the master branch 2 times, most recently from e047447 to 181cf6e Compare December 6, 2023 02:47

SergeyKanzhelev reviewed Dec 6, 2023

View reviewed changes

duguhaotian force-pushed the master branch from 181cf6e to 4217f85 Compare December 7, 2023 01:50

duguhaotian requested a review from SergeyKanzhelev December 7, 2023 02:05

duguhaotian force-pushed the master branch from 4217f85 to 27b3d4e Compare December 7, 2023 09:35

saschagrunert reviewed Dec 7, 2023

View reviewed changes

duguhaotian requested a review from saschagrunert December 7, 2023 11:22

duguhaotian force-pushed the master branch from 27b3d4e to 12ba3d2 Compare December 8, 2023 01:39

saschagrunert reviewed Dec 8, 2023

View reviewed changes

duguhaotian force-pushed the master branch from 12ba3d2 to d4b9408 Compare December 8, 2023 10:53

duguhaotian requested a review from saschagrunert December 8, 2023 10:55

saschagrunert requested changes Dec 11, 2023

View reviewed changes

k8s-ci-robot assigned saschagrunert Dec 11, 2023

saschagrunert mentioned this pull request Dec 11, 2023

Support v5 websocket protocol in cri/streaming kubernetes/kubernetes#122263

Closed

duguhaotian force-pushed the master branch from d4b9408 to 81d8f2f Compare December 15, 2023 02:22

duguhaotian requested a review from saschagrunert December 15, 2023 02:29

aojea reviewed Dec 16, 2023

View reviewed changes

support websocket for exec and attach

603fde3

Signed-off-by: haozi007 <duguhaotian@gmail.com>

duguhaotian force-pushed the master branch from 81d8f2f to 603fde3 Compare December 17, 2023 06:46

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2023

duguhaotian requested a review from aojea December 17, 2023 06:52

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 16, 2024

saschagrunert requested changes Mar 21, 2024

View reviewed changes

saschagrunert mentioned this pull request Mar 21, 2024

Support websocket transport for exec and attach #1383

Merged

saschagrunert closed this Mar 21, 2024

mikebrow mentioned this pull request Aug 9, 2024

kubectl >= 1.30.0 triggers leak of goroutines in containerd on kubectl exec kubernetes/kubernetes#126608

Closed

		Usage: "transport protocol, One of: spdy\|websocket\|default",
		Value: "default",

support websocket for exec and attach #1305

support websocket for exec and attach #1305

Conversation

duguhaotian commented Dec 5, 2023 • edited Loading

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?

test

k8s-ci-robot commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duguhaotian commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duguhaotian Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duguhaotian commented Dec 8, 2023

saschagrunert commented Dec 8, 2023

duguhaotian commented Dec 11, 2023 • edited Loading

saschagrunert commented Dec 11, 2023 • edited Loading

duguhaotian commented Dec 11, 2023

saschagrunert Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

duguhaotian Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Dec 11, 2023

k8s-ci-robot commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Dec 16, 2023

duguhaotian commented Dec 17, 2023

aojea commented Dec 17, 2023

k8s-triage-robot commented Mar 16, 2024

saschagrunert commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert commented Mar 21, 2024

duguhaotian commented Dec 5, 2023 •

edited

Loading

duguhaotian Dec 7, 2023 •

edited

Loading

duguhaotian commented Dec 11, 2023 •

edited

Loading

saschagrunert commented Dec 11, 2023 •

edited

Loading

saschagrunert Dec 11, 2023 •

edited

Loading

saschagrunert Dec 11, 2023 •

edited

Loading

duguhaotian Dec 11, 2023 •

edited

Loading