hello team 👋 I’ve been exploring https for the query console. Learned a lot about Grizzly server. I was unable able to get Grizzly to load certificates in its keystores with default jvm -Djavax.net.ssl.keyStore properties. I even ended up building a SSLEngineConfigurator with the flag that explicitly mentions loading configuration from System.properties(), but without luck. So I ended up customizing

ControllerConf

to provide explicit tls properties to feed a custom  SSLEngineConfigurator`. Long story short, got it working, expect a polished PR. But this introduces a new property namespace for the

ControllerConf

. Behold the properties :

controller.query.console.tls.truststore.path=generated.truststore.jks
controller.query.console.tls.truststore.password=tG6p920GWB5AwXN4QLUoFRMriUCcNN
controller.query.console.tls.keystore.path=generated.keystore.jks
controller.query.console.tls.keystore.password=tG6p920GWB5AwXN4QLUoFRMriUCcNN

Do you have a PR?

Soon

Thanks @User. @User /@User is this how we support https in LinkedIn?

At lnkd, we don't use grizzly server, it is the MP port with container jetty

I see

I was exploring to add https on the grizzly server based endpoint sometime back, but didn't follow through. Would be nice to have this feature.

And of course… update the docs

@User I spoke too soon on the https support at Lnkd for controller. My comment applies to pinot-broker. For controller, there may be another solution like nginx proxy for terminating ssl. Even so, would be good to have native https support.

Yeah, the current ControllerConf clearly is designed for a LB termination. The config allows to configure Swagger with https but the actual server ignores that setting.

@User Native https support is nice to have, thanks. Please make it configurable so that we can have both ssl and plain text (in different ports, of course). Question: Why https only for query console? Why not for all of the metadata as well?

Btw, here is a contribution (long back, never merged) on https on the broker: https://github.com/apache/incubator-pinot/pull/3675

I want to TLS all the things!

But baby steps 😛

thank you for the reference! I will educate myself on it

thanks @User

Afternoon! What’s the overall team approach for breaking changes on configuration properties between MINOR pre-1.0.0 versions? If I add configurable http listeners, I would like to refactor the

controller.port

configuration to add explicit notions of listeners such as

controller.http.enable/port

and

controller.https.enable/port

. My take would be to deprecate then decomission. 0.5.0 would accept both and provides warning until 0.6.0 requires the listener configs (with default values of course). @User Thoughs?

While open source pinot is still in minor release version, LinkedIn has deployed pinot on multiple site-facing use cases, serving 100s of millions of members. Breaking backward compatibility can be bad, and should not be done unless absolutely unavoidable. Also, we should make sure that any new rollouts can be backed out safely (consider the steps: Roll out new code, upgrade config to use new ports, oops ne code found some other bug, so roll it back but still with updated config). So, your proposal of deprecating the old config for 0.5.0 and removing it in 0.6.0 seems to be the way to go. Extra configs should be ignored.

@User ^^

I can keep

controller.port

as a master port configuration that adapts to http and https unless an explicit listeners config map is provided. This would keep backward compatibility to the maximum but introduce some head-scratching dynamic configuration behaviour based on the combination of properties provided.

Agree. keep the existing property(deprecated) and its behavior, add new properties as you suggested and give higher preference to new properties and fallback to old properties. We can remove old properties later

keep controller.port for http. Introduce either one other port for https (preferred) or two new ports -- one for http and one for https (not sure why we want to do this)

I have doubts now.

controller.port

is used to generate a helix instance id in combination with

controller.host

. I assume that a migration path shouldn’t involve changing the instance id. This means that either controller.port is reused for https after http is turned of. Or the instance id generation dynamically picks between the http and https port. If a controller changes his helix id and present itself with the same data dir to ZK, is it a big deal for the data consistency?

The reason I would like to introduce dedicated and explicit configurations for http and https is that it would get the operator to go through the tough process of planing his listeners migration. If I keep the same properties, this opens up a tricky matrix of checks to ensure backward compatibility since the same property would present different behaviors between different versions. With distinct properties, I can keep the keep distinct behaviors based on the configured properties.

I’m actually starting to think that with the new configurable listeners, the helix instance id should be decoupled and defined through it’s own property.

Spot on @User. There is a second-order effect to certain features (specifically, ones related to cross cutting concerns). If we lock ourselves into the backward compatibility fallacy of minimizing risk to LinkedIn deployments, then we're also accepting the fact that the reason for this risk is also what caused it in the first place. I would add contrast to @User's point that breaking backward compatibility should be avoided at all costs. If the platform was built under the right constraints, then there would be less fragility in upgrades. Ultimately it is LinkedIn's responsibility to canary each upgrade so that their rollouts minimize the risk of a widespread impact to users. But more importantly, to @User's point, it's not about breaking changes... the challenge is making the risk an explicit cost to an operator that is performing the upgrade.

Perhaps we are overloading backward compatibility in this context? From the discussion above, it seems that having dedicated new configs (while marking old ones deprecated) was the consensus, and does not break any existing deployments (LinkedIn or otherwise)?

Yes

and offers a long term migration option

@User arguments is from my understanding mostly around not multiplying the numbers of available configurations.