Support broker grpc connection with auth#16170
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds authentication support for broker gRPC connections, enabling secure communication between clients and Pinot brokers over gRPC.
- Extended authentication framework to handle gRPC metadata in brokers
- Enhanced JDBC gRPC connection to support case-insensitive headers and auth injection
- Updated examples and quickstart with gRPC auth scenarios and new grpc port property
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pinot-tools/src/main/java/org/apache/pinot/tools/AuthQuickstart.java | Added pinot.broker.grpc.port property for quickstart config |
| pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/grpc/PinotGrpcConnection.java | Updated metadata matching loop and added DriverUtils.handleAuth |
| pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/examples/PinotJdbcExample.java | Added overloads and examples for JDBC gRPC authentication |
| pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java | Extended identity check and token extraction for gRPC |
Comments suppressed due to low confidence (3)
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/grpc/PinotGrpcConnection.java:92
- Consider adding unit tests to verify that
handleAuthcorrectly injects authentication metadata and handles case-insensitive header keys in the gRPC connection.
DriverUtils.handleAuth(properties, _metadataMap);
pinot-tools/src/main/java/org/apache/pinot/tools/AuthQuickstart.java:67
- Add a comment or update the quickstart README to explain the new
pinot.broker.grpc.portsetting so users know why and how to override it.
properties.put("pinot.broker.grpc.port", "8010");
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/grpc/PinotGrpcConnection.java:61
- Introducing a checked
throws SQLExceptionin this constructor changes the public API signature. Consider handling the exception internally or documenting this breaking change for downstream consumers.
throws SQLException {
...ients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/examples/PinotJdbcExample.java
Outdated
Show resolved
Hide resolved
| requesterIdentity instanceof HttpRequesterIdentity || requesterIdentity instanceof GrpcRequesterIdentity, | ||
| "BasicAuthAccessControl only supports HttpRequesterIdentity or GrpcRequesterIdentity, got %s", | ||
| requesterIdentity.getClass().getName()); | ||
| Collection<String> tokens = null; |
There was a problem hiding this comment.
[nitpick] There is duplicated logic for extracting HTTP and gRPC tokens; consider refactoring into a shared helper to simplify getPrincipalOpt and reduce code duplication.
bf384d2 to
89ad6d2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16170 +/- ##
============================================
+ Coverage 62.90% 63.18% +0.28%
+ Complexity 1386 1357 -29
============================================
Files 2867 2953 +86
Lines 163354 169951 +6597
Branches 24952 25997 +1045
============================================
+ Hits 102755 107382 +4627
- Misses 52847 54449 +1602
- Partials 7752 8120 +368
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
89ad6d2 to
659ae38
Compare
This PR adds authentication support for broker gRPC connections, enabling secure communication between clients and Pinot brokers over gRPC protocol.