You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/01/06 19:43:42 UTC

[GitHub] [incubator-pinot] apucher opened a new pull request #6415: Client-Broker TLS support

apucher opened a new pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415


   ## Description
   We add support for TLS-secured client-broker connections, similar to the secure client-controller connection already implemented.
   
   Comprehensive example with modified QuickStart for illustration: https://github.com/apache/incubator-pinot/pull/6413
   
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
   **No**
   
   Does this PR fix a zero-downtime upgrade introduced earlier?
   **No**
   
   Does this PR otherwise need attention when creating release notes?
   **Yes**
   
   ## Release Notes
   Add support for TLS-secured client-broker connections. Broker TLS can be configured using the following new properties:
   - **pinot.broker.client.protocol** (http **OR** https)
   - **pinot.broker.client.tls.keystore.path**
   - **pinot.broker.client.tls.keystore.password**
   - **pinot.broker.client.tls.truststore.path**
   - **pinot.broker.client.tls.truststore.password**
   - **pinot.broker.client.tls.requires_client_auth**
   
   Furthermore, to enable controller-broker relay requests via https, the controller can be configured to use a specific protocol via:
   - **controller.broker.protocol**
   
   ## Documentation
   https://github.com/pinot-contrib/pinot-docs/pull/18
   
   


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552972499



##########
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:
       done




