Skip to content
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.

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

Header Connection: close causes kubectl to fail exec #33050

Merged
merged 2 commits into from Oct 9, 2023

Conversation

tigrato
Copy link
Contributor

@tigrato tigrato commented Oct 5, 2023

The header Connection: close causes failure in kubectl when it upgrades the connection to SPDY.

The ReadTimeout and WriteTimeout are known to cause problems with Kubernetes watch streams.

Fixes #33020

The header `Connection: close` causes failure in kubetl when it upgrades
the connection to SPDY.

The `ReadTimeout` and `WriteTimeout` are known to cause problems to
Kubernetes watch streams.

Fixes #33020

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
@tigrato tigrato marked this pull request as ready for review October 5, 2023 21:59
@github-actions github-actions bot added size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Oct 5, 2023
Copy link
Contributor

@jakule jakule left a comment

Choose a reason for hiding this comment

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

Please add a test to prevent regression.

@tigrato
Copy link
Contributor Author

tigrato commented Oct 7, 2023

Please add a test to prevent regression.

I added a test in tsh package that tests kubectl exec flow to prevent regressions

// runKubectlExec runs a kubectl exec command in a dummy pod.
// The mock Kubernetes API server will return the pod name and the stdin data
// written to the pod.
func runKubectlExec(t *testing.T, config *rest.Config) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a cheap way to replace mustGetKubePod in integration tests with doing kubectl exec? Or maybe the integration tests should do both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean downloading kubectl instead of running all this weird logic?

Copy link
Member

Choose a reason for hiding this comment

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

No, I meant replicating what you did in tsh tests in this PR in integration tests for kube proxy. If integration tests also did exec into a container then maybe we'd have caught that sooner.

I typically also did just kubectl get po -A to verify that kube access in Connect works as expected, but I'm going to update the test plan to explicitly mention execing into a container.

@tigrato tigrato added this pull request to the merge queue Oct 9, 2023
@tigrato tigrato removed this pull request from the merge queue due to a manual request Oct 9, 2023
@tigrato tigrato added this pull request to the merge queue Oct 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2023
@tigrato tigrato added this pull request to the merge queue Oct 9, 2023
Merged via the queue into master with commit 8f71301 Oct 9, 2023
30 checks passed
@tigrato tigrato deleted the tigrato/fix-tsh-kube-proxy branch October 9, 2023 19:10
@public-teleport-github-review-bot

@tigrato See the table below for backport results.

Branch Result
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v14 size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
6 participants