search

liquidat / nagios-icinga-openvpn

Nagios/Icinga check for OpenVPN availability monitoring
MIT License
51 stars 27 forks source link

clean up, response validation #16

Closed andiwand closed 7 years ago

andiwand commented 7 years ago

should fix #14

liquidat commented 7 years ago

@andiwand As usual, thanks for this impressive PR - how about we move the code to a repository where you get full rights as well? :)

One thing though before I merge this: currently if you check a VPN server without any further details like a client key, the check fails because the return function does try to return a value which was not set:

./check_openvpn -p 443 -t boran.bayz.de 
Traceback (most recent call last):
  File "./check_openvpn", line 257, in <module>
    code = main()
  File "./check_openvpn", line 254, in main
    return check(args.host, args.port, args.tcp, args.timeout, digest, digest_size, client_key, server_key, args.retrycount, args.validate)
UnboundLocalError: local variable 'client_key' referenced before assignment

I would like to keep the script as simple as possible for "normal" use cases, so without that a client key is not a must. Could you check if you can make the client key only optional?

andiwand commented 7 years ago

@liquidat thanks! fixed that and tested against https server.

about code moving: i thought about that aswell, but this is the one which is starred and you find first i guess. i only have one more idea of improving this, then you will be free of merging my PRs :) it's also nice to have someone else testing this, since i would continuously merge bugs, if you dont mind... :D

liquidat commented 7 years ago

@andiwand 

$ ./check_openvpn -p 443 -t boran.bayz.de
CRIT: Invalid response
$ ./check_openvpn -p 443 -t tuzak.bayz.de                                                                                                                                                CRIT: Invalid response

tuzak.bayz.de on port 443 is an http server - there the critical response is correct. boran.bayz.de on port 443 is a VPN server (sorry, I should have mentioned that) ;)

So the current critical response seems to be wrong, the server is working fine for me. Not sure where the error is though... :-/

And regarding code moving: you are right that this place is well known already... but if you ever feel like moving the code, just let me know, I would help with it. Until then I hunt you with tests and bugs ;)

andiwand commented 7 years ago

@liquidat if you could test it again... i hope for the last time :P

liquidat commented 7 years ago

Tests run perfectly fine! I have not tested the key feature yet but trust you that it works as expected.

Thanks again! I will merge it. How about we add you to the list of contributors in the source code file?

andiwand commented 7 years ago

Yeah i tested the key feature! And i would appreciate that : )