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

newdevice: Added support for Dasan NOS #5179 + disco change #5255

Merged
merged 11 commits into from Jan 3, 2017

Conversation

Projects
None yet
4 participants
@laf
Member

laf commented Dec 28, 2016

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.

Fixes: #5179

Also adds support for discovery devices via sysObjectId and sysDescr.

@librenms/reviewers

@laf laf added the Device 🖥 label Dec 28, 2016

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 28, 2016

Auto-Deploy finished, Test PR at http://5255.ci.librenms.org or https://5255.ci.librenms.org

1 similar comment
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 28, 2016

Auto-Deploy finished, Test PR at http://5255.ci.librenms.org or https://5255.ci.librenms.org

$tmp = Symfony\Component\Yaml\Yaml::parse(
file_get_contents($file)
);
if ($tmp['discovery']['method'] === 'sysObjectId' && is_array($tmp['discovery']['sysObjectId'])) {

This comment has been minimized.

@murrant

murrant Dec 28, 2016

Member

You don't need 'method', just check if sysObjectId or syDescr isset.

This comment has been minimized.

@laf

laf Dec 28, 2016

Member

The idea with method would be to expand it out like 'sysDescr_regex' and similar (totally made up).

return $tmp['os'];
}
} elseif ($tmp['discovery']['method'] === 'sysDescr' && is_array($tmp['discovery']['sysDescr'])) {
if (starts_with($sysDescr, $tmp['discovery']['sysDescr'])) {

This comment has been minimized.

@murrant

murrant Dec 28, 2016

Member

starts_with doesn't seem very useful for sysDescr, perhaps str_contains or preg_match

This comment has been minimized.

@laf

laf Dec 28, 2016

Member

Can change, will use str_contains.

if (starts_with($sysObjectId, $tmp['discovery']['sysObjectId'])) {
return $tmp['os'];
}
} elseif ($tmp['discovery']['method'] === 'sysDescr' && is_array($tmp['discovery']['sysDescr'])) {

This comment has been minimized.

@murrant

murrant Dec 28, 2016

Member

Some os require both sysObjectId and sysDescr checks, some allow either or it would be nice to accommodate that somehow.

This comment has been minimized.

@laf

laf Dec 28, 2016

Member

Already know about that but just not expanded the code out, kept it simple at present as a poc.

discovery:
method: sysObjectId
sysObjectId:
- .1.3.6.1.4.1.6296.1.2.5.12

This comment has been minimized.

@murrant

murrant Dec 28, 2016

Member

Perhaps something like this:

discovery:
    - sysObjectId: .1.3.6.1.4.1.6296.
    - sysDescr: Description
    - { sysObjectId: .1.3.6.1.4.1.6296., sysDescr: Description }

The first two lines would match either sysObjectId or sysDescr.
The third line would require both to match.

This comment has been minimized.

@laf

laf Dec 28, 2016

Member

How would you do something had an array of sysObjectId to check AND an array of sysDescr?

What about if you want or instead of and? That's the reason I've put a method value in.

This comment has been minimized.

@murrant

murrant Dec 28, 2016

Member

Just lots of loops :(
I would think only specific pairs of sysObjectId and sysDescr would be valid. That is why I used the syntax like this. If you can give me an example(s), I would be able to show you what I think it should look like in yaml.

This comment has been minimized.

@laf

laf Dec 28, 2016

Member

zynos

quanta

powerconnect

macosx

To be honest, I want to keep the yaml simple along with detection. Maybe it's just worth sticking with matching only on an array of sysDescr or sysObjectId?

This comment has been minimized.

@murrant

murrant Dec 29, 2016

Member

My goal would be to keep it simple, but allow some complexity.

@murrant

This comment has been minimized.

Member

murrant commented Dec 29, 2016

I threw this together quick without testing and it might be a little hard to follow, but here is basically what I'm thinking for the processing:

if (isset($tmp['discovery']) && is_array($tmp['discovery'])) {
    foreach ($tmp['discovery'] as $item) {
        if (!is_array($item) || empty($item)) {
            break;
        }

        // all items must be true
        $result = true;
        foreach ($item as $key => $value) {
            switch ($key) {
                case 'sysObjectId':
                    $result &= starts_with($sysObjectId, $value);
                    break;
                case 'sysDescr':
                    $result &= str_contains($sysDescr, $value);
                    break;
                case 'sysDescr_regex':
                    $result &= preg_match_each($sysDescr, $value);
                    break;
                default:
            }
        }
        if ($result) {
            return $tmp['os'];
        }
    }
}

this means that each array under discovery is tested and all items in that array must match. The values for each can also be an array, such as:

discovery:
    - sysObjectId:
        - .1.3.6.1.4.1.6296.
        - .1.3.6.1.4.1.3443.

if this is the case any one of those values can match. I hope this is understandable, I'm a little sick and might not be expressing myself clearly.

@laf

This comment has been minimized.

Member

laf commented Dec 29, 2016

I've updated to use your code :)

Wasn't sure if by preg_match_each you meant preg_match or preg_match_all so switched to preg_match.

The discovery in this new OS works fine using the code

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 29, 2016

Auto-Deploy finished, Test PR at http://5255.ci.librenms.org or https://5255.ci.librenms.org

@murrant

This comment has been minimized.

Member

murrant commented Dec 30, 2016

Just a simple function, looked very ugly inline. allows for a list of regexes just like the others allow for a list of strings to check.

function preg_match_any($subject, $regexes) {
    foreach((array)$regexes as $regex) {
        if (preg_match($regex, $subject) {
            return true;
        }
    }
    return false;
}
@murrant

This comment has been minimized.

Member

murrant commented Dec 30, 2016

Can we get a unit test for this os??

@laf

This comment has been minimized.

Member

laf commented Jan 3, 2017

Test unit pushed.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 3, 2017

Auto-Deploy finished, Test PR at http://5255.ci.librenms.org or https://5255.ci.librenms.org

@laf laf added Blocker 🚫 and removed Blocker 🚫 labels Jan 3, 2017

@laf

This comment has been minimized.

Member

laf commented Jan 3, 2017

This is ready to go, can add further support but need a walk from a device so can be added later.

@murrant

murrant approved these changes Jan 3, 2017

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2017

@laf Made a quick addition to allow multiple regex statements like the others. If you can rebase this it is ready to merge I think.

@laf

This comment has been minimized.

Member

laf commented Jan 3, 2017

Rebasing + doing docs.

@laf

This comment has been minimized.

Member

laf commented Jan 3, 2017

Rebased + docs added.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 3, 2017

Auto-Deploy finished, Test PR at http://5255.ci.librenms.org or https://5255.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 3, 2017

The inspection completed: 3 updated code elements

@murrant

murrant approved these changes Jan 3, 2017

@murrant murrant merged commit cc8e31d into librenms:master Jan 3, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:issue-5179 branch Jan 3, 2017

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