Bazaar

Merge lp:~vila/bzr/186920-lp-proxy into lp:bzr

  1. 186920-lp-proxy
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vila/bzr/186920-lp-proxy
Merge into: lp:bzr
Diff against target: 430 lines
6 files modified
NEWS (+3/-0)
bzrlib/plugins/launchpad/__init__.py (+12/-23)
bzrlib/plugins/launchpad/lp_registration.py (+25/-4)
bzrlib/plugins/launchpad/test_lp_directory.py (+144/-1)
bzrlib/tests/test_http.py (+55/-1)
bzrlib/transport/http/_urllib2_wrappers.py (+19/-7)
To merge this branch: bzr merge lp:~vila/bzr/186920-lp-proxy
Reviewer Review Type Date Requested Status
John A Meinel Pending
Review via email: mp+14238@code.launchpad.net

This proposal supersedes a proposal from 2009-10-30.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

Thanks to Gordon Tyler for starting a proof-of-concept implementation, I've finally
been able to fix bug #186920 et all (the one about lp URLS that can't be
resolved behind proxies).

This patch implement an XMLRPC transport class using our _urllib2_wrappers (the fundations
for the urllib http client), that gives us proxy support for free.

I still haven't implemented a true proxy test server but I've added tests that exercise
the full urllib http stack.

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/186920-lp-proxy into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> Thanks to Gordon Tyler for starting a proof-of-concept implementation, I've finally
> been able to fix bug #186920 et all (the one about lp URLS that can't be
> resolved behind proxies).
>
> This patch implement an XMLRPC transport class using our _urllib2_wrappers (the fundations
> for the urllib http client), that gives us proxy support for free.
>
> I still haven't implemented a true proxy test server but I've added tests that exercise
> the full urllib http stack.
>

+class Transport(xmlrpclib.Transport):
+
+ def __init__(self, scheme, use_datetime=0):

^- Shouldn't we have our own name here? We *can* re-use the existing
name, but it is potentially confusing.

Especially given that we have a "Transport" hierarchy already (and not
to mention that so does Paramiko), I'd like to at least call it
"XMLRPCTransport".

Also, it seems that Gordon responded that there was a problem with not
passing the original port to the proxy? Did you fix that in this? Or did
his comment come after the merge proposal?

I assume that the http/https trick is this line:

+ self._opener = _urllib2_wrappers.Opener()

(Opener supposedly creates the right instance of HTTP or HTTPS based on
the url/the url of the proxy?) I assume that is why you don't need:
- - if uri_type == 'https':
- - transport = xmlrpclib.SafeTransport()
- - else:
- - transport = xmlrpclib.Transport()
+ transport = Transport(uri_type)

anymore.

+
+ self.wfile.write(tcs.canned_response)
+
+class PreCannedServerMixin(object):

^- bit of whitespace here

Otherwise:

  review: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrrGn4ACgkQJdeBCYSNAAN9SQCdFnI2kiY1Pykdz2xEMmQtYVBZ
VZ0An3B1OXrswFOFltoF0851r217zJiF
=Vmrj
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> +class Transport(xmlrpclib.Transport):
    jam> +
    jam> + def __init__(self, scheme, use_datetime=0):

    jam> ^- Shouldn't we have our own name here? We *can* re-use
    jam> the existing name, but it is potentially confusing.

    jam> Especially given that we have a "Transport" hierarchy
    jam> already (and not to mention that so does Paramiko), I'd
    jam> like to at least call it "XMLRPCTransport".

Sure, done.

    jam> Also, it seems that Gordon responded that there was a
    jam> problem with not passing the original port to the proxy?
    jam> Did you fix that in this? Or did his comment come after
    jam> the merge proposal?

His comment went after the mp, I've fixed it since so I'll
resubmit.

    jam> I assume that the http/https trick is this line:

    jam> + self._opener = _urllib2_wrappers.Opener()

    jam> (Opener supposedly creates the right instance of HTTP or HTTPS based on
    jam> the url/the url of the proxy?) I assume that is why you don't need:
    jam> - if uri_type == 'https':
    jam> - transport = xmlrpclib.SafeTransport()
    jam> - else:
    jam> - transport = xmlrpclib.Transport()
    jam> + transport = Transport(uri_type)

    jam> anymore.

