sts-sponsors team mailing list archive

[Adam Collard <mp+435458@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
LP:2002111 manage no_proxy better in bootsources

Restore environment after we download images

Extend rather than replace no_proxy

Cherry-pick: e9a00ca55d3a1c892c88b880b0c75c465e9c1283


Requested reviews:
  Adam Collard (adam-collard)

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

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_bootresources.py b/src/maasserver/tests/test_bootresources.py
index 52298a7..92ad426 100644
--- a/src/maasserver/tests/test_bootresources.py
+++ b/src/maasserver/tests/test_bootresources.py
@@ -133,7 +133,7 @@ class SimplestreamsEnvFixture(Fixture):
     def setUp(self):
         super().setUp()
         prior_env = {}
-        for key in ["GNUPGHOME", "http_proxy", "https_proxy"]:
+        for key in ["GNUPGHOME", "http_proxy", "https_proxy", "no_proxy"]:
             prior_env[key] = os.environ.get(key, "")
         self.addCleanup(os.environ.update, prior_env)
 
diff --git a/src/maasserver/tests/test_bootsources.py b/src/maasserver/tests/test_bootsources.py
index b37e025..5d94798 100644
--- a/src/maasserver/tests/test_bootsources.py
+++ b/src/maasserver/tests/test_bootsources.py
@@ -364,7 +364,8 @@ class TestPrivateCacheBootSources(MAASTransactionServerTestCase):
             self
         )
         factory.make_BootSource(keyring_data=b"1234")
-        cache_boot_sources()
+        with patch.dict("os.environ", {"no_proxy": ""}):
+            cache_boot_sources()
         self.assertEqual(
             (proxy_address, proxy_address, "127.0.0.1,localhost"),
             (
@@ -384,7 +385,8 @@ class TestPrivateCacheBootSources(MAASTransactionServerTestCase):
         factory.make_BootSource(
             keyring_data=b"1234", url="http://192.168.1.100:8080/ephemeral-v3/";
         )
-        cache_boot_sources()
+        with patch.dict("os.environ", {"no_proxy": ""}):
+            cache_boot_sources()
         no_proxy_hosts = "127.0.0.1,localhost,192.168.1.100"
         self.assertEqual(
             (proxy_address, proxy_address, no_proxy_hosts),
@@ -395,6 +397,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"