Use fping6 instead of fping for hosts with udp6/tcp6 SNMP transports #1959

Merged
merged 7 commits into from Sep 27, 2015

Projects

None yet

5 participants

@n-st
Contributor
n-st commented Sep 22, 2015

LibreNMS currently uses fping to try and ping hosts, even in cases where they have been configured to use SNMP via IPv6 and might not even have a public IPv4 address.
The code changed in this PR will detect the address family (IPv4 or IPv6) used for the SNMP connection and select fping or fping6 accordingly.

@n-st n-st Use fping6 instead of fping for hosts with udp6/tcp6 SNMP transports
ac00119
@laf
Member
laf commented Sep 22, 2015

Pasting irc comments in here:

08:26 @laf n-st: The fping6 PR, I think we need to tweak this slightly
08:26 @laf The issue is for CentOS users the default path to fping6 is /usr/sbin/fping6
08:27 @blahdeblah laf: if fping defaults /usr/bin/, then it seems reasonable to default fping6 to there as well...
08:27 @blahdeblah laf: But maybe deriving one from the other if fping6 is not set is reasonable also
08:27 @laf My thoughts on tackling this would be to remove fping6 from defaults.inc.php then when calling the fping binary, if it's IPv6 and $config['fping6'] isn't defined, use $config['fping'].'6'. It's not ideal but maintains compatibility for Centos
08:27 @laf blahdeblah: ^^
08:28 @laf blahdeblah: Also In docs for CentOS we explicitly tell people to update that config option so that's not an issue
08:28 @laf The issue is if we default to turning fping6 on, for anyone on Centos who uses it all v6 hosts will show as down as fping6 won't execute until they add $config['fping6'] to config.php - this isn't ideal
08:29 @blahdeblah laf: I would be more inclined to set $config['fping6'] = $config['fping'].'6' in defaults.inc.php...
08:29 @blahdeblah (Although, that may not work)
08:30 @laf blahdeblah: That wouldn't work for centos still as fping is defined later in config.php

@n-st n-st Use relative path for fping6
fping6 will be in /usr/bin on Debian-based systems, but /usr/sbin on CentOS.
Have PHP figure out where fping6 is, to avoid breaking existing setups on CentOS.
772d176
@n-st n-st Fixed typo in variable name
4c2d667
@laf
Member
laf commented Sep 22, 2015

👍 from me.

@paulgear can I get a quick check on this to be sure.

@paulgear paulgear was assigned by laf Sep 22, 2015
@f0o
Member
f0o commented Sep 23, 2015

Blocker: snmp-scan.php also uses isPingable(), as you introduce a new parameter in the middle, please adjust the call there.

Side-Note: I'm not 100% happy with not using full path to the executable. I think we should ship full paths and encourage to keep them absolute. If you prefer relative or shell-expansion you can feel free to do so in your config regardless of the defaults. But that's just my 2 cents.

Apart from above's blocker it's 👍 from me as well

//Edit: Would it be too much to ask to add a docblock around the isPingable() function? We've to gradually start completing these and as you just touched it, it might be a good idea to include it then - Dont want to sound naggy hehe

@n-st
Contributor
n-st commented Sep 23, 2015

@f0o

Blocker: snmp-scan.php also uses isPingable(), as you introduce a new parameter in the middle, please adjust the call there.

The new parameter to isPingable() has a default (AF_INET, i.e. IPv4), just like the now-third parameter does, so there's no need to adjust calls that don't use the second or third parameter.

Side-Note: I'm not 100% happy with not using full path to the executable. I think we should ship full paths and encourage to keep them absolute.

We had a discussion about that in IRC (see below) and it seems like relative paths are the only way to introduce this without breaking deployments on CentOS (due to not having any kind of release notes to warn the user or post-update script to search for the fping6 binary).

Now while I haven't found anything about default search paths in PHP, the docs I've seen and tests I've run indicate that PHP does at least find binaries in /usr/bin and /usr/sbin, so it should be safe to use non-absolute paths as defaults.

Would it be too much to ask to add a docblock around the isPingable() function?

I haven't used PHPDoc before (I last used PHP when I was still a teenager with no idea about programming style), but I'll look into it.


