livekit / livekit-helm

LiveKit Helm charts
https://docs.livekit.io
Apache License 2.0
55 stars 64 forks source link

feat: no load balancer fallback to cluster ip #53

Closed zifeo closed 1 year ago

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

davidzhao commented 2 years ago

Can you describe why this is needed? Which Kubernetes engine requires cluster IP?

zifeo commented 2 years ago

@davidzhao I am using RKE2 k8s distribution with ingress-nginx to:

The current nodeport service is not needed for this setup and can cause some troubles on small cluster deployment. I am currently adding a custom service to manage my use case, however I believe this use case may be worth supporting upstream.

davidzhao commented 2 years ago

@davidzhao I am using RKE2 k8s distribution with ingress-nginx to:

  • expose tcp and single udp port
  • get tls on web socket with cert-manager

The current nodeport service is not needed for this setup and can cause some troubles on small cluster deployment. I am currently adding a custom service to manage my use case, however I believe this use case may be worth supporting upstream.

The default Service type is already ClusterIP (kube docs). isn't your change asking for the same behavior?

zifeo commented 2 years ago

@davidzhao Good catch! And I saw that missed most of the patches I wanted to submit. I roll-back to default and added the missing elements.

zifeo commented 2 years ago

@davidzhao Can you please review again?

davidzhao commented 2 years ago

I'm traveling this week, will review when I get back on Sunday.

zifeo commented 1 year ago

@davidzhao You are hopefully back now :) ?

davidzhao commented 1 year ago

My bad, dropped the ball on this. will have a look this week!

davidzhao commented 1 year ago

looks like the PR has been changed to enable usage without host networking and instead rely on single-port udp mode?

We do not recommend using single port UDP in production, as it has bottlenecks wrt performance. Also, enabling service port mapping does not seem to be related to the type of load balancer one wants to specify to me. Could you explain the motivation for that?

zifeo commented 1 year ago

@davidzhao I know it is not advised, but this setup is the only that can work when there is no load balancer available within the cluster (e.g. only a single one, locked with an ingress). Therefore I believe that you should also support this option, especially for small deployment that do not require a perfect performance.

zifeo commented 1 year ago

@davidzhao Can you please merge this or shall I create a fork in that use cases supported?