KEP-4006: Initial iteration of provisional WebSockets KEP

[seans3]

• Initial alpha KEP for transitioning from SPDY to WebSockets

• Issue link: Transition from SPDY to WebSockets #4006

• Implements all alpha sections including

  • Summary
  • Motivation
  • Proposal
  • Design Details
  • Test Plan
  • Production Readiness Review Questionnaire

• Adds initial reviewers and approvers

[seans3]

/cc @jpbetz
/cc @deads2k
/assign @aojea
/assign @ardaguclu

[aojea]

This is required to solve the existing gap in the implementation with half-close stdin, isn't it?
kubernetes/kubernetes#89899

[seans3]

Yes--I believe the half-close stdin for WebSockets is a protocol increment. But I believe there may be a use of the v5 subprotocol version which may be more important, and I need your feedback on this.

There are several other WebSocket clients currently communicating with the API Server: 1) the OpenShift browser/javascript client, and 2) the Kubelet using WebSockets for watches, etc. So an important question becomes, how do we distinguish between these legacy WebSocket use cases, and the new kubectl RemoteCommand use cases which will need the new StreamTranslatorProxy ? In our current proof-of-concept PR, we substituted the new StreamTranslatorProxy instead of the UpgradeAwareProxy when handling the exec subresource. I believe this is causing a lot of the e2e test failures. I believe we should integrate these two proxies as follows

<Within tryUpgrade of UpgradeAwareProxy, examining HTTP Request>
if <Upgrade == WebSockets && version == v5.channel.k8s.io && WEBSOCKETS_FEATURE_FLAG >
  delegate request to StreamTranslatorProxy
else
  <continue with legacy code in UpgradeAwareProxy>

Importantly, this allows us to only target the kubectl RemoteCommand for the StreamTranslatorProxy, and the other current WebSocket use cases (as well as other SPDY use cases) are unaffected.

What do you think?

[ardaguclu]

Since we are changing bi-directional protocol, I agree that new subprotocol (v5.channel.k8s.io )would be better for the sake of maintainability. It also serves managing the fallback mechanism as said above.

[aojea]

we should have a more meaningful name, but I'm bad for names KubectlWebsockets or ClientWebsockets ??

[jpbetz]

+1. If there is going to be a separate KEP for kubectl port-forward then maybe something like ClientSubcommandWebsockets?

EDIT: Or if we do include port-forward (I've opened a separate comment thread on that), maybe WebsocketClientChannels or something similar?

[seans3]

Added ClientRemoteCommandWebsockets.

[aojea]

an interesting metrics will be to expose the number of failed connections and fallbacks, i.e., how many times the upgrade to websockets has to fall back

[seans3]

This is not currently possible in the apiserver, since each request is separate. There is currently no way to know that the second, fallback SPDY request is connected to a first WebSockets request. We can have metrics like num_websocket_requests and num_websocket_failures.

[aojea]

This is http2 does not expose any streaming capabilities so we should have to implement again all the streaming protocols and maintain our low level http2 library

[seans3]

Yes. I just put a placeholder to remind myself to flesh this out in a future PR.

[aojea]

These are going to be important since those can allow us to exercise the fallback mechanism

[seans3]

Agreed. I intend to submit separate PRs to outline the testing plan as well as the Production Readiness questions within the KEP. Submitting this initial iteration of the provisional KEP will allow others to more easily contribute to unimplemented sections.

[seans3]

@aojea @ardaguclu @jpbetz Hoping to get feedback on the components requiring a feature flag.

1. Do we need feature flags for both the API Server and kubectl ?
2. Can we get away with just a feature flag on kubectl ? The code paths in the API Server (StreamTranslatorProxy) should not be reachable without the new kubectl requesting v5 of RemoteCommand subprotocol.
3. If we have feature flags for both components, can they be the same name ?

[ardaguclu]

I think, v5.channel.k8s.io subprotocol is already a some kind of a feature flag in API server. Everything will work as is, as long as clients send requests in current subprotocols(v4..v1) and v5.channel.k8s.io subprotocol will be used in kubectl, only when it's feature flag is enabled.

[aojea]

summoning @liggitt , he is a natural for these things ;)

[seans3]

Added feature flags for both kubectl and the API Server for this alpha KEP. If we change it, I will update the KEP.

[jpbetz]

