Properly handle stale hashes of files even when file got manually removed/modified

src/yunohost/regenconf.py

@@ -200,6 +200,42 @@ def regen_conf(operation_logger, names=[], with_diff=False, force=False, dry_run
             conf_hashes[sshd_config] = _calculate_hash(conf_files[sshd_config])
             _update_conf_hashes(category, conf_hashes)
 
+        # 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)
@@ -211,20 +247,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)
 
-            # -> system conf does not exists
+            # -> 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
+            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'
@@ -301,7 +362,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
@@ -417,7 +478,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()

src/yunohost/tests/test_regenconf.py

@@ -161,3 +161,48 @@ def test_stale_hashes_if_file_manually_deleted():
 
     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")