[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6415: Client-Broker TLS support

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552936366



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +64,51 @@ protected void configure() {
     registerClasses(io.swagger.jaxrs.listing.SwaggerSerializers.class);
   }
 
-  public void start(int httpPort) {
-    Preconditions.checkArgument(httpPort > 0);
-    _baseUri = URI.create("http://0.0.0.0:" + httpPort + "/");
-    _httpServer = GrizzlyHttpServerFactory.createHttpServer(_baseUri, this);
+  public void start(PinotConfiguration brokerConf) {
+    int brokerQueryPort = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT,
+        CommonConstants.Helix.DEFAULT_BROKER_QUERY_PORT);
+    String brokerQueryProtocol = brokerConf.getProperty(CONFIG_OF_BROKER_CLIENT_PROTOCOL,

Review comment:
       Validate the protocol? We can only support HTTP and HTTPS

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -27,14 +27,20 @@
 import org.apache.pinot.broker.routing.RoutingManager;
 import org.apache.pinot.common.metrics.BrokerMetrics;
 import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.spi.env.PinotConfiguration;
 import org.glassfish.grizzly.http.server.CLStaticHttpHandler;
 import org.glassfish.grizzly.http.server.HttpHandler;
 import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
 import org.glassfish.hk2.utilities.binding.AbstractBinder;
 import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
 import org.glassfish.jersey.jackson.JacksonFeature;
 import org.glassfish.jersey.server.ResourceConfig;
 
+import static org.apache.pinot.common.utils.CommonConstants.Broker.*;

Review comment:
       Avoid static imports in production class

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -177,6 +177,15 @@
     public static final String CONFIG_OF_BROKER_GROUPBY_TRIM_THRESHOLD = "pinot.broker.groupby.trim.threshold";
     public static final int DEFAULT_BROKER_GROUPBY_TRIM_THRESHOLD = 1_000_000;
 
+    public static final String CONFIG_OF_BROKER_CLIENT_PROTOCOL = "pinot.broker.client.protocol";
+    public static final String DEFAULT_BROKER_CLIENT_PROTOCOL = "http";
+    public static final String CONFIG_OF_BROKER_CLIENT_TLS_KEYSTORE_PATH = "pinot.broker.client.tls.keystore.path";
+    public static final String CONFIG_OF_BROKER_CLIENT_TLS_KEYSTORE_PASSWORD = "pinot.broker.client.tls.keystore.password";
+    public static final String CONFIG_OF_BROKER_CLIENT_TLS_TRUSTSTORE_PATH = "pinot.broker.client.tls.truststore.path";
+    public static final String CONFIG_OF_BROKER_CLIENT_TLS_TRUSTSTORE_PASSWORD = "pinot.broker.client.tls.truststore.password";
+    public static final String CONFIG_OF_BROKER_CLIENT_TLS_REQUIRES_CLIENT_AUTH = "pinot.broker.client.tls.requires_client_auth";

Review comment:
       Avoid `_` in the config key, use `.` as the separator

##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java
##########
@@ -82,6 +86,11 @@ public PostQueryCommand setBrokerPort(String port) {
     return this;
   }
 
+  public PostQueryCommand setBrokerProtocol(String protocol) {
+    _brokerProtocol = protocol;

Review comment:
       Validate the protocol

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -356,6 +361,14 @@ public String getControllerVipProtocol() {
         .orElse(CommonConstants.HTTP_PROTOCOL);
   }
 
+  public String getControllerBrokerProtocol() {
+    return Optional.ofNullable(getProperty(CONTROLLER_BROKER_PROTOCOL))

Review comment:
       Validate the protocol?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org