disable-multi-stage-tls #14275
disable-multi-stage-tls #14275
Conversation
yashmayya
left a comment
yashmayya
left a comment
There was a problem hiding this comment.
@gortiz any particular reason why we're introducing the config here even though we won't support it being turned on? Why not simply make it off by default and introduce the config when the issue with the dispatch client is fixed?
pinot-server/src/main/java/org/apache/pinot/server/starter/ServerInstance.java
Outdated
Show resolved
Hide resolved
…verInstance.java Co-authored-by: Yash Mayya <[email protected]>
We could just rollback #13645, but I think most of the changes from @anandheritage can be kept. It is just a matter of enabling TLS in the client side. What we are also going to need for sure is a property to enable/disable TLS between broker and server in multi-stage. But maybe you are right, just disable it would be better. Mainly because configs are pretty complex right now and my property would also require to add a similar (but slightly different) property for brokers in future. Therefore I'm going to approve the rollback from @soumitra-st (#14259) and then we can implement TLS with more time to think about how to configure it easily. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14275 +/- ##
============================================
+ Coverage 61.75% 63.72% +1.97%
- Complexity 207 1536 +1329
============================================
Files 2436 2628 +192
Lines 133233 145004 +11771
Branches 20636 22188 +1552
============================================
+ Hits 82274 92400 +10126
- Misses 44911 45778 +867
- Partials 6048 6826 +778
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3 participants
Pinot 1.2.0 doesn't support TLS connections between multi-stage nodes. PR #13645 tried fix that, but there were a couple of issues in the implementation. Given it is difficult to test TLS without an actual environment, that PR was merged without testing it. That PR is partial in the sense that query servers are enabled with TLS but it is not enabled in query clients (basically the brokers).
In general we tend to add new flags for new features whenever is possible to minimize the impact on deployment and in case there is an issue be able to disable the new feature. But the #13645 enabled the feature as long as TLS was configured for single-stage queries. As a result, when a cluster where TLS is enabled is started with the code in master, multi-stage queries fail because servers try to connect without TLS and while servers ask for it.
This PR doesn't fix the original problem but:
The reason to not fix the issue right now is the lack of time to test it properly. Instead we need to fix the regression in our test deployments.