Merge into trunk : os_token_string_type

Status:

Merged

Approved by:

Martin Packman on 2012-10-03

Approved revision:

583

Merged at revision:

586

Proposed branch:

lp:~gz/pyjuju/os_token_string_type_1030897

Merge into:

lp:pyjuju

Diff against target:

0 lines

To merge this branch:

bzr merge lp:~gz/pyjuju/os_token_string_type_1030897

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Clint Byrum (community)		2012-09-28	Approve on 2012-09-29
Review via email: mp+127008@code.launchpad.net

Description of the change

Fix TypeError with unicode tokens from keystone

One line change to make tokens from v2 keystone auth use the str type
rather than unicode. In mysterious circumstances, that breaks when
twisted tries to construct the request.

Also adds some testing to cover this case and enforce the type.

https://codereview.appspot.com/6573068/

Revision history for this message

Martin Packman (gz) wrote on 2012-09-28:

#

Download full text (6.2 KiB)

Reviewers: mp+127008_code.launchpad.net,

Message:
Please take a look.

Description:
Fix TypeError with unicode tokens from keystone

One line change to make tokens from v2 keystone auth use the str type
rather than unicode. In mysterious circumstances, that breaks when
twisted tries to construct the request.

Also adds some testing to cover this case and enforce the type.

https://code.launchpad.net/~gz/juju/os_token_string_type_1030897/+merge/127008

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6573068/

Affected files:
   A [revision details]
   M juju/providers/openstack/client.py
   M juju/providers/openstack/tests/test_client.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/providers/openstack/client.py
=== modified file 'juju/providers/openstack/client.py'
--- juju/providers/openstack/client.py 2012-09-26 04:56:39 +0000
+++ juju/providers/openstack/client.py 2012-09-28 15:53:06 +0000
@@ -33,9 +33,9 @@
  from twisted.web.client import Agent
  # Older twisted versions don't expose _newclient exceptions via client
module
  try:
- from twisted.web.client import ResponseFailed
+ from twisted.web.client import ResponseDone, ResponseFailed
  except ImportError:
- from twisted.web._newclient import ResponseFailed
+ from twisted.web._newclient import ResponseDone, ResponseFailed
  from twisted.web.http_headers import Headers
  from zope.interface import implements

@@ -249,7 +249,10 @@
      def _handle_v2_auth(self, result):
          access_details = self._json(result, 200, 'access')
          token_details = access_details["token"]
- self.token = token_details["id"]
+ # Decoded json uses unicode for all string values, but that can
upset
+ # twisted when serialising headers later. Really should encode at
that
+ # point, but as keystone should only give ascii tokens a cast will
do.
+ self.token = str(token_details["id"])

# TODO: care about token_details["expires"]
# Don't need to we're not preserving tokens.

Index: juju/providers/openstack/tests/test_client.py
=== modified file 'juju/providers/openstack/tests/test_client.py'
--- juju/providers/openstack/tests/test_client.py 2012-09-26 04:54:55 +0000
+++ juju/providers/openstack/tests/test_client.py 2012-09-28 15:53:06 +0000
@@ -62,10 +62,19 @@
class FakeResponse(object):
"""Bare minimum needed to look like a twisted http response"""

- def __init__(self, code, headers):
+ def __init__(self, code, headers, body=None):
self.code = code
self.headers = headers
- self.length = 0
+ if body is None:
+ self.length = 0
+ else:
+ self.length = len(body)
+ self.body = body
+
+ def deliverBody(self, reader):
+ reader.connectionMade()
+ reader.dataReceived(self.body)
+ reader.connectionL...

Reviewers: mp+127008_code.launchpad.net,

Message:
Please take a look.

Description:
Fix TypeError with unicode tokens from keystone

One line change to make tokens from v2 keystone auth use the str type
rather than unicode. In mysterious circumstances, that breaks when
twisted tries to construct the request.

Also adds some testing to cover this case and enforce the type.

https://code.launchpad.net/~gz/juju/os_token_string_type_1030897/+merge/127008

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6573068/

Affected files:
   A [revision details]
   M juju/providers/openstack/client.py
   M juju/providers/openstack/tests/test_client.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: clint@ubuntu.com-20120927145050-47lnm5jzshpfhnla
+New revision: martin.packman@canonical.com-20120928155306-5zinepns8rf4ccs8

