Extract data from api using shell scripting

Question

So we have many vlans at work, often it is nice to see which ip ranges are associated with said vlan. We have a well developed api for displaying information about our network and thus I wrote a small script to extract this information.

The only issue is that to access the API we require a token, no older than X hours. So, the script both need to check this token, and renew it if need be, before running the command.

The API is not public but the response data looks like

Query

v1/networks/?vlan=119

Response

{
  "count": 2,
  "next": null,
  "previous": null,
  "results": [
    {
      "id": 13,
      "excluded_ranges": [],
      "created_at": "2019-10-16T14:57:55.932564+02:00",
      "updated_at": "2019-10-23T12:23:30.914823+02:00",
      "network": "yyy.xxx.2.64/27",
      "description": "core-3",
      "vlan": 119,
      "dns_delegated": false,
      "category": "so",
      "location": "",
      "frozen": false,
      "reserved": 3
    },
    {
      "id": 1110,
      "excluded_ranges": [],
      "created_at": "2019-10-16T14:57:55.932564+02:00",
      "updated_at": "2019-10-10T12:44:18.912178+02:00",
      "network": "xxx:yyy:zzz:540::/64",
      "description": "core-3",
      "vlan": 119,
      "dns_delegated": false,
      "category": "",
      "location": "",
      "frozen": false,
      "reserved": 3
    }
  ]
}

The code is used as ./ulan.sh 119 with response

yyy.xxx.2.64/27       core-3
xxx:yyy:zzz:540::/64  core-3

The code works as intended, it passes shellcheck and is formated using shfmt -l -w -i 4 ulan.sh. However, I feel there is still much to be improved.

Full code

#!/bin/bash
#
# Author: Nebuchadnezzar
#
# License: GPLv3+

# Set PATH
PATH=/bin:/usr/bin
export PATH

# Usage
usage=$(
    cat <<-EOF
$(basename "$0") [-h]

List ip-range and description associated with the uio drifted vlan

where:
    -h,  --help                     show this help text

examples:
  command:
    ulan 119

  ouput:
    yyy.xxx.2.64/27       core-3
    xxx:yyy:zzz:540::/64  core-3
EOF
)

# If no arguments display usage
if [ -z "$1" ]; then
    echo >&2 "$usage"
    exit 1
fi

# Help
if [ "$1" == "-h" ] || [ "$1" == "--help" ]; then
    echo "$usage"
    exit 0
fi

MREG_TOKEN_PATH="$HOME/.mreg-cli_auth_token"
MREG_VERSION="1"
MREG_API="https://mreg.uio.no/api"
VLAN=$1

function create_mreg_token {
    echo "Connecting to https://mreg.uio.no"
    printf "Username: " >&2
    read -r USER
    printf 'Password for %s:' "$USER"
    trap 'stty echo' INT EXIT
    stty -echo
    read -r MYPASS
    printf "\n"
    token_object=$(
        curl -X POST -H "Content-Type: application/json" --silent \
            --data "{\"username\":\"$USER\",\"password\":\"$MYPASS\"}" \
            "${MREG_API}/token-auth/"
    )
    token=$(echo "$token_object" | jq -r '.token')
    echo -n "$USER¤$token" >"$MREG_TOKEN_PATH"
}

