One suggestion of where to put the common library might be under pkg/k8s/common? Thus keeping all the k8s related code in one place?

One suggestion of where to put the common library might be under pkg/k8s/common? Thus keeping all the k8s related code in one place?

That's fair. My first inclination was to put it under pkg/k8s but I wasn't sure how exactly, especially without clashing with the existing client generated package. I also fear pkg/k8s/common/k8s would be confusing ?

Any suggestion on a path/package rename ?

I also fear pkg/k8s/common/k8s would be confusing ?

yeah that would be a bit redundant.

One thing I'm wondering is the code in this library is that this code isn't supposed to be a general k8s client library used by the agent daemon, but rather is more specific to things like cilium/hubble cli.

Maybe k8s is just not a good package name for this anymore following the cli being moved to cilium/cilium?

Anyway I don't feel that strongly about this, so I'll approve for sig-k8s. Maybe one of the other reviewers will have some thoughts.

I also fear pkg/k8s/common/k8s would be confusing ?

yeah that would be a bit redundant.

One thing I'm wondering is the code in this library is that this code isn't supposed to be a general k8s client library used by the agent daemon, but rather is more specific to things like cilium/hubble cli.

Maybe k8s is just not a good package name for this anymore following the cli being moved to cilium/cilium?

Anyway I don't feel that strongly about this, so I'll approve for sig-k8s. Maybe one of the other reviewers will have some thoughts.

I agree that this package is more a high-level k8s-related helper client and therefore k8s is probably not the best name.
On the other hand, the refactor following a package rename will be much larger.

If someone else can propose a better alternative, I'd be happy to make the change. Otherwise we could wing it for now and revisit later if desired.

Seems reasonable. One alternative name could be pkg/cli (for packages used by the CLIs) since pkg/common seems a bit overly generic

I like it. Let me update the PR to reflect the change to use: pkg/cli/k8s as the new location.

I'm not completely convinced yet. The cilium-cli client still is pretty well coupled to cilium-cli --

• no API stability guarantees
• embedded names, e.g. FieldManager in ApplyGeneric
• a separate Helm driver

Plus it's all a bit heavyweight.

If the goal is a functional port-forwarding implementation, then let's extract that, and have it take a k8s.ClientSet instead.

I'm not completely convinced yet. The cilium-cli client still is pretty well coupled to cilium-cli --

• no API stability guarantees
• embedded names, e.g. FieldManager in ApplyGeneric
• a separate Helm driver

Plus it's all a bit heavyweight.

If the goal is a functional port-forwarding implementation, then let's extract that, and have it take a k8s.ClientSet instead.

I think that's fair @squeed.

In the following branch I moved the port-forwarding logic that already exists on the client into a new PortForwarder type under pkg/k8s, and re-used it in the cilium-cli client and hubble: https://github.com/cilium/cilium/compare/main...devodev:pr/devodev/hubble-auto-port-forward-native3?expand=1

Let me know what you think. If this is more inline with what you had in mind, and others agree, I can move on into rebasing the other PR and closing this one.

Thanks for the review!

Closing in favour of introducing a new PortForwarder type that takes in a Kubernetes clientset.
Will update the linked PR shortly.