Skip to content
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.

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

Check if url has https protocol scheme and add test #71

Merged
merged 2 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions gateway-ha/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,12 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>com.h2database</groupId>
<artifactId>h2</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ public ClusterStats monitor(ProxyBackendConfiguration backend) {
jdbcUrl = String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are getting smarter about setting the port based on the protocol, we should also use the backend URL to set the SSL parameter. Currently it is set in BackendStateConfiguration, so if you have a mixture of http and https backends it will necessarily be invalid for some of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with SSL parameters. So I'm assuming SSL can be set depending on http or https?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep
https => SSL=true
http => SSL=false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add that and then I think we should be good to merge this soon.

.format("jdbc:trino://%s:%s/system",
parsedUrl.getHost(),
parsedUrl.getPort() == -1 ? 80 : parsedUrl.getPort()
parsedUrl.getPort() == -1 ? parsedUrl.getDefaultPort() : parsedUrl.getPort()
);
// automatically set ssl config based on url protocol
properties.setProperty("SSL", String.valueOf(parsedUrl.getProtocol().equals("https")));
} catch (MalformedURLException e) {
log.error("could not parse backend url {} ", url);
return clusterStats;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package io.trino.gateway.ha.clustermonitor;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.when;

import io.trino.gateway.ha.config.BackendStateConfiguration;
import io.trino.gateway.ha.config.ProxyBackendConfiguration;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.util.Properties;
import java.util.stream.Stream;
import lombok.extern.slf4j.Slf4j;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestInstance.Lifecycle;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.Mock;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;

@Slf4j
@TestInstance(Lifecycle.PER_CLASS)
public class TestClusterStatsJdbcMonitor {
ClusterStatsJdbcMonitor clusterStatsJdbcMonitor;
@Mock
private Connection connection;
@Mock
private PreparedStatement preparedStatement;
@Mock
private ResultSet resultSet;

private Properties properties;

@BeforeEach
public void initMocks() {
log.info("initializing test");
MockitoAnnotations.openMocks(this);
}

@AfterAll
public void resetMocks() {
log.info("resetting mocks");
Mockito.reset(connection);
Mockito.reset(preparedStatement);
Mockito.reset(resultSet);
}

@BeforeAll
public void setUp() {
BackendStateConfiguration backendStateConfiguration = new BackendStateConfiguration();
backendStateConfiguration.setUsername("Trino");
properties = new Properties();
properties.setProperty("user", backendStateConfiguration.getUsername());
properties.setProperty("password", backendStateConfiguration.getPassword());
properties.setProperty("SSL", String.valueOf(backendStateConfiguration.getSsl()));
clusterStatsJdbcMonitor = new ClusterStatsJdbcMonitor(backendStateConfiguration);
}

private static Stream<Arguments> provideProtocolAndPortAndSsl(){
return Stream.of(
Arguments.of("https", "90", "jdbc:trino://trino.example.com:90/system", "true"),
Arguments.of("http", "90", "jdbc:trino://trino.example.com:90/system", "false"),
Arguments.of("https", "", "jdbc:trino://trino.example.com:443/system", "true"),
Arguments.of("http", "", "jdbc:trino://trino.example.com:80/system", "false")
);
}
@ParameterizedTest
@MethodSource("provideProtocolAndPortAndSsl")
public void testProtocolAndSsl(String scheme, String port, String expectedJdbcUrl, String expectedSsl) throws java.sql.SQLException {
try(MockedStatic<DriverManager> m = Mockito.mockStatic(java.sql.DriverManager.class)) {
m.when(() -> DriverManager.getConnection(anyString(), any())).thenReturn(connection);
when(connection.prepareStatement(anyString())).thenReturn(preparedStatement);
when(preparedStatement.executeQuery()).thenReturn(resultSet);
ProxyBackendConfiguration proxyBackend = new ProxyBackendConfiguration();
proxyBackend.setProxyTo(String.format("%s://trino.example.com:%s", scheme, port));
proxyBackend.setName("abc");

clusterStatsJdbcMonitor.monitor(proxyBackend);

properties.setProperty("SSL", expectedSsl);

m.verify(() -> DriverManager.getConnection(expectedJdbcUrl, properties));
}
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private void addMockBackends() {
proxyBackend.setProxyTo(backend + ".trino.example.com");
proxyBackend.setExternalUrl("trino.example.com");
backendManager.addBackend(proxyBackend);
//set backend as healthyti start with
//set backend as healthy start with
haRoutingManager.upateBackEndHealth(backend, true);
}

Expand Down
5 changes: 0 additions & 5 deletions proxyserver/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,6 @@
<artifactId>commons-lang3</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing this as this is a dup of line 42

</dependencies>
<build>
<plugins>
Expand Down