Correct. I could have refactored even more aggressively (to pass
the full service url and set the agent) but I wanted to keep the
patch as minimal as possible.

        Vincent

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2009-10-30 00:00:37 +0000
+++ NEWS 2009-10-30 21:10:26 +0000
@@ -36,6 +36,9 @@
36 fails intermittently with "second push failed to complete".36 fails intermittently with "second push failed to complete".
37 (Andrew Bennetts, #437626)37 (Andrew Bennetts, #437626)
3838
39* Launchpad urls can now be resolved from behind proxies.
40 (Gordon Tyler, Vincent Ladeuil, #198920)
41
39* PreviewTree file names are not limited by the encoding of the temp42* PreviewTree file names are not limited by the encoding of the temp
40 directory's filesystem. (Aaron Bentley, #436794)43 directory's filesystem. (Aaron Bentley, #436794)
4144
4245
=== modified file 'bzrlib/plugins/launchpad/__init__.py'
--- bzrlib/plugins/launchpad/__init__.py 2009-07-06 13:00:23 +0000
+++ bzrlib/plugins/launchpad/__init__.py 2009-10-30 21:10:26 +0000
@@ -262,30 +262,19 @@
262_register_directory()262_register_directory()
263263
264264
265def test_suite():265def load_tests(basic_tests, module, loader):
266 """Called by bzrlib to fetch tests for this plugin"""266 testmod_names = [
267 from unittest import TestSuite, TestLoader267 'test_account',
268 from bzrlib.plugins.launchpad import (268 'test_register',
269 test_account,269 'test_lp_directory',
270 test_lp_directory,270 'test_lp_login',
271 test_lp_login,271 'test_lp_open',
272 test_lp_open,272 'test_lp_service',
273 test_lp_service,273 ]
274 test_register,274 basic_tests.addTest(loader.loadTestsFromModuleNames(
275 )275 ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
276 return basic_tests
276277
277 loader = TestLoader()
278 suite = TestSuite()
279 for module in [
280 test_account,
281 test_register,
282 test_lp_directory,
283 test_lp_login,
284 test_lp_open,
285 test_lp_service,
286 ]:
287 suite.addTests(loader.loadTestsFromModule(module))
288 return suite
289278
290_launchpad_help = """Integration with Launchpad.net279_launchpad_help = """Integration with Launchpad.net
291280
292281
=== modified file 'bzrlib/plugins/launchpad/lp_registration.py'
--- bzrlib/plugins/launchpad/lp_registration.py 2009-07-04 16:22:16 +0000
+++ bzrlib/plugins/launchpad/lp_registration.py 2009-10-30 21:10:26 +0000
@@ -31,6 +31,7 @@
31 errors,31 errors,
32 __version__ as _bzrlib_version,32 __version__ as _bzrlib_version,
33 )33 )
34from bzrlib.transport.http import _urllib2_wrappers
3435
35# for testing, do36# for testing, do
36'''37'''
@@ -53,6 +54,29 @@
53 errors.BzrError.__init__(self, url=url)54 errors.BzrError.__init__(self, url=url)
5455
5556
57class XMLRPCTransport(xmlrpclib.Transport):
58
59 def __init__(self, scheme, use_datetime=0):
60 xmlrpclib.Transport.__init__(self, use_datetime=use_datetime)
61 self._scheme = scheme
62 self._opener = _urllib2_wrappers.Opener()
63 self.verbose = 0
64
65 def request(self, host, handler, request_body, verbose=0):
66 self.verbose = verbose
67 url = self._scheme + "://" + host + handler
68 request = _urllib2_wrappers.Request("POST", url, request_body)
69 # FIXME: _urllib2_wrappers will override user-agent with its own
70 # request.add_header("User-Agent", self.user_agent)
71 request.add_header("Content-Type", "text/xml")
72
73 response = self._opener.open(request)
74 if response.code != 200:
75 raise xmlrpclib.ProtocolError(host + handler, response.code,
76 response.msg, response.info())
77 return self.parse_response(response)
78
79
56class LaunchpadService(object):80class LaunchpadService(object):
57 """A service to talk to Launchpad via XMLRPC.81 """A service to talk to Launchpad via XMLRPC.
5882
@@ -90,10 +114,7 @@
90 self._lp_instance = lp_instance114 self._lp_instance = lp_instance
91 if transport is None:115 if transport is None:
92 uri_type = urllib.splittype(self.service_url)[0]116 uri_type = urllib.splittype(self.service_url)[0]
93 if uri_type == 'https':117 transport = XMLRPCTransport(uri_type)
94 transport = xmlrpclib.SafeTransport()
95 else:
96 transport = xmlrpclib.Transport()
97 transport.user_agent = 'bzr/%s (xmlrpclib/%s)' \118 transport.user_agent = 'bzr/%s (xmlrpclib/%s)' \
98 % (_bzrlib_version, xmlrpclib.__version__)119 % (_bzrlib_version, xmlrpclib.__version__)
99 self.transport = transport120 self.transport = transport
100121
=== modified file 'bzrlib/plugins/launchpad/test_lp_directory.py'
--- bzrlib/plugins/launchpad/test_lp_directory.py 2009-03-23 14:59:43 +0000
+++ bzrlib/plugins/launchpad/test_lp_directory.py 2009-10-30 21:10:26 +0000
@@ -16,10 +16,12 @@
1616
17"""Tests for directory lookup through Launchpad.net"""17"""Tests for directory lookup through Launchpad.net"""
1818
19import os
19import xmlrpclib20import xmlrpclib
2021
21from bzrlib import (22from bzrlib import (
22 errors,23 errors,
24 tests,
23 )25 )
24from bzrlib.branch import Branch26from bzrlib.branch import Branch
25from bzrlib.directory_service import directories27from bzrlib.directory_service import directories
@@ -28,10 +30,38 @@
28 TestCaseWithMemoryTransport30 TestCaseWithMemoryTransport
29)31)
30from bzrlib.transport import get_transport32from bzrlib.transport import get_transport
31from bzrlib.plugins.launchpad import _register_directory33from bzrlib.plugins.launchpad import (
34 _register_directory,
35 lp_registration,
36 )
32from bzrlib.plugins.launchpad.lp_directory import (37from bzrlib.plugins.launchpad.lp_directory import (
33 LaunchpadDirectory)38 LaunchpadDirectory)
34from bzrlib.plugins.launchpad.account import get_lp_login39from bzrlib.plugins.launchpad.account import get_lp_login
40from bzrlib.tests import (
41 http_server,
42 http_utils,
43 )
44
45
46def load_tests(standard_tests, module, loader):
47 result = loader.suiteClass()
48 t_tests, remaining_tests = tests.split_suite_by_condition(
49 standard_tests, tests.condition_isinstance((
50 TestXMLRPCTransport,
51 )))
52 transport_scenarios = [
53 ('http', dict(server_class=PreCannedHTTPServer,)),
54 ]
55 if tests.HTTPSServerFeature.available():
56 transport_scenarios.append(
57 ('https', dict(server_class=PreCannedHTTPSServer,)),
58 )
59 tests.multiply_tests(t_tests, transport_scenarios, result)
60
61 # No parametrization for the remaining tests
62 result.addTests(remaining_tests)
63
64 return result
3565
3666
37class FakeResolveFactory(object):67class FakeResolveFactory(object):
@@ -190,3 +220,116 @@
190 transport = get_transport('lp:///apt')220 transport = get_transport('lp:///apt')
191 branch = Branch.open_from_transport(transport)221 branch = Branch.open_from_transport(transport)
192 self.assertEqual(target_branch.base, branch.base)222 self.assertEqual(target_branch.base, branch.base)
223
224
225class PredefinedRequestHandler(http_server.TestingHTTPRequestHandler):
226 """Request handler for a unique and pre-defined request.
227
228 The only thing we care about here is that we receive a connection. But
229 since we want to dialog with a real http client, we have to send it correct
230 responses.
231
232 We expect to receive a *single* request nothing more (and we won't even
233 check what request it is), the tests will recognize us from our response.
234 """
235
236 def handle_one_request(self):
237 tcs = self.server.test_case_server
238 requestline = self.rfile.readline()
239 headers = self.MessageClass(self.rfile, 0)
240 if requestline.startswith('POST'):
241 # The body should be a single line (or we don't know where it ends
242 # and we don't want to issue a blocking read)
243 body = self.rfile.readline()
244
245 self.wfile.write(tcs.canned_response)
246
247
248class PreCannedServerMixin(object):
249
250 def __init__(self):
251 super(PreCannedServerMixin, self).__init__(
252 request_handler=PredefinedRequestHandler)
253 # Bytes read and written by the server
254 self.bytes_read = 0
255 self.bytes_written = 0
256 self.canned_response = None
257
258
259class PreCannedHTTPServer(PreCannedServerMixin, http_server.HttpServer):
260 pass
261
262
263if tests.HTTPSServerFeature.available():
264 from bzrlib.tests import https_server
265 class PreCannedHTTPSServer(PreCannedServerMixin, https_server.HTTPSServer):
266 pass
267
268
269class TestXMLRPCTransport(tests.TestCase):
270
271 # set by load_tests
272 server_class = None
273
274 def setUp(self):
275 tests.TestCase.setUp(self)
276 self.server = self.server_class()
277 self.server.setUp()
278 # Ensure we don't clobber env
279 self._captureVar('BZR_LP_XMLRPC_URL', None)
280
281 def tearDown(self):
282 self.server.tearDown()
283 tests.TestCase.tearDown(self)
284
285 def set_canned_response(self, server, path):
286 response_format = '''HTTP/1.1 200 OK\r
287Date: Tue, 11 Jul 2006 04:32:56 GMT\r
288Server: Apache/2.0.54 (Fedora)\r
289Last-Modified: Sun, 23 Apr 2006 19:35:20 GMT\r
290ETag: "56691-23-38e9ae00"\r
291Accept-Ranges: bytes\r
292Content-Length: %(length)d\r
293Connection: close\r
294Content-Type: text/plain; charset=UTF-8\r
295\r
296<?xml version='1.0'?>
297<methodResponse>
298<params>
299<param>
300<value><struct>
301<member>
302<name>urls</name>
303<value><array><data>
304<value><string>bzr+ssh://bazaar.launchpad.net/%(path)s</string></value>
305<value><string>http://bazaar.launchpad.net/%(path)s</string></value>
306</data></array></value>
307</member>
308</struct></value>
309</param>
310</params>
311</methodResponse>
312'''
313 length = 334 + 2 * len(path)
314 server.canned_response = response_format % dict(length=length,
315 path=path)
316
317 def do_request(self, server_url):
318 os.environ['BZR_LP_XMLRPC_URL'] = self.server.get_url()
319 service = lp_registration.LaunchpadService()
320 resolve = lp_registration.ResolveLaunchpadPathRequest('bzr')
321 result = resolve.submit(service)
322 return result
323
324 def test_direct_request(self):
325 self.set_canned_response(self.server, '~bzr-pqm/bzr/bzr.dev')
326 result = self.do_request(self.server.get_url())
327 urls = result.get('urls', None)
328 self.assertIsNot(None, urls)
329 self.assertEquals(
330 ['bzr+ssh://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev',
331 'http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev'],
332 urls)
333 # FIXME: we need to test with a real proxy, I can't find a way so simulate
334 # CONNECT without leaving one server hanging the test :-/ Since that maybe
335 # related to the leaking tests problems, I'll punt for now -- vila 20091030
193336
=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py 2009-09-17 11:54:41 +0000
+++ bzrlib/tests/test_http.py 2009-10-30 21:10:26 +0000
@@ -1956,7 +1956,7 @@
1956 pass1956 pass
19571957
19581958
1959class TestActivity(tests.TestCase):1959class TestActivityMixin(object):
1960 """Test socket activity reporting.1960 """Test socket activity reporting.
19611961
1962 We use a special purpose server to control the bytes sent and received and1962 We use a special purpose server to control the bytes sent and received and
@@ -2100,3 +2100,57 @@
2100 code, f = t._post('abc def end-of-body\n')2100 code, f = t._post('abc def end-of-body\n')
2101 self.assertEqual('lalala whatever as long as itsssss\n', f.read())2101 self.assertEqual('lalala whatever as long as itsssss\n', f.read())
2102 self.assertActivitiesMatch()2102 self.assertActivitiesMatch()
2103
2104
2105class TestActivity(tests.TestCase, TestActivityMixin):
2106
2107 def setUp(self):
2108 tests.TestCase.setUp(self)
2109 self.server = self._activity_server(self._protocol_version)
2110 self.server.setUp()
2111 self.activities = {}
2112 def report_activity(t, bytes, direction):
2113 count = self.activities.get(direction, 0)
2114 count += bytes
2115 self.activities[direction] = count
2116
2117 # We override at class level because constructors may propagate the
2118 # bound method and render instance overriding ineffective (an
2119 # alternative would be to define a specific ui factory instead...)
2120 self.orig_report_activity = self._transport._report_activity
2121 self._transport._report_activity = report_activity
2122
2123 def tearDown(self):
2124 self._transport._report_activity = self.orig_report_activity
2125 self.server.tearDown()
2126 tests.TestCase.tearDown(self)
2127
2128
2129class TestNoReportActivity(tests.TestCase, TestActivityMixin):
2130
2131 def setUp(self):
2132 tests.TestCase.setUp(self)
2133 # Unlike TestActivity, we are really testing ReportingFileSocket and
2134 # ReportingSocket, so we don't need all the parametrization. Since
2135 # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
2136 # test them through their use by the transport than directly (that's a
2137 # bit less clean but far more simpler and effective).
2138 self.server = ActivityHTTPServer('HTTP/1.1')
2139 self._transport=_urllib.HttpTransport_urllib
2140
2141 self.server.setUp()
2142
2143 # We override at class level because constructors may propagate the
2144 # bound method and render instance overriding ineffective (an
2145 # alternative would be to define a specific ui factory instead...)
2146 self.orig_report_activity = self._transport._report_activity
2147 self._transport._report_activity = None
2148
2149 def tearDown(self):
2150 self._transport._report_activity = self.orig_report_activity
2151 self.server.tearDown()
2152 tests.TestCase.tearDown(self)
2153
2154 def assertActivitiesMatch(self):
2155 # Nothing to check here
2156 pass
21032157
=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py 2009-08-19 16:33:39 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py 2009-10-30 21:10:26 +0000
@@ -80,10 +80,13 @@
80 self.filesock = filesock80 self.filesock = filesock
81 self._report_activity = report_activity81 self._report_activity = report_activity
8282
83 def report_activity(self, size, direction):
84 if self._report_activity:
85 self._report_activity(size, direction)
8386
84 def read(self, size=1):87 def read(self, size=1):
85 s = self.filesock.read(size)88 s = self.filesock.read(size)
86 self._report_activity(len(s), 'read')89 self.report_activity(len(s), 'read')
87 return s90 return s
8891
89 def readline(self):92 def readline(self):
@@ -93,7 +96,7 @@
93 # don't *need* the size parameter we'll stay with readline(self)96 # don't *need* the size parameter we'll stay with readline(self)
94 # -- vila 2009020997 # -- vila 20090209
95 s = self.filesock.readline()98 s = self.filesock.readline()
96 self._report_activity(len(s), 'read')99 self.report_activity(len(s), 'read')
97 return s100 return s
98101
99 def __getattr__(self, name):102 def __getattr__(self, name):
@@ -106,13 +109,17 @@
106 self.sock = sock109 self.sock = sock
107 self._report_activity = report_activity110 self._report_activity = report_activity
108111
112 def report_activity(self, size, direction):
113 if self._report_activity:
114 self._report_activity(size, direction)
115
109 def sendall(self, s, *args):116 def sendall(self, s, *args):
110 self.sock.sendall(s, *args)117 self.sock.sendall(s, *args)
111 self._report_activity(len(s), 'write')118 self.report_activity(len(s), 'write')
112119
113 def recv(self, *args):120 def recv(self, *args):
114 s = self.sock.recv(*args)121 s = self.sock.recv(*args)
115 self._report_activity(len(s), 'read')122 self.report_activity(len(s), 'read')
116 return s123 return s
117124
118 def makefile(self, mode='r', bufsize=-1):125 def makefile(self, mode='r', bufsize=-1):
@@ -219,8 +226,7 @@
219 # we want to warn. But not below a given thresold.226 # we want to warn. But not below a given thresold.
220 _range_warning_thresold = 1024 * 1024227 _range_warning_thresold = 1024 * 1024
221228
222 def __init__(self,229 def __init__(self, report_activity=None):
223 report_activity=None):
224 self._response = None230 self._response = None
225 self._report_activity = report_activity231 self._report_activity = report_activity
226 self._ranges_received_whole_file = None232 self._ranges_received_whole_file = None
@@ -360,7 +366,13 @@
360366
361 def set_proxy(self, proxy, type):367 def set_proxy(self, proxy, type):
362 """Set the proxy and remember the proxied host."""368 """Set the proxy and remember the proxied host."""
363 self.proxied_host = self.get_host()369 # We need to set the default port ourselves way before it gets set
370 # in the HTTP[S]Connection object at build time.
371 if self.type == 'https':
372 conn_class = HTTPSConnection
373 else:
374 conn_class = HTTPConnection
375 self.proxied_host = '%s:%s' % (self.get_host(), conn_class.default_port)
364 urllib2.Request.set_proxy(self, proxy, type)376 urllib2.Request.set_proxy(self, proxy, type)
365377
366378