From d026e9f0ccc05d55dfeca5cdbd1246617e9b7c5e Mon Sep 17 00:00:00 2001 From: Tony Murray Date: Thu, 21 Apr 2022 21:49:26 -0500 Subject: [PATCH] Allow unordered OIDs (global and per-os) (#13923) * Allow unordered OIDs (global and per-os) Fix global no_bulk setting, was ignored before (to fix global needed to rework Config::getCombined() a bit to allow a global prefix to be specified) Removed invalid use of getCombined and updated tests * fix whitespace * update os schema --- LibreNMS/Config.php | 23 +++++++++-------- LibreNMS/Data/Source/NetSnmpQuery.php | 29 ++++++++++++++-------- includes/definitions/bintec-beip-plus.yaml | 3 +++ includes/discovery/arp-table.inc.php | 7 +----- includes/discovery/functions.inc.php | 12 ++++----- includes/snmp.inc.php | 9 ++++++- misc/config_definitions.json | 9 ++++++- misc/os_schema.json | 3 +++ phpstan-baseline.neon | 15 ----------- resources/lang/en/settings.php | 4 +++ tests/ConfigTest.php | 24 +++++++++++++----- 11 files changed, 82 insertions(+), 56 deletions(-) diff --git a/LibreNMS/Config.php b/LibreNMS/Config.php index bb9579ad2cf4..39c70abc30f7 100644 --- a/LibreNMS/Config.php +++ b/LibreNMS/Config.php @@ -204,29 +204,32 @@ public static function getOsSetting($os, $key, $default = null) * Removes any duplicates. * When the arrays have keys, os settings take precedence over global settings * - * @param string $os The os name + * @param string|null $os The os name * @param string $key period separated config variable name + * @param string $global_prefix prefix for global setting * @param array $default optional array to return if the setting is not set * @return array */ - public static function getCombined($os, $key, $default = []) + public static function getCombined(?string $os, string $key, string $global_prefix = '', array $default = []): array { - if (! self::has($key)) { - return self::getOsSetting($os, $key, $default); - } + $global_key = $global_prefix . $key; if (! isset(self::$config['os'][$os][$key])) { - if (! Str::contains($key, '.')) { - return self::get($key, $default); + if (! Str::contains($global_key, '.')) { + return (array) self::get($global_key, $default); } if (! self::has("os.$os.$key")) { - return self::get($key, $default); + return (array) self::get($global_key, $default); } } + if (! self::has("os.$os.$key")) { + return (array) self::get($global_key, $default); + } + return array_unique(array_merge( - (array) self::get($key, $default), - (array) self::getOsSetting($os, $key, $default) + (array) self::get($global_key), + (array) self::getOsSetting($os, $key) )); } diff --git a/LibreNMS/Data/Source/NetSnmpQuery.php b/LibreNMS/Data/Source/NetSnmpQuery.php index 705694a62fca..20de0b0aea29 100644 --- a/LibreNMS/Data/Source/NetSnmpQuery.php +++ b/LibreNMS/Data/Source/NetSnmpQuery.php @@ -355,19 +355,26 @@ private function exec(string $command, array $oids): SnmpResponse private function initCommand(string $binary, array $oids): array { - if ($binary == 'snmpwalk' - && $this->device->snmpver !== 'v1' - && Config::getOsSetting($this->device->os, 'snmp_bulk', true) - && empty(array_intersect($oids, Config::getCombined($this->device->os, 'oids.no_bulk'))) // skip for oids that do not work with bulk - ) { - $snmpcmd = [Config::get('snmpbulkwalk', 'snmpbulkwalk')]; - - $max_repeaters = $this->device->getAttrib('snmp_max_repeaters') ?: Config::getOsSetting($this->device->os, 'snmp.max_repeaters', Config::get('snmp.max_repeaters', false)); - if ($max_repeaters > 0) { - $snmpcmd[] = "-Cr$max_repeaters"; + if ($binary == 'snmpwalk') { + // allow unordered responses for specific oids + if (! empty(array_intersect($oids, Config::getCombined($this->device->os, 'oids.unordered', 'snmp.')))) { + $this->allowUnordered(); } - return $snmpcmd; + // handle bulk settings + if ($this->device->snmpver !== 'v1' + && Config::getOsSetting($this->device->os, 'snmp_bulk', true) + && empty(array_intersect($oids, Config::getCombined($this->device->os, 'oids.no_bulk', 'snmp.'))) // skip for oids that do not work with bulk + ) { + $snmpcmd = [Config::get('snmpbulkwalk', 'snmpbulkwalk')]; + + $max_repeaters = $this->device->getAttrib('snmp_max_repeaters') ?: Config::getOsSetting($this->device->os, 'snmp.max_repeaters', Config::get('snmp.max_repeaters', false)); + if ($max_repeaters > 0) { + $snmpcmd[] = "-Cr$max_repeaters"; + } + + return $snmpcmd; + } } return [Config::get($binary, $binary)]; diff --git a/includes/definitions/bintec-beip-plus.yaml b/includes/definitions/bintec-beip-plus.yaml index b1dfeffec2f3..dd68edc19f62 100644 --- a/includes/definitions/bintec-beip-plus.yaml +++ b/includes/definitions/bintec-beip-plus.yaml @@ -11,3 +11,6 @@ over: discovery: - sysObjectID: - .1.3.6.1.4.1.272.4.201.66.69.50.48 +oids: + unordered: + - IP-MIB::ipNetToMediaPhysAddress diff --git a/includes/discovery/arp-table.inc.php b/includes/discovery/arp-table.inc.php index e9664c066fd1..114e13501dce 100644 --- a/includes/discovery/arp-table.inc.php +++ b/includes/discovery/arp-table.inc.php @@ -30,12 +30,7 @@ include Config::get('install_dir') . "/includes/discovery/arp-table/{$device['os']}.inc.php"; } else { $arp_data = SnmpQuery::context($context_name)->walk('IP-MIB::ipNetToPhysicalPhysAddress')->table(1); - - $mediaQuery = SnmpQuery::context($context_name); - if ($device['os'] == 'bintec-beip-plus') { - $mediaQuery->allowUnordered(); - } - $arp_data = $mediaQuery->walk('IP-MIB::ipNetToMediaPhysAddress')->table(1, $arp_data); + SnmpQuery::context($context_name)->walk('IP-MIB::ipNetToMediaPhysAddress')->table(1, $arp_data); } $sql = 'SELECT * from `ipv4_mac` WHERE `device_id`=? AND `context_name`=?'; diff --git a/includes/discovery/functions.inc.php b/includes/discovery/functions.inc.php index f4b239089ded..7a639bc15a07 100644 --- a/includes/discovery/functions.inc.php +++ b/includes/discovery/functions.inc.php @@ -815,7 +815,7 @@ function get_device_divisor($device, $os_version, $sensor_type, $oid) */ function ignore_storage($os, $descr) { - foreach (Config::getCombined($os, 'ignore_mount', []) as $im) { + foreach (Config::getCombined($os, 'ignore_mount') as $im) { if ($im == $descr) { d_echo("ignored $descr (matched: $im)\n"); @@ -823,7 +823,7 @@ function ignore_storage($os, $descr) } } - foreach (Config::getCombined($os, 'ignore_mount_string', []) as $ims) { + foreach (Config::getCombined($os, 'ignore_mount_string') as $ims) { if (Str::contains($descr, $ims)) { d_echo("ignored $descr (matched: $ims)\n"); @@ -831,7 +831,7 @@ function ignore_storage($os, $descr) } } - foreach (Config::getCombined($os, 'ignore_mount_regexp', []) as $imr) { + foreach (Config::getCombined($os, 'ignore_mount_regexp') as $imr) { if (preg_match($imr, $descr)) { d_echo("ignored $descr (matched: $imr)\n"); @@ -1182,15 +1182,15 @@ function add_cbgp_peer($device, $peer, $afi, $safi) */ function can_skip_sensor($device, $sensor_class = '', $sensor_descr = '') { - if (! empty($sensor_class) && Config::getCombined($device['os'], "disabled_sensors.$sensor_class", false)) { + if (! empty($sensor_class) && (Config::getOsSetting($device['os'], "disabled_sensors.$sensor_class") || Config::get("disabled_sensors.$sensor_class"))) { return true; } - foreach (Config::getCombined($device['os'], 'disabled_sensors_regex', []) as $skipRegex) { + foreach (Config::getCombined($device['os'], 'disabled_sensors_regex') as $skipRegex) { if (! empty($sensor_descr) && preg_match($skipRegex, $sensor_descr)) { return true; } } - foreach (Config::getCombined($device['os'], "disabled_sensors_regex.$sensor_class", []) as $skipRegex) { + foreach (Config::getCombined($device['os'], "disabled_sensors_regex.$sensor_class") as $skipRegex) { if (! empty($sensor_descr) && preg_match($skipRegex, $sensor_descr)) { return true; } diff --git a/includes/snmp.inc.php b/includes/snmp.inc.php index ec6792f155cb..20f271ed825c 100644 --- a/includes/snmp.inc.php +++ b/includes/snmp.inc.php @@ -130,9 +130,11 @@ function gen_snmpget_cmd($device, $oids, $options = null, $mib = null, $mibdir = */ function gen_snmpwalk_cmd($device, $oids, $options = null, $mib = null, $mibdir = null) { + $oids = Arr::wrap($oids); + if ($device['snmpver'] == 'v1' || (isset($device['os']) && (Config::getOsSetting($device['os'], 'snmp_bulk', true) == false - || ! empty(array_intersect(Arr::wrap($oids), Config::getCombined($device['os'], 'oids.no_bulk'))))) // skip for oids that do not work with bulk + || ! empty(array_intersect($oids, Config::getCombined($device['os'], 'oids.no_bulk', 'snmp.'))))) // skip for oids that do not work with bulk ) { $snmpcmd = [Config::get('snmpwalk')]; } else { @@ -143,6 +145,11 @@ function gen_snmpwalk_cmd($device, $oids, $options = null, $mib = null, $mibdir } } + // allow unordered responses for specific oids + if (! empty(array_intersect($oids, Config::getCombined($device['os'], 'oids.unordered', 'snmp.')))) { + $snmpcmd[] = '-Cc'; + } + return gen_snmp_cmd($snmpcmd, $device, $oids, $options, $mib, $mibdir); } //end gen_snmpwalk_cmd() diff --git a/misc/config_definitions.json b/misc/config_definitions.json index d0525b0f6155..7e8444d8cc8b 100644 --- a/misc/config_definitions.json +++ b/misc/config_definitions.json @@ -5182,7 +5182,14 @@ "section": "snmp", "order": 20, "type": "array", - "default": null + "default": [] + }, + "snmp.oids.unordered": { + "group": "poller", + "section": "snmp", + "order": 21, + "type": "array", + "default": [] }, "snmp.timeout": { "group": "poller", diff --git a/misc/os_schema.json b/misc/os_schema.json index 9b779ba72db4..7e6660c14bd2 100644 --- a/misc/os_schema.json +++ b/misc/os_schema.json @@ -368,6 +368,9 @@ "properties": { "no_bulk": { "type": "array" + }, + "unordered": { + "type": "array" } }, "additionalProperties": false diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 832d9b4dd159..3dbbf1cc199b 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -10295,11 +10295,6 @@ parameters: count: 1 path: tests/ConfigTest.php - - - message: "#^Method LibreNMS\\\\Tests\\\\ConfigTest\\:\\:testGetCombined\\(\\) has no return type specified\\.$#" - count: 1 - path: tests/ConfigTest.php - - message: "#^Method LibreNMS\\\\Tests\\\\ConfigTest\\:\\:testGetDeviceSetting\\(\\) has no return type specified\\.$#" count: 1 @@ -10350,16 +10345,6 @@ parameters: count: 1 path: tests/ConfigTest.php - - - message: "#^Parameter \\#3 \\$default of static method LibreNMS\\\\Config\\:\\:getCombined\\(\\) expects array, false given\\.$#" - count: 2 - path: tests/ConfigTest.php - - - - message: "#^Parameter \\#3 \\$default of static method LibreNMS\\\\Config\\:\\:getCombined\\(\\) expects array, true given\\.$#" - count: 1 - path: tests/ConfigTest.php - - message: "#^Property LibreNMS\\\\Tests\\\\ConfigTest\\:\\:\\$config has no type specified\\.$#" count: 1 diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index 763f3a82f2e8..ecaef6554738 100644 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -1387,6 +1387,10 @@ 'description' => 'Disable snmp bulk for OIDs', 'help' => 'Disable snmp bulk operation for certain OIDs. Generally, this should be set on an OS instead. Format should be MIB::OID', ], + 'unordered' => [ + 'description' => 'Allow out of order snmp respsonse for OIDs', + 'help' => 'Ignore unordered OIDs in snmp responses for certain OIDs. Unordered OIDs could result in an oid loop during an snmpwalk. Generally, this should be set on an OS instead. Format should be MIB::OID', + ], ], 'port' => [ 'description' => 'Port', diff --git a/tests/ConfigTest.php b/tests/ConfigTest.php index fbf12e73097a..cc9c9009d1fc 100644 --- a/tests/ConfigTest.php +++ b/tests/ConfigTest.php @@ -101,26 +101,38 @@ public function testGetOsSetting() $this->assertNull(Config::getOsSetting('nullos', 'fallback'), 'Incorrectly loaded global setting'); } - public function testGetCombined() + public function testGetCombined(): void { $this->setConfig(function (&$config) { $config['num'] = ['one', 'two']; + $config['withprefix']['num'] = ['four', 'five']; $config['os']['nullos']['num'] = ['two', 'three']; $config['assoc'] = ['a' => 'same', 'b' => 'same']; + $config['withprefix']['assoc'] = ['a' => 'prefix_same', 'd' => 'prefix_same']; $config['os']['nullos']['assoc'] = ['b' => 'different', 'c' => 'still same']; - $config['os']['nullos']['osset'] = true; - $config['gset'] = true; + $config['os']['nullos']['osset'] = 'ossetting'; + $config['gset'] = 'fallbackone'; + $config['withprefix']['gset'] = 'fallbacktwo'; }); - $this->assertTrue(Config::getCombined('nullos', 'non-existent', true), 'Did not return default value on non-existent key'); - $this->assertTrue(Config::getCombined('nullos', 'osset', false), 'Did not return OS value when global value is not set'); - $this->assertTrue(Config::getCombined('nullos', 'gset', false), 'Did not return global value when OS value is not set'); + $this->assertSame(['default'], Config::getCombined('nullos', 'non-existent', '', ['default']), 'Did not return default value on non-existent key'); + $this->assertSame(['ossetting'], Config::getCombined('nullos', 'osset', '', ['default']), 'Did not return OS value when global value is not set'); + $this->assertSame(['fallbackone'], Config::getCombined('nullos', 'gset', '', ['default']), 'Did not return global value when OS value is not set'); + $this->assertSame(['default'], Config::getCombined('nullos', 'non-existent', 'withprefix.', ['default']), 'Did not return default value on non-existent key'); + $this->assertSame(['ossetting'], Config::getCombined('nullos', 'osset', 'withprefix.', ['default']), 'Did not return OS value when global value is not set'); + $this->assertSame(['fallbacktwo'], Config::getCombined('nullos', 'gset', 'withprefix.', ['default']), 'Did not return global value when OS value is not set'); $combined = Config::getCombined('nullos', 'num'); sort($combined); $this->assertEquals(['one', 'three', 'two'], $combined); + $combined = Config::getCombined('nullos', 'num', 'withprefix.'); + sort($combined); + $this->assertEquals(['five', 'four', 'three', 'two'], $combined); + $this->assertSame(['a' => 'same', 'b' => 'different', 'c' => 'still same'], Config::getCombined('nullos', 'assoc')); + // should associative not ignore same values (d=>prefix_same)? are associative arrays actually used? + $this->assertSame(['a' => 'prefix_same', 'b' => 'different', 'c' => 'still same'], Config::getCombined('nullos', 'assoc', 'withprefix.')); } public function testSet()