New feature flag functionality and fix includes failing silently (#367)

Build time feature flags are now defined in cloudinit/features.py.
Feature flags can be added to toggle configuration options or
deprecated features. Feature flag overrides can be placed in
cloudinit/feature_overrides.py. Further documentation can be found in
HACKING.rst.

Additionally, updated default behavior to exit with an exception
if #include can't retrieve resources as expected. This behavior
can be toggled with a feature flag.

LP: #1734939

5 files changed, 147 insertions, 9 deletions

diff --git a/HACKING.rst b/HACKING.rst
index d026cf71..81e6c266 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -324,6 +324,13 @@ variable annotations specified in `PEP-526`_ were introduced in Python
 
    in a xenial lxd container with python3-pytest installed.
 
+Feature Flags
+-------------
+
+.. automodule:: cloudinit.features
+   :members:
+
+
 Ongoing Refactors
 =================
 
diff --git a/cloudinit/features.py b/cloudinit/features.py
new file mode 100644
index 00000000..e455213d
--- /dev/null
+++ b/cloudinit/features.py
@@ -0,0 +1,33 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+"""
+Feature flags are used as a way to easily toggle configuration
+**at build time**. They are provided to accommodate feature deprecation and
+downstream configuration changes.
+
+Currently used upstream values for feature flags are set in
+``cloudinit/features.py``. Overrides to these values (typically via quilt
+patch) can be placed
+in a file called ``feature_overrides.py`` in the same directory. Any value
+set in ``feature_overrides.py`` will override the original value set
+in ``features.py``.
+
+Each flag should include a short comment regarding the reason for
+the flag and intended lifetime.
+
+Tests are required for new feature flags, and tests must verify
+all valid states of a flag, not just the default state.
+"""
+
+ERROR_ON_USER_DATA_FAILURE = True
+"""
+If there is a failure in obtaining user data (i.e., #include or
+decompress fails), old behavior is to log a warning and proceed.
+After the 20.2 release, we instead raise an exception.
+This flag can be removed after Focal is no longer supported
+"""
+
+try:
+    # pylint: disable=wildcard-import
+    from cloudinit.feature_overrides import *  # noqa
+except ImportError:
+    pass
diff --git a/cloudinit/tests/test_features.py b/cloudinit/tests/test_features.py
new file mode 100644
index 00000000..d7a7226d
--- /dev/null
+++ b/cloudinit/tests/test_features.py
@@ -0,0 +1,60 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+# pylint: disable=no-member,no-name-in-module
+"""
+This file is for testing the feature flag functionality itself,
+NOT for testing any individual feature flag
+"""
+import pytest
+import sys
+from pathlib import Path
+
+import cloudinit
+
+
+@pytest.yield_fixture()
+def create_override(request):
+    """
+    Create a feature overrides file and do some module wizardry to make
+    it seem like we're importing the features file for the first time.
+
+    After creating the override file with the values passed by the test,
+    we need to reload cloudinit.features
+    to get all of the current features (including the overridden ones).
+    Once the test is complete, we remove the file we created and set
+    features and feature_overrides modules to how they were before
+    the test started
+    """
+    override_path = Path(cloudinit.__file__).parent / 'feature_overrides.py'
+    if override_path.exists():
+        raise Exception("feature_overrides.py unexpectedly exists! "
+                        "Remove it to run this test.")
+    with override_path.open('w') as f:
+        for key, value in request.param.items():
+            f.write('{} = {}\n'.format(key, value))
+
+    sys.modules.pop('cloudinit.features', None)
+
+    yield
+
+    override_path.unlink()
+    sys.modules.pop('cloudinit.feature_overrides', None)
+
+
+class TestFeatures:
+    def test_feature_without_override(self):
+        from cloudinit.features import ERROR_ON_USER_DATA_FAILURE
+        assert ERROR_ON_USER_DATA_FAILURE is True
+
+    @pytest.mark.parametrize('create_override',
+                             [{'ERROR_ON_USER_DATA_FAILURE': False}],
+                             indirect=True)
+    def test_feature_with_override(self, create_override):
+        from cloudinit.features import ERROR_ON_USER_DATA_FAILURE
+        assert ERROR_ON_USER_DATA_FAILURE is False
+
+    @pytest.mark.parametrize('create_override',
+                             [{'SPAM': True}],
+                             indirect=True)
+    def test_feature_only_in_override(self, create_override):
+        from cloudinit.features import SPAM
+        assert SPAM is True
diff --git a/cloudinit/user_data.py b/cloudinit/user_data.py
index 670dbee6..67bdf981 100644
--- a/cloudinit/user_data.py
+++ b/cloudinit/user_data.py
@@ -16,6 +16,7 @@ from email.mime.text import MIMEText
 
 from cloudinit import handlers
 from cloudinit import log as logging
+from cloudinit import features
 from cloudinit.url_helper import read_file_or_url, UrlError
 from cloudinit import util
 
@@ -69,6 +70,13 @@ def _set_filename(msg, filename):
                    'attachment', filename=str(filename))
 
 
