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

direct node access logs/portforward/exec/attach via kubelet debug interface #1428

Merged
merged 13 commits into from
Mar 22, 2024
Merged

direct node access logs/portforward/exec/attach via kubelet debug interface #1428

merged 13 commits into from
Mar 22, 2024

Conversation

XciD
Copy link
Contributor

@XciD XciD commented Mar 18, 2024

Adding support for kubelet debug handlers as described in #1425

These methods require a kubelet-debug feature flag.

@XciD XciD changed the title Direct node feat: direct node access logs/portforward/exec/attach Mar 18, 2024
@clux clux linked an issue Mar 18, 2024 that may be closed by this pull request
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.

thanks a lot. this largely looks good to me, just a few nits on testing, top level interface and docs.

kube-core/src/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/lib.rs Outdated Show resolved Hide resolved
kube-client/src/lib.rs Outdated Show resolved Hide resolved
kube-client/src/client/mod.rs Outdated Show resolved Hide resolved
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.

some minor nits and questions, stuff is looking very good though

kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
examples/pod_log_node_proxy.rs Outdated Show resolved Hide resolved
@clux
Copy link
Member

clux commented Mar 19, 2024

Btw, are you testing this with k3d locally? I tried it against my cluster and i can't get it to work out of the box and the port stuff needs a node filter:

INFO[0000] portmapping '10250:10250' lacks a nodefilter, but there's more than one node: defaulting to [servers:*:proxy agents:*:proxy]
FATA[0000] K3sExtraArg '--kube-apiserver-arg=feature-gates=WatchList=true' lacks a node filter, but there's more than one node

@XciD
Copy link
Contributor Author

XciD commented Mar 19, 2024

Btw, are you testing this with k3d locally? I tried it against my cluster and i can't get it to work out of the box and the port stuff needs a node filter:

INFO[0000] portmapping '10250:10250' lacks a nodefilter, but there's more than one node: defaulting to [servers:*:proxy agents:*:proxy]
FATA[0000] K3sExtraArg '--kube-apiserver-arg=feature-gates=WatchList=true' lacks a node filter, but there's more than one node

I've tested with a simple command: k3d cluster create -p 10250:10250, which command are you using ?

@clux
Copy link
Member

clux commented Mar 19, 2024

Ah. It was my own command misuse.

(It seems that possibly --no-lb does not work well with -p -possibly k3d-io/k3d#380. it's normally not a blocking warning but it seems to get elevated when using certain api args.. works without that flag)

At any rate, I have now tried your example and it works 👍

@clux clux added the changelog-add changelog added category for prs label Mar 19, 2024
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.

put some example comments for the type of documentation, feel free to take it verbatim. i am likely out most of tomorrow, but can fixup whatever things remain after that.

kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/node_proxy.rs Outdated Show resolved Hide resolved
kube-client/src/client/mod.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 7.14286% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 72.2%. Comparing base (8df88ce) to head (a56235a).
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1428     +/-   ##
=======================================
+ Coverage   72.1%   72.2%   +0.1%     
=======================================
  Files         75      78      +3     
  Lines       6470    6546     +76     
=======================================
+ Hits        4663    4722     +59     
- Misses      1807    1824     +17     
Files Coverage Δ
kube-client/src/api/subresource.rs 44.0% <ø> (ø)
kube-client/src/client/builder.rs 59.0% <ø> (ø)
kube-client/src/client/mod.rs 72.2% <ø> (+0.3%) ⬆️
kube-client/src/lib.rs 75.2% <ø> (ø)
kube-core/src/subresource.rs 78.5% <100.0%> (ø)
kube-client/src/client/kubelet_debug.rs 0.0% <0.0%> (ø)
kube-core/src/kubelet_debug.rs 0.0% <0.0%> (ø)

... and 2 files with indirect coverage changes

Signed-off-by: Adrien <[email protected]>
@clux clux mentioned this pull request Mar 22, 2024
@clux
Copy link
Member

clux commented Mar 22, 2024

Went through the pr and tried to clean up the documentation a bit further. I have a commit you can cherry-pick at 237e78d if you are happy with it.
Did a few extra renames and stuff to try to more clearly disambiguate.

Then i am happy to send this through :-)

since it shows up in `Client::` impls it should have caveats.
have linked to alternatives, prefixed the names with kubelet_
(which is a little redundant, but it helps here).

also documented and split up a few related things i found with just doc.

Signed-off-by: clux <[email protected]>
@XciD
Copy link
Contributor Author

XciD commented Mar 22, 2024

Went through the pr and tried to clean up the documentation a bit further. I have a commit you can cherry-pick at 237e78d if you are happy with it. Did a few extra renames and stuff to try to more clearly disambiguate.

Then i am happy to send this through :-)

LGTM :) thank you !

@clux clux merged commit e9094ca into kube-rs:main Mar 22, 2024
17 checks passed
@clux clux added this to the 0.89.0 milestone Mar 22, 2024
@XciD
Copy link
Contributor Author

XciD commented Mar 22, 2024

🙏

BTW, don't know if you saw, we sponsored you because your library is really cool. Great job !

@clux
Copy link
Member

clux commented Mar 22, 2024

Ah, I assumed that might have been parts of your doing. Thanks a lot! 🙇

Feel free to add yourselves to kube.rs/adopters when/if you feel comfortable with that.

and thanks again for the PRs!

@clux clux changed the title feat: direct node access logs/portforward/exec/attach feat: direct node access logs/portforward/exec/attach via kubelet debug interface Mar 25, 2024
@clux clux changed the title feat: direct node access logs/portforward/exec/attach via kubelet debug interface direct node access logs/portforward/exec/attach via kubelet debug interface Mar 25, 2024
@XciD XciD deleted the direct-node branch April 30, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access Kubelet exec/logs/portforward endpoints
2 participants