function check_vlan {
    if [ $# -eq 0 ]; then
        echo "No arguments supplied"
        exit 1
    fi
    vlan="$1"
    if [ ! -f "$MREG_TOKEN_PATH" ]; then
        touch "$MREG_TOKEN_PATH"
    fi
    mreg_token=$(cut -d "¤" -f2 "$MREG_TOKEN_PATH")
    mreg_query="${MREG_API}/v${MREG_VERSION}/networks/?vlan=$vlan"
    mreg_response_http_code=$(
        curl \
            --write-out "%{http_code}" --silent --output /dev/null \
            -s -H "Authorization: Token ${mreg_token}" \
            "${mreg_query}"
    )
    if [ "$mreg_response_http_code" -ne 200 ]; then
        echo "Invalid mreg token! Creating.."
        create_mreg_token
        mreg_token=$(cut -d "¤" -f2 "$MREG_TOKEN_PATH")
    fi
    mreg_response=$(
        curl -s -H "Authorization: Token ${mreg_token}" \ 
        "$mreg_query"
    )
    result=$(echo "$mreg_response" | jq -c .results)
    for row in $(echo "${result}" | jq -r '.[] | @base64'); do
        _jq() {
            echo "${row}" | base64 --decode | jq -r "${1}"
        }
        echo "$(_jq '.network') $(_jq '.description')"
    done | column -t
}

check_vlan "${VLAN}"

Answer 1

Improve the validation of command line arguments

The condition means "the first argument is not empty", but the comment says something else, which is confusing:

# If no arguments display usage
if [ -z "$1" ]; then

It would be better to fix the condition (as you did elsewhere already):

if [ $# -eq 0 ]; then

Keep in mind that this changes the behavior of the program, because now the condition will let through invocations with an empty first argument.

I think a good solution will be to make the input validation stronger, and ensure that the VLAN argument is actually an integer:

if ! [[ $1 =~ ^[0-9]+$ ]] ; then
   echo >&2 "error: VLAN must be a positive integer; got $1"
   exit 1
fi

If there is a valid range of numbers, then you might want to add another condition to validate that too.

Finally, I would put VLAN=$1 right after the input validation, while the meaning of $1 is the most fresh in the reader's head.

Improve the usage message

The first line of the usage message is:

$(basename "$0") [-h]

It doesn't mention that a VLAN parameter is required. It would be better to add VLAN on that line, and in the description clarify the expected values.

Later the example text says:

examples:
  command:
    ulan 119

There the use of literal "ulan" can be different from $(basename "$0") earlier above. This can be confusing, and it would be good to avoid by using the same expression, a variable in both places, which stores the output of $(basename "$0").

Avoid running programs in a loop

Executing commands in a loop can be costly, because of the overhead of running a process. So it's good to look for opportunities to avoid that.

Instead of this:

result=$(echo "$mreg_response" | jq -c .results)
for row in $(echo "${result}" | jq -r '.[] | @base64'); do
    _jq() {
        echo "${row}" | base64 --decode | jq -r "${1}"
    }
    echo "$(_jq '.network') $(_jq '.description')"
done | column -t

You can write simpler and faster as:

jq -r '.results[] | (.network + " " + .description)' <<< "$mreg_response" | column -t

Consider validating required non-standard commands

jq is not a program that's generally installed by default in operating systems. It's good to add a check and fail early in the script if the program is missing.

Avoid unnecessary HTTP calls

When the file $MREG_TOKEN_PATH does not exist, this code creates an empty file, reads from it, and makes an HTTP call:

if [ ! -f "$MREG_TOKEN_PATH" ]; then
    touch "$MREG_TOKEN_PATH"
fi
mreg_token=$(cut -d "¤" -f2 "$MREG_TOKEN_PATH")
mreg_query="${MREG_API}/v${MREG_VERSION}/networks/?vlan=$vlan"
mreg_response_http_code=$(
    curl \
        --write-out "%{http_code}" --silent --output /dev/null \
        -s -H "Authorization: Token ${mreg_token}" \
        "${mreg_query}"
)

Can that HTTP call ever succeed with an empty token? If not, then I suggest to rework the code accordingly.

Add error handling for create_mreg_token

check_vlan calls this function, and assumes that it successfully created a token, but that's not guaranteed.

A better solution would be for create_mreg_token to raise an error if it could not create a token and exit.

Also, it could echo the token, so that check_vlan could use the output, rather than parsing the password file.

Simplify reading input using builtin features of read

Instead of printing a prompt with printf "Username: ", you can use read -p "Username: " username.

To avoid echoing the password as the user types, you can use the -s flag:

read -sp "Password: " password

Write actionable error messages

In this code:

if [ $# -eq 0 ]; then
    echo "No arguments supplied"
    exit 1
fi

The error message implies that there must be some arguments, but it doesn't make it clear enough what is expected.

Print error messages on stderr consistently

In some places the program prints error messages on stdout, for example here:

if [ $# -eq 0 ]; then
    echo "No arguments supplied"
    exit 1
fi

It would be good to make that consistent.

Consider not storing the username at all

create_mreg_token reads the username from stdin, makes an HTTP call to get a token, stores the username and the token in a file.

check_vlan extracts the token from the file, and disregards the username.

If the username is not needed outside this script, then no need to store it, and the script can be a bit simpler.

(The next two sections of this review only arise because of the added complexity of storing the username.)

Use a constant for separator characters

One part of the code creates a file with a field separator:

echo -n "$USER¤$token" >"$MREG_TOKEN_PATH"

Another part of the code reads from that file, expecting the same separator:

mreg_token=$(cut -d "¤" -f2 "$MREG_TOKEN_PATH")

It would be good to put the separator in a constant, that way the reader can be sure that different parts of the code will use the same value.

Avoid unreadable characters in scripts when possible

This command uses an unreadable separator character:

echo -n "$USER¤$token" >"$MREG_TOKEN_PATH"

I would like to be able to copy-paste script snippets correctly, but if I cannot read a character I'm not confident it comes out correctly.

Use lowercase variable names for local variables

Local variables in functions should be declared using the local keyword, to avoid accidental reading or writing to global variables, and lowercase naming is recommended.

Use modern style function declaration

Instead of:

function foo {

The preferred form is:

foo() {

Use more blank lines

The two functions in the posted code do multiple logical steps, and have no blank lines. This makes them a bit difficult to read. Consider adding some blank lines, like you did in the read of the code.

Avoid unnecessary comments

These comments in this code don't add value, I suggest to delete them:

# Set PATH
PATH=/bin:/usr/bin
export PATH

# Usage
usage=$(...)

# If no arguments display usage
if [ -z "$1" ]; then
    ...

# Help
if [ "$1" == "-h" ] || [ "$1" == "--help" ]; then
    ...

Don't use echo if you need to use any flags

The flags of echo such as -n and -e don't work reliably in all systems. It's better to avoid using them.

The script uses echo -n in one place, and you can safely drop the -n without changing the behavior. If for some reason outside this script it's important to not write a terminating newline, then use printf instead.

Answer 2

@janos has already given great feedback that covers basically everything I would say and more when it comes to the shell script. I'd only add suggestions to factor out the error exits into a function (DRY), and to consider [[ test ]] && function (or ||) for asserts, and to think about what should be going to stdout and stderr (and why).

I do, however, want to raise a very different suggestion; shell scripts are not great for interfacing with neither APIs nor with JSON data, so maybe consider a different language? If you went for, for example, python, it'd give you improvements e.g. with argument handling, query parameters, testability, and would have native support for interfacing with JSON.