Index: juju/providers/openstack/client.py
=== modified file 'juju/providers/openstack/client.py'
--- juju/providers/openstack/client.py	2012-09-26 04:56:39 +0000
+++ juju/providers/openstack/client.py	2012-09-28 15:53:06 +0000
@@ -33,9 +33,9 @@
  from twisted.web.client import Agent
  # Older twisted versions don't expose _newclient exceptions via client  
module
  try:
-    from twisted.web.client import ResponseFailed
+    from twisted.web.client import ResponseDone, ResponseFailed
  except ImportError:
-    from twisted.web._newclient import ResponseFailed
+    from twisted.web._newclient import ResponseDone, ResponseFailed
  from twisted.web.http_headers import Headers
  from zope.interface import implements

@@ -249,7 +249,10 @@
      def _handle_v2_auth(self, result):
          access_details = self._json(result, 200, 'access')
          token_details = access_details["token"]
-        self.token = token_details["id"]
+        # Decoded json uses unicode for all string values, but that can  
upset
+        # twisted when serialising headers later. Really should encode at  
that
+        # point, but as keystone should only give ascii tokens a cast will  
do.
+        self.token = str(token_details["id"])

# TODO: care about token_details["expires"]
          # Don't need to we're not preserving tokens.

Index: juju/providers/openstack/tests/test_client.py
=== modified file 'juju/providers/openstack/tests/test_client.py'
--- juju/providers/openstack/tests/test_client.py	2012-09-26 04:54:55 +0000
+++ juju/providers/openstack/tests/test_client.py	2012-09-28 15:53:06 +0000
@@ -62,10 +62,19 @@
  class FakeResponse(object):
      """Bare minimum needed to look like a twisted http response"""

-    def __init__(self, code, headers):
+    def __init__(self, code, headers, body=None):
          self.code = code
          self.headers = headers
-        self.length = 0
+        if body is None:
+            self.length = 0
+        else:
+            self.length = len(body)
+        self.body = body
+
+    def deliverBody(self, reader):
+        reader.connectionMade()
+        reader.dataReceived(self.body)
+        reader.connectionLost(client.ResponseDone())

class ClientTests(testing.TestCase):
@@ -106,6 +115,18 @@
          self.mocker.result(self.mock_agent)
          return config, osc

+    def make_client_userpass(self):
+        config = {
+            "auth-url": "https://testing.invalid/v2.0/",
+            "username": "user",
+            "password": "pass",
+            "project-name": "project",
+            }
+        osc = client._OpenStackClient(self.get_credentials(config), True)
+        self.mock_agent(reactor,  
contextFactory=mocker.MATCH(self.is_checker))
+        self.mocker.result(self.mock_agent)
+        return config, osc
+
      @defer.inlineCallbacks
      def test_auth_legacy(self):
          config, osc = self.make_client_legacy()
@@ -119,10 +140,49 @@
          self.mocker.replay()
          log = self.capture_logging()
          yield osc.authenticate()
-        self.assertEqual("tok", osc.token)
-        self.assertEqual("http://testing.invalid/compute/path",
-            osc._make_url("compute", ["path"]))
-        self.assertIn("compute service not using secure", log.getvalue())
+        self.assertIsInstance(osc.token, str)
+        self.assertEqual("tok", osc.token)
+        self.assertEqual("http://testing.invalid/compute/path",
+            osc._make_url("compute", ["path"]))
+        self.assertIn("compute service not using secure", log.getvalue())
+
+    @defer.inlineCallbacks
+    def test_auth_userpass(self):
+        config, osc = self.make_client_userpass()
+         
self.mock_agent.request("POST", "https://testing.invalid/v2.0/tokens",
+            mocker.ANY, mocker.ANY)
+        response = FakeResponse(200, http_headers.Headers({
+            "Content-Type": ["application/json"],
+            }),
+            json.dumps({'access': {
+                'token': {'id': "tok", 'expires': "shortly"},
+                'serviceCatalog': [
+                    {
+                        'type': "compute",
+                        'endpoints': [
+                             
{'publicURL': "http://testing.invalid/compute"},
+                        ],
+                    },
+                    {
+                        'type': "object-store",
+                        'endpoints': [
+                             
{'publicURL': "http://testing.invalid/objstore"},
+                        ],
+                    },
+                ],
+            }}))
+        self.mocker.result(defer.succeed(response))
+        self.mocker.replay()
+        log = self.capture_logging()
+        yield osc.authenticate()
+        self.assertIsInstance(osc.token, str)
+        self.assertEqual("tok", osc.token)
+        self.assertEqual("http://testing.invalid/compute/path",
+            osc._make_url("compute", ["path"]))
+        self.assertEqual("http://testing.invalid/objstore/path",
+            osc._make_url("object-store", ["path"]))
+        self.assertIn("compute service not using secure", log.getvalue())
+        self.assertIn("object-store service not using secure",  
log.getvalue())

