Skip to content

Conversation

@yashmayya
Copy link
Contributor

  • Allow configuring TLS between brokers and servers for the multi-stage engine #14387 added support for configuring TLS between brokers and servers (gRPC query dispatch client in the broker, gRPC query server in the servers) for the multi-stage query engine.
  • However, this wasn't "complete" because the multi-stage engine also involves data shuffle between servers via a mailbox mechanism which also should be secured via TLS when the new config pinot.multistage.engine.tls.enabled is set to true. The mailbox mechanism is also used to stream the final query results from servers to brokers.
  • This patch adds TLS support to the mailboxes as well, behind the same configuration option introduced in the previous patch. The previously added integration test also covers this addition due to the use of the mailbox mechanism for final data exchange between server and broker in stage 0 of a query plan.

@yashmayya yashmayya requested a review from gortiz November 18, 2024 09:24
@yashmayya yashmayya added security multi-stage Related to the multi-stage query engine feature labels Nov 18, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 42.10526% with 33 lines in your changes missing coverage. Please review.

Project coverage is 63.81%. Comparing base (59551e4) to head (992dd99).
Report is 1343 commits behind head on master.

Files with missing lines Patch % Lines
...pache/pinot/common/utils/grpc/GrpcQueryClient.java 0.00% 11 Missing ⚠️
...he/pinot/query/mailbox/channel/ChannelManager.java 56.25% 6 Missing and 1 partial ⚠️
...pinot/query/mailbox/channel/GrpcMailboxServer.java 46.15% 6 Missing and 1 partial ⚠️
...che/pinot/core/transport/grpc/GrpcQueryServer.java 0.00% 5 Missing ⚠️
...requesthandler/MultiStageBrokerRequestHandler.java 66.66% 0 Missing and 1 partial ⚠️
...apache/pinot/query/service/server/QueryServer.java 0.00% 1 Missing ⚠️
.../apache/pinot/server/worker/WorkerQueryServer.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14476      +/-   ##
============================================
+ Coverage     61.75%   63.81%   +2.06%     
- Complexity      207     1567    +1360     
============================================
  Files          2436     2663     +227     
  Lines        133233   146225   +12992     
  Branches      20636    22404    +1768     
============================================
+ Hits          82274    93313   +11039     
- Misses        44911    46022    +1111     
- Partials       6048     6890     +842     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.76% <42.10%> (+2.05%) ⬆️
java-21 63.68% <42.10%> (+2.06%) ⬆️
skip-bytebuffers-false 63.78% <42.10%> (+2.04%) ⬆️
skip-bytebuffers-true 63.66% <42.10%> (+35.93%) ⬆️
temurin 63.81% <42.10%> (+2.06%) ⬆️
unittests 63.81% <42.10%> (+2.06%) ⬆️
unittests1 55.57% <41.50%> (+8.68%) ⬆️
unittests2 34.12% <29.82%> (+6.39%) ⬆️

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.

channelBuilder =
ManagedChannelBuilder.forAddress(host, port).maxInboundMessageSize(config.getMaxInboundMessageSizeBytes())
.usePlaintext();
channelBuilder = ManagedChannelBuilder
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only refactors in this file.

if (tlsConfig != null) {
try {
_server = NettyServerBuilder.forPort(port).sslContext(buildGRpcSslContext(tlsConfig))
_server = NettyServerBuilder.forPort(port).sslContext(buildGrpcSslContext(tlsConfig))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only refactors in this file.

@yashmayya yashmayya merged commit f08c159 into apache:master Nov 19, 2024
21 checks passed
davecromberge pushed a commit to davecromberge/pinot that referenced this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature multi-stage Related to the multi-stage query engine security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants