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

[lhotari]

.Values.tls.proxy.enableTlsWithBroker:

pulsar-helm-chart/helm-chart-sources/pulsar/values.yaml

Lines 200 to 202 in 08fde0f

	proxy:
	# Applies to connections to standalone function worker, too.
	enableTlsWithBroker: false

.Values.broker.enableForProxyToBroker:

pulsar-helm-chart/helm-chart-sources/pulsar/values.yaml

Lines 214 to 215 in 08fde0f

	broker:
	enableForProxyToBroker: false

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]

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 #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.

pulsar-helm-chart/helm-chart-sources/pulsar/values.yaml

Lines 178 to 181 in 08fde0f

	## TLS is enabled for the function worker, broker, and proxy when enableTls is true. The below <component>.enableTlsWithBroker
	## flags are used to determine whether the component's client should use TLS when connecting to the broker or the
	## function worker. This is an inversion of the paradigm used for the zookeeper and bookkeeper configurations above,
	## which is used to enable TLS for all components interacting with bookkeeper or zookeeper.

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.