Allow unordered OIDs (global and per-os) (librenms#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
nox-x · Apr 22, 2022 · d026e9f · d026e9f
1 parent 15feac7
commit d026e9f
Show file tree

Hide file tree

Showing 11 changed files with 82 additions and 56 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -815,23 +815,23 @@ 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");
 
             return true;
         }
     }
 
-    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");
 
             return true;
         }
     }
 
-    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
@@ -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
@@ -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
@@ -368,6 +368,9 @@
             "properties": {
                 "no_bulk": {
                     "type": "array"
+                },
+                "unordered": {
+                    "type": "array"
                 }
             },
             "additionalProperties": false

diff --git 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
@@ -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
@@ -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()