Skip to content

kube: properly return the reason for connection disruption#51398

Merged
tigrato merged 3 commits intomasterfrom
tigrato/fix-kube-incorrect-messages
Jan 24, 2025
Merged

kube: properly return the reason for connection disruption#51398
tigrato merged 3 commits intomasterfrom
tigrato/fix-kube-incorrect-messages

Conversation

@tigrato
Copy link
Copy Markdown
Contributor

@tigrato tigrato commented Jan 23, 2025

There are several cases where connection monitor can terminate an ongoing connection. Idle timeout, certificate expiring among others are some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to the client so they don't receive the reason for the exec session being terminated.

This PR fixes that by adding an hook to write the client error response into the connection error channel for clients to be aware.

Part of #18496

Changelog: Improved handling of client session termination during Kubernetes Exec sessions. The disconnection reason is now accurately returned for cases such as certificate expiration, forced lock activation, or idle timeout.

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496
@tigrato tigrato force-pushed the tigrato/fix-kube-incorrect-messages branch from cadd765 to 4bd836b Compare January 23, 2025 11:13
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems reasonable, but I can't say I follow the subtleties of the lib/kube package. You'll probably want a kube reviewer to take a look too.

Comment thread lib/kube/proxy/forwarder.go Outdated
Comment thread integration/kube_integration_test.go Outdated
Comment thread lib/kube/proxy/forwarder.go
Comment thread lib/kube/proxy/forwarder.go Outdated
Comment thread lib/kube/proxy/forwarder.go
Comment thread lib/kube/proxy/remotecommand_websocket.go Outdated
Comment thread lib/kube/proxy/remotecommand_websocket.go Outdated
Comment thread lib/kube/proxy/forwarder.go
Comment thread lib/kube/proxy/remotecommand.go
@tigrato tigrato requested a review from codingllama January 23, 2025 16:06
@tigrato tigrato added this pull request to the merge queue Jan 24, 2025
Merged via the queue into master with commit 2e072c2 Jan 24, 2025
@tigrato tigrato deleted the tigrato/fix-kube-incorrect-messages branch January 24, 2025 10:55
@public-teleport-github-review-bot
Copy link
Copy Markdown

@tigrato See the table below for backport results.

Branch Result
branch/v15 Failed
branch/v16 Failed
branch/v17 Failed

tigrato added a commit that referenced this pull request Jan 24, 2025
* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496

* handle review comments

* handle review comments
tigrato added a commit that referenced this pull request Jan 24, 2025
* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496

* handle review comments

* handle review comments
tigrato added a commit that referenced this pull request Jan 24, 2025
* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496

* handle review comments

* handle review comments
github-merge-queue Bot pushed a commit that referenced this pull request Jan 24, 2025
)

* kube: properly return the reason for connection disruption (#51398)

* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496

* handle review comments

* handle review comments

* fix slog ref
github-merge-queue Bot pushed a commit that referenced this pull request Jan 24, 2025
)

* kube: properly return the reason for connection disruption (#51398)

* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496

* handle review comments

* handle review comments

* fix slog ref
github-merge-queue Bot pushed a commit that referenced this pull request Jan 24, 2025
)

* kube: properly return the reason for connection disruption (#51398)

* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of #18496

* handle review comments

* handle review comments

* fix slog ref

* fix missing import
carloscastrojumo pushed a commit to carloscastrojumo/teleport that referenced this pull request Feb 19, 2025
…onal#51398)

* kube: properly return the reason for connection disruption

There are several cases where connection monitor can terminate an
ongoing connection. Iddle timeout, certificate expiring among others are
some reasons for the connection to be terminated.

For Kubernetes access, the underlying error is never propagated back to
the client so they don't receive the reason for the exec session being
terminated.

This PR fixes that by adding an hook to write the client error response
into the connection error channel for clients to be aware.

Part of gravitational#18496

* handle review comments

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants