3
\$\begingroup\$

I'm new to bash scripting. I have pieced together a bash script to rsync my website files and databases.

Any guidance on ways I could improve or make my script more secure would be greatly appreciated.

#!/bin/bash

set -e

site_host=(
    "test1@grid.co.uk"
    "test2@grid.co.uk"
)

backup_dest=(
    "/Users/computername/Desktop/rsync/site1.co.uk"
    "/Users/computername/Desktop/rsync/site2.co.uk"
)

now=`date "+%d/%m/%Y %H:%M:%S"`
today=`date +"%d-%m-%Y"`
yesterday=`date -v -1d +"%d-%m-%Y"`
backup_old=`date -v -2d +"%d-%m-%Y"`

log="/Users/computername/Desktop/rsync/rsync.log"

site_count=${#site_host[@]}

db_creds=()
db_creds+=("")
db_creds+=("localhost,user,pass,dbname")

for (( i = 0; i < site_count; i++ )); do

    # Source and destination
    site_source="${site_host[$i]}:~/public_html"
    site_dest="${backup_dest[$i]}/$today/"

    # DB creds
    for val in ${db_creds[$i]}; do
        subset=($(echo $val | tr ',' ' '))
        db_host=${subset[0]}
        db_user=${subset[1]}
        db_pass=${subset[2]}
        db_name=${subset[3]}
    done

    if [ "$db_host" ]; then

        # Set DB seperate directory
        mkdir -p ${backup_dest[$i]}/$today/db

        # Database backup
        ssh -p22 ${site_host[$i]} "mysqldump \
                --host=$db_host \
                --user=$db_user \
                --password=$db_pass \
                --databases $db_name \
                --lock-tables \
                | bzip2" > ${backup_dest[$i]}/$today/db/$today.sql.bz2

        echo "$now - SUCCESS - DB Backup - ${backup_dest[$i]}/$today/db/$today.sql.bz2" >> $log 

    fi

    # Remove old backups    
    if [ -d "${backup_dest[$i]}/$backup_old" ]; then 

        rm -Rf ${backup_dest[$i]}/$backup_old; 

        echo "$now - DELETED - Full Backup - ${backup_dest[$i]}/$backup_old" >> $log 

    fi

    if  rsync -zavx -e 'ssh -p22' \
            --numeric-ids \
            --delete -r \
            --link-dest=../$yesterday $site_source $site_dest;
    then

        echo "$now - SUCCESS - File Backup - ${backup_dest[$i]}/$today" >> $log 

    else

        echo "$now - FAILED - File Backup - ${backup_dest[$i]}/$today" >> $log 

    fi

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

good

  • yay for specifying bash in your #! line.
  • nice indentation of loops and conditionals
  • good variables names
  • Thanks for splitting long commands onto multiple lines, which are also nicely indented. (Another way to to do this is to built up the command options in a variable and then pass that as the command argument.)

suggestions

  • I'm a big fan of https://www.shellcheck.net/ which is basically a linter for shell scripts. Don't worry about fixing all of its suggestions, but most of them are worth considering. My next few suggestions are based on its output.
  • Using backticks (`) for command substitution is not the current best practice. Using $() for command substitution is nicer since it matches the () used for subshells and it stands out a bit more. Lots of folks were confused by quotes being used for non-quoting things. SC2006 mentions a few other reasons.
  • When you do mkdir -p ${backup_dest[$i]}/$today/db on line 46, if one of the backup_dest had spaces in it you'd end up making two directories instead of one. It is best when doing variable substitutions in the shell to enclose them in double quotes. mkdir -p "${backup_dest[$i]}/$today/db" is safer. SC2086 explains more.
  • Are you expecting more db_creds in the future? The for loop at line 35 seems superfluous. Even if you had 99 entries in db_creds it is only going to set the db_* variables for the last one. If you want to do something for each one you'd have to put more of the work inside the loop. Or am I missing some intent here?
  • checking to make sure you got a db_host in line 43 is a good idea, but should there be an else do this that warns about it being missing?
  • does it matter if any of the stuff before the rsync fails? Do you want to exit with FAILED for anything before the rsync?
  • putting a bit more documentation in the code would be better, but the comments that are there are good.

Overall, an excellent job for someone new to shell scripting.

\$\endgroup\$
  • \$\begingroup\$ Hi Chicks. Thank you! I have been running over your points and applying and learning them as I go. The 'shell check' site is fantastic! Regarding your 6th point. Here are you suggesting that I check if the 'site source' and 'site destination' exist? If the db fails, I am logging this, so I am happy for the remainder to run. Correct me if I am way off here. Really appreciate your help. \$\endgroup\$ – ccdavies Jun 13 '18 at 14:23
  • \$\begingroup\$ Since rsync can fill or empty disks I tend to be pretty cautious about invoking it, but if this is fine for your use case so be it. It was just something to consider and not something necessarily wrong with the code. And it sounds like you considered it so cool. \$\endgroup\$ – chicks Jun 13 '18 at 15:23
  • \$\begingroup\$ Thanks. Is there anything you would suggest to protect the 'source' from rsync? \$\endgroup\$ – ccdavies Jun 13 '18 at 15:49
  • \$\begingroup\$ Am I correct in thinking that as I am using 'set -e', if the 'site_source' didn't exist, then the script would exit? \$\endgroup\$ – ccdavies Jun 13 '18 at 17:37
2
\$\begingroup\$

As the other review pointed out, nicely done! I have a few minor things to add on top.


It seems that site_host, backup_dest and db_creds are closely related: they will always have the same number of entries, and the i-th values of these arrays are treated together. db_creds is defined a bit further away from the other two. It would be better if it was right next to the others. Note that you can write db_creds in the same style as the others:

db_creds=(
    ""
    "localhost,user,pass,dbname"
)

The value $now is used at multiple places but does not actually represent "now". It is set once at the beginning of the script. If you really wanted to use "now" in the log messages, you could use a function instead:

now() {
    date "+%d/%m/%Y %H:%M:%S"
}

echo "$(now) - SUCCESS - DB Backup - ..."

The echo $val | tr ',' ' ' is not necessary to split the value. It would be more efficient to use substitution:

subset=(${val//,/ /})

Or, alternatively, you could define the values directly with spaces instead of commas:

db_creds=(
    ""
    "localhost user pass dbname"
)

subset=($val)
\$\endgroup\$
  • \$\begingroup\$ Hi Janos. Thank you! I have been looking at adding your suggestions (and learning why, how etc). The use of the function. If I swap out 'now=$(date "+%d/%m/%Y %H:%M:%S")' for the 'now()' variable nothing is output through '$now'. Is there a different way to output functions than variables? \$\endgroup\$ – ccdavies Jun 13 '18 at 14:19
  • \$\begingroup\$ @ccdavies oops I made a mistake, see my update. When it's a function, instead of echo "$now" write echo "$(now)" \$\endgroup\$ – janos Jun 13 '18 at 15:51

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.