diff --git a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java index 9df8da6675d0..8318a865bc72 100755 --- a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java +++ b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java @@ -23,6 +23,7 @@ import io.airlift.units.MinDuration; import javax.annotation.PostConstruct; +import javax.validation.constraints.NotEmpty; import javax.validation.constraints.NotNull; import java.net.URI; @@ -64,7 +65,7 @@ public class PinotConfig private boolean grpcEnabled = true; private DataSize targetSegmentPageSize = DataSize.of(1, MEGABYTE); - @NotNull + @NotEmpty(message = "pinot.controller-urls cannot be empty") public List getControllerUrls() { return controllerUrls; diff --git a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java index efd687343daf..268b805996c2 100755 --- a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java +++ b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java @@ -90,7 +90,6 @@ import static io.trino.collect.cache.SafeCaches.buildNonEvictableCache; import static io.trino.plugin.pinot.PinotErrorCode.PINOT_AMBIGUOUS_TABLE_NAME; import static io.trino.plugin.pinot.PinotErrorCode.PINOT_EXCEPTION; -import static io.trino.plugin.pinot.PinotErrorCode.PINOT_INVALID_CONFIGURATION; import static io.trino.plugin.pinot.PinotErrorCode.PINOT_UNABLE_TO_FIND_BROKER; import static io.trino.plugin.pinot.PinotMetadata.SCHEMA_NAME; import static java.lang.String.format; @@ -152,9 +151,6 @@ public PinotClient( .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)).jsonCodec(Schema.class); this.brokerResponseCodec = requireNonNull(brokerResponseCodec, "brokerResponseCodec is null"); requireNonNull(config, "config is null"); - if (config.getControllerUrls() == null || config.getControllerUrls().isEmpty()) { - throw new PinotException(PINOT_INVALID_CONFIGURATION, Optional.empty(), "No pinot controllers specified"); - } this.pinotHostMapper = requireNonNull(pinotHostMapper, "pinotHostMapper is null"); this.controllerUrls = config.getControllerUrls(); diff --git a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java index eee143e123c4..00354584c8e4 100644 --- a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java +++ b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java @@ -178,13 +178,13 @@ public static class TlsGrpcQueryClientFactory implements GrpcQueryClientFactory { // Extracted from org.apache.pinot.common.utils.TlsUtils - private static final String KEYSTORE_TYPE = "keystore.type"; - private static final String KEYSTORE_PATH = "keystore.path"; - private static final String KEYSTORE_PASSWORD = "keystore.password"; - private static final String TRUSTSTORE_TYPE = "truststore.type"; - private static final String TRUSTSTORE_PATH = "truststore.path"; - private static final String TRUSTSTORE_PASSWORD = "truststore.password"; - private static final String SSL_PROVIDER = "ssl.provider"; + private static final String KEYSTORE_TYPE = GRPC_TLS_PREFIX + "." + "keystore.type"; + private static final String KEYSTORE_PATH = GRPC_TLS_PREFIX + "." + "keystore.path"; + private static final String KEYSTORE_PASSWORD = GRPC_TLS_PREFIX + "." + "keystore.password"; + private static final String TRUSTSTORE_TYPE = GRPC_TLS_PREFIX + "." + "truststore.type"; + private static final String TRUSTSTORE_PATH = GRPC_TLS_PREFIX + "." + "truststore.path"; + private static final String TRUSTSTORE_PASSWORD = GRPC_TLS_PREFIX + "." + "truststore.password"; + private static final String SSL_PROVIDER = GRPC_TLS_PREFIX + "." + "ssl.provider"; private final GrpcQueryClient.Config config; @@ -193,17 +193,21 @@ public TlsGrpcQueryClientFactory(PinotGrpcServerQueryClientConfig grpcClientConf { requireNonNull(grpcClientConfig, "grpcClientConfig is null"); requireNonNull(tlsConfig, "tlsConfig is null"); - this.config = new GrpcQueryClient.Config(ImmutableMap.builder() - .put(CONFIG_MAX_INBOUND_MESSAGE_BYTES_SIZE, String.valueOf(grpcClientConfig.getMaxInboundMessageSize().toBytes())) - .put(CONFIG_USE_PLAIN_TEXT, String.valueOf(grpcClientConfig.isUsePlainText())) - .put(GRPC_TLS_PREFIX + "." + KEYSTORE_TYPE, tlsConfig.getKeystoreType()) - .put(GRPC_TLS_PREFIX + "." + KEYSTORE_PATH, tlsConfig.getKeystorePath()) - .put(GRPC_TLS_PREFIX + "." + KEYSTORE_PASSWORD, tlsConfig.getKeystorePassword()) - .put(GRPC_TLS_PREFIX + "." + TRUSTSTORE_TYPE, tlsConfig.getTruststoreType()) - .put(GRPC_TLS_PREFIX + "." + TRUSTSTORE_PATH, tlsConfig.getTruststorePath()) - .put(GRPC_TLS_PREFIX + "." + TRUSTSTORE_PASSWORD, tlsConfig.getTruststorePassword()) - .put(GRPC_TLS_PREFIX + "." + SSL_PROVIDER, tlsConfig.getSslProvider()) - .buildOrThrow()); + + ImmutableMap.Builder tlsConfigBuilder = ImmutableMap.builder(); + if (tlsConfig.getKeystorePath().isPresent()) { + tlsConfigBuilder.put(KEYSTORE_TYPE, tlsConfig.getKeystoreType()); + tlsConfigBuilder.put(KEYSTORE_PATH, tlsConfig.getKeystorePath().get()); + tlsConfig.getKeystorePassword().ifPresent(password -> tlsConfigBuilder.put(KEYSTORE_PASSWORD, password)); + } + if (tlsConfig.getTruststorePath().isPresent()) { + tlsConfigBuilder.put(TRUSTSTORE_TYPE, tlsConfig.getTruststoreType()); + tlsConfigBuilder.put(TRUSTSTORE_PATH, tlsConfig.getTruststorePath().get()); + tlsConfig.getTruststorePassword().ifPresent(password -> tlsConfigBuilder.put(TRUSTSTORE_PASSWORD, password)); + } + tlsConfigBuilder.put(SSL_PROVIDER, tlsConfig.getSslProvider()); + + this.config = new GrpcQueryClient.Config(tlsConfigBuilder.buildOrThrow()); } @Override diff --git a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java index 0d931b8b05f1..2183f34f064e 100644 --- a/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java +++ b/plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java @@ -13,20 +13,32 @@ */ package io.trino.plugin.pinot.client; +import com.google.common.collect.ImmutableList; +import com.google.inject.ConfigurationException; +import com.google.inject.spi.Message; import io.airlift.configuration.Config; +import io.airlift.configuration.ConfigSecuritySensitive; +import io.airlift.configuration.validation.FileExists; + +import javax.annotation.PostConstruct; +import javax.validation.constraints.NotNull; + +import java.io.File; +import java.util.Optional; import static io.trino.plugin.pinot.client.PinotKeystoreTrustStoreType.JKS; public class PinotGrpcServerQueryClientTlsConfig { private PinotKeystoreTrustStoreType keystoreType = JKS; - private String keystorePath; + private File keystorePath; private String keystorePassword; private PinotKeystoreTrustStoreType truststoreType = JKS; - private String truststorePath; + private File truststorePath; private String truststorePassword; private String sslProvider = "JDK"; + @NotNull public PinotKeystoreTrustStoreType getKeystoreType() { return keystoreType; @@ -39,30 +51,32 @@ public PinotGrpcServerQueryClientTlsConfig setKeystoreType(PinotKeystoreTrustSto return this; } - public String getKeystorePath() + public Optional<@FileExists File> getKeystorePath() { - return keystorePath; + return Optional.ofNullable(keystorePath); } @Config("pinot.grpc.tls.keystore-path") - public PinotGrpcServerQueryClientTlsConfig setKeystorePath(String keystorePath) + public PinotGrpcServerQueryClientTlsConfig setKeystorePath(File keystorePath) { this.keystorePath = keystorePath; return this; } - public String getKeystorePassword() + public Optional getKeystorePassword() { - return keystorePassword; + return Optional.ofNullable(keystorePassword); } @Config("pinot.grpc.tls.keystore-password") + @ConfigSecuritySensitive public PinotGrpcServerQueryClientTlsConfig setKeystorePassword(String keystorePassword) { this.keystorePassword = keystorePassword; return this; } + @NotNull public PinotKeystoreTrustStoreType getTruststoreType() { return truststoreType; @@ -75,30 +89,32 @@ public PinotGrpcServerQueryClientTlsConfig setTruststoreType(PinotKeystoreTrustS return this; } - public String getTruststorePath() + public Optional<@FileExists File> getTruststorePath() { - return truststorePath; + return Optional.ofNullable(truststorePath); } @Config("pinot.grpc.tls.truststore-path") - public PinotGrpcServerQueryClientTlsConfig setTruststorePath(String truststorePath) + public PinotGrpcServerQueryClientTlsConfig setTruststorePath(File truststorePath) { this.truststorePath = truststorePath; return this; } - public String getTruststorePassword() + public Optional getTruststorePassword() { - return truststorePassword; + return Optional.ofNullable(truststorePassword); } @Config("pinot.grpc.tls.truststore-password") + @ConfigSecuritySensitive public PinotGrpcServerQueryClientTlsConfig setTruststorePassword(String truststorePassword) { this.truststorePassword = truststorePassword; return this; } + @NotNull public String getSslProvider() { return sslProvider; @@ -110,4 +126,15 @@ public PinotGrpcServerQueryClientTlsConfig setSslProvider(String sslProvider) this.sslProvider = sslProvider; return this; } + + @PostConstruct + public void validate() + { + if (getKeystorePath().isPresent() && getKeystorePassword().isEmpty()) { + throw new ConfigurationException(ImmutableList.of(new Message("pinot.grpc.tls.keystore-password must set when pinot.grpc.tls.keystore-path is given"))); + } + if (getTruststorePath().isPresent() && getTruststorePassword().isEmpty()) { + throw new ConfigurationException(ImmutableList.of(new Message("pinot.grpc.tls.truststore-password must set when pinot.grpc.tls.truststore-path is given"))); + } + } } diff --git a/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java b/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java index 35dd18068086..27ce338851ba 100644 --- a/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java +++ b/plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java @@ -14,14 +14,21 @@ package io.trino.plugin.pinot; import com.google.common.collect.ImmutableMap; +import com.google.inject.ConfigurationException; import io.airlift.configuration.testing.ConfigAssertions; import io.trino.plugin.pinot.client.PinotGrpcServerQueryClientTlsConfig; import org.testng.annotations.Test; +import java.io.FileWriter; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.Map; import static io.trino.plugin.pinot.client.PinotKeystoreTrustStoreType.JKS; import static io.trino.plugin.pinot.client.PinotKeystoreTrustStoreType.PKCS12; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestPinotGrpcServerQueryClientTlsConfig { @@ -41,24 +48,68 @@ public void testDefaults() @Test public void testExplicitPropertyMappings() + throws Exception { + Path keystoreFile = Files.createTempFile(null, null); + Path truststoreFile = Files.createTempFile(null, null); + Map properties = ImmutableMap.builder() .put("pinot.grpc.tls.keystore-type", "PKCS12") - .put("pinot.grpc.tls.keystore-path", "/root") + .put("pinot.grpc.tls.keystore-path", keystoreFile.toString()) .put("pinot.grpc.tls.keystore-password", "password") .put("pinot.grpc.tls.truststore-type", "PKCS12") - .put("pinot.grpc.tls.truststore-path", "/root") + .put("pinot.grpc.tls.truststore-path", truststoreFile.toString()) .put("pinot.grpc.tls.truststore-password", "password") .put("pinot.grpc.tls.ssl-provider", "OPENSSL") .buildOrThrow(); PinotGrpcServerQueryClientTlsConfig expected = new PinotGrpcServerQueryClientTlsConfig() .setKeystoreType(PKCS12) - .setKeystorePath("/root") + .setKeystorePath(keystoreFile.toFile()) .setKeystorePassword("password") .setTruststoreType(PKCS12) - .setTruststorePath("/root") + .setTruststorePath(truststoreFile.toFile()) .setTruststorePassword("password") .setSslProvider("OPENSSL"); ConfigAssertions.assertFullMapping(properties, expected); } + + @Test + public void testFailOnMissingKeystorePasswordWithKeystorePathSet() + throws Exception + { + String secret = "pinot"; + Path keystorePath = Files.createTempFile("keystore", ".p12"); + + writeToFile(keystorePath, secret); + + PinotGrpcServerQueryClientTlsConfig config = new PinotGrpcServerQueryClientTlsConfig(); + config.setKeystorePath(keystorePath.toFile()); + assertThatThrownBy(config::validate) + .isInstanceOf(ConfigurationException.class) + .hasMessageContaining("pinot.grpc.tls.keystore-password must set when pinot.grpc.tls.keystore-path is given"); + } + + @Test + public void testFailOnMissingTruststorePasswordWithTruststorePathSet() + throws Exception + { + String secret = "pinot"; + Path truststorePath = Files.createTempFile("truststore", ".p12"); + + writeToFile(truststorePath, secret); + + PinotGrpcServerQueryClientTlsConfig config = new PinotGrpcServerQueryClientTlsConfig(); + config.setTruststorePath(truststorePath.toFile()); + assertThatThrownBy(config::validate) + .isInstanceOf(ConfigurationException.class) + .hasMessageContaining("pinot.grpc.tls.truststore-password must set when pinot.grpc.tls.truststore-path is given"); + } + + private void writeToFile(Path filepath, String content) + throws IOException + { + try (FileWriter writer = new FileWriter(filepath.toFile(), UTF_8)) { + writer.write(content); + } + } }