faf1e5
From f9fcf18105845fbb933925ae7b0a2f1033f75127 Mon Sep 17 00:00:00 2001
faf1e5
From: Eduardo Otubo <otubo@redhat.com>
faf1e5
Date: Wed, 20 May 2020 10:11:14 +0200
faf1e5
Subject: [PATCH] url_helper: read_file_or_url should pass headers param into
faf1e5
 readurl (#66)
faf1e5
faf1e5
RH-Author: Eduardo Otubo <otubo@redhat.com>
faf1e5
Message-id: <20200519105653.20249-1-otubo@redhat.com>
faf1e5
Patchwork-id: 96613
faf1e5
O-Subject: [RHEL-7.8.z cloud-init PATCH] url_helper: read_file_or_url should pass headers param into readurl (#66)
faf1e5
Bugzilla: 1832177
faf1e5
RH-Acked-by: Cathy Avery <cavery@redhat.com>
faf1e5
RH-Acked-by: Mohammed Gamal <mgamal@redhat.com>
faf1e5
RH-Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>
faf1e5
faf1e5
commit f69d33a723b805fec3ee70c3a6127c8cadcb02d8
faf1e5
Author: Chad Smith <chad.smith@canonical.com>
faf1e5
Date:   Mon Dec 2 16:24:18 2019 -0700
faf1e5
faf1e5
    url_helper: read_file_or_url should pass headers param into readurl (#66)
faf1e5
faf1e5
    Headers param was accidentally omitted and no longer passed through to
faf1e5
    readurl due to a previous commit.
faf1e5
faf1e5
    To avoid this omission of params in the future, drop positional param
faf1e5
    definitions from read_file_or_url and pass all kwargs through to readurl
faf1e5
    when we are not operating on a file.
faf1e5
faf1e5
    In util:read_seeded, correct the case where invalid positional param
faf1e5
    file_retries was being passed into read_file_or_url.
faf1e5
faf1e5
    Also drop duplicated file:// prefix addition from read_seeded because
faf1e5
    read_file_or_url does that work anyway.
faf1e5
faf1e5
    LP: #1854084
faf1e5
faf1e5
Signed-off-by: Eduardo Otubo <otubo@redhat.com>
faf1e5
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
faf1e5
---
faf1e5
 cloudinit/sources/helpers/azure.py                 |  6 ++-
faf1e5
 cloudinit/tests/test_url_helper.py                 | 52 ++++++++++++++++++++++
faf1e5
 cloudinit/url_helper.py                            | 47 +++++++++++++++----
faf1e5
 cloudinit/user_data.py                             |  2 +-
faf1e5
 cloudinit/util.py                                  | 15 ++-----
faf1e5
 .../unittests/test_datasource/test_azure_helper.py | 18 +++++---
faf1e5
 6 files changed, 112 insertions(+), 28 deletions(-)
faf1e5
faf1e5
diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py
faf1e5
index c2a57cc..b99c484 100755
faf1e5
--- a/cloudinit/sources/helpers/azure.py
faf1e5
+++ b/cloudinit/sources/helpers/azure.py
faf1e5
@@ -103,14 +103,16 @@ class AzureEndpointHttpClient(object):
faf1e5
         if secure:
faf1e5
             headers = self.headers.copy()
faf1e5
             headers.update(self.extra_secure_headers)
faf1e5
-        return url_helper.read_file_or_url(url, headers=headers)
faf1e5
+        return url_helper.read_file_or_url(url, headers=headers, timeout=5,
faf1e5
+                                           retries=10)
faf1e5
 
faf1e5
     def post(self, url, data=None, extra_headers=None):
faf1e5
         headers = self.headers
faf1e5
         if extra_headers is not None:
faf1e5
             headers = self.headers.copy()
faf1e5
             headers.update(extra_headers)
faf1e5
-        return url_helper.read_file_or_url(url, data=data, headers=headers)
faf1e5
+        return url_helper.read_file_or_url(url, data=data, headers=headers,
faf1e5
+                                           timeout=5, retries=10)
faf1e5
 
faf1e5
 
faf1e5
 class GoalState(object):
faf1e5
diff --git a/cloudinit/tests/test_url_helper.py b/cloudinit/tests/test_url_helper.py
faf1e5
index aa9f3ec..e883ddc 100644
faf1e5
--- a/cloudinit/tests/test_url_helper.py
faf1e5
+++ b/cloudinit/tests/test_url_helper.py
faf1e5
@@ -4,6 +4,7 @@ from cloudinit.url_helper import (
faf1e5
     NOT_FOUND, UrlError, oauth_headers, read_file_or_url, retry_on_url_exc)
faf1e5
 from cloudinit.tests.helpers import CiTestCase, mock, skipIf
faf1e5
 from cloudinit import util
faf1e5
+from cloudinit import version
faf1e5
 
faf1e5
 import httpretty
faf1e5
 import requests
faf1e5
@@ -17,6 +18,9 @@ except ImportError:
faf1e5
     _missing_oauthlib_dep = True
faf1e5
 
faf1e5
 
faf1e5
+M_PATH = 'cloudinit.url_helper.'
faf1e5
+
faf1e5
+
faf1e5
 class TestOAuthHeaders(CiTestCase):
faf1e5
 
faf1e5
     def test_oauth_headers_raises_not_implemented_when_oathlib_missing(self):
faf1e5
@@ -67,6 +71,54 @@ class TestReadFileOrUrl(CiTestCase):
faf1e5
         self.assertEqual(result.contents, data)
faf1e5
         self.assertEqual(str(result), data.decode('utf-8'))
faf1e5
 
faf1e5
+    @mock.patch(M_PATH + 'readurl')
faf1e5
+    def test_read_file_or_url_passes_params_to_readurl(self, m_readurl):
faf1e5
+        """read_file_or_url passes all params through to readurl."""
faf1e5
+        url = 'http://hostname/path'
faf1e5
+        response = 'This is my url content\n'
faf1e5
+        m_readurl.return_value = response
faf1e5
+        params = {'url': url, 'timeout': 1, 'retries': 2,
faf1e5
+                  'headers': {'somehdr': 'val'},
faf1e5
+                  'data': 'data', 'sec_between': 1,
faf1e5
+                  'ssl_details': {'cert_file': '/path/cert.pem'},
faf1e5
+                  'headers_cb': 'headers_cb', 'exception_cb': 'exception_cb'}
faf1e5
+        self.assertEqual(response, read_file_or_url(**params))
faf1e5
+        params.pop('url')  # url is passed in as a positional arg
faf1e5
+        self.assertEqual([mock.call(url, **params)], m_readurl.call_args_list)
faf1e5
+
faf1e5
+    def test_wb_read_url_defaults_honored_by_read_file_or_url_callers(self):
faf1e5
+        """Readurl param defaults used when unspecified by read_file_or_url
faf1e5
+
faf1e5
+        Param defaults tested are as follows:
faf1e5
+            retries: 0, additional headers None beyond default, method: GET,
faf1e5
+            data: None, check_status: True and allow_redirects: True
faf1e5
+        """
faf1e5
+        url = 'http://hostname/path'
faf1e5
+
faf1e5
+        m_response = mock.MagicMock()
faf1e5
+
faf1e5
+        class FakeSession(requests.Session):
faf1e5
+            def request(cls, **kwargs):
faf1e5
+                self.assertEqual(
faf1e5
+                    {'url': url, 'allow_redirects': True, 'method': 'GET',
faf1e5
+                     'headers': {
faf1e5
+                         'User-Agent': 'Cloud-Init/%s' % (
faf1e5
+                             version.version_string())}},
faf1e5
+                    kwargs)
faf1e5
+                return m_response
faf1e5
+
faf1e5
+        with mock.patch(M_PATH + 'requests.Session') as m_session:
faf1e5
+            error = requests.exceptions.HTTPError('broke')
faf1e5
+            m_session.side_effect = [error, FakeSession()]
faf1e5
+            # assert no retries and check_status == True
faf1e5
+            with self.assertRaises(UrlError) as context_manager:
faf1e5
+                response = read_file_or_url(url)
faf1e5
+            self.assertEqual('broke', str(context_manager.exception))
faf1e5
+            # assert default headers, method, url and allow_redirects True
faf1e5
+            # Success on 2nd call with FakeSession
faf1e5
+            response = read_file_or_url(url)
faf1e5
+        self.assertEqual(m_response, response._response)
faf1e5
+
faf1e5
 
faf1e5
 class TestRetryOnUrlExc(CiTestCase):
faf1e5
 
faf1e5
diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py
faf1e5
index a951b8b..beb6873 100644
faf1e5
--- a/cloudinit/url_helper.py
faf1e5
+++ b/cloudinit/url_helper.py
faf1e5
@@ -81,14 +81,19 @@ def combine_url(base, *add_ons):
faf1e5
     return url
faf1e5
 
faf1e5
 
faf1e5
-def read_file_or_url(url, timeout=5, retries=10,
faf1e5
-                     headers=None, data=None, sec_between=1, ssl_details=None,
faf1e5
-                     headers_cb=None, exception_cb=None):
faf1e5
+def read_file_or_url(url, **kwargs):
faf1e5
+    """Wrapper function around readurl to allow passing a file path as url.
faf1e5
+
faf1e5
+    When url is not a local file path, passthrough any kwargs to readurl.
faf1e5
+
faf1e5
+    In the case of parameter passthrough to readurl, default values for some
faf1e5
+    parameters. See: call-signature of readurl in this module for param docs.
faf1e5
+    """
faf1e5
     url = url.lstrip()
faf1e5
     if url.startswith("/"):
faf1e5
         url = "file://%s" % url
faf1e5
     if url.lower().startswith("file://"):
faf1e5
-        if data:
faf1e5
+        if kwargs.get("data"):
faf1e5
             LOG.warning("Unable to post data to file resource %s", url)
faf1e5
         file_path = url[len("file://"):]
faf1e5
         try:
faf1e5
@@ -101,10 +106,7 @@ def read_file_or_url(url, timeout=5, retries=10,
faf1e5
             raise UrlError(cause=e, code=code, headers=None, url=url)
faf1e5
         return FileResponse(file_path, contents=contents)
faf1e5
     else:
faf1e5
-        return readurl(url, timeout=timeout, retries=retries,
faf1e5
-                       headers_cb=headers_cb, data=data,
faf1e5
-                       sec_between=sec_between, ssl_details=ssl_details,
faf1e5
-                       exception_cb=exception_cb)
faf1e5
+        return readurl(url, **kwargs)
faf1e5
 
faf1e5
 
faf1e5
 # Made to have same accessors as UrlResponse so that the
faf1e5
@@ -201,6 +203,35 @@ def readurl(url, data=None, timeout=None, retries=0, sec_between=1,
faf1e5
             check_status=True, allow_redirects=True, exception_cb=None,
faf1e5
             session=None, infinite=False, log_req_resp=True,
faf1e5
             request_method=None):
faf1e5
+    """Wrapper around requests.Session to read the url and retry if necessary
faf1e5
+
faf1e5
+    :param url: Mandatory url to request.
faf1e5
+    :param data: Optional form data to post the URL. Will set request_method
faf1e5
+        to 'POST' if present.
faf1e5
+    :param timeout: Timeout in seconds to wait for a response
faf1e5
+    :param retries: Number of times to retry on exception if exception_cb is
faf1e5
+        None or exception_cb returns True for the exception caught. Default is
faf1e5
+        to fail with 0 retries on exception.
faf1e5
+    :param sec_between: Default 1: amount of seconds passed to time.sleep
faf1e5
+        between retries. None or -1 means don't sleep.
faf1e5
+    :param headers: Optional dict of headers to send during request
faf1e5
+    :param headers_cb: Optional callable returning a dict of values to send as
faf1e5
+        headers during request
faf1e5
+    :param ssl_details: Optional dict providing key_file, ca_certs, and
faf1e5
+        cert_file keys for use on in ssl connections.
faf1e5
+    :param check_status: Optional boolean set True to raise when HTTPError
faf1e5
+        occurs. Default: True.
faf1e5
+    :param allow_redirects: Optional boolean passed straight to Session.request
faf1e5
+        as 'allow_redirects'. Default: True.
faf1e5
+    :param exception_cb: Optional callable which accepts the params
faf1e5
+        msg and exception and returns a boolean True if retries are permitted.
faf1e5
+    :param session: Optional exiting requests.Session instance to reuse.
faf1e5
+    :param infinite: Bool, set True to retry indefinitely. Default: False.
faf1e5
+    :param log_req_resp: Set False to turn off verbose debug messages.
faf1e5
+    :param request_method: String passed as 'method' to Session.request.
faf1e5
+        Typically GET, or POST. Default: POST if data is provided, GET
faf1e5
+        otherwise.
faf1e5
+    """
faf1e5
     url = _cleanurl(url)
faf1e5
     req_args = {
faf1e5
         'url': url,
faf1e5
diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
faf1e5
index ed83d2d..15af1da 100644
faf1e5
--- a/cloudinit/user_data.py
faf1e5
+++ b/cloudinit/user_data.py
faf1e5
@@ -224,7 +224,7 @@ class UserDataProcessor(object):
faf1e5
                 content = util.load_file(include_once_fn)
faf1e5
             else:
faf1e5
                 try:
faf1e5
-                    resp = read_file_or_url(include_url,
faf1e5
+                    resp = read_file_or_url(include_url, timeout=5, retries=10,
faf1e5
                                             ssl_details=self.ssl_details)
faf1e5
                     if include_once_on and resp.ok():
faf1e5
                         util.write_file(include_once_fn, resp.contents,
faf1e5
diff --git a/cloudinit/util.py b/cloudinit/util.py
faf1e5
index 2c9ac66..db9a229 100644
faf1e5
--- a/cloudinit/util.py
faf1e5
+++ b/cloudinit/util.py
faf1e5
@@ -966,13 +966,6 @@ def load_yaml(blob, default=None, allowed=(dict,)):
faf1e5
 
faf1e5
 
faf1e5
 def read_seeded(base="", ext="", timeout=5, retries=10, file_retries=0):
faf1e5
-    if base.startswith("/"):
faf1e5
-        base = "file://%s" % base
faf1e5
-
faf1e5
-    # default retries for file is 0. for network is 10
faf1e5
-    if base.startswith("file://"):
faf1e5
-        retries = file_retries
faf1e5
-
faf1e5
     if base.find("%s") >= 0:
faf1e5
         ud_url = base % ("user-data" + ext)
faf1e5
         md_url = base % ("meta-data" + ext)
faf1e5
@@ -980,14 +973,14 @@ def read_seeded(base="", ext="", timeout=5, retries=10, file_retries=0):
faf1e5
         ud_url = "%s%s%s" % (base, "user-data", ext)
faf1e5
         md_url = "%s%s%s" % (base, "meta-data", ext)
faf1e5
 
faf1e5
-    md_resp = url_helper.read_file_or_url(md_url, timeout, retries,
faf1e5
-                                          file_retries)
faf1e5
+    md_resp = url_helper.read_file_or_url(md_url, timeout=timeout,
faf1e5
+                                          retries=retries)
faf1e5
     md = None
faf1e5
     if md_resp.ok():
faf1e5
         md = load_yaml(decode_binary(md_resp.contents), default={})
faf1e5
 
faf1e5
-    ud_resp = url_helper.read_file_or_url(ud_url, timeout, retries,
faf1e5
-                                          file_retries)
faf1e5
+    ud_resp = url_helper.read_file_or_url(ud_url, timeout=timeout,
faf1e5
+                                          retries=retries)
faf1e5
     ud = None
faf1e5
     if ud_resp.ok():
faf1e5
         ud = ud_resp.contents
faf1e5
diff --git a/tests/unittests/test_datasource/test_azure_helper.py b/tests/unittests/test_datasource/test_azure_helper.py
faf1e5
index 7ad5cc1..007df09 100644
faf1e5
--- a/tests/unittests/test_datasource/test_azure_helper.py
faf1e5
+++ b/tests/unittests/test_datasource/test_azure_helper.py
faf1e5
@@ -231,8 +231,10 @@ class TestAzureEndpointHttpClient(CiTestCase):
faf1e5
         response = client.get(url, secure=False)
faf1e5
         self.assertEqual(1, self.read_file_or_url.call_count)
faf1e5
         self.assertEqual(self.read_file_or_url.return_value, response)
faf1e5
-        self.assertEqual(mock.call(url, headers=self.regular_headers),
faf1e5
-                         self.read_file_or_url.call_args)
faf1e5
+        self.assertEqual(
faf1e5
+            mock.call(url, headers=self.regular_headers, retries=10,
faf1e5
+                      timeout=5),
faf1e5
+            self.read_file_or_url.call_args)
faf1e5
 
faf1e5
     def test_secure_get(self):
faf1e5
         url = 'MyTestUrl'
faf1e5
@@ -246,8 +248,10 @@ class TestAzureEndpointHttpClient(CiTestCase):
faf1e5
         response = client.get(url, secure=True)
faf1e5
         self.assertEqual(1, self.read_file_or_url.call_count)
faf1e5
         self.assertEqual(self.read_file_or_url.return_value, response)
faf1e5
-        self.assertEqual(mock.call(url, headers=expected_headers),
faf1e5
-                         self.read_file_or_url.call_args)
faf1e5
+        self.assertEqual(
faf1e5
+            mock.call(url, headers=expected_headers, retries=10,
faf1e5
+                      timeout=5),
faf1e5
+            self.read_file_or_url.call_args)
faf1e5
 
faf1e5
     def test_post(self):
faf1e5
         data = mock.MagicMock()
faf1e5
@@ -257,7 +261,8 @@ class TestAzureEndpointHttpClient(CiTestCase):
faf1e5
         self.assertEqual(1, self.read_file_or_url.call_count)
faf1e5
         self.assertEqual(self.read_file_or_url.return_value, response)
faf1e5
         self.assertEqual(
faf1e5
-            mock.call(url, data=data, headers=self.regular_headers),
faf1e5
+            mock.call(url, data=data, headers=self.regular_headers, retries=10,
faf1e5
+                      timeout=5),
faf1e5
             self.read_file_or_url.call_args)
faf1e5
 
faf1e5
     def test_post_with_extra_headers(self):
faf1e5
@@ -269,7 +274,8 @@ class TestAzureEndpointHttpClient(CiTestCase):
faf1e5
         expected_headers = self.regular_headers.copy()
faf1e5
         expected_headers.update(extra_headers)
faf1e5
         self.assertEqual(
faf1e5
-            mock.call(mock.ANY, data=mock.ANY, headers=expected_headers),
faf1e5
+            mock.call(mock.ANY, data=mock.ANY, headers=expected_headers,
faf1e5
+                      retries=10, timeout=5),
faf1e5
             self.read_file_or_url.call_args)
faf1e5
 
faf1e5
 
faf1e5
-- 
faf1e5
1.8.3.1
faf1e5