Skip to content

Minor improvement in Pinot config classes #13422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ebyhr merged 2 commits into from
Aug 1, 2022
Merged

Minor improvement in Pinot config classes #13422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61a229a
Use `@NotEmpty` annotation for pinot.controller-urls
ebyhr Jul 29, 2022
de36fe6
Improve Pinot gRPC TLS config class
ebyhr Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")
Comment thread
ebyhr marked this conversation as resolved.
Outdated
public List<URI> getControllerUrls()
{
return controllerUrls;
Expand Down
4 changes: 0 additions & 4 deletions plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
40 changes: 22 additions & 18 deletions plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcDataFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -193,17 +193,21 @@ public TlsGrpcQueryClientFactory(PinotGrpcServerQueryClientConfig grpcClientConf
{
requireNonNull(grpcClientConfig, "grpcClientConfig is null");
requireNonNull(tlsConfig, "tlsConfig is null");
this.config = new GrpcQueryClient.Config(ImmutableMap.<String, Object>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<String, Object> 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
Expand Down
51 changes: 39 additions & 12 deletions ...pinot/src/main/java/io/trino/plugin/pinot/client/PinotGrpcServerQueryClientTlsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String> 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;
Expand All @@ -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<String> 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;
Expand All @@ -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")));
}
}
Comment thread
hashhar marked this conversation as resolved.
}
59 changes: 55 additions & 4 deletions ...no-pinot/src/test/java/io/trino/plugin/pinot/TestPinotGrpcServerQueryClientTlsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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<String, String> properties = ImmutableMap.<String, String>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);
}
}
}