Review: Approve nice, +1
a couple minor nits inline Diff comments: > 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 > @@ -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} this test and the one above could use @pytest.mark.parametrize for the url scheme, so they each assert just one thing > + > + 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 > @@ -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} please add an XXX comment here, since we should eventually remove this hack > return hvac.Client(url=url, **kwargs) > > -- https://code.launchpad.net/~adam-collard/maas/+git/maas/+merge/435584 Your team MAAS Committers is subscribed to branch maas:master. -- Mailing list: https://launchpad.net/~sts-sponsors Post to : sts-sponsors@lists.launchpad.net Unsubscribe : https://launchpad.net/~sts-sponsors More help : https://help.launchpad.net/ListHelp
