ci(test-infra): deploy StrandsTestInfraStack from GitHub Actions - #3713
ci(test-infra): deploy StrandsTestInfraStack from GitHub Actions#3713strandly-the-agent wants to merge 10 commits into
Conversation
…needs Prepares test-infra/ for automated deployment (workflow added in the following commit): - GitHubDeployRole: the identity CI assumes, defined in the stack so its trust and permissions are reviewable. GitHub OIDC pinned to refs/heads/main of this repo; permission to assume nothing but the CDK bootstrap roles. - Bump the aws-cdk CLI 2.1125.0 -> 2.1135.1. The pinned CLI cannot read the cloud assembly aws-cdk-lib 2.260.0 emits (schema 54.0.0 vs max 53.x.x), so `cdk synth`/`deploy` fails before this bump. - Trim and drop blank entries in the internal env lists. GitHub passes an unset secret to a step as an empty string, which turned STRANDS_TEST_INFRA_RUNNER_ROLES into a trust principal with an empty role name.
Add .github/workflows/test-infra-deploy.yml. The stack owns the IAM role integration tests assume, but nothing deployed it: a permission change merged to main had no effect until someone ran `cdk deploy` by hand. strands-agents#3691 added `bedrock-mantle:ListModels` and the integ test still failed, because the live role never got it. Deploys on push to main under test-infra/, plus workflow_dispatch from main. Pull requests only type-check and unit-test — IAM changes should not reach the account before review lands. Fails before touching AWS when a secret is missing or the ref is not main, because an empty list secret does not fail a deploy, it deploys the role with those entries removed.
|
@mkmeral this is the Two things need someone with account access before the workflow can do anything (both in the PR body and Worth your eyes specifically: the OIDC trust conditions on that role, and whether |
|
@strandly-the-agent can we also run this in PRs with manual approval? we have examples in other CIs. I'd like to be able to see the diff in roles and stuff before approving though ideally (I can do through pr, but for more programmatic changes I'd like to see what it resolved to too) |
Trust policies move off the OIDC ref subject onto environment subjects: pull_request_target reports the default branch in GITHUB_REF, so a pull-request job with no environment mints the same subject a push-to-main job does. The ref cannot distinguish reviewed code from a PR's; the environment can. job_workflow_ref still pins branch and workflow file. The diff job runs a PR's own CDK code, so it gets its own read-only role (StrandsTestInfraDiffRole, bootstrap lookup role only) and environments disjoint from the deploy job's — otherwise a diff-job token would satisfy the deploy role's trust policy. The workflow half of this change lands in the following commit.
A pull request now gets the resolved `cdk diff` posted to it, and can be deployed before merge by approving the test-infra-deploy-approval environment. The trigger is pull_request_target, so every job runs main's copy of this file: a PR cannot edit the steps that handle it, and job_workflow_ref stays pinned to main. The diff runs the PR's own CDK code, so it holds only the read-only diff role, and posts through a separate job that has no checkout — the pull-requests: write token never coexists with PR code. Secret-derived names are masked in the log and redacted by an inlined script before the diff reaches the job summary, the artifact or the comment: this is a public repository and all three are world-readable.
… group The workflow-level group is keyed per pull request, so a pull-request deploy and a main deploy would land in different groups and could run at once against the one stack. The workflow half of the fix lands next.
The workflow-level group is keyed per pull request, which keeps diffs parallel but put a pull-request deploy and a main deploy in different groups — two `cdk deploy` runs against the one stack, which is what the workflow-level comment says CloudFormation rejects. The deploy job now has a constant group of its own.
Round-1 review finding: the deploy job holds the same STRANDS_TEST_INFRA_* lists as the diff job but had none of the masking or redaction, so every unattended post-merge run published the resolved IAM diff to a job summary on a public repository. The mask/redact assertions now cover both jobs, and the README claim that the unredacted diff only appears behind an approval was false — corrected. The workflow half lands next.
Round-1 review finding, and the worse half of the leak: the deploy job holds the same STRANDS_TEST_INFRA_* lists as the diff job but had none of the masking or redaction, so every unattended post-merge run wrote the resolved IAM diff — private repos, bucket and secret names, the account id — into a job summary on a public repository. Job summaries are files, so ::add-mask:: never covered them, and nobody approves a push to main. The deploy job now masks and redacts exactly as the diff job does, and `cdk deploy` is piped through the redactor as it streams, stderr included: CloudFormation events name the resources they touch. The redactor becomes a line filter so one implementation serves a finished file and a live stream, and it now also neutralizes line-leading `::`. That text is a rendering of the pull request's own CDK code, so a resource named `::stop-commands::…` would otherwise have been executed as a workflow command when printed.
|
Note Posted by a review subagent on its own initiative — my fault, I didn't scope its tools to exclude GitHub writes. Kept rather than deleted because the content is accurate and independent. Both items it raised are now closed: the redactor copies are byte-identical with a test asserting it, and the CDK line-wrap caveat is recorded as a comment in the redactor — both in Round 2 review — APPROVE (posted as a comment, not a formal review: GitHub won't let an author self-approve). Verified all round-1 fixes independently at ✅ Checked: Fix 1 (masking/redaction in
|
Round-2 review finding: the redactor is inlined once per credentialed job (a composite action would be the pull request's own code on pull_request_target), and nothing noticed when the reviewer mutated only the deploy copy's sort order, losing the longest-match-first property that keeps a short entry from clobbering a longer one. Now asserted equal. The workflow half lands next.
Round-2 review finding. The redactor is inlined once per credentialed job because a composite action read from the tree would be the pull request's own action on pull_request_target, so drift between the copies is the standing risk — the reviewer mutated only the deploy copy's sort order, losing the longest-match-first property, and nothing failed. The copies are now identical and a test asserts it. Also records why a line-at-a-time filter is safe here: CDK's table renderer wraps to `stream.columns`, which is undefined for non-TTY output, so a redactable name is never split over two lines. Worth rechecking on a CDK CLI bump.
|
@mkmeral both are in, at
Secret-derived names are redacted to Needs you, and nothing works until then: create Before you approve a PR deploy: it runs that pull request's TypeScript with credentials that can change the account — The trust policy changed shape — worth your eyes, since roles are what you wanted to reviewIt used to pin the OIDC subject So both roles now pin Two consequences to know: the diff role can assume only the bootstrap Two caveats on the first run
Full detail, the gate output and the four-round review ledger are in the PR body; round 4's independent write-up is above. |
Description
test-infra/provisions the IAM role the integration tests assume, but nothing deployed it — the stack only moved when someone remembered to runnpx cdk deploy. That is how #3691 broke CI: the new integ test'sbedrock-mantle:ListModelsgrant merged, the live role never got it, and the failure looked like a missing IAM permission rather than a missing deployment.This adds a workflow that deploys the stack, the two identities it uses — defined in the stack, so their trust and permissions are reviewable rather than hand-made in the console — and, at
mkmeral's request, a pull-request path: the resolvedcdk diffis posted to the PR, and the PR's own version of the stack can be deployed before merge behind a manual approval.mainundertest-infra/**, or Run workflow frommaincdk diff→cdk deployStrandsTestInfraDeployRolein thetest-infra-deployenvironmentcdk diffposted to the PRStrandsTestInfraDiffRole(read-only) inauto-approve/manual-approvaldeployjobcdk diff→cdk deployof the PR's codeStrandsTestInfraDeployRolein thetest-infra-deploy-approvalenvironmentFiles
.github/workflows/test-infra-deploy.ymlpull_request_target, notpull_request: a fork PR gets no secrets onpull_request, so the synthesized template would not match the live one — andpull_request_targetruns main's copy of the workflow, so a PR cannot edit the steps that handle it.test-infra/lib/constructs/github-oidc.tstest-infra/lib/constructs/github-deploy-role.tsStrandsTestInfraDeployRole— may assume the four CDK bootstrap roles, which is allcdk deployneeds. Powerful in effect (the bootstrap deploy role can pass an AdministratorAccess execution role), so the trust policy is what contains it.test-infra/lib/constructs/github-diff-role.tsStrandsTestInfraDiffRole— may assume the bootstrap lookup role only (ReadOnlyAccess). This is the role a pull request's own code holds.test-infra/lib/constructs/integ-test-role.ts"", which previously produced a trust principal with an empty role name.test-infra/package.json+ lockfileaws-cdkCLI 2.1125.0 → 2.1135.1 — a prerequisite, not a drive-by. With the pinned CLI anycdk synthof this stack fails today:Cloud assembly schema version mismatch: Maximum schema version supported is 53.x.x, but found 54.0.0. Onlyaws-cdkmoves; the lockfile diff is 8 lines.test-infra/test/*test-infra/README.md,test-infra/AGENTS.md,AGENTS.mdThe trust model, and why it changed
The deploy role used to pin the OIDC subject
repo:strands-agents/harness-sdk:ref:refs/heads/main. That cannot survive a pull-request path: GitHub setsGITHUB_REFto the default branch forpull_request_target, so a PR job with noenvironment:mints exactly the push-to-main subject. The ref cannot tell reviewed code from a pull request's — so both roles now pin, asStringEquals:job_workflow_ref= this workflow file@refs/heads/main— no other workflow can assume either role, and aworkflow_dispatchfrom another branch is refused;sub= a list ofrepo:strands-agents/harness-sdk:environment:<name>, and the two roles' environment sets are disjoint. GitHub mints that subject only for a job declaring that environment, so a protected environment's required reviewers are enforced by IAM as well as by GitHub — and a token minted in the diff job (which runs unreviewed PR code) does not satisfy the deploy role's trust policy. A unit test asserts the disjointness.Before this can work (needs a maintainer with account access)
test-infra-deploywith no protection rules (a merge must deploy unattended) andtest-infra-deploy-approvalwith required reviewers. Neither may be reused by another workflow — their names are what the deploy role trusts.auto-approve/manual-approvalalready exist for the rest of the repo and are reused as-is.test-infra/README.md. Each must match what the live role currently has — an empty list does not fail the deploy, it deploys the role with those entries removed, so the workflow refuses to start if any is unset. The diff role needs no secret of its own: its ARN is derived from the account secret plus its fixed name.cdk deploy(the stack defines the roles that deploy it) and setSTRANDS_TEST_INFRA_DEPLOY_ROLEto the created ARN. Commands are intest-infra/README.md→ Automated deployment → One-time setup.Worth knowing before approving a PR deploy
Approving
test-infra-deploy-approvalauthorizes that pull request's TypeScript to run with credentials that can change the account —cdk deployexecutesbin/test-infra.tsand everything it imports, from the PR, including a fork's. The approval is the only gate; read the code, not just the diff. The account then holds an unmerged stack untilmaindeploys again, which the job says in its summary.Deliberately not in this PR
test-infralane inci.yml.test-infra/has no enforced CI today, so the newvalidatejob is advisory only. Wiring it intodetect-changes+alls-greentouches the merge gate and belongs in its own PR./test-infrais missing from.github/dependabot.yml— which is how the CDK CLI pin drifted far enough to break synth. Also a follow-up.Related Issues
No tracking issue; requested directly by
mkmeral. Context for the outage: #3691.Documentation PR
Included —
test-infra/README.md,test-infra/AGENTS.md, rootAGENTS.md. Nosite/changes (internal infrastructure, not user-facing docs).Type of Change
Other: CI / test infrastructure.
Testing
Not verified live. My sandbox has no AWS credentials, so no real synth, diff, or deploy ran, and neither role's permission set has ever been exercised. The first real run needs watching. Note also that
pull_request_targetruns main's copy of the workflow, so this PR cannot exercise its own new workflow — that only takes effect once merged.Verbatim gate output
Those two actionlint errors are its schema being behind, not a bug:
environment.deploymentis used by this repo'spython-integration-test.ymlandtypescript-integration-test.yml, actionlint flags those files identically, and that workflow's latestpull_request_targetrun succeeded. Kept for consistency with them.Also verified, without AWS:
cdk diff --method=templateis required, not cosmetic.cdk diff --helpon the pinned CLI: the default method (auto) creates a change set, which uses the deploy role;template"compares templates directly and uses the lookup role". A read-only diff has to ask for it.--no-failparses and exits 0 with differences present (checked against a local template), so a diff does not false-fail the job.::stop-commands::line, a ``` fence, secret values, and a mid-line::that is correctly left alone — with no secret-derived value surviving into the redacted diff or the job summary; `cdk diff` failing (status propagates, fence still closed); `cdk deploy` failing through the pipe (exit 7 propagates); and the deploy preflight's ref check for `pull_request_target` / `workflow_dispatch` off main / `push`.--method=template; overlapping the diff and deploy environments; reverting to the bare-ref subject; leakingSTRANDS_TEST_INFRA_DEPLOY_ROLEinto the diff job; switching topull_request; dropping the redactor call in either job; dropping thecdk deploypipe; dropping only its2>&1; removing the::neutralization; drifting one redactor copy's sort order; keying the deploy job's concurrency per-PR; renaming the workflow file; droppingRUNNER_ROLESfrom the preflight; reverting the env-list trim.pull_request_targetreports the default branch inGITHUB_REF(the reason the trust moved to environment subjects) and that this repo predates the July 2026 immutable-subject change, so the legacysubformat is correct.Review-loop ledger — 4 rounds, independent fresh-context reviewers
Rounds 1–2 covered the original merge-to-main-only design: 8 findings, 7 fixed (
RUNNER_ROLESmissing from the preflight; no account pin; trust not pinned to the workflow; docs missing required env vars; test gaps; opaque non-maindispatch failure; stale test count), then APPROVE. One finding was deferred with reasoning — "cdk diffis advisory, and the trust pin forecloses anenvironment:approval gate" — and this round is that finding being implemented, so it is no longer disputed.Round 3 (independent reviewer, this round's design) — CHANGES REQUESTED, 2 findings, both fixed:
deployjob held the sameSTRANDS_TEST_INFRA_*lists but had none of the diff job's masking or redaction, so every unattended post-merge run wrote the resolved IAM diff — private repos, bucket and secret names, the account id — into a job summary on a public repo. Job summaries are files, so::add-mask::never covered them, and nobody approves a push tomain. My README also claimed the unredacted diff only appears behind an approval, which was false.e7e3c791) — the deploy job masks and redacts exactly as the diff job does,cdk deployis piped through the redactor as it streams with2>&1, README corrected, and the mask/redact test now runs over both jobscatunescaped, so a resource named::stop-commands::…would be executed as a workflow commande7e3c791) — the redactor neutralizes line-leading::; it also became a line filter so one implementation serves a finished file and a live streamRound 4 (new reviewer, given round-3's findings and fixes) — APPROVE, no blockers; full write-up posted in this thread. It re-derived both fixes from source rather than re-reading them (including the pipefail exit-code claim with a live repro, and the
diff-job-skips-on-push control flow from GitHub'ssuccess()/needssemantics), and found one gap round 3 missed:0153004d) — the copies are byte-identical and a test asserts itstream.columnsis undefined for non-TTY output) but an undocumented third-party dependency0153004d) — recorded as a comment in the redactor, flagged to recheck on a CDK bumpOne self-found fix, outside the review rounds: the deploy job now serializes on a constant concurrency group (
d54325c4). The workflow-level group is keyed per pull request, so a PR deploy and a main deploy would have landed in different groups and could have run at once against the one stack — the thing the workflow's own comment says CloudFormation rejects.Open / residual: nothing live has been exercised, and the environments plus secrets above must exist first. A fifth adversarial pass (attacking the OIDC trust model specifically) timed out twice without reporting, so that angle has had two reviewers' attention but not a dedicated one.
Checklist
hatch run prepare— N/A, no Python touched🤖 Opened by
strandly-the-agentatmkmeral's request. Four independent review passes ran across two design iterations, but a human should still verify the IAM and OIDC details and watch the first real run — the parts that cannot be checked from a sandbox.