On Wed, Jun 30, 2010 at 06:04:56PM +0200, Michael Hanselmann wrote: > Currently the RAPI client uses the urllib2 and httplib modules from > Python's standard library. They're used with pyOpenSSL in a very > fragile way, and there are known issues when receiving large > responses from a RAPI server. > > By switching to PycURL we leverage the power and stability of the > widely-used curl library (libcurl). This brings us much more > flexibility than before, and timeouts were easily implemented > (something that would have involved a lot of work with the built-in > modules). > > There's one small drawback: Programs using libcurl have to call > curl_global_init(3) (available as pycurl.global_init) when only > one thread is running and are supposed to call > curl_global_cleanup(3) (available as pycurl.global_cleanup) upon > exiting. A decorator is provided to simplify this.
What do you mean "when only one thread is running"? Can we skip this step in multithreaded programs? > Unittests for the new code are provided, increasing the test coverage > of the RAPI client from 74% to 89%. Nice :) > Signed-off-by: Michael Hanselmann <[email protected]> > --- > INSTALL | 4 +- > daemons/ganeti-watcher | 8 +- > lib/rapi/client.py | 465 > +++++++++++++++-------------------- > qa/ganeti-qa.py | 4 + > qa/qa_rapi.py | 6 +- > test/ganeti.rapi.client_unittest.py | 263 +++++++++++++++++--- > tools/move-instance | 15 +- > 7 files changed, 445 insertions(+), 320 deletions(-) > > - @staticmethod > - def _VerifySslCertCb(logger, _, cert, errnum, errdepth, ok): > - """Callback for SSL certificate verification. > - > - @param logger: Logging object > - > - """ > - if ok: > - log_fn = logger.debug > + logger.debug("Using cURL version %s", pycurl.version) > + > + sslver = _pycurl_version_fn()[5] this seems a bit cryptic; could you add either a comment with that that function should return, or a comment with the actual function being used? (I had to dig it up from the keyword args to this function). > - ssl = OpenSSL.SSL.Connection(ctx, sock) > - ssl.connect((self.host, self.port)) > - > - if self._SUPPORT_FAKESOCKET: > - self.sock = httplib.FakeSocket(sock, ssl) > + raise NotImplementedError("cURL uses unsupported SSL version '%s'" % > + sslver) > + > + curl.setopt(pycurl.VERBOSE, verbose) > + curl.setopt(pycurl.NOSIGNAL, not use_signal) > + > + # Whether to verify remote peer's CN > + if verify_hostname: > + # curl_easy_setopt(3): "When CURLOPT_SSL_VERIFYHOST is 2, that > + # certificate must indicate that the server is the server to which you > + # meant to connect, or the connection fails. [...] When the value is 1, > + # the certificate must contain a Common Name field, but it doesn't > matter > + # what name it says. [...]" > + curl.setopt(pycurl.SSL_VERIFYHOST, 2) thanks for the nice comment. > @@ -355,25 +254,37 @@ class GanetiRapiClient(object): > > self._base_url = "https://%s:%s"; % (host, port) > > - handlers = [_HTTPSHandler(self._logger, config_ssl_verification)] > - > + # Create pycURL object if not supplied > + if not curl: > + curl = pycurl.Curl() > + > + # Default cURL settings > + curl.setopt(pycurl.VERBOSE, False) > + curl.setopt(pycurl.FOLLOWLOCATION, True) why followlocation? we shouldn't have this in ganeti rapi daemon, right? > + curl.setopt(pycurl.MAXREDIRS, 5) > + curl.setopt(pycurl.NOSIGNAL, True) > + curl.setopt(pycurl.USERAGENT, self.USER_AGENT) > + curl.setopt(pycurl.SSL_VERIFYHOST, 0) > + curl.setopt(pycurl.SSL_VERIFYPEER, False) > + curl.setopt(pycurl.HTTPHEADER, [ > + "Accept: %s" % HTTP_APP_JSON, > + "Content-type: %s" % HTTP_APP_JSON, > + ]) > + > try: > - resp = self._http.open(req) > - encoded_response_content = resp.read() > - except (OpenSSL.SSL.Error, OpenSSL.crypto.Error), err: > - raise CertificateError("SSL issue: %s (%r)" % (err, err)) > - except urllib2.HTTPError, err: > - raise GanetiApiError(str(err), code=err.code) > - except urllib2.URLError, err: > - raise GanetiApiError(str(err)) > - > - if encoded_response_content: > - response_content = simplejson.loads(encoded_response_content) > + # Send request and wait for response > + try: > + curl.perform() > + except pycurl.error, err: > + if err.args[0] in _CURL_SSL_CERT_ERRORS: > + raise CertificateError("SSL certificate error %s" % err) > + > + raise GanetiApiError(str(err)) > + finally: > + # Reset settings to not keep references to large objects in memory > + # between requests > + curl.setopt(pycurl.POSTFIELDS, "") > + curl.setopt(pycurl.WRITEFUNCTION, lambda _: None) > + > + # Get HTTP response code > + http_code = curl.getinfo(pycurl.RESPONSE_CODE) > + > + if encoded_resp_body.tell(): > + response_content = simplejson.loads(encoded_resp_body.getvalue()) > else: > response_content = None I'm a bit confused how this tell() works… Can you clarify? > + def testCertVerifyCurlBundle(self): > + cfgfn = client.GenericCurlConfig(use_curl_cabundle=True) > + > + curl = FakeCurl(RapiMock()) > + cl = client.GanetiRapiClient("master.foo.com", curl_config_fn=cfgfn, > + curl=curl) s/foo.com/example.com/ ? > + self.assert_(curl.getopt(pycurl.SSL_VERIFYPEER)) > + self.assertFalse(curl.getopt(pycurl.CAINFO)) > + self.assertFalse(curl.getopt(pycurl.CAPATH)) > + > + def testCertVerifyCafile(self): > + mycert = "/tmp/some/cert/file.pem" maybe adding a comment that these paths (here and below) are not actually used would be helpful :) I didn't do a very detailed check, but it seems fine. I think I give a half-LGTM, I'll take a closer look tomorrow. iustin
