Skip to content

Commit

Permalink
[Feature] Provide SSLSocketFactory in HttpClient (#333)
Browse files Browse the repository at this point in the history
## Changes
<!-- Summary of your changes that are easy to understand -->
We need to provide a way for clients to specify their own SSL socket
factory in case they want to handle TLS connections in a different way

## Tests
<!-- How is this tested? -->
Unit tests and local testing
  • Loading branch information
vikrantpuppala authored Aug 28, 2024
1 parent b634b0d commit bea4755
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ private void initHttp() {
return;
}
// eventually it'll get decoupled from config.
httpClient = new CommonsHttpClient(this);
httpClient = new CommonsHttpClient.Builder().withDatabricksConfig(this).build();
}

public synchronized Map<String, String> authenticate() throws DatabricksException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.http.*;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.*;
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
import org.apache.http.entity.InputStreamEntity;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.client.CloseableHttpClient;
Expand All @@ -36,18 +37,101 @@
import org.slf4j.LoggerFactory;

public class CommonsHttpClient implements HttpClient {
/**
* Builder for CommonsHttpClient. This class is used to construct instances of CommonsHttpClient
* with configurable parameters for the underlying Apache HttpClient.
*/
public static class Builder {
private DatabricksConfig databricksConfig;
private Integer timeoutSeconds;
private ProxyConfig proxyConfig;
private SSLConnectionSocketFactory sslSocketFactory;

/**
* @param databricksConfig The DatabricksConfig to use for the HttpClient. If the
* DatabricksConfig has an httpTimeoutSeconds set, it will be used as the default timeout
* for the HttpClient.
* @return This builder.
*/
public Builder withDatabricksConfig(DatabricksConfig databricksConfig) {
this.databricksConfig = databricksConfig;
return this;
}

/**
* @param timeoutSeconds The timeout in seconds to use for the HttpClient. This will override
* any timeout set in the DatabricksConfig.
* @return This builder.
*/
public Builder withTimeoutSeconds(int timeoutSeconds) {
this.timeoutSeconds = timeoutSeconds;
return this;
}

/**
* @param proxyConfig the proxy configuration to use for the HttpClient.
* @return This builder.
*/
public Builder withProxyConfig(ProxyConfig proxyConfig) {
this.proxyConfig = proxyConfig;
return this;
}

/**
* @param sslSocketFactory the SSLConnectionSocketFactory to use for the HttpClient.
* @return This builder.
*/
public Builder withSslSocketFactory(SSLConnectionSocketFactory sslSocketFactory) {
this.sslSocketFactory = sslSocketFactory;
return this;
}

/** Builds a new instance of CommonsHttpClient with the configured parameters. */
public CommonsHttpClient build() {
return new CommonsHttpClient(this);
}
}

private static final Logger LOG = LoggerFactory.getLogger(CommonsHttpClient.class);
private final PoolingHttpClientConnectionManager connectionManager =
new PoolingHttpClientConnectionManager();
private final CloseableHttpClient hc;
private int timeout;

private CommonsHttpClient(Builder builder) {
int timeoutSeconds = 300;
if (builder.databricksConfig != null
&& builder.databricksConfig.getHttpTimeoutSeconds() != null) {
timeoutSeconds = builder.databricksConfig.getHttpTimeoutSeconds();
}
if (builder.timeoutSeconds != null) {
timeoutSeconds = builder.timeoutSeconds;
}
timeout = timeoutSeconds * 1000;
connectionManager.setMaxTotal(100);
HttpClientBuilder httpClientBuilder =
HttpClientBuilder.create()
.setConnectionManager(connectionManager)
.setDefaultRequestConfig(makeRequestConfig());
if (builder.proxyConfig != null) {
ProxyUtils.setupProxy(builder.proxyConfig, httpClientBuilder);
}
if (builder.sslSocketFactory != null) {
httpClientBuilder.setSSLSocketFactory(builder.sslSocketFactory);
}
hc = httpClientBuilder.build();
}

// These constructors have been deprecate in favour of a builder pattern.
// They will be removed in a future release.
@Deprecated
public CommonsHttpClient(int timeoutSeconds) {
timeout = timeoutSeconds * 1000;
connectionManager.setMaxTotal(100);
hc = makeClosableHttpClient();
}

@Deprecated
public CommonsHttpClient(DatabricksConfig databricksConfig) {
this(
databricksConfig.getHttpTimeoutSeconds() == null
Expand All @@ -56,6 +140,7 @@ public CommonsHttpClient(DatabricksConfig databricksConfig) {
new ProxyConfig(databricksConfig));
}

@Deprecated
public CommonsHttpClient(int timeoutSeconds, ProxyConfig proxyConfig) {
timeout = timeoutSeconds * 1000;
connectionManager.setMaxTotal(100);
Expand All @@ -70,13 +155,15 @@ private RequestConfig makeRequestConfig() {
.build();
}

@Deprecated
private CloseableHttpClient makeClosableHttpClient() {
return HttpClientBuilder.create()
.setConnectionManager(connectionManager)
.setDefaultRequestConfig(makeRequestConfig())
.build();
}

@Deprecated
private CloseableHttpClient makeClosableHttpClient(ProxyConfig proxyConfig) {
HttpClientBuilder builder =
HttpClientBuilder.create()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public static class Builder {
private String clientId;
private String clientSecret;
private String tokenUrl;
private HttpClient hc = new CommonsHttpClient(30);
private HttpClient hc = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
private Map<String, String> endpointParams = Collections.emptyMap();
private List<String> scopes = Collections.emptyList();
private AuthParameterPosition position = AuthParameterPosition.BODY;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public class Consent implements Serializable {
private final String clientSecret;

public static class Builder {
private HttpClient hc = new CommonsHttpClient(30);
private HttpClient hc = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
private String authUrl;
private String verifier;
private String state;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class CommonsHttpClientTest {
@Test
public void itWorks() throws IOException {
try (FixtureServer fixtures = new FixtureServer().with("GET", "/foo?x=y", "bar")) {
HttpClient httpClient = new CommonsHttpClient(30);
HttpClient httpClient = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
Request in = new Request("GET", fixtures.getUrl() + "/foo").withQueryParam("x", "y");
Response out = httpClient.execute(in);
assertEquals("bar", out.getDebugBody().trim());
Expand All @@ -37,7 +37,7 @@ public void testStringBody() throws IOException {
.withResponse("quux")
.build();
try (FixtureServer fixtures = new FixtureServer().with(fixture)) {
HttpClient httpClient = new CommonsHttpClient(30);
HttpClient httpClient = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
Request in = new Request("POST", fixtures.getUrl() + "/foo", "bar");
Response out = httpClient.execute(in);
assertEquals("quux", out.getDebugBody().trim());
Expand All @@ -58,7 +58,7 @@ public void testInputStreamBody() throws IOException {
.withResponse("quux")
.build();
try (FixtureServer fixtures = new FixtureServer().with(fixture)) {
HttpClient httpClient = new CommonsHttpClient(30);
HttpClient httpClient = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
Request in =
new Request("POST", fixtures.getUrl() + "/foo", IOUtils.toInputStream("bar", "UTF-8"));
Response out = httpClient.execute(in);
Expand All @@ -76,7 +76,7 @@ public void testNoRedirection() throws IOException {
.build();

try (FixtureServer fixtures = new FixtureServer().with(fixture)) {
HttpClient httpClient = new CommonsHttpClient(30);
HttpClient httpClient = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
Request in = new Request("GET", fixtures.getUrl() + "/redirect");
in.setRedirectionBehavior(
false); // If we don't set redirection behavior to false, we get 200 as it gets
Expand Down Expand Up @@ -106,7 +106,7 @@ public void testRedirection() throws IOException {
.build());

try (FixtureServer server = new FixtureServer().with(fixtures)) {
HttpClient httpClient = new CommonsHttpClient(30);
HttpClient httpClient = new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
Request in = new Request("GET", server.getUrl() + "/redirect");
Response out = httpClient.execute(in);
assertEquals(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ void clientAndConsentTest() throws IOException {
.setAuthType("external-browser")
.setHost(fixtures.getUrl())
.setClientId("test-client-id")
.setHttpClient(new CommonsHttpClient(30));
.setHttpClient(new CommonsHttpClient.Builder().withTimeoutSeconds(30).build());
config.resolve();

assertEquals("tokenEndPointFromServer", config.getOidcEndpoints().getTokenEndpoint());
Expand Down Expand Up @@ -70,7 +70,7 @@ void clientAndConsentTestWithCustomRedirectUrl() throws IOException {
.setAuthType("external-browser")
.setHost(fixtures.getUrl())
.setClientId("test-client-id")
.setHttpClient(new CommonsHttpClient(30))
.setHttpClient(new CommonsHttpClient.Builder().withTimeoutSeconds(30).build())
.setOAuthRedirectUrl("http://localhost:8010")
.setScopes(Arrays.asList("sql"));
config.resolve();
Expand Down Expand Up @@ -98,7 +98,7 @@ void openIDConnectEndPointsTestAccounts() throws IOException {
new DatabricksConfig()
.setAuthType("external-browser")
.setHost("https://accounts.cloud.databricks.com")
.setHttpClient(new CommonsHttpClient(30))
.setHttpClient(new CommonsHttpClient.Builder().withTimeoutSeconds(30).build())
.setAccountId("testAccountId");
config.resolve();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public static void main(String[] args) {

@Bean
public HttpClient getHttpClient() {
return new CommonsHttpClient(30);
return new CommonsHttpClient.Builder().withTimeoutSeconds(30).build();
}

@Bean
Expand Down

0 comments on commit bea4755

Please sign in to comment.