Service module has conflicted configuration files #5903

Closed
FTBZ opened this Issue Feb 15, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@FTBZ
Contributor

FTBZ commented Feb 15, 2017

Component Version
LibreNMS 3fd56d7
DB Schema 164
PHP 7.0.11
MySQL 5.5.50-MariaDB
RRDTool 1.4.8
SNMP NET-SNMP 5.7.2

Service module has conflicted configuration files

I noticed that the service module has some conflict in the files that make some variables to be ignored. To explain my problem, we're using this example only the Nagios check_http module.

When configuring LibreNMS to check a service that's NOT linked to a device, I noticed that the field "IP Address" is ignored with the check_http module and not with the check_ldap module.

Browsing the source code to understand my problem I noticed this problem:

  • librenms/includes/services.inc.php has a way to handle services that have not a specific file present in librenms/includes/services/ (line 128)
  • The code presents at this line include the ability to use the IP address field
  • In the other hand, the file check_http.inc.php ignores the IP variable field

services.inc.php code:
$config['nagios_plugins'] . "/check_" . $service['service_type'] . " -H " . ($service['service_ip'] ? $service['service_ip'] : $service['hostname']);

check_http.inc.php code:
$config['nagios_plugins'] . "/check_http -H ".$service['hostname']." ".$service['service_param'];

So the question is why using custom files for services that's compatible with the "standard" method present on line 128 using the -H argument? This is redundant?

To correct this problem we need to implement the $ip variable to all the files in librenms/includes/services/ or to remove the files that use exactly the same format at the line 128 I think. If technically there is no reason to use a separated file, I prefer the second method.

Perhaps @adaniels21487 can help for this?

@laf laf added the Bug 🐞 label Feb 15, 2017

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Feb 17, 2017

Contributor

So, without more advice I will PR the second solution. @InsaneSplash (Latest committer) can give its opinion before?

Contributor

FTBZ commented Feb 17, 2017

So, without more advice I will PR the second solution. @InsaneSplash (Latest committer) can give its opinion before?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 17, 2017

Member

I gave you my advice last night on irc :/

Member

laf commented Feb 17, 2017

I gave you my advice last night on irc :/

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Feb 17, 2017

Contributor

Sure, this is why I say that I will PR the second solution. But I just want the opinion of @InsaneSplash because we have talked together about her last commits to these files.

Contributor

FTBZ commented Feb 17, 2017

Sure, this is why I say that I will PR the second solution. But I just want the opinion of @InsaneSplash because we have talked together about her last commits to these files.

@adaniels21487

This comment has been minimized.

Show comment
Hide comment
@adaniels21487

adaniels21487 Feb 20, 2017

Contributor

Sorry, I'm a little late to the party...
The code that's there came from the original, forked codebase. All I added was the statistics collection.
If the computed $check_cmd doesn't differ from the default one, I see no need to keep the separate files.

Thanks,
Aaron

Contributor

adaniels21487 commented Feb 20, 2017

Sorry, I'm a little late to the party...
The code that's there came from the original, forked codebase. All I added was the statistics collection.
If the computed $check_cmd doesn't differ from the default one, I see no need to keep the separate files.

Thanks,
Aaron

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Feb 23, 2017

Contributor

Fix was merged. Closing.

Contributor

FTBZ commented Feb 23, 2017

Fix was merged. Closing.

@FTBZ FTBZ closed this Feb 23, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@librenms librenms locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.