Provide a better default user-agent string

This change provides a default user-agent with more information and
adjusts the handling of user-agent arguments to fall in line with RFC
7231 Section 5.5.3. We now specify the package name and version for the
SDK as well as the Python implementation and version as the default.

From there, a user can specify their application name and version but it
does not replace the overall user-agent. As the RFC states,
product/version pairs are listed in decreasing order, so anything the
user specifies is prepended to whatever string is currently set for that
Transport instance.

If headers are passed in to a Transport.request method which carry a
`User-Agent`, they are used as-is unless the caller also specified a
`user_agent` parameter.

This change simplifies the tests slightly without reducing any coverage.

Change-Id: I80ca26fff3f2522b8232472676396abb86166f91

doc/source/users/transport.rst

@@ -1,6 +1,7 @@
 Transport
 =========
 .. automodule:: openstack.transport
+   :members:
 
 Transport Object
 ----------------

doc/source/users/userguides/usage.rst

@@ -1,4 +1,5 @@
 .. TODO(briancurtin): turn this into a full guide on the Connection class
+.. TODO(briancurtin): cover user_agent setting
 
 =====
 Usage

openstack/connection.py

@@ -64,16 +64,13 @@ from openstack import module_loader
 from openstack import session
 from openstack import transport as xport
 
-
-USER_AGENT = 'OSPythonSDK'
-"""Default value for the HTTP User-Agent header"""
 _logger = logging.getLogger(__name__)
 
 
 class Connection(object):
 
     def __init__(self, transport=None, authenticator=None, preference=None,
-                 verify=True, user_agent=USER_AGENT,
+                 verify=True, user_agent=None,
                  auth_plugin=None, **auth_args):
         """Create a context for a connection to a cloud provider.
 
@@ -106,10 +103,11 @@ class Connection(object):
             is set to true, which is the default, the SSL cert will be
             verified.  It can also be set to a CA_BUNDLE path.
         :param str user_agent: If a transport is not provided to the
-            connection, this parameter will be used to create a transport.
+            connection, this parameter will be used when creating a transport.
-            The value of this parameter is used for the ``User-Agent`` HTTP
+            The value given here will be prepended to the default, which is
-            header. The default value is the module level attribute
+            specified in :attr:`~openstack.transport.USER_AGENT`.
-            ``USER_AGENT`` which is set to ``"OSPythonSDK"``.
+            The resulting ``user_agent`` value is used for the ``User-Agent``
+            HTTP header.
         :param str auth_plugin: The name of authentication plugin to use.  If
             the authentication plugin name is not provided, the connection will
             try to guess what plugin to use based on the *auth_url* in the

openstack/tests/test_connection.py

@@ -36,7 +36,7 @@ class TestConnection(base.TestCase):
         conn = connection.Connection(authenticator='2', verify=True,
                                      user_agent='1')
         self.assertTrue(conn.transport.verify)
-        self.assertEqual('1', conn.transport._user_agent)
+        self.assertIn('1', conn.transport._user_agent)
 
     def test_create_authenticator_v2(self):
         auth_args = {
@@ -121,6 +121,12 @@ class TestConnection(base.TestCase):
         self.assertEqual('openstack.telemetry.v2._proxy',
                          conn.telemetry.__class__.__module__)
 
+    def test_custom_user_agent(self):
+        user_agent = "MyProgram/1.0"
+        conn = connection.Connection(authenticator=self.auth,
+                                     user_agent=user_agent)
+        self.assertTrue(conn.transport._user_agent.startswith(user_agent))
+
 
 class TestService(service_filter.ServiceFilter):
     valid_versions = [service_filter.ValidVersion('v2')]

openstack/tests/test_transport.py

@@ -44,6 +44,17 @@ fake_record2 = {
 
 class TestTransport(base.TestTransportBase):
 
+    def setUp(self):
+        super(TestTransport, self).setUp()
+
+        self._orig_user_agent = transport.USER_AGENT
+        self.test_user_agent = transport.USER_AGENT = "testing/1.0"
+
+    def tearDown(self):
+        super(TestTransport, self).tearDown()
+
+        transport.USER_AGENT = self._orig_user_agent
+
     @httpretty.activate
     def test_request(self):
         self.stub_url(httpretty.GET, body=fake_response)
@@ -169,220 +180,76 @@ class TestTransport(base.TestTransportBase):
 
         resp = xport.get(self.TEST_URL, accept=None)
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual(
+        self.assertRequestHeaderEqual('User-Agent', self.test_user_agent)
-            'User-Agent',
-            transport.DEFAULT_USER_AGENT,
-        )
 
-        resp = xport.get(
+        resp = xport.get(self.TEST_URL, headers={'User-Agent': None},
-            self.TEST_URL,
+                         accept=None)
-            headers={'User-Agent': None},
-            accept=None,
-        )
         self.assertTrue(resp.ok)
         self.assertRequestHeaderEqual('User-Agent', None)
 
         resp = xport.get(self.TEST_URL, user_agent=None, accept=None)
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
+        self.assertRequestHeaderEqual('User-Agent', self.test_user_agent)
 
-        resp = xport.get(
+        resp = xport.get(self.TEST_URL, headers={'User-Agent': ''},
-            self.TEST_URL,
+                         accept=None)
-            headers={'User-Agent': ''},
-            accept=None,
-        )
         self.assertTrue(resp.ok)
         self.assertRequestHeaderEqual('User-Agent', '')
 
         resp = xport.get(self.TEST_URL, user_agent='', accept=None)
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', '')
+        self.assertRequestHeaderEqual('User-Agent', self.test_user_agent)
 
-        resp = xport.get(
+        new_agent = 'new-agent'
-            self.TEST_URL,
+        resp = xport.get(self.TEST_URL, headers={'User-Agent': new_agent},
-            headers={'User-Agent': 'new-agent'},
+                         accept=None)
-            accept=None,
-        )
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'new-agent')
+        self.assertRequestHeaderEqual('User-Agent', new_agent)
 
-        resp = xport.get(self.TEST_URL, user_agent='new-agent', accept=None)
+        resp = xport.get(self.TEST_URL, user_agent=new_agent, accept=None)
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'new-agent')
+        self.assertRequestHeaderEqual('User-Agent', '%s %s' % (
+                                      new_agent, self.test_user_agent))
 
-        resp = xport.get(
+        custom_value = 'new-agent'
-            self.TEST_URL,
+        resp = xport.get(self.TEST_URL, headers={'User-Agent': custom_value},
-            headers={'User-Agent': 'new-agent'},
+                         user_agent=None, accept=None)
-            user_agent=None,
-            accept=None,
-        )
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
+        self.assertRequestHeaderEqual('User-Agent', custom_value)
 
-        resp = xport.get(
+        override = 'overrides-agent'
-            self.TEST_URL,
+        resp = xport.get(self.TEST_URL, headers={'User-Agent': None},
-            headers={'User-Agent': None},
+                         user_agent=override, accept=None)
-            user_agent='overrides-agent',
-            accept=None,
-        )
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'overrides-agent')
+        self.assertRequestHeaderEqual('User-Agent', '%s %s' % (
+                                      override, self.test_user_agent))
 
-        resp = xport.get(
+        resp = xport.get(self.TEST_URL, headers={'User-Agent': custom_value},
-            self.TEST_URL,
+                         user_agent=override, accept=None)
-            headers={'User-Agent': 'new-agent'},
-            user_agent='overrides-agent',
-            accept=None,
-        )
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'overrides-agent')
+        self.assertRequestHeaderEqual('User-Agent', '%s %s' % (
+                                      override, self.test_user_agent))
 
     @httpretty.activate
     def test_user_agent_arg_none(self):
+        # None gets converted to the transport.USER_AGENT by default.
         self.stub_url(httpretty.GET, body=fake_response)
         xport = transport.Transport(user_agent=None)
 
         resp = xport.get(self.TEST_URL, accept=None)
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual(
+        self.assertRequestHeaderEqual('User-Agent', self.test_user_agent)
-            'User-Agent',
-            transport.DEFAULT_USER_AGENT,
-        )
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': None},
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
-
-        resp = xport.get(self.TEST_URL, user_agent=None, accept=None)
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': ''},
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', '')
-
-        resp = xport.get(self.TEST_URL, user_agent='', accept=None)
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', '')
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': 'new-agent'},
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'new-agent')
-
-        resp = xport.get(self.TEST_URL, user_agent='new-agent', accept=None)
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'new-agent')
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': 'new-agent'},
-            user_agent=None,
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': None},
-            user_agent='overrides-agent',
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'overrides-agent')
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': 'new-agent'},
-            user_agent='overrides-agent',
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'overrides-agent')
 
     @httpretty.activate
     def test_user_agent_arg_default(self):
         self.stub_url(httpretty.GET, body=fake_response)
-        xport = transport.Transport(user_agent='test-agent')
+        agent = 'test-agent'
+        xport = transport.Transport(user_agent=agent)
 
         resp = xport.get(self.TEST_URL, accept=None)
         self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'test-agent')
+        self.assertRequestHeaderEqual('User-Agent', '%s %s' % (
-
+                                      agent, self.test_user_agent))
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': None},
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
-
-        resp = xport.get(self.TEST_URL, user_agent=None, accept=None)
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': ''},
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', '')
-
-        resp = xport.get(self.TEST_URL, user_agent='', accept=None)
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', '')
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': 'new-agent'},
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'new-agent')
-
-        resp = xport.get(self.TEST_URL, user_agent='new-agent', accept=None)
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'new-agent')
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': 'new-agent'},
-            user_agent=None,
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', None)
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': None},
-            user_agent='overrides-agent',
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'overrides-agent')
-
-        resp = xport.get(
-            self.TEST_URL,
-            headers={'User-Agent': 'new-agent'},
-            user_agent='overrides-agent',
-            accept=None,
-        )
-        self.assertTrue(resp.ok)
-        self.assertRequestHeaderEqual('User-Agent', 'overrides-agent')
 
     def test_verify_no_arg(self):
         xport = transport.Transport()

openstack/transport.py

@@ -92,18 +92,28 @@ for an API.
 
 See: https://en.wikipedia.org/wiki/Post/Redirect/Get
 
-User Agent
+User-Agent
 ~~~~~~~~~~
 
-The ``User-Agent`` header may be set when the Transport object is created in
+User-Agent handling as constructed by this class follows
-addition to the existing per-request mode.  The determination of how to set
+`RFC 7231 Section 5.5.3 <http://tools.ietf.org/html/rfc7231#section-5.5.3>`_.
-the ``User-Agent`` header is as follows:
+A well-formed user-agent is constructed on name/version product identifiers,
+such that ``MyProgram/1.0`` is a proper user-agent.
+
+* The default :attr:`~openstack.transport.USER_AGENT` contains
+  the SDK version as well as RFC-compliant values from
+  ``requests.utils.default_user_agent``, including versions of ``requests``,
+  Python, and the operating system.
+* Any ``user_agent`` argument passed when creating a
+  :class:`~openstack.transport.Transport` is prepended to the default.
+* Any ``user_agent`` passed in a
+  :meth:`~openstack.transport.Transport.request` call is prepended
+  to one used for that ``Transport`` instance.
+* Any string passed as the ``User-Agent`` in a dictionary of
+  headers to :meth:`~openstack.transport.Transport.request` will be
+  used directly. If at the same time a ``user_agent`` argument has been passed
+  to ``request()``, it will be used and follows the rules above.
 
-* If the ``user_agent`` argument is included in the ``request()`` call use it
-* Else if ``User-Agent`` is set in the headers dict use it
-* Else if ``user_agent`` argument is included in the
-  :class:`~openstack.transport.Transport` construction use it
-* Else use ``transport.DEFAULT_USER_AGENT``
 """
 
 import json
@@ -116,8 +126,11 @@ from six.moves import urllib
 import openstack
 from openstack import exceptions
 
-
+#: Default value for the HTTP User-Agent header. The default includes the
-DEFAULT_USER_AGENT = 'python-OpenStackSDK/' + openstack.__version__
+#: version information of the SDK as well as ``requests``, Python,
+#: and the operating system.
+USER_AGENT = "python-openstacksdk/%s %s" % (
+    openstack.__version__, requests.utils.default_user_agent())
 
 _logger = logging.getLogger(__name__)
 JSON = 'application/json'
@@ -125,8 +138,6 @@ JSON = 'application/json'
 
 class Transport(requests.Session):
 
-    _user_agent = DEFAULT_USER_AGENT
-
     REDIRECT_STATUSES = (301, 302, 303, 305, 307)
     DEFAULT_REDIRECT_LIMIT = 30
 
@@ -142,9 +153,12 @@ class Transport(requests.Session):
         In addition to those listed below, all arguments available to
         ``requests.Session`` are available here:
 
-        :param string user_agent: Set the default ``User-Agent`` header;
+        :param string user_agent: Set the ``User-Agent`` header. When
-                                  Header is omitted if ``None`` and no value
+                                  no value is provided, the default of
-                                  is supplied in the ``request()`` call.
+                                  :attr:`~openstack.transport.USER_AGENT`
+                                  will be used. When a value is provided,
+                                  it will be prepended to
+                                  :attr:`~openstack.transport.USER_AGENT`.
         :param boolean/string verify: If ``True``, the SSL cert will be
                                       verified. A CA_BUNDLE path can also be
                                       provided.
