[WIP] kubectl exec/cp/attach over WebSocket#116778
Conversation
69ae4f3 to
c7c075a
Compare
|
/cc |
c7c075a to
3a3dee2
Compare
|
/assign @brianpursley |
|
/remove-sig api-machinery |
|
Just a reminder that the gorilla project as a whole has been archived. I think we should look at alternatives for that library for using websocket APIs. It seems like the nhooyr/websocket library is our best option for this? Unsure what others think. Still need to give the PR a review in general. Another option would be gobwas/ws. I'm not too sure how active either of these libraries are unfortunately. It looks like the last release for both libraries was in 2021. |
brianpursley
left a comment
There was a problem hiding this comment.
I haven't had a chance to go through this completely yet, and haven't tried it out yet, but I did take a quick look at the code this evening and left a few comments.
There was a problem hiding this comment.
It looks like ctx is never used.
Would it make sense to move the Stream's implementation into StreamWithContext and propagate ctx to other calls like http.NewRequestWithContext?
Then, Stream would just be something like this:
func (e *wsStreamExecutor) Stream(options StreamOptions) error {
return e.StreamWithContext(context.TODO(), options)
}
func (e *wsStreamExecutor) StreamWithContext(ctx context.Context, options StreamOptions) error {
req, err := http.NewRequestWithContext(ctx, e.method, e.url, nil)
...
It doesn't look like there is any way to pass context to streamer.stream() so I don't know how useful it really is without being able to do that, but it seems like if it takes context, it should try to do something with it if possible.
There was a problem hiding this comment.
apparently ioutil.ReadAll has been deprecated. Might as well use io.ReadAll I guess.
There was a problem hiding this comment.
I know this TODO was brought over from spdy/roundtripper, but it has 7+ years old. Is there anything that needs to be done for this, or should it just be removed?
Or at least maybe provide some additional context about what actually needs to be done, perhaps turning this into a github issue instead of a TODO comment, so hopefully it doesn't remaing a TODO comment for another 7 years.
There was a problem hiding this comment.
Should we be using exec.StreamWithContext() here (and the other two places)?
Per this comment, exec.Stream() is deprecated:
There was a problem hiding this comment.
Something feels a little bit off about this.
For one thing, the interface name of streamCreator might not make sense anymore, as it now does more than just create streams.
What also strikes me is that SPDY has a "do nothing" implementation of Run() which tells me this really is specific to websockets.
I wonder if there is some way to not add Run to the interface and just make it be part of wsStreamExecutor's internal implementation.
For example, maybe do it inside wsStreamExecutor.StreamWithContext(), so then its existence is kept inside the websocket-specific implementation...
creator := newWSStreamCreator(conn)
go creator.run()
return streamer.stream(creator)
Just a thought. I'm not trying to redesign things here, but noticed this interface change and thought maybe there was something not generic enough about it to be on an interface.
67126da to
c28a2b5
Compare
c28a2b5 to
750ff46
Compare
|
/assign @aojea please ping me when is ready for review |
|
Do we have news about this feature? We have issues with external WAFs refusing |
|
This PR was broken up into two smaller, more manageable PRs:
Closing this Proof-of-Concept PR. |
Important - based on previous @ash2k PR: [WIP] Kubectl exec/attach/cp over WebSocket #110142
Proof-of-Concept PR which transitions
kubectl exec,kubectl cp, andkubectl attachfrom using SPDY to WebSockets for the bi-directional streaming protocol communicating between k8s clients and the API Server.Creates WebSockets client (
remotecommand.NewWebSocketExecutor).Creates
StreamTranslatorproxy within the API Server. This proxy contains a WebSockets server and a SPDY client. It terminates the WebSockets communication from the client, sending the data into a SPDY client which is connected upstream to kubelet. In this way, the client to API Server communication uses WebSockets, while communication upstream does not substantially change, continuing to use the SPDY protocol.Manually tested successfully with the following commands using the
nginxpod:$ ./kubectl exec nginx -- date(basic command with STDOUT)$ echo "this is a test" | ./kubectl exec -i nginx -- cat(pipe STDIN to command running on container)$ ./kubectl cp ~/deploy-1.yaml nginx:/(cp file from local to container)$ ./kubectl cp nginx:/product_name /tmp/product_name(cp file from container to local)$ ./kubectl exec -it nginx -- bash(interactive terminal)TODO (as of May 1st, 2023)
Ping/Pong).Resetthe logical stream for a channel within the WebSockets protocol.Resetmeans no data is coming AND no data should be sent (it will be discarded).apiserverand SPDY client code inclient-gointoapimachinery, since the proxy located inapimachineryneeds this code, whileapiserverandclient-gocode can not be imported intoapimachinery. NOTE Currently, since this is a POC PR, the WebSockets server code and SPDY client code has been copied intoapimachinery./kind feature
/sig api-machinery
Fixes: #89163
Example
kubectl execover WebSocketExample
kubectl cpusing WebSockets from container to localExample
kubectl cpusing WebSockets from local to containerExample interactive use of
kubectl execover WebSocketsExample
kubectl execover WebSockets using STDIN