New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KEP-3396: WebSocket protocol enhancements #3401

Closed

ash2k wants to merge 4 commits into kubernetes:master from ash2k:3396-websocket-kep

Member

ash2k commented Jun 16, 2022 •

edited

Loading

One-line PR description: this is a KEP to enhance WebSocket protocol in API server and migrate some kubectl commands to WebSocket from SPDY.

Issue link: WebSocket protocol enhancements #3396

Other comments: it is my first KEP and I need help to flesh it out.

k8s-ci-robot added cncf-cla: yes size/XL labels

Contributor

k8s-ci-robot commented Jun 16, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ash2k
To complete the pull request process, please assign fedebongio after the PR has been reviewed.
You can assign the PR to them by writing /assign @fedebongio in a comment when ready.

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

Needs approval from an approver in each of these files:

keps/sig-api-machinery/OWNERS

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 the kind/kep label

k8s-ci-robot requested review from apelisse and lavalamp

June 16, 2022 05:38

k8s-ci-robot added the sig/api-machinery label

ash2k mentioned this pull request

WebSocket protocol enhancements #3396

Closed

4 tasks

ash2k force-pushed the 3396-websocket-kep branch from 085c930 to eb6dfb6 Compare

June 16, 2022 05:52

ash2k marked this pull request as draft

June 16, 2022 06:08

k8s-ci-robot added the do-not-merge/work-in-progress label

This was referenced Jun 16, 2022

exec over web sockets protocol is flawed kubernetes/kubernetes#89899

Closed

Support kubectl exec/attach using Websocket kubernetes/kubernetes#89163

Closed

ash2k commented

View reviewed changes

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md Outdated Show resolved Hide resolved


          WebSocket protocol enhancements kep

5424a6a

ash2k force-pushed the 3396-websocket-kep branch from eb6dfb6 to 5424a6a Compare

June 16, 2022 12:24

ash2k marked this pull request as ready for review

June 17, 2022 02:36

k8s-ci-robot removed the do-not-merge/work-in-progress label

k8s-ci-robot requested a review from fedebongio

June 17, 2022 02:36

sftim reviewed

View reviewed changes

Contributor

sftim left a comment

An informal review - hope it helps.

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md Outdated

+              The "Design Details" section below is for the real
+              nitty-gritty.
+              -->

Contributor

sftim Jun 22, 2022

Suggested change

      
            - Update the WebSocket support in the API server. Support a v5 framing protocol on top of WebSocket.

?

Member Author

ash2k Jul 4, 2022

My understanding is that this section describes what changes this KEP is proposing. What you are suggesting is HOW to implement those changes, so I think it doesn't need to be here 🤷

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md

Comment on lines +328 to +333

+              An alternative is to try to establish `v5.channel.k8s.io` and fall back to `SPDY/3.1` but that is 2x
+              the number of requests when newer client is used with an older server. Perhaps this is acceptable
+              since `exec`, `attach`, and `cp` are I/O-heavy anyway and are not used all the time. This is a temporary
+              overhead, eventually this situation will become less and less frequent as servers upgrade to a version with
+              the new protocol. If the overhead is not acceptable, user can almost always use a client of a matching version.
+              At some point SPDY support should be removed and this problem will go away completely.

Contributor

sftim Jun 22, 2022

Consider moving this under ## Alternatives, and replace the moved text with a link to it.

Member Author

ash2k Jul 4, 2022

This is an alternative to the protocol negotiation section only, not to the whole proposal. I think it will be out of context if move to another section.

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md

Comment on lines +316 to +326

