clastix / kamaji

Kamaji is the Hosted Control Plane Manager for Kubernetes.
https://kamaji.clastix.io
Apache License 2.0
1.14k stars 104 forks source link

Should it be possible to use "HTTP connect" as the mode for Konnectivity #598

Open andrewheberle opened 1 month ago

andrewheberle commented 1 month ago

When adding --mode=http-connect to TenantControlPlane.spec.addons.konnectivity.server.extraArgs this gets overridden to be set to "grpc" here:

https://github.com/clastix/kamaji/blob/7e08b9a7cede820ab4c41f751ac6703d6957e10a/internal/builders/controlplane/konnectivity_server.go#L52

Is "grpc" hard coded for a particular reason? Or is "http-connect" not a good idea to use?

Thanks.

prometherion commented 1 month ago

Don't want to play the Uno reverse card here: why are you trying to setup http connect mode? I guess it could be related to network firewalls but any feedback would be really valuable.

The gRPC one is the same used by other providers and we went with that choice, as well as the documentation from Kubernetes suggests using this mode.

If you don't mind, I'll move this as a Discussion since it's not a bug per se.

andrewheberle commented 1 month ago

I might play a Uno draw four ;)

But the additional context for “why” might have been useful…

The reason was we have a reverse proxy solution for our Tenant Control Plane’s that doesn’t handle gRPC well sometimes (which you may argue is the real problem) so I’d hoped HTTP Connect mode might provide a workable solution, however I’ve not tested this outside of Kamajj so it may be a dead end for my own purposes…but I guess it’s more a question of why gRPC is the only option.

If HTTP Connect is terrible for some reason I’m perfectly fine with this decision of forcing gRPC though.

prometherion commented 1 month ago

I might play a Uno draw four ;)

I'll leave this here.


I remember we worked with @jwitko on 7ac8e5e5 in allowing overriding Konnectivity, as well as API Server, parameters, taking for granted users knew what they were doing.

My main remark is that these kinds of customisations could potentially create issues with Kamaji, and newbie users would blame Kamaji for its instability, even tho it's their "fault".

I'm open tho in supporting the customisations of the konnectivity server, unless there's a well-known and proven best practice in preferring gRPC over HTTP, besides the fact gRPC is based on a binary format which is more performant and robust.

andrewheberle commented 1 month ago

Touché for the Uno reference.

I’m honestly fine if the position is that gRPC is a superior solution so that’s all Kamaji will support…as being opinionated in this regard is no problem.

I don’t know enough about the internals of Kamaji or Konnectivity to comment on the suitability of HTTP Connect as an option, it was more to (hopefully) work around a deficiency in our environment.

I can put tog PR so that option is not explicitly set if that helps?

Then it can be accepted or rejected either way?

prometherion commented 1 month ago

Rather than superior, I'd say easier: I had a quick review of the current code and such a change would require a change in the TenantControlPlane specification, since we need a value parity between the konnectivity server and the EgressSelector file stored as a ConfigMap.

https://github.com/clastix/kamaji/blob/7e08b9a7cede820ab4c41f751ac6703d6957e10a/internal/resources/konnectivity/egress_selector_configuration_resource.go#L98

https://github.com/clastix/kamaji/blob/7e08b9a7cede820ab4c41f751ac6703d6957e10a/internal/builders/controlplane/konnectivity_server.go#L52

Not a big deal, tho, we could provide an enum in the Konnectivity Addon struct we're already using with a default value of GRPC. Contributions are definitely welcome and warmly accepted, thanks for giving a shot to Kamaji!