Add support for Kuberentes streaming subprotocol v5 within Teleport by AntonAM · Pull Request #39337 · gravitational/teleport

AntonAM · 2024-03-14T14:01:56Z

This PR adds support Kuberentes streaming subprotocol v5, when making calls within Teleport (and to target Kubernetes clusters). Kubernetes team is deprecating SPDY protocol for streaming commands (exec, attach, port forward) and adding websocket based subprotocol v5. They released it as alpha in Kubernetes version 1.29 and it will be default in 1.30. Port forwarding migration to websocket was delayed, currently alpha is planned for 1.30.

tigrato · 2024-03-15T09:32:59Z

We can't use this websocket executor because if the kubernetes_service doesn't support protocol v5, it will fallback to websocket v4 which doesn't support termination signals.

I see interest in dropping SPDY executor between Teleport components so we can say goodbye to the legacy and unmaintained SPDY protocol. But in order to do it, we need to ensure the other teleport component we are dialing supports protocol v5, otherwise kubectl will be mostly broken when stdin terminates.

You can either:

check the version of the kubernetes_service and if > some version, we allow the websocket executor because we know it supports it.

We implement our own fallback if the server doesn't support v5

we delay this until teleport v16

I think the teleport v16 approach is the sanest for now.

Just to be clear, we can still forward the requests internally using SPDY while the users request websockets. When I wrote the WS implementation, I had that in mind and teleport accepts connections for SPDY and Websocket but always initiates the requests as SPDY between components and even the kube cluster.

The only part that needs to be delayed is the internal requests/kube requests using web sockets. We can still support websocket v5 as user's frontend.

Nothing else needs to be changed.

Yep, I know, I'm looking into fallback approach and generally it works great, but there's a problem with whole session context cancellation by the trackingConnection (s.connMonitorCancel), it was added to fix a flaky test, maybe there's a way around it.

I've implemented fallback approach and tested it in all configurations and permutations - proxying to a v5 capable kube, v5 capable our kube_service, legacy service with both tty and tty-less execs, everything seems to be working fine, so we don't have to check if target kube/kube_service supports v5 protocol, and we can merge this and backport it to at least v15.

As per discussion with Tiago - I've split this PR in two parts, first (now PR #39755 ) will add support for receiving incoming connection (e.g. kubectl) in websocket subprotocol v5, and it will be backported to all supported Teleport versions. It's a safe change, since we only receive subprotocol v5, but then make proxying requests using old stable SPDY implementation. And in this PR we add making proxying requests (to Teleport kube_services and target Kubernetes clusters) using v5 subprotocol and we'll leave it to v16.

tigrato · 2024-03-15T09:42:17Z

kube service should never forward the request to kube api as websocket without properly ensuring it supports websocket v5

tigrato · 2024-04-01T09:26:44Z

