code cleanup #75

Manually merged
benpro merged 78 commits from cleanup into fast-debian-check 2019-04-05 10:59:26 +02:00
Owner

big cleanup

  • tests organization
  • shellcheck conventions
  • variables extractions
  • quotes and indentations
big cleanup * tests organization * shellcheck conventions * variables extractions * quotes and indentations
Author
Owner

This pull-request is too big and nearly impossible to review 😞

I'll make a script to deploy the new script and compare the results with the current version, on a lot of servers. This will most probably show some regressions.

This pull-request is too big and nearly impossible to review :disappointed: I'll make a script to deploy the new script and compare the results with the current version, on a lot of servers. This will most probably show some regressions.
Contributor

This pull-request is too big and nearly impossible to review

Yes indeed, and it is more than cleanup, you have rewritten some logic that could add regression.
I would suggest you to divide your work and add X merge requests.
Let's say 5 checks by 5 checks?

> This pull-request is too big and nearly impossible to review Yes indeed, and it is more than cleanup, you have rewritten some logic that could add regression. I would suggest you to divide your work and add X merge requests. Let's say 5 checks by 5 checks?
Author
Owner

I already have a system in place to do the regression testing. I'll run in on all our servers.

I already have a system in place to do the regression testing. I'll run in on all our servers.
benpro reviewed 2019-03-22 14:36:55 +01:00
evocheck.sh Outdated
@ -397,0 +447,4 @@
fi
else
if test -e /etc/apache2/apache2.conf; then
(grep --quiet --no-messages --extended-regexp "^env.url.*/server-status-[[:alnum:]]{4,}" /etc/munin/plugin-conf.d/munin-node \
Contributor

The famous debate about long or short args.
grep -qsE

Everywhere in the script, it is short args. :)

The famous debate about long or short args. `grep -qsE` Everywhere in the script, it is short args. :)
evocheck.sh Outdated
@ -441,3 +506,3 @@
# Verification de l'activation de Squid dans le cas d'un pack mail
if [ "$IS_SQUID" = 1 ]; then
squidconffile=/etc/squid*/squid.conf
squidconffile="/etc/squid*/squid.conf"
Contributor

Using quote drop the globing support!

Using quote drop the globing support!
Author
Owner

It seems OK to me :

# squidconffile="/etc/squid*/squid.conf"
# ls ${squidconffile}
/etc/squid/squid.conf

It fails if we quote the variable inside the grep command.

It seems OK to me : ``` # squidconffile="/etc/squid*/squid.conf" # ls ${squidconffile} /etc/squid/squid.conf ``` It fails if we quote the variable inside the `grep` command.
evocheck.sh Outdated
@ -470,2 +543,3 @@
if [ "$IS_LOG2MAILRUNNING" = 1 ]; then
is_pack_web && (is_installed log2mail && pgrep log2mail >/dev/null || echo 'IS_LOG2MAILRUNNING')
if is_pack_web && is_installed log2mail; then
pgrep log2mail >/dev/null || failed 'IS_LOG2MAILRUNNING'
Contributor

Don't know why it was pgrep here...

Don't know why it was `pgrep` here...
evocheck.sh Outdated
@ -478,2 +553,3 @@
fi
is_pack_web && ( is_installed log2mail && grep -q "^file = /var/log/apache2/error.log" $conf 2>/dev/null || failed "IS_LOG2MAILAPACHE" )
if is_pack_web && is_installed log2mail; then
grep -q "^file = /var/log/apache2/error.log" $conf 2>/dev/null \
Contributor

-s in place of redirecting to /dev/null.

