patternbashMinor
nagios plugin webdav check
Viewed 0 times
webdavpluginnagioscheck
Problem
I have written a small plugin for nagios to check a WebDAV server. It is supposed to connect using a username and password and upload & delete a test file
This is my code:
Can you suggest any improvements?
This is my code:
#!/bin/bash
#Exit codes
STATE_OK=0
STATE_WARNING=1
STATE_CRITICAL=2
STATE_UNKNOWN=3
FILENAME=/tmp/test_monitor
FILE=test_monitor
HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4
# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a status of 1 using exit 1 command.
[ $# -ne 4 ] && { echo "Usage: $0 "; exit 1; }
# create temp file
truncate -s 1M /tmp/test_monitor
# connect to server and upload file
curl --fail -T $FILENAME -u $USERNAME:$PASSWORD http://$HOSTNAME/$WEBDAVDIR/
status=$?
case $status in
0)
echo "OK - File uploaded on WebDAV Server: $HOSTNAME"
# remove test file
curl --fail -X DELETE -u $USERNAME:$PASSWORD http://$HOSTNAME/$WEBDAVDIR/$FILE
exit $STATE_OK
;;
1)
echo "CRITICAL - Cannot upload file on $HOSTNAME"
exit $STATE_CRITICAL
;;
6)
echo "CRITICAL - Could not resolve host $HOSTNAME"
exit $STATE_CRITICAL
;;
26)
echo "CRITICAL - Cannot open $FILENAME for upload"
exit $STATE_CRITICAL
;;
22)
echo "CRITICAL - The requested URL returned error"
exit $STATE_CRITICAL
;;
127)
echo "CRITICAL - Command not found"
exit $STATE_CRITICAL
;;
*)
echo "UNKNOWN - "
exit $STATE_UNKNOWN
esacCan you suggest any improvements?
Solution
Overall, the code is clean and easy to follow. I appreciate the consistent indentation. I would not be upset about running your code in production. Here are some suggestions that would make it even better:
use your own variables
With the lines
we get a temporary file, but since we name it explicitly it isn't clear that it gets used by the curl right after. It would be easier to follow if you did:
If you want to be really careful, check the exit value of
check before assigning
You assign your command line arguments before checking if they are even there. I prefer to check before using so I would like to see:
for usage exit UNKNOWN rather than WARNING
If somebody runs your plugin within nagios without enough command line arguments your script will print the helpful usage and then
duplicate variables
From your script:
I understand that you need this in different places, one without the path, and otherwise with the path. But we can reduce duplication by doing something like
I would go one step further and rename the variables as
now
tips for nagios plugin writers
For writing a nagios plugin in any language I would encourage folks to:
btw
The locals don't like it if you revise your code in the same post. So if you want another review, please start another thread. Dropping a link to the new one in the comments here is also nice.
use your own variables
With the lines
# create temp file
truncate -s 1M /tmp/test_monitorwe get a temporary file, but since we name it explicitly it isn't clear that it gets used by the curl right after. It would be easier to follow if you did:
truncate -s 1M $FILENAMEIf you want to be really careful, check the exit value of
truncate or see if the file is the right size (which also implies that it exists).check before assigning
You assign your command line arguments before checking if they are even there. I prefer to check before using so I would like to see:
# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a status of 1 using exit 1 command.
[ $# -ne 4 ] && { echo "Usage: $0 "; exit 1; }
HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4for usage exit UNKNOWN rather than WARNING
If somebody runs your plugin within nagios without enough command line arguments your script will print the helpful usage and then
exit 1 which we know nagios will interpret as a warning. That's something that could get missed in a big nagios install for a while so I'd prefer to exit $STATE_UNKNOWN so that it will be obvious in nagios that something is broken with the config.duplicate variables
From your script:
FILENAME=/tmp/test_monitor
FILE=test_monitorI understand that you need this in different places, one without the path, and otherwise with the path. But we can reduce duplication by doing something like
FILE=test_monitor
FILENAME=/tmp/$FILEI would go one step further and rename the variables as
FILENAME=test_monitor
FQFN=/tmp/$FILEnow
FILENAME contains the name of the file without the path and FQFN is he "Fully Qualified File Name" and is the one you will mostly use.tips for nagios plugin writers
For writing a nagios plugin in any language I would encourage folks to:
- include a sampling of
commandconfig entries
- maintain your code on github so you get a free issue tracker and such
- register with the icinga registry since the nagios one has been broken lately
btw
The locals don't like it if you revise your code in the same post. So if you want another review, please start another thread. Dropping a link to the new one in the comments here is also nice.
:-)Code Snippets
# create temp file
truncate -s 1M /tmp/test_monitortruncate -s 1M $FILENAME# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a status of 1 using exit 1 command.
[ $# -ne 4 ] && { echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"; exit 1; }
HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4FILENAME=/tmp/test_monitor
FILE=test_monitorFILE=test_monitor
FILENAME=/tmp/$FILEContext
StackExchange Code Review Q#118751, answer score: 2
Revisions (0)
No revisions yet.