Merge lp:~jtv/maas/bug-1372767 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Kludge around Twisted bug (our bug 1372767): its web client breaks when the cluster controller tries to access the region controller API on a URL based on an IPv6 address.

This monkey-patches into place a derived-and-improved version of class twisted.web.client._URI. The replacement can deal with IPv6 addresses in URLs. It could probably be smarter but given that I'm already resorting to a fairly ugly-looking regex, I figured I'd try to keep it dumb so bugs won't come down to something as hard to notice as a parenthesis being in the wrong place.

Jeroen

[Gavin Panella (allenap)]

Tip top.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks for the review. I was sort of expecting to be chastised for the kludge.

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2014-09-23 09:24:19 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2014-09-23 11:50:34 +0000
@@ -19,6 +19,7 @@
 import json
 import logging
 import random
+import re
 from urlparse import urlparse
 
 from apiclient.creds import convert_string_to_tuple
@@ -81,6 +82,8 @@
 from twisted.internet.threads import deferToThread
 from twisted.protocols import amp
 from twisted.python import log
+from twisted.web import http
+import twisted.web.client
 from twisted.web.client import getPage
 from zope.interface import implementer
 
@@ -380,6 +383,49 @@
         log.msg("Peer certificate: %r" % self.peerCertificate)
 
 
+class PatchedURI(twisted.web.client._URI):
+
+    @classmethod
+    def fromBytes(cls, uri, defaultPort=None):
+        """Patched replacement for `twisted.web.client._URI.fromBytes`.
+
+        The Twisted version of this function breaks when you give it a URL
+        whose netloc is based on an IPv6 address.
+        """
+        uri = uri.strip()
+        scheme, netloc, path, params, query, fragment = http.urlparse(uri)
+
+        if defaultPort is None:
+            scheme_ports = {
+                'https': 443,
+                'http': 80,
+                }
+            defaultPort = scheme_ports.get(scheme, 80)
+
+        if '[' in netloc:
+            # IPv6 address.  This is complicated.
+            parsed_netloc = re.match(
+                '(?P<host>\\[[0-9A-Fa-f:.]+\\])([:](?P<port>[0-9]+))?$',
+                netloc)
+            host, port = parsed_netloc.group('host', 'port')
+        elif ':' in netloc:
+            # IPv4 address or hostname, with port spec.  This is easy.
+            host, port = netloc.split(':')
+        else:
+            # IPv4 address or hostname, without port spec.  This is trivial.
+            host = netloc
+            port = None
+
+        if port is None:
+            port = defaultPort
+        try:
+            port = int(port)
+        except ValueError:
+            port = defaultPort
+
+        return cls(scheme, netloc, host, port, path, params, query, fragment)
+
+
 class ClusterClientService(TimerService, object):
     """A cluster controller RPC client service.
 
@@ -402,6 +448,14 @@
         self.connections = {}
         self.clock = reactor
 
+        # XXX jtv 2014-09-23, bug=1372767: Fix
+        # twisted.web.client._URI.fromBytes to handle IPv6 addresses.
+        # A `getPage` call on Twisted's web client breaks if you give it a
+        # URL with an IPv6 address, at the point where `_makeGetterFactory`
+        # calls `fromBytes`.  That last function assumes that a colon can only
+        # occur in the URL's netloc portion as part of a port specification.
+        twisted.web.client._URI = PatchedURI
+
     def getClient(self):
         """Returns a :class:`common.Client` connected to a region.
 

=== modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py'
--- src/provisioningserver/rpc/tests/test_clusterservice.py	2014-09-23 09:24:19 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2014-09-23 11:50:34 +0000
@@ -39,6 +39,7 @@
     MAASTwistedRunTest,
     )
 from mock import (
+    ANY,
     call,
     Mock,
     sentinel,
@@ -71,6 +72,7 @@
     Cluster,
     ClusterClient,
     ClusterClientService,
+    PatchedURI,
     )
 from provisioningserver.rpc.interfaces import IConnection
 from provisioningserver.rpc.monitors import (
@@ -105,7 +107,10 @@
     Not,
     )
 from twisted.application.internet import TimerService