def test_cert_failure(self):
          config, osc = self.make_client_legacy()

Revision history for this message

Clint Byrum (clint-fewbar) wrote on 2012-09-29:

#

LGTM, Simple change, nice covering test case. Would be great if we cited the openstack API docs where the token content is defined.

review: Approve

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2012-10-01:

#

https://codereview.appspot.com/6573068/diff/1/juju/providers/openstack/client.py
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6573068/diff/1/juju/providers/openstack/client.py#newcode255
juju/providers/openstack/client.py:255: self.token =
str(token_details["id"])
pls explictly encode utf8. str('some unicode char') is environment
dependent.

https://codereview.appspot.com/6573068/

Revision history for this message

Martin Packman (gz) wrote on 2012-10-01:

#

On 2012/10/01 14:42:48, hazmat wrote:

> pls explictly encode utf8. str('some unicode char') is environment
dependent.

So, this is the messy detail I was trying to avoid, but a summary:

Behaviour of str()
==================

Assuming a unicode object (otherwise __str__ is called), this is
basically equivalent to:

obj.encode(sys.getdefaultencoding())

This is not locale dependent, and in modern Pythons the knob for
changing the default encoding is hard to access, in the context of the
juju client we are basically assured equivalence to:

obj.encode('ascii')

Which we could do instead, as the str() spelling is generally a bit of a
red flag as people like using it to try papering over actual encoding
problems.

HTTP header encoding
====================

Header field content is specified in HTTP 1.1 as being either ISO-8859-1
or encoded as per MIME headers:

<http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2>
<http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2>
<http://www.ietf.org/rfc/rfc2047.txt>

This is not a very common or easy to use encoding scheme, and in
practice, no one really does it. The in progress revision of the
specification is changing the requirements around this:

<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/74>

So, keystone should never give a non-ascii token but for documentation
purposes encoding as utf-8 is confusing given that's not the correct
scheme in this context.

https://codereview.appspot.com/6573068/

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2012-10-03:

#

On 2012/10/01 17:20:44, gz wrote:
> On 2012/10/01 14:42:48, hazmat wrote:
> >
> > pls explictly encode utf8. str('some unicode char') is environment
dependent.

> So, this is the messy detail I was trying to avoid, but a summary:

> Behaviour of str()
> ==================

> Assuming a unicode object (otherwise __str__ is called), this is
basically
> equivalent to:

> obj.encode(sys.getdefaultencoding())

> This is not locale dependent, and in modern Pythons the knob for
changing the
> default encoding is hard to access, in the context of the juju client
we are
> basically assured equivalence to:

> obj.encode('ascii')

It may be hard, but people still do it, and its global for the
interpreter. Its sadly common for py2 web apps to make this utf8 via
sitecustomize. If we want ascii and we know, it we should just be
explicit about it then relying on implicit conversion.

> Which we could do instead, as the str() spelling is generally a bit of
a red
> flag as people like using it to try papering over actual encoding
problems.

yup. sounds good.

> HTTP header encoding
> ====================

> Header field content is specified in HTTP 1.1 as being either
ISO-8859-1 or
> encoded as per MIME headers:

> <http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2>
> <http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2>
> <http://www.ietf.org/rfc/rfc2047.txt>

> This is not a very common or easy to use encoding scheme, and in
practice, no
> one really does it. The in progress revision of the specification is
changing
> the requirements around this:

> <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/74>

> So, keystone should never give a non-ascii token but for documentation
purposes
> encoding as utf-8 is confusing given that's not the correct scheme in
this
> context.

fair enough.

https://codereview.appspot.com/6573068/

lp:~gz/pyjuju/os_token_string_type_1030897 updated on 2012-10-03

584. By Martin Packman on 2012-10-03: Explicitly encode as ascii rather than casting to str as discussed in review

pyjuju

Merge lp:~gz/pyjuju/os_token_string_type_1030897 into lp:pyjuju

Commit message

Description of the change

Preview Diff

Subscribers