Merge pull request #946 from YunoHost/be-more-robust-against-manually-modified-domain-conf

[fix] Be more robust against manually modified domain conf

.gitlab-ci.yml

@@ -92,3 +92,10 @@ test-user-group:
   script:
     - cd src/yunohost
     - py.test tests/test_user-group.py
+    
+test-regenconf:
+  extends: .tests
+  stage: tests
+  script:
+    - cd src/yunohost
+    - py.test tests/test_regenconf.py

data/hooks/conf_regen/15-nginx

@@ -27,7 +27,8 @@ do_init_regen() {
   ynh_render_template "yunohost_admin.conf" "${nginx_conf_dir}/yunohost_admin.conf"
 
   # Restart nginx if conf looks good, otherwise display error and exit unhappy
-  nginx -t 2>/dev/null && service nginx restart || (nginx -t && exit 1)
+  nginx -t 2>/dev/null || { nginx -t; exit 1; }
+  systemctl restart nginx || { journalctl --no-pager --lines=10 -u nginx >&2; exit 1; }
 
   exit 0
 }
@@ -125,9 +126,9 @@ do_post_regen() {
       fi
   done
 
-
+  # Reload nginx if conf looks good, otherwise display error and exit unhappy
-  # Reload nginx configuration
+  nginx -t 2>/dev/null || { nginx -t; exit 1; }
-  pgrep nginx && service nginx reload
+  pgrep nginx && systemctl reload nginx || { journalctl --no-pager --lines=10 -u nginx >&2; exit 1; }
 }
 
 FORCE=${2:-0}

src/yunohost/domain.py

@@ -33,7 +33,7 @@ from yunohost.utils.error import YunohostError
 from moulinette.utils.log import getActionLogger
 
 from yunohost.app import app_ssowatconf
-from yunohost.regenconf import regen_conf
+from yunohost.regenconf import regen_conf, _force_clear_hashes, _process_regen_conf
 from yunohost.utils.network import get_public_ip
 from yunohost.log import is_unit_operation
 from yunohost.hook import hook_callback
@@ -124,6 +124,17 @@ def domain_add(operation_logger, domain, dyndns=False):
 
         # Don't regen these conf if we're still in postinstall
         if os.path.exists('/etc/yunohost/installed'):
+            # Sometime we have weird issues with the regenconf where some files
+            # appears as manually modified even though they weren't touched ...
+            # There are a few ideas why this happens (like backup/restore nginx
+            # conf ... which we shouldnt do ...). This in turns creates funky
+            # situation where the regenconf may refuse to re-create the conf
+            # (when re-creating a domain..)
+            # So here we force-clear the has out of the regenconf if it exists.
+            # This is a pretty ad hoc solution and only applied to nginx
+            # because it's one of the major service, but in the long term we
+            # should identify the root of this bug...
+            _force_clear_hashes(["/etc/nginx/conf.d/%s.conf" % domain])
             regen_conf(names=['nginx', 'metronome', 'dnsmasq', 'postfix', 'rspamd'])
             app_ssowatconf()
 
@@ -188,6 +199,25 @@ def domain_remove(operation_logger, domain, force=False):
 
     os.system('rm -rf /etc/yunohost/certs/%s' % domain)
 
+    # Sometime we have weird issues with the regenconf where some files
+    # appears as manually modified even though they weren't touched ...
+    # There are a few ideas why this happens (like backup/restore nginx
+    # conf ... which we shouldnt do ...). This in turns creates funky
+    # situation where the regenconf may refuse to re-create the conf
+    # (when re-creating a domain..)
+    #
+    # So here we force-clear the has out of the regenconf if it exists.
+    # This is a pretty ad hoc solution and only applied to nginx
+    # because it's one of the major service, but in the long term we
+    # should identify the root of this bug...
+    _force_clear_hashes(["/etc/nginx/conf.d/%s.conf" % domain])
+    # And in addition we even force-delete the file Otherwise, if the file was
+    # manually modified, it may not get removed by the regenconf which leads to
+    # catastrophic consequences of nginx breaking because it can't load the
+    # cert file which disappeared etc..
+    if os.path.exists("/etc/nginx/conf.d/%s.conf" % domain):
+        _process_regen_conf("/etc/nginx/conf.d/%s.conf" % domain, new_conf=None, save=True)
+
     regen_conf(names=['nginx', 'metronome', 'dnsmasq', 'postfix'])
     app_ssowatconf()
 

src/yunohost/regenconf.py

@@ -473,6 +473,18 @@ def _update_conf_hashes(category, hashes):
     _save_regenconf_infos(categories)
 
 
+def _force_clear_hashes(paths):
+
+    categories = _get_regenconf_infos()
+    for path in paths:
+        for category in categories.keys():
+            if path in categories[category]['conffiles']:
+                logger.debug("force-clearing old conf hash for %s in category %s" % (path, category))
+                del categories[category]['conffiles'][path]
+
+    _save_regenconf_infos(categories)
+
+
 def _process_regen_conf(system_conf, new_conf=None, save=True):
     """Regenerate a given system configuration file
 

src/yunohost/tests/test_regenconf.py

@@ -0,0 +1,80 @@
+import glob
+import os
+import pytest
+import shutil
+import requests
+
+from conftest import message, raiseYunohostError
+
+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"
+
+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
+    assert os.system("pgrep nginx >/dev/null") == 0
+
+    if TEST_DOMAIN in domain_list()["domains"]:
+        domain_remove(TEST_DOMAIN)
+        assert not os.path.exists(TEST_DOMAIN_NGINX_CONFIG)
+
+    os.system("rm -f %s" % TEST_DOMAIN_NGINX_CONFIG)
+
+    assert os.system("nginx -t 2>/dev/null") == 0
+
+    assert not os.path.exists(TEST_DOMAIN_NGINX_CONFIG)
+    assert TEST_DOMAIN_NGINX_CONFIG not in _get_conf_hashes("nginx")
+    assert TEST_DOMAIN_NGINX_CONFIG not in manually_modified_files()
+
+
+def test_add_domain():
+
+    domain_add(TEST_DOMAIN)
+
+    assert TEST_DOMAIN in domain_list()["domains"]
+
+    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_add_and_edit_domain_conf():
+
+    domain_add(TEST_DOMAIN)
+
+    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()
+
+    os.system("echo ' ' >> %s" % TEST_DOMAIN_NGINX_CONFIG)
+
+    assert TEST_DOMAIN_NGINX_CONFIG in manually_modified_files()
+
+
+def test_add_domain_conf_already_exists():
+
+    os.system("echo ' ' >> %s" % TEST_DOMAIN_NGINX_CONFIG)
+
+    domain_add(TEST_DOMAIN)
+
+    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()