`-s` in place of redirecting to /dev/null.
evocheck.sh Outdated
@ -575,2 +680,3 @@
if [ "$IS_SAMBAPINPRIORITY" = 1 ]; then
is_pack_samba && grep -qrE "^[^#].*backport" /etc/apt/sources.list{,.d} && ( priority=`grep -E -A2 "^Package:.*samba" /etc/apt/preferences |grep -A1 "^Pin: release a=lenny-backports" |grep "^Pin-Priority:" |cut -f2 -d" "` && test $priority -gt 500 || failed "IS_SAMBAPINPRIORITY" )
if is_pack_samba; then
grep -qrE "^[^#].*backport" /etc/apt/sources.list{,.d} && ( priority=$(grep -E -A2 "^Package:.*samba" /etc/apt/preferences |grep -A1 "^Pin: release a=lenny-backports" |grep "^Pin-Priority:" |cut -f2 -d" ") && test $priority -gt 500 || failed "IS_SAMBAPINPRIORITY" )
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -670,1 +797,3 @@
&& grep -q '^%evolinux-sudo ALL=(ALL:ALL) ALL' /etc/sudoers.d/evolinux) || failed "IS_EVOLINUXSUDOGROUP"
(grep -q "^evolinux-sudo:" /etc/group \
&& grep -q '^%evolinux-sudo ALL=(ALL:ALL) ALL' /etc/sudoers.d/evolinux) \
|| failed "IS_EVOLINUXSUDOGROUP"
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some if; then for better readability. As you has done with other.
evocheck.sh Outdated
@ -685,2 +814,3 @@
&& test -L /etc/apache2/conf-enabled/zzz-evolinux-custom.conf \
&& test -f /etc/apache2/ipaddr_whitelist.conf) || failed "IS_APACHE2EVOLINUXCONF"
&& test -f /etc/apache2/ipaddr_whitelist.conf) \
|| failed "IS_APACHE2EVOLINUXCONF"
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some if; then for better readability. As you has done with other.
evocheck.sh Outdated
@ -701,3 +831,3 @@
if [ "$IS_BIND9MUNIN" = 1 ]; then
if is_debian_stretch && is_installed bind9; then
(test -L /etc/munin/plugins/bind9 && test -e /etc/munin/plugin-conf.d/bind9) || failed "IS_BIND9MUNIN"
(test -L /etc/munin/plugins/bind9 && test -e /etc/munin/plugin-conf.d/bind9) \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -713,3 +844,3 @@
if [ "$IS_BROADCOMFIRMWARE" = 1 ]; then
if lspci | grep -q 'NetXtreme II'; then
(is_installed firmware-bnx2 && grep -q "^deb http://mirror.evolix.org/debian.* non-free" /etc/apt/sources.list) || failed "IS_BROADCOMFIRMWARE"
(is_installed firmware-bnx2 && grep -q "^deb http://mirror.evolix.org/debian.* non-free" /etc/apt/sources.list) \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -722,0 +855,4 @@
fi
if lspci | grep -q 'Hewlett-Packard Company Smart Array'; then
is_installed cciss-vol-status \
|| failed "IS_HARDWARERAIDTOOL"
Contributor

useless break line

useless break line
evocheck.sh Outdated
@ -724,3 +862,3 @@
if [ "$IS_LOG2MAILSYSTEMDUNIT" = 1 ]; then
if is_debian_stretch; then
(systemctl -q is-active log2mail.service && test -f /etc/systemd/system/log2mail.service && ! test -f /etc/init.d/log2mail) || failed "IS_LOG2MAILSYSTEMDUNIT"
(systemctl -q is-active log2mail.service && test -f /etc/systemd/system/log2mail.service && ! test -f /etc/init.d/log2mail) \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -729,3 +868,3 @@
if [ "$IS_LISTUPGRADE" = 1 ]; then
(test -f /etc/cron.d/listupgrade && test -x /usr/share/scripts/listupgrade.sh) || failed "IS_LISTUPGRADE"
(test -f /etc/cron.d/listupgrade && test -x /usr/share/scripts/listupgrade.sh) \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -802,3 +947,3 @@
if [ "$IS_MARIADBSYSTEMDUNIT" = 1 ]; then
if is_debian_stretch && is_installed mariadb-server; then
(systemctl -q is-active mariadb.service && test -f /etc/systemd/system/mariadb.service.d/evolinux.conf) || failed "IS_MARIADBSYSTEMDUNIT"
(systemctl -q is-active mariadb.service && test -f /etc/systemd/system/mariadb.service.d/evolinux.conf) \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -830,1 +974,3 @@
&& grep -q -F "command[check_mysql]=/usr/lib/nagios/plugins/check_mysql -H localhost -f ~nagios/.my.cnf") || failed "IS_MYSQLNRPE"
&& [ "$(stat -c %U ~nagios/.my.cnf)" = "nagios" ] \
&& [ "$(stat -c %a ~nagios/.my.cnf)" = "600" ] \
&& grep -q -F "command[check_mysql]=/usr/lib/nagios/plugins/check_mysql -H localhost -f ~nagios/.my.cnf") \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
evocheck.sh Outdated
@ -835,3 +982,3 @@
if is_debian_stretch && is_installed php; then
(test -f /etc/php/7.0/cli/conf.d/z-evolinux-defaults.ini \
&& test -f /etc/php/7.0/cli/conf.d/zzz-evolinux-custom.ini) || failed "IS_PHPEVOLINUXCONF"
&& test -f /etc/php/7.0/cli/conf.d/zzz-evolinux-custom.ini) \
Contributor

This check should be using some if; then for better readability. As you has done with other.

This check should be using some `if; then` for better readability. As you has done with other.
Author
Owner

@benpro I need explanation for this portion of code, from the current master branch :

test -e /etc/apache2/apache2.conf && (
    is_debian_stretch || (
        grep -E -q "^env.url.*/server-status-[[:alnum:]]{4}" /etc/munin/plugin-conf.d/munin-node \
        && grep -E -q "/server-status-[[:alnum:]]{4}" /etc/apache2/apache2.conf \
        || grep -E -q "/server-status-[[:alnum:]]{4}" /etc/apache2/apache2.conf /etc/apache2/mods-enabled/status.conf 2>/dev/null\
        || failed "IS_APACHEMUNIN"
    )
)

I don't really understand what situation must trigger the failed call.

@benpro I need explanation for this portion of code, from the current master branch : ```bash test -e /etc/apache2/apache2.conf && ( is_debian_stretch || ( grep -E -q "^env.url.*/server-status-[[:alnum:]]{4}" /etc/munin/plugin-conf.d/munin-node \ && grep -E -q "/server-status-[[:alnum:]]{4}" /etc/apache2/apache2.conf \ || grep -E -q "/server-status-[[:alnum:]]{4}" /etc/apache2/apache2.conf /etc/apache2/mods-enabled/status.conf 2>/dev/null\ || failed "IS_APACHEMUNIN" ) ) ``` I don't really understand what situation must trigger the `failed` call.
Contributor

I think it is:
If it is not a stretch we check that server-status with 4 digits is properly configured in /etc/munin/plugin-conf.d/munin-node and /etc/apache2/apache2.conf or /etc/apache2/mods-enabled/status.conf

I think it is: If it is not a stretch we check that server-status with 4 digits is properly configured in `/etc/munin/plugin-conf.d/munin-node` and `/etc/apache2/apache2.conf` or `/etc/apache2/mods-enabled/status.conf`
Author
Owner

@benpro the current tests were failing when the server-status was defined in another file in /etc/munin/plugin-conf.d/ hence the weird failures in the master branch. And we didn't see it probably because there was a logic error in the nested parenthesis.

I've fixed this in 67765a03c4 and d814936446

@benpro the current tests were failing when the server-status was defined in another file in `/etc/munin/plugin-conf.d/` hence the weird failures in the master branch. And we didn't see it probably because there was a logic error in the nested parenthesis. I've fixed this in 67765a03c4 and d814936446
Author
Owner

I've done a ton of small fixes.
Now even shellcheck is happy (with 5 or 6 specific style warnings disabled).

I've done a ton of small fixes. Now even shellcheck is happy (with 5 or 6 specific style warnings disabled).
Author
Owner

I've added an option parser, just like evomaintenance.

# evocheck.sh --help
evocheck is a script that verifies Evolix conventions on Debian/OpenBSD servers.

Usage: evocheck
  or   evocheck --cron
  or   evocheck --quiet
  or   evocheck --verbose

Options
     --cron                  disable a few checks
 -v, --verbose               increase verbosity of checks
 -q, --quiet                 nothing is printed on stdout nor stderr
 -h, --help                  print this message and exit
     --version               print version and exit
