Merge ~ajorgens/cloud-init:_include-urlerror into cloud-init:master

[Andrew Jorgensen]

Catch UrlError when #include'ing URLs

Without this the entire stage can fail, which will leave an instance
unaccessible.

Catch UrlError when #include'ing URLs

Without this the entire stage can fail, which will leave an instance unaccessible.

[Scott Moser (smoser)]

i approve with 2 small questions in line.

[Andrew Jorgensen (ajorgens)]

Revised commit per smoser's advice.

[Ryan Harper (raharper)]

On Tue, Oct 3, 2017 at 8:23 AM, Scott Moser <email address hidden> wrote:

> i approve with 2 small questions in line.
>
>
> Diff comments:
>
> > diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
> > index 88cb7f8..de459c9 100644
> > --- a/cloudinit/user_data.py
> > +++ b/cloudinit/user_data.py
> > @@ -222,16 +223,25 @@ class UserDataProcessor(object):
> > if include_once_on and os.path.isfile(include_once_fn):
> > content = util.load_file(include_once_fn)
> > else:
> > - resp = util.read_file_or_url(include_url,
> > -
> ssl_details=self.ssl_details)
> > - if include_once_on and resp.ok():
> > - util.write_file(include_once_fn, resp.contents,
> mode=0o600)
> > - if resp.ok():
> > - content = resp.contents
> > - else:
> > - LOG.warning(("Fetching from %s resulted in"
> > - " a invalid http code of %s"),
> > - include_url, resp.code)
> > + try:
> > + resp = util.read_file_or_url(include_url,
> > +
> ssl_details=self.ssl_details)
> > + if include_once_on and resp.ok():
> > + util.write_file(include_once_fn, resp.contents,
> > + mode=0o600)
> > + if resp.ok():
> > + content = resp.contents
> > + else:
> > + LOG.warning(("Fetching from %s resulted in"
> > + " a invalid http code of %s"),
> > + include_url, resp.code)
> > + except UrlError as urle:
> > + LOG.warning(
> > + "Fetching from %s resulted in a UrlError: %s",
> > + include_url, urle.cause)
>
> i think just printing urle here in its string form should be reasonable.
> str(urle) would look like:
> cloudinit.url_helper.UrlError('404 Client Error: Not Found for url:
> http://example.com/asdf')
> str(urle.cause) would look like:
> requests.exceptions.HTTPError('404 Client Error: Not Found for url:
> http://example.com/asdf')
>

> > + except IOError as ioe:
> > + LOG.warning("Fetching from %s resulted in an
> IOError: %s",
> > + include_url, ioe.strerror)
>
> i guess fine, but is there a reason for 'ioe.strerror' rather than just
> str(ioe) ?
>
> >
> > if content is not None:
> > new_msg = convert_string(content)
>
>
In both cases, using an attribute which is already a string will be faster
than calling str() which introduces some lookup and call overhead.

In an exception path, I don't think this matters; however generally I would
say we should avoid
additional function calls if they're not needed.

[Andrew Jorgensen (ajorgens)]

> In an exception path, I don't think this matters;

Do I take this to mean it's okay as it is now, and you approve? Or would you like me to undo the change that Scott suggested before it gets merged?

[Ryan Harper (raharper)]

On Wed, Oct 4, 2017 at 9:28 AM, Andrew Jorgensen <email address hidden>
wrote:

> > In an exception path, I don't think this matters;
>
> Do I take this to mean it's okay as it is now, and you approve? Or would
> you like me to undo the change that Scott suggested before it gets merged?
>

That was a comment for Scott; I'm find with his suggested change; I just
wanted to comment on the general case w.r.t
str() vs. %s formatting overhead.

> --
> https://code.launchpad.net/~ajorgens/cloud-init/+git/
> cloud-init/+merge/331660
> Your team cloud-init commiters is requested to review the proposed merge
> of ~ajorgens/cloud-init:_include-urlerror into cloud-init:master.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~cloud-init-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~cloud-init-dev
> More help : https://help.launchpad.net/ListHelp
>

[Scott Moser (smoser)]

@Ryan,

str(foo) == '%s' % foo.

In my comments i was not suggessting that we should use 'str(foo)', but just that the we should print the string representation of the error rather than error.cause.

review: Approve

diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
index 88cb7f8..e163c72 100644
--- a/cloudinit/user_data.py
+++ b/cloudinit/user_data.py
@@ -19,6 +19,7 @@ import six
 
 from cloudinit import handlers
 from cloudinit import log as logging
+from cloudinit.url_helper import UrlError
 from cloudinit import util
 
 LOG = logging.getLogger(__name__)
@@ -222,16 +223,23 @@ class UserDataProcessor(object):
             if include_once_on and os.path.isfile(include_once_fn):
                 content = util.load_file(include_once_fn)
             else:
-                resp = util.read_file_or_url(include_url,
-                                             ssl_details=self.ssl_details)
-                if include_once_on and resp.ok():
-                    util.write_file(include_once_fn, resp.contents, mode=0o600)
-                if resp.ok():
-                    content = resp.contents
-                else:
-                    LOG.warning(("Fetching from %s resulted in"
-                                 " a invalid http code of %s"),
-                                include_url, resp.code)
+                try:
+                    resp = util.read_file_or_url(include_url,
+                                                 ssl_details=self.ssl_details)
+                    if include_once_on and resp.ok():
+                        util.write_file(include_once_fn, resp.contents,
+                                        mode=0o600)
+                    if resp.ok():
+                        content = resp.contents
+                    else:
+                        LOG.warning(("Fetching from %s resulted in"
+                                     " a invalid http code of %s"),
+                                    include_url, resp.code)
+                except UrlError as urle:
+                    LOG.warning(urle)
+                except IOError as ioe:
+                    LOG.warning("Fetching from %s resulted in %s",
+                                include_url, ioe)
 
             if content is not None:
                 new_msg = convert_string(content)
diff --git a/tests/unittests/test_data.py b/tests/unittests/test_data.py
index 6d621d2..275b16d 100644
--- a/tests/unittests/test_data.py
+++ b/tests/unittests/test_data.py
@@ -18,6 +18,8 @@ from email.mime.application import MIMEApplication
 from email.mime.base import MIMEBase
 from email.mime.multipart import MIMEMultipart
 
+import httpretty
+
 from cloudinit import handlers
 from cloudinit import helpers as c_helpers
 from cloudinit import log
@@ -522,6 +524,54 @@ c: 4
         self.assertEqual(cfg.get('password'), 'gocubs')
         self.assertEqual(cfg.get('locale'), 'chicago')
 
+    @httpretty.activate
+    @mock.patch('cloudinit.url_helper.time.sleep')
+    def test_include(self, mock_sleep):
+        """Test #include."""
+        included_url = 'http://hostname/path'
+        included_data = '#cloud-config\nincluded: true\n'
+        httpretty.register_uri(httpretty.GET, included_url, included_data)
+
+        blob = '#include\n%s\n' % included_url
+
+        self.reRoot()
+        ci = stages.Init()
+        ci.datasource = FakeDataSource(blob)
+        ci.fetch()
+        ci.consume_data()
+        cc_contents = util.load_file(ci.paths.get_ipath("cloud_config"))
+        cc = util.load_yaml(cc_contents)
+        self.assertTrue(cc.get('included'))
+
+    @httpretty.activate
+    @mock.patch('cloudinit.url_helper.time.sleep')
+    def test_include_bad_url(self, mock_sleep):
+        """Test #include with a bad URL."""
+        bad_url = 'http://bad/forbidden'
+        bad_data = '#cloud-config\nbad: true\n'
+        httpretty.register_uri(httpretty.GET, bad_url, bad_data, status=403)
+
+        included_url = 'http://hostname/path'
+        included_data = '#cloud-config\nincluded: true\n'
+        httpretty.register_uri(httpretty.GET, included_url, included_data)
+
+        blob = '#include\n%s\n%s' % (bad_url, included_url)
+
+        self.reRoot()
+        ci = stages.Init()
+        ci.datasource = FakeDataSource(blob)
+        log_file = self.capture_log(logging.WARNING)
+        ci.fetch()
+        ci.consume_data()
+
+        self.assertIn("403 Client Error: Forbidden for url: %s" % bad_url,
+                      log_file.getvalue())
+
+        cc_contents = util.load_file(ci.paths.get_ipath("cloud_config"))
+        cc = util.load_yaml(cc_contents)
+        self.assertIsNone(cc.get('bad'))
+        self.assertTrue(cc.get('included'))
+
 
 class TestUDProcess(helpers.ResourceUsingTestCase):