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
