3
\$\begingroup\$

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:

#!/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 <hostname> <username> <password> <webdav_dir>"; 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
esac

Can you suggest any improvements?

\$\endgroup\$
2
\$\begingroup\$

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

# create temp file
truncate -s 1M /tmp/test_monitor

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:

truncate -s 1M $FILENAME

If 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 <hostname> <username> <password> <webdav_dir>"; exit 1; }

HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4

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 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_monitor

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

FILE=test_monitor
FILENAME=/tmp/$FILE

I would go one step further and rename the variables as

FILENAME=test_monitor
FQFN=/tmp/$FILE

now 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 command config 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. :-)

\$\endgroup\$
2
\$\begingroup\$

Nice script!

In addition to @chicks' great review, I suggest to eliminate the duplication in the curl commands. They both share curl --fail -u $USERNAME:$PASSWORD and the base URL http://$HOSTNAME/$WEBDAVDIR.

  • Create BASEURL=http://$HOSTNAME/$WEBDAVDIR and reuse
  • Create a _curl function that calls curl --fail -u $USERNAME:$PASSWORD "$@", and replace your current calls with:
    • _curl -T $FILENAME $BASEURL/
    • _curl -X DELETE $BASEURL/$FILE
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.