From f9fcf18105845fbb933925ae7b0a2f1033f75127 Mon Sep 17 00:00:00 2001

From: Eduardo Otubo <otubo@redhat.com>

Date: Wed, 20 May 2020 10:11:14 +0200

Subject: [PATCH] url_helper: read_file_or_url should pass headers param into

 readurl (#66)

RH-Author: Eduardo Otubo <otubo@redhat.com>

Message-id: <20200519105653.20249-1-otubo@redhat.com>

Patchwork-id: 96613

O-Subject: [RHEL-7.8.z cloud-init PATCH] url_helper: read_file_or_url should pass headers param into readurl (#66)

Bugzilla: 1832177

RH-Acked-by: Cathy Avery <cavery@redhat.com>

RH-Acked-by: Mohammed Gamal <mgamal@redhat.com>

RH-Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>

commit f69d33a723b805fec3ee70c3a6127c8cadcb02d8

Author: Chad Smith <chad.smith@canonical.com>

Date:   Mon Dec 2 16:24:18 2019 -0700

    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

Signed-off-by: Eduardo Otubo <otubo@redhat.com>

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

---

 cloudinit/sources/helpers/azure.py                 |  6 ++-

 cloudinit/tests/test_url_helper.py                 | 52 ++++++++++++++++++++++

 cloudinit/url_helper.py                            | 47 +++++++++++++++----

 cloudinit/user_data.py                             |  2 +-

 cloudinit/util.py                                  | 15 ++-----

 .../unittests/test_datasource/test_azure_helper.py | 18 +++++---

 6 files changed, 112 insertions(+), 28 deletions(-)

diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py

index c2a57cc..b99c484 100755

--- a/cloudinit/sources/helpers/azure.py

+++ b/cloudinit/sources/helpers/azure.py

@@ -103,14 +103,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 aa9f3ec..e883ddc 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 a951b8b..beb6873 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 ed83d2d..15af1da 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 2c9ac66..db9a229 100644

--- a/cloudinit/util.py

+++ b/cloudinit/util.py

@@ -966,13 +966,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)

@@ -980,14 +973,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

diff --git a/tests/unittests/test_datasource/test_azure_helper.py b/tests/unittests/test_datasource/test_azure_helper.py

index 7ad5cc1..007df09 100644

--- a/tests/unittests/test_datasource/test_azure_helper.py

+++ b/tests/unittests/test_datasource/test_azure_helper.py

@@ -231,8 +231,10 @@ class TestAzureEndpointHttpClient(CiTestCase):

         response = client.get(url, secure=False)

         self.assertEqual(1, self.read_file_or_url.call_count)

         self.assertEqual(self.read_file_or_url.return_value, response)

-        self.assertEqual(mock.call(url, headers=self.regular_headers),

-                         self.read_file_or_url.call_args)

+        self.assertEqual(

+            mock.call(url, headers=self.regular_headers, retries=10,

+                      timeout=5),

+            self.read_file_or_url.call_args)

     def test_secure_get(self):

         url = 'MyTestUrl'

@@ -246,8 +248,10 @@ class TestAzureEndpointHttpClient(CiTestCase):

         response = client.get(url, secure=True)

         self.assertEqual(1, self.read_file_or_url.call_count)

         self.assertEqual(self.read_file_or_url.return_value, response)

-        self.assertEqual(mock.call(url, headers=expected_headers),

-                         self.read_file_or_url.call_args)

+        self.assertEqual(

+            mock.call(url, headers=expected_headers, retries=10,

+                      timeout=5),

+            self.read_file_or_url.call_args)

     def test_post(self):

         data = mock.MagicMock()

@@ -257,7 +261,8 @@ class TestAzureEndpointHttpClient(CiTestCase):

         self.assertEqual(1, self.read_file_or_url.call_count)

         self.assertEqual(self.read_file_or_url.return_value, response)

         self.assertEqual(

-            mock.call(url, data=data, headers=self.regular_headers),

+            mock.call(url, data=data, headers=self.regular_headers, retries=10,

+                      timeout=5),

             self.read_file_or_url.call_args)

     def test_post_with_extra_headers(self):

@@ -269,7 +274,8 @@ class TestAzureEndpointHttpClient(CiTestCase):

         expected_headers = self.regular_headers.copy()

         expected_headers.update(extra_headers)

         self.assertEqual(

-            mock.call(mock.ANY, data=mock.ANY, headers=expected_headers),

+            mock.call(mock.ANY, data=mock.ANY, headers=expected_headers,

+                      retries=10, timeout=5),

             self.read_file_or_url.call_args)

-- 

1.8.3.1