url_helper: read_file_or_url should pass headers param into readurl (#66)

Headers param was accidentally omitted and no longer passed through to
readurl due to a previous commit.

To avoid this omission of params in the future, drop positional param
definitions from read_file_or_url and pass all kwargs through to readurl
when we are not operating on a file.

In util:read_seeded, correct the case where invalid positional param
file_retries was being passed into read_file_or_url.

Also drop duplicated file:// prefix addition from read_seeded because
read_file_or_url does that work anyway.

LP: #1854084

5 files changed, 100 insertions, 22 deletions

diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py
index f1fba175..f5cdb3fd 100755
--- a/cloudinit/sources/helpers/azure.py
+++ b/cloudinit/sources/helpers/azure.py
@@ -183,14 +183,16 @@ class AzureEndpointHttpClient(object):
         if secure:
             headers = self.headers.copy()
             headers.update(self.extra_secure_headers)
-        return url_helper.read_file_or_url(url, headers=headers)
+        return url_helper.read_file_or_url(url, headers=headers, timeout=5,
+                                           retries=10)
 
     def post(self, url, data=None, extra_headers=None):
         headers = self.headers
         if extra_headers is not None:
             headers = self.headers.copy()
             headers.update(extra_headers)
-        return url_helper.read_file_or_url(url, data=data, headers=headers)
+        return url_helper.read_file_or_url(url, data=data, headers=headers,
+                                           timeout=5, retries=10)
 
 
 class GoalState(object):
diff --git a/cloudinit/tests/test_url_helper.py b/cloudinit/tests/test_url_helper.py
index aa9f3ec1..e883ddc2 100644
--- a/cloudinit/tests/test_url_helper.py
+++ b/cloudinit/tests/test_url_helper.py
@@ -4,6 +4,7 @@ from cloudinit.url_helper import (
     NOT_FOUND, UrlError, oauth_headers, read_file_or_url, retry_on_url_exc)
 from cloudinit.tests.helpers import CiTestCase, mock, skipIf
 from cloudinit import util
+from cloudinit import version
 
 import httpretty
 import requests
@@ -17,6 +18,9 @@ except ImportError:
     _missing_oauthlib_dep = True
 
 
+M_PATH = 'cloudinit.url_helper.'
+
+
 class TestOAuthHeaders(CiTestCase):
 
     def test_oauth_headers_raises_not_implemented_when_oathlib_missing(self):
@@ -67,6 +71,54 @@ class TestReadFileOrUrl(CiTestCase):
         self.assertEqual(result.contents, data)
         self.assertEqual(str(result), data.decode('utf-8'))
 
+    @mock.patch(M_PATH + 'readurl')
+    def test_read_file_or_url_passes_params_to_readurl(self, m_readurl):
+        """read_file_or_url passes all params through to readurl."""
+        url = 'http://hostname/path'
+        response = 'This is my url content\n'
+        m_readurl.return_value = response
+        params = {'url': url, 'timeout': 1, 'retries': 2,
+                  'headers': {'somehdr': 'val'},
+                  'data': 'data', 'sec_between': 1,
+                  'ssl_details': {'cert_file': '/path/cert.pem'},
+                  'headers_cb': 'headers_cb', 'exception_cb': 'exception_cb'}
+        self.assertEqual(response, read_file_or_url(**params))
+        params.pop('url')  # url is passed in as a positional arg
+        self.assertEqual([mock.call(url, **params)], m_readurl.call_args_list)
+
+    def test_wb_read_url_defaults_honored_by_read_file_or_url_callers(self):
+        """Readurl param defaults used when unspecified by read_file_or_url
+
+        Param defaults tested are as follows:
+            retries: 0, additional headers None beyond default, method: GET,
+            data: None, check_status: True and allow_redirects: True
+        """
+        url = 'http://hostname/path'
+
+        m_response = mock.MagicMock()
+
+        class FakeSession(requests.Session):
+            def request(cls, **kwargs):
+                self.assertEqual(
+                    {'url': url, 'allow_redirects': True, 'method': 'GET',
+                     'headers': {
+                         'User-Agent': 'Cloud-Init/%s' % (
+                             version.version_string())}},
+                    kwargs)
+                return m_response
+
+        with mock.patch(M_PATH + 'requests.Session') as m_session:
+            error = requests.exceptions.HTTPError('broke')
+            m_session.side_effect = [error, FakeSession()]
+            # assert no retries and check_status == True
+            with self.assertRaises(UrlError) as context_manager:
+                response = read_file_or_url(url)
+            self.assertEqual('broke', str(context_manager.exception))
+            # assert default headers, method, url and allow_redirects True
+            # Success on 2nd call with FakeSession
+            response = read_file_or_url(url)
+        self.assertEqual(m_response, response._response)
+
 
 class TestRetryOnUrlExc(CiTestCase):
 
diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py
index 48ddae45..1496a471 100644
--- a/cloudinit/url_helper.py
+++ b/cloudinit/url_helper.py
@@ -81,14 +81,19 @@ def combine_url(base, *add_ons):
     return url
 
 
-def read_file_or_url(url, timeout=5, retries=10,
-                     headers=None, data=None, sec_between=1, ssl_details=None,
-                     headers_cb=None, exception_cb=None):
+def read_file_or_url(url, **kwargs):
+    """Wrapper function around readurl to allow passing a file path as url.
+
+    When url is not a local file path, passthrough any kwargs to readurl.
+
+    In the case of parameter passthrough to readurl, default values for some
+    parameters. See: call-signature of readurl in this module for param docs.
+    """
     url = url.lstrip()
     if url.startswith("/"):
         url = "file://%s" % url
     if url.lower().startswith("file://"):
-        if data:
+        if kwargs.get("data"):
             LOG.warning("Unable to post data to file resource %s", url)
         file_path = url[len("file://"):]
         try:
@@ -101,10 +106,7 @@ def read_file_or_url(url, timeout=5, retries=10,
             raise UrlError(cause=e, code=code, headers=None, url=url)
         return FileResponse(file_path, contents=contents)
     else:
-        return readurl(url, timeout=timeout, retries=retries,
-                       headers_cb=headers_cb, data=data,
-                       sec_between=sec_between, ssl_details=ssl_details,
-                       exception_cb=exception_cb)
+        return readurl(url, **kwargs)
 
 
 # Made to have same accessors as UrlResponse so that the
@@ -201,6 +203,35 @@ def readurl(url, data=None, timeout=None, retries=0, sec_between=1,
             check_status=True, allow_redirects=True, exception_cb=None,
             session=None, infinite=False, log_req_resp=True,
             request_method=None):
+    """Wrapper around requests.Session to read the url and retry if necessary
+
+    :param url: Mandatory url to request.
+    :param data: Optional form data to post the URL. Will set request_method
+        to 'POST' if present.
+    :param timeout: Timeout in seconds to wait for a response
+    :param retries: Number of times to retry on exception if exception_cb is
+        None or exception_cb returns True for the exception caught. Default is
+        to fail with 0 retries on exception.
+    :param sec_between: Default 1: amount of seconds passed to time.sleep
+        between retries. None or -1 means don't sleep.
+    :param headers: Optional dict of headers to send during request
+    :param headers_cb: Optional callable returning a dict of values to send as
+        headers during request
+    :param ssl_details: Optional dict providing key_file, ca_certs, and
+        cert_file keys for use on in ssl connections.
+    :param check_status: Optional boolean set True to raise when HTTPError
+        occurs. Default: True.
+    :param allow_redirects: Optional boolean passed straight to Session.request
+        as 'allow_redirects'. Default: True.
+    :param exception_cb: Optional callable which accepts the params
+        msg and exception and returns a boolean True if retries are permitted.
+    :param session: Optional exiting requests.Session instance to reuse.
+    :param infinite: Bool, set True to retry indefinitely. Default: False.
+    :param log_req_resp: Set False to turn off verbose debug messages.
+    :param request_method: String passed as 'method' to Session.request.
+        Typically GET, or POST. Default: POST if data is provided, GET
+        otherwise.
+    """
     url = _cleanurl(url)
     req_args = {
         'url': url,
diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
index ed83d2d8..15af1daf 100644
--- a/cloudinit/user_data.py
+++ b/cloudinit/user_data.py
@@ -224,7 +224,7 @@ class UserDataProcessor(object):
                 content = util.load_file(include_once_fn)
             else:
                 try:
-                    resp = read_file_or_url(include_url,
+                    resp = read_file_or_url(include_url, timeout=5, retries=10,
                                             ssl_details=self.ssl_details)
                     if include_once_on and resp.ok():
                         util.write_file(include_once_fn, resp.contents,
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 78b6a2d0..9d9d5c72 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -986,13 +986,6 @@ def load_yaml(blob, default=None, allowed=(dict,)):
 
 
 def read_seeded(base="", ext="", timeout=5, retries=10, file_retries=0):
-    if base.startswith("/"):
-        base = "file://%s" % base
-
-    # default retries for file is 0. for network is 10
-    if base.startswith("file://"):
-        retries = file_retries
-
     if base.find("%s") >= 0:
         ud_url = base % ("user-data" + ext)
         md_url = base % ("meta-data" + ext)
@@ -1000,14 +993,14 @@ def read_seeded(base="", ext="", timeout=5, retries=10, file_retries=0):
         ud_url = "%s%s%s" % (base, "user-data", ext)
         md_url = "%s%s%s" % (base, "meta-data", ext)
 
-    md_resp = url_helper.read_file_or_url(md_url, timeout, retries,
-                                          file_retries)
+    md_resp = url_helper.read_file_or_url(md_url, timeout=timeout,
+                                          retries=retries)
     md = None
     if md_resp.ok():
         md = load_yaml(decode_binary(md_resp.contents), default={})
 
-    ud_resp = url_helper.read_file_or_url(ud_url, timeout, retries,
-                                          file_retries)
+    ud_resp = url_helper.read_file_or_url(ud_url, timeout=timeout,
+                                          retries=retries)
     ud = None
     if ud_resp.ok():
         ud = ud_resp.contents