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

Features: Add state sensors for Palo Alto Networks firewall #7482

Merged
merged 8 commits into from Nov 2, 2017

Conversation

Projects
None yet
4 participants
@FTBZ
Contributor

FTBZ commented Oct 13, 2017

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.

Testers

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


  • Add state sensors for the HA
  • Add an alert rule to the collection
  • Used PHP and not YAML because I need to convert string to integer
  • Not sure where to store the MIB file using PHP
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 15, 2017

Member

As this works then it means the mib is converting the response to a text string. Why not try using a numerical oid in the yaml, I've never actually tried it before but it might work ok.

Member

laf commented Oct 15, 2017

As this works then it means the mib is converting the response to a text string. Why not try using a numerical oid in the yaml, I've never actually tried it before but it might work ok.

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 16, 2017

Contributor

Tested without success with some variante of this code:

        state:
            data:
                -
                    oid: 1.3.6.1.4.1.25461.2.1.2.1.13.0
                    num_oid: .1.3.6.1.4.1.25461.2.1.2.1.13.
                    index: 'panSysHAMode.{{ $index }}'
                    descr: High Availability Mode
                    states:
                        - { descr: Active-Passive, graph: 1, value: 1, generic: 0 }
                        - { descr: Active-Active, graph: 1, value: 2, generic: 0 }

Idea?

Contributor

FTBZ commented Oct 16, 2017

Tested without success with some variante of this code:

        state:
            data:
                -
                    oid: 1.3.6.1.4.1.25461.2.1.2.1.13.0
                    num_oid: .1.3.6.1.4.1.25461.2.1.2.1.13.
                    index: 'panSysHAMode.{{ $index }}'
                    descr: High Availability Mode
                    states:
                        - { descr: Active-Passive, graph: 1, value: 1, generic: 0 }
                        - { descr: Active-Active, graph: 1, value: 2, generic: 0 }

Idea?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 17, 2017

Member

Try oid: .1.3.6.1.4.1.25461.2.1.2.1.13

Member

laf commented Oct 17, 2017

Try oid: .1.3.6.1.4.1.25461.2.1.2.1.13

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 18, 2017

Contributor

No change, this is always the string and not the integer output.

Contributor

FTBZ commented Oct 18, 2017

No change, this is always the string and not the integer output.

@FTBZ FTBZ referenced this pull request Oct 18, 2017

Merged

fix: Add missing MIB for Palo Alto Networks firewall #7509

1 of 1 task complete
Show outdated Hide outdated includes/discovery/sensors/state/panos.inc.php Outdated
@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 19, 2017

Contributor

I'm checking if everything is working fine. Wait before merging, don't want to break something more :p

Contributor

FTBZ commented Oct 19, 2017

I'm checking if everything is working fine. Wait before merging, don't want to break something more :p

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 20, 2017

Member

@laf if the mib is loaded it will use the textual convention even for numeric oids. Also, this appears to be a raw string returned by the device.

Member

murrant commented Oct 20, 2017

@laf if the mib is loaded it will use the textual convention even for numeric oids. Also, this appears to be a raw string returned by the device.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 20, 2017

Member

@FTBZ The state sensor code always converts strings to int. I think the description needs to match the string returned.

Member

murrant commented Oct 20, 2017

@FTBZ The state sensor code always converts strings to int. I think the description needs to match the string returned.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 20, 2017

Member

@FTBZ Changing the descriptions to lowercase fixed the yaml.

mib: PAN-COMMON-MIB
modules:
    sensors:
        state:
            data:
                -
                    oid: panSysHAMode
                    num_oid: .1.3.6.1.4.1.25461.2.1.2.1.13.
                    index: 'panSysHAMode.{{ $index }}'
                    descr: High Availability Mode
                    states:
                        - { value: 0, generic: 0, graph: 1, descr: active-passive }
                        - { value: 1, generic: 0, graph: 1, descr: active-active }
Member

murrant commented Oct 20, 2017

@FTBZ Changing the descriptions to lowercase fixed the yaml.

mib: PAN-COMMON-MIB
modules:
    sensors:
        state:
            data:
                -
                    oid: panSysHAMode
                    num_oid: .1.3.6.1.4.1.25461.2.1.2.1.13.
                    index: 'panSysHAMode.{{ $index }}'
                    descr: High Availability Mode
                    states:
                        - { value: 0, generic: 0, graph: 1, descr: active-passive }
                        - { value: 1, generic: 0, graph: 1, descr: active-active }

FTBZ added some commits Oct 20, 2017

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 20, 2017

Contributor

Thanks for the help, someone can double check?

Contributor

FTBZ commented Oct 20, 2017

Thanks for the help, someone can double check?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 20, 2017

Member

@FTBZ just to verify, you want all states for these to show as green?

Member

murrant commented Oct 20, 2017

@FTBZ just to verify, you want all states for these to show as green?

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 20, 2017

Contributor

@murrant Yes, I think it's better because a device can be active OR passive during the initial configuration. Depending the primary status it's not a bad thing.

Contributor

FTBZ commented Oct 20, 2017

@murrant Yes, I think it's better because a device can be active OR passive during the initial configuration. Depending the primary status it's not a bad thing.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 20, 2017

Member

You could set it to "unknown" which is gray for passive.

Member

murrant commented Oct 20, 2017

You could set it to "unknown" which is gray for passive.

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 23, 2017

Contributor

As you wish...

Contributor

FTBZ commented Oct 23, 2017

As you wish...

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 25, 2017

Member

@FTBZ are you updating this as murrant suggested or leaving as is?

Member

laf commented Oct 25, 2017

@FTBZ are you updating this as murrant suggested or leaving as is?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 30, 2017

Member

@FTBZ prod :)

Member

laf commented Oct 30, 2017

@FTBZ prod :)

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 31, 2017

Contributor

Ok, changed to unknown/grey for passive device.

screen shot 2017-10-31 at 07 19 00

screen shot 2017-10-31 at 07 18 19

Contributor

FTBZ commented Oct 31, 2017

Ok, changed to unknown/grey for passive device.

screen shot 2017-10-31 at 07 19 00

screen shot 2017-10-31 at 07 18 19

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Oct 31, 2017

The inspection completed: 26 new issues, 99 updated code elements

scrutinizer-notifier commented Oct 31, 2017

The inspection completed: 26 new issues, 99 updated code elements

@laf laf merged commit aef1a5f into librenms:master Nov 2, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

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

lock bot commented May 17, 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 17, 2018

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