From 5dba5c60f6ff5d0587398d46a9c65a9069ba41d8 Mon Sep 17 00:00:00 2001
From: Adrian Vladu <avladu@cloudbasesolutions.com>
Date: Fri, 25 Oct 2019 16:52:03 +0300
Subject: [PATCH] Check all configdrive types if one errors out

By default, all config drive types are checked.
But if somehow, the implementation of each type
fails, the metadata service errors out and does not
check for the next type.

The issue was reported here:
https://ask.cloudbase.it/question/3094/windows-server-2016-extendvolumespluginp-doesnt-work/

In that case, in the method is_vfat_drive:
  match = VOLUME_LABEL_REGEX.search(out)
  return match.group(1) in CONFIG_DRIVE_LABELS

if match value is None, the return line throws an error:
  AttributeError: 'NoneType' object has no attribute 'group'

To make sure that no other implementation will bubble up
the error, we catch the error in the config_drive metadata
service.

Catching the error will allow that the next config_drive
type will be checked for metadata.

Change-Id: I0d9967ec6a81214c7d78be667cffa4a98758587a
---
 .../services/osconfigdrive/windows.py         | 22 ++++++++++++-------
 .../services/osconfigdrive/test_windows.py    |  8 +++++++
 .../tests/utils/windows/test_vfat.py          | 13 +++++++++++
 cloudbaseinit/utils/windows/vfat.py           |  2 +-
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/cloudbaseinit/metadata/services/osconfigdrive/windows.py b/cloudbaseinit/metadata/services/osconfigdrive/windows.py
index 81a6238a..a2fcc3b1 100644
--- a/cloudbaseinit/metadata/services/osconfigdrive/windows.py
+++ b/cloudbaseinit/metadata/services/osconfigdrive/windows.py
@@ -176,14 +176,20 @@ class WindowsConfigDriveManager(base.BaseConfigDriveManager):
         return False
 
     def _get_config_drive_files(self, cd_type, cd_location):
-        get_config_drive = self.config_drive_type_location.get(
-            "{}_{}".format(cd_location, cd_type))
-        if get_config_drive:
-            return get_config_drive()
-        else:
-            LOG.debug("Irrelevant type %(type)s in %(location)s location; "
-                      "skip",
-                      {"type": cd_type, "location": cd_location})
+        try:
+            get_config_drive = self.config_drive_type_location.get(
+                "{}_{}".format(cd_location, cd_type))
+            if get_config_drive:
+                return get_config_drive()
+            else:
+                LOG.debug("Irrelevant type %(type)s in %(location)s "
+                          "location; skip",
+                          {"type": cd_type, "location": cd_location})
+        except Exception as exc:
+            LOG.warning("Config type %(type)s not found in %(loc)s "
+                        "location; Error: '%(err)r'",
+                        {"type": cd_type, "loc": cd_location, "err": exc})
+
         return False
 
     def get_config_drive_files(self, searched_types=None,
diff --git a/cloudbaseinit/tests/metadata/services/osconfigdrive/test_windows.py b/cloudbaseinit/tests/metadata/services/osconfigdrive/test_windows.py
index f972b3d4..70fbab0a 100644
--- a/cloudbaseinit/tests/metadata/services/osconfigdrive/test_windows.py
+++ b/cloudbaseinit/tests/metadata/services/osconfigdrive/test_windows.py
@@ -394,6 +394,14 @@ class TestWindowsConfigDriveManager(unittest.TestCase):
         self._test__get_config_drive_files(
             "iso", "cdrom", func)
 
+    @mock.patch('cloudbaseinit.metadata.services.osconfigdrive.windows.'
+                'WindowsConfigDriveManager.'
+                '_get_config_drive_from_cdrom_drive')
+    def test__get_config_drive_files_cdrom_iso_failed(self, func):
+        func.side_effect = Exception
+        self._test__get_config_drive_files(
+            "iso", "cdrom", func, found=False)
+
     def test__get_config_drive_files_cdrom_vfat(self):
         self._test__get_config_drive_files(
             "vfat", "cdrom", None)
diff --git a/cloudbaseinit/tests/utils/windows/test_vfat.py b/cloudbaseinit/tests/utils/windows/test_vfat.py
index 35468b61..53219717 100644
--- a/cloudbaseinit/tests/utils/windows/test_vfat.py
+++ b/cloudbaseinit/tests/utils/windows/test_vfat.py
@@ -105,6 +105,19 @@ class TestVfat(unittest.TestCase):
                                  expected_logging=expected_logging,
                                  expected_response=expected_response)
 
+    def test_is_vfat_drive_with_wrong_label(self):
+        mock_out = b"Not volu label  \r\n"
+        expected_logging = [
+            "Obtained label information for drive %r: %r"
+            % (mock.sentinel.drive, mock_out)
+        ]
+        execute_process_value = (mock_out, None, 0)
+        expected_response = False
+
+        self._test_is_vfat_drive(execute_process_value=execute_process_value,
+                                 expected_logging=expected_logging,
+                                 expected_response=expected_response)
+
     @testutils.ConfPatcher('mtools_path', 'mtools_path')
     @mock.patch('os.chdir')
     def test_copy(self, mock_os_chdir):
diff --git a/cloudbaseinit/utils/windows/vfat.py b/cloudbaseinit/utils/windows/vfat.py
index 60b91df6..9705d965 100644
--- a/cloudbaseinit/utils/windows/vfat.py
+++ b/cloudbaseinit/utils/windows/vfat.py
@@ -50,7 +50,7 @@ def is_vfat_drive(osutils, drive_path):
     LOG.debug("Obtained label information for drive %r: %r", drive_path, out)
     out = out.decode().strip()
     match = VOLUME_LABEL_REGEX.search(out)
-    return match.group(1) in CONFIG_DRIVE_LABELS
+    return match.group(1) in CONFIG_DRIVE_LABELS if match else False
 
 
 def copy_from_vfat_drive(osutils, drive_path, target_path):