Control: tags -1 moreinfo Pirate Praveen: > Package: release.debian.org > Severity: normal > User: release.debian....@packages.debian.org > Usertags: unblock > > Please unblock package gitlab > > This fixes RC bug #857967. Also sine the version in stretch is currently > not installable, please reduce the age. > > debdiff with 8.13.11+dfsg-5 attached (changes upto this version is > already approved) > > unblock gitlab/8.13.11+dfsg-6 > > [...]
Hi Pirate, Sorry for the delay in getting back to you. I have reviewed gitlab and I got some remarks. Please note that some of these may be remarks that we should have noticed in earlier reviews. Apologies for overlooking them previously if this is the case, but I decided to review some of the files from the original source files rather than the diff. * In the postrm (RC bug): """ su ${gitlab_user} -c 'psql gitlab_production -c ""' && \ su postgres -c "dropdb gitlab_production" """ This does not appear to be idempotent. If the database is dropped but a later part of the purge fails, then this line will prevent dpkg from rerunning the purge and reach the desired state (as the code is run under "set -e") You probably want something like: """ if su ${gitlab_user} -c 'psql gitlab_production -c ""'; then su postgres -c "dropdb gitlab_production" fi """ A similar remark can be made for the following code where it would be prudent to apply the same fix: """ test -e ${gitlab_log_dir} && rm -rf ${gitlab_log_dir} test -e ${gitlab_cache_path} && rm -rf ${gitlab_cache_path} test -e ${gitlab_pid_path} && rm -rf ${gitlab_pid_path} test -e ${gitlab_data_dir} && rm -rf ${gitlab_data_dir} [...] id -u ${gitlab_user} && userdel -r ${gitlab_user} [...] test -f ${nginx_site} && echo "Found nginx site...." [...] # remove the configuration file itself test -f ${nginx_site} && rm -f ${nginx_site} test -f ${gitlab_debian_conf} && rm -f ${gitlab_debian_conf} test -f ${gitlab_yml} && rm -f ${gitlab_yml} test -f ${gitlab_tmpfiles} && rm -f ${gitlab_tmpfiles} test -f ${gitlab_shell_config} && rm -f ${gitlab_shell_config} [...] test -n "${nginx_site}" && ucf --purge ${nginx_site} test -n "${gitlab_debian_conf}" && ucf --purge ${gitlab_debian_conf} test -n "${gitlab_yml}" && ucf --purge ${gitlab_yml} test -n "${gitlab_tmpfiles}" && ucf --purge ${gitlab_tmpfiles} test -n "${gitlab_shell_config}" && ucf -purge ${gitlab_shell_config} [...] test -n "${nginx_site}" && ucfr --purge gitlab ${nginx_site} test -n "${gitlab_debian_conf}" && ucfr --purge gitlab ${gitlab_debian_conf} test -n "${gitlab_yml}" && ucfr --purge gitlab ${gitlab_yml} test -n "${gitlab_tmpfiles}" && ucfr --purge gitlab ${gitlab_tmpfiles} test -n "${gitlab_shell_config}" && ucfr -purge gitlab ${gitlab_shell_config} """ Note I got a follow up remark for the "id+userdel" line below. As for the "test -n", these mean that the script will fail if these values are not (still) present in the config OR worse - if the gitlab config is already deleted (this is also listed a separate item below) * debian/postrm (RC bug): The postrm will fail if the admin removes the gitlab config files prior to purging gitlab. This happens because then most of the variables are set and as shown above, this will make several test statements evaluate to false on the left-hand-side of an && under "set -e". Admittedly, this is partly mitigated by gitlab providing its own default as a copy under /var/lib/gitlab/gitlab-debian.defaults. But in theory, the machine could fail after removing "/var/lib/gitlab/gitlab-debian.defaults" but prior to dpkg committing that it had purged gitlab. It is an unlikely corner-case, but as it leaves the admin with an unpurgeable package it is RC. To my knowledge, it is *not* sufficient to make the removal of the /var/lib file the last thing. That just narrows the window for the issue without fixing it. * debian/postrm (important) As I recall, the general consensus on handling of system users is that we should lock them rather than remove them. The gitlab postrm deletes the user: """ id -u ${gitlab_user} && userdel -r ${gitlab_user} """ That said, I believe gitlab is not the only package deleting the user, so it is not RC. * debian/config (important/RC bug): The config file uses the debconf database as a registry. It should "pre-seed" itself with the defaults from the configuration files. See "man 8 debconf-devel" under "config file handling": https://manpages.debian.org/jessie/debconf-doc/debconf-devel.7.en.html#ADVANCED_PROGRAMMING_WITH_DEBCONF * debian/postinst (RC bug): """ # Override User for systemd services for service in mailroom unicorn sidekiq workhorse; do path=/etc/systemd/system/gitlab-${service}.service.d mkdir -p $path printf "[Service]\nUser=${gitlab_user}\n" > $path/override.conf done """ This part appears to override "/etc/systemd/system/gitlab-${service}.service.d/override.conf" completely, disregarding an existing file the admin may have created. AFAICT, this happens on "configure" (i.e. it will also apply to upgrades) - but even if it only happened on first install, it would break admins who use puppet to push out a config file before installing gitlab. There is a similar snippet in the postrm, but it occurs only on purge, so that should be fine. * debian/postinst (minor <-> RC bug, not sure): If the postinst script is rerun with a new gitlab user, then the references in "${gitlab_debian_conf_private}" and "${gitlab_yml_private}" is not updated. I suspect we are in the "RC Bug" level because that means the postinst will use the wrong user when using ucf to compare the "${gitlab_yml_private}" config with the actual config. * debian/{postinst,postrm} (possible RC bug): I am not entirely confident that the handling of gitlab user is sound in case the admin uses a non-standard gitlab user and removes the config file prior to purging. AFAICT, it will cause the postrm to attempt to remove the "gitlab" user (which is the default name) rather than the actual user. Even if the postrm would gracefully handle that the default user does not exist, then we still leave the system with an open system account that should have been locked/deleted. I wish I could say I had a proper solution for that, but I don't. I am really not sure how to handle that gracefully AND correctly at the same time. Worst case, the best/only option may be to undo making the gitlab user name customizable. At the very least, it is a lot more complex than we initially thought. * debian/README.Debian (minor): This file uses "export $(cat /etc/gitlab/gitlab-debian.conf)" in at least 4 places, which will fail if there are comments in the config file. It would be nice if those were fixed to source the file instead like the maintainer scripts does. I must admit, I am not entirely confident that I found all the possible issues in the maintainer scripts. I would highly recommend a review from another person once the above have been fixed. Thanks, ~Niels