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/07 05:33:01 UTC
[GitHub] [incubator-pinot] apucher opened a new pull request #6418: TLS-support for client-pinot and pinot-internode connections
apucher opened a new pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418
**DRAFT**
**DISCUSS**: use single TLS keystore/truststore config for all TLS connections of a single process, e.g. combine `pinot.broker.client.tls` and `pinot.broker.netty.tls`?
## Description
We add support for TLS-secured connections between pinot clients, brokers, controllers, and servers:
- client-controller (https, refactored existing)
- client-broker (htps)
- controller-broker relay (https)
- broker-server (netty)
The implementation supports legacy http, 1-way TLS, and 2-way TLS.
## 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 connections. TLS can be configured using the following new (or refactored) properties:
pinot-broker REST api
- 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.client.auth (`true` or `false`)
pinot-broker netty client
- pinot.broker.netty.tls.enabled (`true` or `false`)
- pinot.broker.netty.tls.keystore.path
- pinot.broker.netty.tls.keystore.password
- pinot.broker.netty.tls.truststore.path
- pinot.broker.netty.tls.truststore.password
- pinot.broker.netty.tls.client.auth (`true` or `false`)
pinot-controller REST api
- pinot.controller.access.protocols (`http` or `https` or `http,https`)
- pinot.controller.access.protocols.https.tls.keystore.path
- pinot.controller.access.protocols.https.tls.keystore.password
- pinot.controller.access.protocols.https.tls.truststore.path
- pinot.controller.access.protocols.https.tls.truststore.password
- pinot.controller.access.protocols.https.tls.client.auth (`true` or `false`)
pinot-controller REST broker relay
- pinot.controller.broker.protocol (`http` or `https`)
- pinot.controller.broker.tls.keystore.path
- pinot.controller.broker.tls.keystore.password
- pinot.controller.broker.tls.truststore.path
- pinot.controller.broker.tls.truststore.password
- pinot.controller.broker.tls.client.auth (`true` or `false`)
pinot-server netty server
- pinot.server.netty.tls.enabled (`true` or `false`)
- pinot.server.netty.tls.keystore.path
- pinot.server.netty.tls.keystore.password
- pinot.server.netty.tls.truststore.path
- pinot.server.netty.tls.truststore.password
- pinot.server.netty.tls.client.auth (`true` or `false`)
## Documentation
TBD
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (8c40963) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.36%`.
> The diff coverage is `38.84%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.07% -22.37%
===========================================
Files 1075 1320 +245
Lines 54773 64417 +9644
Branches 8168 9374 +1206
===========================================
- Hits 36396 28395 -8001
- Misses 15700 33656 +17956
+ Partials 2677 2366 -311
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.07% <38.84%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1306 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...504d0d8](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (9b50c3e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.36%`.
> The diff coverage is `38.93%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.08% -22.37%
===========================================
Files 1075 1320 +245
Lines 54773 64425 +9652
Branches 8168 9379 +1211
===========================================
- Hits 36396 28400 -7996
- Misses 15700 33648 +17948
+ Partials 2677 2377 -300
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.08% <38.93%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1307 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...9b50c3e](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 merged pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher merged pull request #6418:
URL: 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] apucher commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557829988
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,258 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.UnknownHostException;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.commons.httpclient.ConnectTimeoutException;
+import org.apache.commons.httpclient.params.HttpConnectionParams;
+import org.apache.commons.httpclient.protocol.Protocol;
+import org.apache.commons.httpclient.protocol.SecureProtocolSocketFactory;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH_ENABLED = "client.auth.enabled";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration key, with optional defaults.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(TlsConfig tlsDefaults, PinotConfiguration pinotConfig, String namespace) {
+ TlsConfig tlsConfig = new TlsConfig();
+ tlsConfig.setEnabled(tlsDefaults.isEnabled());
+ tlsConfig.setClientAuthEnabled(tlsDefaults.isClientAuthEnabled());
+ tlsConfig.setKeyStorePath(tlsDefaults.getKeyStorePath());
+ tlsConfig.setKeyStorePassword(tlsDefaults.getKeyStorePassword());
+ tlsConfig.setTrustStorePath(tlsDefaults.getTrustStorePath());
+ tlsConfig.setTrustStorePassword(tlsDefaults.getTrustStorePassword());
+
+ if (pinotConfig.containsKey(key(namespace, ENABLED))) {
+ tlsConfig.setEnabled(pinotConfig.getProperty(key(namespace, ENABLED), false));
Review comment:
unfortunately, this seems to be the only way to gracefully access boolean properties via PinotConfiguration.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (af7003d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.50%`.
> The diff coverage is `57.15%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
- Coverage 66.44% 64.94% -1.51%
==========================================
Files 1075 1321 +246
Lines 54773 64679 +9906
Branches 8168 9431 +1263
==========================================
+ Hits 36396 42006 +5610
- Misses 15700 19671 +3971
- Partials 2677 3002 +325
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.94% <57.15%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1158 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [3d24302...af7003d](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557827279
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -72,7 +69,7 @@ private void setupSwagger() {
beanConfig.setContact("https://github.com/apache/incubator-pinot");
beanConfig.setVersion("1.0");
beanConfig.setSchemes(new String[]{CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL});
- beanConfig.setBasePath(_baseUri.getPath());
+ beanConfig.setBasePath("/");
Review comment:
baseUri is a useless variable. the path is always set to "/" (see above in prev version, controller uses "/" directly too).
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-756326662
pinot's config mechanism could definitely use a make-over. I think, however, that coming up with a pattern for protocol definitions (rather than just tls) while we don't support multi-protocol will add to the confusion later on when (if) this is actually implemented and the pattern changes, again.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (00be12e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.30%`.
> The diff coverage is `39.01%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.14% -22.31%
===========================================
Files 1075 1321 +246
Lines 54773 64678 +9905
Branches 8168 9432 +1264
===========================================
- Hits 36396 28553 -7843
- Misses 15700 33737 +18037
+ Partials 2677 2388 -289
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.14% <39.01%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1307 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [3d24302...00be12e](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r561215986
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}
+ private int inferPort() {
+ return Optional.ofNullable(_brokerConf.getProperty(Helix.KEY_OF_BROKER_QUERY_PORT)).map(Integer::parseInt)
Review comment:
Do we do that correctly if nothing is configured?
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (9e46f91) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.50%`.
> The diff coverage is `56.83%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
- Coverage 66.44% 64.94% -1.51%
==========================================
Files 1075 1320 +245
Lines 54773 64425 +9652
Branches 8168 9379 +1211
==========================================
+ Hits 36396 41843 +5447
- Misses 15700 19601 +3901
- Partials 2677 2981 +304
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.94% <56.83%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1156 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...9e46f91](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r554279196
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/TlsConfig.java
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.transport;
+
+/**
+ * Container object for TLS/SSL configuration of pinot clients and servers (netty, grizzly, etc.)
+ */
+public class TlsConfig {
+ private boolean _enabled;
+ private boolean _clientAuth;
Review comment:
done
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
Review comment:
done
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePassword(), "key store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getKeyStorePath())) {
+ keyStore.load(is, tlsConfig.getKeyStorePassword().toCharArray());
+ }
+
+ KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ keyManagerFactory.init(keyStore, tlsConfig.getKeyStorePassword().toCharArray());
+
+ return keyManagerFactory;
+
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create key manager factory '%s'",
+ tlsConfig.getKeyStorePath()), e);
+ }
+ }
+
+ /**
+ * Create a TrustManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return TrustManagerFactory
+ */
+ public static TrustManagerFactory createTrustManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePath(), "trust store path is null");
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePassword(), "trust store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getTrustStorePath())) {
+ keyStore.load(is, tlsConfig.getTrustStorePassword().toCharArray());
+ }
+
+ TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(keyStore);
+
+ return trustManagerFactory;
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create trust manager factory '%s'",
+ tlsConfig.getTrustStorePath()), e);
+ }
+ }
+
+ /**
+ * Installs a default TLS socket factory for all HttpsURLConnection instances based on a given TlsConfig (1 or 2-way)
+ *
+ * @param tlsConfig TLS config
+ */
+ public static void installDefaultSSLSocketFactory(TlsConfig tlsConfig) {
+ KeyManager[] keyManagers = null;
+ if (tlsConfig.getKeyStorePath() != null) {
+ keyManagers = createKeyManagerFactory(tlsConfig).getKeyManagers();
+ }
+
+ TrustManager[] trustManagers = null;
+ if (tlsConfig.getTrustStorePath() != null) {
+ trustManagers = createTrustManagerFactory(tlsConfig).getTrustManagers();
+ }
+
+ try {
+ SSLContext sc = SSLContext.getInstance("SSL");
+ sc.init(keyManagers, trustManagers, new java.security.SecureRandom());
+ HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
+ } catch (GeneralSecurityException ignore) {
+ // ignore
Review comment:
fixed
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -238,14 +240,16 @@ public void start()
// Initialize FunctionRegistry before starting the broker request handler
FunctionRegistry.init();
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
+ // Configure TLS
Review comment:
done
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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, "broker client port must be > 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));
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_CLIENT_TLS_PREFIX, CommonConstants.Broker.BROKER_TLS_PREFIX);
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (54c3ce4) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.24%`.
> The diff coverage is `39.05%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.20% -22.25%
===========================================
Files 1075 1321 +246
Lines 54773 64689 +9916
Branches 8168 9432 +1264
===========================================
- Hits 36396 28594 -7802
- Misses 15700 33714 +18014
+ Partials 2677 2381 -296
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.20% <39.05%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1305 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [3d24302...54c3ce4](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557630534
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -108,13 +108,17 @@
public static final String INSTANCE_ID_KEY = "instanceId";
public static final String DATA_DIR_KEY = "dataDir";
public static final String ADMIN_PORT_KEY = "adminPort";
+ public static final String ADMIN_HTTPS_PORT_KEY = "adminHttpsPort";
public static final String GRPC_PORT_KEY = "grpcPort";
+ public static final String NETTYTLS_PORT_KEY = "nettyTlsPort";
}
public static final String SET_INSTANCE_ID_TO_HOSTNAME_KEY = "pinot.set.instance.id.to.hostname";
public static final String KEY_OF_SERVER_NETTY_PORT = "pinot.server.netty.port";
public static final int DEFAULT_SERVER_NETTY_PORT = 8098;
+ public static final String KEY_OF_SERVER_NETTYTLS_PORT = "pinot.server.nettytls.port";
Review comment:
nit: use `BROKER_NETTYTLS_PREFIX` here?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -72,7 +69,7 @@ private void setupSwagger() {
beanConfig.setContact("https://github.com/apache/incubator-pinot");
beanConfig.setVersion("1.0");
beanConfig.setSchemes(new String[]{CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL});
- beanConfig.setBasePath(_baseUri.getPath());
+ beanConfig.setBasePath("/");
Review comment:
I am not sure if this is related to TLS or not. What breaks if this change is not done?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
+ private static final String DEFAULT_HOST = "0.0.0.0";
+ private static final String DOT_ACCESS_PROTOCOLS = ".access.protocols";
+
+ public static final Set<String> SUPPORTED_PROTOCOLS = new HashSet<>(
+ Arrays.asList(CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL));
+
+ /**
+ * Generates {@link ListenerConfig} instances based on the combination
+ * of properties such as *.port and *.access.protocols.
+ *
+ * @param config property holders for controller configuration
+ * @param namespace property namespace to extract from
+ *
+ * @return List of {@link ListenerConfig} for which http listeners
+ * should be created.
+ */
+ public static List<ListenerConfig> buildListenerConfigs(PinotConfiguration config, String namespace,
+ TlsConfig tlsDefaults) {
+ if (StringUtils.isBlank(config.getProperty(namespace + DOT_ACCESS_PROTOCOLS))) {
+ return new ArrayList<>();
+ }
+
+ String[] protocols = config.getProperty(namespace + DOT_ACCESS_PROTOCOLS).split(",");
+
+ return Arrays.stream(protocols)
+ .peek(protocol -> Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(protocol),
+ "Unsupported protocol '%s' in config namespace '%s'", protocol, namespace))
+ .map(protocol -> buildListenerConfig(config, namespace, protocol, tlsDefaults))
+ .collect(Collectors.toList());
+ }
+
+ public static List<ListenerConfig> buildControllerConfigs(PinotConfiguration controllerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String portString = controllerConf.getProperty("controller.port");
+ if (portString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(portString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(controllerConf, "controller.tls");
+
+ listeners.addAll(buildListenerConfigs(controllerConf, "controller", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildBrokerConfigs(PinotConfiguration brokerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String queryPortString = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT);
+ if (queryPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(queryPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(brokerConf, "pinot.broker.client", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration serverConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String adminApiPortString = serverConf.getProperty(CommonConstants.Server.CONFIG_OF_ADMIN_API_PORT);
+ if (adminApiPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(adminApiPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(serverConf, CommonConstants.Server.SERVER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(serverConf, "pinot.server.adminapi", tlsDefaults));
+
+ return listeners;
+ }
+
+ private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
+ TlsConfig tlsDefaults) {
+ String protocolNamespace = namespace + DOT_ACCESS_PROTOCOLS + "." + protocol;
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, config, protocolNamespace + ".tls");
+ tlsConfig.setEnabled(CommonConstants.HTTPS_PROTOCOL.equals(protocol));
+
+ return new ListenerConfig(protocol,
+ getHost(config.getProperty(protocolNamespace + ".host", DEFAULT_HOST)),
+ getPort(config.getProperty(protocolNamespace + ".port")), protocol, tlsConfig);
+ }
+
+ private static String getHost(String configuredHost) {
+ return Optional.ofNullable(configuredHost).filter(host -> !host.trim().isEmpty())
+ .orElseThrow(() -> new IllegalArgumentException(configuredHost + " is not a valid host"));
+ }
+
+ private static int getPort(String configuredPort) {
+ return Optional.ofNullable(configuredPort).filter(port -> !port.trim().isEmpty()).<Integer> map(Integer::valueOf)
+ .orElseThrow(() -> new IllegalArgumentException(configuredPort + " is not a valid port"));
+ }
+
+ public static HttpServer buildHttpServer(ResourceConfig resConfig, List<ListenerConfig> listenerConfigs) {
+ Preconditions.checkNotNull(listenerConfigs);
+
+ // The URI is irrelevant since the default listener will be manually rewritten.
+ HttpServer httpServer = GrizzlyHttpServerFactory.createHttpServer(URI.create("http://0.0.0.0/"), resConfig, false);
+
+ // Listeners cannot be configured with the factory. Manual overrides are required as instructed by Javadoc.
+ httpServer.removeListener("grizzly");
+
+ listenerConfigs.forEach(listenerConfig -> configureListener(httpServer, listenerConfig));
+
+ try {
Review comment:
Do we really need to start the http server here? The caller may choose to build the server ahead of time, and then start it in its own start process. I suggest we let the caller start the server.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (1113259) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.27%`.
> The diff coverage is `39.01%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.17% -22.28%
===========================================
Files 1075 1321 +246
Lines 54773 64657 +9884
Branches 8168 9431 +1263
===========================================
- Hits 36396 28560 -7836
- Misses 15700 33718 +18018
+ Partials 2677 2379 -298
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.17% <39.01%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1306 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [3d24302...1113259](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557669864
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,258 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.UnknownHostException;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.commons.httpclient.ConnectTimeoutException;
+import org.apache.commons.httpclient.params.HttpConnectionParams;
+import org.apache.commons.httpclient.protocol.Protocol;
+import org.apache.commons.httpclient.protocol.SecureProtocolSocketFactory;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH_ENABLED = "client.auth.enabled";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration key, with optional defaults.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(TlsConfig tlsDefaults, PinotConfiguration pinotConfig, String namespace) {
+ TlsConfig tlsConfig = new TlsConfig();
+ tlsConfig.setEnabled(tlsDefaults.isEnabled());
+ tlsConfig.setClientAuthEnabled(tlsDefaults.isClientAuthEnabled());
+ tlsConfig.setKeyStorePath(tlsDefaults.getKeyStorePath());
+ tlsConfig.setKeyStorePassword(tlsDefaults.getKeyStorePassword());
+ tlsConfig.setTrustStorePath(tlsDefaults.getTrustStorePath());
+ tlsConfig.setTrustStorePassword(tlsDefaults.getTrustStorePassword());
+
+ if (pinotConfig.containsKey(key(namespace, ENABLED))) {
+ tlsConfig.setEnabled(pinotConfig.getProperty(key(namespace, ENABLED), false));
Review comment:
if pinotConfig contains the key, then we don't need to call the API with default of false, right? We can just set it to whatever is in pinotconfig. Or, are you considering some erroneous configuration there?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}
+ private int inferPort() {
+ return Optional.ofNullable(_brokerConf.getProperty(Helix.KEY_OF_BROKER_QUERY_PORT)).map(Integer::parseInt)
Review comment:
Not sure how this ensures that if nothing is configured, we still start without tls on DEFAULT_BROKER_QUERY_PORT (previous behavior).
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerRoutingInstance.java
##########
@@ -37,14 +38,20 @@
private static final String SHORT_REALTIME_SUFFIX = "_R";
private static final Map<String, String> SHORT_HOSTNAME_MAP = new ConcurrentHashMap<>();
+ private final boolean _tls;
Review comment:
rename to `isTlsEnabled`
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,258 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.UnknownHostException;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.commons.httpclient.ConnectTimeoutException;
+import org.apache.commons.httpclient.params.HttpConnectionParams;
+import org.apache.commons.httpclient.protocol.Protocol;
+import org.apache.commons.httpclient.protocol.SecureProtocolSocketFactory;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH_ENABLED = "client.auth.enabled";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration key, with optional defaults.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace namespace prefix
Review comment:
Add javadoc for tlsDefaults
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/QueryRouter.java
##########
@@ -60,20 +84,23 @@ public AsyncQueryResponse submitQuery(long requestId, String rawTableName,
long timeoutMs) {
assert offlineBrokerRequest != null || realtimeBrokerRequest != null;
+ // can prefer but not require TLS until all servers guaranteed to be on TLS
Review comment:
Not sure I understand completely. If I follow the code, it seems like preferTLS == requireTLS. If it errors out, do we try to go with plain? Maybe I missed that code?
----------------------------------------------------------------
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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r554246048
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/TlsConfig.java
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.transport;
+
+/**
+ * Container object for TLS/SSL configuration of pinot clients and servers (netty, grizzly, etc.)
+ */
+public class TlsConfig {
+ private boolean _enabled;
+ private boolean _clientAuth;
Review comment:
I think this indicates whether or not client auth should be done? Can we call it `_clientAuthEnabled`?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePassword(), "key store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getKeyStorePath())) {
+ keyStore.load(is, tlsConfig.getKeyStorePassword().toCharArray());
+ }
+
+ KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ keyManagerFactory.init(keyStore, tlsConfig.getKeyStorePassword().toCharArray());
+
+ return keyManagerFactory;
+
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create key manager factory '%s'",
+ tlsConfig.getKeyStorePath()), e);
+ }
+ }
+
+ /**
+ * Create a TrustManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return TrustManagerFactory
+ */
+ public static TrustManagerFactory createTrustManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePath(), "trust store path is null");
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePassword(), "trust store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getTrustStorePath())) {
+ keyStore.load(is, tlsConfig.getTrustStorePassword().toCharArray());
+ }
+
+ TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(keyStore);
+
+ return trustManagerFactory;
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create trust manager factory '%s'",
+ tlsConfig.getTrustStorePath()), e);
+ }
+ }
+
+ /**
+ * Installs a default TLS socket factory for all HttpsURLConnection instances based on a given TlsConfig (1 or 2-way)
+ *
+ * @param tlsConfig TLS config
+ */
+ public static void installDefaultSSLSocketFactory(TlsConfig tlsConfig) {
+ KeyManager[] keyManagers = null;
+ if (tlsConfig.getKeyStorePath() != null) {
+ keyManagers = createKeyManagerFactory(tlsConfig).getKeyManagers();
+ }
+
+ TrustManager[] trustManagers = null;
+ if (tlsConfig.getTrustStorePath() != null) {
+ trustManagers = createTrustManagerFactory(tlsConfig).getTrustManagers();
+ }
+
+ try {
+ SSLContext sc = SSLContext.getInstance("SSL");
+ sc.init(keyManagers, trustManagers, new java.security.SecureRandom());
+ HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
+ } catch (GeneralSecurityException ignore) {
+ // ignore
Review comment:
Why ignore this exception?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -177,6 +177,12 @@
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";
Review comment:
In order to support backward compat and migration, you should introduce config so that both http and https can be supported, both for client and netty. The way installations can migrate brokers by enabling both http and https, changing clients to https, ad then disabling https on the broker port.
For netty is a bit trickier. We need to upgrade the servers to support both, change the broker to start using https, and then disable the servers to use http.
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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,
Review comment:
Dont you want to support backward compatibility and migration path? You should take two ports, and start http as well as https service, so that installations can upgrade without down time
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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, "broker client port must be > 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));
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_CLIENT_TLS_PREFIX, CommonConstants.Broker.BROKER_TLS_PREFIX);
Review comment:
can u move this below line 88?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
Review comment:
Shouldn't these checks be done only if TlsConfig has enabled ?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -238,14 +240,16 @@ public void start()
// Initialize FunctionRegistry before starting the broker request handler
FunctionRegistry.init();
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
+ // Configure TLS
Review comment:
```suggestion
// Configure TLS for netty connection to server
```
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r561278733
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}
+ private int inferPort() {
+ return Optional.ofNullable(_brokerConf.getProperty(Helix.KEY_OF_BROKER_QUERY_PORT)).map(Integer::parseInt)
Review comment:
This is the same code as on the multi-ingress controller. For completeness sake, I verified the behavior for different scenarios via quickstart:
pinot.broker.client.queryPort only: `Broker_192.168.0.126_8000`
pinot.broker.client.queryPort and https ingress: `Broker_192.168.0.126_8000`
http and https ingress only: `Broker_192.168.0.126_8000`
https ingress only: `Broker_192.168.0.126_8443`
no config: `exception "No value present”`
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557829165
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}
+ private int inferPort() {
+ return Optional.ofNullable(_brokerConf.getProperty(Helix.KEY_OF_BROKER_QUERY_PORT)).map(Integer::parseInt)
Review comment:
starting on DEFAULT_BROKER_QUERY_PORT is ensured by ListenerConfigUtil. Here, we merely need to figure out a port number to generate an instance id for Helix
----------------------------------------------------------------
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 edited a comment on pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-762493512
@Jackie-Jiang I removed the connection-specific TLS settings, and now rely on global keystore/truststore settings only. Enabling TLS is now implicitly a function of the protocol being used (e.g. "HTTPS"). This should reduce the complexity a bit.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (80f62e9) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.57%`.
> The diff coverage is `58.23%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
- Coverage 66.44% 64.87% -1.58%
==========================================
Files 1075 1334 +259
Lines 54773 65525 +10752
Branches 8168 9553 +1385
==========================================
+ Hits 36396 42507 +6111
- Misses 15700 19979 +4279
- Partials 2677 3039 +362
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.87% <58.23%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1176 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [2e7cdcd...80f62e9](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-762493512
I removed the connection-specific TLS settings, and now rely on global keystore/truststore settings only. Enabling TLS is now implicitly a function of the protocol being used (e.g. "HTTPS"). This should reduce the complexity a bit.
----------------------------------------------------------------
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] mcvsubbu commented on pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-757074013
Can you add some comments in the checkin as to the tests you did?
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-760684980
@mcvsubbu Thank you for the in-depth review. Re tests executed:
I modified my Quickstart locally to cursory test all permutations of broker/controller/server unsecured, 1-way TLS, and 2-way TLS. The table-size related queries were tested for controller/server-permutations only.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (29e999e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.36%`.
> The diff coverage is `57.02%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
- Coverage 66.44% 65.08% -1.37%
==========================================
Files 1075 1332 +257
Lines 54773 65182 +10409
Branches 8168 9509 +1341
==========================================
+ Hits 36396 42423 +6027
- Misses 15700 19716 +4016
- Partials 2677 3043 +366
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.08% <57.02%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1175 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [8085fb7...29e999e](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-757074013
Can you add some comments in the checkin as to the tests you did?
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (b63ab5f) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.31%`.
> The diff coverage is `38.66%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.13% -22.32%
===========================================
Files 1075 1321 +246
Lines 54773 64586 +9813
Branches 8168 9415 +1247
===========================================
- Hits 36396 28507 -7889
- Misses 15700 33708 +18008
+ Partials 2677 2371 -306
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.13% <38.66%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1305 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...b63ab5f](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-756315746
Should we make all rest API config keys the same format as the controller? I prefer making them consistent, and It will be more flexible if we can support http and https at the same time on different port (we might add more protocols in the future)
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r561278733
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}
+ private int inferPort() {
+ return Optional.ofNullable(_brokerConf.getProperty(Helix.KEY_OF_BROKER_QUERY_PORT)).map(Integer::parseInt)
Review comment:
This is the same code as on the multi-ingress controller. For completeness sake, I verified the behavior for different scenarios via quickstart:
pinot.broker.client.queryPort only: `Broker_192.168.0.126_8000`
pinot.broker.client.queryPort and https ingress: `Broker_192.168.0.126_8000`
http and https ingress only: `Broker_192.168.0.126_8000`
https ingress only: `Broker_192.168.0.126_8443`
no config: `exception "No value present”`
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (ed6c724) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.46%`.
> The diff coverage is `56.83%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
- Coverage 66.44% 64.98% -1.47%
==========================================
Files 1075 1321 +246
Lines 54773 64578 +9805
Branches 8168 9414 +1246
==========================================
+ Hits 36396 41964 +5568
- Misses 15700 19614 +3914
- Partials 2677 3000 +323
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.98% <56.83%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1157 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...ed6c724](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-760619992
> Can we unify the format of the config keys? Use either `*.tls.*` or `*.protocol.https.tls.*`, but not mixed for easier configuration. I would suggest the former as `tls` does not tie to `https`
removed `.tls` (also reflected in the release notes above). Configs now looks like this: `controller.access.protocols.https.keystore.password`
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557836431
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -413,10 +413,21 @@ protected void configure() {
}
});
- _adminApp.start(_listenerConfigs);
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(_config, ControllerConf.CONTROLLER_TLS_PREFIX);
+
+ // install default SSL context if necessary
+ if (CommonConstants.HTTPS_PROTOCOL.equals(_config.getProperty(ControllerConf.CONTROLLER_BROKER_PROTOCOL))) {
+ LOGGER.info("Installing default SSL context for broker relay requests");
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, _config, ControllerConf.CONTROLLER_BROKER_TLS_PREFIX);
Review comment:
imo "principle of least surprise" applies here. If I create a config pojo I wouldn't expect the side-effect of someone rewriting my ssl-factories
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557836115
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
+ private static final String DEFAULT_HOST = "0.0.0.0";
+ private static final String DOT_ACCESS_PROTOCOLS = ".access.protocols";
+
+ public static final Set<String> SUPPORTED_PROTOCOLS = new HashSet<>(
+ Arrays.asList(CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL));
+
+ /**
+ * Generates {@link ListenerConfig} instances based on the combination
+ * of properties such as *.port and *.access.protocols.
+ *
+ * @param config property holders for controller configuration
+ * @param namespace property namespace to extract from
+ *
+ * @return List of {@link ListenerConfig} for which http listeners
+ * should be created.
+ */
+ public static List<ListenerConfig> buildListenerConfigs(PinotConfiguration config, String namespace,
+ TlsConfig tlsDefaults) {
+ if (StringUtils.isBlank(config.getProperty(namespace + DOT_ACCESS_PROTOCOLS))) {
+ return new ArrayList<>();
+ }
+
+ String[] protocols = config.getProperty(namespace + DOT_ACCESS_PROTOCOLS).split(",");
+
+ return Arrays.stream(protocols)
+ .peek(protocol -> Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(protocol),
+ "Unsupported protocol '%s' in config namespace '%s'", protocol, namespace))
+ .map(protocol -> buildListenerConfig(config, namespace, protocol, tlsDefaults))
+ .collect(Collectors.toList());
+ }
+
+ public static List<ListenerConfig> buildControllerConfigs(PinotConfiguration controllerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String portString = controllerConf.getProperty("controller.port");
+ if (portString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(portString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(controllerConf, "controller.tls");
+
+ listeners.addAll(buildListenerConfigs(controllerConf, "controller", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildBrokerConfigs(PinotConfiguration brokerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String queryPortString = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT);
+ if (queryPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(queryPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(brokerConf, "pinot.broker.client", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration serverConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String adminApiPortString = serverConf.getProperty(CommonConstants.Server.CONFIG_OF_ADMIN_API_PORT);
+ if (adminApiPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(adminApiPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(serverConf, CommonConstants.Server.SERVER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(serverConf, "pinot.server.adminapi", tlsDefaults));
+
+ return listeners;
+ }
+
+ private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
+ TlsConfig tlsDefaults) {
+ String protocolNamespace = namespace + DOT_ACCESS_PROTOCOLS + "." + protocol;
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, config, protocolNamespace + ".tls");
+ tlsConfig.setEnabled(CommonConstants.HTTPS_PROTOCOL.equals(protocol));
+
+ return new ListenerConfig(protocol,
+ getHost(config.getProperty(protocolNamespace + ".host", DEFAULT_HOST)),
+ getPort(config.getProperty(protocolNamespace + ".port")), protocol, tlsConfig);
+ }
+
+ private static String getHost(String configuredHost) {
+ return Optional.ofNullable(configuredHost).filter(host -> !host.trim().isEmpty())
+ .orElseThrow(() -> new IllegalArgumentException(configuredHost + " is not a valid host"));
+ }
+
+ private static int getPort(String configuredPort) {
+ return Optional.ofNullable(configuredPort).filter(port -> !port.trim().isEmpty()).<Integer> map(Integer::valueOf)
+ .orElseThrow(() -> new IllegalArgumentException(configuredPort + " is not a valid port"));
+ }
+
+ public static HttpServer buildHttpServer(ResourceConfig resConfig, List<ListenerConfig> listenerConfigs) {
+ Preconditions.checkNotNull(listenerConfigs);
+
+ // The URI is irrelevant since the default listener will be manually rewritten.
+ HttpServer httpServer = GrizzlyHttpServerFactory.createHttpServer(URI.create("http://0.0.0.0/"), resConfig, false);
+
+ // Listeners cannot be configured with the factory. Manual overrides are required as instructed by Javadoc.
Review comment:
added
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -413,10 +413,21 @@ protected void configure() {
}
});
- _adminApp.start(_listenerConfigs);
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(_config, ControllerConf.CONTROLLER_TLS_PREFIX);
+
+ // install default SSL context if necessary
+ if (CommonConstants.HTTPS_PROTOCOL.equals(_config.getProperty(ControllerConf.CONTROLLER_BROKER_PROTOCOL))) {
+ LOGGER.info("Installing default SSL context for broker relay requests");
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557830377
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/ServerRoutingInstance.java
##########
@@ -37,14 +38,20 @@
private static final String SHORT_REALTIME_SUFFIX = "_R";
private static final Map<String, String> SHORT_HOSTNAME_MAP = new ConcurrentHashMap<>();
+ private final boolean _tls;
Review comment:
fixed
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,258 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.UnknownHostException;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.commons.httpclient.ConnectTimeoutException;
+import org.apache.commons.httpclient.params.HttpConnectionParams;
+import org.apache.commons.httpclient.protocol.Protocol;
+import org.apache.commons.httpclient.protocol.SecureProtocolSocketFactory;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH_ENABLED = "client.auth.enabled";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration key, with optional defaults.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace namespace prefix
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-765648243
@mcvsubbu fixed the message. thanks for bearing with me.
----------------------------------------------------------------
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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r554246048
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/TlsConfig.java
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.transport;
+
+/**
+ * Container object for TLS/SSL configuration of pinot clients and servers (netty, grizzly, etc.)
+ */
+public class TlsConfig {
+ private boolean _enabled;
+ private boolean _clientAuth;
Review comment:
I think this indicates whether or not client auth should be done? Can we call it `_clientAuthEnabled`?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePassword(), "key store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getKeyStorePath())) {
+ keyStore.load(is, tlsConfig.getKeyStorePassword().toCharArray());
+ }
+
+ KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ keyManagerFactory.init(keyStore, tlsConfig.getKeyStorePassword().toCharArray());
+
+ return keyManagerFactory;
+
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create key manager factory '%s'",
+ tlsConfig.getKeyStorePath()), e);
+ }
+ }
+
+ /**
+ * Create a TrustManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return TrustManagerFactory
+ */
+ public static TrustManagerFactory createTrustManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePath(), "trust store path is null");
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePassword(), "trust store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getTrustStorePath())) {
+ keyStore.load(is, tlsConfig.getTrustStorePassword().toCharArray());
+ }
+
+ TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(keyStore);
+
+ return trustManagerFactory;
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create trust manager factory '%s'",
+ tlsConfig.getTrustStorePath()), e);
+ }
+ }
+
+ /**
+ * Installs a default TLS socket factory for all HttpsURLConnection instances based on a given TlsConfig (1 or 2-way)
+ *
+ * @param tlsConfig TLS config
+ */
+ public static void installDefaultSSLSocketFactory(TlsConfig tlsConfig) {
+ KeyManager[] keyManagers = null;
+ if (tlsConfig.getKeyStorePath() != null) {
+ keyManagers = createKeyManagerFactory(tlsConfig).getKeyManagers();
+ }
+
+ TrustManager[] trustManagers = null;
+ if (tlsConfig.getTrustStorePath() != null) {
+ trustManagers = createTrustManagerFactory(tlsConfig).getTrustManagers();
+ }
+
+ try {
+ SSLContext sc = SSLContext.getInstance("SSL");
+ sc.init(keyManagers, trustManagers, new java.security.SecureRandom());
+ HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
+ } catch (GeneralSecurityException ignore) {
+ // ignore
Review comment:
Why ignore this exception?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -177,6 +177,12 @@
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";
Review comment:
In order to support backward compat and migration, you should introduce config so that both http and https can be supported, both for client and netty. The way installations can migrate brokers by enabling both http and https, changing clients to https, ad then disabling https on the broker port.
For netty is a bit trickier. We need to upgrade the servers to support both, change the broker to start using https, and then disable the servers to use http.
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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,
Review comment:
Dont you want to support backward compatibility and migration path? You should take two ports, and start http as well as https service, so that installations can upgrade without down time
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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, "broker client port must be > 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));
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_CLIENT_TLS_PREFIX, CommonConstants.Broker.BROKER_TLS_PREFIX);
Review comment:
can u move this below line 88?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
Review comment:
Shouldn't these checks be done only if TlsConfig has enabled ?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -238,14 +240,16 @@ public void start()
// Initialize FunctionRegistry before starting the broker request handler
FunctionRegistry.init();
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
+ // Configure TLS
Review comment:
```suggestion
// Configure TLS for netty connection to server
```
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r556009033
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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,
Review comment:
I added multi-ingress support for broker and server. unfortunately, this substantially increased the complexity of the PR.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-762496331
@mcvsubbu here's the results of the manual TLS-upgrade test from fully unsecured, over multi-ingress, to 2-way TLS without global cluster downtime:
**Unsecured - defaults**
HTTP
Add a table - pass
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
No HTTPS ingress - pass
**Unsecured - TLS configured, not enabled**
Set keystore and truststore paths via TLS defaults, but don’t enable https/nettytls
HTTP
Add a table - pass
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
No HTTPS ingress - pass (not reachable)
**Unsecured - TLS active via multi-ingress**
Enable TLS ingress via alternate ports, still use insecure egress
HTTP
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
HTTPS
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
**Secured - TLS active via multi-ingress**
Enable TLS ingress and egress via alternate ports, still allow insecure ingress too
HTTP
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
HTTPS
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
**Secured - TLS active only, 2-way TLS internode**
Enable TLS ingress and egress via alternate ports only
No HTTP access - pass
HTTPS
Add a segment - pass
Query broker - pass
Query controller - pass
Query tablesize - pass
Delete the segment - pass
Delete the table - pass
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557828740
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -108,13 +108,17 @@
public static final String INSTANCE_ID_KEY = "instanceId";
public static final String DATA_DIR_KEY = "dataDir";
public static final String ADMIN_PORT_KEY = "adminPort";
+ public static final String ADMIN_HTTPS_PORT_KEY = "adminHttpsPort";
public static final String GRPC_PORT_KEY = "grpcPort";
+ public static final String NETTYTLS_PORT_KEY = "nettyTlsPort";
}
public static final String SET_INSTANCE_ID_TO_HOSTNAME_KEY = "pinot.set.instance.id.to.hostname";
public static final String KEY_OF_SERVER_NETTY_PORT = "pinot.server.netty.port";
public static final int DEFAULT_SERVER_NETTY_PORT = 8098;
+ public static final String KEY_OF_SERVER_NETTYTLS_PORT = "pinot.server.nettytls.port";
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] apucher commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557833202
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
+ private static final String DEFAULT_HOST = "0.0.0.0";
+ private static final String DOT_ACCESS_PROTOCOLS = ".access.protocols";
+
+ public static final Set<String> SUPPORTED_PROTOCOLS = new HashSet<>(
+ Arrays.asList(CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL));
+
+ /**
+ * Generates {@link ListenerConfig} instances based on the combination
+ * of properties such as *.port and *.access.protocols.
+ *
+ * @param config property holders for controller configuration
+ * @param namespace property namespace to extract from
+ *
+ * @return List of {@link ListenerConfig} for which http listeners
+ * should be created.
+ */
+ public static List<ListenerConfig> buildListenerConfigs(PinotConfiguration config, String namespace,
+ TlsConfig tlsDefaults) {
+ if (StringUtils.isBlank(config.getProperty(namespace + DOT_ACCESS_PROTOCOLS))) {
+ return new ArrayList<>();
+ }
+
+ String[] protocols = config.getProperty(namespace + DOT_ACCESS_PROTOCOLS).split(",");
+
+ return Arrays.stream(protocols)
+ .peek(protocol -> Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(protocol),
+ "Unsupported protocol '%s' in config namespace '%s'", protocol, namespace))
+ .map(protocol -> buildListenerConfig(config, namespace, protocol, tlsDefaults))
+ .collect(Collectors.toList());
+ }
+
+ public static List<ListenerConfig> buildControllerConfigs(PinotConfiguration controllerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String portString = controllerConf.getProperty("controller.port");
+ if (portString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(portString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(controllerConf, "controller.tls");
+
+ listeners.addAll(buildListenerConfigs(controllerConf, "controller", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildBrokerConfigs(PinotConfiguration brokerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String queryPortString = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT);
+ if (queryPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(queryPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(brokerConf, "pinot.broker.client", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration serverConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String adminApiPortString = serverConf.getProperty(CommonConstants.Server.CONFIG_OF_ADMIN_API_PORT);
+ if (adminApiPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(adminApiPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(serverConf, CommonConstants.Server.SERVER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(serverConf, "pinot.server.adminapi", tlsDefaults));
+
+ return listeners;
+ }
+
+ private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
+ TlsConfig tlsDefaults) {
+ String protocolNamespace = namespace + DOT_ACCESS_PROTOCOLS + "." + protocol;
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, config, protocolNamespace + ".tls");
+ tlsConfig.setEnabled(CommonConstants.HTTPS_PROTOCOL.equals(protocol));
+
+ return new ListenerConfig(protocol,
+ getHost(config.getProperty(protocolNamespace + ".host", DEFAULT_HOST)),
+ getPort(config.getProperty(protocolNamespace + ".port")), protocol, tlsConfig);
+ }
+
+ private static String getHost(String configuredHost) {
+ return Optional.ofNullable(configuredHost).filter(host -> !host.trim().isEmpty())
+ .orElseThrow(() -> new IllegalArgumentException(configuredHost + " is not a valid host"));
+ }
+
+ private static int getPort(String configuredPort) {
+ return Optional.ofNullable(configuredPort).filter(port -> !port.trim().isEmpty()).<Integer> map(Integer::valueOf)
+ .orElseThrow(() -> new IllegalArgumentException(configuredPort + " is not a valid port"));
+ }
+
+ public static HttpServer buildHttpServer(ResourceConfig resConfig, List<ListenerConfig> listenerConfigs) {
+ Preconditions.checkNotNull(listenerConfigs);
+
+ // The URI is irrelevant since the default listener will be manually rewritten.
+ HttpServer httpServer = GrizzlyHttpServerFactory.createHttpServer(URI.create("http://0.0.0.0/"), resConfig, false);
+
+ // Listeners cannot be configured with the factory. Manual overrides are required as instructed by Javadoc.
+ httpServer.removeListener("grizzly");
+
+ listenerConfigs.forEach(listenerConfig -> configureListener(httpServer, listenerConfig));
+
+ try {
+ httpServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to start http server", e);
+ }
+
+ return httpServer;
+ }
+
+ public static void configureListener(HttpServer httpServer, ListenerConfig listenerConfig) {
+ final NetworkListener listener = new NetworkListener(listenerConfig.getName() + "-" + listenerConfig.getPort(),
+ listenerConfig.getHost(), listenerConfig.getPort());
+
+ listener.getTransport().getWorkerThreadPoolConfig()
+ .setThreadFactory(new ThreadFactoryBuilder().setNameFormat("grizzly-http-server-%d")
+ .setUncaughtExceptionHandler(new JerseyProcessingUncaughtExceptionHandler()).build());
+
+ if (listenerConfig.getTlsConfig().isEnabled()) {
+ listener.setSecure(true);
+ listener.setSSLEngineConfig(buildSSLConfig(listenerConfig.getTlsConfig()));
+ }
+
+ httpServer.addListener(listener);
+ }
+
+ private static SSLEngineConfigurator buildSSLConfig(TlsConfig tlsConfig) {
+ SSLContextConfigurator sslContextConfigurator = new SSLContextConfigurator();
+
+ sslContextConfigurator.setKeyStoreFile(tlsConfig.getKeyStorePath());
+ sslContextConfigurator.setKeyStorePass(tlsConfig.getKeyStorePassword());
+ sslContextConfigurator.setTrustStoreFile(tlsConfig.getTrustStorePath());
+ sslContextConfigurator.setTrustStorePass(tlsConfig.getTrustStorePassword());
+
+ return new SSLEngineConfigurator(sslContextConfigurator).setClientMode(false)
+ .setNeedClientAuth(tlsConfig.isClientAuthEnabled()).setEnabledProtocols(new String[] { "TLSv1.2" });
+ }
+
+ public static String toString(Collection<? extends ListenerConfig> listenerConfigs) {
Review comment:
It takes a collection. imo keep the POJOs as clean as possible.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (504d0d8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.40%`.
> The diff coverage is `38.84%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
===========================================
- Coverage 66.44% 44.04% -22.41%
===========================================
Files 1075 1320 +245
Lines 54773 64417 +9644
Branches 8168 9374 +1206
===========================================
- Hits 36396 28372 -8024
- Misses 15700 33682 +17982
+ Partials 2677 2363 -314
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.04% <38.84%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1307 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...504d0d8](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r562797116
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
_brokerConf.addProperty(Broker.CONFIG_OF_BROKER_ID, _brokerId);
}
+ private int inferPort() {
+ return Optional.ofNullable(_brokerConf.getProperty(Helix.KEY_OF_BROKER_QUERY_PORT)).map(Integer::parseInt)
Review comment:
Thanks, I think this exception is ok, since we are the same behavior as the controller. But it is useful to throw the right error message in the exception mentioning which configuration the users should provide.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (9e46f91) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.90%`.
> The diff coverage is `72.57%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
+ Coverage 66.44% 73.35% +6.90%
==========================================
Files 1075 1320 +245
Lines 54773 64425 +9652
Branches 8168 9379 +1211
==========================================
+ Hits 36396 47257 +10861
+ Misses 15700 14099 -1601
- Partials 2677 3069 +392
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.21% <38.84%> (?)` | |
| unittests | `64.94% <56.83%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../common/config/tuner/TableConfigTunerRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1RhYmxlQ29uZmlnVHVuZXJSZWdpc3RyeS5qYXZh) | `72.00% <ø> (ø)` | |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1111 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...9e46f91](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-760406406
I will review this soon. Meanwhile, I believe it is totally worth it to test an upgrade/downgrade scenario.
e.g. the controller is turned on to use https, but broker/server are not, and then the controller/broker is turned on, but server is not, and then finally the server is turned on to use https as well.
In each of these scenarios, it is useful to test the following steps: add a table, add a segment, query it, delete the segment, and delete the table. You can write a simple script that you can run on each scenario.
I know this is a bit of manual testing, but a few hours spent now, will save multiple issues in deployments, not to mention better user experience. It will also help in filling out the compat questions, and avoid problems when we turn on these configs.
Thanks for the patience.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557830134
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,258 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.UnknownHostException;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.commons.httpclient.ConnectTimeoutException;
+import org.apache.commons.httpclient.params.HttpConnectionParams;
+import org.apache.commons.httpclient.protocol.Protocol;
+import org.apache.commons.httpclient.protocol.SecureProtocolSocketFactory;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH_ENABLED = "client.auth.enabled";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration key, with optional defaults.
+ *
+ * @param pinotConfig pinot configuration
+ * @param namespace namespace prefix
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r556011781
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -177,6 +177,12 @@
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";
Review comment:
controllers, brokers, and servers now support multi-ingress for different protocol. this opens up a three-phase migration path by first enabling TLS-secured servers, then upgrading client connections to TLS, and finally shutting off unsecured server ingress.
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
----------------------------------------------------------------
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 pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-760611347
Can we unify the format of the config keys? Use either `*.tls.*` or `*.protocol.https.tls.*`, but not mixed for easier configuration. I would suggest the former as `tls` does not tie to `https`
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r554279196
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/TlsConfig.java
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.transport;
+
+/**
+ * Container object for TLS/SSL configuration of pinot clients and servers (netty, grizzly, etc.)
+ */
+public class TlsConfig {
+ private boolean _enabled;
+ private boolean _clientAuth;
Review comment:
done
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
Review comment:
done
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TlsUtils.java
##########
@@ -0,0 +1,182 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.FileInputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Utility class for shared TLS configuration logic
+ */
+public final class TlsUtils {
+ private static final String ENABLED = "enabled";
+ private static final String CLIENT_AUTH = "client.auth";
+ private static final String KEYSTORE_PATH = "keystore.path";
+ private static final String KEYSTORE_PASSWORD = "keystore.password";
+ private static final String TRUSTSTORE_PATH = "truststore.path";
+ private static final String TRUSTSTORE_PASSWORD = "truststore.password";
+
+ private TlsUtils() {
+ // left blank
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys.
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix) {
+ return extractTlsConfig(new TlsConfig(), pinotConfig, prefix);
+ }
+
+ /**
+ * Extract a TlsConfig instance from a namespaced set of configuration keys, with defaults pulled from an alternative
+ * namespace
+ *
+ * @param pinotConfig pinot configuration
+ * @param prefix namespace prefix
+ * @param prefixDefaults namespace prefix for defaults
+ *
+ * @return TlsConfig instance
+ */
+ public static TlsConfig extractTlsConfig(PinotConfiguration pinotConfig, String prefix, String prefixDefaults) {
+ return extractTlsConfig(extractTlsConfig(pinotConfig, prefixDefaults), pinotConfig, prefix);
+ }
+
+ /**
+ * Create a KeyManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return KeyManagerFactory
+ */
+ public static KeyManagerFactory createKeyManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePath(), "key store path is null");
+ Preconditions.checkNotNull(tlsConfig.getKeyStorePassword(), "key store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getKeyStorePath())) {
+ keyStore.load(is, tlsConfig.getKeyStorePassword().toCharArray());
+ }
+
+ KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ keyManagerFactory.init(keyStore, tlsConfig.getKeyStorePassword().toCharArray());
+
+ return keyManagerFactory;
+
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create key manager factory '%s'",
+ tlsConfig.getKeyStorePath()), e);
+ }
+ }
+
+ /**
+ * Create a TrustManagerFactory instance from a given TlsConfig.
+ *
+ * @param tlsConfig TLS config
+ *
+ * @return TrustManagerFactory
+ */
+ public static TrustManagerFactory createTrustManagerFactory(TlsConfig tlsConfig) {
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePath(), "trust store path is null");
+ Preconditions.checkNotNull(tlsConfig.getTrustStorePassword(), "trust store password is null");
+
+ try {
+ KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+ try (FileInputStream is = new FileInputStream(tlsConfig.getTrustStorePath())) {
+ keyStore.load(is, tlsConfig.getTrustStorePassword().toCharArray());
+ }
+
+ TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ trustManagerFactory.init(keyStore);
+
+ return trustManagerFactory;
+ } catch (Exception e) {
+ throw new RuntimeException(String.format("Could not create trust manager factory '%s'",
+ tlsConfig.getTrustStorePath()), e);
+ }
+ }
+
+ /**
+ * Installs a default TLS socket factory for all HttpsURLConnection instances based on a given TlsConfig (1 or 2-way)
+ *
+ * @param tlsConfig TLS config
+ */
+ public static void installDefaultSSLSocketFactory(TlsConfig tlsConfig) {
+ KeyManager[] keyManagers = null;
+ if (tlsConfig.getKeyStorePath() != null) {
+ keyManagers = createKeyManagerFactory(tlsConfig).getKeyManagers();
+ }
+
+ TrustManager[] trustManagers = null;
+ if (tlsConfig.getTrustStorePath() != null) {
+ trustManagers = createTrustManagerFactory(tlsConfig).getTrustManagers();
+ }
+
+ try {
+ SSLContext sc = SSLContext.getInstance("SSL");
+ sc.init(keyManagers, trustManagers, new java.security.SecureRandom());
+ HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
+ } catch (GeneralSecurityException ignore) {
+ // ignore
Review comment:
fixed
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -238,14 +240,16 @@ public void start()
// Initialize FunctionRegistry before starting the broker request handler
FunctionRegistry.init();
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
+ // Configure TLS
Review comment:
done
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java
##########
@@ -58,20 +69,56 @@ 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, "broker client port must be > 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));
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_CLIENT_TLS_PREFIX, CommonConstants.Broker.BROKER_TLS_PREFIX);
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] mcvsubbu commented on a change in pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557739666
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
Review comment:
May be a problem here. If a broker is registered as Broker_hostxxx_8990, can it now come up as Broker_hostxxx_9661 (where 9661 is the https port)? This may mean that admins need to change the instance config for the broker, and the admins should be aware of that, right? Good to include as notes in the checkin comments, so that docs can follow that.
Let me know if my understanding is wrong.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -413,10 +413,21 @@ protected void configure() {
}
});
- _adminApp.start(_listenerConfigs);
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(_config, ControllerConf.CONTROLLER_TLS_PREFIX);
+
+ // install default SSL context if necessary
+ if (CommonConstants.HTTPS_PROTOCOL.equals(_config.getProperty(ControllerConf.CONTROLLER_BROKER_PROTOCOL))) {
+ LOGGER.info("Installing default SSL context for broker relay requests");
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, _config, ControllerConf.CONTROLLER_BROKER_TLS_PREFIX);
Review comment:
Does it make sense to move this to ListenerConfigUtil.buildControllerConfigs() method? It has all the data needed to install the right factory. Correct?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
##########
@@ -413,10 +413,21 @@ protected void configure() {
}
});
- _adminApp.start(_listenerConfigs);
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(_config, ControllerConf.CONTROLLER_TLS_PREFIX);
+
+ // install default SSL context if necessary
+ if (CommonConstants.HTTPS_PROTOCOL.equals(_config.getProperty(ControllerConf.CONTROLLER_BROKER_PROTOCOL))) {
+ LOGGER.info("Installing default SSL context for broker relay requests");
Review comment:
```suggestion
LOGGER.info("Installing default SSL context for forwarding console query requests to broker");
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
+ private static final String DEFAULT_HOST = "0.0.0.0";
+ private static final String DOT_ACCESS_PROTOCOLS = ".access.protocols";
+
+ public static final Set<String> SUPPORTED_PROTOCOLS = new HashSet<>(
+ Arrays.asList(CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL));
+
+ /**
+ * Generates {@link ListenerConfig} instances based on the combination
+ * of properties such as *.port and *.access.protocols.
+ *
+ * @param config property holders for controller configuration
+ * @param namespace property namespace to extract from
+ *
+ * @return List of {@link ListenerConfig} for which http listeners
+ * should be created.
+ */
+ public static List<ListenerConfig> buildListenerConfigs(PinotConfiguration config, String namespace,
+ TlsConfig tlsDefaults) {
+ if (StringUtils.isBlank(config.getProperty(namespace + DOT_ACCESS_PROTOCOLS))) {
+ return new ArrayList<>();
+ }
+
+ String[] protocols = config.getProperty(namespace + DOT_ACCESS_PROTOCOLS).split(",");
+
+ return Arrays.stream(protocols)
+ .peek(protocol -> Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(protocol),
+ "Unsupported protocol '%s' in config namespace '%s'", protocol, namespace))
+ .map(protocol -> buildListenerConfig(config, namespace, protocol, tlsDefaults))
+ .collect(Collectors.toList());
+ }
+
+ public static List<ListenerConfig> buildControllerConfigs(PinotConfiguration controllerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String portString = controllerConf.getProperty("controller.port");
+ if (portString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(portString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(controllerConf, "controller.tls");
+
+ listeners.addAll(buildListenerConfigs(controllerConf, "controller", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildBrokerConfigs(PinotConfiguration brokerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String queryPortString = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT);
+ if (queryPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(queryPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(brokerConf, "pinot.broker.client", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration serverConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String adminApiPortString = serverConf.getProperty(CommonConstants.Server.CONFIG_OF_ADMIN_API_PORT);
+ if (adminApiPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(adminApiPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(serverConf, CommonConstants.Server.SERVER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(serverConf, "pinot.server.adminapi", tlsDefaults));
+
+ return listeners;
+ }
+
+ private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
+ TlsConfig tlsDefaults) {
+ String protocolNamespace = namespace + DOT_ACCESS_PROTOCOLS + "." + protocol;
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, config, protocolNamespace + ".tls");
+ tlsConfig.setEnabled(CommonConstants.HTTPS_PROTOCOL.equals(protocol));
+
+ return new ListenerConfig(protocol,
+ getHost(config.getProperty(protocolNamespace + ".host", DEFAULT_HOST)),
+ getPort(config.getProperty(protocolNamespace + ".port")), protocol, tlsConfig);
+ }
+
+ private static String getHost(String configuredHost) {
+ return Optional.ofNullable(configuredHost).filter(host -> !host.trim().isEmpty())
+ .orElseThrow(() -> new IllegalArgumentException(configuredHost + " is not a valid host"));
+ }
+
+ private static int getPort(String configuredPort) {
+ return Optional.ofNullable(configuredPort).filter(port -> !port.trim().isEmpty()).<Integer> map(Integer::valueOf)
+ .orElseThrow(() -> new IllegalArgumentException(configuredPort + " is not a valid port"));
+ }
+
+ public static HttpServer buildHttpServer(ResourceConfig resConfig, List<ListenerConfig> listenerConfigs) {
+ Preconditions.checkNotNull(listenerConfigs);
+
+ // The URI is irrelevant since the default listener will be manually rewritten.
+ HttpServer httpServer = GrizzlyHttpServerFactory.createHttpServer(URI.create("http://0.0.0.0/"), resConfig, false);
+
+ // Listeners cannot be configured with the factory. Manual overrides are required as instructed by Javadoc.
+ httpServer.removeListener("grizzly");
+
+ listenerConfigs.forEach(listenerConfig -> configureListener(httpServer, listenerConfig));
+
+ try {
+ httpServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to start http server", e);
+ }
+
+ return httpServer;
+ }
+
+ public static void configureListener(HttpServer httpServer, ListenerConfig listenerConfig) {
+ final NetworkListener listener = new NetworkListener(listenerConfig.getName() + "-" + listenerConfig.getPort(),
+ listenerConfig.getHost(), listenerConfig.getPort());
+
+ listener.getTransport().getWorkerThreadPoolConfig()
+ .setThreadFactory(new ThreadFactoryBuilder().setNameFormat("grizzly-http-server-%d")
+ .setUncaughtExceptionHandler(new JerseyProcessingUncaughtExceptionHandler()).build());
+
+ if (listenerConfig.getTlsConfig().isEnabled()) {
+ listener.setSecure(true);
+ listener.setSSLEngineConfig(buildSSLConfig(listenerConfig.getTlsConfig()));
+ }
+
+ httpServer.addListener(listener);
+ }
+
+ private static SSLEngineConfigurator buildSSLConfig(TlsConfig tlsConfig) {
+ SSLContextConfigurator sslContextConfigurator = new SSLContextConfigurator();
+
+ sslContextConfigurator.setKeyStoreFile(tlsConfig.getKeyStorePath());
+ sslContextConfigurator.setKeyStorePass(tlsConfig.getKeyStorePassword());
+ sslContextConfigurator.setTrustStoreFile(tlsConfig.getTrustStorePath());
+ sslContextConfigurator.setTrustStorePass(tlsConfig.getTrustStorePassword());
+
+ return new SSLEngineConfigurator(sslContextConfigurator).setClientMode(false)
+ .setNeedClientAuth(tlsConfig.isClientAuthEnabled()).setEnabledProtocols(new String[] { "TLSv1.2" });
+ }
+
+ public static String toString(Collection<? extends ListenerConfig> listenerConfigs) {
Review comment:
Why not move this to ListenerConfig class?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
Review comment:
Why abstract?
Also, can we avoid static methods if possible? We can new() one of these. Makes it easier to test, move around, etc.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
+ private static final String DEFAULT_HOST = "0.0.0.0";
+ private static final String DOT_ACCESS_PROTOCOLS = ".access.protocols";
+
+ public static final Set<String> SUPPORTED_PROTOCOLS = new HashSet<>(
+ Arrays.asList(CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL));
+
+ /**
+ * Generates {@link ListenerConfig} instances based on the combination
+ * of properties such as *.port and *.access.protocols.
+ *
+ * @param config property holders for controller configuration
+ * @param namespace property namespace to extract from
+ *
+ * @return List of {@link ListenerConfig} for which http listeners
+ * should be created.
+ */
+ public static List<ListenerConfig> buildListenerConfigs(PinotConfiguration config, String namespace,
+ TlsConfig tlsDefaults) {
+ if (StringUtils.isBlank(config.getProperty(namespace + DOT_ACCESS_PROTOCOLS))) {
+ return new ArrayList<>();
+ }
+
+ String[] protocols = config.getProperty(namespace + DOT_ACCESS_PROTOCOLS).split(",");
+
+ return Arrays.stream(protocols)
+ .peek(protocol -> Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(protocol),
+ "Unsupported protocol '%s' in config namespace '%s'", protocol, namespace))
+ .map(protocol -> buildListenerConfig(config, namespace, protocol, tlsDefaults))
+ .collect(Collectors.toList());
+ }
+
+ public static List<ListenerConfig> buildControllerConfigs(PinotConfiguration controllerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String portString = controllerConf.getProperty("controller.port");
+ if (portString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(portString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(controllerConf, "controller.tls");
+
+ listeners.addAll(buildListenerConfigs(controllerConf, "controller", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildBrokerConfigs(PinotConfiguration brokerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String queryPortString = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT);
+ if (queryPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(queryPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(brokerConf, "pinot.broker.client", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration serverConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String adminApiPortString = serverConf.getProperty(CommonConstants.Server.CONFIG_OF_ADMIN_API_PORT);
+ if (adminApiPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(adminApiPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(serverConf, CommonConstants.Server.SERVER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(serverConf, "pinot.server.adminapi", tlsDefaults));
+
+ return listeners;
+ }
+
+ private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
+ TlsConfig tlsDefaults) {
+ String protocolNamespace = namespace + DOT_ACCESS_PROTOCOLS + "." + protocol;
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, config, protocolNamespace + ".tls");
+ tlsConfig.setEnabled(CommonConstants.HTTPS_PROTOCOL.equals(protocol));
+
+ return new ListenerConfig(protocol,
+ getHost(config.getProperty(protocolNamespace + ".host", DEFAULT_HOST)),
+ getPort(config.getProperty(protocolNamespace + ".port")), protocol, tlsConfig);
+ }
+
+ private static String getHost(String configuredHost) {
+ return Optional.ofNullable(configuredHost).filter(host -> !host.trim().isEmpty())
+ .orElseThrow(() -> new IllegalArgumentException(configuredHost + " is not a valid host"));
+ }
+
+ private static int getPort(String configuredPort) {
+ return Optional.ofNullable(configuredPort).filter(port -> !port.trim().isEmpty()).<Integer> map(Integer::valueOf)
+ .orElseThrow(() -> new IllegalArgumentException(configuredPort + " is not a valid port"));
+ }
+
+ public static HttpServer buildHttpServer(ResourceConfig resConfig, List<ListenerConfig> listenerConfigs) {
+ Preconditions.checkNotNull(listenerConfigs);
+
+ // The URI is irrelevant since the default listener will be manually rewritten.
+ HttpServer httpServer = GrizzlyHttpServerFactory.createHttpServer(URI.create("http://0.0.0.0/"), resConfig, false);
+
+ // Listeners cannot be configured with the factory. Manual overrides are required as instructed by Javadoc.
Review comment:
pointer to the javadoc will be nice
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (80f62e9) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.57%`.
> The diff coverage is `58.23%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
- Coverage 66.44% 64.87% -1.58%
==========================================
Files 1075 1334 +259
Lines 54773 65525 +10752
Branches 8168 9553 +1385
==========================================
+ Hits 36396 42507 +6111
- Misses 15700 19979 +4279
- Partials 2677 3039 +362
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.87% <58.23%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1176 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [2e7cdcd...80f62e9](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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] mcvsubbu commented on pull request #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-760422087
> > I will review this soon. Meanwhile, I believe it is totally worth it to test an upgrade/downgrade scenario.
> > e.g. the controller is turned on to use https, but broker/server are not, and then the controller/broker is turned on, but server is not, and then finally the server is turned on to use https as well.
> > In each of these scenarios, it is useful to test the following steps: add a table, add a segment, query it, delete the segment, and delete the table. You can write a simple script that you can run on each scenario.
> > I know this is a bit of manual testing, but a few hours spent now, will save multiple issues in deployments, not to mention better user experience. It will also help in filling out the compat questions, and avoid problems when we turn on these configs.
> > Thanks for the patience.
>
> I missed a couple of more tests : (1) execute a query from the console (tests the controller/broker path) (2) Execute the tablesize api on the controller (tests the controller/server path)
Or, did I get the upgrade order wrong? Perhaps we need to upgrade to the new release, and turn on https in the order:
server, broker and then controller. And then turn off http in any order amongst these components. Correct?
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#issuecomment-755917563
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=h1) Report
> Merging [#6418](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=desc) (ed6c724) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.85%`.
> The diff coverage is `72.57%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6418?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6418 +/- ##
==========================================
+ Coverage 66.44% 73.30% +6.85%
==========================================
Files 1075 1321 +246
Lines 54773 64578 +9805
Branches 8168 9414 +1246
==========================================
+ Hits 36396 47338 +10942
+ Misses 15700 14147 -1553
- Partials 2677 3093 +416
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.06% <38.84%> (?)` | |
| unittests | `64.98% <56.83%> (?)` | |
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/6418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/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/6418/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/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../common/config/tuner/TableConfigTunerRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1RhYmxlQ29uZmlnVHVuZXJSZWdpc3RyeS5qYXZh) | `72.00% <ø> (ø)` | |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6418/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/6418/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1110 more](https://codecov.io/gh/apache/incubator-pinot/pull/6418/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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/6418?src=pr&el=footer). Last update [f09de82...ed6c724](https://codecov.io/gh/apache/incubator-pinot/pull/6418?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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557827940
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
+ private static final String DEFAULT_HOST = "0.0.0.0";
+ private static final String DOT_ACCESS_PROTOCOLS = ".access.protocols";
+
+ public static final Set<String> SUPPORTED_PROTOCOLS = new HashSet<>(
+ Arrays.asList(CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL));
+
+ /**
+ * Generates {@link ListenerConfig} instances based on the combination
+ * of properties such as *.port and *.access.protocols.
+ *
+ * @param config property holders for controller configuration
+ * @param namespace property namespace to extract from
+ *
+ * @return List of {@link ListenerConfig} for which http listeners
+ * should be created.
+ */
+ public static List<ListenerConfig> buildListenerConfigs(PinotConfiguration config, String namespace,
+ TlsConfig tlsDefaults) {
+ if (StringUtils.isBlank(config.getProperty(namespace + DOT_ACCESS_PROTOCOLS))) {
+ return new ArrayList<>();
+ }
+
+ String[] protocols = config.getProperty(namespace + DOT_ACCESS_PROTOCOLS).split(",");
+
+ return Arrays.stream(protocols)
+ .peek(protocol -> Preconditions.checkArgument(SUPPORTED_PROTOCOLS.contains(protocol),
+ "Unsupported protocol '%s' in config namespace '%s'", protocol, namespace))
+ .map(protocol -> buildListenerConfig(config, namespace, protocol, tlsDefaults))
+ .collect(Collectors.toList());
+ }
+
+ public static List<ListenerConfig> buildControllerConfigs(PinotConfiguration controllerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String portString = controllerConf.getProperty("controller.port");
+ if (portString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(portString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(controllerConf, "controller.tls");
+
+ listeners.addAll(buildListenerConfigs(controllerConf, "controller", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildBrokerConfigs(PinotConfiguration brokerConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String queryPortString = brokerConf.getProperty(CommonConstants.Helix.KEY_OF_BROKER_QUERY_PORT);
+ if (queryPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(queryPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(brokerConf, CommonConstants.Broker.BROKER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(brokerConf, "pinot.broker.client", tlsDefaults));
+
+ return listeners;
+ }
+
+ public static List<ListenerConfig> buildServerAdminConfigs(PinotConfiguration serverConf) {
+ List<ListenerConfig> listeners = new ArrayList<>();
+
+ String adminApiPortString = serverConf.getProperty(CommonConstants.Server.CONFIG_OF_ADMIN_API_PORT);
+ if (adminApiPortString != null) {
+ listeners.add(new ListenerConfig(CommonConstants.HTTP_PROTOCOL, DEFAULT_HOST, Integer.parseInt(adminApiPortString),
+ CommonConstants.HTTP_PROTOCOL, new TlsConfig()));
+ }
+
+ TlsConfig tlsDefaults = TlsUtils.extractTlsConfig(serverConf, CommonConstants.Server.SERVER_TLS_PREFIX);
+
+ listeners.addAll(buildListenerConfigs(serverConf, "pinot.server.adminapi", tlsDefaults));
+
+ return listeners;
+ }
+
+ private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
+ TlsConfig tlsDefaults) {
+ String protocolNamespace = namespace + DOT_ACCESS_PROTOCOLS + "." + protocol;
+
+ TlsConfig tlsConfig = TlsUtils.extractTlsConfig(tlsDefaults, config, protocolNamespace + ".tls");
+ tlsConfig.setEnabled(CommonConstants.HTTPS_PROTOCOL.equals(protocol));
+
+ return new ListenerConfig(protocol,
+ getHost(config.getProperty(protocolNamespace + ".host", DEFAULT_HOST)),
+ getPort(config.getProperty(protocolNamespace + ".port")), protocol, tlsConfig);
+ }
+
+ private static String getHost(String configuredHost) {
+ return Optional.ofNullable(configuredHost).filter(host -> !host.trim().isEmpty())
+ .orElseThrow(() -> new IllegalArgumentException(configuredHost + " is not a valid host"));
+ }
+
+ private static int getPort(String configuredPort) {
+ return Optional.ofNullable(configuredPort).filter(port -> !port.trim().isEmpty()).<Integer> map(Integer::valueOf)
+ .orElseThrow(() -> new IllegalArgumentException(configuredPort + " is not a valid port"));
+ }
+
+ public static HttpServer buildHttpServer(ResourceConfig resConfig, List<ListenerConfig> listenerConfigs) {
+ Preconditions.checkNotNull(listenerConfigs);
+
+ // The URI is irrelevant since the default listener will be manually rewritten.
+ HttpServer httpServer = GrizzlyHttpServerFactory.createHttpServer(URI.create("http://0.0.0.0/"), resConfig, false);
+
+ // Listeners cannot be configured with the factory. Manual overrides are required as instructed by Javadoc.
+ httpServer.removeListener("grizzly");
+
+ listenerConfigs.forEach(listenerConfig -> configureListener(httpServer, listenerConfig));
+
+ try {
Review comment:
changed
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557833005
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
##########
@@ -117,13 +124,18 @@ public HelixBrokerStarter(PinotConfiguration brokerConf, String clusterName, Str
brokerHost = _brokerConf.getProperty(CommonConstants.Helix.SET_INSTANCE_ID_TO_HOSTNAME_KEY, false) ? NetUtil
.getHostnameOrAddress() : NetUtil.getHostAddress();
}
+
_brokerId = _brokerConf.getProperty(Helix.Instance.INSTANCE_ID_KEY,
- Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + _brokerConf
- .getProperty(Helix.KEY_OF_BROKER_QUERY_PORT, Helix.DEFAULT_BROKER_QUERY_PORT));
+ Helix.PREFIX_OF_BROKER_INSTANCE + brokerHost + "_" + inferPort());
Review comment:
We'll pick whichever port is configured first - which is usually the HTTP port, even in a multi-ingress scenario. If only HTTPS is enabled then that explicitly-configured port will be used for the instance id.
Is this what you suggest I note in the release-notes?
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557831263
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/transport/QueryRouter.java
##########
@@ -60,20 +84,23 @@ public AsyncQueryResponse submitQuery(long requestId, String rawTableName,
long timeoutMs) {
assert offlineBrokerRequest != null || realtimeBrokerRequest != null;
+ // can prefer but not require TLS until all servers guaranteed to be on TLS
Review comment:
prefers != requires. the call to `toServerRoutingInstance()` below will attempt to retrieve a ServerRoutingInstance with `tlsEnabled == true` but may not be successful in doing so (if the server doesn't offer netty-tls yet). In that case, we connect via unsecured netty (see QueryRouter:115)
----------------------------------------------------------------
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 #6418: TLS-support for client-pinot and pinot-internode connections
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6418:
URL: https://github.com/apache/incubator-pinot/pull/6418#discussion_r557833602
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/ListenerConfigUtil.java
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.core.transport.ListenerConfig;
+import org.apache.pinot.core.transport.TlsConfig;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.glassfish.grizzly.http.server.HttpServer;
+import org.glassfish.grizzly.http.server.NetworkListener;
+import org.glassfish.grizzly.ssl.SSLContextConfigurator;
+import org.glassfish.grizzly.ssl.SSLEngineConfigurator;
+import org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpServerFactory;
+import org.glassfish.jersey.internal.guava.ThreadFactoryBuilder;
+import org.glassfish.jersey.process.JerseyProcessingUncaughtExceptionHandler;
+import org.glassfish.jersey.server.ResourceConfig;
+
+
+/**
+ * Utility class that generates Http {@link ListenerConfig} instances
+ * based on the properties provided by a property namespace in {@link PinotConfiguration}.
+ */
+public abstract class ListenerConfigUtil {
Review comment:
made final, with private constructor. should never be instantiated.
----------------------------------------------------------------
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