🌱 kcp: enable websocket dialer with fallback to spdy #12902
Conversation
k8s-ci-robot
added
area/provider/control-plane-kubeadm
k8s-ci-robot
added
the
size/S
|
/test help |
|
@chrischdi: The specified target(s) for The following commands are available to trigger optional jobs: Use In response to this:
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-sigs/prow repository. |
|
/test pull-cluster-api-e2e-conformance-ci-latest-main |
|
|
||
| dialer := spdy.NewDialer(d.upgrader, httpClient, "POST", req.URL()) | ||
|
|
||
| // Configure websocket dialer and keep spdy as fallback |
There was a problem hiding this comment.
Change looks easy enough. Any ideas on how I can validate if this is correct? (e.g. is there some prior art somewhere else?)
There was a problem hiding this comment.
I did via using tilt.
I added a breakpoint here for KCP accordingly to check if it uses websocket or does the fallback.
For v1.34: it did not do the fallback and successfully used websockets (verified via the dial method)
For v1.30 it did use the fallback / old code SPDY
There was a problem hiding this comment.
Thx!
For future reference, kubectl does something similar: https://github.com/kubernetes/kubernetes/blob/c3f15fd707a092e6cb7d96b84b81ada1f118d759/staging/src/k8s.io/kubectl/pkg/cmd/portforward/portforward.go#L144-L155
|
Thank you very much! /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: de6e2d1331db7d0e37dd7e4b0149fa91d1b338c9
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Let's not implement it for the in-memory provider. Just additional code that we would have to maintain, and the test coverage that we get for the SPDY fallback doesn't hurt |
|
/cherry-pick release-1.11 |
|
@chrischdi: new pull request created: #12931 In response to this:
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-sigs/prow repository. |
4 participants
What this PR does / why we need it:
Note: tested via the debugger that:
I think we are fine with our in-memory implementation to keep using spdy as long as we don't drop it.
Didn't find a easy way to implement it for in-memory, but should definetly be possible.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
/area provider/control-plane-kubeadm