sts-sponsors team mailing list archive

[Adam Collard <mp+435584@xxxxxxxxxxxxxxxxxx>]

Adam Collard has proposed merging ~adam-collard/maas:vault-proxy into maas:master.

Commit message:
LP:2002111 explicitly set proxies for hvac.Client



Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~adam-collard/maas/+git/maas/+merge/435584
-- 
Your team MAAS Committers is subscribed to branch maas:master.

diff --git a/src/maasserver/pytest_tests/test_vault.py b/src/maasserver/pytest_tests/test_vault.py
index 7277dbb..d0c28e8 100644
--- a/src/maasserver/pytest_tests/test_vault.py
+++ b/src/maasserver/pytest_tests/test_vault.py
@@ -1,10 +1,11 @@
+from contextlib import suppress
 from datetime import datetime, timedelta, timezone
-from os import environ
 from unittest.mock import ANY, MagicMock
 
 from django.core.exceptions import ImproperlyConfigured
 from hvac.exceptions import VaultError as HVACVaultError
 import pytest
+from requests.adapters import HTTPAdapter
 
 from maasserver import vault
 from maasserver.models import Config
@@ -144,24 +145,54 @@ class TestVaultClient:
 
 
 class TestNoProxySettingForHVAC:
-    def test_no_proxy_set(self, monkeypatch):
-        url = "http://url.to.vault:8200";
-        monkeypatch.delenv("no_proxy", raising=False)
-        _create_hvac_client(url)
-        assert environ.get("no_proxy") == url
-
-    def test_no_proxy_appended(self, monkeypatch):
-        url = "http://url.to.vault:8200";
-        monkeypatch.setenv("no_proxy", "localhost,127.0.0.1")
-        _create_hvac_client(url)
-        assert environ.get("no_proxy") == f"localhost,127.0.0.1,{url}"
-
-    def test_idempotency(self, monkeypatch):
-        url = "http://url.to.vault:8200";
-        monkeypatch.delenv("no_proxy", raising=False)
-        _create_hvac_client(url)
-        _create_hvac_client(url)
-        assert environ.get("no_proxy") == url
+    def test_proxy_for_vault_scheme_set_to_None(self):
+        """HVAC client should be configured to not use a proxy."""
+        http_client = _create_hvac_client("http://url.to.vault:8200";)
+        assert http_client.session.proxies == {"http": None}
+        https_client = _create_hvac_client("https://url.to.vault:8200";)
+        assert https_client.session.proxies == {"https": None}
+
+    def test_proxy_for_with_env(self, monkeypatch):
+        """HVAC client should ignore standard proxy environment variables."""
+        monkeypatch.setenv("http_proxy", "http://squid.proxy:3128";)
+        monkeypatch.setenv("https_proxy", "http://squid.proxy:3128";)
+        monkeypatch.setenv("no_proxy", "127.0.0.1.localhost")
+
+        http_client = _create_hvac_client("http://url.to.vault:8200";)
+        assert http_client.session.proxies == {"http": None}
+        https_client = _create_hvac_client("https://url.to.vault:8200";)
+        assert https_client.session.proxies == {"https": None}
+
+    def test_request_honours_proxies(self, monkeypatch):
+        """Verify that the request made by requests follows the rules."""
+        monkeypatch.setenv("http_proxy", "http://squid.proxy:3128";)
+        monkeypatch.setenv("https_proxy", "http://squid.proxy:3128";)
+        monkeypatch.setenv("no_proxy", "127.0.0.1.localhost")
+        http_client = _create_hvac_client("http://url.to.vault:8200";)
+
+        class ProxyRecordingAdapter(HTTPAdapter):
+            """
+            A basic subclass of the HTTPAdapter that records the arguments used to
+            ``send``.
+            """
+
+            def __init__(self2, *args, **kwargs):
+                super().__init__(*args, **kwargs)
+                self2.proxies = []
+
+            def send(self2, request, **kwargs):
+                self2.proxies.append(kwargs.get("proxies"))
+                return
+
+        adapter = ProxyRecordingAdapter()
+        http_client.session.mount("http://";, adapter)
+
+        # Since we return None from ProxyRecordingAdapter.send, it throws
+        # AttributeErrors, we just want to see the request that was
+        # attempted
+        with suppress(AttributeError):
+            http_client.seal_status
+        assert "http" not in adapter.proxies[0]
 
 
 class TestGetRegionVaultClient:
diff --git a/src/maasserver/vault.py b/src/maasserver/vault.py
index 9ff213f..884bee6 100644
--- a/src/maasserver/vault.py
+++ b/src/maasserver/vault.py
@@ -1,8 +1,8 @@
 from datetime import datetime, timedelta, timezone
 from functools import cache, wraps
 import logging
-import os
 from typing import Any, Callable, NamedTuple, Optional
+from urllib.parse import urlparse
 
 from dateutil.parser import isoparse
 from django.core.exceptions import ImproperlyConfigured
@@ -50,11 +50,7 @@ def _create_hvac_client(url: str, **kwargs) -> hvac.Client:
     """Create HVAC client for the given URL, with no proxies set."""
     # This is gross, and unfortunately necessary due to bootsources.get_simplestreams_env
     # and https://github.com/psf/requests/issues/2018
-    if no_proxy := os.environ.get("no_proxy"):
-        if url not in no_proxy.split(","):
-            os.environ["no_proxy"] = f"{no_proxy},{url}"
-    else:
-        os.environ["no_proxy"] = url
+    kwargs["proxies"] = {urlparse(url).scheme: None}
     return hvac.Client(url=url, **kwargs)