Skip to content

feat(ops): wrap day-2 account-scope update-stack-set in a script - #110

Open
mintun-myo-2020 wants to merge 1 commit into
aws-samples:mainfrom
mintun-myo-2020:feat/map-tagger-scope-wrapper
Open

feat(ops): wrap day-2 account-scope update-stack-set in a script#110
mintun-myo-2020 wants to merge 1 commit into
aws-samples:mainfrom
mintun-myo-2020:feat/map-tagger-scope-wrapper

Conversation

@mintun-myo-2020

Copy link
Copy Markdown

Add scripts/map-tagger.sh, which takes plain account IDs and builds the documented update-stack-set command — the escaped ScopedAccountIds JSON and the full UsePreviousValue parameter list. Full-replacement semantics matching docs/INSTRUCTIONS.md; validates account IDs (12 digits or ALL). README day-2 section now leads with the script, keeping the raw CLI command as a fallback.

Description

Re-scoping a multi-account deployment currently means running a long CloudShell one-liner by hand: eight UsePreviousValue clauses plus a triple-escaped ParameterValue="[\"111...\",\"222...\"]". One wrong backslash silently resets a parameter to its template default. scripts/map-tagger.sh takes plain account IDs and generates the exact documented command, with input validation. It adds no AWS resources, IAM, or template changes: it only assembles an existing CLI call.

./scripts/map-tagger.sh <MPE_ID> <REGION> 111111111111 222222222222
./scripts/map-tagger.sh <MPE_ID> <REGION> ALL

Type of change

  • Bug fix
  • New resource type support
  • Security improvement
  • Documentation update
  • Other: ops/developer tooling (new helper script; not part of the deployed solution)

Testing

  • Tested end-to-end in a real AWS account (or CI E2E will cover this)
  • No false positives observed
  • CloudWatch logs reviewed for errors
  • no change to configurator

Checklist

  • CloudFormation template is valid (cfn-lint passes — checked by CI)
  • IAM permissions follow least-privilege (only tag actions added)
  • If adding a new service handler: corresponding resource added to .github/scripts/resource_groups/ for E2E coverage
  • CHANGELOG.md updated

CI Notes

Layer 1 (lint) runs immediately on every PR — ~1 min.
Layer 2 (E2E) runs when configurator.yaml or configurator.html changes — ~37 min across 7 AWS accounts. No AWS credentials needed.
If Layer 2 fails, download verification-report.json from the Actions run for details.

Add scripts/map-tagger.sh, which takes plain account IDs and builds the
documented update-stack-set command — the escaped ScopedAccountIds JSON and
the full UsePreviousValue parameter list. Full-replacement semantics matching
docs/INSTRUCTIONS.md; validates account IDs (12 digits or ALL). README day-2
section now leads with the script, keeping the raw CLI command as a fallback.

@ngjinshan ngjinshan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the script against the template source (src/js/deploy/template-main.js), the Lambda's scope semantics, and the #108 upgrade behavior. The problem is real and the shell is carefully written (set -euo pipefail, correct empty-array guard, describe-stack-set preflight, /dev/tty guard, byte-identical escaped output). Requesting changes for two issues that recreate the failure class this script exists to prevent.

1. Hardcoded 8-parameter list recreates the "silently reset a parameter" bug class (blocking)

The script already fetches CURRENT_KEYS from describe-stack-set, then ignores it and hardcodes the 8 known parameters. The next template parameter added (exactly as CentralAlertAccountId was in #108) will be silently reset to its default by this script — the same failure mode the PR description cites as motivation.

Since the keys are already in hand, build the list dynamically: every key in CURRENT_KEYS except ScopedAccountIds gets UsePreviousValue=true. The special-cased CentralAlertAccountId preserve-branch then falls out for free (only the "not set yet, ask to add" branch remains), and the script never goes stale.

2. Raw fallback command still silently resets CentralAlertAccountId (blocking)

The script handles CentralAlertAccountId, but the raw one-liner kept in README + INSTRUCTIONS (×3 locations) as the fallback does not. On any post-#108 stack with central alerting configured, running the documented fallback resets CentralAlertAccountId to '' and silently disables central alarm delivery — the CT6-class silent-alerting failure again. The fallback commands should gain ParameterKey=CentralAlertAccountId,UsePreviousValue=true (with a note that pre-#108 stacks must omit it), or at minimum a warning.

3. <MPE_ID> notation inconsistency

Everywhere else in the docs, <MPE_ID> is the bare ID — stack names are written map-auto-tagger-mig<MPE_ID>. The script requires the mig-prefixed value (^mig[a-zA-Z0-9]+$), but the new doc snippets say ./scripts/map-tagger.sh <MPE_ID> <REGION>. A user following the docs' own convention passes the bare ID and gets a validation error. Either write mig<MPE_ID> in the snippets or accept both in the script (prepend mig when missing).

4. ALL can be mixed with account IDs

./scripts/map-tagger.sh mig… us-east-1 ALL 111111111111 is accepted and produces ["ALL","111111111111"]. The Lambda's if 'ALL' in scoped: makes the extra IDs silently meaningless — and mixing suggests the user misunderstood the full-replacement semantics. Reject ALL combined with anything else.

Minor

  • CloudShell gap: docs say "run in CloudShell", but CloudShell won't have the repo cloned. One line on how to get the script there would help.
  • CI coverage: lint_shell_injection.py only guards generated scripts. A shellcheck scripts/map-tagger.sh step in lint.yml would be cheap, and the quoting is the load-bearing part of this script.
  • Naming: map-tagger.sh names the whole project, not the action — update-scope.sh / set-account-scope.sh would age better if more day-2 helpers arrive.
  • Testing checklist: several checked items (CloudWatch logs reviewed, IAM least-privilege) don't apply to this change — can you confirm the script was actually run against a real StackSet at least once? The quoting equivalence deserves one live verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants