Add port forward #446

kazk · 2021-03-01T01:47:14Z

Super rough sketch I mentioned almost 2 months ago. #127 (comment)

I don't like this at all, but an example with Nginx (from Python client example) works and can be a starting point for discussion. Any feedback/ideas are appreciated, but I honestly don't remember much, so I might not be able to answer "why?".

Protocol

WebSocket messages are binary ([channel, ...data]) like /attach and /exec.

For /portforward, channels are associated with the specified ports and each port uses 2 channels: a duplex data channel (2*i) and a read-only error channel (2*i + 1) for ith port.

The first message for a channel is always 3 bytes and describes this association:

channel (u8)
port number (u16 LE)

Idea

EDIT: Maybe this can be done on top of the lower level API provided by kube in this PR. I won't be trying this for now.

Make Portforwarder tower::MakeService (Service of Service)? I haven't tried, but it might be possible, and it'll make it a lot more usable.

let portforwarder = pods.portforward("example", &[80]).await?;
// takes port and returns Service<Request<Body>>
let mut forwarder = portforwarder.into_service();
let mut svc = forwarder.call(port).await?;
let response = svc.call(request).await?;

Error handling
Add example similar to kubectl port-forward example local_port:remote_port.

kube/src/api/portforward.rs

kube/src/api/subresource.rs

clux · 2021-03-01T08:54:13Z

Hey, this is great! A lot of the hairy channel management (spawning a task for each port makes sense) looks like it's done. Error handling perhaps needs a bit more improvements, but you've already got a nice stream interfaces on top of Api. The returned Portforwarder letting you drill into the Port with the various methods is very user friendly.

Well, insofar as letting people write raw http requests into a stream can be user friendly.
I do think we should strive towards a way to have people dump more ergonomic requests in rather than raw-text, even though I guess they can construct http requests themselves and serialize them manually.

EDIT: But I guess we can't go all in HTTP as this is meant to be protocol agnostic.

The Service idea does sound interesting though! At least if we can something like the interface you posted.

clux · 2021-05-22T13:35:28Z

Ah, forgot about this. Yeah, the example still works, and presents the basic functionality. We certainly can merge it as it stands as an initial version of it. But also feel like it might be better to have the canonical kubectl port-forward style example (where we actually listen on that port and forward the tcp traffic) working before releasing.

Then on the other hand, people needing that probably aren't writing up against kube do do that atm, and just use kubectl.

partiallyordered · 2021-07-05T12:44:10Z

I've started using this branch and it's working admirably, thanks both 😀 . I don't know if it fits here, but consider merging my PR here containing some example code putting hyper on top of the port-forward: kazk#4 before merging #446 .

partiallyordered · 2021-07-05T12:54:36Z

Come to think of it, I might also be able to extract a tokio-tungstenite-based websocket example from my code in the next few days if that's interesting.

codecov-commenter · 2022-01-18T05:43:05Z

Codecov Report

Merging #446 (20a91e5) into master (3381e69) will decrease coverage by 1.90%.
The diff coverage is 14.50%.

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   71.92%   70.02%   -1.91%     
==========================================
  Files          56       57       +1     
  Lines        3775     3906     +131     
==========================================
+ Hits         2715     2735      +20     
- Misses       1060     1171     +111

