sts-sponsors team mailing list archive

[Adam Collard <mp+435596@xxxxxxxxxxxxxxxxxx>]

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

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

Cherry-pick 79bc358e2

Requested reviews:
  Adam Collard (adam-collard)
Related bugs:
  Bug #2002111 in MAAS: "Connection to local Vault fails if proxy is configured"
  https://bugs.launchpad.net/maas/+bug/2002111

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

diff --git a/src/maasserver/bootsources.py b/src/maasserver/bootsources.py
index 58c6cb7..5c2b081 100644
--- a/src/maasserver/bootsources.py
+++ b/src/maasserver/bootsources.py
@@ -378,6 +378,7 @@ def cache_boot_sources():
 
     # FIXME: This modifies the environment of the entire process, which is Not
     # Cool. We should integrate with simplestreams in a more Pythonic manner.
+    # See maasserver.vault._create_hvac_client - LP:2002111
     pristine_env = yield deferToDatabase(set_simplestreams_env)
 
     errors = []
diff --git a/src/maasserver/pytest_tests/test_vault.py b/src/maasserver/pytest_tests/test_vault.py
index 7277dbb..94db5ff 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
@@ -143,25 +144,52 @@ class TestVaultClient:
         ensure_auth.assert_called_once()
 
 
+@pytest.mark.parametrize("scheme", ["http", "https"])
 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, scheme):
+        """HVAC client should be configured to not use a proxy."""
+        hvac_client = _create_hvac_client(f"{scheme}://url.to.vault:8200")
+        assert hvac_client.session.proxies == {scheme: None}
+
+    def test_proxy_for_with_env(self, scheme, 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")
+
+        hvac_client = _create_hvac_client(f"{scheme}://url.to.vault:8200")
+        assert hvac_client.session.proxies == {scheme: None}
+
+    def test_request_honours_proxies(self, scheme, 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")
+        hvac_client = _create_hvac_client(f"{scheme}://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()
+        hvac_client.session.mount(f"{scheme}://", adapter)
+
+        # Since we return None from ProxyRecordingAdapter.send, it throws
+        # AttributeErrors, we just want to see the request that was
+        # attempted
+        with suppress(AttributeError):
+            hvac_client.seal_status
+        assert scheme not in adapter.proxies[0]
 
 
 class TestGetRegionVaultClient:
diff --git a/src/maasserver/vault.py b/src/maasserver/vault.py
index 9ff213f..2b28a3d 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
@@ -48,13 +48,11 @@ def wrap_errors(func: Callable) -> Callable:
 
 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
+    # FIXME: This is gross, and unfortunately necessary due to
+    # bootsources.get_simplestreams_env and
+    # https://github.com/psf/requests/issues/2018
+    # LP:2002528
+    kwargs["proxies"] = {urlparse(url).scheme: None}
     return hvac.Client(url=url, **kwargs)