diff --git a/.github/workflows/pr-checks-master.yml b/.github/workflows/pr-checks-master.yml index 24d0c8349b..1b2d319d05 100644 --- a/.github/workflows/pr-checks-master.yml +++ b/.github/workflows/pr-checks-master.yml @@ -272,12 +272,34 @@ jobs: fast-integ-tests: runs-on: ubuntu-latest needs: [detect-changes] - if: contains(fromJson(needs.detect-changes.outputs.submodules), 'sagemaker-train') + # Same-repo PRs only, because this is the one job here that puts PR code on a + # runner. actions/checkout refuses to place a fork's head commit in a + # pull_request_target job -- correctly: this job holds the base repo's + # GITHUB_TOKEN and assumes CI_AWS_ROLE_ARN, so a fork could edit conftest.py + # and read those credentials out. That is the "pwn request" shape, and on a + # public repo it is a live credential-exfiltration path, so the guard skips + # the job rather than overriding the refusal with allow-unsafe-pr-checkout. + # + # Known consequence: nearly every PR to this repo comes from a fork, so this + # is not yet a gate on the PRs that matter. Closing that gap means running + # the suite where the other three test jobs already run PR code -- inside + # CodeBuild via source-version-override, which never exposes the runner's + # token or secrets to it (see integ-tests above). That needs a + # -ci-sagemaker-train-fast-integ-tests project in the CDK that owns the + # existing ones. Until it exists this suite is effectively a local signal -- + # whether the daily CI-health run over refs/heads/master reaches + # tests/integ/train/shallow depends on that buildspec's selection, which also + # lives outside this repo. + if: >- + contains(fromJson(needs.detect-changes.outputs.submodules), 'sagemaker-train') + && github.event.pull_request.head.repo.full_name == github.repository steps: - uses: actions/checkout@v3 with: # pull_request_target checks out the base ref by default; these tests - # must run against the PR's code. + # must run against the PR's code. Safe only because the job is guarded + # to same-repo PRs above -- a branch in this repo already requires push + # access, so checking it out grants nothing its author lacks. ref: 'refs/pull/${{ github.event.pull_request.number }}/head' - name: Set up Python diff --git a/sagemaker-train/tests/integ/train/shallow/README.md b/sagemaker-train/tests/integ/train/shallow/README.md index 02de12214d..a1d8dd6a28 100644 --- a/sagemaker-train/tests/integ/train/shallow/README.md +++ b/sagemaker-train/tests/integ/train/shallow/README.md @@ -8,6 +8,21 @@ What this suite changes about the gate is not which job runs, but what the exist one selects: the `gpu_intensive` marks added here deselect the deep tests that submit a job and wait for it, and this suite covers those code paths instead. +> **Current limitation — the job only runs on same-repo PRs.** `fast-integ-tests` +> is the one job in `pr-checks-master.yml` that puts PR code on a GitHub runner, +> and that runner assumes `CI_AWS_ROLE_ARN`. `actions/checkout` refuses to place a +> fork's head commit in a `pull_request_target` job for exactly that reason, so the +> job is guarded to same-repo PRs. Almost every PR here comes from a fork, so in +> practice this is presently a **local** signal rather than a gate. The fix is to +> run it the way the other three test jobs run PR code — inside CodeBuild via +> `source-version-override`, which never exposes the runner's token or secrets to +> it — which needs a CodeBuild project provisioned alongside the existing ones. +> +> Until then, run this suite locally when changing a submission path, and note that +> the deselection described above trades gate coverage for coverage that depends on +> whether the daily CI-health buildspec reaches this directory — that selection +> lives outside this repo and is worth confirming. + ## What a passing test proves Each test submits a real `CreateTrainingJob`, asserts the service returned a