Merge ~bjornt/maas:bug-1845351-2.4 into maas:2.4

[Björn Tillenius]

LP #1845351: MAAS sometimes get in a state where machine tags aren't updated from their definitions

Don't use a proxy for the rack-region API communication.

We require the rack to have direct access to the region without going
through a proxy.

Using a proxy for the rack-region communication (HTTP API only) has
partly worked, but been broken.

This fixes the test_check_tag_applied_to_all_machines() failure that
fails intermittently in CI.

(cherry picked from commit 4eee730d67082731ed43fe1075a4ce883fcd3823)

[Björn Tillenius (bjornt)]

Self-approve backport.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b bug-1845351-2.4 lp:~bjornt/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6795/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b bug-1845351-2.4 lp:~bjornt/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6800/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug-1845351-2.4 lp:~bjornt/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6804/console
COMMIT: 2058405835f46f2aee1a1f8cb7a0e46d8652c4ce

review: Needs Fixing

[MAAS Lander (maas-lander)]

LANDING
-b bug-1845351-2.4 lp:~bjornt/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6809/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b bug-1845351-2.4 lp:~bjornt/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6816/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b bug-1845351-2.4 lp:~bjornt/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6817/consoleText

diff --git a/src/apiclient/maas_client.py b/src/apiclient/maas_client.py
index a719a29..625b2ad 100644
--- a/src/apiclient/maas_client.py
+++ b/src/apiclient/maas_client.py
@@ -79,8 +79,14 @@ class MAASDispatcher:
     Be careful when changing its API: this class is designed so that it
     can be replaced with a Twisted-enabled alternative.  See the MAAS
     provider in Juju for the code this would require.
+
+    @ivar autodetect_proxies: Extract proxy information from the
+        environment variables (http_proxy, no_proxy). Default True
     """
 
+    def __init__(self, autodetect_proxies=True):
+        self.autodetect_proxies = autodetect_proxies
+
     def dispatch_query(self, request_url, headers, method="GET", data=None):
         """Synchronously dispatch an OAuth-signed request to L{request_url}.
 
@@ -109,7 +115,11 @@ class MAASDispatcher:
         if data is not None and not isinstance(data, bytes):
             data = bytes(data, 'utf-8')
         req = RequestWithMethod(request_url, data, headers, method=method)
-        res = urllib.request.urlopen(req)
+        handlers = []
+        if not self.autodetect_proxies:
+            handlers.append(urllib.request.ProxyHandler({}))
+        opener = urllib.request.build_opener(*handlers)
+        res = opener.open(req)
         # If we set the Accept-encoding header, then we decode the header for
         # the caller.
         is_gzip = (
diff --git a/src/apiclient/tests/test_maas_client.py b/src/apiclient/tests/test_maas_client.py
index 5b126b3..da4b654 100644
--- a/src/apiclient/tests/test_maas_client.py
+++ b/src/apiclient/tests/test_maas_client.py
@@ -8,8 +8,13 @@ __all__ = []
 import gzip
 from io import BytesIO
 import json
+import os
 from random import randint
-from unittest.mock import ANY
+from unittest.mock import (
+    ANY,
+    MagicMock,
+    patch,
+)
 import urllib.error
 import urllib.parse
 from urllib.parse import (
@@ -46,6 +51,22 @@ class TestMAASOAuth(MAASTestCase):
 
 
 class TestMAASDispatcher(MAASTestCase):
+    def setUp(self):
+        super().setUp()
+        self.patch_urllib()
+
+    def patch_urllib(self):
+        def patch_build_opener(*args, **kwargs):
+            self.opener = real_build_opener(*args, **kwargs)
+            self.orig_open_func = self.opener.open
+            if self.open_func is not None:
+                self.opener.open = self.open_func
+            return self.opener
+
+        real_build_opener = urllib.request.build_opener
+        build_opener_mock = self.patch(urllib.request, "build_opener")
+        build_opener_mock.side_effect = patch_build_opener
+        self.open_func = None
 
     def test_dispatch_query_makes_direct_call(self):
         contents = factory.make_string().encode("ascii")
@@ -56,13 +77,13 @@ class TestMAASDispatcher(MAASTestCase):
     def test_dispatch_query_encodes_string_data(self):
         # urllib, used by MAASDispatcher, requires data encoded into bytes. We
         # encode into utf-8 in dispatch_query if necessary.
-        request = self.patch(urllib.request.Request, '__init__')
-        urlopen = self.patch(urllib.request, 'urlopen')
+        request = self.patch(urllib.request.Request, "__init__")
+        self.patch_urllib()
+        self.open_func = lambda *args: MagicMock()
         url = factory.make_url()
         data = factory.make_string(300, spaces=True)
         MAASDispatcher().dispatch_query(url, {}, method="POST", data=data)
         request.assert_called_once_with(ANY, url, bytes(data, "utf-8"), ANY)
-        urlopen.assert_called_once_with(ANY)
 
     def test_request_from_http(self):
         # We can't just call self.make_file because HTTPServerFixture will only
@@ -106,12 +127,12 @@ class TestMAASDispatcher(MAASTestCase):
         content = factory.make_string(300).encode('ascii')
         factory.make_file(location='.', name=name, contents=content)
         called = []
-        orig_urllib = urllib.request.urlopen
 
-        def logging_urlopen(*args, **kwargs):
+        def logging_open(*args, **kwargs):
             called.append((args, kwargs))
-            return orig_urllib(*args, **kwargs)
-        self.patch(urllib.request, 'urlopen', logging_urlopen)
+            return self.orig_open_func(*args, **kwargs)
+
+        self.open_func = logging_open
         with HTTPServerFixture() as httpd:
             url = urljoin(httpd.url, name)
             res = MAASDispatcher().dispatch_query(url, {})
@@ -139,6 +160,44 @@ class TestMAASDispatcher(MAASTestCase):
             mode='rb', fileobj=BytesIO(raw_content)).read()
         self.assertEqual(content, read_content)
 
+    def test_autodetects_proxies(self):
+        self.open_func = lambda *args: MagicMock()
+        url = factory.make_url()
+        proxy_variables = {
+            "http_proxy": "http://proxy.example.com",
+            "https_proxy": "https://proxy.example.com",
+            "no_proxy": "noproxy.example.com",
+        }
+        with patch.dict(os.environ, proxy_variables):
+            MAASDispatcher().dispatch_query(url, {}, method="GET")
+        for handler in self.opener.handle_open["http"]:
+            if isinstance(handler, urllib.request.ProxyHandler):
+                break
+        else:
+            raise AssertionError("No ProxyHandler installed")
+        expected = {
+            "http": proxy_variables["http_proxy"],
+            "https": proxy_variables["https_proxy"],
+            "no": proxy_variables["no_proxy"],
+        }
+        for key, value in expected.items():
+            self.assertEqual(value, handler.proxies[key])
+
+    def test_no_autodetects_proxies(self):
+        self.open_func = lambda *args: MagicMock()
+        url = factory.make_url()
+        proxy_variables = {
+            "http_proxy": "http://proxy.example.com",
+            "https_proxy": "https://proxy.example.com",
+            "no_proxy": "noproxy.example.com",
+        }
+        with patch.dict(os.environ, proxy_variables):
+            dispatcher = MAASDispatcher(autodetect_proxies=False)
+            dispatcher.dispatch_query(url, {}, method="GET")
+        for handler in self.opener.handle_open["http"]:
+            if isinstance(handler, urllib.request.ProxyHandler):
+                raise AssertionError("ProxyHandler shouldn't be there")
+
 
 def make_path():
     """Create an arbitrary resource path."""
diff --git a/src/provisioningserver/rpc/tags.py b/src/provisioningserver/rpc/tags.py
index 12a4c53..c72dd3e 100644
--- a/src/provisioningserver/rpc/tags.py
+++ b/src/provisioningserver/rpc/tags.py
@@ -31,9 +31,14 @@ def evaluate_tag(
     """
     with ClusterConfiguration.open() as config:
         maas_url = config.maas_url
+
+    # Turn off proxy detection, since the rack should talk directly to
+    # the region, even if a system-wide proxy is configured.
     client = MAASClient(
-        auth=MAASOAuth(*credentials), dispatcher=MAASDispatcher(),
-        base_url=maas_url)
+        auth=MAASOAuth(*credentials),
+        dispatcher=MAASDispatcher(autodetect_proxies=False),
+        base_url=maas_url,
+    )
     process_node_tags(
         rack_id=system_id, nodes=nodes,
         tag_name=tag_name, tag_definition=tag_definition,
diff --git a/src/provisioningserver/rpc/tests/test_tags.py b/src/provisioningserver/rpc/tests/test_tags.py
index 4e9558b..cd40376 100644
--- a/src/provisioningserver/rpc/tests/test_tags.py
+++ b/src/provisioningserver/rpc/tests/test_tags.py
@@ -64,3 +64,14 @@ class TestEvaluateTag(MAASTestCase):
         self.assertIsInstance(client.auth, MAASOAuth)
         self.assertThat(tags.MAASOAuth, MockCalledOnceWith(
             consumer_key, resource_token, resource_secret))
+
+    def test__constructs_client_with_no_proxies(self):
+        credentials = "aaa", "bbb", "ccc"
+        rack_id = factory.make_name("rack")
+        process_node_tags = self.patch_autospec(tags, "process_node_tags")
+        tags.evaluate_tag(
+            rack_id, [], sentinel.tag_name, sentinel.tag_definition,
+            sentinel.tag_nsmap, credentials)
+
+        client = process_node_tags.call_args[1]["client"]
+        self.assertFalse(client.dispatcher.autodetect_proxies)