New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed UniFi AP hardware type and firmware version retrieval #8005

Merged
merged 10 commits into from Jan 9, 2018

Conversation

Projects
None yet
5 participants
@neg2led
Contributor

neg2led commented Jan 3, 2018

Hi All,

Just a small change here - the existing code to pull hardware/version details from UniFi APs uses a part of IEEE802dot11-MIB which the UniFi APs are... somewhat confused about how to respond to.

Short version; instead of pulling dot11manufacturerName and dot11manufacturerProductVersion from IEEE802dot11-MIB, we pull unifiApSystemModel.0 and unifiApSystemVersion.0 from UBNT-UniFi-MIB. I've also dropped the nobulk flag from the OS definition as it's redundant.

Long version;

In the existing code, we iterate through several possible entity IDs in an attempt to find a valid dot11manufacturerName.x and dot11manufacturerProductVersion.x in IEEE802dot11-MIB. These entities are indexed by vAP interface (SSID/vwire) - in my installation I've seen the index as high as 27 as it increments on config change;

screenshot 2018-01-03 17 40 07

There's also an issue with the dot11manufacturerProductVersion OID - Ubiquiti released a major UAP firmware upgrade (3.9.15) a few weeks ago; amongst other things, it replaces tinysnmpd with Net-SNMP & brings v2c/v3 support. Devices on the newer firmware don't respond to dot11manufacturerProductVersion at all; a getnext for the OID skips down the tree to sysDescr.0 without returning any entities.

Between these two things, model and version detection are broken for UAPs on this firmware and several beta versions. Fortunately, UBNT-UniFi-MIB provides the answer; this MIB is supported by both old and new SNMP daemons, hasn't changed in quite a while, and gives us unifiApSystemModel.0 + unifiApSystemVersion.0. So we just pull those instead, and call it a day.

While we're at it, I've dropped the nobulk flag from the OS. This doesn't break compatibility with older firmwares; the ubiquiti tinysnmpd implementation only supported SNMPv1, which doesn't do bulkwalks, and snmp.inc.php disables bulkwalks for v1 devices too just for good measure;