+def _handle_error(error_message, source_exception=None):
+    if features.ERROR_ON_USER_DATA_FAILURE:
+        raise Exception(error_message) from source_exception
+    else:
+        LOG.warning(error_message)
+
+
 class UserDataProcessor(object):
     def __init__(self, paths):
         self.paths = paths
@@ -108,9 +116,11 @@ class UserDataProcessor(object):
                     ctype_orig = None
                     was_compressed = True
                 except util.DecompressionError as e:
-                    LOG.warning("Failed decompressing payload from %s of"
-                                " length %s due to: %s",
-                                ctype_orig, len(payload), e)
+                    error_message = (
+                        "Failed decompressing payload from {} of"
+                        " length {} due to: {}".format(
+                            ctype_orig, len(payload), e))
+                    _handle_error(error_message, e)
                     continue
 
             # Attempt to figure out the payloads content-type
@@ -231,19 +241,22 @@ class UserDataProcessor(object):
                     if resp.ok():
                         content = resp.contents
                     else:
-                        LOG.warning(("Fetching from %s resulted in"
-                                     " a invalid http code of %s"),
-                                    include_url, resp.code)
+                        error_message = (
+                            "Fetching from {} resulted in"
+                            " a invalid http code of {}".format(
+                                include_url, resp.code))
+                        _handle_error(error_message)
                 except UrlError as urle:
                     message = str(urle)
                     # Older versions of requests.exceptions.HTTPError may not
                     # include the errant url. Append it for clarity in logs.
                     if include_url not in message:
                         message += ' for url: {0}'.format(include_url)
-                    LOG.warning(message)
+                    _handle_error(message, urle)
                 except IOError as ioe:
-                    LOG.warning("Fetching from %s resulted in %s",
-                                include_url, ioe)
+                    error_message = "Fetching from {} resulted in {}".format(
+                        include_url, ioe)
+                    _handle_error(error_message, 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 a4261609..7fb9c3ab 100644
--- a/tests/unittests/test_data.py
+++ b/tests/unittests/test_data.py
@@ -639,6 +639,31 @@ class TestConsumeUserDataHttp(TestConsumeUserData, helpers.HttprettyTestCase):
         self.reRoot()
         ci = stages.Init()
         ci.datasource = FakeDataSource(blob)
+        ci.fetch()
+        with self.assertRaises(Exception) as context:
+            ci.consume_data()
+        self.assertIn('403', str(context.exception))
+
+        with self.assertRaises(FileNotFoundError):
+            util.load_file(ci.paths.get_ipath("cloud_config"))
+
+    @mock.patch('cloudinit.url_helper.time.sleep')
+    @mock.patch('cloudinit.features.ERROR_ON_USER_DATA_FAILURE', False)
+    def test_include_bad_url_no_fail(self, mock_sleep):
+        """Test #include with a bad URL and failure disabled"""
+        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()