+              If we have a new protocol, then older servers will not have support for it. In that case a newer client will try
+              v5 but an older server will not accept that. To be backwards compatible, ideally client should use the intended
+              mechanism that HTTP `Upgrade` provides - send a list of protocols. I.e. client would send a header with
+              something like `Upgrade: v5.channel.k8s.io, SPDY/3.1` and server would pick a protocol it supports,
+              taking into account client's preference (the list is ordered).
+              The difficulty with the above is that both Gorilla WebSocket and SPDY libraries are not built to allow to
+              use them this way. Both encapsulate negotiation, and you cannot compose them to get the above behavior.
+              Refactoring them is not an option since SPDY is dead and nobody will spend time on the library and Gorilla
+              WebSocket is [looking for maintainers](https://github.com/gorilla/websocket/issues/370) and hence
+              a PR is unlikely to get merged quickly even if at all (since this is a big API addition/change).

Contributor

sftim Jun 22, 2022

Perhaps the Kubernetes API could provide a way for the cluster to expose what protocols it can negotiate?

Eg, GET /api/v1alpha1/protocol-availability (or something similar)

Member Author

ash2k Jul 4, 2022

This wouldn't reduce the number of API calls compared to what is being suggesting in this PR right now. The current suggestion will do 1 more call temporarily, and only when client is newer than the server. Eventually old clients will come out of use and clients will not be affected anymore.

keps/sig-api-machinery/3396-websocket-protocol-enhancements/README.md Outdated

Comment on lines 857 to 861

+              <!--
+              What other approaches did you consider, and why did you rule them out? These do
+              not need to be as detailed as the proposal, but should include enough
+              information to express the idea and why it was not acceptable.
+              -->

Contributor

sftim Jun 22, 2022

One alternative I'd hope to see listed is WebRTC data channels. We should document what this might look like and briefly state why this isn't a good choice (it's really complicated, for a start).

Member Author

ash2k Jul 4, 2022

:) There are probably a few protocols that can mux multiple streams on top of TCP. We don't need to list them all. I only want to solve the problem with a small organic change, don't want to rework how Kubernetes does streaming (not a goal of this KEP). Kubernetes already has WebSocket support, so that's what I want to use, just an improved version.

Member Author

ash2k Jul 4, 2022

Added this explicitly as not a goal.

Contributor

sftim commented Jun 22, 2022

BTW @ash2k the PR description still says that this is a draft - is that right?

sftim mentioned this pull request

Generic port forwarding #1846

Closed

ash2k mentioned this pull request

[WIP] Kubectl exec/attach/cp over WebSocket kubernetes/kubernetes#110142

Closed

ash2k added 2 commits

July 4, 2022 10:59


          Remove comments

25703ec


          Add port-forward

c2b7721

k8s-ci-robot added size/L and removed size/XL labels


          Move milestone

0d2c98a

tigrato mentioned this pull request

Support for Kubernetes Websocket V5 protocol gravitational/teleport#15474

Closed

k8s-triage-robot commented Oct 3, 2022

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

This bot triages issues and 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 issue is closed

You can:

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

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

/lifecycle stale

k8s-ci-robot added the lifecycle/stale label

k8s-triage-robot commented Nov 2, 2022

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

This bot triages issues and 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 issue is closed

You can:

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

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

/lifecycle rotten

k8s-ci-robot added lifecycle/rotten and removed lifecycle/stale labels

k8s-triage-robot commented Dec 2, 2022

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and 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:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot closed this

Contributor

k8s-ci-robot commented Dec 2, 2022

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and 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:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ash2k mentioned this pull request

kubectl cp fails on large files kubernetes/kubernetes#60140

Open

seans3 mentioned this pull request

[WIP] kubectl exec/cp/attach over WebSocket kubernetes/kubernetes#116778

Closed

GreenCappuccino mentioned this pull request

SPDY is deprecated. Switch to HTTP/2. kubernetes/kubernetes#7452

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sftim sftim left review comments

mikedanese mikedanese left review comments

apelisse Awaiting requested review from apelisse

lavalamp Awaiting requested review from lavalamp

fedebongio Awaiting requested review from fedebongio

Labels

cncf-cla: yes kind/kep lifecycle/rotten sig/api-machinery size/L