Merge ~cgrabowski/maas:vault-proxy into maas:master

[Christian Grabowski]

account for no_proxy set on test host

LP:2002111 manage no_proxy better in bootsources
Signed-off-by: Adam Collard <email address hidden>

[MAAS Lander (maas-lander)]

UNIT TESTS
-b vault-proxy lp:~cgrabowski/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/1725/consoleText
COMMIT: b1c3a662782de5fd08cf1cb326723943838a9c9f

review: Needs Fixing

[Adam Collard (adam-collard)]

Thanks for trying to get this over the line - i hadn't seen this branch when i pushed a revised commit on my branch - https://code.launchpad.net/~adam-collard/maas/+git/maas/+merge/435387

I took the approach of patching out no_proxy in the environ before running the tests.

review: Disapprove

Unmerged commits

b1c3a66... by Christian Grabowski on 2023-01-09

account for no_proxy set on test host

0a04d97... by Adam Collard on 2023-01-09

LP:2002111 manage no_proxy better in bootsources

Signed-off-by: Adam Collard <email address hidden>

diff --git a/src/maasserver/bootsources.py b/src/maasserver/bootsources.py
index c58b19a..58c6cb7 100644
--- a/src/maasserver/bootsources.py
+++ b/src/maasserver/bootsources.py
@@ -97,6 +97,15 @@ def get_boot_sources():
     return [source.to_dict() for source in BootSource.objects.all()]
 
 
+def _upsert_no_proxy_env(env, entry):
+    """Updates $no_proxy appropriately."""
+    if no_proxy := env.get("no_proxy"):
+        if entry not in no_proxy.split(","):
+            env["no_proxy"] = f"{no_proxy},{entry}"
+    else:
+        env["no_proxy"] = entry
+
+
 @transactional
 def get_simplestreams_env():
     """Get environment that should be used with simplestreams."""
@@ -106,24 +115,28 @@ def get_simplestreams_env():
         if http_proxy is not None:
             env["http_proxy"] = http_proxy
             env["https_proxy"] = http_proxy
+            if no_proxy := os.environ.get("no_proxy"):
+                env["no_proxy"] = no_proxy
             # When the proxy environment variables are set they effect the
             # entire process, including controller refresh. When the region
             # needs to refresh itself it sends itself results over HTTP to
             # 127.0.0.1.
-            no_proxy_hosts = "127.0.0.1,localhost"
+            no_proxy_hosts = ["127.0.0.1", "localhost"]
             # When using a proxy and using an image mirror, we may not want
             # to use the proxy to download the images, as they could be
             # located in the local network, hence it makes no sense to use
             # it. With this, we add the image mirror location(s) to the
             # no proxy variable, which ensures MAAS contacts the mirror
             # directly instead of through the proxy.
-            no_proxy = Config.objects.get_config("boot_images_no_proxy")
-            if no_proxy:
-                sources = get_boot_sources()
-                for source in sources:
-                    host = urlparse(source["url"]).netloc.split(":")[0]
-                    no_proxy_hosts = ",".join((no_proxy_hosts, host))
-            env["no_proxy"] = no_proxy_hosts
+            if Config.objects.get_config("boot_images_no_proxy"):
+                no_proxy_hosts.extend(
+                    [
+                        urlparse(source["url"]).hostname
+                        for source in get_boot_sources()
+                    ]
+                )
+            for host in no_proxy_hosts:
+                _upsert_no_proxy_env(env, host)
     else:
         # The proxy is disabled, let's not accidentally use proxy from
         # encompassing environment.
@@ -134,12 +147,24 @@ def get_simplestreams_env():
 
 def set_simplestreams_env():
     """Set the environment that simplestreams needs."""
+    bodged_env = get_simplestreams_env()
+    pristine_env = {k: os.environ.get(k) for k in bodged_env}
     # We simply set the env variables here as another simplestreams-based
     # import might be running concurrently (bootresources._import_resources)
     # and we don't want to use the environment_variables context manager that
     # would reset the environment variables (they are global to the entire
     # process) while the other import is still running.
-    os.environ.update(get_simplestreams_env())
+    os.environ.update(bodged_env)
+    return pristine_env
+
+
+def restore_pristine_env(pristine_env):
+    """Restored the environment that simplestreams needs' bodged."""
+    for key, value in pristine_env.items():
+        if value is None and key in os.environ:
+            del os.environ[key]
+        else:
+            os.environ[key] = value
 
 
 def get_os_info_from_boot_sources(os):
@@ -353,7 +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.
-    yield deferToDatabase(set_simplestreams_env)
+    pristine_env = yield deferToDatabase(set_simplestreams_env)
 
     errors = []
     sources = yield deferToDatabase(get_sources)
@@ -392,6 +417,7 @@ def cache_boot_sources():
             else:
                 yield deferToDatabase(_update_cache, source, descriptions)
 
+    yield deferToDatabase(restore_pristine_env, pristine_env)
     yield deferToDatabase(check_commissioning_series_selected)
 
     component = COMPONENT.REGION_IMAGE_IMPORT
diff --git a/src/maasserver/tests/test_bootsources.py b/src/maasserver/tests/test_bootsources.py
index b37e025..bc2534d 100644
--- a/src/maasserver/tests/test_bootsources.py
+++ b/src/maasserver/tests/test_bootsources.py
@@ -365,8 +365,11 @@ class TestPrivateCacheBootSources(MAASTransactionServerTestCase):
         )
         factory.make_BootSource(keyring_data=b"1234")
         cache_boot_sources()
+        no_proxy_hosts = "127.0.0.1,localhost"
+        if environ.get("no_proxy"):
+            no_proxy_hosts = f"{environ['no_proxy']},{no_proxy_hosts}"
         self.assertEqual(
-            (proxy_address, proxy_address, "127.0.0.1,localhost"),
+            (proxy_address, proxy_address, no_proxy_hosts),
             (
                 capture.env["http_proxy"],
                 capture.env["https_proxy"],
@@ -386,6 +389,8 @@ class TestPrivateCacheBootSources(MAASTransactionServerTestCase):
         )
         cache_boot_sources()
         no_proxy_hosts = "127.0.0.1,localhost,192.168.1.100"
+        if environ.get("no_proxy"):
+            no_proxy_hosts = f"{environ['no_proxy']},{no_proxy_hosts}"
         self.assertEqual(
             (proxy_address, proxy_address, no_proxy_hosts),
             (
@@ -395,6 +400,45 @@ class TestPrivateCacheBootSources(MAASTransactionServerTestCase):
             ),
         )
 
+    def test_retains_existing_no_proxy(self):
+        proxy_address = factory.make_name("proxy")
+        Config.objects.set_config("http_proxy", proxy_address)
+        Config.objects.set_config("boot_images_no_proxy", True)
+        capture = patch_and_capture_env_for_download_all_image_descriptions(
+            self
+        )
+        factory.make_BootSource(
+            keyring_data=b"1234", url="http://192.168.1.100:8080/ephemeral-v3/"
+        )
+        with patch.dict("os.environ", {"no_proxy": "http://my.direct.host"}):
+            cache_boot_sources()
+        no_proxy_hosts = (
+            "http://my.direct.host,127.0.0.1,localhost,192.168.1.100"
+        )
+        self.assertEqual(no_proxy_hosts, capture.env["no_proxy"])
+
+    def test_restores_proxy_settings_post_call(self):
+        proxy_address = factory.make_name("proxy")
+        Config.objects.set_config("http_proxy", proxy_address)
+        Config.objects.set_config("boot_images_no_proxy", True)
+        mock_download = self.patch(
+            bootsources, "download_all_image_descriptions"
+        )
+        mock_download.return_value = make_boot_image_mapping()
+
+        with patch.dict(
+            "os.environ",
+            {
+                "no_proxy": "my.no_proxy",
+                "http_proxy": "my.http_proxy",
+                "https_proxy": "my.https_proxy",
+            },
+        ):
+            cache_boot_sources()
+            self.assertEqual(environ.get("no_proxy"), "my.no_proxy")
+            self.assertEqual(environ.get("http_proxy"), "my.http_proxy")
+            self.assertEqual(environ.get("https_proxy"), "my.https_proxy")
+
     def test_passes_user_agent_with_maas_version(self):
         mock_download = self.patch(
             bootsources, "download_all_image_descriptions"