+	cfg := &rest.Config{
+		// WrapTransport will replace default roundTripper created for the WebsocketExecutor
+		// and on successfully established connection we will set upgrader's websocket connection.
+		WrapTransport: func(baseRt http.RoundTripper) http.RoundTripper {


Are we supposed to do this?
What's the reasoning behind this wrap?
How many requests do we do using this wrapper? Doesn't the default round tripper built by remotecommand.NewWebSocketExecutor, does a request?

We need to use our custom dialer to be able to reach kube service over the reverse tunnel, default round tripper won't be able to dial. We do the same thing for the SPDY executor - we provide our custom dialer:

The only thing that for SPDY they provide NewSPDYExecutorForTransports where we can set our custom transport directly, but for the websocket executor they didn't expose this API, so we replace transport in the wrap to achieve the same effect.

tigrato · 2024-04-01T09:30:54Z

+}
+
 func (f *Forwarder) getExecutor(sess *clusterSession, req *http.Request) (remotecommand.Executor, error) {
+	spdyExec, err := f.getSPDYExecutor(sess, req)


Can we use the default executor if the agent version is >= supported version?
Check 07514ea#diff-d003554d2ebabbc29e7d8188865c07e1e968baf83d650eca34fdd57d6b4808ae to gather more details on how to check the kube service version.

It's just easier to do a single request to reduce latency and resource usage. we can delete the new code on teleport 17. We should do the same for Kube clusters <=1.29

I've added version checking instead of fallback executor 7436a53

tigrato · 2024-04-09T13:49:57Z

+}
+
 func (f *Forwarder) getExecutor(sess *clusterSession, req *http.Request) (remotecommand.Executor, error) {
+	if details, ok := f.clusterDetails[sess.kubeClusterName]; ok &&


kube details must be protected with the lock.

tigrato · 2024-04-09T13:52:54Z

+
 func (f *Forwarder) getExecutor(sess *clusterSession, req *http.Request) (remotecommand.Executor, error) {
+	if details, ok := f.clusterDetails[sess.kubeClusterName]; ok &&
+		kubernetesSupportsExecSubprotocolV5(details.kubeClusterVersion) && f.allServersSupportExecSubprotocolV5(sess) {


We don't need to enforce both properties.

If the service is a kube_service or a legacy proxy serving the kube cluster directly, you should check details.KubeClusterVersion, if it's a proxy or a legacy proxy not serving the kube cluster, you check allServersSupportExecSubprotocolV5.

We can use the new protocol between teleport components and rely on SPDY for the final forwarding to kube cluster.

Done 3d6d5cb

rosstimothy · 2024-04-18T12:34:57Z

PTAL @tigrato

…executor.

Now we don't use fallback executor, so we will not do possible second connection during execution.

… requirement we need.

AntonAM added the kubernetes-access label Mar 14, 2024

AntonAM requested a review from tigrato March 14, 2024 15:13

AntonAM marked this pull request as ready for review March 14, 2024 15:13

github-actions Bot requested a review from lxea March 14, 2024 15:14

github-actions Bot added the size/md label Mar 14, 2024

AntonAM added backport/branch/v15 and removed size/md labels Mar 14, 2024

zmb3 reviewed Mar 14, 2024

View reviewed changes

Comment thread lib/kube/proxy/forwarder.go

tigrato reviewed Mar 15, 2024

View reviewed changes

AntonAM force-pushed the anton/kube-subprotocol-v5 branch 4 times, most recently from 39a53c2 to e5243f1 Compare March 22, 2024 03:55

rosstimothy requested a review from tigrato March 22, 2024 21:18

AntonAM mentioned this pull request Mar 24, 2024

Add support for Kubernetes websocket subprotocol v5 incoming connections. #39755

Merged

AntonAM force-pushed the anton/kube-subprotocol-v5 branch from c9ee558 to 5a4ed51 Compare March 24, 2024 22:43

AntonAM changed the base branch from master to anton/kube-wsv5-incoming March 24, 2024 22:43

AntonAM changed the title ~~Add support for Kuberentes streaming subprotocol v5~~ Add support for Kuberentes streaming subprotocol v5 within Teleport Mar 24, 2024

AntonAM force-pushed the anton/kube-wsv5-incoming branch from 56c5787 to 4b87801 Compare March 24, 2024 22:50

AntonAM force-pushed the anton/kube-subprotocol-v5 branch from 5a4ed51 to 0144084 Compare March 24, 2024 22:55

lxea approved these changes Mar 25, 2024

View reviewed changes

Base automatically changed from anton/kube-wsv5-incoming to master March 25, 2024 13:40

AntonAM added the no-changelog Indicates that a PR does not require a changelog entry label Mar 28, 2024

AntonAM force-pushed the anton/kube-subprotocol-v5 branch from 0144084 to a8173f9 Compare March 28, 2024 09:56

AntonAM removed the backport/branch/v15 label Mar 28, 2024

zmb3 reviewed Mar 28, 2024

View reviewed changes

Comment thread lib/kube/proxy/remotecommand.go

tigrato reviewed Apr 1, 2024

View reviewed changes

AntonAM requested a review from tigrato April 5, 2024 21:46

tigrato reviewed Apr 9, 2024

View reviewed changes

AntonAM requested a review from tigrato April 16, 2024 20:00

tigrato approved these changes Apr 18, 2024

View reviewed changes

AntonAM added this pull request to the merge queue Apr 18, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 18, 2024

AntonAM added 6 commits April 18, 2024 09:52

Add support for Kubernetes websockets subprotocol v5 within Teleport

905cafb

Return more specific error.

858137d

Check version of kube services and kubernetes server before creating …

3404a63

…executor.

Revert changes related to connection monitoring contexts

a858144

Now we don't use fallback executor, so we will not do possible second connection during execution.

Apply locks for accesing kubernetes server version and only check the…

cd2c30f

… requirement we need.

Fix v5 subprotocol k8s min version.

a7074e1

AntonAM force-pushed the anton/kube-subprotocol-v5 branch from 2b4dee5 to a7074e1 Compare April 18, 2024 16:52

AntonAM added this pull request to the merge queue Apr 18, 2024

Merged via the queue into master with commit fb0645e Apr 18, 2024

AntonAM deleted the anton/kube-subprotocol-v5 branch April 18, 2024 17:57

Conversation

AntonAM commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosstimothy commented Apr 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AntonAM commented Mar 14, 2024 •

edited

Loading