Enable discovery of CDP neighbour by IP address #3561

Merged
merged 3 commits into from May 26, 2016

Projects

None yet

4 participants

@geordish
Contributor

Fix for #2715.

Setting $config['discovery_by_ip'] = true allows CDP neighbour to be discovered by IP instead of hostname.

@geordish geordish Enable discovery of CDP neighbour by IP address
db5d074
@paszczus

I have discovered that problem today, wow, very nice that you are fixing this @geordish !

@geordish
Contributor
geordish commented May 25, 2016 edited

@paszczus no problem. Feel free to test my branch! I would appreciate feedback.

http://docs.librenms.org/Support/FAQ/#how-can-i-test-another-users-branch

make sure you have the following in your config.php. Presumably you do anyway if you have no DNS set.

$config['discovery_by_ip'] = true;

@paszczus
paszczus commented May 25, 2016 edited

Yes I have already set this up.
I am trying to checkout your branch but i am doing something wrong:

-bash-4.2$ git remote add geordish https://github.com/librenms/librenms.git 
-bash-4.2$ git remote update geordish
Fetching geordish
From https://github.com/librenms/librenms
 * [new branch]      master     -> geordish/master
 * [new branch]      revert-2097-issue-2092 -> geordish/revert-2097-issue-2092
-bash-4.2$ git checkout issue-2715
error: pathspec 'issue-2715' did not match any file(s) known to git.
@paszczus

Ok, i have manually changed that file. Testing now ;-)

@geordish
Contributor

Hmm, those instructions look like they may be wrong. The correct add command is

git remote add f0o https://github.com/geordish/librenms.git

I will create a PR to correct that :)

@paszczus

Now it found less devices than before and only one with name instead of ip:

Trying community public ...
No reply on community public using v2c
Could not reach HP-E2620-48 with given SNMP community using v2c
Trying v3 parameters root/noAuthNoPriv ... 
No reply on credentials root/noAuthNoPriv using v3
Could not reach HP-E2620-48 with given SNMP community using v3
@geordish
Contributor

Do you have $config['discovery_by_ip'] = true; in config.php ?

On 25 May 2016 at 14:34, paszczus notifications@github.com wrote:

Now it found less devices than before and only one with name instead of ip:

Trying community public ...
No reply on community public using v2c
Could not reach HP-E2620-48 with given SNMP community using v2c
Trying v3 parameters root/noAuthNoPriv ...
No reply on credentials root/noAuthNoPriv using v3
Could not reach HP-E2620-48 with given SNMP community using v3


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3561 (comment)

@paszczus

Yes:

-bash-4.2$ grep discovery config.php
### List of RFC1918 networks to allow scanning-based discovery
$config['autodiscovery']['xdp']            = TRUE;
$config['autodiscovery']['ospf']           = TRUE;
$config['autodiscovery']['bgp']            = TRUE;
$config['autodiscovery']['snmpscan']       = TRUE;
$config['discovery_by_ip'] = true;
@murrant
Contributor
murrant commented May 25, 2016

The name of the config variable could perhaps use some improvement. It may imply that all discovery protocols will use IP. At a glance of the code, it seemed like this only affects CDP.

@geordish geordish Correct array name
18eab48
@geordish
Contributor

@paszczus whoops, fixed a bug! Try again?

@murrant the config variable already exists, and I'm making use of it. From the docs:

$config['discovery_by_ip'] = true;
Enable auto discovery by IP. By default we only discover based on hostnames but manually adding by IP is allowed. Please note this could lead to duplicate devices being added based on IP, Hostname or sysName.

To me, this is the correct use of this variable. If you would rather me add a new one specific for this discovery mechanism, then I will. It feels like duplication to me though.

@murrant
Contributor
murrant commented May 25, 2016

@geordish Thanks for the clarification. I agree that the current naming is correct.

@paszczus

@geordish after your fix librenms successfully found my devices, thank you so much!

@laf laf and 1 other commented on an outdated diff May 26, 2016
includes/discovery/discovery-protocols.inc.php
@@ -55,7 +55,21 @@
$remote_device_id = dbFetchCell('SELECT `device_id` FROM `devices` WHERE `sysName` = ? OR `hostname` = ?', array($cdp['cdpCacheDeviceId'], $cdp['cdpCacheDeviceId']));
if (!$remote_device_id) {
- $remote_device_id = discover_new_device($cdp['cdpCacheDeviceId'], $device, 'CDP', $interface);
+ if(!$config['discovery_by_ip']) {
@laf
laf May 26, 2016 Member

I think it would be better to do $config['discovery_by_ip'] !== true here.

@geordish
geordish May 26, 2016 Contributor

Your wish is my command.

@geordish geordish Changed syntax of if statement for readability
a5d0b0a
@laf laf merged commit da2788e into librenms:master May 26, 2016

1 of 2 checks passed

Auto-Deploy Triggered
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geordish geordish deleted the geordish:issue-2715 branch Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment