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

[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.6 lp:~bjornt/maas/+git/maas into -b 2.6 lp:~maas-committers/maas

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

diff --git a/src/apiclient/maas_client.py b/src/apiclient/maas_client.py
index 6bd7f50..7f76d2e 100644
--- a/src/apiclient/maas_client.py
+++ b/src/apiclient/maas_client.py
@@ -81,8 +81,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}.
 
@@ -112,9 +118,13 @@ class MAASDispatcher:
             data = bytes(data, 'utf-8')
         req = RequestWithMethod(request_url, data, headers, method=method)
         # Retry the request maximum of 3 times.
+        handlers = []
+        if not self.autodetect_proxies:
+            handlers.append(urllib.request.ProxyHandler({}))
         for try_count in range(3):
+            opener = urllib.request.build_opener(*handlers)
             try:
-                res = urllib.request.urlopen(req)
+                res = opener.open(req)
             except urllib.error.HTTPError as exc:
                 if exc.code == 503:
                     # HTTP 503 Service Unavailable - MAAS might still be
diff --git a/src/apiclient/tests/test_maas_client.py b/src/apiclient/tests/test_maas_client.py
index 5aade88..caa1f81 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, {})
@@ -146,19 +167,18 @@ class TestMAASDispatcher(MAASTestCase):
         factory.make_file(location='.', name=name, contents=content)
         with HTTPServerFixture() as httpd:
             url = urljoin(httpd.url, name)
-            original_urlopen = urllib.request.urlopen
 
             counter = {'count': 0}
 
-            def _wrap_urlopen(*args, **kwargs):
-                if counter['count'] < 2:
-                    counter['count'] += 1
+            def _wrap_open(*args, **kwargs):
+                if counter["count"] < 2:
+                    counter["count"] += 1
                     raise urllib.error.HTTPError(
                         url, 503, "service unavailable", {}, None)
                 else:
-                    return original_urlopen(*args, **kwargs)
+                    return self.orig_open_func(*args, **kwargs)
 
-            self.patch(urllib.request, "urlopen").side_effect = _wrap_urlopen
+            self.open_func = _wrap_open
             response = MAASDispatcher().dispatch_query(url, {})
             self.assertEqual(200, response.code)
             self.assertEqual(content, response.read())
@@ -171,16 +191,54 @@ class TestMAASDispatcher(MAASTestCase):
         with HTTPServerFixture() as httpd:
             url = urljoin(httpd.url, name)
 
-            def _wrap_urlopen(*args, **kwargs):
+            def _wrap_open(*args, **kwargs):
                 raise urllib.error.HTTPError(
                     url, 503, "service unavailable", {}, None)
 
-            self.patch(urllib.request, "urlopen").side_effect = _wrap_urlopen
+            self.open_func = _wrap_open
             err = self.assertRaises(
                 urllib.error.HTTPError,
                 MAASDispatcher().dispatch_query, url, {})
             self.assertEqual(503, err.code)
 
+    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 f8bc2aa..262d837 100644
--- a/src/provisioningserver/rpc/tags.py
+++ b/src/provisioningserver/rpc/tags.py
@@ -30,9 +30,13 @@ def evaluate_tag(
     :param credentials: A 3-tuple of OAuth credentials.
     :param maas_url: URL of the MAAS API.
     """
+    # 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 9576003..40208c9 100644
--- a/src/provisioningserver/rpc/tests/test_tags.py
+++ b/src/provisioningserver/rpc/tests/test_tags.py
@@ -60,5 +60,24 @@ class TestEvaluateTag(MAASTestCase):
         self.assertEqual(self.mock_url, client.url)
         self.assertIsInstance(client.dispatcher, MAASDispatcher)
         self.assertIsInstance(client.auth, MAASOAuth)
-        self.assertThat(tags.MAASOAuth, MockCalledOnceWith(
-            consumer_key, resource_token, resource_secret))
+        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,
+            self.mock_url,
+        )
+
+        client = process_node_tags.call_args[1]["client"]
+        self.assertFalse(client.dispatcher.autodetect_proxies)