installer/scripts/maintenance/: AWS tag and delete scripts #1239

estroz · 2017-06-29T01:05:56Z

maintenance/: scripts that tag and delete AWS resources using the grafiti Docker container (see https://github.com/coreos/grafiti), and a script that only tags Route53 hosted zones using the AWS CLI.

These scripts can be used to tag and delete AWS resources by region, provided you have access to quay.io/coreos/grafiti.

Quentin-M · 2017-06-29T18:29:01Z

installer/scripts/maintenance/clean-aws.sh

+fi
+
+echo "Tags to delete:"
+cat "$tag_file"


Do you think it'd be valuable to add a prompt here to ask whether it is going to do what was intended? As we also run this in CI, could add a -f flag to bypass it.

As in prompt the user with a printout of tag/config files before actually deleting resources with those tags?

Yeah. Tags to delete: ... Are you sure?

Yup I can add that

The last worldwide S3 outage was due to a cleanup/scale-down user-operated script that was given 'a little more'.

Quentin-M · 2017-06-29T18:29:41Z

installer/scripts/maintenance/clean-aws.sh

+
+set +eux
+
+rm -rf $tmp_dir


You already trap that, right?

Quentin-M · 2017-06-29T18:30:52Z

installer/scripts/maintenance/clean-aws.sh

+    --tag-file        A file containing a TagFilter list.
+    --date-override   (optional) Date of the format YYYY-MM-DD that overrides
+                      the default tag value. Only use if --tag-file is not used.
+    --workspace-dir   (optional) Parent directory for a temporary directory.


I feel like the description is not super clear, especially given this script will remove that directory. What if I pass /tmp there? :'(

the script will create a "${workspace}/config" dir and only remove that

Quentin-M · 2017-06-29T18:31:22Z

installer/scripts/maintenance/tag-aws.sh

+    --end-hour        Integer hour to end looking at CloudTrail logs.
+    --date-override   (optional) Date of the format YYYY-MM-DD that overrides
+                      the default tag value.
+    --workspace-dir   (optional) Parent directory for a temporary directory.


Quentin-M · 2017-06-29T18:33:21Z

installer/scripts/maintenance/tag-aws.sh

+
+set +eux
+
+rm -rf "$tmp_dir"


Quentin-M · 2017-06-29T18:34:05Z

installer/scripts/maintenance/clean-aws.sh

+	-e AWS_ACCESS_KEY_ID="$AWS_ACCESS_KEY_ID" \
+	-e AWS_SECRET_ACCESS_KEY="$AWS_SECRET_ACCESS_KEY" \
+	-e CONFIG_FILE="/tmp/config/delete-config-${region}.toml" \
+	-e TAG_FILE="/tmp/config/$(basename $tag_file)" \


Hardcoded /tmp/config, versus $tmp_dir.

once the file is in the Docker container, its location does not matter

oh yeah cool soz, didn't pay attention

Quentin-M · 2017-06-29T18:34:15Z

installer/scripts/maintenance/tag-aws.sh

+	-e AWS_ACCESS_KEY_ID="$AWS_ACCESS_KEY_ID" \
+	-e AWS_SECRET_ACCESS_KEY="$AWS_SECRET_ACCESS_KEY" \
+	-e CONFIG_FILE="/tmp/config/tag-config-${region}.toml" \
+	-e TAG_FILE="/tmp/config/tag-exclude.json" \


Quentin-M · 2017-06-29T18:38:54Z

Great stuff, thanks!

Could you explain me why do we need installer/scripts/maintenance/tag-route53-hosted-zones.sh?

kans · 2017-06-29T19:55:21Z

installer/scripts/maintenance/clean-aws.sh

+    --aws-region      The AWS region you wish to query for taggable resources. This
+                      flag is optional if AWS_REGION is set.
+    --tag-file        A file containing a TagFilter list.
+    --date-override   (optional) Date of the format YYYY-MM-DD that overrides


A bit more description would be nice here too.

sym3tri · 2017-06-29T19:59:09Z

installer/scripts/maintenance/clean-aws.sh

+[grafiti]
+region = "$region"
+backoffTime = 500
+EOF


nit: Would be nice if some day grafiti didn't require config files, and you could pass these as flags or env vars to avoid needing stuff like this.

or if at least some of them could be overridden with flags

I agree. I'll make an issue in coreos/grafiti, unless you'd like to

sym3tri · 2017-06-29T20:02:41Z

installer/scripts/maintenance/clean-aws.sh

@@ -0,0 +1,111 @@
+#!/usr/bin/env bash


In general it would be nice if most of this logic could be moved into Go or accommodated by grafiti in some way. Having a big bash wrapper script is not fun.

@sym3tri would you like to postpone merging these scripts until that time? I'm going to add env variable support for certain config file fields asap.

@estroz https://github.com/spf13/viper is your best friend. CLI arguments + Env-based configuration + YAML/JSON/TOML/HCL! configuration in one tool. Usually used with https://github.com/spf13/cobra too.

@estroz Is this concern resolved? Are we addressing this in another PR?

@Quentin-M @sym3tri final thoughts? Writing this in Go wouldn't be difficult, but perhaps it should be written in Ruby as per @mxinden's work

estroz · 2017-06-29T20:21:10Z

@Quentin-M grafiti can't parse Route53 events because they are not available via the CloudTrail API (only via CloudTrail S3 log files), so I wrote that script to tag all hosted zones in the same manner as tag-aws.sh does: expirationDate: some-date-string

estroz · 2017-07-26T16:44:08Z

@mxinden

mxinden · 2017-07-27T07:45:27Z

@estroz What is the difference between the scripts clean-aws.sh, tag-aws.sh and the Jenkins jobs clean-aws, tag-aws? Are they just out of sync or do they serve two different use cases?

If I understand correctly you want to check the clean-aws and tag-aws job as a Jenkins DSL into this repository, right?

estroz · 2017-08-02T16:54:25Z

@mxinden yes these scripts correspond to those Jenkins jobs. These scripts are meant to be more versatile than the Jenkins jobs, i.e. we can use them from the command line if needed.

cpanato · 2017-08-03T12:44:10Z

@estroz One comment/question, the grafiti image is public? or do we need login into quay to get the image?

thanks

estroz · 2017-08-03T16:41:53Z

@cpanato the image is indeed public

mxinden · 2017-08-04T12:10:18Z

@estroz What is the current status of this PR? Are there any blockers?

estroz · 2017-08-04T23:46:47Z

@mxinden PR is good to go

mxinden · 2017-08-07T08:05:02Z

Removing label do-not-test, so shellcheck will run over the changes.

mxinden · 2017-08-07T08:09:31Z

@estroz Could you address the failing shellcheck tests?
https://jenkins-tectonic-installer.prod.coreos.systems/blue/organizations/jenkins/tectonic-installer/detail/PR-1239/15/pipeline#step-18-log-147

scripts/maintenance: scripts that tag and delete AWS resources using the `grafiti` Docker container (see https://github.com/coreos/grafiti), and a script that only tags Route53 hosted zones using the AWS CLI.

mxinden · 2017-08-10T09:56:27Z

@estroz Thanks for fixing the shellcheck issues.

If there are no further objections by anyone I would like to merge here in order to proceed with #1564 as well.

@estroz Outside of this PR: Do you have time to move this logic into the Grafiti go binary so that it can be both configured via a config file as well as via command line parameters? Would that be an option?

estroz · 2017-08-10T17:21:30Z

@mxinden I'm reworking some of grafiti's configuration options. I'll take into account this use case. We can revisit this issue when I'm finished.

For now let's merge this!

The typos are from 82bdd9f (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.

…ing 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 82bdd9f (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.

The 'echo -e' calls landed with the script in 82bdd9f (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 ;).

So folks don't need to bother setting --aws-region or $AWS_REGION to use their usual default. Docs for the config file settings are in [1]. I've also adjusted the logic so that the precedence is: 1. --aws-region, falling back to 2. $AWS_REGION, falling back to 3. ~/.aws Previously, $AWS_REGION took precedence, and has since the scripts landed in 82bdd9f (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239). But having environment variables override explicitly-set command line options is not idiomatic. [1]: https://docs.aws.amazon.com/cli/latest/userguide/cli-config-files.html

tag-aws.sh is using grafiti, whose tagPatterns takes jq expressions [1]. We've been using strftime since the script landed in 82bdd9f (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239). jq's strftime doesn't respect your configured $TZ, but the coming jq 1.6 will add strflocaltime which does [2,3]. jq uses seconds since the epoch for date-time values [4]. You can test the new construct with: $ jq --null-input --raw-output 'now + 24*60*60 | strftime("%Y-%m-%d")' 2018-07-27 -d is not part of the POSIX date specification [5], but it (and the 'tomorrow' value) are supported by GNU Coreutils [6,7]. We've been using -d in clean-aws.sh for a while now, so this is now a new dependency. I've also dropped date_override, since we can just set date_string directly. And I've shuffled around some of the conditionals to avoid calling the 'date' and 'jq' commands needlessly when --date-override is set. I've also replaced the multiple date calls in clean-aws.sh with a single call to jq. jq was already a required dependency for this script, and only needing a single child process is much faster: $ time for i in $(seq 100); do A="$(jq --null-input '[["%Y-%m-%d", "%Y-%-m-%-d", "%m-%d-%Y", "%m-%-d-%-Y", "%-m-%-d-%-Y", "%d-%m-%Y", "%d-%-m-%-Y"][] | . as $format | [now, now - 24*60*60][] | strftime($format)]')"; done real 0m0.256s user 0m0.186s sys 0m0.077s $ time for i in $(seq 100); do A="$(date "+%Y-%m-%d" -d "-1 day")\",\"$(date "+%Y-%-m-%-d" -d "-1 day")\",\"$(date "+%m-%-d-%-Y" -d "-1 day")\",\"$(date "+%-m-%-d-%-Y" -d "-1 day")\",\"$(date "+%d-%m-%-Y" -d "-1 day")\",\"$(date "+%d-%-m-%-Y" -d "-1 day")\",\"$(date +%m-%d-%Y)\",\"$(date +%d-%m-%Y)\",\"$(date +%d-%-m-%Y)\",\"$(date +%Y-%m-%d)\",\"$(date +%Y-%-m-%-d)"; done real 0m1.358s user 0m0.604s sys 0m0.832s And that's despite the fact that the old approach skipped some formats for today (e.g. %m-%-d-%-Y had been only used to format yesterday). The plethora of date formats are mostly from 3995263 (ci: add more date format when grafiti apply the cleanning, 2017-09-12, coreos/tectonic-installer#1890), although we've had some since 82bdd9f. The motivation seems to be matching human-generated tags [8], which are less reliably formatted. [1]: https://github.com/coreos/grafiti/blob/89a8bc92ad7fde49cd3dd78197c6b3616a857f36/README.md#configure-grafiti [2]: https://github.com/stedolan/jq/wiki/FAQ [3]: jqlang/jq@06f2060 [4]: https://stedolan.github.io/jq/manual/v1.5/#Dates [5]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/date.html [6]: https://www.gnu.org/software/coreutils/manual/html_node/Options-for-date.html [7]: https://www.gnu.org/software/coreutils/manual/html_node/Relative-items-in-date-strings.html [8]: coreos/tectonic-installer#1890 (comment)

So folks don't need to bother setting --aws-region or $AWS_REGION to use their usual default. Docs for the config file settings are in [1]. I've also adjusted the logic so that the precedence is: 1. --aws-region, falling back to 2. $AWS_REGION, falling back to 3. ~/.aws Previously, $AWS_REGION took precedence, and has since the scripts landed in 82bdd9f (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239). But having environment variables override explicitly-set command line options is not idiomatic. [1]: https://docs.aws.amazon.com/cli/latest/userguide/cli-config-files.html

tag-aws.sh is using grafiti, whose tagPatterns takes jq expressions [1]. We've been using strftime since the script landed in 82bdd9f (installer/scripts: AWS tag and delete scripts, 2017-06-28, coreos/tectonic-installer#1239). jq's strftime doesn't respect your configured $TZ, but the coming jq 1.6 will add strflocaltime which does [2,3]. jq uses seconds since the epoch for date-time values [4]. You can test the new construct with: $ jq --null-input --raw-output 'now + 24*60*60 | strftime("%Y-%m-%d")' 2018-07-27 -d is not part of the POSIX date specification [5], but it (and the 'tomorrow' value) are supported by GNU Coreutils [6,7]. We've been using -d in clean-aws.sh for a while now, so this is now a new dependency. I've also dropped date_override, since we can just set date_string directly. And I've shuffled around some of the conditionals to avoid calling the 'date' and 'jq' commands needlessly when --date-override is set. I've also replaced the multiple date calls in clean-aws.sh with a single call to jq. jq was already a required dependency for this script, and only needing a single child process is much faster: $ time for i in $(seq 100); do A="$(jq --null-input '[["%Y-%m-%d", "%Y-%-m-%-d", "%m-%d-%Y", "%m-%-d-%-Y", "%-m-%-d-%-Y", "%d-%m-%Y", "%d-%-m-%-Y"][] | . as $format | [now, now - 24*60*60][] | strftime($format)]')"; done real 0m0.256s user 0m0.186s sys 0m0.077s $ time for i in $(seq 100); do A="$(date "+%Y-%m-%d" -d "-1 day")\",\"$(date "+%Y-%-m-%-d" -d "-1 day")\",\"$(date "+%m-%-d-%-Y" -d "-1 day")\",\"$(date "+%-m-%-d-%-Y" -d "-1 day")\",\"$(date "+%d-%m-%-Y" -d "-1 day")\",\"$(date "+%d-%-m-%-Y" -d "-1 day")\",\"$(date +%m-%d-%Y)\",\"$(date +%d-%m-%Y)\",\"$(date +%d-%-m-%Y)\",\"$(date +%Y-%m-%d)\",\"$(date +%Y-%-m-%-d)"; done real 0m1.358s user 0m0.604s sys 0m0.832s And that's despite the fact that the old approach skipped some formats for today (e.g. %m-%-d-%-Y had been only used to format yesterday). The plethora of date formats are mostly from 3995263 (ci: add more date format when grafiti apply the cleanning, 2017-09-12, coreos/tectonic-installer#1890), although we've had some since 82bdd9f. The motivation seems to be matching human-generated tags [8], which are less reliably formatted. [1]: https://github.com/coreos/grafiti/blob/89a8bc92ad7fde49cd3dd78197c6b3616a857f36/README.md#configure-grafiti [2]: https://github.com/stedolan/jq/wiki/FAQ [3]: jqlang/jq@06f2060 [4]: https://stedolan.github.io/jq/manual/v1.5/#Dates [5]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/date.html [6]: https://www.gnu.org/software/coreutils/manual/html_node/Options-for-date.html [7]: https://www.gnu.org/software/coreutils/manual/html_node/Relative-items-in-date-strings.html [8]: coreos/tectonic-installer#1890 (comment)

estroz requested review from Quentin-M, kans, squat and sym3tri June 29, 2017 01:05

estroz force-pushed the grafiti-clean-script branch from 92195ff to 57f7781 Compare June 29, 2017 01:07

Quentin-M reviewed Jun 29, 2017

View reviewed changes

installer/scripts/maintenance/tag-aws.sh Outdated

set +eux

rm -rf "$tmp_dir"

Copy link

Contributor

Quentin-M Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Quentin-M reviewed Jun 29, 2017

View reviewed changes

kans reviewed Jun 29, 2017

View reviewed changes

sym3tri reviewed Jun 29, 2017

View reviewed changes

estroz force-pushed the grafiti-clean-script branch 2 times, most recently from 185050f to 46d17e1 Compare June 29, 2017 20:20

estroz force-pushed the grafiti-clean-script branch 4 times, most recently from 3fefd29 to d3bd9df Compare June 29, 2017 23:58

estroz force-pushed the grafiti-clean-script branch 2 times, most recently from 7facba8 to a20abb2 Compare August 2, 2017 16:51

estroz force-pushed the grafiti-clean-script branch from a20abb2 to 9ee6d53 Compare August 2, 2017 19:06

estroz added the do-not-test label Aug 2, 2017

estroz mentioned this pull request Aug 2, 2017

tests/ci: add tag/clean aws jenkins jobs as a code. #1564

Merged

estroz force-pushed the grafiti-clean-script branch 3 times, most recently from 57c44af to 73c46ac Compare August 4, 2017 23:46

mxinden removed the do-not-test label Aug 7, 2017

installer/scripts: AWS tag and delete scripts

82bdd9f

scripts/maintenance: scripts that tag and delete AWS resources using the `grafiti` Docker container (see https://github.com/coreos/grafiti), and a script that only tags Route53 hosted zones using the AWS CLI.

estroz force-pushed the grafiti-clean-script branch from 73c46ac to 82bdd9f Compare August 7, 2017 18:06

mxinden approved these changes Aug 11, 2017

View reviewed changes

mxinden merged commit c41dc5b into coreos:master Aug 11, 2017

wking mentioned this pull request Jul 26, 2018

scripts/maintenance: Support ~/.aws configs for region openshift/installer#75

Merged

wking mentioned this pull request Jul 26, 2018

scripts/maintenance: Fix dates (tomorrow vs. today, etc.) openshift/installer#77

Merged

installer/scripts/maintenance/: AWS tag and delete scripts #1239

installer/scripts/maintenance/: AWS tag and delete scripts #1239

Uh oh!

Conversation

estroz commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quentin-M Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quentin-M commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz commented Jun 29, 2017

Uh oh!

estroz commented Jul 26, 2017

Uh oh!

mxinden commented Jul 27, 2017

Uh oh!

estroz commented Aug 2, 2017

Uh oh!

cpanato commented Aug 3, 2017

Uh oh!

estroz commented Aug 3, 2017

Uh oh!

mxinden commented Aug 4, 2017

Uh oh!

estroz commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxinden commented Aug 7, 2017

Uh oh!

mxinden commented Aug 7, 2017

Uh oh!

mxinden commented Aug 10, 2017

Quentin-M Jun 29, 2017 •

edited

Loading

estroz commented Aug 4, 2017 •

edited

Loading

estroz commented Aug 10, 2017 •

edited

Loading