@@ -155,18 +169,18 @@ class Transport(requests.Session):
                                          if True. (optional)
         :param string accept: Type of output to accept
 
-        User agent handling is as follows:
-
-        * if user_agent arg is included in the request() call, use it
-        * else if 'User-Agent' is set in the headers dict, use it
-        * else if user_agent arg is included in the __init__() call, use it
-        * else use DEFAULT_USER_AGENT
-
         """
 
         super(Transport, self).__init__()
-        if user_agent:
+
-            self._user_agent = user_agent
+        # Per RFC 7231 Section 5.5.3, identifiers in a user-agent should
+        # be ordered by decreasing significance. If a user sets their product,
+        # we prepend it to the SDK version and then the Python version.
+        if user_agent is None:
+            self._user_agent = USER_AGENT
+        else:
+            self._user_agent = "%s %s" % (user_agent, USER_AGENT)
+
         self.verify = verify
         self._redirect = redirect
         self._accept = accept
@@ -192,9 +206,8 @@ class Transport(requests.Session):
         :param string accept: Set the ``Accept`` header; overwrites
                                   any value that may be in the headers dict.
                                   Header is omitted if ``None``.
-        :param string user_agent: Set the ``User-Agent`` header; overwrites
+        :param string user_agent: Prepend an additional value to the existing
-                                  any value that may be in the headers dict.
+                                  ``User-Agent`` header.
-                                  Header is omitted if ``None``.
 
         Remaining kw args from requests.Session.request() supported
 
@@ -209,14 +222,19 @@ class Transport(requests.Session):
             kwargs['data'] = json.dumps(json_data)
             headers['Content-Type'] = JSON
 
-        # Set User-Agent header if user_agent arg included, or
+        # Prepend the caller's user_agent to User-Agent header if included,
-        # fall through the default chain as described above
+        # or use the default that this transport was created with.
-        if 'user_agent' in kwargs:
+        # Note: Only attempt to work with strings and avoid needlessly
-            headers['User-Agent'] = kwargs.pop('user_agent')
+        # concatenating an empty string.
-        elif self._user_agent:
+        user_agent = kwargs.pop('user_agent', None)
-            headers.setdefault('User-Agent', self._user_agent)
+        if isinstance(user_agent, six.string_types) and user_agent != '':
+            headers['User-Agent'] = '%s %s' % (user_agent, self._user_agent)
+        elif 'User-Agent' in headers:
+            # If they've specified their own headers with a User-Agent,
+            # use that directly.
+            pass
         else:
-            headers.setdefault('User-Agent', DEFAULT_USER_AGENT)
+            headers.setdefault('User-Agent', self._user_agent)
 
         if redirect is None:
             redirect = self._redirect