SNOW-1898533: Do not set proxy in global request config (#2060)

sfc-gh-dprzybysz · web-flow · commit 873bf872345b · 2025-02-06T10:22:33.000+01:00
diff --git a/src/main/java/net/snowflake/client/core/HttpUtil.java b/src/main/java/net/snowflake/client/core/HttpUtil.java
@@ -129,11 +129,13 @@ public static Duration getSocketTimeout() {
   @SnowflakeJdbcInternalApi
   public static void setConnectionTimeout(int timeout) {
     connectionTimeout = Duration.ofMillis(timeout);
+    initDefaultRequestConfig(connectionTimeout.toMillis(), getSocketTimeout().toMillis());
   }
 
   @SnowflakeJdbcInternalApi
   public static void setSocketTimeout(int timeout) {
     socketTimeout = Duration.ofMillis(timeout);
+    initDefaultRequestConfig(getConnectionTimeout().toMillis(), socketTimeout.toMillis());
   }
 
   public static long getDownloadedConditionTimeoutInSeconds() {
@@ -299,49 +301,11 @@ public static CloseableHttpClient buildHttpClient(
         socketTimeout,
         timeToLive);
 
-    // Set proxy settings for DefaultRequestConfig. If current proxy settings are the same as for
-    // the last request, keep the current DefaultRequestConfig. If not, build a new
-    // DefaultRequestConfig and set the new proxy settings for it
-    HttpHost proxy =
-        (key != null && key.usesProxy())
-            ? new HttpHost(
-                key.getProxyHost(), key.getProxyPort(), key.getProxyHttpProtocol().getScheme())
-            : null;
-    // If defaultrequestconfig is not initialized or its proxy settings do not match current proxy
-    // settings, re-build it (current or old proxy settings could be null, so null check is
-    // included)
-    boolean noDefaultRequestConfig =
-        DefaultRequestConfig == null || DefaultRequestConfig.getProxy() == null;
-    if (noDefaultRequestConfig || !DefaultRequestConfig.getProxy().equals(proxy)) {
-      RequestConfig.Builder builder =
-          RequestConfig.custom()
-              .setConnectTimeout((int) connectTimeout)
-              .setConnectionRequestTimeout((int) connectTimeout)
-              .setSocketTimeout((int) socketTimeout);
-      // only set the proxy settings if they are not null
-      // but no value has been specified for nonProxyHosts
-      // the route planner will determine whether to use a proxy based on nonProxyHosts value.
-      String logMessage =
-          "Rebuilding request config. Connect timeout: "
-              + connectTimeout
-              + " ms, connection request "
-              + "timeout: "
-              + connectTimeout
-              + " ms, socket timeout: "
-              + socketTimeout
-              + " ms";
-      if (proxy != null && Strings.isNullOrEmpty(key.getNonProxyHosts())) {
-        builder.setProxy(proxy);
-        logMessage +=
-            ", host: "
-                + key.getProxyHost()
-                + ", port: "
-                + key.getProxyPort()
-                + ", scheme: "
-                + key.getProxyHttpProtocol().getScheme();
-      }
-      logger.debug(logMessage);
-      DefaultRequestConfig = builder.build();
+    // Create default request config without proxy since different connections could use different
+    // proxies in multi tenant environments
+    // Proxy is set later with route planner
+    if (DefaultRequestConfig == null) {
+      initDefaultRequestConfig(connectTimeout, socketTimeout);
     }
 
     TrustManager[] trustManagers = null;
@@ -411,11 +375,18 @@ public static CloseableHttpClient buildHttpClient(
               .useSystemProperties()
               .setRedirectStrategy(new DefaultRedirectStrategy())
               .setUserAgent(buildUserAgent(userAgentSuffix)) // needed for Okta
-              .disableCookieManagement(); // SNOW-39748
-
+              .disableCookieManagement() // SNOW-39748
+              .setDefaultRequestConfig(DefaultRequestConfig);
       if (key != null && key.usesProxy()) {
+        HttpHost proxy =
+            new HttpHost(
+                key.getProxyHost(), key.getProxyPort(), key.getProxyHttpProtocol().getScheme());
         logger.debug(
-            "Instantiating proxy route planner with non-proxy hosts: {}", key.getNonProxyHosts());
+            "Configuring proxy and route planner - host: {}, port: {}, scheme: {}, nonProxyHosts: {}",
+            key.getProxyHost(),
+            key.getProxyPort(),
+            key.getProxyHttpProtocol().getScheme(),
+            key.getNonProxyHosts());
         // use the custom proxy properties
         SnowflakeMutableProxyRoutePlanner sdkProxyRoutePlanner =
             httpClientRoutePlanner.computeIfAbsent(
@@ -426,7 +397,7 @@ public static CloseableHttpClient buildHttpClient(
                         key.getProxyPort(),
                         key.getProxyHttpProtocol(),
                         key.getNonProxyHosts()));
-        httpClientBuilder = httpClientBuilder.setProxy(proxy).setRoutePlanner(sdkProxyRoutePlanner);
+        httpClientBuilder.setProxy(proxy).setRoutePlanner(sdkProxyRoutePlanner);
         if (!Strings.isNullOrEmpty(key.getProxyUser())
             && !Strings.isNullOrEmpty(key.getProxyPassword())) {
           Credentials credentials =
@@ -440,20 +411,33 @@ public static CloseableHttpClient buildHttpClient(
               key.getProxyHost(),
               key.getProxyPort());
           credentialsProvider.setCredentials(authScope, credentials);
-          httpClientBuilder = httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider);
+          httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider);
         }
       }
-      httpClientBuilder.setDefaultRequestConfig(DefaultRequestConfig);
       if (downloadUnCompressed) {
         logger.debug("Disabling content compression for http client");
-        httpClientBuilder = httpClientBuilder.disableContentCompression();
+        httpClientBuilder.disableContentCompression();
       }
       return httpClientBuilder.build();
     } catch (NoSuchAlgorithmException | KeyManagementException ex) {
       throw new SSLInitializationException(ex.getMessage(), ex);
     }
   }
 
+  private static void initDefaultRequestConfig(long connectTimeout, long socketTimeout) {
+    RequestConfig.Builder builder =
+        RequestConfig.custom()
+            .setConnectTimeout((int) connectTimeout)
+            .setConnectionRequestTimeout((int) connectTimeout)
+            .setSocketTimeout((int) socketTimeout);
+    logger.debug(
+        "Rebuilding request config. Connect timeout: {} ms, connection request timeout: {} ms, socket timeout: {} ms",
+        connectTimeout,
+        connectTimeout,
+        socketTimeout);
+    DefaultRequestConfig = builder.build();
+  }
+
   public static void updateRoutePlanner(HttpClientSettingsKey key) {
     if (httpClientRoutePlanner.containsKey(key)
         && !httpClientRoutePlanner
diff --git a/src/test/java/net/snowflake/client/core/HttpUtilTest.java b/src/test/java/net/snowflake/client/core/HttpUtilTest.java
@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2025 Snowflake Computing Inc. All right reserved.
+ */
+package net.snowflake.client.core;
+
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import java.lang.reflect.Field;
+import java.util.AbstractMap;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.Configurable;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.hamcrest.CoreMatchers;
+import org.hamcrest.Matcher;
+import org.hamcrest.MatcherAssert;
+import org.junit.jupiter.api.Test;
+
+public class HttpUtilTest {
+
+  /**
+   * Test based on <a href="https://github.com/snowflakedb/snowflake-jdbc/issues/2047">reported
+   * issue in SNOW-1898533</a>
+   */
+  @Test
+  public void buildHttpClientRace() throws InterruptedException {
+    HttpUtil.httpClient.clear();
+    // start two threads but only need one to fail
+    CountDownLatch latch = new CountDownLatch(1);
+    final Queue<AbstractMap.SimpleEntry<Thread, Throwable>> failures =
+        new ConcurrentLinkedQueue<>();
+    final HttpClientSettingsKey noProxyKey = new HttpClientSettingsKey(null);
+    final HttpClientSettingsKey proxyKey =
+        new HttpClientSettingsKey(
+            null, "some.proxy.host", 8080, null, null, null, "http", null, false);
+
+    Thread noProxyThread =
+        new Thread(() -> verifyProxyUsage(noProxyKey, failures, latch), "noProxyThread");
+    noProxyThread.start();
+
+    Thread withProxyThread =
+        new Thread(() -> verifyProxyUsage(proxyKey, failures, latch), "withProxyThread");
+    withProxyThread.start();
+
+    // if latch goes to zero, then one of the threads failed
+    // if await times out (returns false), then neither thread has failed (both still running)
+    boolean failed = latch.await(1, TimeUnit.SECONDS);
+    noProxyThread.interrupt();
+    withProxyThread.interrupt();
+    if (failed) {
+      AbstractMap.SimpleEntry<Thread, Throwable> failure = failures.remove();
+      fail(failure.getKey().getName() + " failed", failure.getValue());
+    }
+  }
+
+  private static void verifyProxyUsage(
+      HttpClientSettingsKey key,
+      Queue<AbstractMap.SimpleEntry<Thread, Throwable>> failures,
+      CountDownLatch latch) {
+    while (!Thread.currentThread().isInterrupted()) {
+      try (CloseableHttpClient client = HttpUtil.buildHttpClient(key, null, false)) {
+        assertHttpClientUsesProxy(client, key.usesProxy());
+      } catch (Throwable e) {
+        failures.add(new AbstractMap.SimpleEntry<>(Thread.currentThread(), e));
+        latch.countDown();
+        break;
+      }
+    }
+  }
+
+  private static void assertHttpClientUsesProxy(CloseableHttpClient client, boolean proxyUsed) {
+    assertRequestConfigWithoutProxyConfig(client);
+    assertRoutePlannerOverridden(client, proxyUsed);
+  }
+
+  private static void assertRequestConfigWithoutProxyConfig(CloseableHttpClient client) {
+    MatcherAssert.assertThat(client, CoreMatchers.instanceOf(Configurable.class));
+    Configurable c = (Configurable) client;
+    RequestConfig config = c.getConfig();
+    assertNull(config.getProxy(), "request config has configured proxy");
+  }
+
+  private static void assertRoutePlannerOverridden(CloseableHttpClient client, boolean proxyUsed) {
+    try {
+      // HTTP client does not provide information about proxy settings so to detect that we are
+      // using proxy we have to look inside via reflection and if the route planner is overridden to
+      // our proxy class
+      Field routePlannerField = client.getClass().getDeclaredField("routePlanner");
+      routePlannerField.setAccessible(true);
+      Matcher<Object> snowflakeProxyPlannerClassMatcher =
+          CoreMatchers.instanceOf(SnowflakeMutableProxyRoutePlanner.class);
+      MatcherAssert.assertThat(
+          routePlannerField.get(client),
+          proxyUsed
+              ? snowflakeProxyPlannerClassMatcher
+              : CoreMatchers.not(snowflakeProxyPlannerClassMatcher));
+    } catch (NoSuchFieldException | IllegalAccessException e) {
+      throw new RuntimeException(e);
+    }
+  }
+}
diff --git a/src/test/java/net/snowflake/client/core/SessionUtilTest.java b/src/test/java/net/snowflake/client/core/SessionUtilTest.java
@@ -88,25 +88,30 @@ public void testParameterParsing() {
 
   @Test
   public void testConvertSystemPropertyToIntValue() {
-    // Test that setting real value works
-    System.setProperty("net.snowflake.jdbc.max_connections", "500");
-    assertEquals(
-        500,
-        SystemUtil.convertSystemPropertyToIntValue(
-            HttpUtil.JDBC_MAX_CONNECTIONS_PROPERTY, HttpUtil.DEFAULT_MAX_CONNECTIONS));
-    // Test that entering a non-int sets the value to the default
-    System.setProperty("net.snowflake.jdbc.max_connections", "notAnInteger");
-    assertEquals(
-        HttpUtil.DEFAULT_MAX_CONNECTIONS,
-        SystemUtil.convertSystemPropertyToIntValue(
-            HttpUtil.JDBC_MAX_CONNECTIONS_PROPERTY, HttpUtil.DEFAULT_MAX_CONNECTIONS));
-    // Test another system property
-    System.setProperty("net.snowflake.jdbc.max_connections_per_route", "30");
-    assertEquals(
-        30,
-        SystemUtil.convertSystemPropertyToIntValue(
-            HttpUtil.JDBC_MAX_CONNECTIONS_PER_ROUTE_PROPERTY,
-            HttpUtil.DEFAULT_MAX_CONNECTIONS_PER_ROUTE));
+    try {
+      // Test that setting real value works
+      System.setProperty("net.snowflake.jdbc.max_connections", "500");
+      assertEquals(
+          500,
+          SystemUtil.convertSystemPropertyToIntValue(
+              HttpUtil.JDBC_MAX_CONNECTIONS_PROPERTY, HttpUtil.DEFAULT_MAX_CONNECTIONS));
+      // Test that entering a non-int sets the value to the default
+      System.setProperty("net.snowflake.jdbc.max_connections", "notAnInteger");
+      assertEquals(
+          HttpUtil.DEFAULT_MAX_CONNECTIONS,
+          SystemUtil.convertSystemPropertyToIntValue(
+              HttpUtil.JDBC_MAX_CONNECTIONS_PROPERTY, HttpUtil.DEFAULT_MAX_CONNECTIONS));
+      // Test another system property
+      System.setProperty("net.snowflake.jdbc.max_connections_per_route", "30");
+      assertEquals(
+          30,
+          SystemUtil.convertSystemPropertyToIntValue(
+              HttpUtil.JDBC_MAX_CONNECTIONS_PER_ROUTE_PROPERTY,
+              HttpUtil.DEFAULT_MAX_CONNECTIONS_PER_ROUTE));
+    } finally {
+      System.clearProperty("net.snowflake.jdbc.max_connections");
+      System.clearProperty("net.snowflake.jdbc.max_connections_per_route");
+    }
   }
 
   @Test
