WIP: Add support for automatic port-forwarding in Hubble CLI (kubectl) #35353
Conversation
|
Commit fdf4a7c does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>
fdf4a7c to
c3e3d3b
Compare
…-forwarding Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>
…d impl supporting port-forwarding Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>
c3e3d3b to
92c7129
Compare
| "github.com/cilium/cilium/pkg/safeio" | ||
| ) | ||
|
|
||
| // Slim version of: https://github.com/cilium/cilium/tree/v1.17.0-pre.1/cilium-cli/k8s/client.go |
There was a problem hiding this comment.
Can refactor the Cilium CLI and Hubble CLI to share common code for the port-forwarding and service retrieval? Then in a follow up PR we can switch both to "native" port-forwarding instead of exec.
There was a problem hiding this comment.
That works yes. Looking through the repo, seems cilium/pkg/hubble would be a good place to put this common logic (something like func RelayPortForward(...)), and have a small k8sclient interface for GetService there, so each client can use its own k8s client (I don't think we could re-use the same client as cilium-cli without tying ourselves to cilium-isms/helm configs/etc).
While at it, I could also update UIPortForward in hubble to re-use that as well instead of hardcoding port 80.
wdyt ?
There was a problem hiding this comment.
Hmm. ideally we could share the implementation for getting services, and eventually the implementation for ProxyTCP. I think it probably isn't a big deal, most of the logic in cilium-cli/k8s looks like it isn't too specific to Cilium CLI. There is quite a bit of logic we aren't going to use, but I don't think that's too big of a deal, the same is true for client-go in general. The question is how much work is it going to be to move that package somewhere else, and where to move it. Maybe we can get input from @kaworu or @rolinh on this.
Additionally, it seems like a good portion of Cilium CLI is already using "native" (non-exec) based port forwarding, it's just cilium port-forward that isn't using it, funnily enough. Sysdump uses native port-forward for a bunch of stuff already it looks like. So we could actually even switch to native port-forwarding for this initial PR I think without it being a big deal.
There was a problem hiding this comment.
Also @michi-covalent maybe you know about sharing the k8s related code between cilium CLI and Hubble CLI.
There was a problem hiding this comment.
Thanks for the feedback. I would not be against doing that work in this PR if we are comfortable with that. As you mentioned, it depends if we want to tackle moving the k8s client outside cilium-cli. It could also be its own PR if we want this to be self-contained.
Let's see what others think, and in the meantime, I'll take a look at the work needed to move the k8s client.
|
Lets follow-up in this alternative PR where I implemented this using native port-forwarding instead: #35483 |
Description
hubble: Implement automatic port-forwarding capability (opt-in)
Adds new server flag (and friends): --auto-port-forward that launches a kubectl port-forward process.
Re-use same worflow as
cilium hubble port-forwardas intial impl and consider re-placing both with native k8s client tcp port-forwarding in a subsequent change.Fixes: #34935
Release Notes
Checklist
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.