per-binary flags for wherever this feature is implemented sounds right to me. I don't have an opinion on if the binaries can/share share the same flag name or not.. I would probably give them different flag names incase anyone has infra that doesn't allow one flag to be set differently on different binaries, but I'll admit I don't know this stuff that well.

[ardaguclu]

Why it certainly falls back to version four (v4.channel.k8s.io)?. Since we are using legacy SPDY executor and if very old API server uses v3.channel.k8s.io, SPDY executor can fall back to version three?

[aojea]

there is only n-3 versions supported, we should focus only on the supported versions

[seans3]

So the current SPDY implementation asks for four versions, with v4.channel.k8s.io being the highest priority (v3 next, etc.). The request headers looks like

I0518 16:16:00.390959  633715 round_trippers.go:469] Request Headers:
I0518 16:16:00.390963  633715 round_trippers.go:473]     X-Stream-Protocol-Version: v4.channel.k8s.io
I0518 16:16:00.390967  633715 round_trippers.go:473]     X-Stream-Protocol-Version: v3.channel.k8s.io
I0518 16:16:00.390969  633715 round_trippers.go:473]     X-Stream-Protocol-Version: v2.channel.k8s.io
I0518 16:16:00.390972  633715 round_trippers.go:473]     X-Stream-Protocol-Version: channel.k8s.io

Since it has been so many releases since v4 has been implemented, this is almost always the version selected in the handshake.

I do not think we should change anything about the current SPDY implementation. In other words, I believe it would be fine to keep the request for these four versions (in which it is almost certain the v4 version is chosen).

[ardaguclu]

Since we are changing bi-directional protocol, I agree that new subprotocol (v5.channel.k8s.io )would be better for the sake of maintainability. It also serves managing the fallback mechanism as said above.

[ardaguclu]

Why StreamTranslatorProxy terminates websocket connection?. If the websocket connection is terminated between kubectl - API server, how data transfer will be done between these two components?

[seans3]

When I say the connection is terminated in the StreamTranslatorProxy, I do not mean that the data messages stop there. I just mean that the WebSockets data messages are received there by the WebSockets server. These received data messages are then proxied onto a SPDY client to continue upstream (to Kubelet and then the Container Runtime). So the StreamTranslatorProxy proxies the data by translating the streamed data messages from WebSockets to SPDY.

[ardaguclu]

To assure that preserving feature parity of what SPDY has, we might need to add tests cases not only for websocket but also existing SPDY and I think it deserves being mentioned in alpha.

[seans3]

I've added another note to supplement SPDY integration and e2e tests to ensure no regressions. Please let me know what you think.

[ardaguclu]

I think, v5.channel.k8s.io subprotocol is already a some kind of a feature flag in API server. Everything will work as is, as long as clients send requests in current subprotocols(v4..v1) and v5.channel.k8s.io subprotocol will be used in kubectl, only when it's feature flag is enabled.

[jpbetz]

Also mention that this is a feature of SPDY that was not adopted by HTTP 2.0?

[seans3]

I've updated the Motivation section with a mention of why HTTP/2.0 was not considered. NOTE There is an Alternatives section in the KEP where we intend to more fully explore why HTTP/2.0 is inadequate.

[jpbetz]

Are we planning to leave the API Server->Kubelet leg as SPDY forever? I'd strongly prefer that our long term plan be to eliminate SPDY entirely.

[aojea]

indeed we should details the long term plan ... but this implies modifies the container runtimes, last change I remember that depended on those took 2 years

[jpbetz]

Looks like there are two remaining legs:

• API Server -> Kubelet - I'd like a long term plan for this
• Kubelet -> CRI - ?? I agree that this is likely to leave us supporting two protocols indefinitely. Explaining this clearly would be good enough for me.

[seans3]

Definitely, the plan is to incrementally transition each of the SPDY communication legs to WebSockets. We do not intend to support two protocols indefinitely. So I've added a section describing the transitions: Future: Kubelet StreamTranslatorProxy. This new section details the straightforward process to transition the next communication leg, as well as the plan to transition the final leg from Kubelet to the Container Runtimes. Please let me know what you think of this new section.

As a side note, I'm pretty sure @deads2k already agreed to this incremental transition plan during the May 3rd SIG API Machinery meeting. If I recall correctly, he agreed to allow us to initially target the communication leg from kubectl to the API Server, if we will transition the next leg (from the API Server to Kubelet) after the initial leg goes GA. If anyone else has a different recollection of that exchange, please let me know.