if ($device['snmpver'] == 'v1' || (isset($device['os'], $config['os'][$device['os']]['nobulk']) && $config['os'][$device['os']]['nobulk'])) {
$snmpcmd = $config['snmpwalk'];
} else {
$snmpcmd = $config['snmpbulkwalk'];

And the change has the desired effect;

screenshot 2018-01-03 18 04 23

Shiny. Works just fine on firmwares as far back as 3.7.37, too.

screenshot 2018-01-03 18 41 20

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Pre-commit says I'm fine! yay.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2018

What UniFi devices have you tested?

@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 3, 2018

I've tested on all of the Gen2 802.11ac hardware except UAP-AC-LR, so that's UAP-AC-Pro, UAP-AC-Lite, UAP-AC-M, UAP-AC-M-Pro - as well as UAP-AC-HD, which is Gen3.

All hardware tested was on firmware 3.9.18, though I also tested on a UAP-AC-Pro running firmware 3.7.37.

I don't have access to any older devices/firmwares to test; this MIB was officially released mid-2016, and hasn't changed much (if any) since release. For earlier firmwares such as v3.2.7 from circa 2014, there are snmpwalk outputs posted on the UBNT forums;

iso.2.840.10036.3.1.2.1.3.0 : UAP-Outdoor+
iso.2.840.10036.3.1.2.1.4.0 : BZ.ar7240.v3.2.7.2816.141027.1535

This actually matches up with the earlier OIDs used - dot11manufacturerProductName and dot11manufacturerProductVersion, but is available at the .0 entity as well as the entities indexed by vAP (unlike the newer firmware). The version here also returns the full firmware ID, so the regex from the original file would need to come back if we want a clean version number.

It looks like the original code here would have worked fine with the older configuration & was modified to check higher entity numbers semi-recently? These older units/firmwares report a sysObjectID of .1.3.6.1.4.1.10002.1, while the newer firmwares report .1.3.6.1.4.1.8072.3.2.10 (which we're already using for OS detection) - should I add a sysObjectID check & poll the old/new OID appropriately, just poll both and look for a valid response, or something else entirely?

@crcro

This comment has been minimized.

Contributor

crcro commented Jan 3, 2018

@murrant ... tested this patch now with UAP-AC-Lite and UAP-Pro both on 3.9.15.8011 (latest stable) and works great. Before this patch didn't had any Platform and OS Version for them.

@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 4, 2018

Edit: Argh! My bad, a few extra characters here and there. Cleaning this up, will push a new commit.

Dropped in the sysObjectID check and swapped the multiple independent snmp_get()s out for a single snmp_get_multi() for each type (old/new).

Should I be doing more sanity-checking here? OS identification happens long before we get to this bit, and AFAICT all UBNT gear should respond to this in an appropriate fashion - if we can find someone who's still running very old HW/firmware versions to test with that would be useful.

neg2led added some commits Jan 4, 2018

Revert "Modified to pull from appropriate OID depending on sysObjectI…
…D" - typos in snmp_get_multi() commands

This reverts commit 5c2b603.
Revert "Revert "Modified to pull from appropriate OID depending on sy…
…sObjectID" - typos in snmp_get_multi() commands"

This reverts commit bccf301, so I can submit everything in the one commit
@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 4, 2018

Apologies for the mess, git still does my head in. Anyway, this now attempts to pull the newer OIDs and (if that should fail) pulls the older .0 OID which is shown to work in firmwares over 3 years old.

Should we be covering the possible edge case of a device that doesn't report unifiApSystemModel.0 or dot11manufacturerName.0 but might report dot11manufacturerName.5 (or some other ID) via a couple of snmp_getnext()s if the original multi-get fails? I can't rule out the possibility that somewhere between 2014 and mid-2016 there may be a firmware that behaves like this.

@Zmegolaz Zmegolaz referenced this pull request Jan 4, 2018

Closed

Ubiquiti NB 5AC 16 missing information #7996

4 of 5 tasks complete
@murrant

This comment has been minimized.

Member

murrant commented Jan 4, 2018

No, don't write it to work around things that might not even exist.

@laf

laf approved these changes Jan 4, 2018

LGTM

@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 5, 2018

Oh hey I messed up the commits again. Woo. Alright, just the one new commit with the right change. The reverts were probably completely unnecessary but here we are - git, man. git.

This should cover both old and new OID :)

if ($data = snmp_get_multi($device, 'unifiApSystemModel unifiApSystemVersion', '-OUQs', 'UBNT-UniFi-MIB')) {
$hardware = $data[0]['unifiApSystemModel'];
$version = $data[0]['unifiApSystemVersion'];
} elseif ($data = snmp_get_multi($device, 'dot11manufacturerProductName dot11manufacturerProductVersion', '-OQUs', 'IEEE802dot11-MIB')) {

This comment has been minimized.

@murrant

murrant Jan 5, 2018

Member

This needs to be two separate snmp_getnext() calls. But one can be inside the if statement ;)

This comment has been minimized.

@neg2led

neg2led Jan 5, 2018

Contributor

Ah yep, not a problem. This does make the code a bit messier - snmp_get_multi() returns a nice convenient array indexed by OID, while snmp_getnext() might return from an OID we didn't ask for & is a single string.

@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 5, 2018

I've borrowed some of the logic from snmp_get_multi() here for parsing/sanity checking since there's a nonzero chance that a getnext on one or both OIDs might return (say) sysDescr.0, and it saves an SNMP op - snmpgetnext is quite happy to operate on multiple OIDs too, perhaps an snmp_getnext_multi() function might be worth making...

Works great on my test unit (commented out the UBNT-UniFi-MIB check for this test since this AP responds to both);

SNMP[/usr/bin/snmpgetnext -v1 -c COMMUNITY -Oqs -m IEEE802dot11-MIB -M /opt/librenms/mibs:/opt/librenms/mibs/ubnt udp:HOSTNAME:1611 dot11manufacturerProductName dot11manufacturerProductVersion]
dot11manufacturerProductName.4 UAP-AC-Pro-Gen2
dot11manufacturerProductVersion.4 BZ.qca956x.v*65.170118.0908

SQL[INSERT INTO `eventlog` (`host`,`device_id`,`reference`,`type`,`datetime`,`severity`,`message`,`username`)  VALUES ('48','48','NULL','system',NOW(),'3','OS Version -> v3.7.37.6065','')]
Hardware: UAP-AC-Pro-Gen2
Version: v3.7.37.6065
Features:
Serial:

Edit: And I messed this up again - with those same two superfluous [0]s that won't leave me alone... I need to do better at this & I should have started by opening this as an issue rather than a pull request, shouldn't I?

@murrant

This comment has been minimized.

Member

murrant commented Jan 5, 2018

I did not know snmpgetnext worked on multiple oids, nice. Perhaps a snmp_getnext_multi() function would be a good thing.

@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 5, 2018

It's pretty simple to add, too - almost a carbon copy of snmp_get_multi(), and it makes this fix a lot cleaner.

I can close this PR and go down that route? It seems like the smarter move long-term... Makes sanity checking getnext results much easier too as the returned OID gets used for the index.

@murrant

This comment has been minimized.

Member

murrant commented Jan 5, 2018

You can just let this sit open and add the snmp_getnext_multi() and we'll merge it first.

@neg2led neg2led referenced this pull request Jan 9, 2018

Merged

Add snmp_getnext_multi() function #8052

1 of 1 task complete
@neg2led

This comment has been minimized.

Contributor

neg2led commented Jan 9, 2018

Submitted the snmp_getnext_multi() function in #8052 & updated this branch to make use of the new function 👍

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 9, 2018

The inspection completed: No new issues

@murrant murrant merged commit e372023 into librenms:master Jan 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant referenced this pull request Jan 9, 2018

Closed

Ubiquiti AP OS Version #7997

4 of 5 tasks complete

@neg2led neg2led deleted the neg2led:unifi-os-pollfix branch Jan 9, 2018

@neg2led neg2led referenced this pull request Jan 20, 2018

Merged

Fix UniFi OS polling (because I broke it last time) #8114

1 of 1 task complete
@lock

This comment has been minimized.

lock bot commented May 16, 2018

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

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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