Invalid link target for enabling a module reports false instead of true

mtdeguzis commented 5 years ago

Describe the bug

When enabling a module, if a bad link is within /etc/icingaweb2/enabledModules, you will get a logger warning. I see this when building a new node out, but only on the first start/init, and not after. I was able to reproduce it in a way.

To Reproduce

Provide a link to a live example, or an unambiguous set of steps to reproduce this issue. Include configuration, logs, etc. to reproduce, if relevant.

[root@udaicinga-dev /]# ln -s /nothing /etc/icingaweb2/enabledModules/batman
[root@udaicinga-dev /]# ls -la /etc/icingaweb2/enabledModules                                                                                                            
total 8
drwxrws---. 2 icinga icingaweb2 4096 Nov 21 14:05 .
drwxrws---. 6 icinga icingaweb2 4096 Nov 20 15:48 ..
lrwxrwxrwx. 1 icinga icingaweb2   35 Nov 20 15:27 audit -> /usr/share/icingaweb2/modules/audit
lrwxrwxrwx. 1 root   icingaweb2    8 Nov 21 14:05 batman -> /nothing
lrwxrwxrwx. 1 icinga icingaweb2   38 Nov 20 15:01 director -> /usr/share/icingaweb2/modules/director
lrwxrwxrwx. 1 icinga icingaweb2   33 Nov 20 15:27 doc -> /usr/share/icingaweb2/modules/doc
-rwxrwx---. 1 icinga icingaweb2    0 Nov 20 14:29 .gitkeep
lrwxrwxrwx. 1 icinga icingaweb2   38 Nov 21 09:50 graphite -> /usr/share/icingaweb2/modules/graphite
lrwxrwxrwx. 1 icinga icingaweb2   39 Nov 20 15:27 incubator -> /usr/share/icingaweb2/modules/incubator
lrwxrwxrwx. 1 icinga icingaweb2   33 Nov 20 15:27 ipl -> /usr/share/icingaweb2/modules/ipl
lrwxrwxrwx. 1 icinga icingaweb2   40 Nov 20 15:01 monitoring -> /usr/share/icingaweb2/modules/monitoring
lrwxrwxrwx. 1 icinga icingaweb2   41 Nov 20 15:27 reactbundle -> /usr/share/icingaweb2/modules/reactbundle
lrwxrwxrwx. 1 icinga icingaweb2   35 Nov 20 15:27 setup -> /usr/share/icingaweb2/modules/setup
lrwxrwxrwx. 1 icinga icingaweb2   40 Nov 20 15:22 theme-dark -> /usr/share/icingaweb2/modules/theme-dark

[root@udaicinga-dev /]# icingacli module enable batman                                                                                                                   
Found invalid module in enabledModule directory "/etc/icingaweb2/enabledModules": "/etc/icingaweb2/enabledModules/batman" points to non existing path "false"

Expected behavior

The message should be

Found invalid module in enabledModule directory "/etc/icingaweb2/enabledModules": "/etc/icingaweb2/enabledModules/batman" points to non existing path "true"

.. or a better message sent to the user.

Your Environment

Include as many relevant details about the environment you experienced the problem in

Icinga Web 2 version and modules (System - About):
Web browser used: Chrome

Icinga 2 version used (icinga2 --version):

# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: 2.11.0-1)

PHP version used (php --version):

/opt/rh/rh-php71/root/usr/sbin/php-fpm -v
PHP 7.1.30 (fpm-fcgi) (built: Jul 10 2019 09:46:42)
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies

Server operating system and version: RHEL 7.8

Additional context

Add any other context about the problem here.

nilmerg commented 5 years ago

Hi, you want a true instead of a false, is this right?

Found invalid module in enabledModule directory "/etc/icingaweb2/enabledModules": "/etc/icingaweb2/enabledModules/batman" points to non existing path "~~false~~true"

What is better in this case? Or am I missing something here?

mtdeguzis commented 5 years ago

I'm not really clear on intention as it stands, the wording is a double negative. I'm not really sure what their desired state is or what it actually is when the error is thrown.

Off the cuff I'd think 'pointing to a non-existing path 'false' would be a good thing. I.E. the path does exist.

If the error is to be constructed in %s string pieces like this, I would think you would want true, as in "points to non-existing path, true", or drop the true/false entirely, as the statement is telling you the path it points to does not exist and is only thrown at the end of the logic check.

nilmerg commented 5 years ago

Oh come on. Of course false is just what the result is after resolving the symbolic link. false just means the link points to nothing. There may be another reason for why a link is invalid (a link pointing to a file) and that is why we just include the result in the log message. Sure, we may omit the result if it is false. Though, to be honest, I guess this gets changed faster if you do it by yourself in a PR.

Icinga / icingaweb2