ci: run integration tests on CodeBuild-hosted runners - #654
ci: run integration tests on CodeBuild-hosted runners#654ThirdEyeSqueegee wants to merge 1 commit into
Conversation
|
Claude finished @ThirdEyeSqueegee's task in 44m 54s —— View job Code Review: CodeBuild-hosted integration test runnersReviewed I left 8 inline comments with specifics and suggested diffs. Summary below. Worth resolving before merge1. 2.
3. 4. 5. Smaller items
Checked and clearRuling these out so they don't get re-litigated: the Pre-existing, out of scopeNot caused by this PR ( Note: I can't push changes to |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #654 +/- ##
=======================================
Coverage 61.62% 61.62%
=======================================
Files 11 11
Lines 740 740
=======================================
Hits 456 456
Misses 265 265
Partials 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| - name: Harden Runner | ||
| # Self-hosted runners need a StepSecurity agent, which CodeBuild runners do not have. | ||
| if: runner.environment == 'github-hosted' |
There was a problem hiding this comment.
This guard can never be true, so Harden Runner is now dead code rather than conditionally skipped.
runs-on (line 55) is unconditionally a CodeBuild label, and runner.environment evaluates to self-hosted for every self-hosted runner — CodeBuild-hosted ones included. The condition is therefore false on 4/4 matrix legs, permanently, with no path that turns it back on.
The comment's reasoning is right (harden-runner does need a StepSecurity agent), but the net effect isn't "skip on self-hosted" — it's egress auditing removed from the integration job entirely, which is easy to miss when the diff reads as a one-line if:. That's the largest loss here because this job:
- is the only one in the repo that holds AWS credentials and creates/destroys real EKS clusters, secrets and SSM parameters;
- runs several mutable third-party tags (
azure/setup-kubectl@v5,azure/setup-helm@v5,bats-core/bats-action@4.0.0); - is reachable from
build-test-image.yml, which triggers onpull_request_targetand checks out PR-authored code withallow-unsafe-pr-checkout: true.
Every other workflow in the repo still hardens unconditionally (build-test-image.yml:38, go.yml:21, pr_sync.yml:19, release-chart.yml:46), so this job is now the sole exception.
Two options that are both clearer than a dead if::
- Delete the step outright, so the diff shows the control being removed and reviewers can weigh it.
- Keep it, but gate on something that can actually vary, and pair it with a CodeBuild-side control (VPC egress rules or the project's network configuration) so the capability isn't just dropped.
If you go with (1), a note in the PR description about what replaces egress monitoring on the CodeBuild side would help.
| image: arm-3.0 | ||
| # CodeBuild-hosted runner. AWS credentials come from the runner's own IAM role, so the workflow | ||
| # does not assume a role. `image` picks the compute: linux-5.0 is x86_64, arm-3.0 is Graviton. | ||
| runs-on: codebuild-ascp-runner-${{ github.run_id }}-${{ github.run_attempt }}-${{ matrix.image }}-large |
There was a problem hiding this comment.
Moving to a self-hosted label without a timeout-minutes opens two new failure modes, one of which leaks EKS clusters.
The label form itself is correct — codebuild-<project>-<run_id>-<run_attempt>-<image>-<size> is the documented single-label override, and linux-5.0, arm-3.0 and large are all valid identifiers. The concern is what happens around it.
1. The job can queue for up to 24h and wedge every later integration run. ubuntu-latest was always available; a self-hosted label is not. If the project isn't named exactly ascp-runner, or its webhook isn't subscribed to WORKFLOW_JOB_QUEUED, or the org/repo scoping is off, AWS's docs note that CodeBuild won't process the webhook and the workflow may hang. No runner registers, and GitHub holds the job Queued up to its 24-hour self-hosted limit. Because concurrency (lines 24–28) uses group: integration-tests with cancel-in-progress: false, the next push to main — including the release path via release-chart.yml — queues behind the stuck run. This is also the fork/workflow_dispatch story: a contributor triggering this from a fork gets a job that hangs with no signal as to why.
2. CodeBuild's build timeout can kill the runner mid-job, and then if: always() doesn't run. The effective ceiling is now min(GitHub's 360-minute default, the project's build timeout), and the CodeBuild project default is 60 minutes. When CodeBuild terminates the container, the runner process dies with it, so GitHub never gets to execute the if: always() cleanup at line 131 — integ-cluster-<arch>-<auth_type>, its VPC, NAT gateway and ENIs all survive. That's exactly the network-interface quota exhaustion the concurrency comment on line 25 exists to prevent, and it compounds: the next run hits AlreadyExistsException on eksctl create cluster. Under GitHub-hosted runners the cleanup step always got a chance to run.
The role-duration-seconds: 14400 this PR deletes is evidence the maintainers budgeted up to 4 hours for these runs, so a 60-minute project default would be a real regression. Worth confirming the project's timeoutInMinutes (and queuedTimeoutInMinutes/concurrentBuildLimit while you're there), then setting a job timeout comfortably below it so GitHub, not CodeBuild, is the one that stops the job and cleanup still fires:
| runs-on: codebuild-ascp-runner-${{ github.run_id }}-${{ github.run_attempt }}-${{ matrix.image }}-large | |
| runs-on: codebuild-ascp-runner-${{ github.run_id }}-${{ github.run_attempt }}-${{ matrix.image }}-large | |
| timeout-minutes: 90 |
Two smaller things on this line:
- The label doesn't include
auth_type, so theirsaandpod-identitylegs for a given image render byte-identicalruns-onstrings. That's supported, but it makes CodeBuild build ↔ GitHub job correlation ambiguous when you're debugging, and rules out per-leg cost attribution. The positional label format leaves nowhere to add a discriminator. -largeis hardcoded for a job that spends most of its wall-clock blocked on EKS control-plane and nodegroup provisioning. Now that compute is billed per-minute across 4 legs on every run,-small/-mediumis worth measuring against — this is not a CPU-bound workload.
|
|
||
| - name: Clean up stale clusters | ||
| run: | | ||
| CLUSTER_NAME=integ-cluster-${{ matrix.arch-short }}-${{ matrix.auth_type }} |
There was a problem hiding this comment.
Deleting Configure AWS credentials removed the only fail-fast credential check, and this step swallows the error that replaces it.
aws-actions/configure-aws-credentials used to fail the job immediately and loudly if the role couldn't be assumed. With it gone, the first AWS call in the job is eksctl get cluster ... 2>/dev/null on the next line — and 2>/dev/null plus if means any non-zero exit is interpreted as "no stale cluster":
- Credential/permission problems surface ~15 minutes late. If the CodeBuild service role lacks an EKS permission, or the call is throttled, stderr is discarded, the
iftakes the false branch, and the job carries on as though everything is fine. The failure then appears inside batssetup_fileas an opaque error well into the run. - A genuinely leaked cluster gets masked. If the describe call fails for any reason other than not-found, the guard this step exists to provide silently doesn't fire, and
eksctl create clusterdies withAlreadyExistsException— the exact case this step was added to prevent.
Worth distinguishing "not found" from "call failed" rather than collapsing both to absent, e.g. capture the output and check the exit status / error text explicitly, and drop the blanket 2>/dev/null. A cheap aws sts get-caller-identity early in the job would also restore fail-fast and confirm the runner's identity is what you expect (see the note below about POD_IDENTITY_ROLE_ARN).
Two related consequences of removing that step:
- CloudTrail attribution is gone.
role-session-name: csi-driver-ci-${{ github.run_id }}-${{ matrix.arch }}tagged everyCreateCluster/DeleteSecret/DeleteClusterwith a run id and arch. All four legs — and anything else sharing theascp-runnerproject — now appear as the same service-role identity. Given that the legs create and force-delete similarly-named resources in one account, that's a meaningful loss for incident triage. - The account identity changes. Credentials now come from the CodeBuild project's service role, so the caller account is whatever account hosts
ascp-runner.secrets.POD_IDENTITY_ROLE_ARN(line 123) is a hardcoded ARN, andgenerate-test-files.py:244passes it toeksctl create podidentityassociation. If the CodeBuild project doesn't live in the same account as that role, bothpod-identitylegs fail while bothirsalegs pass — a 2-of-4 split that reads like a flaky test. Confirming this is a same-account move (or noting it in the PR description) would save someone that debugging session.
|
|
||
| env: | ||
| imageTag: ${{ matrix.arch == 'amd64' && inputs.imageTagAmd64 || inputs.imageTagArm64 }} | ||
| AWS_REGION: us-west-2 |
There was a problem hiding this comment.
This replaces an action that set both region variables with one that sets only AWS_REGION.
aws-actions/configure-aws-credentials exported AWS_REGION and AWS_DEFAULT_REGION consistently. CodeBuild also injects both, pointing at the project's own region. Overriding only AWS_REGION means that if ascp-runner isn't in us-west-2, the two disagree for the rest of the job.
Nothing breaks today — I checked: eksctl (aws-sdk-go-v2) and AWS CLI v2 both prefer AWS_REGION, every boto3.client(...) in tests/generate-test-files.py passes region_name= explicitly, and every aws CLI call in the test scripts passes --region. So this is latent, not a live bug. But botocore honours only AWS_DEFAULT_REGION, so the first client added without an explicit region_name would quietly create or delete resources in the project's region instead of us-west-2 — and cross-region resource deletion is not a fun failure to diagnose. Cheap to close now:
| AWS_REGION: us-west-2 | |
| AWS_REGION: us-west-2 | |
| AWS_DEFAULT_REGION: us-west-2 |
While this block is being touched: us-west-2 is now hardcoded five times in this file (here, lines 115, 117, 128, 135) and us-east-2 twice (129, 136), and tests/integration.bats.template:10 re-exports REGION=us-west-2 unconditionally on top of all of it. Hoisting both regions into this job-level env: (or into workflow_call inputs) and letting the steps inherit would cut that to one place each.
| fail-fast: false | ||
| matrix: | ||
| runner: [ubuntu-latest, ubuntu-24.04-arm] | ||
| image: [linux-5.0, arm-3.0] |
There was a problem hiding this comment.
This line changes the base OS from Ubuntu to Amazon Linux, and two tools the tests depend on come free on ubuntu-latest but aren't guaranteed on the CodeBuild curated images.
The workflow is careful to install kubectl, Helm, eksctl, Python and bats explicitly — but the test scripts also reach for:
envsubst(ships ingettext, not a base package) —tests/integration.bats.template:173runsenvsubst < BasicTestMountSPC-<arch>-<auth>.yaml | kubectl apply -f -jq—tests/integration.bats.template:153andtests/helpers.bash:73
Neither is installed here. If envsubst is absent, the SecretProviderClass is applied as an unexpanded template or not at all, roughly 20 minutes into the run, on all 4 legs — and it presents as a test bug rather than a missing host dependency. AWS calls this out directly: "If a dependency provided by GitHub-hosted runners is unavailable in the CodeBuild environment, you can install the dependency using GitHub Actions."
Worth adding an explicit step (and it's harmless if the tools are already present):
- name: Install test dependencies
run: sudo dnf install -y gettext jqRelated, on line 95: actions/setup-python@v6 resolves prebuilt CPython from the actions/python-versions manifest, which is built on Ubuntu images; linux-arm64 coverage there is thinner than linux-x64. Since tests/run-tests.sh calls python3 generate-test-files.py on both the test path (:21) and the clean path (:44), a setup-python failure takes out cleanup as well as the run — leaving a live EKS cluster behind. Pinning an explicit minor version rather than "3", and echoing python3 --version / which pip once on each image, would confirm this works on both legs before you rely on it. (Line 101 uses pip rather than pip3, which works only because setup-python puts it on PATH.)
| SUDO="" | ||
| [ "$(id -u)" -ne 0 ] && SUDO=sudo | ||
|
|
||
| $SUDO install -m 0755 /tmp/eksctl /usr/local/bin && rm /tmp/eksctl |
There was a problem hiding this comment.
The real bug on this line is the bare destination, not the sudo probe.
install SRC DEST only installs into DEST when DEST already exists as a directory; otherwise DEST is the target filename. On an image where /usr/local/bin doesn't exist, install writes the eksctl ELF to the path /usr/local/bin, exits 0, rm /tmp/eksctl succeeds, and this step goes green. The failure then shows up one step later at line 93 as a bare eksctl: command not found — and /usr/local/bin is now a regular file, so nothing else can be installed there for the remainder of the job. -D makes it explicit and creates the directory if needed:
| SUDO="" | |
| [ "$(id -u)" -ne 0 ] && SUDO=sudo | |
| $SUDO install -m 0755 /tmp/eksctl /usr/local/bin && rm /tmp/eksctl | |
| SUDO="" | |
| if [ "$(id -u)" -ne 0 ]; then SUDO=sudo; fi | |
| $SUDO install -D -m 0755 /tmp/eksctl /usr/local/bin/eksctl && rm /tmp/eksctl |
To be explicit about what is not a problem here, since it looks like one: [ "$(id -u)" -ne 0 ] && SUDO=sudo does not abort the step under the default bash -e {0} shell. Bash's errexit exemption covers a failing command that isn't the last in an && list, and this list isn't the script's final command either. So no bug — but the step's exit status does depend on a later line existing, which is why the if form above is worth taking: it survives someone reordering or copy-pasting the idiom to the end of a script.
Two other notes:
- The non-root branch assumes
sudois installed and the runner user is a sudoer. If neither privilege path is reliable, sidestepping it entirely is more robust than probing —install -D -m 0755 /tmp/eksctl "$HOME/.local/bin/eksctl"plusecho "$HOME/.local/bin" >> "$GITHUB_PATH", which takes effect from the next step onward (line 93). - Line 84 fetches
eksctl_checksums.txtfrom the samereleases/latest/download/pointer as the tarball on line 82, so the checksum verifies the download against a file from the same mutable source — it detects a truncated transfer but pins nothing and provides no supply-chain guarantee. Pinning an eksctl version (and reading the checksum from that tag) would make it meaningful. Bothcurlcalls also lack--fail/--retry, and with pipefail off,curl ... | grep | sha256sum --checkreports onlysha256sum's status — a 404 body yields an empty grep and a confusing downstream error rather than a clean failure.
| include: | ||
| - os: linux | ||
| - arch: amd64 | ||
| arch-short: x64 | ||
| runner: ubuntu-latest | ||
| image: linux-5.0 | ||
| - arch: arm64 | ||
| arch-short: arm | ||
| runner: ubuntu-24.04-arm | ||
| runs-on: ${{ matrix.runner }} | ||
| image: arm-3.0 |
There was a problem hiding this comment.
Minor: - os: linux is dead, and the rename leaves three parallel encodings of one binary choice.
The runner → image rename preserves matrix semantics correctly. Two cleanups while this block is open:
- os: linux is an include entry with no key overlapping any matrix dimension, so it's merged into all four combinations — but matrix.os is referenced nowhere in this file (only build-test-image.yml:59). It can go:
| include: | |
| - os: linux | |
| - arch: amd64 | |
| arch-short: x64 | |
| runner: ubuntu-latest | |
| image: linux-5.0 | |
| - arch: arm64 | |
| arch-short: arm | |
| runner: ubuntu-24.04-arm | |
| runs-on: ${{ matrix.runner }} | |
| image: arm-3.0 | |
| include: | |
| - image: linux-5.0 | |
| arch: amd64 | |
| arch-short: x64 | |
| - image: arm-3.0 | |
| arch: arm64 | |
| arch-short: arm |
Beyond that, image (linux-5.0/arm-3.0), arch (amd64/arm64) and arch-short (x64/arm) are three spellings of the same x86-vs-ARM decision, wired together through this lookup table. The footgun is silent: add a value to line 43 without a matching include and you get legs with no arch/arch-short, so ./run-tests.sh -irsa runs against a target name that matches nothing. Dropping the image: list and driving the matrix from a single list of objects removes the mismatch class entirely:
matrix:
auth_type: [irsa, pod-identity]
target:
- { image: linux-5.0, arch: amd64, arch-short: x64 }
- { image: arm-3.0, arch: arm64, arch-short: arm }(That does mean matrix.target.arch at the reference sites, so it's a judgement call on churn — fine to leave as-is if you'd rather keep the diff tight.)
| @@ -83,7 +83,6 @@ jobs: | |||
| imageTagAmd64: latest-amd64-${{ needs.build-docker.outputs.privTagSuffix }} | |||
| imageTagArm64: latest-arm64-${{ needs.build-docker.outputs.privTagSuffix }} | |||
| permissions: | |||
There was a problem hiding this comment.
Right change, but the OIDC teardown is only half done.
Dropping id-token: write here is correct — the sole consumer, aws-actions/configure-aws-credentials, is gone from integ.yml, and a called workflow can't use a permission it doesn't declare anyway. Three loose ends elsewhere, though:
build-test-image.yml:10still grantsid-token: writeat the workflow level. Top-levelpermissionsare the default for any job that omits its own block, so this remains a standing over-grant on a workflow triggered bypull_request_targetwithsecrets: inherit— and it reads as if OIDC is still in play, contradicting the two places this PR removes it.build-dockerauthenticates to ghcr.io withsecrets.GITHUB_TOKEN, so nothing here needs it.release-chart.yml:36is the exact same call site and still grants it, plus line 8 at the workflow level. That file also callsinteg.yml, so leaving it untouched means the two callers now disagree about what the reusable workflow needs.secrets.ROLE_ARNno longer has a consumer anywhere in the repo — I grepped, and the only remaining*ROLE_ARNreferences arePOD_IDENTITY_ROLE_ARNand unrelated docs. Worth deleting the repo/org secret so it isn't mistaken for live configuration later.
More broadly: the IAM identity this workflow now depends on lives entirely outside the repo. Nothing in .github/, tests/README.md or CONTRIBUTING.md describes the ascp-runner CodeBuild project, its service role, or the permissions that role needs (EKS cluster create/delete, IAM OIDC provider, iamserviceaccount, podidentityassociation, iam:PassRole on the pod-identity role, plus Secrets Manager and SSM writes in two regions). With the OIDC role gone, that's the only thing standing between a maintainer and a working integration run — a short section in tests/README.md (or IaC) would keep it discoverable.
Runs the integration tests on CodeBuild-hosted GitHub Actions runners instead of GitHub-hosted runners.
On a CodeBuild-hosted runner, AWS credentials come from the IAM role attached to the runner itself, so the workflow no
longer assumes a role through the GitHub OIDC provider and no longer needs
id-token: write. Runners stay ephemeral —one job per runner — and arm64 jobs run on Graviton rather than emulated hardware.
Changes:
runs-onuses the CodeBuild label form. The matrix dimensionrunnerbecomesimage:linux-5.0for x86_64 andarm-3.0for Graviton.Configure AWS credentialsstep andid-token: write, here and in theinteg.ymlcall frombuild-test-image.yml.AWS_REGIONis now set on the job.harden-runnerruns only on GitHub-hosted runners; on self-hosted runners it needs a StepSecurity agent thatCodeBuild runners do not have.
sudois available, since the CodeBuild image runs as root.Merge this only once the
ascp-runnerCodeBuild project exists in the test account, otherwise the job waits for arunner that never registers. Contributors running these tests from their own fork will not get a runner, since the
project is scoped to this repository — that path needs a CodeBuild project of their own. The
safe-to-testlabel gateon
build-test-image.ymlis unchanged.Reference: https://docs.aws.amazon.com/codebuild/latest/userguide/action-runner.html