feat(plugin-pinot): Add TLS support for self-signed certificate by Dilli-Babu-Godari · Pull Request #26151 · prestodb/presto

Dilli-Babu-Godari · 2025-09-25T16:12:10Z

Description

Added TLS support for Pinot for self-signed certificate. Used the existing TLS config properties to set truststore path and password.

Co-authored-by: Dilli-Babu-Godari godari728@gmail.com
Co-authored-by: Nishitha-Bhaskaran nishithakbhaskaran@gmail.com
Co-authored-by: lukmanulhakkeem lukmanul.hakkem.a@ibm.com
Co-authored-by: Reetika Agrawal agrawal.reetika786@gmail.com

Motivation and Context

Impact

Test Plan

Tested in local.

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Pinot Connector Changes
* Add TLS support for self-signed certificate.

linux-foundation-easycla · 2025-09-25T16:12:15Z

The committers listed above are authorized under a signed CLA.

✅ login: Dilli-Babu-Godari / name: Dilli-Babu-Godari (f87ba7c)

sourcery-ai · 2025-09-25T16:12:22Z

Reviewer's Guide

This PR adds end-to-end TLS support for Pinot by introducing a grpc-protobuf dependency, configuring a secure JettyHttpClient with truststore options, and updating authentication headers to use the Basic scheme for all HTTP calls.

File-Level Changes

Change	Details	Files
Add grpc-protobuf dependency for TLS support	Added io.grpc:grpc-protobuf runtime dependency Excluded conflicting artifacts (common-protos, error_prone, animal-sniffer, protobuf-java, grpc-netty-shaded, grpc-core, grpc-api)	`presto-pinot/pom.xml`
Configure secure HttpClient in PinotClusterInfoFetcher	Introduced HttpClientConfig for trustStore settings Conditionally instantiate JettyHttpClient when useSecureConnection is true and trustStore path is provided	`presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotClusterInfoFetcher.java`
Update authentication header to include Basic scheme	Prefixed controller authentication header with 'Basic ' Prefixed broker authentication header with 'Basic '	`presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotClusterInfoFetcher.java`
Prefix Basic scheme for broker query authentication	Added 'Basic ' prefix to brokerAuthenticationProvider token in PinotBrokerPageSource	`presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotBrokerPageSource.java`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

Refactor HttpClient creation in PinotClusterInfoFetcher to accept a pre-configured client via injection or factory instead of instantiating JettyHttpClient inline with a hardcoded name and null parameters for better configurability and testability.
Consolidate AUTHORIZATION header logic to avoid setting it twice and ensure the "Basic " prefix is applied consistently across all controller, broker GET, and POST requests.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Refactor HttpClient creation in PinotClusterInfoFetcher to accept a pre-configured client via injection or factory instead of instantiating JettyHttpClient inline with a hardcoded name and null parameters for better configurability and testability.
- Consolidate AUTHORIZATION header logic to avoid setting it twice and ensure the "Basic " prefix is applied consistently across all controller, broker GET, and POST requests.

## Individual Comments

### Comment 1
<location> `presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotBrokerPageSource.java:349-350` </location>
<code_context>
             Request.Builder builder = Request.Builder
                     .preparePost()
                     .setUri(URI.create(String.format(QUERY_URL_TEMPLATE, pinotConfig.isUseSecureConnection() ? "https" : "http", queryHost)));
+            brokerAuthenticationProvider.getAuthenticationToken(session).ifPresent(token -> builder.setHeader(AUTHORIZATION, "Basic" + " " + token));
             brokerAuthenticationProvider.getAuthenticationToken(session).ifPresent(token -> builder.setHeader(AUTHORIZATION, token));
             String body = clusterInfoFetcher.doHttpActionWithHeaders(builder, Optional.of(getRequestPayload(pinotQuery)), rpcService);
             return populateFromQueryResults(pinotQuery, blockBuilders, types, body);
</code_context>

<issue_to_address>
**issue:** Duplicate header setting for AUTHORIZATION may cause unexpected behavior.

The second assignment will override the first. Remove one to prevent ambiguity and ensure the correct header value is used.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotBrokerPageSource.java

github-actions · 2025-10-06T07:25:20Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6616215...f87ba7c.

No notifications.

steveburnett · 2025-10-06T15:43:57Z

Thanks for the release note! Nits.

== RELEASE NOTES ==

General Changes
* Add TLS support for Pinot.

ShahimSharafudeen · 2025-10-30T04:53:13Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotClusterInfoFetcher.java

        this.timeBoundaryJsonCodec = requireNonNull(timeBoundaryJsonCodec, "time boundary json codec is null");
        this.instanceJsonCodec = requireNonNull(instanceJsonCodec, "instance json codec is null");
+        HttpClientConfig config = new HttpClientConfig();
+        if (pinotConfig.isUseSecureConnection()) {


Please optimize the code to avoid duplicating the else condition. just like below

if (pinotConfig.isUseSecureConnection() && pinotConfig.getGrpcTlsTrustStorePath() != null && !pinotConfig.getGrpcTlsTrustStorePath().isEmpty()) { config.setTrustStorePath(pinotConfig.getGrpcTlsTrustStorePath()); config.setTrustStorePassword(pinotConfig.getGrpcTlsTrustStorePassword()); this.httpClient = new JettyHttpClient("myclient", config, null, Collections.emptyList()); } else { this.httpClient = requireNonNull(httpClient, "httpClient is null"); }

Instead of creating new httpClient here, added these configs during the initial binding time

ShahimSharafudeen · 2025-10-30T04:56:27Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotClusterInfoFetcher.java

                .build();
        Request.Builder builder = Request.builder().prepareGet().setUri(controllerPathUri);
-        controllerAuthenticationProvider.getAuthenticationToken().ifPresent(token -> builder.setHeader(AUTHORIZATION, token));
+        controllerAuthenticationProvider.getAuthenticationToken().ifPresent(token -> builder.setHeader(AUTHORIZATION, "Basic" + " " + token));


Why did you change the Authorization header value from token to "Basic" + " " + token?

It will fix the issue "HTTP protocol violation: Authentication challenge without WWW-Authenticate header"

ShahimSharafudeen · 2025-10-30T05:06:32Z

presto-pinot/pom.xml

+        <!--  GRPC protobuf-->
+        <dependency>
+            <groupId>io.grpc</groupId>
+            <artifactId>grpc-protobuf</artifactId>


Is this runtime dependency relevant for this change?

prestodb-ci · 2025-11-19T18:12:40Z

@agrawalreetika imported this issue as lakehouse/presto #26151

steveburnett · 2026-01-06T19:15:35Z

Do we need any changes to the documentation in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/pinot.rst#catalog-properties ?

nishithakbhaskaran · 2026-01-07T09:19:55Z

Do we need any changes to the documentation in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/pinot.rst#catalog-properties ?

No new properties are added as per this PR. So changes to this doc file is not requried.

ShahimSharafudeen

Thanks for the clarifications. LGTM.

hantangwangd

@Dilli-Babu-Godari, thanks for this feature. Just one little thing, otherwise looks good!

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotBrokerPageSource.java

Co-authored-by: Nishitha-Bhaskaran <nishithakbhaskaran@gmail.com> Co-authored-by: Lukmanul-hakkeem-A <lukmanul.hakkem.a@ibm.com> Co-authored-by: Reetika Agrawal <agrawal.reetika786@gmail.com>

nishithakbhaskaran · 2026-01-16T15:29:04Z

@Dilli-Babu-Godari, thanks for this feature. Just one little thing, otherwise looks good!

It was a rebase conflict mistake.Updated the code. Thanks @hantangwangd for pointing this out.

hantangwangd

Thanks @nishithakbhaskaran, lgtm!

Dilli-Babu-Godari requested a review from a team as a code owner September 25, 2025 16:12

prestodb-ci added the from:IBM PR from IBM label Sep 25, 2025

prestodb-ci requested a review from a team September 25, 2025 16:12

prestodb-ci requested review from ShahimSharafudeen and anandamideShakyan and removed request for a team September 25, 2025 16:12

Dilli-Babu-Godari force-pushed the add_tls_pinot branch from 68953c0 to a9765f5 Compare September 25, 2025 16:13

sourcery-ai bot reviewed Sep 25, 2025

View reviewed changes

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotBrokerPageSource.java Outdated Show resolved Hide resolved

Dilli-Babu-Godari force-pushed the add_tls_pinot branch 2 times, most recently from ae654e8 to bfa2f50 Compare October 6, 2025 07:16

Dilli-Babu-Godari changed the title ~~Added TLS support for Pinot~~ feat(pinot): Add TLS support Oct 6, 2025

Dilli-Babu-Godari force-pushed the add_tls_pinot branch from bfa2f50 to 59e5074 Compare October 9, 2025 05:59

Dilli-Babu-Godari force-pushed the add_tls_pinot branch from 59e5074 to e52b157 Compare October 29, 2025 15:19

Dilli-Babu-Godari changed the title ~~feat(pinot): Add TLS support~~ feat(plugin-pinot): Add TLS support Oct 29, 2025

Dilli-Babu-Godari force-pushed the add_tls_pinot branch 2 times, most recently from 5a778db to ed5de8a Compare October 29, 2025 18:16

Dilli-Babu-Godari requested a review from pratyakshsharma October 30, 2025 04:42

ShahimSharafudeen reviewed Oct 30, 2025

View reviewed changes

imsayari404 mentioned this pull request Nov 26, 2025

fix(security): Upgrade pinot to version 1.40 and Override vulnerable lz4-java dependency #26684

Merged

7 tasks

nishithakbhaskaran force-pushed the add_tls_pinot branch 3 times, most recently from 71bc01a to cf0b85c Compare December 10, 2025 05:27

nishithakbhaskaran requested a review from ShahimSharafudeen December 10, 2025 08:27

nishithakbhaskaran changed the title ~~feat(plugin-pinot): Add TLS support~~ feat(plugin-pinot): Add TLS support for self-signed certificate Dec 18, 2025

ShahimSharafudeen approved these changes Jan 7, 2026

View reviewed changes

nishithakbhaskaran force-pushed the add_tls_pinot branch 2 times, most recently from 7a98092 to 370f49e Compare January 9, 2026 06:41

hantangwangd reviewed Jan 16, 2026

View reviewed changes

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotBrokerPageSource.java Outdated Show resolved Hide resolved

Added TLS support for Pinot

f87ba7c

Co-authored-by: Nishitha-Bhaskaran <nishithakbhaskaran@gmail.com> Co-authored-by: Lukmanul-hakkeem-A <lukmanul.hakkem.a@ibm.com> Co-authored-by: Reetika Agrawal <agrawal.reetika786@gmail.com>

nishithakbhaskaran force-pushed the add_tls_pinot branch from cd5b2c9 to f87ba7c Compare January 16, 2026 15:27

nishithakbhaskaran requested a review from hantangwangd January 16, 2026 15:30

hantangwangd approved these changes Jan 16, 2026

View reviewed changes

nishithakbhaskaran merged commit b3968a0 into prestodb:master Jan 16, 2026
87 of 89 checks passed

nishithakbhaskaran deleted the add_tls_pinot branch January 19, 2026 05:40

Conversation

Dilli-Babu-Godari commented Sep 25, 2025 • edited by nishithakbhaskaran Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

linux-foundation-easycla bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcery-ai bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett commented Oct 6, 2025

Uh oh!

ShahimSharafudeen Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

nishithakbhaskaran Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

ShahimSharafudeen Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

nishithakbhaskaran Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

ShahimSharafudeen Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

nishithakbhaskaran Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

prestodb-ci commented Nov 19, 2025

Uh oh!

steveburnett commented Jan 6, 2026

Uh oh!

nishithakbhaskaran commented Jan 7, 2026

Uh oh!

ShahimSharafudeen left a comment

Choose a reason for hiding this comment

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nishithakbhaskaran commented Jan 16, 2026

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dilli-Babu-Godari commented Sep 25, 2025 •

edited by nishithakbhaskaran

Loading

linux-foundation-easycla bot commented Sep 25, 2025 •

edited

Loading

sourcery-ai bot commented Sep 25, 2025 •

edited

Loading

github-actions bot commented Oct 6, 2025 •

edited

Loading