TLS Configuration Support for QueryServer and Dispatch Client #13645

anandheritage · 2024-07-17T16:33:29Z

TLS support for the QueryServer and Dispatch client in Apache Pinot. The changes include:

TLS Configuration for QueryServer:

Addition of TLS settings to enable secure communication for the QueryServer.
Implementation of SSL context building to manage certificates and keys.
Dispatch Client Updates:

Integration of TLS support for the Dispatch client to ensure secure data dispatch.
These enhancements aim to improve the security of inter-service communication within Apache Pinot, aligning with modern security standards.

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java

anandheritage · 2024-07-24T07:21:34Z

@Jackie-Jiang @xiangfu0
Can you enable the workflow here.

codecov-commenter · 2024-07-24T13:05:36Z

Codecov Report

Attention: Patch coverage is 23.52941% with 26 lines in your changes missing coverage. Please review.

Project coverage is 57.91%. Comparing base (59551e4) to head (a5acd61).
Report is 1060 commits behind head on master.

Files with missing lines	Patch %	Lines
...rg/apache/pinot/server/starter/ServerInstance.java	0.00%	12 Missing ⚠️
.../apache/pinot/server/worker/WorkerQueryServer.java	0.00%	6 Missing ⚠️
...e/pinot/query/service/dispatch/DispatchClient.java	50.00%	3 Missing and 1 partial ⚠️
...apache/pinot/query/service/server/QueryServer.java	50.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13645      +/-   ##
============================================
- Coverage     61.75%   57.91%   -3.84%     
- Complexity      207      219      +12     
============================================
  Files          2436     2613     +177     
  Lines        133233   143283   +10050     
  Branches      20636    21998    +1362     
============================================
+ Hits          82274    82987     +713     
- Misses        44911    53804    +8893     
- Partials       6048     6492     +444

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`57.89% <23.52%> (-3.82%)`	⬇️
java-21	`57.77% <23.52%> (-3.86%)`	⬇️
skip-bytebuffers-false	`57.91% <23.52%> (-3.84%)`	⬇️
skip-bytebuffers-true	`57.74% <23.52%> (+30.02%)`	⬆️
temurin	`57.91% <23.52%> (-3.84%)`	⬇️
unittests	`57.91% <23.52%> (-3.84%)`	⬇️
unittests1	`40.73% <50.00%> (-6.16%)`	⬇️
unittests2	`27.97% <0.00%> (+0.24%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anandheritage · 2024-07-30T11:00:39Z

@xiangfu0 @npawar Can you help me get it reviewed ?

pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/GrpcMailboxServer.java

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/DispatchClient.java

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/DispatchClient.java

soumitra-st · 2024-10-19T02:48:09Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/DispatchClient.java

+    this(host, port, new GrpcConfig(Collections.emptyMap()));
+  }
+
+  public DispatchClient(String host, int port, GrpcConfig grpcConfig) {


This constructor is only used in the other constructor to create non-tls DispatchClient. Does it mean that if gRPC TLS is enabled on the server side, then DispatchClient will always fail? I don't see a place where TLS enabled DispatchClient is created. cc: @gortiz @Jackie-Jiang

gortiz · 2024-10-19T08:18:37Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java

@@ -55,16 +58,18 @@ public class QueryServer extends PinotQueryWorkerGrpc.PinotQueryWorkerImplBase {

  private final int _port;
  private final QueryRunner _queryRunner;
+  private final TlsConfig _tlsConfig;


@nullable annotation is needed here

gortiz · 2024-10-19T08:18:46Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java

  // query submission service is only used for plan submission for now.
  // TODO: with complex query submission logic we should allow asynchronous query submission return instead of
  //   directly return from submission response observer.
  private final ExecutorService _querySubmissionExecutorService;

  private Server _server = null;

-  public QueryServer(int port, QueryRunner queryRunner) {
+  public QueryServer(int port, QueryRunner queryRunner, TlsConfig tlsConfig) {


@nullabe annotation is needed here

gortiz · 2024-10-22T13:19:48Z

This PR is buggy. It looks like QueryServer enables TLS when it is enabled in netty config but query dispatcher is never using TLS, so multi-stage query engine ends up not working at all if TLS is enabled in netty.

anandheritage · 2024-10-22T13:30:47Z

@gortiz Can you help me with the changes expected ?

When I had made the changes didn't had the dev env to test.

anandheritage · 2024-10-22T13:37:36Z

Let me know if you want me to revert this for now and maybe we discuss if major changes are required ?

anandheritage changed the title ~~Anandsh/tlsconfig~~ [Draft] TLS config support for QueryServer and Dispatch client Jul 17, 2024

anandheritage mentioned this pull request Jul 17, 2024

Configure QueryServer and GrpcMailboxServer for TLS #11777

Closed

mcvsubbu reviewed Jul 18, 2024

View reviewed changes

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java Outdated Show resolved Hide resolved

mgranderath reviewed Jul 19, 2024

View reviewed changes

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java Outdated Show resolved Hide resolved

mgranderath reviewed Jul 19, 2024

View reviewed changes

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java Outdated Show resolved Hide resolved

anandheritage changed the title ~~[Draft] TLS config support for QueryServer and Dispatch client~~ TLS Configuration Support for QueryServer and Dispatch Client Jul 24, 2024

anandheritage mentioned this pull request Jul 24, 2024

Add TLS in dispatch client #12269

Closed

mgranderath approved these changes Jul 24, 2024

View reviewed changes

xiangfu0 requested review from xiangfu0, Jackie-Jiang and klsince July 26, 2024 04:29

Jackie-Jiang added feature security labels Aug 12, 2024

ankitsultana reviewed Aug 23, 2024

View reviewed changes

anandheritage force-pushed the anandsh/tlsconfig branch from 8f4cfd0 to 336baa8 Compare August 25, 2024 05:37

anandheritage requested a review from ankitsultana August 26, 2024 06:28

anandkrshaw added 10 commits September 14, 2024 08:58

Adding tls support for QueryServer

aa7f666

Adding tls support for QueryServer

cb35968

Improve logging

79880f8

Add server tls config

8c669f9

Remove violation

3040823

Add TLS support to Dispacth client

64c409a

Fix test cases

0a44b73

Fix for review comments

0071294

Fix for review comments

1b4d5b7

Fix for review commenst

d771f61

anandheritage force-pushed the anandsh/tlsconfig branch from 336baa8 to d771f61 Compare September 14, 2024 14:29

anandkrshaw added 5 commits September 14, 2024 20:55

Make the SSL build context as util

d4e7b97

Make the SSL build context as util

22d0f23

Make the SSL build context as util

4915032

Remove Local SSL context

4c5a937

Remove Local SSL context

a5acd61

Jackie-Jiang approved these changes Sep 17, 2024

View reviewed changes

ankitsultana merged commit 21ff6bf into apache:master Sep 18, 2024
21 checks passed

soumitra-st reviewed Oct 19, 2024

View reviewed changes

soumitra-st added a commit to soumitra-st/pinot that referenced this pull request Oct 19, 2024

Reverting apache#13645 PR

b8d42f1

soumitra-st mentioned this pull request Oct 19, 2024

Reverting https://github.com/apache/pinot/pull/13645 PR #14259

Merged

gortiz reviewed Oct 19, 2024

View reviewed changes

gortiz mentioned this pull request Oct 22, 2024

disable-multi-stage-tls #14275

Closed

gortiz pushed a commit that referenced this pull request Oct 22, 2024

Reverting #13645 PR (#14259)

736733d

yashmayya mentioned this pull request Nov 5, 2024

Allow configuring TLS between brokers and servers for the multi-stage engine #14387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS Configuration Support for QueryServer and Dispatch Client #13645

TLS Configuration Support for QueryServer and Dispatch Client #13645

anandheritage commented Jul 17, 2024 •

edited

Loading

anandheritage commented Jul 24, 2024

codecov-commenter commented Jul 24, 2024 •

edited

Loading

anandheritage commented Jul 30, 2024

soumitra-st Oct 19, 2024 •

edited

Loading

gortiz Oct 19, 2024

gortiz Oct 19, 2024

gortiz commented Oct 22, 2024

anandheritage commented Oct 22, 2024

anandheritage commented Oct 22, 2024

TLS Configuration Support for QueryServer and Dispatch Client #13645

TLS Configuration Support for QueryServer and Dispatch Client #13645

Conversation

anandheritage commented Jul 17, 2024 • edited Loading

anandheritage commented Jul 24, 2024

codecov-commenter commented Jul 24, 2024 • edited Loading

Codecov Report

anandheritage commented Jul 30, 2024

soumitra-st Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

gortiz Oct 19, 2024

Choose a reason for hiding this comment

gortiz Oct 19, 2024

Choose a reason for hiding this comment

gortiz commented Oct 22, 2024

anandheritage commented Oct 22, 2024

anandheritage commented Oct 22, 2024

anandheritage commented Jul 17, 2024 •

edited

Loading

codecov-commenter commented Jul 24, 2024 •

edited

Loading

soumitra-st Oct 19, 2024 •

edited

Loading