search

datastax / pulsar-helm-chart

Apache Pulsar Helm chart
Apache License 2.0
47 stars 38 forks source link

Configuration .Values.tls.proxy.enableTlsWithBroker and .Values.broker.enableForProxyToBroker are conflicting #193

Open lhotari opened 2 years ago

lhotari commented 2 years ago

.Values.tls.proxy.enableTlsWithBroker: https://github.com/datastax/pulsar-helm-chart/blob/08fde0f3c6e34077ba8d95a96888116d0949fc89/helm-chart-sources/pulsar/values.yaml#L200-L202

.Values.broker.enableForProxyToBroker: https://github.com/datastax/pulsar-helm-chart/blob/08fde0f3c6e34077ba8d95a96888116d0949fc89/helm-chart-sources/pulsar/values.yaml#L214-L215

Similar problem seems to exist with .Values.tls.function.enableTlsWithBroker and .Values.tls.broker.enableForFunctionWorkerToBroker.

Wouldn't it make sense to have .Values.tls.broker.enabled instead?

michaeljmarshall commented 2 years ago

Good question. .Values.tls.broker.enableForProxyToBroker and .Values.tls.broker.enableForFunctionWorkerToBroker were introduced in #172, but they've never been used. I added them by accident, and I have a PR to remove them here https://github.com/datastax/pulsar-helm-chart/pull/195. The config for .Values.tls.proxy.enableTlsWithBroker and .Values.tls.function.enableTlsWithBroker are used. .Values.tls.proxy.enableTlsWithBroker is poorly named because it also determines the type of connection to the function worker, too.

Wouldn't it make sense to have .Values.tls.broker.enabled instead?

I considered this design when writing #169, #170, and #172. The primary reason I didn't use it is because I was worried it'd be a bit ambiguous what was being enabled (TLS is currently enabled on the broker via enableTls). However, in revisiting the logic, I am not sure that I agree with it anymore. I think your solution is good, but the ideal solution is probably even simpler. We really have two TLS features: enable TLS on inbound connections to the proxy (and maybe the broker/function worker?) or enable TLS for all component networking. The current chart is too configurable. For example, I don't know of a use case that would require TLS for bookkeeper connections but not for zookeeeper connections.

For more context, I seem to have noticed the logical inversion based on the below comment. For some reason (I don't remember why), I didn't view .Values.tls.broker.enabled as a good alternative to prevent this inversion.

https://github.com/datastax/pulsar-helm-chart/blob/08fde0f3c6e34077ba8d95a96888116d0949fc89/helm-chart-sources/pulsar/values.yaml#L178-L181

Moving forward, I think we should prepare for a 3.0 release and try to greatly simplify the TLS configuration while making a few breaking changes by ignoring certain configs that make the chart too configurable.