Merge pull request #1009 from YunoHost/remove-stale-files-in-regen-conf

Clean stale file/hashes in regen-conf

src/yunohost/regenconf.py

@@ -185,6 +185,42 @@ def regen_conf(operation_logger, names=[], with_diff=False, force=False, dry_run
         succeed_regen = {}
         failed_regen = {}
 
+        # Consider the following scenario:
+        # - you add a domain foo.bar
+        # - the regen-conf creates file /etc/dnsmasq.d/foo.bar
+        # - the admin manually *deletes* /etc/dnsmasq.d/foo.bar
+        # - the file is now understood as manually deleted because there's the old file hash in regenconf.yml
+        #
+        # ... so far so good, that's the expected behavior.
+        #
+        # But then:
+        # - the admin remove domain foo.bar entirely
+        # - but now the hash for /etc/dnsmasq.d/foo.bar is *still* in
+        # regenconf.yml and and the file is still flagged as manually
+        # modified/deleted... And the user cannot even do anything about it
+        # except removing the hash in regenconf.yml...
+        #
+        # Expected behavior: it should forget about that
+        # hash because dnsmasq's regen-conf doesn't say anything about what's
+        # the state of that file so it should assume that it should be deleted.
+        #
+        # - then the admin tries to *re-add* foo.bar !
+        # - ... but because the file is still flagged as manually modified
+        # the regen-conf refuses to re-create the file.
+        #
+        # Excepted behavior : the regen-conf should have forgot about the hash
+        # from earlier and this wouldnt happen.
+        # ------
+        # conf_files contain files explicitly set by the current regen conf run
+        # conf_hashes contain all files known from the past runs
+        # we compare these to get the list of stale hashes and flag the file as
+        # "should be removed"
+        stale_files = set(conf_hashes.keys()) - set(conf_files.keys())
+        stale_files_with_non_empty_hash = [f for f in stale_files if conf_hashes.get(f)]
+        for f in stale_files_with_non_empty_hash:
+            conf_files[f] = None
+        # </> End discussion about stale file hashes
+
         for system_path, pending_path in conf_files.items():
             logger.debug("processing pending conf '%s' to system conf '%s'",
                          pending_path, system_path)
@@ -196,20 +232,45 @@ def regen_conf(operation_logger, names=[], with_diff=False, force=False, dry_run
                 system_path, pending_path, True) if with_diff else None
 
             # Check if the conf must be removed
-            to_remove = True if os.path.getsize(pending_path) == 0 else False
+            to_remove = True if pending_path and os.path.getsize(pending_path) == 0 else False
 
             # Retrieve and calculate hashes
             system_hash = _calculate_hash(system_path)
             saved_hash = conf_hashes.get(system_path, None)
             new_hash = None if to_remove else _calculate_hash(pending_path)
 
+            # -> configuration was previously managed by yunohost but should now
+            # be removed / unmanaged
+            if system_path in stale_files_with_non_empty_hash:
+                # File is already deleted, so let's just silently forget about this hash entirely
+                if not system_hash:
+                    logger.debug("> forgetting about stale file/hash")
+                    conf_hashes[system_path] = None
+                    conf_status = 'forget-about-it'
+                    regenerated = True
+                # Otherwise there's still a file on the system but it's not managed by
+                # Yunohost anymore... But if user requested --force we shall
+                # force-erase it
+                elif force:
+                    logger.debug("> force-remove stale file")
+                    regenerated = _regen(system_path)
+                    conf_status = 'force-removed'
+                # Otherwise, flag the file as manually modified
+                else:
+                    logger.warning(m18n.n(
+                        'regenconf_file_manually_modified',
+                        conf=system_path))
+                    conf_status = 'modified'
+
             # -> system conf does not exists
-            if not system_hash:
+            elif not system_hash:
                 if to_remove:
                     logger.debug("> system conf is already removed")
                     os.remove(pending_path)
-                    continue
+                    conf_hashes[system_path] = None
-                if not saved_hash or force:
+                    conf_status = 'forget-about-it'
+                    regenerated = True
+                elif not saved_hash or force:
                     if force:
                         logger.debug("> system conf has been manually removed")
                         conf_status = 'force-created'
@@ -283,7 +344,7 @@ def regen_conf(operation_logger, names=[], with_diff=False, force=False, dry_run
             if regenerated:
                 succeed_regen[system_path] = conf_result
                 conf_hashes[system_path] = new_hash
-                if os.path.isfile(pending_path):
+                if pending_path and os.path.isfile(pending_path):
                     os.remove(pending_path)
             else:
                 failed_regen[system_path] = conf_result
@@ -399,7 +460,7 @@ def _get_files_diff(orig_file, new_file, as_string=False, skip_header=True):
 def _calculate_hash(path):
     """Calculate the MD5 hash of a file"""
 
-    if not os.path.exists(path):
+    if not path or not os.path.exists(path):
         return None
 
     hasher = hashlib.md5()
@@ -485,6 +546,12 @@ def _update_conf_hashes(category, hashes):
     if category_conf is None:
         category_conf = {}
 
+    # If a file shall be removed and is indeed removed, forget entirely about
+    # that path.
+    # It avoid keeping weird old entries like
+    # /etc/nginx/conf.d/some.domain.that.got.removed.conf
+    hashes = {path: hash_ for path, hash_ in hashes.items() if hash_ is not None or os.path.exists(path)}
+
     category_conf['conffiles'] = hashes
     categories[category] = category_conf
     _save_regenconf_infos(categories)

src/yunohost/tests/test_regenconf.py

@@ -1,31 +1,25 @@
-import glob
 import os
-import pytest
-import shutil
-import requests
 
-from conftest import message, raiseYunohostError
+from yunohost.domain import domain_add, domain_remove, domain_list
-
+from yunohost.regenconf import regen_conf, manually_modified_files, _get_conf_hashes, _force_clear_hashes
-from moulinette import m18n
-from moulinette.utils.filesystem import mkdir
-
-from yunohost.domain import _get_maindomain, domain_add, domain_remove, domain_list
-from yunohost.utils.error import YunohostError
-from yunohost.regenconf import manually_modified_files, _get_conf_hashes, _force_clear_hashes
 
 TEST_DOMAIN = "secondarydomain.test"
-TEST_DOMAIN_NGINX_CONFIG = "/etc/nginx/conf.d/secondarydomain.test.conf"
+TEST_DOMAIN_NGINX_CONFIG = "/etc/nginx/conf.d/%s.conf" % TEST_DOMAIN
+TEST_DOMAIN_DNSMASQ_CONFIG = "/etc/dnsmasq.d/%s" % TEST_DOMAIN
+
 
 def setup_function(function):
 
     _force_clear_hashes([TEST_DOMAIN_NGINX_CONFIG])
     clean()
 
+
 def teardown_function(function):
 
     clean()
     _force_clear_hashes([TEST_DOMAIN_NGINX_CONFIG])
 
+
 def clean():
 
     assert os.system("pgrep slapd >/dev/null") == 0
@@ -78,3 +72,94 @@ def test_add_domain_conf_already_exists():
     assert os.path.exists(TEST_DOMAIN_NGINX_CONFIG)
     assert TEST_DOMAIN_NGINX_CONFIG in _get_conf_hashes("nginx")
     assert TEST_DOMAIN_NGINX_CONFIG not in manually_modified_files()
+
+
+def test_stale_hashes_get_removed_if_empty():
+    """
+    This is intended to test that if a file gets removed and is indeed removed,
+    we don't keep a useless empty hash corresponding to an old file.
+    In this case, we test this using the dnsmasq conf file (we don't do this
+    using the nginx conf file because it's already force-removed during
+    domain_remove())
+    """
+
+    domain_add(TEST_DOMAIN)
+
+    assert os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+    assert TEST_DOMAIN_DNSMASQ_CONFIG in _get_conf_hashes("dnsmasq")
+
+    domain_remove(TEST_DOMAIN)
+
+    assert not os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+    assert TEST_DOMAIN_DNSMASQ_CONFIG not in _get_conf_hashes("dnsmasq")
+
+
+def test_stale_hashes_if_file_manually_deleted():
+    """
+    Same as other test, but manually delete the file in between and check
+    behavior
+    """
+
+    domain_add(TEST_DOMAIN)
+
+    assert os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+    assert TEST_DOMAIN_DNSMASQ_CONFIG in _get_conf_hashes("dnsmasq")
+
+    os.remove(TEST_DOMAIN_DNSMASQ_CONFIG)
+
+    assert not os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+
+    regen_conf(names=["dnsmasq"])
+
+    assert not os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+    assert TEST_DOMAIN_DNSMASQ_CONFIG in _get_conf_hashes("dnsmasq")
+
+    domain_remove(TEST_DOMAIN)
+
+    assert not os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+    assert TEST_DOMAIN_DNSMASQ_CONFIG not in _get_conf_hashes("dnsmasq")
+
+# This test only works if you comment the part at the end of the regen-conf in
+# dnsmasq that auto-flag /etc/dnsmasq.d/foo.bar as "to be removed" (using touch)
+# ... But we want to keep it because they also possibly flag files that were
+# never known by the regen-conf (e.g. if somebody adds a
+# /etc/dnsmasq.d/my.custom.extension)
+# Ideally we could use a system that's able to properly state 'no file in this
+# folder should exist except the ones excplicitly defined by regen-conf' but
+# that's too much work for the scope of this commit.
+#
+# ... Anyway, the proper way to write these tests would be to use a dummy
+# regen-conf hook just for tests but meh I'm lazy
+#
+#def test_stale_hashes_if_file_manually_modified():
+#    """
+#    Same as other test, but manually delete the file in between and check
+#    behavior
+#    """
+#
+#    domain_add(TEST_DOMAIN)
+#
+#    assert os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+#    assert TEST_DOMAIN_DNSMASQ_CONFIG in _get_conf_hashes("dnsmasq")
+#
+#    os.system("echo '#pwet' > %s" % TEST_DOMAIN_DNSMASQ_CONFIG)
+#
+#    assert os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+#    assert open(TEST_DOMAIN_DNSMASQ_CONFIG).read().strip() == "#pwet"
+#
+#    regen_conf(names=["dnsmasq"])
+#
+#    assert os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+#    assert open(TEST_DOMAIN_DNSMASQ_CONFIG).read().strip() == "#pwet"
+#    assert TEST_DOMAIN_DNSMASQ_CONFIG in _get_conf_hashes("dnsmasq")
+#
+#    domain_remove(TEST_DOMAIN)
+#
+#    assert os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+#    assert open(TEST_DOMAIN_DNSMASQ_CONFIG).read().strip() == "#pwet"
+#    assert TEST_DOMAIN_DNSMASQ_CONFIG in _get_conf_hashes("dnsmasq")
+#
+#    regen_conf(names=["dnsmasq"], force=True)
+#
+#    assert not os.path.exists(TEST_DOMAIN_DNSMASQ_CONFIG)
+#    assert TEST_DOMAIN_DNSMASQ_CONFIG not in _get_conf_hashes("dnsmasq")