[soltysh]

I think the ask here is to have an explicit plan how the other two legs will be handled. Will those be done through separate KEPs (if so I'd suggest opening issues right away and link them in the proposal as next steps). Or what's the rough plan, if other?

[seans3]

The two legs will be straightforward to implement, since both the API Server and the kubelet proxy streaming data using the exact same code (UpgradeAwareProxy). Our plan for implementing a WebSocket communication path is to delegate from the UpgradeAwareProxy to a proxy which translates WebSocket data into upstream SPDY data; this is our StreamTranslatorProxy. BTW, we have a proof-of-concept PR which implements this StreamTranslatorProxy. In order to extend the WebSocket communication path from the API Server to kubelet, we will only have to move the StreamTranslatorProxy delegation code from the API Server to the kubelet. We will not need separate KEP's.

[jpbetz]

That's great news. From a feature tracking perspective having everything in one KEP is ideal. It's totally fine to break implementation work across releases like @soltysh described. We don't even need to know, up front, what goes into what exact release, we just need to list out all the changes that we want to have in place before we consider beta graduation. Then you're free to tackle as many as you can get to in each release.

[soltysh]

That's great news, just call it out in the proposal (only high level mention that kube-apiserver -> kubelet will be handled at beta, for example, etc) at which stages you're planning to deal with the other legs, this will allow everyone the scope of changes.

[soltysh]

You can call it out as 3 stages:

1. communication between kubectl and kube-apiserver
2. communication between kube-apisever and kubelet
3. communication between kubelet and CRI
   Step 1 will be implemented during alpha (name the kubectl commands here) and beta (kubectl port-forward). Step 2 and step 3 will be implemented at beta (or whatever is appropriate).

Remember it's not set in stone, we can always update this document as we go, but it's good to write down the initial plan, it'll be easier for everyone to understand the progress and you can always update to match the actual progress.

[jpbetz]

What if we end up needing a new channel version for SPDY? I'm a bit uncomfortable sharing the version numbers across protocols. Could we instead have something more like X-Stream-Protocol-Version: v1.websocket-channel.k8s.io ?

[soltysh]

Or just v1.websocket.k8s.io?

[jpbetz]

Why can't content negotiation handle this in a single request? Client could send:

POST /api/v1/…/pods/nginx/exec?command=<CMD>... HTTP/1.1
Connection: Upgrade
Upgrade: SPDY/3.1
X-Stream-Protocol-Version: v5.channel.k8s.io (or v1.websocket-channel.k8s.io)
X-Stream-Protocol-Version: v4.channel.k8s.io
...

Server responds with the upgrade path it can support, which could be either:

HTTP/1.1 101 Switching Protocols
Connection: Upgrade
Upgrade: websocket
...

or

HTTP/1.1 101 Switching Protocols
Connection: Upgrade
Upgrade: SPDY/3.1
X-Stream-Protocol-Version: v4.channel.k8s.io

?

[jpbetz]

Ah I see the below reasons. I think it's fine if clients choose to make two round trips. The server should be able to handle an upgrade request for both protocols and then pick the preferred protocol.

[jpbetz]

I'd like enough design detail to show that we will be able to switch kubectl port-forward to websockets in the future. Incremental progress toward websockets is great. Having two protocols indefinitely is an outcome I want to avoid.

[jpbetz]

Whay are we excluding this? Eliminating all SPDY on one connection leg seems like a good minimum for this KEP because it allows clients and intermediaries to stop caring about SPDY.

[seans3]

This is an excellent question. Simply put, we are excluding port-forward from this release in order to reduce the scope of our effort and finally make some tangible progress. This issue has been outstanding for over eight years (since it was first identified in 2015). We have confidence that kubectl exec, cp, and attach (which use the RemoteCommand subprotocol) can be completed for alpha this release because we've already mostly proven it's possible with a proof-of-concept PR: kubernetes/kubernetes#116778. But progress on port-forward has been more difficult to achieve. Clayton Coleman abandoned and closed an attempt to implement port-forward over WebSockets back in 2017: kubernetes/kubernetes#50428. It appears he realized the scope of that effort would be much larger than he initially thought. It is doubtful that we would be able to implement both subprotocols over WebSockets this release. To be clear, it is definitely our intent to implement both subprotocols over WebSockets. But our strategy is to not let the perfect be the enemy of the good. We believe getting 3 out of 4 kubectl streaming commands out sooner is better than attempting to get out all 4 later. If we do not agree on this point, I think we should add an UNRESOLVED tag about this issue to the KEP, so we can still merge this initial provisional KEP. Please let me know what you think.

[fedebongio]

I'm leaning to start with the initial proposal to avoid scope creeping: in the end it's Alpha, we could decide the minimum set of functionality we want right? Especially if Sean thinks that port-forward could delay this to the point of not making it in the next release. It's been 8 years, I'd prefer to be able to add incremental value step by step, rather than waiting more to have it complete.

[soltysh]

I'll second Fede and Joe here, the doc can point out all of them, but implementation can be spread across several releases. We can explicitly point out that alpha1 will be 3 commands, alpah2 will be port-forward, and only once we have all of them we'll transition to beta. Alternatively, we can say that port-forward will block GA of this feature. This is fine as well, imo.

[seans3]

In order to get 3 out of 4 commands into users hands sooner rather than later, I think your second proposal is better; we can say that port-forward blocks GA (but not beta).

[seans3]

I've added a section about how we intend to iterate (and the maturity levels) for the PortFoward as well as RemoteCommand subprotocols. This section is titled: Future: Port Forward Subprotocol. Please let me know what you think.

[soltysh]

Same minor nits, but overall this looks great for alpha, thx @seans3 for patience 😅

/lgtm
from sig-cli pov

[soltysh]

👍

[jpbetz]

@deads2k PRR LGTM for alpha.

[deads2k]

Given detection in the kube-apiserver that is similar to kubectl, the kube-apiserver to kubelet connection could switch using the same mechanism. The auto-detection is important since kubelets can skew heavily, but that detection needs to happen regardless.

Is there a reason similar detection and patterns cannot work for kube-apiserver to kubelet cannot work?

[seans3]

I have modified the KEP to schedule extending WebSocket to the Kubelet from Post-GA to Pre-GA. Please let me know if this addresses your concern.

You are correct that the mechanism for delegating to the StreamTranslatorProxy (which implements the WebSockets) is the same in the API Server as it is in the Kubelet, since both the API Server and the Kubelet are using the same UpgradeAwareProxy currently. Extending WebSockets to Kubelet will be 1) adding the same conditional delegation to StreamTranslatorProxy into the Kubelet, and 2) adding a step in the API Server to see if the WebSockets upgrade succeeded (101 Switching Protocols) signaling Kubelet supports the StreamTranslatorProxy. I think this step should be pretty straightforward.