----------------------------------------------------------------
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

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755667310


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=h1) Report
   > Merging [#6415](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=desc) (8d142b1) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.48%`.
   > The diff coverage is `56.62%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6415/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6415      +/-   ##
   ==========================================
   - Coverage   66.44%   64.96%   -1.49%     
   ==========================================
     Files        1075     1318     +243     
     Lines       54773    64190    +9417     
     Branches     8168     9336    +1168     
   ==========================================
   + Hits        36396    41700    +5304     
   - Misses      15700    19519    +3819     
   - Partials     2677     2971     +294     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.96% <56.62%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [...common/config/tuner/NoOpTableTableConfigTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
   | ... and [1152 more](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=footer). Last update [f09de82...8d142b1](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552964054



##########
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:
       fixed

##########
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:
       fixed




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552977865



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +62,60 @@ 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);
+
+    Preconditions.checkArgument(brokerQueryPort > 0);

Review comment:
       fixed

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +62,60 @@ 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);
+
+    Preconditions.checkArgument(brokerQueryPort > 0);
+    _baseUri = URI.create(String.format("%s://0.0.0.0:%d/", getBrokerClientProtocol(brokerConf), brokerQueryPort));
+
+    _httpServer = buildHttpsServer(brokerConf);
     setupSwagger();
   }
 
+  private HttpServer buildHttpsServer(PinotConfiguration brokerConf) {
+    boolean isSecure = CommonConstants.HTTPS_PROTOCOL.equals(getBrokerClientProtocol(brokerConf));
+
+    if (isSecure) {
+      return GrizzlyHttpServerFactory.createHttpServer(_baseUri, this, true, buildSSLConfig(brokerConf));
+    }
+
+    return GrizzlyHttpServerFactory.createHttpServer(_baseUri, this);
+  }
+
+  private SSLEngineConfigurator buildSSLConfig(PinotConfiguration brokerConf) {
+    SSLContextConfigurator sslContextConfigurator = new SSLContextConfigurator();
+
+    sslContextConfigurator.setKeyStoreFile(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_KEYSTORE_PATH));
+    sslContextConfigurator.setKeyStorePass(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_KEYSTORE_PASSWORD));
+    sslContextConfigurator.setTrustStoreFile(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_TRUSTSTORE_PATH));
+    sslContextConfigurator.setTrustStorePass(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_TRUSTSTORE_PASSWORD));
+
+    boolean requiresClientAuth = brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_CLIENT_AUTH,
+        CommonConstants.Broker.DEFAULT_BROKER_CLIENT_TLS_CLIENT_AUTH);
+
+    return new SSLEngineConfigurator(sslContextConfigurator).setClientMode(false)
+        .setWantClientAuth(requiresClientAuth).setEnabledProtocols(new String[] { "TLSv1.2" });
+  }
+
+  private static String getBrokerClientProtocol(PinotConfiguration brokerConf) {
+    return Optional.ofNullable(brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_PROTOCOL))

Review comment:
       fixed




----------------------------------------------------------------
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

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755667310


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=h1) Report
   > Merging [#6415](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=desc) (8d142b1) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.88%`.
   > The diff coverage is `72.67%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6415/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6415      +/-   ##
   ==========================================
   + Coverage   66.44%   73.33%   +6.88%     
   ==========================================
     Files        1075     1318     +243     
     Lines       54773    64190    +9417     
     Branches     8168     9336    +1168     
   ==========================================
   + Hits        36396    47073   +10677     
   + Misses      15700    14059    -1641     
   - Partials     2677     3058     +381     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `44.19% <38.86%> (?)` | |
   | unittests | `64.96% <56.62%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `78.57% <ø> (+5.40%)` | :arrow_up: |
   | [...common/config/tuner/NoOpTableTableConfigTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../common/config/tuner/TableConfigTunerRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1RhYmxlQ29uZmlnVHVuZXJSZWdpc3RyeS5qYXZh) | `72.00% <ø> (ø)` | |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `100.00% <ø> (ø)` | |
   | ... and [1105 more](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=footer). Last update [f09de82...8d142b1](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755667310


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=h1) Report
   > Merging [#6415](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=desc) (71a98cd) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.51%`.
   > The diff coverage is `56.58%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6415/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6415      +/-   ##
   ==========================================
   - Coverage   66.44%   64.93%   -1.52%     
   ==========================================
     Files        1075     1318     +243     
     Lines       54773    64189    +9416     
     Branches     8168     9336    +1168     
   ==========================================
   + Hits        36396    41683    +5287     
   - Misses      15700    19535    +3835     
   - Partials     2677     2971     +294     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.93% <56.58%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [...common/config/tuner/NoOpTableTableConfigTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
   | ... and [1153 more](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=footer). Last update [f09de82...8d142b1](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-pinot] apucher closed pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
apucher closed pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415


   


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552940682



##########
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:
       validation is right below




----------------------------------------------------------------
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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552964144



##########
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:
       fixed




----------------------------------------------------------------
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

[GitHub] [incubator-pinot] apucher commented on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755895172


   to be continued here: https://github.com/apache/incubator-pinot/pull/6418


----------------------------------------------------------------
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

[GitHub] [incubator-pinot] codecov-io commented on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755667310


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=h1) Report
   > Merging [#6415](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=desc) (a36446a) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.10%`.
   > The diff coverage is `38.69%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6415/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #6415       +/-   ##
   ===========================================
   - Coverage   66.44%   44.34%   -22.11%     
   ===========================================
     Files        1075     1318      +243     
     Lines       54773    64187     +9414     
     Branches     8168     9336     +1168     
   ===========================================
   - Hits        36396    28463     -7933     
   - Misses      15700    33366    +17666     
   + Partials     2677     2358      -319     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `44.34% <38.69%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | ... and [1303 more](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=footer). Last update [f09de82...a36446a](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552963028



##########
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:
       this directly opens a HttpURLConnection. If the protocol/schema is invalid, it immediately calls it out.




----------------------------------------------------------------
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

[GitHub] [incubator-pinot] apucher commented on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755810845


   on standby for review by @mcvsubbu 


----------------------------------------------------------------
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

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6415: Client-Broker TLS support

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#issuecomment-755667310


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=h1) Report
   > Merging [#6415](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=desc) (c9309f6) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.17%`.
   > The diff coverage is `38.86%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6415/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #6415       +/-   ##
   ===========================================
   - Coverage   66.44%   44.27%   -22.18%     
   ===========================================
     Files        1075     1318      +243     
     Lines       54773    64190     +9417     
     Branches     8168     9336     +1168     
   ===========================================
   - Hits        36396    28418     -7978     
   - Misses      15700    33410    +17710     
   + Partials     2677     2362      -315     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `44.27% <38.86%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | ... and [1304 more](https://codecov.io/gh/apache/incubator-pinot/pull/6415/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=footer). Last update [f09de82...8d142b1](https://codecov.io/gh/apache/incubator-pinot/pull/6415?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6415:
URL: https://github.com/apache/incubator-pinot/pull/6415#discussion_r552965948



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +62,60 @@ 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);
+
+    Preconditions.checkArgument(brokerQueryPort > 0);

Review comment:
       (nit) Add some error message

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +62,60 @@ 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);
+
+    Preconditions.checkArgument(brokerQueryPort > 0);
+    _baseUri = URI.create(String.format("%s://0.0.0.0:%d/", getBrokerClientProtocol(brokerConf), brokerQueryPort));
+
+    _httpServer = buildHttpsServer(brokerConf);
     setupSwagger();
   }
 
+  private HttpServer buildHttpsServer(PinotConfiguration brokerConf) {
+    boolean isSecure = CommonConstants.HTTPS_PROTOCOL.equals(getBrokerClientProtocol(brokerConf));
+
+    if (isSecure) {
+      return GrizzlyHttpServerFactory.createHttpServer(_baseUri, this, true, buildSSLConfig(brokerConf));
+    }
+
+    return GrizzlyHttpServerFactory.createHttpServer(_baseUri, this);
+  }
+
+  private SSLEngineConfigurator buildSSLConfig(PinotConfiguration brokerConf) {
+    SSLContextConfigurator sslContextConfigurator = new SSLContextConfigurator();
+
+    sslContextConfigurator.setKeyStoreFile(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_KEYSTORE_PATH));
+    sslContextConfigurator.setKeyStorePass(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_KEYSTORE_PASSWORD));
+    sslContextConfigurator.setTrustStoreFile(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_TRUSTSTORE_PATH));
+    sslContextConfigurator.setTrustStorePass(brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_TRUSTSTORE_PASSWORD));
+
+    boolean requiresClientAuth = brokerConf.getProperty(
+        CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_TLS_CLIENT_AUTH,
+        CommonConstants.Broker.DEFAULT_BROKER_CLIENT_TLS_CLIENT_AUTH);
+
+    return new SSLEngineConfigurator(sslContextConfigurator).setClientMode(false)
+        .setWantClientAuth(requiresClientAuth).setEnabledProtocols(new String[] { "TLSv1.2" });
+  }
+
+  private static String getBrokerClientProtocol(PinotConfiguration brokerConf) {
+    return Optional.ofNullable(brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_CLIENT_PROTOCOL))

Review comment:
       What I mean is that when user have an invalid protocol (e.g. `ftp`) in their config, we should throw exception instead of blindly use `http` 

##########
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:
       Throw exception when the protocol configured is not supported?




----------------------------------------------------------------
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