Connecting to EC2 instances using the instance name instead of IP

Question

I'm an intensive user of AWS EC2 instances, many instances are launched, stopped, repurposed, etc.

To connect to any instance using SSH I must keep track of their IPs.

The bash script I wrote (following the question I asked on SO) uses aws-cli to do the heavy lifting for me, leaving me to remember only the logical names I've given to my instances.

Here is the main code, including the auto-completion code:

# connect to machine
function sash {
  if [ -z $1 ]; then
    echo "Please enter machine name"
    return 1
  fi
  local instance ip pem
  instance=$(aws ec2 describe-instances --filters "Name=tag:Name,Values=$1" "Name=instance-state-name,Values=running" --query 'Reservations[*].Instances[].[KeyName,PublicIpAddress]' --output text)

  if [ -z "${instance}" ]; then
    echo Could not find an instance named $1
    return 1
  else
    ip=$(echo $instance | awk '{print $2}')
    pem=$(echo $instance | awk '{print $1}')

    echo "Connecting to $1 ($ip)"
    ssh -i ~/.aws/$pem.pem ubuntu@$ip
  fi
}

function clear_sash {
  unset -v _sash_instances
}

# completion command
function _sash {
  if [ -z "${_sash_instances}" ]; then
    _sash_instances="$( aws ec2 describe-tags --filter Name=key,Values=Name Name=resource-type,Values=instance --query Tags[].Value --output text )"
  fi

  local curw
  COMPREPLY=()
  curw=${COMP_WORDS[COMP_CWORD]}
  COMPREPLY=($(compgen -W "${_sash_instances}" -- $curw))
  return 0
}

complete -F _sash sash

The main function (sash) takes the first parameter, and queries aws ec2 for a machine with a 'Name' tag with that value, extracts its public ip and pem file, and calls the proper ssh command.

The auto-completion command (_sash) enumerates the names of all EC2 machines, and keeps them in a cache in the scope of the shell.

A small helper command (clear_sash) is used to clear the cache for the auto-complete.

Since this is the first function I've written in bash, I would love to hear some opinions on the code, style, caching decisions, etc.

Answer 1

It's mostly fine, but I would suggest some minor improvements.

Give a proper name to $1 early on, for example:

host=$1
if [ -z $host ]; then
  echo "Please enter machine name"
  return 1
fi

You use it in several places later and it will make the code more readable.

---

Instead of this:

if [ -z "${instance}" ]; then

you can use [[ ... ]] instead and drop the double quotes:

if [[ -z $instance ]]; then

---

This is a bit ugly:

ip=$(echo $instance | awk '{print $2}')
pem=$(echo $instance | awk '{print $1}')

It's ugly because you're spawning two awk processes for the same input. You could use a single read instead:

read pem ip junk <<< $instance

Or perhaps slightly cleaner to read into an array:

read -a arr <<< $instance
ip=${arr[1]}
pem=${arr[0]}

The array solution is especially good if the indexes of ip and pem are dynamic, for example if they come from variables:

read -a arr <<< $instance
# given: ip_idx=2 and pem_idx=1
ip=${arr[$ip_idx - 1]}
pem=${arr[$pem_idx - 1]}

(It would be cleaner to have the *_idx 0-based, but I used this example for the sake of illustrating simple arithmetics in array indexes.)

For the record, here's my earlier uglier awk solution:

set -- $(awk '{print $1, $2}' <<< $instance)
pem=$1
ip=$2

This is not as good as read, because it spawns an awk process, and it overwrites the positional parameters $1, $2, ... Other variants using $pem_idx and $ip_idx:

set -- $(awk "{print \$$pem_idx, \$$ip_idx}" <<< $instance)
# or:
set -- $(awk -v ip_idx=$ip_idx -v pem_idx=$pem_idx '{print $(pem_idx), $(ip_idx)}' <<< $instance)

---

In this code:

local curw
COMPREPLY=()
curw=${COMP_WORDS[COMP_CWORD]}
COMPREPLY=($(compgen -W "${_sash_instances}" -- $curw))

The first COMPREPLY=() is unnecessary because you overwrite it soon after anyway.

Also, I think it's better to write local on the same line as the assignment:

local curw=${COMP_WORDS[COMP_CWORD]}

Actually, since the line fits within 80 characters, I'm not sure I would use the local curw at all:

COMPREPLY=($(compgen -W "${_sash_instances}" -- ${COMP_WORDS[COMP_CWORD]}))

---

The final return 0 is unnecessary if the last operation is successful. If the last operation is not successful, you probably want to let the function return failure anyway.