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

Add WebSocket connection support and implement /attach and /exec #360

Merged
merged 24 commits into from Jan 2, 2021

Conversation

kazk
Copy link
Member

@kazk kazk commented Dec 26, 2020

Attempt to support WebSocket connection.

@clux

I'd like to know if this is on the right track before spending more time on it. I think it works, but haven't tried. (outputting to stdout works, see pod_attach example)
This is based on #229 (comment)

I had to change Config a bit so that it's less coupled with reqwest to create AsyncTlsConnector for WebSocket connection when creating a Client.


  • Support WebSocket connection
    • native-tls
    • rustls (accept_invalid_certs not supported at the moment, need rustls 0.17.0+ and that requires updating tokio first)
  • Make WebSocket opt-in with ws feature
  • Decide how to implement attach/exec methods.
    • Added AttachedProcess struct. Need review/feedback.
  • Add /attach (see pod_attach example)
  • Add /exec (see pod_exec example)
  • Move most of ws specific code into a separate module. This should make it much easier to maintain.
  • Validate params. We only get 400 status error without any information when making connection :(
  • Make WebSocketConfig configurable. Will do when there's a need.
  • Make the sizes of internal buffers configurable
  • Rename api/streaming to api/remote_command
  • Improve documentation for AttachParams
    • Explain how stdin, stdout, stderr allows access to writer/streams from AttachedProcess
  • Improve documentation for AttachedProcess
    • Explain how stdin(), stdout(), stderr() works

@kazk kazk force-pushed the websocket-connection branch 3 times, most recently from d87b214 to 52541f8 Compare December 26, 2020 10:49
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

quick initial comments.

kube/Cargo.toml Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/client/mod.rs Outdated Show resolved Hide resolved
kube/src/config/utils.rs Outdated Show resolved Hide resolved
kube/src/config/incluster_config.rs Show resolved Hide resolved
@clux
Copy link
Member

clux commented Dec 26, 2020

I have to admit that I've not really looked at the websocket stuff at all within kubernetes, nor have I dealt much with them otherwise, so you'll have to bear with me a bit unless there are others to help review; feel free to ping others from the original issue* to try and get help. I will ping this PR in the discord (linked in the readme).

But integration wise, holy crap, you've done an excellent job of making it fit the patterns of kube, you've juggled a lot of the really hard bits in the config around TLS, and even have a reasonable setup for the features as well (which always become hairy). Everything is in the place I would expect, and is very readable 👍. I at least think you are going in the right direction.

The only thing I would request is that we try to make the tungstenite (and anything it pulls in) part of a "websocket" or "attach" feature (or something else that describes it better). The additional subresources this supports, while useful to some, is probably not going to be as popular for the people - whose dependency trees are inflated - without a use for them.

(There is also the more philosophical point that this moves kube even further away from not owning the underlying clients; it's already hard to juggle clients, and now we have another type of clients. I was always hoping we'd be able to make it generic long run ala #100 , but I don't really know how to best proceed in that area. If you don't either, don't worry, just noting down some thoughts.)

kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
@kazk kazk force-pushed the websocket-connection branch 2 times, most recently from f685b04 to 480af85 Compare December 27, 2020 02:15
@kazk kazk force-pushed the websocket-connection branch 5 times, most recently from 9d524b7 to 0e09127 Compare December 28, 2020 07:17
@kazk kazk force-pushed the websocket-connection branch 5 times, most recently from f67066a to 65b4376 Compare December 30, 2020 06:30
@kazk kazk changed the title Support WebSocket connection Add WebSocket connection support and implement /attach and /exec Dec 30, 2020
Copy link
Member Author

@kazk kazk left a comment

Choose a reason for hiding this comment

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

There are few more chores left to do, but I think it's now ready for reviews.

kube/src/api/streaming.rs Outdated Show resolved Hide resolved
kube/src/api/streaming.rs Outdated Show resolved Hide resolved
kube/src/api/streaming.rs Outdated Show resolved Hide resolved
kube/src/api/streaming.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Show resolved Hide resolved
kube/src/api/subresource.rs Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
kube/src/client/mod.rs Show resolved Hide resolved
kube/src/client/mod.rs Outdated Show resolved Hide resolved
kube/src/api/streaming.rs Outdated Show resolved Hide resolved
@kazk kazk marked this pull request as ready for review December 30, 2020 09:23
kube/src/client/mod.rs Outdated Show resolved Hide resolved
kube/src/api/subresource.rs Outdated Show resolved Hide resolved
@clux
Copy link
Member

clux commented Jan 1, 2021

Found the compile issue: examples or kube need to at least rev tokio to 0.22.4 for tokio::io::duplex. The 0.22.0 version does not have it (and that's what i got with a blank build).

@clux
Copy link
Member

clux commented Jan 1, 2021

This is looking really good now. Have had a chance to fiddle around with examples and check that things work as expected.

There is one thing I feel is a little awkward re: stream conversions, but that might not be anything at fault herein.

I pushed an example to pod_shell branch trying to redirect the example's process stdin to ws stdin and ws stdout to process stdout. See ccd2c25
It's currently failing to compile on the second tokio::io::copy, haven't dug heavily into why this is, but felt that perhaps should have worked. What do you think?

- Rename `start` to `start_message_loop` to avoid confusion
- Filter to reduce the possible cases within loop
- Clarify error cases
@kazk
Copy link
Member Author

kazk commented Jan 1, 2021

@clux Maybe it's because of futures and tokio having different AsyncRead traits? I'll look into it.

@nightkr
Copy link
Member

nightkr commented Jan 1, 2021

@clux Maybe it's because of futures and tokio having different AsyncRead traits? I'll look into it.

https://docs.rs/tokio-util/0.6.0/tokio_util/compat/index.html might be relevant here

@clux clux mentioned this pull request Jan 1, 2021
@kazk
Copy link
Member Author

kazk commented Jan 2, 2021

I couldn't figure it out, but I decided to just return AsyncRead and AsyncWrite instead because users can convert AsyncRead to a stream easily if they want using tokio::io::read_stream() (tokio_util::io::ReadStream::new() for tokio 1.0).

Got pod_shell example to compile and kind of working.
image

@clux
Copy link
Member

clux commented Jan 2, 2021

Wow, yeah, it works! If users put some ansi unescaping on top of that they'd have something damn close to kubectl exec -it.
The messages returned seem to wrap at 80 characters, which is probably the unsupported terminal resize, so that's expected.

One small spot, while playing around: there is some truncation somewhere. If you send a very large echo "some 2000chars" it seems to truncate the response to around ~1040 characters. Not sure if you have any idea why that might be. We can always open up a follow up issue if not.

At any rate, error handling is looking great, as does the rest of the code. I don't have anything else that I can really nitpick at anymore. So whenever you are satisfied, lemme know and we'll merge it :-).

Thank you so much for all the hard work over the holiday, this is really a huge contribution. Have invited you to the collaborators so you can push branches + merge on here in the future.

@kazk
Copy link
Member Author

kazk commented Jan 2, 2021

One small spot, while playing around: there is some truncation somewhere. If you send a very large echo "some 2000chars" it seems to truncate the response to around ~1040 characters. Not sure if you have any idea why that might be. We can always open up a follow up issue if not.

It might have something to do with the internal buffer size. The default is 1024 bytes.

Thank you so much for all the hard work over the holiday, this is really a huge contribution. Have invited you to the collaborators so you can push branches + merge on here in the future.

Thanks :) This is ready to be merged 👍 I'll do some clean up after #363 and look into the truncation.

@clux clux merged commit b8cf11f into kube-rs:master Jan 2, 2021
@clux
Copy link
Member

clux commented Jan 2, 2021

🎉🎉🎉

Will put it in the next minor, probably try to do it today so we can release it (unless reqwest gets a tokio one version today).

@kazk kazk deleted the websocket-connection branch January 2, 2021 12:30
@praveenperera
Copy link
Contributor

Thanks for this PR @kazk, this came at a perfect time for me personally.

I believe this is going to be super useful for me in the next 2 weeks.

Really appreciate the work.

clux added a commit that referenced this pull request Jan 2, 2021
@clux
Copy link
Member

clux commented Jan 2, 2021

Released in 0.46.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants