Merge ~bjornt/maas:bug-1845351-update-tags-no-proxy into maas:master

[Björn Tillenius]

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.

[Alberto Donato (ack)]

LGTM, +1

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug-1845351-update-tags-no-proxy lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6771/console
COMMIT: 1523767a1851eca9202edbea70d553679a429d51

review: Needs Fixing

e9296dc... by Björn Tillenius on 2019-10-16

Fix test failure.

226898c... by Björn Tillenius on 2019-10-16

Typo.

diff --git a/src/apiclient/maas_client.py b/src/apiclient/maas_client.py
index d0607e7..95d98e1 100644
--- a/src/apiclient/maas_client.py
+++ b/src/apiclient/maas_client.py
@@ -87,8 +87,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}.
 
@@ -118,9 +124,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 36701d1..6e06581 100644
--- a/src/apiclient/tests/test_maas_client.py
+++ b/src/apiclient/tests/test_maas_client.py
@@ -8,8 +8,9 @@ __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 parse_qs, urljoin, urlparse
@@ -33,6 +34,23 @@ 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")
         url = "file://%s" % self.make_file(contents=contents)
@@ -44,12 +62,12 @@ class TestMAASDispatcher(MAASTestCase):
         # 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")
+        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
@@ -97,13 +115,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)
+            return self.orig_open_func(*args, **kwargs)
 
-        self.patch(urllib.request, "urlopen", logging_urlopen)
+        self.open_func = logging_open
         with HTTPServerFixture() as httpd:
             url = urljoin(httpd.url, name)
             res = MAASDispatcher().dispatch_query(url, {})
@@ -139,20 +156,19 @@ 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):
+            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())
@@ -165,12 +181,12 @@ 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,
@@ -179,6 +195,44 @@ class TestMAASDispatcher(MAASTestCase):
             )
             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 3b436f6..653e777 100644
--- a/src/provisioningserver/rpc/tags.py
+++ b/src/provisioningserver/rpc/tags.py
@@ -30,9 +30,11 @@ 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(),
+        dispatcher=MAASDispatcher(autodetect_proxies=False),
         base_url=maas_url,
     )
     process_node_tags(
diff --git a/src/provisioningserver/rpc/tests/test_tags.py b/src/provisioningserver/rpc/tests/test_tags.py
index 7a7d0b3..a3493e5 100644
--- a/src/provisioningserver/rpc/tests/test_tags.py
+++ b/src/provisioningserver/rpc/tests/test_tags.py
@@ -73,3 +73,20 @@ class TestEvaluateTag(MAASTestCase):
             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)