Impacted Files	Coverage Δ
kube-client/src/api/mod.rs	`86.66% <ø> (ø)`
kube-client/src/api/portforward.rs	`0.00% <0.00%> (ø)`
kube-client/src/api/subresource.rs	`54.54% <41.66%> (-1.42%)`	⬇️
kube-core/src/subresource.rs	`76.03% <77.77%> (+0.30%)`	⬆️
kube-runtime/src/wait.rs	`70.00% <0.00%> (+2.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3381e69...20a91e5. Read the comment docs.

clux

thanks for fixing this up - we haven'd had real feedback on this in a year, so it's probably fine to add it to get more users. i am happy to approve, just left a few minor nits here first that i think could be easy wins if they work

kube-client/src/api/portforward.rs

examples/pod_portforward_hyper_http.rs

kube-client/src/api/portforward.rs

clux · 2022-01-20T05:17:38Z

I thought the port stuff could be done a little cleaner, but ultimately don't see a good way - so resolved my gripes with some comments. Added some minor suggested tweaks to docs

ultimately though, the functionality here works well and we should probably merge this to invite more users. How are you feeling about it now @kazk?

clux · 2022-01-24T04:41:33Z

Leaving this out of the release as it is still a draft PR - although I would be happy merging it.

kazk · 2022-01-29T11:19:25Z

Sorry for the delay.
I've refactored to avoid panics, and split each loop into smaller functions. It should be easier to understand and iterate on. It's still missing an example that binds to local port, but we can do it later/ask for help.

kube-client/src/api/portforward.rs

kazk · 2022-01-30T04:54:29Z

I got an example to bind local port working. I'll push it after cleaning it up.

clux

Ah, didn't realise you had such a large refactoring left in you. Thanks a lot, it's definitely cleaner!
Minor comments left, but no real suggestions other than the superficial example doc thing.

kube-client/src/api/portforward.rs

examples/pod_portforward_hyper_http.rs

clux

very cool full port-forward example in just over 100 lines with lot of helpful technical parts in it!

everything seems like it's working well! don't have any other outstanding comments.
looking forward to have this in :-)

clux · 2022-01-30T17:05:21Z

examples/pod_portforward_bind.rs

+    let context = Arc::new(Mutex::new(sender));
+    let make_service = make_service_fn(move |_conn| {
+        let context = context.clone();
+        let service = service_fn(move |req| handle(context.clone(), req));
+        async move { Ok::<_, Infallible>(service) }
+    });


cool quick use of tower!

Signed-off-by: kazk <[email protected]>

Co-authored-by: Eirik A <[email protected]> Signed-off-by: kazk <[email protected]>

Signed-off-by: kazk <[email protected]>

Co-authored-by: Eirik A <[email protected]> Signed-off-by: kazk <[email protected]>

Signed-off-by: kazk <[email protected]>

kazk · 2022-01-31T19:43:49Z

Yeah, the local port example was easier than I had expected. I think this is good enough as a first version.
Merging now. Not squashing to get @partiallyordered in the contributors list.

clux · 2022-02-01T05:04:48Z

Very nice! I'll do a release later on today. Tons of stuff ready.
Will try to put a paragraph in the CHANGELOG about it as it's one of the biggest additions (and technically completes the penultimate requirement for client-gold)

kazk mentioned this pull request Mar 1, 2021

implement the last subresources #127

Open

11 tasks

clux reviewed Mar 1, 2021

View reviewed changes

kube/src/api/portforward.rs Outdated Show resolved Hide resolved

kube/src/api/portforward.rs Outdated Show resolved Hide resolved

kube/src/api/portforward.rs Outdated Show resolved Hide resolved

kube/src/api/subresource.rs Outdated Show resolved Hide resolved

kazk force-pushed the portforward branch from 7411ef4 to e1b02c5 Compare May 22, 2021 02:38

clux approved these changes May 22, 2021

View reviewed changes

kazk force-pushed the portforward branch from 5426720 to aa58c61 Compare October 7, 2021 01:12

kazk self-assigned this Oct 26, 2021

clux added the client-gold gold client requirements label Oct 30, 2021

kazk force-pushed the portforward branch 2 times, most recently from 4445087 to 61d2add Compare January 18, 2022 05:35

clux reviewed Jan 18, 2022

View reviewed changes

clux added the changelog-add changelog added category for prs label Jan 18, 2022

clux added this to the 0.67.0 milestone Jan 18, 2022

nightkr mentioned this pull request Jan 18, 2022

runtime: Replace DashMap with a locked AHashMap #785

Merged

kazk force-pushed the portforward branch from 4187482 to 4fc43ca Compare January 19, 2022 01:20

clux reviewed Jan 20, 2022

View reviewed changes

kube-client/src/api/portforward.rs Show resolved Hide resolved

clux reviewed Jan 20, 2022

View reviewed changes

kube-client/src/api/portforward.rs Outdated Show resolved Hide resolved

clux approved these changes Jan 20, 2022

View reviewed changes

clux modified the milestones: 0.67.0, 0.68.0 Jan 24, 2022

kazk force-pushed the portforward branch 2 times, most recently from 1c697d5 to 2a21db8 Compare January 29, 2022 11:12

kazk commented Jan 29, 2022

View reviewed changes

kube-client/src/api/portforward.rs Show resolved Hide resolved

kazk marked this pull request as ready for review January 29, 2022 11:27

kazk changed the title ~~Add /portforward support~~ Add port forward Jan 29, 2022

clux approved these changes Jan 30, 2022

View reviewed changes

kazk force-pushed the portforward branch from 20a91e5 to 710ce28 Compare January 30, 2022 06:02

clux approved these changes Jan 30, 2022

View reviewed changes

clux mentioned this pull request Jan 31, 2022

Update parking_lot requirement from 0.11 to 0.12 #799

Closed

kazk and others added 9 commits January 31, 2022 11:13

Add Portforward support

fbf8d94

Signed-off-by: kazk <[email protected]>

Pod port-forward hyper HTTP request example

4a96785

Signed-off-by: kazk <[email protected]>

Use await_condition and reduce unwrap in examples

84a154c

Signed-off-by: kazk <[email protected]>

Change Portforwarder::ports() to return &mut [Port]

36b0bdd

Signed-off-by: kazk <[email protected]>

Improve docs

d0da0a8

Co-authored-by: Eirik A <[email protected]> Signed-off-by: kazk <[email protected]>

Avoid panics and split into smaller functions

bc05653

Signed-off-by: kazk <[email protected]>

Clarify the purpose of the task

5582177

Co-authored-by: Eirik A <[email protected]> Signed-off-by: kazk <[email protected]>

Fix unused import

bc2984a

Signed-off-by: kazk <[email protected]>

Add an example to bind a local port

c3cb3fe

Signed-off-by: kazk <[email protected]>

kazk force-pushed the portforward branch from 710ce28 to c3cb3fe Compare January 31, 2022 19:13

kazk merged commit 2c5573b into kube-rs:master Jan 31, 2022

kazk deleted the portforward branch January 31, 2022 19:44

kazk mentioned this pull request Mar 22, 2022

portforward: Improve API and support background task cancelation #854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add port forward #446

Add port forward #446

kazk commented Mar 1, 2021 •

edited

Loading

clux commented Mar 1, 2021 •

edited

Loading

clux commented May 22, 2021 •

edited

Loading

partiallyordered commented Jul 5, 2021

partiallyordered commented Jul 5, 2021 •

edited

Loading

codecov-commenter commented Jan 18, 2022 •

edited

Loading

clux left a comment

clux commented Jan 20, 2022

clux commented Jan 24, 2022

kazk commented Jan 29, 2022

kazk commented Jan 30, 2022

clux left a comment

clux left a comment

clux Jan 30, 2022

kazk commented Jan 31, 2022

clux commented Feb 1, 2022

Add port forward #446

Add port forward #446

Conversation

kazk commented Mar 1, 2021 • edited Loading

Protocol

Idea

clux commented Mar 1, 2021 • edited Loading

clux commented May 22, 2021 • edited Loading

partiallyordered commented Jul 5, 2021

partiallyordered commented Jul 5, 2021 • edited Loading

codecov-commenter commented Jan 18, 2022 • edited Loading

Codecov Report

clux left a comment

Choose a reason for hiding this comment

clux commented Jan 20, 2022

clux commented Jan 24, 2022

kazk commented Jan 29, 2022

kazk commented Jan 30, 2022

clux left a comment

Choose a reason for hiding this comment

clux left a comment

Choose a reason for hiding this comment

clux Jan 30, 2022

Choose a reason for hiding this comment

kazk commented Jan 31, 2022

clux commented Feb 1, 2022

kazk commented Mar 1, 2021 •

edited

Loading

clux commented Mar 1, 2021 •

edited

Loading

clux commented May 22, 2021 •

edited

Loading

partiallyordered commented Jul 5, 2021 •

edited

Loading

codecov-commenter commented Jan 18, 2022 •

edited

Loading