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
1=== modified file 'NEWS'
2--- NEWS 2009-10-30 00:00:37 +0000
3+++ NEWS 2009-10-30 21:10:26 +0000
4@@ -36,6 +36,9 @@
5 fails intermittently with "second push failed to complete".
6 (Andrew Bennetts, #437626)
7
8+* Launchpad urls can now be resolved from behind proxies.
9+ (Gordon Tyler, Vincent Ladeuil, #198920)
10+
11 * PreviewTree file names are not limited by the encoding of the temp
12 directory's filesystem. (Aaron Bentley, #436794)
13
14
15=== modified file 'bzrlib/plugins/launchpad/__init__.py'
16--- bzrlib/plugins/launchpad/__init__.py 2009-07-06 13:00:23 +0000
17+++ bzrlib/plugins/launchpad/__init__.py 2009-10-30 21:10:26 +0000
18@@ -262,30 +262,19 @@
19 _register_directory()
20
21
22-def test_suite():
23- """Called by bzrlib to fetch tests for this plugin"""
24- from unittest import TestSuite, TestLoader
25- from bzrlib.plugins.launchpad import (
26- test_account,
27- test_lp_directory,
28- test_lp_login,
29- test_lp_open,
30- test_lp_service,
31- test_register,
32- )
33+def load_tests(basic_tests, module, loader):
34+ testmod_names = [
35+ 'test_account',
36+ 'test_register',
37+ 'test_lp_directory',
38+ 'test_lp_login',
39+ 'test_lp_open',
40+ 'test_lp_service',
41+ ]
42+ basic_tests.addTest(loader.loadTestsFromModuleNames(
43+ ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
44+ return basic_tests
45
46- loader = TestLoader()
47- suite = TestSuite()
48- for module in [
49- test_account,
50- test_register,
51- test_lp_directory,
52- test_lp_login,
53- test_lp_open,
54- test_lp_service,
55- ]:
56- suite.addTests(loader.loadTestsFromModule(module))
57- return suite
58
59 _launchpad_help = """Integration with Launchpad.net
60
61
62=== modified file 'bzrlib/plugins/launchpad/lp_registration.py'
63--- bzrlib/plugins/launchpad/lp_registration.py 2009-07-04 16:22:16 +0000
64+++ bzrlib/plugins/launchpad/lp_registration.py 2009-10-30 21:10:26 +0000
65@@ -31,6 +31,7 @@
66 errors,
67 __version__ as _bzrlib_version,
68 )
69+from bzrlib.transport.http import _urllib2_wrappers
70
71 # for testing, do
72 '''
73@@ -53,6 +54,29 @@
74 errors.BzrError.__init__(self, url=url)
75
76
77+class XMLRPCTransport(xmlrpclib.Transport):
78+
79+ def __init__(self, scheme, use_datetime=0):
80+ xmlrpclib.Transport.__init__(self, use_datetime=use_datetime)
81+ self._scheme = scheme
82+ self._opener = _urllib2_wrappers.Opener()
83+ self.verbose = 0
84+
85+ def request(self, host, handler, request_body, verbose=0):
86+ self.verbose = verbose
87+ url = self._scheme + "://" + host + handler
88+ request = _urllib2_wrappers.Request("POST", url, request_body)
89+ # FIXME: _urllib2_wrappers will override user-agent with its own
90+ # request.add_header("User-Agent", self.user_agent)
91+ request.add_header("Content-Type", "text/xml")
92+
93+ response = self._opener.open(request)
94+ if response.code != 200:
95+ raise xmlrpclib.ProtocolError(host + handler, response.code,
96+ response.msg, response.info())
97+ return self.parse_response(response)
98+
99+
100 class LaunchpadService(object):
101 """A service to talk to Launchpad via XMLRPC.
102
103@@ -90,10 +114,7 @@
104 self._lp_instance = lp_instance
105 if transport is None:
106 uri_type = urllib.splittype(self.service_url)[0]
107- if uri_type == 'https':
108- transport = xmlrpclib.SafeTransport()
109- else:
110- transport = xmlrpclib.Transport()
111+ transport = XMLRPCTransport(uri_type)
112 transport.user_agent = 'bzr/%s (xmlrpclib/%s)' \
113 % (_bzrlib_version, xmlrpclib.__version__)
114 self.transport = transport
115
116=== modified file 'bzrlib/plugins/launchpad/test_lp_directory.py'
117--- bzrlib/plugins/launchpad/test_lp_directory.py 2009-03-23 14:59:43 +0000
118+++ bzrlib/plugins/launchpad/test_lp_directory.py 2009-10-30 21:10:26 +0000
119@@ -16,10 +16,12 @@
120
121 """Tests for directory lookup through Launchpad.net"""
122
123+import os
124 import xmlrpclib
125
126 from bzrlib import (
127 errors,
128+ tests,
129 )
130 from bzrlib.branch import Branch
131 from bzrlib.directory_service import directories
132@@ -28,10 +30,38 @@
133 TestCaseWithMemoryTransport
134 )
135 from bzrlib.transport import get_transport
136-from bzrlib.plugins.launchpad import _register_directory
137+from bzrlib.plugins.launchpad import (
138+ _register_directory,
139+ lp_registration,
140+ )
141 from bzrlib.plugins.launchpad.lp_directory import (
142 LaunchpadDirectory)
143 from bzrlib.plugins.launchpad.account import get_lp_login
144+from bzrlib.tests import (
145+ http_server,
146+ http_utils,
147+ )
148+
149+
150+def load_tests(standard_tests, module, loader):
151+ result = loader.suiteClass()
152+ t_tests, remaining_tests = tests.split_suite_by_condition(
153+ standard_tests, tests.condition_isinstance((
154+ TestXMLRPCTransport,
155+ )))
156+ transport_scenarios = [
157+ ('http', dict(server_class=PreCannedHTTPServer,)),
158+ ]
159+ if tests.HTTPSServerFeature.available():
160+ transport_scenarios.append(
161+ ('https', dict(server_class=PreCannedHTTPSServer,)),
162+ )
163+ tests.multiply_tests(t_tests, transport_scenarios, result)
164+
165+ # No parametrization for the remaining tests
166+ result.addTests(remaining_tests)
167+
168+ return result
169
170
171 class FakeResolveFactory(object):
172@@ -190,3 +220,116 @@
173 transport = get_transport('lp:///apt')
174 branch = Branch.open_from_transport(transport)
175 self.assertEqual(target_branch.base, branch.base)
176+
177+
178+class PredefinedRequestHandler(http_server.TestingHTTPRequestHandler):
179+ """Request handler for a unique and pre-defined request.
180+
181+ The only thing we care about here is that we receive a connection. But
182+ since we want to dialog with a real http client, we have to send it correct
183+ responses.
184+
185+ We expect to receive a *single* request nothing more (and we won't even
186+ check what request it is), the tests will recognize us from our response.
187+ """
188+
189+ def handle_one_request(self):
190+ tcs = self.server.test_case_server
191+ requestline = self.rfile.readline()
192+ headers = self.MessageClass(self.rfile, 0)
193+ if requestline.startswith('POST'):
194+ # The body should be a single line (or we don't know where it ends
195+ # and we don't want to issue a blocking read)
196+ body = self.rfile.readline()
197+
198+ self.wfile.write(tcs.canned_response)
199+
200+
201+class PreCannedServerMixin(object):
202+
203+ def __init__(self):
204+ super(PreCannedServerMixin, self).__init__(
205+ request_handler=PredefinedRequestHandler)
206+ # Bytes read and written by the server
207+ self.bytes_read = 0
208+ self.bytes_written = 0
209+ self.canned_response = None
210+
211+
212+class PreCannedHTTPServer(PreCannedServerMixin, http_server.HttpServer):
213+ pass
214+
215+
216+if tests.HTTPSServerFeature.available():
217+ from bzrlib.tests import https_server
218+ class PreCannedHTTPSServer(PreCannedServerMixin, https_server.HTTPSServer):
219+ pass
220+
221+
222+class TestXMLRPCTransport(tests.TestCase):
223+
224+ # set by load_tests
225+ server_class = None
226+
227+ def setUp(self):
228+ tests.TestCase.setUp(self)
229+ self.server = self.server_class()
230+ self.server.setUp()
231+ # Ensure we don't clobber env
232+ self._captureVar('BZR_LP_XMLRPC_URL', None)
233+
234+ def tearDown(self):
235+ self.server.tearDown()
236+ tests.TestCase.tearDown(self)
237+
238+ def set_canned_response(self, server, path):
239+ response_format = '''HTTP/1.1 200 OK\r
240+Date: Tue, 11 Jul 2006 04:32:56 GMT\r
241+Server: Apache/2.0.54 (Fedora)\r
242+Last-Modified: Sun, 23 Apr 2006 19:35:20 GMT\r
243+ETag: "56691-23-38e9ae00"\r
244+Accept-Ranges: bytes\r
245+Content-Length: %(length)d\r
246+Connection: close\r
247+Content-Type: text/plain; charset=UTF-8\r
248+\r
249+<?xml version='1.0'?>
250+<methodResponse>
251+<params>
252+<param>
253+<value><struct>
254+<member>
255+<name>urls</name>
256+<value><array><data>
257+<value><string>bzr+ssh://bazaar.launchpad.net/%(path)s</string></value>
258+<value><string>http://bazaar.launchpad.net/%(path)s</string></value>
259+</data></array></value>
260+</member>
261+</struct></value>
262+</param>
263+</params>
264+</methodResponse>
265+'''
266+ length = 334 + 2 * len(path)
267+ server.canned_response = response_format % dict(length=length,
268+ path=path)
269+
270+ def do_request(self, server_url):
271+ os.environ['BZR_LP_XMLRPC_URL'] = self.server.get_url()
272+ service = lp_registration.LaunchpadService()
273+ resolve = lp_registration.ResolveLaunchpadPathRequest('bzr')
274+ result = resolve.submit(service)
275+ return result
276+
277+ def test_direct_request(self):
278+ self.set_canned_response(self.server, '~bzr-pqm/bzr/bzr.dev')
279+ result = self.do_request(self.server.get_url())
280+ urls = result.get('urls', None)
281+ self.assertIsNot(None, urls)
282+ self.assertEquals(
283+ ['bzr+ssh://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev',
284+ 'http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev'],
285+ urls)
286+ # FIXME: we need to test with a real proxy, I can't find a way so simulate
287+ # CONNECT without leaving one server hanging the test :-/ Since that maybe
288+ # related to the leaking tests problems, I'll punt for now -- vila 20091030
289
290=== modified file 'bzrlib/tests/test_http.py'
291--- bzrlib/tests/test_http.py 2009-09-17 11:54:41 +0000
292+++ bzrlib/tests/test_http.py 2009-10-30 21:10:26 +0000
293@@ -1956,7 +1956,7 @@
294 pass
295
296
297-class TestActivity(tests.TestCase):
298+class TestActivityMixin(object):
299 """Test socket activity reporting.
300
301 We use a special purpose server to control the bytes sent and received and
302@@ -2100,3 +2100,57 @@
303 code, f = t._post('abc def end-of-body\n')
304 self.assertEqual('lalala whatever as long as itsssss\n', f.read())
305 self.assertActivitiesMatch()
306+
307+
308+class TestActivity(tests.TestCase, TestActivityMixin):
309+
310+ def setUp(self):
311+ tests.TestCase.setUp(self)
312+ self.server = self._activity_server(self._protocol_version)
313+ self.server.setUp()
314+ self.activities = {}
315+ def report_activity(t, bytes, direction):
316+ count = self.activities.get(direction, 0)
317+ count += bytes
318+ self.activities[direction] = count
319+
320+ # We override at class level because constructors may propagate the
321+ # bound method and render instance overriding ineffective (an
322+ # alternative would be to define a specific ui factory instead...)
323+ self.orig_report_activity = self._transport._report_activity
324+ self._transport._report_activity = report_activity
325+
326+ def tearDown(self):
327+ self._transport._report_activity = self.orig_report_activity
328+ self.server.tearDown()
329+ tests.TestCase.tearDown(self)
330+
331+
332+class TestNoReportActivity(tests.TestCase, TestActivityMixin):
333+
334+ def setUp(self):
335+ tests.TestCase.setUp(self)
336+ # Unlike TestActivity, we are really testing ReportingFileSocket and
337+ # ReportingSocket, so we don't need all the parametrization. Since
338+ # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
339+ # test them through their use by the transport than directly (that's a
340+ # bit less clean but far more simpler and effective).
341+ self.server = ActivityHTTPServer('HTTP/1.1')
342+ self._transport=_urllib.HttpTransport_urllib
343+
344+ self.server.setUp()
345+
346+ # We override at class level because constructors may propagate the
347+ # bound method and render instance overriding ineffective (an
348+ # alternative would be to define a specific ui factory instead...)
349+ self.orig_report_activity = self._transport._report_activity
350+ self._transport._report_activity = None
351+
352+ def tearDown(self):
353+ self._transport._report_activity = self.orig_report_activity
354+ self.server.tearDown()
355+ tests.TestCase.tearDown(self)
356+
357+ def assertActivitiesMatch(self):
358+ # Nothing to check here
359+ pass
360
361=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
362--- bzrlib/transport/http/_urllib2_wrappers.py 2009-08-19 16:33:39 +0000
363+++ bzrlib/transport/http/_urllib2_wrappers.py 2009-10-30 21:10:26 +0000
364@@ -80,10 +80,13 @@
365 self.filesock = filesock
366 self._report_activity = report_activity
367
368+ def report_activity(self, size, direction):
369+ if self._report_activity:
370+ self._report_activity(size, direction)
371
372 def read(self, size=1):
373 s = self.filesock.read(size)
374- self._report_activity(len(s), 'read')
375+ self.report_activity(len(s), 'read')
376 return s
377
378 def readline(self):
379@@ -93,7 +96,7 @@
380 # don't *need* the size parameter we'll stay with readline(self)
381 # -- vila 20090209
382 s = self.filesock.readline()
383- self._report_activity(len(s), 'read')
384+ self.report_activity(len(s), 'read')
385 return s
386
387 def __getattr__(self, name):
388@@ -106,13 +109,17 @@
389 self.sock = sock
390 self._report_activity = report_activity
391
392+ def report_activity(self, size, direction):
393+ if self._report_activity:
394+ self._report_activity(size, direction)
395+
396 def sendall(self, s, *args):
397 self.sock.sendall(s, *args)
398- self._report_activity(len(s), 'write')
399+ self.report_activity(len(s), 'write')
400
401 def recv(self, *args):
402 s = self.sock.recv(*args)
403- self._report_activity(len(s), 'read')
404+ self.report_activity(len(s), 'read')
405 return s
406
407 def makefile(self, mode='r', bufsize=-1):
408@@ -219,8 +226,7 @@
409 # we want to warn. But not below a given thresold.
410 _range_warning_thresold = 1024 * 1024
411
412- def __init__(self,
413- report_activity=None):
414+ def __init__(self, report_activity=None):
415 self._response = None
416 self._report_activity = report_activity
417 self._ranges_received_whole_file = None
418@@ -360,7 +366,13 @@
419
420 def set_proxy(self, proxy, type):
421 """Set the proxy and remember the proxied host."""
422- self.proxied_host = self.get_host()
423+ # We need to set the default port ourselves way before it gets set
424+ # in the HTTP[S]Connection object at build time.
425+ if self.type == 'https':
426+ conn_class = HTTPSConnection
427+ else:
428+ conn_class = HTTPConnection
429+ self.proxied_host = '%s:%s' % (self.get_host(), conn_class.default_port)
430 urllib2.Request.set_proxy(self, proxy, type)
431
432