I've added an option parser, just like evomaintenance. ``` # evocheck.sh --help evocheck is a script that verifies Evolix conventions on Debian/OpenBSD servers. Usage: evocheck or evocheck --cron or evocheck --quiet or evocheck --verbose Options --cron disable a few checks -v, --verbose increase verbosity of checks -q, --quiet nothing is printed on stdout nor stderr -h, --help print this message and exit --version print version and exit ```
benpro was assigned by jlecour 2019-03-23 02:38:41 +01:00
benpro added the
enhancement
label 2019-04-04 16:59:20 +02:00
benpro approved these changes 2019-04-04 17:52:10 +02:00
evocheck.sh Outdated
@ -373,1 +502,3 @@
test -d /etc/nagios && ls -ld /etc/nagios | grep -q drwxr-x--- || failed "IS_NRPEPERMS"
if [ -d /etc/nagios ]; then
actual=$(stat --format "%A" /etc/nagios)
expected="drwxr-x---"
Contributor

Check with numbers.

Check with numbers.
evocheck.sh Outdated
@ -397,0 +543,4 @@
&& test -h /etc/munin/plugins/apache_accesses \
&& test -h /etc/munin/plugins/apache_processes \
&& test -h /etc/munin/plugins/apache_volume; } \
|| failed "IS_APACHEMUNIN" "mising munin plugins for Apache"
Contributor

missing

missing
evocheck.sh Outdated
@ -493,2 +678,2 @@
if [ "$(md5sum /usr/sbin/named |cut -f 1 -d ' ')" != "$(md5sum /var/chroot-bind/usr/sbin/named |cut -f 1 -d ' ')" ]; then
failed "IS_BINDCHROOT"
if is_installed bind9; then
if netstat -utpln | grep "/named" | grep :53 | grep -qvE "(127.0.0.1|::1)"; then
Contributor

ss inplace of netstat

ss inplace of netstat
Contributor

using lsof -i :53 in place of netstat….

using `lsof -i :53` in place of `netstat…`.
evocheck.sh Outdated
@ -577,0 +793,4 @@
priority=$(grep -E -A2 "^Package:.*samba" /etc/apt/preferences | grep -A1 "^Pin: release a=.*-backports" | grep "^Pin-Priority:" | cut -f2 -d" ")
test "$priority" -gt 500 || failed "IS_SAMBAPINPRIORITY"
fi
fi
Contributor

Limited to Lenny.

Limited to Lenny.
evocheck.sh Outdated
@ -677,1 +947,3 @@
groups $user |grep -q adm || failed "IS_USERINADMGROUP"
users=$(grep "^evolinux-sudo:" /etc/group | awk -F: '{print $4}' | tr ',' ' ')
for user in $users; do
groups "$user" | grep -q adm || failed "IS_USERINADMGROUP" "User $user doesn't belong to \`adm' group"
Contributor

Maybe add a break.

Maybe add a break.
evocheck.sh Outdated
@ -1061,2 +1380,3 @@
if [ "$IS_TMP_1777" = 1 ]; then
ls -ld /tmp | grep -q drwxrwxrwt || failed "IS_TMP_1777"
actual=$(stat --format "%A" /tmp)
expected="drwxrwxrwt"
Contributor

Numbers.

Numbers.
evocheck.sh Outdated
@ -1065,2 +1386,3 @@
if [ "$IS_ROOT_0700" = 1 ]; then
ls -ld /root | grep -q drwx------ || failed "IS_ROOT_0700"
actual=$(stat --format "%A" /root)
expected="drwx------"
Contributor

Numbers

Numbers
evocheck.sh Outdated
@ -1069,2 +1392,3 @@
if [ "$IS_USRSHARESCRIPTS" = 1 ]; then
ls -ld /usr/share/scripts | grep -q drwx------ || failed "IS_USRSHARESCRIPTS"
actual=$(stat --format "%A" /usr/share/scripts)
expected="drwx------"
Contributor

Numbers

Numbers
Author
Owner

@benpro I've commmited the changes I was comfortable doing (ie. all of them but the "netstat/ss" replacement and "lsof").

As far as I cal tell, it's good to merge.

@benpro I've commmited the changes I was comfortable doing (ie. all of them but the "netstat/ss" replacement and "lsof"). As far as I cal tell, it's good to merge.
jlecour changed title from WIP: code cleanup to code cleanup 2019-04-04 18:35:11 +02:00
Author
Owner

there are conflicts, but I guess it's going to be mergeable when #70 is merged, or i'll deal with the remaining conflicts

there are conflicts, but I guess it's going to be mergeable when #70 is merged, or i'll deal with the remaining conflicts
benpro closed this pull request 2019-04-05 10:59:26 +02:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: evolix/evocheck#75
No description provided.