DandyDeveloper / charts

Various helm charts migrated from [helm/stable] due to deprecation
https://dandydeveloper.github.io/charts
Apache License 2.0
157 stars 145 forks source link

[chart/redis-ha][Make redis service actually have a ClusterIP address, not to be headless always] #240

Open lord-kyron opened 1 year ago

lord-kyron commented 1 year ago

Is your feature request related to a problem? Please describe. We need the option to choose from the values if the main Redis cluster service is headless (as by default now) or actually has a ClusterIP address. Or even, make it possible to choose whether we want it to be ClusterIP, LoadBalancer or NodePort type.

Describe the solution you'd like We would like the option for example: service: type: ClusterIP / Headless to be configurable in the helm chart values and exposed to users to choose.

Describe alternatives you've considered Or expose the whole service configuration to be configured as values in the values.yaml and parsed to the service template directly as yaml config.

Additional context We need this feature, as sometimes we need outside services to connect and use the redis service inside k8s cluster and when the service is headless, we actually don't have IP to provide them (if don't use ingress or some other service mesh exposed to outside)

lord-kyron commented 1 year ago

@DandyDeveloper what do you think?

DandyDeveloper commented 1 year ago

@lord-kyron Yes, it's pretty common place to allow the service to be overwritten. No complaints here.

The only thing I don't remember, do the endpointslice objects still get populated with the pod IP per the selector if its no longer a headless service?

I don't remember if all CNIs behave the same way here?

If its fine and the pods are still resolveable (E.G. redis-ha-0.redis-ha,default.svc.cluster.local) then there's no reason why we can't do this.

lord-kyron commented 1 year ago

@DandyDeveloper I think they get populated with the pod IP always. The only difference here is that the service itself is getting an IP address, while the headless one is not reserving Ip form the services subnet. The same goes for LoadBalancer service type. However, there are some additional options to cove here. If we choose cluster Ip but no headless, we have to able to also choose exact ClusterIP if we want to use exact Ip address. If we choose LoadBalancer type, we have to be able to choose exact clusterIP and loadBalancerIP also. And if the type is NodePort, then in the Ports section we should be able to define the nodePort option.

DandyDeveloper commented 1 year ago

@lord-kyron Yeah we'll just offer the same standardized templating most others do: https://github.com/prometheus-community/helm-charts/blob/main/charts/prometheus/templates/service.yaml

Prometheus example is probably overkill but does showcase how I'd approach this.

lord-kyron commented 1 year ago

@DandyDeveloper is there also a possibility to include an option to avoid deploying the announce services, being created for each pod, and to use PODs IP directly (when this option is turned on)? It can by default have the same behavior as now. We need such, as we are using Hashicorps Consul for service advertising to applications, so when they announce services are also on we are now having the following path: consul -> headless main SVC -> sentinel on some pod -> redirect to announce SVC IP on Redis port -> POD on Redis port We want to simplify this by having an option to choose a straight approach - Consul -> main headless( or ClusterIP) SVC -> Pod on sentinel port -> pod on Redis port