From c5320eb22386c34d83435fa0b9aa03b178996b52 Mon Sep 17 00:00:00 2001
From: Mark Goddard <mark@stackhpc.com>
Date: Thu, 6 Aug 2020 10:50:25 +0100
Subject: [PATCH] Fix kolla_set_configs --check with a directory

There are several issues with kolla_set_configs --check:

1. We calculate the destination path incorrectly when comparing a file
   in a directory, due to passing arguments to os.path.relpath in the
   wrong order
2. For directories that have not changed, we also attempt to compare
   them as files, which fails when they are open()ed.
3. If the config JSON does not have a config_files key, it fails with a
   KeyError.

The first two issues affect the fluentd container, which specifies
directories as the source, without using a glob. The third affects OVN
containers.

This patch fixes these issues.

Closes-Bug: #1890567

Change-Id: I8921befe51da4282121443849177a7ca5ebe8822
---
 docker/base/set_configs.py                    | 11 ++---
 .../fix-config-check-c973c462761a4fa9.yaml    |  6 +++
 tests/test_set_config.py                      | 41 +++++++++++++++++++
 3 files changed, 53 insertions(+), 5 deletions(-)
 create mode 100644 releasenotes/notes/fix-config-check-c973c462761a4fa9.yaml

diff --git a/docker/base/set_configs.py b/docker/base/set_configs.py
index 5f030866fb..572e4a001c 100644
--- a/docker/base/set_configs.py
+++ b/docker/base/set_configs.py
@@ -204,8 +204,8 @@ class ConfigFile(object):
                     return False
             for file_ in files:
                 full_path = os.path.join(root, file_)
-                dest_full_path = os.path.join(dest, os.path.relpath(source,
-                                                                    full_path))
+                dest_full_path = os.path.join(dest, os.path.relpath(full_path,
+                                                                    source))
                 if not self._cmp_file(full_path, dest_full_path):
                     return False
         return True
@@ -225,8 +225,9 @@ class ConfigFile(object):
             # otherwise means copy the source to dest
             if dest.endswith(os.sep):
                 dest = os.path.join(dest, os.path.basename(source))
-            if os.path.isdir(source) and not self._cmp_dir(source, dest):
-                bad_state_files.append(source)
+            if os.path.isdir(source):
+                if not self._cmp_dir(source, dest):
+                    bad_state_files.append(source)
             elif not self._cmp_file(source, dest):
                 bad_state_files.append(source)
         if len(bad_state_files) != 0:
@@ -404,7 +405,7 @@ def execute_config_strategy(config):
 
 
 def execute_config_check(config):
-    for data in config['config_files']:
+    for data in config.get('config_files', []):
         config_file = ConfigFile(**data)
         config_file.check()
 
diff --git a/releasenotes/notes/fix-config-check-c973c462761a4fa9.yaml b/releasenotes/notes/fix-config-check-c973c462761a4fa9.yaml
new file mode 100644
index 0000000000..ee87646dfd
--- /dev/null
+++ b/releasenotes/notes/fix-config-check-c973c462761a4fa9.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes an issue with the ``kolla_set_configs --check`` command when the
+    source is a directory. `LP#1890567
+    <https://bugs.launchpad.net/kolla/+bug/1890567>`__
diff --git a/tests/test_set_config.py b/tests/test_set_config.py
index 8af2e618d6..3888a38cca 100644
--- a/tests/test_set_config.py
+++ b/tests/test_set_config.py
@@ -288,3 +288,44 @@ class ConfigFileTest(base.BaseTestCase):
                                     '/foo/bar.conf'),
                           mock.call('/var/lib/kolla/config_files/bar.yml',
                                     '/foo/bar.yml')])
+
+    @mock.patch.object(set_configs.ConfigFile, '_cmp_file')
+    @mock.patch.object(set_configs.ConfigFile, '_cmp_dir')
+    @mock.patch('os.path.isdir', return_value=False)
+    @mock.patch('glob.glob')
+    def test_check_source_dir(self, mock_glob, mock_isdir, mock_cmp_dir,
+                              mock_cmp_file):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644')
+
+        mock_glob.return_value = ['/var/lib/kolla/config_files/bar']
+        mock_isdir.return_value = True
+        mock_cmp_dir.return_value = True
+
+        config_file.check()
+
+        mock_isdir.assert_called_once_with('/var/lib/kolla/config_files/bar')
+        mock_cmp_dir.assert_called_once_with(
+            '/var/lib/kolla/config_files/bar', '/foo')
+        mock_cmp_file.assert_not_called()
+
+    @mock.patch.object(set_configs.ConfigFile, '_cmp_file')
+    @mock.patch.object(set_configs.ConfigFile, '_cmp_dir')
+    @mock.patch('os.path.isdir', return_value=False)
+    @mock.patch('glob.glob')
+    def test_check_source_dir_no_equal(self, mock_glob, mock_isdir,
+                                       mock_cmp_dir, mock_cmp_file):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644')
+
+        mock_glob.return_value = ['/var/lib/kolla/config_files/bar']
+        mock_isdir.return_value = True
+        mock_cmp_dir.return_value = False
+
+        self.assertRaises(set_configs.ConfigFileBadState,
+                          config_file.check)
+
+        mock_isdir.assert_called_once_with('/var/lib/kolla/config_files/bar')
+        mock_cmp_dir.assert_called_once_with(
+            '/var/lib/kolla/config_files/bar', '/foo')
+        mock_cmp_file.assert_not_called()