Sep 22 14:29:16 <n-st>  regarding https://github.com/librenms/librenms/pull/1959: why do we hardcode absolute paths anyway?
Sep 22 14:29:55 <laf>   n-st: Hey. You mean why /usr/bin/fping? If so, what do you propose instead?
Sep 22 14:30:47 <n-st>  laf: can't we just use 'fping' and have php figure out where it is?
Sep 22 14:32:49 <laf>   n-st: How do you propose we get php to figure it out?
Sep 22 14:33:11 <n-st>  https://secure.php.net/manual/en/function.shell-exec.php
Sep 22 14:33:27 <n-st>  looks to me like it should be using $PATH automatically
Sep 22 14:33:33 <n-st>  haven't tested it, though
Sep 22 14:34:34 <n-st>  php -r 'echo shell_exec("fping -h");'
Sep 22 14:34:37 <n-st>  works
Sep 22 14:34:55 <laf>   n-st: We use proc_open, not sure if that's the same
Sep 22 14:35:17 <n-st>  laf: ah, i was looking at snmp_get()
Sep 22 14:35:18 <f0o>   n-st / laf : I wouldnt rely on $PATH expansion with proc_open
Sep 22 14:35:47 <laf>   n-st / f0o Possibly a reason why it was hardcoded in the first place
Sep 22 14:36:00 <f0o>   I'd guess so
Sep 22 14:37:33 <n-st>  laf: is there any reason for using shell_exec in one place and proc_open in another?
Sep 22 14:38:04 <laf>   n-st: We inherited a lot of 'lovely' code :)
Sep 22 14:51:12 <n-st>  laf: just checked: librenms works fine with $config['fping'] = 'fping'; $config['fping6'] = 'fping6';
Sep 22 14:51:55 <n-st>  https://secure.php.net/manual/en/function.proc-open.php also doesn't say anything about requiring an absolute path, so i guess relative paths should be a reasonable default
Sep 22 14:52:04 <laf>   n-st: I'll be honest and say I'd be nervous pushing that out, this is one of those updates where we have limited testing and if it goes wrong then people lose data
Sep 22 14:52:46 <laf>   n-st: I'd guess we can just do $config['fping6'] = 'fping6'; That would limit the issues - leave fping as is
Sep 22 14:53:27 <n-st>  sure
Sep 22 14:53:47 <n-st>  perhaps we could/should also implement a check if all required programs are executable, and generate an alert otherwise
Sep 22 14:54:17 <laf>   n-st: Kind of building upto that in validate.php, it checks fping is accessible but no others at present
Sep 22 14:55:11 <n-st>  by the way, doc/Support/Configuration.md has been containing the line $config['fping6']           = "/usr/bin/fping6"; for several months now (since you committed it on 2015-05-04 13:51:46, to be exact) ;)
Sep 22 14:55:55 <n-st>  so yeah, we've got a wee bit of inconsistency going on here ;)
Sep 22 14:57:17 <laf>   All the config options were taken from includes/defaults.inc.php, fping6 has since been removed from defaults.inc.php by someone else
Sep 22 14:58:53 <n-st>  …
Sep 22 14:59:25 <n-st>  so… relative paths or the workaround you suggested at half past 8 (i.e. $config['fping'].'6')?
Sep 22 15:00:17 <n-st>  laf: ↑
Sep 22 15:01:10 <laf>   Go for $config['fping6'] = 'fping6';
Sep 22 15:01:32 <n-st>  laf: done
@f0o
Member
f0o commented Sep 23, 2015

@n-st
RE: Param: Sorry my bad, you're correct 👍
RE: Paths: as I said it's just a personal preference, not an issue let alone a blocker.

On the DocBlock here a template:

/**
 * Function Name
 * @param type $name Description
 * @return type
 */
function foobar($name) {
...
}
n-st added some commits Sep 24, 2015
@n-st n-st Added PHPDoc for function isPingable() 7d7e709
@n-st n-st Code formatting
e532759
@paulgear
Member

I think @n-st and @f0o have covered everything in their discussions, and I'm happy to merge.

One question: why are we documenting an unused parameter rather than just removing it?

@n-st
Contributor
n-st commented Sep 24, 2015

@paulgear Because @f0o only wanted documentation, not cleanup. ;)

Seriously though, I would want to dig through the history to check where that parameter came from before removing it, and I simply couldn't be bothered to do that today, so it's still here for the moment.

@laf
Member
laf commented Sep 25, 2015

👍 from me :)

@f0o f0o and 2 others commented on an outdated diff Sep 25, 2015
includes/functions.php
@@ -1230,4 +1246,17 @@ function function_check($function) {
function get_last_commit() {
return `git log -n 1|head -n1`;
-}//end get_last_commit
+}//end get_last_commit
+
+function snmpTransportToAddressFamily($transport) {
@f0o
f0o Sep 25, 2015 Member

DocBlock here too please :)

Happy to merge it after this :)

@laf
laf Sep 25, 2015 Member

I think that function is only showing as a new one is added below, as it's nothing to do with this PR we should leave that docblock for another time.

@n-st
n-st Sep 25, 2015 Contributor

@laf snmpTransportToAddressFamily is a new function introduced by this PR, so I understand @f0o's wish to have it documented.
Should get around to that within about a day.

@f0o
f0o Sep 25, 2015 Member

👍

@laf
laf Sep 25, 2015 Member

Ah, sorry I read that as get_last_commit() :)

@n-st n-st PHPDoc for snmpTransportToAddressFamily()
65ca58a
@f0o
Member
f0o commented Sep 26, 2015

👍 Happy to merge it

@f0o f0o merged commit d06ab94 into librenms:master Sep 27, 2015

2 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 1 new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment