Skip to content

fix(fast): match stage-links.sh impersonation patterns to the real stage service accounts - #300

Merged
aghassemlouei merged 3 commits into
google:mainfrom
digitalresistor:fix/stage-links-sa-patterns
Oct 20, 2026
Merged

aghassemlouei merged 3 commits into
google:mainfrom
digitalresistor:fix/stage-links-sa-patterns

Conversation

@digitalresistor

Copy link
Copy Markdown
Contributor

Description

This change makes the stale-impersonation warning in the fast/stage-links.sh script reliable. Before, it fired when the shell impersonated the correct networking stage service account.

  • The 2-networking stages expected an account ID ending in networking-0. The branch-networking.tf file in the fast/stages-aw/1-resman/ stage creates <prefix>-prod-resman-net-0, so the correct account always raised a warning.
  • The 3-security stage rejected its own read-only account, <prefix>-prod-resman-sec-0r, because that name does not contain security-0.
  • The old check matched substrings, so an account ID such as <prefix>-prod-bootstrap-0-old passed the bootstrap check.
  • The test_stage_links.py test module hid the networking defect because it used a made-up test-prod-networking-0 account.

The fix works as follows.

  • Each stage now declares a read-write and a read-only account ID suffix.
  • Every suffix matches the name of a service account in the automation.tf file in the fast/stages-aw/0-bootstrap/ stage, or in the branch-networking.tf and branch-security.tf files in the fast/stages-aw/1-resman/ stage.
  • The script anchors each suffix to the @ separator, so longer account IDs no longer match.
  • The script prints the read-only suffix below the expected pattern.
  • The stale-impersonation warning names both accepted suffixes.
  • The tests use the real account names and cover every stage under fast/stages-aw/.
  • For each stage, the tests check the read-write account, the read-only account and stale accounts from other stages, through both impersonation variables.
  • The impersonation troubleshooting table in the docs/ddg.md guide now lists <prefix>-prod-resman-net-0 and <prefix>-security-0 for the networking and security stages.

The 0-bootstrap-tenant and 1-resman-tenant cases keep their existing suffixes. The script gains only the matching read-only variants for them, which the old substring match already accepted. Neither stage exists under fast/stages-aw/, so this change does not verify their account names.

No tracking issue exists for this defect.

Rebase note

Open pull request #299 moves tests/tools/test_stage_links.py to the flat tests/ directory. If #299 merges first, this branch needs a rebase that applies the test changes to the moved file. The changes do not overlap the open pull request #286, which changes the tools/lint.sh script.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Deployment & Compliance Impact

  • Applicable Regimes:
    • US Region Restricted (e.g., Access Policy constraint)
    • FedRAMP Moderate
    • FedRAMP High
    • DoD IL4
    • DoD IL5
    • General / All
  • NIST 800-53r5 Controls: None. The change affects only a local helper script, its tests and the deployment guide, and leaves Terraform resources unchanged.

Checklist

Code Quality & Reusability

  • My code adheres to the Maximize Reusability principle. I have not redefined common elements and have reused existing base configurations and modules where possible.
  • I have checked that no existing module or configuration in modules/ or fast/stages-aw/ can be leveraged for this change.
  • My code follows the established naming conventions outlined in docs/naming-convention.md.

Documentation

  • I have updated the README.md of the modified module or blueprint.
  • I have added/updated documentation for inputs (variables) and outputs.

Security

  • My change adheres to GCP security best practices and the principle of least privilege.
  • I have ensured compliance with the targeted regime (FedRAMP Moderate, FedRAMP High, IL4, IL5, etc.).

Testing

  • I have tested my changes locally.
  • I have included details of my testing in this PR.

Testing Performed

  • pytest -q tests/tools/test_stage_links.py passes all 5 tests.
  • With the origin/main version of fast/stage-links.sh, the same test module reports 24 failures.
  • Those failures cover the real networking accounts, the security read-only account, the new read-only pattern line, and longer account IDs that the old substring match accepted.
  • pytest -q from the repository root passes all 68 tests.
  • bash -n fast/stage-links.sh and shellcheck fast/stage-links.sh report no findings.
  • pyflakes tests/tools/test_stage_links.py reports no findings.
  • yapf -d with the repository style reports no changes to the lines this change adds.
  • bash tests/scripts/test_shell_syntax.sh passes.
  • python3 tools/check_links.py --no-show-summary docs reports no errors in the docs/ddg.md guide. It still reports existing broken /docs/artifacts/ links in the docs/path-to-authorization.md guide, and origin/main gives the same result.
  • python3 tools/check_boilerplate.py fast tests reports only the existing missing header in the fast/stages-aw/1-resman/branch-tenants.tf file, and origin/main gives the same result. This change adds no new files.
  • A manual run in a 3-security directory with the networking account impersonated prints the read-only pattern line and a stale-impersonation warning that names both accepted suffixes.

The 2-networking stages expected an account ID ending in networking-0,
but 1-resman creates the networking stage service account as
<prefix>-prod-resman-net-0. The stale impersonation warning therefore
fired for the correct account. The 3-security read-only account
<prefix>-prod-resman-sec-0r also triggered a false warning.

Each stage now lists its read-write and read-only account ID suffixes,
taken from 0-bootstrap/automation.tf and the 1-resman branch files, and
the match is anchored to the @ separator. The tests use the real
account names and cover correct and stale impersonation per stage.
The troubleshooting table listed <prefix>-prod-networking-0 and
<prefix>-prod-security-0, which no stage creates. The networking and
security stages run as <prefix>-prod-resman-net-0 and
<prefix>-security-0.
The stale impersonation warning now names the read-write and read-only
account ID suffixes, and the match loop quotes both patterns and skips
an empty one. The stale-account tests also cover the
2-networking-b-il5-ngfw stage and the
CLOUDSDK_AUTH_IMPERSONATE_SERVICE_ACCOUNT variable, and the new test
code follows the repository yapf style.
@github-actions github-actions Bot added the Scope - Fast Related to Fabric FAST deployment stages in the fast directory label Oct 20, 2026

@aghassemlouei aghassemlouei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified that the stage service account suffixes in fast/stage-links.sh and docs/ddg.md match the accounts provisioned in 0-bootstrap/automation.tf, 1-resman/branch-networking.tf, and 1-resman/branch-security.tf, and that anchoring the suffix check to @ prevents substring false positives while accepting the read-only -0r variants.

@aghassemlouei
aghassemlouei merged commit d8634b3 into google:main Oct 20, 2026
13 checks passed
@digitalresistor
digitalresistor deleted the fix/stage-links-sa-patterns branch October 20, 2026 22:26
@aghassemlouei aghassemlouei added Bug Something isn't working Documentation Improvements or additions to documentation Priority - Medium Standard features and non-blocking bugs; important for the current milestone but not urgent Level of Effort - Low Quick, well-defined tasks with no unknowns; takes a few hours up to one day to complete labels Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working Documentation Improvements or additions to documentation Level of Effort - Low Quick, well-defined tasks with no unknowns; takes a few hours up to one day to complete Priority - Medium Standard features and non-blocking bugs; important for the current milestone but not urgent Scope - Fast Related to Fabric FAST deployment stages in the fast directory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants