Consistent handling of HTTP/S proxy environment variables (#2079)

* Remove explicit handling of proxy env variables in favor of downstream handling through Requests library

* Add unit test to verify underlying library respects proxy env vars

* fix formatting errors

Author: davidwegs

libcloud/http.py

@@ -18,7 +18,6 @@
 verification, depending on libcloud.security settings.
 """
 
-import os
 import warnings
 
 import requests
@@ -42,9 +41,6 @@
 # Default timeout for HTTP requests in seconds
 DEFAULT_REQUEST_TIMEOUT = 60
 
-HTTP_PROXY_ENV_VARIABLE_NAME = "http_proxy"
-HTTPS_PROXY_ENV_VARIABLE_NAME = "https_proxy"
-
 
 class SignedHTTPSAdapter(HTTPAdapter):
     def __init__(self, cert_file, key_file):
@@ -88,6 +84,7 @@ def __init__(self):
     def set_http_proxy(self, proxy_url):
         """
         Set a HTTP proxy which will be used with this connection.
+        This will override any proxy environment variables.
 
         :param proxy_url: Proxy URL (e.g. http://<hostname>:<port> without
                           authentication and
@@ -197,13 +194,8 @@ def __init__(self, host, port, secure=None, **kwargs):
             ":{}".format(port) if port not in (80, 443) else "",
         )
 
-        # Support for HTTP(s) proxy
-        # NOTE: We always only use a single proxy (either HTTP or HTTPS)
-        https_proxy_url_env = os.environ.get(HTTPS_PROXY_ENV_VARIABLE_NAME, None)
-        http_proxy_url_env = os.environ.get(HTTP_PROXY_ENV_VARIABLE_NAME, https_proxy_url_env)
-
-        # Connection argument has precedence over environment variables
-        proxy_url = kwargs.pop("proxy_url", http_proxy_url_env)
+        # Connection argument has precedence over environment variables, which are handled downstream
+        proxy_url = kwargs.pop("proxy_url", None)
 
         self._setup_verify()
         self._setup_ca_cert()

libcloud/test/test_connection.py

@@ -21,6 +21,7 @@
 from unittest.mock import Mock, patch
 
 import requests_mock
+from requests.adapters import HTTPAdapter
 from requests.exceptions import ConnectTimeout
 
 import libcloud.common.base
@@ -36,6 +37,7 @@ class BaseConnectionClassTestCase(unittest.TestCase):
     def setUp(self):
         self.orig_http_proxy = os.environ.pop("http_proxy", None)
         self.orig_https_proxy = os.environ.pop("https_proxy", None)
+        self.orig_no_proxy = os.environ.pop("no_proxy", None)
 
     def tearDown(self):
         if self.orig_http_proxy:
@@ -48,6 +50,11 @@ def tearDown(self):
         elif "https_proxy" in os.environ:
             del os.environ["https_proxy"]
 
+        if self.orig_no_proxy:
+            os.environ["no_proxy"] = self.orig_no_proxy
+        elif "no_proxy" in os.environ:
+            del os.environ["no_proxy"]
+
         libcloud.common.base.ALLOW_PATH_DOUBLE_SLASHES = False
 
     def test_parse_proxy_url(self):
@@ -104,22 +111,12 @@ def test_parse_proxy_url(self):
         )
 
     def test_constructor(self):
-        proxy_url = "http://127.0.0.2:3128"
-        os.environ["http_proxy"] = proxy_url
-        conn = LibcloudConnection(host="localhost", port=80)
-        self.assertEqual(conn.proxy_scheme, "http")
-        self.assertEqual(conn.proxy_host, "127.0.0.2")
-        self.assertEqual(conn.proxy_port, 3128)
-        self.assertEqual(
-            conn.session.proxies,
-            {"http": "http://127.0.0.2:3128", "https": "http://127.0.0.2:3128"},
-        )
-
         _ = os.environ.pop("http_proxy", None)
         conn = LibcloudConnection(host="localhost", port=80)
         self.assertIsNone(conn.proxy_scheme)
         self.assertIsNone(conn.proxy_host)
         self.assertIsNone(conn.proxy_port)
+        self.assertTrue(conn.session.proxies is None or not conn.session.proxies)
 
         proxy_url = "http://127.0.0.3:3128"
         conn.set_http_proxy(proxy_url=proxy_url)
@@ -143,7 +140,8 @@ def test_constructor(self):
 
         os.environ["http_proxy"] = proxy_url
         proxy_url = "http://127.0.0.5:3128"
-        conn = LibcloudConnection(host="localhost", port=80, proxy_url=proxy_url)
+        conn = LibcloudConnection(host="localhost", port=80)
+        conn.set_http_proxy(proxy_url=proxy_url)
         self.assertEqual(conn.proxy_scheme, "http")
         self.assertEqual(conn.proxy_host, "127.0.0.5")
         self.assertEqual(conn.proxy_port, 3128)
@@ -163,6 +161,43 @@ def test_constructor(self):
             {"http": "https://127.0.0.6:3129", "https": "https://127.0.0.6:3129"},
         )
 
+    def test_proxy_environment_variables_respected(self):
+        """
+        Test that proxy environment variables are respected by the underlying Requests library
+        """
+
+        def mock_send(self, request, **kwargs):
+            captured_proxies.update(kwargs.get("proxies", {}))
+            nonlocal captured_url
+            captured_url = request.url
+            mock_response = Mock()
+            mock_response.status_code = 200
+            mock_response.headers = {"content-type": "application/json", "location": ""}
+            mock_response.text = "OK"
+            mock_response.history = []  # No redirects
+            return mock_response
+
+        with patch.object(HTTPAdapter, "send", mock_send):
+            os.environ["http_proxy"] = "http://proxy.example.com:8080"
+            os.environ["https_proxy"] = "https://secure-proxy.example.com:8443"
+            os.environ["no_proxy"] = "localhost,127.0.0.1"
+            captured_proxies = {}
+            captured_url = None
+
+            conn = LibcloudConnection(host="localhost", port=80)
+            conn.request("GET", "/get")
+
+            self.assertEqual(captured_proxies, {})
+            self.assertIn("localhost", captured_url)
+
+            conn = LibcloudConnection(host="test.com", port=80)
+            conn.request("GET", "/get")
+
+            self.assertEqual(captured_proxies.get("http", None), "http://proxy.example.com:8080")
+            self.assertEqual(
+                captured_proxies.get("https", None), "https://secure-proxy.example.com:8443"
+            )
+
     def test_connection_to_unusual_port(self):
         conn = LibcloudConnection(host="localhost", port=8080)
         self.assertIsNone(conn.proxy_scheme)