[jpbetz]

/lgtm
/approve

/hold
For @deads2k to review apiserver->kubelet pre-GA requirements and approve PRR

[aojea]

/lgtm

Some context about the decision for the Header name, copied from the slack discussion

I’m rereading the RFC https://www.rfc-editor.org/rfc/rfc6455#section-1.9 and it seems we can use Sec-WebSocket-Protocol or Sec-WebSocket-Extensions,
the problem is that is clear that any value used for those headers MUST be registered in the IANA register, and I don’t think we should go into those business, I think we can propose the addition of a new header based on the spirit of the Sec-WebSocket-Protocol
Optionally, a |Sec-WebSocket-Protocol| header field, with a list
of values indicating which protocols the client would like to
speak, ordered by preference.
https://www.rfc-editor.org/rfc/rfc6455#section-1.9 (edited)

Something as: K8s-Websocket-Protocol is a new header field with the same semantics that Sec-WebSocket-Protocol defined in https://www.rfc-editor.org/rfc/rfc6455#section-1.9, that allows the client to specify the subprotocol used by the server, in this case stream-translate will indicate the apiserver to translate the stream to spdy …

Also, independently of the KEP we shoud merge the websocket protocol fixes, since we'll need them to be revendored by the runtimes, so the sooner the better

kubernetes/kubernetes@6ae9ab3
kubernetes/kubernetes@93ab8d1

[deads2k]

I have modified the KEP to schedule extending WebSocket to the Kubelet from Post-GA to Pre-GA. Please let me know if this addresses your concern.

Excellent. No other concerns.

/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jpbetz, seans3, soltysh

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:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-api-machinery/OWNERS [deads2k,jpbetz]

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