fix: lazily resolve proxy environment variables (#1066)

smithy-lang · Apr 15, 2024 · b211771 · b211771
1 parent 5dddc09
commit b211771
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 6 deletions.
diff --git a/.changes/6ffd0a2b-9801-4814-a6d1-9d84196ff130.json b/.changes/6ffd0a2b-9801-4814-a6d1-9d84196ff130.json
@@ -0,0 +1,8 @@
+{
+    "id": "6ffd0a2b-9801-4814-a6d1-9d84196ff130",
+    "type": "bugfix",
+    "description": "Lazily resolve proxy environment variables",
+    "issues": [
+        "https://github.com/awslabs/aws-sdk-kotlin/issues/1281"
+    ]
+}
diff --git a/...engine-default/jvm/test/aws/smithy/kotlin/runtime/http/engine/HttpEngineConfigImplTest.kt b/...engine-default/jvm/test/aws/smithy/kotlin/runtime/http/engine/HttpEngineConfigImplTest.kt
@@ -130,6 +130,26 @@ class HttpEngineConfigImplTest {
             }
         }
     }
+
+    // reproduces https://github.com/awslabs/aws-sdk-kotlin/issues/1281
+    @Test
+    fun testCanConfigureProxySelectorWithInvalidEnvVarsPresent() {
+        try {
+            System.setProperty("http.proxyHost", "invalid!")
+            System.setProperty("https.proxyHost", "invalid!")
+
+            val builder = HttpEngineConfigImpl.BuilderImpl()
+            builder.buildHttpEngineConfig()
+
+            builder.httpClient {
+                proxySelector = ProxySelector.NoProxy
+            }
+            builder.buildHttpEngineConfig()
+        } finally {
+            System.clearProperty("http.proxyHost")
+            System.clearProperty("https.proxyHost")
+        }
+    }
 }
 
 private fun config(block: HttpEngineConfig.Builder.() -> Unit): HttpEngineConfig =

diff --git a/.../http-client/common/src/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelector.kt b/.../http-client/common/src/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelector.kt
@@ -30,11 +30,9 @@ import aws.smithy.kotlin.runtime.util.PropertyProvider
  * - `no_proxy`, `NO_PROXY`
  */
 internal class EnvironmentProxySelector(provider: PlatformEnvironProvider = PlatformProvider.System) : ProxySelector {
-    private val httpProxy =
-        resolveProxyByProperty(provider, Scheme.HTTP) ?: resolveProxyByEnvironment(provider, Scheme.HTTP)
-    private val httpsProxy =
-        resolveProxyByProperty(provider, Scheme.HTTPS) ?: resolveProxyByEnvironment(provider, Scheme.HTTPS)
-    private val noProxyHosts = resolveNoProxyHosts(provider)
+    private val httpProxy by lazy { resolveProxyByProperty(provider, Scheme.HTTP) ?: resolveProxyByEnvironment(provider, Scheme.HTTP) }
+    private val httpsProxy by lazy { resolveProxyByProperty(provider, Scheme.HTTPS) ?: resolveProxyByEnvironment(provider, Scheme.HTTPS) }
+    private val noProxyHosts by lazy { resolveNoProxyHosts(provider) }
 
     override fun select(url: Url): ProxyConfig {
         if (httpProxy == null && httpsProxy == null || noProxy(url)) return ProxyConfig.Direct

diff --git a/...-client/common/test/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelectorTest.kt b/...-client/common/test/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelectorTest.kt
@@ -102,8 +102,9 @@ class EnvironmentProxySelectorTest {
     fun testSelectFailures() {
         failCases.forEachIndexed { idx, failCase ->
             val testProvider = TestPlatformEnvironmentProvider(failCase.env, failCase.props)
+            val selector = EnvironmentProxySelector(testProvider)
             val exception = assertFailsWith<ClientException>("[idx=$idx] expected ClientException") {
-                EnvironmentProxySelector(testProvider)
+                selector.select(Url.parse("http://localhost:8000")) // call `select` because proxy selector resolves env vars lazily
             }
 
             val expectedError = (failCase.env + failCase.props).map { (k, v) -> """$k="$v"""" }.joinToString(", ")