-from twisted.internet import error
+from twisted.internet import (
+    error,
+    reactor,
+    )
 from twisted.internet.defer import (
     inlineCallbacks,
     succeed,
@@ -305,6 +310,60 @@
             JSON_POWER_TYPE_PARAMETERS, response["power_types"])
 
 
+class TestPatchedURI(MAASTestCase):
+
+    def test__parses_URL_with_hostname(self):
+        hostname = factory.make_name('host').encode('ascii')
+        path = factory.make_name('path').encode('ascii')
+        uri = PatchedURI.fromBytes(b'http://%s/%s' % (hostname, path))
+        self.expectThat(uri.host, Equals(hostname))
+        self.expectThat(uri.path, Equals('/%s' % path))
+        self.expectThat(uri.port, Equals(80))
+
+    def test__parses_URL_with_hostname_and_port(self):
+        hostname = factory.make_name('host').encode('ascii')
+        port = factory.pick_port()
+        path = factory.make_name('path').encode('ascii')
+        uri = PatchedURI.fromBytes(b'http://%s:%d/%s' % (hostname, port, path))
+        self.expectThat(uri.host, Equals(hostname))
+        self.expectThat(uri.path, Equals('/%s' % path))
+        self.expectThat(uri.port, Equals(port))
+
+    def test__parses_URL_with_IPv4_address(self):
+        ip = factory.make_ipv4_address().encode('ascii')
+        path = factory.make_name('path').encode('ascii')
+        uri = PatchedURI.fromBytes(b'http://%s/%s' % (ip, path))
+        self.expectThat(uri.host, Equals(ip.encode('ascii')))
+        self.expectThat(uri.path, Equals('/%s' % path))
+        self.expectThat(uri.port, Equals(80))
+
+    def test__parses_URL_with_IPv4_address_and_port(self):
+        ip = factory.make_ipv4_address().encode('ascii')
+        port = factory.pick_port()
+        path = factory.make_name('path').encode('ascii')
+        uri = PatchedURI.fromBytes(b'http://%s:%d/%s' % (ip, port, path))
+        self.expectThat(uri.host, Equals(ip))
+        self.expectThat(uri.path, Equals('/%s' % path))
+        self.expectThat(uri.port, Equals(port))
+
+    def test__parses_URL_with_IPv6_address(self):
+        ip = factory.make_ipv6_address().encode('ascii')
+        path = factory.make_name('path').encode('ascii')
+        uri = PatchedURI.fromBytes(b'http://[%s]/%s' % (ip, path))
+        self.expectThat(uri.host, Equals(b'[%s]' % ip))
+        self.expectThat(uri.path, Equals(b'/%s' % path))
+        self.expectThat(uri.port, Equals(80))
+
+    def test__parses_URL_with_IPv6_address_and_port(self):
+        ip = factory.make_ipv6_address().encode('ascii')
+        port = factory.pick_port()
+        path = factory.make_name('path').encode('ascii')
+        uri = PatchedURI.fromBytes(b'http://[%s]:%d/%s' % (ip, port, path))
+        self.expectThat(uri.host, Equals(b'[%s]' % ip))
+        self.expectThat(uri.path, Equals(b'/%s' % path))
+        self.expectThat(uri.port, Equals(port))
+
+
 class TestClusterClientService(MAASTestCase):
 
     run_tests_with = MAASTwistedRunTest.make_factory(timeout=5)
@@ -340,6 +399,13 @@
             "Region not available: Connection was refused by other side.",
             logger.dump())
 
+    def test__get_rpc_info_accepts_IPv6_url(self):
+        connect_tcp = self.patch_autospec(reactor, 'connectTCP')
+        url = factory.make_url(
+            scheme='http', netloc='[%s]' % factory.make_ipv6_address())
+        ClusterClientService(Clock())._fetch_rpc_info(url.encode('ascii'))
+        self.assertThat(connect_tcp, MockCalledOnceWith(ANY, ANY, ANY))
+
     # The following represents an example response from the RPC info
     # view in maasserver. Event-loops listen on ephemeral ports, and
     # it's up to the RPC info view to direct clients to them.