From a5eba783d458af8103d89e5e5b541bbf835704d8 Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Fri, 30 Jul 2010 20:44:07 +0200 Subject: [PATCH] RAPI client: Don't re-use PycURL object With this patch, a new PycURL object will be created for each request. This should make the RAPI client safe for simultaneous calls from multiple threads. Unittests are adjusted accordingly. An unnecessary variable assignment is also removed from the unittest script. This patch survived a small QA and unittests. Signed-off-by: Michael Hanselmann Reviewed-by: Guido Trotter --- lib/rapi/client.py | 50 +++++++++++++------- test/ganeti.rapi.client_unittest.py | 89 +++++++++++++++++++++-------------- 2 files changed, 85 insertions(+), 54 deletions(-) diff --git a/lib/rapi/client.py b/lib/rapi/client.py index 444a1d2..4573375 100644 --- a/lib/rapi/client.py +++ b/lib/rapi/client.py @@ -243,7 +243,7 @@ class GanetiRapiClient(object): def __init__(self, host, port=GANETI_RAPI_PORT, username=None, password=None, logger=logging, - curl_config_fn=None, curl=None): + curl_config_fn=None, curl_factory=None): """Initializes this class. @type host: string @@ -259,14 +259,28 @@ class GanetiRapiClient(object): @param logger: Logging object """ - self._host = host - self._port = port + self._username = username + self._password = password self._logger = logger + self._curl_config_fn = curl_config_fn + self._curl_factory = curl_factory self._base_url = "https://%s:%s" % (host, port) - # Create pycURL object if not supplied - if not curl: + if username is not None: + if password is None: + raise Error("Password not specified") + elif password: + raise Error("Specified password without username") + + def _CreateCurl(self): + """Creates a cURL object. + + """ + # Create pycURL object if no factory is provided + if self._curl_factory: + curl = self._curl_factory() + else: curl = pycurl.Curl() # Default cURL settings @@ -282,20 +296,20 @@ class GanetiRapiClient(object): "Content-type: %s" % HTTP_APP_JSON, ]) - # Setup authentication - if username is not None: - if password is None: - raise Error("Password not specified") + assert ((self._username is None and self._password is None) ^ + (self._username is not None and self._password is not None)) + + if self._username: + # Setup authentication curl.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_BASIC) - curl.setopt(pycurl.USERPWD, str("%s:%s" % (username, password))) - elif password: - raise Error("Specified password without username") + curl.setopt(pycurl.USERPWD, + str("%s:%s" % (self._username, self._password))) # Call external configuration function - if curl_config_fn: - curl_config_fn(curl, logger) + if self._curl_config_fn: + self._curl_config_fn(curl, self._logger) - self._curl = curl + return curl @staticmethod def _EncodeQuery(query): @@ -349,7 +363,7 @@ class GanetiRapiClient(object): """ assert path.startswith("/") - curl = self._curl + curl = self._CreateCurl() if content is not None: encoded_content = self._json_encoder.encode(content) @@ -364,8 +378,8 @@ class GanetiRapiClient(object): url = "".join(urlparts) - self._logger.debug("Sending request %s %s to %s:%s (content=%r)", - method, url, self._host, self._port, encoded_content) + self._logger.debug("Sending request %s %s (content=%r)", + method, url, encoded_content) # Buffer for response encoded_resp_body = StringIO() diff --git a/test/ganeti.rapi.client_unittest.py b/test/ganeti.rapi.client_unittest.py index c97b384..eaa29e5 100755 --- a/test/ganeti.rapi.client_unittest.py +++ b/test/ganeti.rapi.client_unittest.py @@ -170,11 +170,11 @@ def _FakeGnuTlsPycurlVersion(): class TestExtendedConfig(unittest.TestCase): def testAuth(self): - curl = FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", username="user", password="pw", - curl=curl) + curl_factory=lambda: FakeCurl(RapiMock())) + curl = cl._CreateCurl() self.assertEqual(curl.getopt(pycurl.HTTPAUTH), pycurl.HTTPAUTH_BASIC) self.assertEqual(curl.getopt(pycurl.USERPWD), "user:pw") @@ -209,10 +209,12 @@ class TestExtendedConfig(unittest.TestCase): verify_hostname=verify_hostname, _pycurl_version_fn=pcverfn) - curl = FakeCurl(RapiMock()) + curl_factory = lambda: FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", - curl_config_fn=cfgfn, curl=curl) + curl_config_fn=cfgfn, + curl_factory=curl_factory) + curl = cl._CreateCurl() self.assertEqual(curl.getopt(pycurl.PROXY), proxy) self.assertEqual(curl.getopt(pycurl.NOSIGNAL), not use_signal) @@ -224,10 +226,11 @@ class TestExtendedConfig(unittest.TestCase): def testNoCertVerify(self): cfgfn = client.GenericCurlConfig() - curl = FakeCurl(RapiMock()) + curl_factory = lambda: FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, - curl=curl) + curl_factory=curl_factory) + curl = cl._CreateCurl() self.assertFalse(curl.getopt(pycurl.SSL_VERIFYPEER)) self.assertFalse(curl.getopt(pycurl.CAINFO)) self.assertFalse(curl.getopt(pycurl.CAPATH)) @@ -235,10 +238,11 @@ class TestExtendedConfig(unittest.TestCase): def testCertVerifyCurlBundle(self): cfgfn = client.GenericCurlConfig(use_curl_cabundle=True) - curl = FakeCurl(RapiMock()) + curl_factory = lambda: FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, - curl=curl) + curl_factory=curl_factory) + curl = cl._CreateCurl() self.assert_(curl.getopt(pycurl.SSL_VERIFYPEER)) self.assertFalse(curl.getopt(pycurl.CAINFO)) self.assertFalse(curl.getopt(pycurl.CAPATH)) @@ -247,10 +251,11 @@ class TestExtendedConfig(unittest.TestCase): mycert = "/tmp/some/UNUSED/cert/file.pem" cfgfn = client.GenericCurlConfig(cafile=mycert) - curl = FakeCurl(RapiMock()) + curl_factory = lambda: FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, - curl=curl) + curl_factory=curl_factory) + curl = cl._CreateCurl() self.assert_(curl.getopt(pycurl.SSL_VERIFYPEER)) self.assertEqual(curl.getopt(pycurl.CAINFO), mycert) self.assertFalse(curl.getopt(pycurl.CAPATH)) @@ -261,10 +266,11 @@ class TestExtendedConfig(unittest.TestCase): cfgfn = client.GenericCurlConfig(capath=certdir, _pycurl_version_fn=pcverfn) - curl = FakeCurl(RapiMock()) + curl_factory = lambda: FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, - curl=curl) + curl_factory=curl_factory) + curl = cl._CreateCurl() self.assert_(curl.getopt(pycurl.SSL_VERIFYPEER)) self.assertEqual(curl.getopt(pycurl.CAPATH), certdir) self.assertFalse(curl.getopt(pycurl.CAINFO)) @@ -275,9 +281,11 @@ class TestExtendedConfig(unittest.TestCase): cfgfn = client.GenericCurlConfig(capath=certdir, _pycurl_version_fn=pcverfn) - curl = FakeCurl(RapiMock()) - self.assertRaises(client.Error, client.GanetiRapiClient, - "master.example.com", curl_config_fn=cfgfn, curl=curl) + curl_factory = lambda: FakeCurl(RapiMock()) + cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, + curl_factory=curl_factory) + + self.assertRaises(client.Error, cl._CreateCurl) def testCertVerifyNoSsl(self): certdir = "/tmp/some/UNUSED/cert/directory" @@ -285,9 +293,11 @@ class TestExtendedConfig(unittest.TestCase): cfgfn = client.GenericCurlConfig(capath=certdir, _pycurl_version_fn=pcverfn) - curl = FakeCurl(RapiMock()) - self.assertRaises(client.Error, client.GanetiRapiClient, - "master.example.com", curl_config_fn=cfgfn, curl=curl) + curl_factory = lambda: FakeCurl(RapiMock()) + cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, + curl_factory=curl_factory) + + self.assertRaises(client.Error, cl._CreateCurl) def testCertVerifyFancySsl(self): certdir = "/tmp/some/UNUSED/cert/directory" @@ -295,9 +305,11 @@ class TestExtendedConfig(unittest.TestCase): cfgfn = client.GenericCurlConfig(capath=certdir, _pycurl_version_fn=pcverfn) - curl = FakeCurl(RapiMock()) - self.assertRaises(NotImplementedError, client.GanetiRapiClient, - "master.example.com", curl_config_fn=cfgfn, curl=curl) + curl_factory = lambda: FakeCurl(RapiMock()) + cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, + curl_factory=curl_factory) + + self.assertRaises(NotImplementedError, cl._CreateCurl) def testCertVerifyCapath(self): for connect_timeout in [None, 1, 5, 10, 30, 60, 300]: @@ -305,10 +317,11 @@ class TestExtendedConfig(unittest.TestCase): cfgfn = client.GenericCurlConfig(connect_timeout=connect_timeout, timeout=timeout) - curl = FakeCurl(RapiMock()) + curl_factory = lambda: FakeCurl(RapiMock()) cl = client.GanetiRapiClient("master.example.com", curl_config_fn=cfgfn, - curl=curl) + curl_factory=curl_factory) + curl = cl._CreateCurl() self.assertEqual(curl.getopt(pycurl.CONNECTTIMEOUT), connect_timeout) self.assertEqual(curl.getopt(pycurl.TIMEOUT), timeout) @@ -320,18 +333,7 @@ class GanetiRapiClientTests(testutils.GanetiTestCase): self.rapi = RapiMock() self.curl = FakeCurl(self.rapi) self.client = client.GanetiRapiClient("master.example.com", - curl=self.curl) - - # Signals should be disabled by default - self.assert_(self.curl.getopt(pycurl.NOSIGNAL)) - - # No auth and no proxy - self.assertFalse(self.curl.getopt(pycurl.USERPWD)) - self.assert_(self.curl.getopt(pycurl.PROXY) is None) - - # Content-type is required for requests - headers = self.curl.getopt(pycurl.HTTPHEADER) - self.assert_("Content-type: application/json" in headers) + curl_factory=lambda: self.curl) def assertHandler(self, handler_cls): self.failUnless(isinstance(self.rapi.GetLastHandler(), handler_cls)) @@ -372,6 +374,22 @@ class GanetiRapiClientTests(testutils.GanetiTestCase): for i in [[1, 2, 3], {"moo": "boo"}, (1, 2, 3)]: self.assertRaises(ValueError, self.client._EncodeQuery, [("x", i)]) + def testCurlSettings(self): + self.rapi.AddResponse("2") + self.assertEqual(2, self.client.GetVersion()) + self.assertHandler(rlib2.R_version) + + # Signals should be disabled by default + self.assert_(self.curl.getopt(pycurl.NOSIGNAL)) + + # No auth and no proxy + self.assertFalse(self.curl.getopt(pycurl.USERPWD)) + self.assert_(self.curl.getopt(pycurl.PROXY) is None) + + # Content-type is required for requests + headers = self.curl.getopt(pycurl.HTTPHEADER) + self.assert_("Content-type: application/json" in headers) + def testHttpError(self): self.rapi.AddResponse(None, code=404) try: @@ -382,7 +400,6 @@ class GanetiRapiClientTests(testutils.GanetiTestCase): self.fail("Didn't raise exception") def testGetVersion(self): - self.client._version = None self.rapi.AddResponse("2") self.assertEqual(2, self.client.GetVersion()) self.assertHandler(rlib2.R_version) -- 1.7.10.4