From da45d540dc3e533606350d8e45168b1c1915f3a3 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 8 Jul 2018 13:39:16 -0700 Subject: [PATCH 1/6] scripts/maintenance: Fix $2 -> $1 for unrecognized options The typos are from 82bdd9fe (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239). While I'm touching these lines, also send their output to stderr instead of stdout (because we're reporting an error). Also exit nonzero in these unrecognized-option cases. --- scripts/maintenance/clean-aws.sh | 4 ++-- scripts/maintenance/tag-aws.sh | 4 ++-- scripts/maintenance/tag-route53-hosted-zones.sh | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/maintenance/clean-aws.sh b/scripts/maintenance/clean-aws.sh index 15a3f613eeb..de42ad22407 100755 --- a/scripts/maintenance/clean-aws.sh +++ b/scripts/maintenance/clean-aws.sh @@ -85,8 +85,8 @@ while [ $# -gt 0 ]; do dry_run="$1" ;; *) - echo "Flag '$2' is not supported." - exit + echo "Flag '$1' is not supported." >&2 + exit 1 ;; esac shift diff --git a/scripts/maintenance/tag-aws.sh b/scripts/maintenance/tag-aws.sh index 38f690e4de3..400374e937f 100755 --- a/scripts/maintenance/tag-aws.sh +++ b/scripts/maintenance/tag-aws.sh @@ -100,8 +100,8 @@ while [ $# -gt 0 ]; do dry_run="$1" ;; *) - echo "Flag '$2' is not supported." - exit + echo "Flag '$1' is not supported." >&2 + exit 1 ;; esac shift diff --git a/scripts/maintenance/tag-route53-hosted-zones.sh b/scripts/maintenance/tag-route53-hosted-zones.sh index 74480b68b6e..f9baa134d4c 100755 --- a/scripts/maintenance/tag-route53-hosted-zones.sh +++ b/scripts/maintenance/tag-route53-hosted-zones.sh @@ -38,8 +38,8 @@ while [ $# -gt 0 ]; do shift ;; *) - echo "Flag '$2' is not supported." - exit + echo "Flag '$1' is not supported." >&2 + exit 1 ;; esac shift From 2aea0527828227b14ce0faba142acb2a7563c69c Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 8 Jul 2018 13:59:20 -0700 Subject: [PATCH 2/6] scripts/maintenance/tag-route53-hosted-zones: Error messages for missing deps At least on Bash 4.4, the -V form writes a missing-command message to stderr: $ echo $BASH_VERSION 4.4.23(1)-release $ command -v does-not-exist >/dev/null $ command -V does-not-exist >/dev/null -bash: command: does-not-exist: not found Because we're not redirecting command's stderr in the script, it will fall through to the caller's stderr and make it easier for them to figure out what went wrong. Also write a "Missing required dependencies" string to stderr. We'd had a "Dependencies not installed." string literal from 82bdd9fe (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239), but it hadn't been written anywhere so it was effectively an internal comment. --- scripts/maintenance/tag-route53-hosted-zones.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/maintenance/tag-route53-hosted-zones.sh b/scripts/maintenance/tag-route53-hosted-zones.sh index f9baa134d4c..dd3dbf4b506 100755 --- a/scripts/maintenance/tag-route53-hosted-zones.sh +++ b/scripts/maintenance/tag-route53-hosted-zones.sh @@ -45,8 +45,8 @@ while [ $# -gt 0 ]; do shift done -if ! command -v jq > /dev/null || ! command -v aws > /dev/null; then - "Dependencies not installed." +if ! command -V jq >/dev/null || ! command -V aws >/dev/null; then + echo "Missing required dependencies" >&2 exit 1 fi From 7af2ce659d23585137ee26dffcdff859657df73c Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 8 Jul 2018 14:20:00 -0700 Subject: [PATCH 3/6] scripts/maintenance: Dependency checks for tag-aws.sh and clean-aws.sh Following the pattern set by tag-route53-hosted-zones.sh. I'm also following tag-route53-hosted-zones.sh in not testing for any POSIX commands we use, although it would be easy to add tests for them as well if we wanted. --- scripts/maintenance/clean-aws.sh | 6 ++++++ scripts/maintenance/tag-aws.sh | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/scripts/maintenance/clean-aws.sh b/scripts/maintenance/clean-aws.sh index de42ad22407..82edcc05bfb 100755 --- a/scripts/maintenance/clean-aws.sh +++ b/scripts/maintenance/clean-aws.sh @@ -4,6 +4,7 @@ usage() { cat </dev/null || ! command -V jq >/dev/null; then + echo "Missing required dependencies" >&2 + exit 1 +fi + if [ -n "$AWS_REGION" ]; then region="${AWS_REGION:-}" fi diff --git a/scripts/maintenance/tag-aws.sh b/scripts/maintenance/tag-aws.sh index 400374e937f..081bda30205 100755 --- a/scripts/maintenance/tag-aws.sh +++ b/scripts/maintenance/tag-aws.sh @@ -5,8 +5,8 @@ usage() { $(basename "$0") tags AWS resources with 'expirationDate: some-date-string', defaulting to the following days' date, and excludes all resources tagged with -tag keys/values specified in an 'exclude' file. Requires that both 'jq' and the -AWS CLI are installed. +tag keys/values specified in an 'exclude' file. Requires that 'docker' is +installed. AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environmental variables must be set. @@ -107,6 +107,11 @@ while [ $# -gt 0 ]; do shift done +if ! command -V docker >/dev/null; then + echo "Missing required dependencies" >&2 + exit 1 +fi + if [ -n "$AWS_REGION" ]; then region="${AWS_REGION:-}" fi From 04fa686ed9e8fa64a2eebc6bc3084d1d9acf733d Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 8 Jul 2018 14:53:00 -0700 Subject: [PATCH 4/6] scripts/maintenance: Write errors to stderr The interactive prompt means these scripts will probably be run from interactive terminals. But still, it's nice to send errors to stderr where they belong. Signed-off-by: W. Trevor King --- scripts/maintenance/clean-aws.sh | 8 ++++---- scripts/maintenance/tag-aws.sh | 8 ++++---- scripts/maintenance/tag-route53-hosted-zones.sh | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/scripts/maintenance/clean-aws.sh b/scripts/maintenance/clean-aws.sh index 82edcc05bfb..514a2ef618b 100755 --- a/scripts/maintenance/clean-aws.sh +++ b/scripts/maintenance/clean-aws.sh @@ -103,17 +103,17 @@ if [ -n "$AWS_REGION" ]; then fi if [ -z "$version" ]; then - echo "Grafiti image version required." + echo "Grafiti image version required." >&2 exit 1 fi if [ -z "$region" ]; then - echo "Must provide an AWS region, set the AWS_REGION, or set a region in your ~/.aws/config}" + echo "Must provide an AWS region, set the AWS_REGION, or set a region in your ~/.aws/config" >&2 exit 1 fi if [ -n "$tag_file" ] && [ -n "$date_override" ]; then - echo "Cannot use both --tag-file and --date-override flags simultaneously." + echo "Cannot use both --tag-file and --date-override flags simultaneously." >&2 exit 1 fi @@ -157,7 +157,7 @@ fi if [ ! $force ]; then read -rp "Proceed deleting these resources? [y/N]: " yn if [ "$yn" != "y" ]; then - echo "Aborting deletion and cleaning up." + echo "Aborting deletion and cleaning up." >&2 exit 1 fi fi diff --git a/scripts/maintenance/tag-aws.sh b/scripts/maintenance/tag-aws.sh index 081bda30205..7a0eb4054f0 100755 --- a/scripts/maintenance/tag-aws.sh +++ b/scripts/maintenance/tag-aws.sh @@ -117,17 +117,17 @@ if [ -n "$AWS_REGION" ]; then fi if [ -z "$version" ]; then - echo "Grafiti image version required." + echo "Grafiti image version required." >&2 exit 1 fi if [ -z "$start_hour" ] || [ -z "$end_hour" ]; then - echo "Start hour and end hour must be specified." + echo "Start hour and end hour must be specified." >&2 exit 1 fi if [ -z "$region" ]; then - echo "Must provide an AWS region, set the AWS_REGION, or set a region in your ~/.aws/config}" + echo "Must provide an AWS region, set the AWS_REGION, or set a region in your ~/.aws/config" >&2 exit 1 fi @@ -179,7 +179,7 @@ fi if [ ! $force ]; then read -rp "Proceed tagging these resources? [y/N]: " yn if [ "$yn" != "y" ]; then - echo "Aborting tagging and cleaning up." + echo "Aborting tagging and cleaning up." >&2 exit 1 fi fi diff --git a/scripts/maintenance/tag-route53-hosted-zones.sh b/scripts/maintenance/tag-route53-hosted-zones.sh index dd3dbf4b506..79714122741 100755 --- a/scripts/maintenance/tag-route53-hosted-zones.sh +++ b/scripts/maintenance/tag-route53-hosted-zones.sh @@ -68,7 +68,7 @@ echo "$tags" if [ ! $force ]; then read -rp "Proceed tagging these resources? [y/N]: " yn if [ "$yn" != "y" ]; then - echo "Aborting tagging and cleaning up." + echo "Aborting tagging and cleaning up." >&2 exit 1 fi fi @@ -91,7 +91,7 @@ for key in $(echo -e "$tags" | jq ".[].Key"); do --resource-id "${zone##*/}"; then echo "Tagged hosted zone ${zone##*/}" else - echo "Error tagging hosted zone ${zone##*/}" + echo "Error tagging hosted zone ${zone##*/}" >&2 fi fi done From 873a035df4fccac74415f5668f54f170e04b415f Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 8 Jul 2018 14:57:48 -0700 Subject: [PATCH 5/6] scripts/maintenance/tag-route53-hosted-zones: Drop '-e' from echo The 'echo -e' calls landed with the script in 82bdd9fe (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239). But $tags doesn't actually need any backslash expansion: $ date_string='2000-01-01' $ tags="[{\"Key\":\"expirationDate\",\"Value\":\"$date_string\"}]" $ echo "${tags}" [{"Key":"expirationDate","Value":"2000-01-01"}] Perhaps the original concern was over the \" in the tags definition, but those are expanded by the shell during that initialization. By the time we get around to invoking echo, there are no backslashes left (unless the user has injected some via --date-override, and we don't want to support that ;). --- scripts/maintenance/tag-route53-hosted-zones.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/maintenance/tag-route53-hosted-zones.sh b/scripts/maintenance/tag-route53-hosted-zones.sh index 79714122741..201c16c5d00 100755 --- a/scripts/maintenance/tag-route53-hosted-zones.sh +++ b/scripts/maintenance/tag-route53-hosted-zones.sh @@ -77,7 +77,7 @@ private_zones=$(aws route53 list-hosted-zones | \ jq ".HostedZones[] | select(.Config.PrivateZone == true) | .Id" | \ sed "s@\"@@g") -for key in $(echo -e "$tags" | jq ".[].Key"); do +for key in $(echo "$tags" | jq ".[].Key"); do for zone in $private_zones; do zone="${zone##*/}" is_not_tagged=$(aws route53 list-tags-for-resource \ @@ -87,7 +87,7 @@ for key in $(echo -e "$tags" | jq ".[].Key"); do if [ -z "$is_not_tagged" ]; then if aws route53 change-tags-for-resource \ --resource-type hostedzone \ - --add-tags "$(echo -e "$tags")" \ + --add-tags "$tags" \ --resource-id "${zone##*/}"; then echo "Tagged hosted zone ${zone##*/}" else From d541760f60192ecd83ad68656e43fd342f6e0872 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 8 Jul 2018 16:18:21 -0700 Subject: [PATCH 6/6] scripts/maintenance/*-aws: Drop --workspace-dir We haven't set $WORKSPACE since 1dea5c84 (tests: Remove unused smoke.sh + tfvars file, 2017-10-04, coreos/tectonic-installer#2036), so there's no longer a need for the --workspace-dir options. Users who care where the scratch files live can set $TMPDIR: $ man 7 environ | grep TMPDIR | sed 's/ */ /g' | cut -b -67 * TMPDIR influences the path prefix of names created by tempnam(3) I'm still calling readlink on the mktemp output in case $TMPDIR (or /tmp, if $TMPDIR is unset) is a symlink. I'm also fixing --config-file, --exclude-file, and --tag-file. Previously we were using: CONFIG_FILE="/tmp/config/$(basename "$config_file")" and similar. But inside the container, /tmp/config is coming from the $tmp_dir volume mount. And when --config-file was set, we weren't writing the referenced content into $tmp_dir. Now we always write the content into $tmp_dir, regardless of whether the content is user-supplied or the script's default. Also avoid some parallel-call races by avoiding a shared /tmp/config (or ${workspace}/config). With the old approach, the trap rm call for one invocation could blow away a /tmp/config used by another invocation. With this commit, we use mktemp to give a secure, unique $tmp_dir. And once we have that, we can hard-code the paths to the config, tag, and exclude files inside $tmp_dir. --- scripts/maintenance/clean-aws.sh | 30 ++++++---------- scripts/maintenance/tag-aws.sh | 34 +++++++------------ .../tag_clean_aws_grafiti_job.groovy | 1 - 3 files changed, 23 insertions(+), 42 deletions(-) mode change 100644 => 100755 tests/jenkins-jobs/maintenance/tag_clean_aws_grafiti_job.groovy diff --git a/scripts/maintenance/clean-aws.sh b/scripts/maintenance/clean-aws.sh index 514a2ef618b..e4b1f461644 100755 --- a/scripts/maintenance/clean-aws.sh +++ b/scripts/maintenance/clean-aws.sh @@ -31,9 +31,6 @@ Options: is replaced with either the following days' date or date-override. Only use if --tag-file is not used. - --workspace-dir (optional) Parent directory for a temporary directory. /tmp is - used by default. - --dry-run (optional) If set, grafiti will only do a dry run, i.e. not delete any resources. @@ -46,7 +43,6 @@ region= config_file= tag_file= date_override= -workspace= dry_run= while [ $# -gt 0 ]; do @@ -78,10 +74,6 @@ while [ $# -gt 0 ]; do date_override="${2:-}" shift ;; - --workspace-dir) - workspace="${2:-}" - shift - ;; --dry-run) dry_run="$1" ;; @@ -119,19 +111,19 @@ fi set -e -tmp_dir="/tmp/config" -if [ -n "$workspace" ]; then - tmp_dir="$(readlink -m "${workspace}/config")" -fi +tmp_dir="$(readlink -m "$(mktemp -d clean-aws-XXXXXXXXXX)")" mkdir -p "$tmp_dir" trap 'rm -rf "$tmp_dir"; exit' EXIT -if [ -z "$config_file" ]; then - config_file="$(mktemp -p "$tmp_dir" --suffix=.toml)" - echo "maxNumRequestRetries = 11" > "$config_file" +if [ -n "$config_file" ]; then + cat "$config_file" >"$tmp_dir/config.toml" +else + echo "maxNumRequestRetries = 11" >"$tmp_dir/config.toml" fi -if [ -z "$tag_file" ]; then +if [ -n "$tag_file" ]; then + cat "$tag_file" >"$tmp_dir/tag.json" +else tag_file="$(mktemp -p "$tmp_dir")" date_string="$(date "+%Y-%m-%d" -d "-1 day")\",\"$(date "+%Y-%-m-%-d" -d "-1 day")\", @@ -142,7 +134,7 @@ if [ -z "$tag_file" ]; then date_string="$date_override" fi - cat < "$tag_file" + cat <"$tmp_dir/tag.json" {"TagFilters":[{"Key":"expirationDate","Values":["${date_string}"]}]} EOF fi @@ -170,8 +162,8 @@ docker run -t --rm --name grafiti-deleter \ -e AWS_SECRET_ACCESS_KEY="$AWS_SECRET_ACCESS_KEY" \ -e AWS_SESSION_TOKEN="$AWS_SESSION_TOKEN" \ -e AWS_REGION="$region" \ - -e CONFIG_FILE="/tmp/config/$(basename "$config_file")" \ - -e TAG_FILE="/tmp/config/$(basename "$tag_file")" \ + -e CONFIG_FILE="/tmp/config/config.toml" \ + -e TAG_FILE="/tmp/config/tag.json" \ quay.io/coreos/grafiti:"${version}" \ bash -c "grafiti $dry_run --config \"\$CONFIG_FILE\" --ignore-errors delete --all-deps --delete-file \"\$TAG_FILE\"" diff --git a/scripts/maintenance/tag-aws.sh b/scripts/maintenance/tag-aws.sh index 7a0eb4054f0..885a6703aa2 100755 --- a/scripts/maintenance/tag-aws.sh +++ b/scripts/maintenance/tag-aws.sh @@ -35,9 +35,6 @@ Options: with 'expirationDate: some-date-string', where some-date-string is replaced with either the following days' date or date-override. - --workspace-dir (optional) Parent directory for a temporary directory. /tmp is - used by default. - --dry-run (optional) If set, grafiti will only do a dry run, i.e. not tag any resources. @@ -50,7 +47,6 @@ region= config_file= exclude_file= date_override= -workspace= start_hour=8 end_hour=1 dry_run= @@ -92,10 +88,6 @@ while [ $# -gt 0 ]; do date_override="${2:-}" shift ;; - --workspace-dir) - workspace="${2:-}" - shift - ;; --dry-run) dry_run="$1" ;; @@ -136,11 +128,7 @@ set -e # Tag all resources present in CloudTrail over the specified time period with the # following day's date as default, or with the DATE_VALUE_OVERRIDE value. # Format YYYY-MM-DD. -tmp_dir="/tmp/config" -if [ -n "$workspace" ]; then - tmp_dir="$(readlink -m "${workspace}/config")" -fi -mkdir -p "$tmp_dir" +tmp_dir="$(readlink -m "$(mktemp -d tag-aws-XXXXXXXXXX)")" trap 'rm -rf "$tmp_dir"; exit' EXIT date_string='now|strftime(\"%Y-%m-%d\")' @@ -150,9 +138,10 @@ fi # Configure grafiti to tag all resources created between START_HOUR and END_HOUR's # ago -if [ -z "$config_file" ]; then - config_file="$(mktemp -p "$tmp_dir" --suffix=.toml)" - cat < "$config_file" +if [ -n "$config_file" ]; then + cat "$config_file" >"$tmp_dir/config.toml" +else + cat <"$tmp_dir/config.toml" endHour = -${end_hour} startHour = -${start_hour} includeEvent = false @@ -164,13 +153,14 @@ fi # Exclusion file prevents tagging of resources that already have tags with the key # "expirationDate" -if [ -z "$exclude_file" ]; then - exclude_file="$(mktemp -p "$tmp_dir")" - echo '{"TagFilters":[{"Key":"expirationDate","Values":[]}]}' > "$exclude_file" +if [ -n "$exclude_file" ]; then + cat "$exclude_file" >"$tmp_dir/exclude" +else + echo '{"TagFilters":[{"Key":"expirationDate","Values":[]}]}' >"$tmp_dir/exclude" fi echo "Tagging resources with the following configuration:" -cat "$config_file" +cat "$tmp_dir/config.toml" if [ -n "$dry_run" ]; then echo "Dry run flag set. Not tagging any resources." @@ -192,8 +182,8 @@ docker run -t --rm --name grafiti-tagger \ -e AWS_SECRET_ACCESS_KEY="$AWS_SECRET_ACCESS_KEY" \ -e AWS_SESSION_TOKEN="$AWS_SESSION_TOKEN" \ -e AWS_REGION="$region" \ - -e CONFIG_FILE="/tmp/config/$(basename "$config_file")" \ - -e TAG_FILE="/tmp/config/$(basename "$exclude_file")" \ + -e CONFIG_FILE="/tmp/config/config.toml" \ + -e TAG_FILE="/tmp/config/exclude" \ quay.io/coreos/grafiti:"${version}" \ bash -c "grafiti --config \"\$CONFIG_FILE\" parse | \ grafiti --config \"\$CONFIG_FILE\" filter --ignore-file \"\$TAG_FILE\" | \ diff --git a/tests/jenkins-jobs/maintenance/tag_clean_aws_grafiti_job.groovy b/tests/jenkins-jobs/maintenance/tag_clean_aws_grafiti_job.groovy old mode 100644 new mode 100755 index 6c786791eb0..fe147a1381b --- a/tests/jenkins-jobs/maintenance/tag_clean_aws_grafiti_job.groovy +++ b/tests/jenkins-jobs/maintenance/tag_clean_aws_grafiti_job.groovy @@ -77,7 +77,6 @@ for region in "\${regions[@]}"; do \$SCRIPT_DIR/maintenance/\$TAG_CLEAN.sh \\ --grafiti-version "\$GRAFITI_VERSION" \\ --aws-region "\$region" \\ - --workspace-dir "\$WORKSPACE" \\ --force \\ \$DATE_OVERRIDE_FLAG done