Skip to content
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

check_fortigate.pl: Return OK in case of no configured VPN tunnels #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bonki
Copy link
Contributor

@bonki bonki commented Sep 19, 2016

In case of zero configured tunnels $oid_ipsectuntableroot does not exist
which resulted in the script exiting with

UNKNOWN: session get table failed for .1.3.6.1.4.1.12356.101.12.2.2.1.1

Expected:

OK: foo (Master: bar): Active SSL-VPN Connections/Tunnels: 0/0: IPSEC Tunnels: Configured/Active: 0/0 |'ActiveSSL-VPN'=0 'ActiveIPSEC'=0

In case of zero configured tunnels $oid_ipsectuntableroot does not exist
which resulted in the script exiting with

  UNKNOWN: session get table failed for .1.3.6.1.4.1.12356.101.12.2.2.1.1
@bonki
Copy link
Contributor Author

bonki commented Sep 19, 2016

You probably don't want to merge this as it is as there's probably a canonical way of checking for
SNMP::NoSuchInstance errors, this is merely meant for testing purposes.

@riskersen
Copy link
Owner

Hey, thanks for your contribution.

Hm, hard to decide, in most cases empty oids aren't okay, except this one. So i would propose to name the function somehow like

get_snmp_table_accept_empty_oid

What do you think?

@bonki
Copy link
Contributor Author

bonki commented Sep 19, 2016

The problem is that get_table could also fail in case of real session errors (and in this case _accept_empty_oid is misleading) even if the OID exists in which case this will silently drop any errors and might wrongfully report zero tunnels which is what makes this more of a hack. In my book that's not very pretty error handling but if we can live with that (for the time being, anyway), go for it :)

@bonki
Copy link
Contributor Author

bonki commented Apr 25, 2017

What do I have to do to get this merged (in which case I'll have to update #36 which is based on latest master)? :)

@riskersen
Copy link
Owner

I have no idea, how this went through, but are you willing to rebase your pull request...?

@sgruber94
Copy link
Contributor

I can create a PR to implement that one. (if @bonki agrees)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants