test: run ITS pipeline e2e checks on pull requests - #3574
dheerajodha wants to merge 10 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughAdds a Tekton PipelineRun that runs for pull requests targeting ChangesEnterprise Contract pull request pipeline
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to No confirmed issue blocks this change. Confirm required-check behavior for unrelated pull requests before enabling enforcement. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Comment |
|
Risk Assessment: moderate (2/5) DetailsRe-review anchoring preserves prior moderate (2) score: Tier 1 signals are unchanged (single new .tekton/ file, 53 lines, small blast radius, returning contributor, no protected paths, no dependencies), Tier 2 defaults to 2 for a new file, and the blocking cross-repo dependency (conforma/e2e-tests#12) that the prior assessor flagged remains unmerged — no signal justifies lowering from the prior score. Previous runRisk Assessment: moderate (2/5) DetailsSingle-file Tekton CI pipeline addition (57 lines) by a returning contributor with small blast radius and no security concerns per Tier 1 signals; fork pins remain unresolved but debug tag was removed since prior assessment, keeping composite steady at moderate. Previous run (2)Risk Assessment: moderate (2/5) DetailsSmall config-only PR (59 lines, 2 files) adding a new CI trigger with a deliberately-broken bundle tag and fork-pinned refs; the debug-tag and fork pins are acknowledged pre-merge restorations, and stable git history plus returning contributor keep composite at moderate. Previous run (3)Risk Assessment: low (1/5) DetailsVery small PR (2 files, 63 lines added, blast=small) with no protected paths, security-sensitive files, dependency changes, or CI workflow modifications by the Tier 1 script's classification. Author is a known human contributor. Primary risk factors are draft status and an unmerged external dependency (e2e-tests#12), which introduce coordination coupling but do not affect the code risk of the change itself. Composite T1=1.1, T2=1.1, T3=2 yields 1.29, rounding to 1 (low), consistent with the PR's own risk/low label. Previous run (4)Risk Assessment: low (1/5) DetailsSingle small Tekton PipelineRun config file (54 lines) added under .tekton/. No protected paths, no security-sensitive content per the metadata script, no dependency changes, no GitHub Actions workflow changes. Author is a known human contributor. Tier 2 applies the all-files-new moderate baseline of 2. Weighted composite (Tier 1 62% x 1.125 + Tier 2 38% x 2) = 1.46, rounds to 1 (low). Note: this baseline score does not reflect the fork-code-execution risk raised as a high finding, which is qualitative and outside the Tier 1 signal set. |
ReviewSince the prior review (SHA FindingsHigh
Medium
Low
Next steps:
Previous runReviewSince the prior review (SHA FindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewSince the prior review (SHA FindingsCritical
High
Medium
Low
Next steps:
Previous run (3)ReviewThe CEL author_association gate added since the prior review ( FindingsCritical
High
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
/retest |
|
Caution There are some errors in your PipelineRun template.
|
|
🤖 Finished Review · ✅ Success · Started 1:46 PM UTC · Completed 1:57 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-7 · Effort: high · Cost: $4.89 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 2:02 PM UTC · Completed 2:12 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-7 · Effort: high · Cost: $4.40 |
fullsend-ai-review
Bot
added
risk/moderate
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 2:48 PM UTC · Completed 2:58 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-7 · Effort: high · Cost: $3.92 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 12:11 PM UTC · Completed 12:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-7 · Effort: high · Cost: $4.50 |
| pipelinesascode.tekton.dev/max-keep-runs: "3" | ||
| pipelinesascode.tekton.dev/on-cel-expression: | | ||
| event == "pull_request" && target_branch == "main" && | ||
| ("pipelines/enterprise-contract/**".pathChanged() || |
There was a problem hiding this comment.
[high] fail-open-authorization
The CEL trigger (lines 11-14) gates only on event == "pull_request", target_branch == "main", and pathChanged() over pipelines/enterprise-contract/** or .tekton/cli-its-pull-request.yaml itself. There is no author_association check and no ok-to-test label check — trust is delegated entirely to Pipelines-as-Code tenant ACLs. Amplifying facts verified against the file: (a) the CEL's pathChanged() clause at line 14 matches this file itself, so a fork PR editing it widens its own trigger surface; (b) pipelineRef (lines 43-51) resolves the pipeline body via the git resolver from a URL+revision pair currently pointing at a personal fork (line 47), meaning fork-controlled pipeline YAML would execute under serviceAccountName: konflux-integration-runner (line 53) with konflux-test-infra (line 30) and mapt-kind-secret (lines 32, 34) in scope. PaC's default fork-PR policy typically requires an owner/collaborator /ok-to-test — that mitigation is external and unverifiable from the diff.
Suggested fix: Either (a) add an explicit CEL guard for trusted actors (e.g., pipelines_as_code.author_association in ["OWNER","MEMBER","COLLABORATOR"] or a hasLabel("ok-to-test") gate) so the trust posture is expressed in-tree, or (b) document the deployed tenant PaC Repository CR trust policy as a hard prerequisite. Independently, pin pipelineRef.url/revision (lines 47, 49) to a canonical conforma/* repository+SHA so fork-authored pipeline bodies cannot execute with the mounted secrets.
| spec: | ||
| params: | ||
| - name: git-url | ||
| value: https://github.com/dheerajodha/conforma-e2e-tests.git |
There was a problem hiding this comment.
[high] unpinned-external-source
Both spec.params (git-url line 24, revision line 26) and pipelineRef (url line 47, revision line 49) reference the personal contributor fork https://github.com/dheerajodha/conforma-e2e-tests.git pinned to 4bbba993a3b44eec5debb89fbc4f8bd1c8d0503d. Per the PR body, upstream dependency conforma/e2e-tests#12 is unmerged. Risks: (a) supply chain — the personal fork can be deleted, renamed, force-pushed, or account-compromised, and its content is resolved into a PipelineRun that mounts konflux-test-infra (line 30) and mapt-kind-secret (lines 32, 34) under konflux-integration-runner (line 53); (b) SHA-orphan — if conforma/e2e-tests#12 is squash-merged (GitHub default), the merge-commit SHA will differ from 4bbba99..., which will then exist only in the personal fork; (c) governance — a shared .tekton/ PipelineRun on main should reference the canonical org repository.
Suggested fix: Block merge until conforma/e2e-tests#12 lands. Then repoint both git-url (line 24) and pipelineRef.url (line 47) to https://github.com/conforma/e2e-tests.git, and repin both revision fields (lines 26 and 49) to the upstream merge-commit SHA.
| spec: | ||
| params: | ||
| - name: git-url | ||
| value: https://github.com/dheerajodha/conforma-e2e-tests.git |
There was a problem hiding this comment.
[medium] merge-gate-absent
Process-risk observation layered on top of the unpinned-external-source finding: the PR has transitioned from draft to non-draft while the fork URL (lines 24, 47) and personal-fork SHA (lines 26, 49) remain in place, and inline reminders present in the prior revision have been removed. No repo-level automated check exists to reject .tekton/.yaml files referencing URLs outside conforma/. The only surviving warning is the PR body, which is author-controlled and dropped from git history on merge. See also: [unpinned-external-source] finding at this location.
Suggested fix: Before merging: (1) replace both dheerajodha/conforma-e2e-tests.git references with conforma/e2e-tests.git (lines 24, 47), and (2) update the pinned revisions (lines 26, 49) once conforma/e2e-tests#12 lands. Longer term, add a repo-level lint or required status check that fails when .tekton/.yaml references git URLs outside the conforma/ org.
| value: mapt-kind-secret | ||
| - name: deprovision-aws-credentials-secret | ||
| value: mapt-kind-secret | ||
| - name: its-pipeline-repo-url |
There was a problem hiding this comment.
[low] api-contract
The params sent to the resolved pipeline (its-pipeline-repo-url, its-pipeline-revision, its-pipeline-path, test-label-filter, lines 35-42) diverge from the naming used by the sibling .tekton/cli-e2e-push.yaml (custom-ec-cli-url, custom-ec-cli-revision). Correctness depends on the fork's .tekton/pipelines/conforma-e2e/pipeline.yaml at revision 4bbba993... declaring these exact param names; the referenced pipeline file lives in another repository and cannot be verified from this diff. If any name/type is mismatched, the PipelineRun will fail admission or the params will be silently ignored.
| build.appstudio.redhat.com/target_branch: '{{target_branch}}' | ||
| pipelinesascode.tekton.dev/cancel-in-progress: "true" | ||
| pipelinesascode.tekton.dev/max-keep-runs: "3" | ||
| pipelinesascode.tekton.dev/on-cel-expression: | |
There was a problem hiding this comment.
[low] trigger-coverage-gap
The CEL only fires when pipelines/enterprise-contract/** or this file itself changes (lines 13-14). Changes to Task sources, Makefile, or CLI code paths that materially shape the built verify bundle will not trigger this ITS run. Informational; not a regression since this is a new job.
| - name: deprovision-aws-credentials-secret | ||
| value: mapt-kind-secret | ||
| - name: its-pipeline-repo-url | ||
| value: '{{source_url}}' |
There was a problem hiding this comment.
[low] param-propagation
its-pipeline-repo-url is bound to {{source_url}} (line 36) and its-pipeline-revision to {{revision}} (line 38). For fork PRs the runner fetches the pipeline-under-test from the contributor's fork at PR head — correct for the stated intent, but runs will fail for any PR whose fork is private or unreachable by the Konflux runner service account.
| @@ -0,0 +1,53 @@ | |||
| apiVersion: tekton.dev/v1 | |||
There was a problem hiding this comment.
[low] missing-authorization
The PR references EC-1943 (external Jira) and depends on conforma/e2e-tests#12 (cross-repo). No linked issue in conforma/cli exists as an authorization record in this repo's public history. Not a code defect.
Suggested fix: Open a tracking issue in conforma/cli mirroring EC-1943 and link it from the PR.
st3penta
left a comment
st3penta
left a comment
There was a problem hiding this comment.
lgtm. remember to update the references to the upstream repo before merging!
This branch has not been deployed
2 participants
Changes to the enterprise-contract ITS pipeline need pre-merge E2E validation. Add a separate Pipelines-as-Code check for PRs targeting main that change
pipelines/enterprise-contract/**or this trigger. It runs only the ITS suite and reports the result on the CLI PR.The runner and test suite are pinned independently of the pipeline under test. The ITS definition is fetched from the PR source URL and exact commit SHA, including fork PRs. The definition retains its own task bundle references; this check does not build a custom CLI image.
Dependency and authorization
Depends on conforma/e2e-tests#12 (EC-1943). This PR can be reviewed now, but must not merge until that dependency lands and the source pins are updated. For pre-merge testing, both runner/test source URLs temporarily use
https://github.com/dheerajodha/conforma-e2e-tests.gitat4bbba993a3b44eec5debb89fbc4f8bd1c8d0503d. Before merge, restore both URLs tohttps://github.com/conforma/e2e-tests.gitand both pins to the resulting merged upstream commit.Matching uses normalized event, target branch, and changed paths. Execution authorization relies on Pipelines-as-Code ACLs and approval policy. Verify the tenant's deployed Repository/global authorization configuration before merge; it has not yet been inspected. A raw-webhook author-association filter was removed because event payload differences prevented execution. The outer runner is pinned to a fixed E2E commit; only the ITS definition under test uses the PR source URL and revision. Pipelines-as-Code documents ACL checks before execution, including
/ok-to-testapproval for unauthorized contributors: https://pipelinesascode.com/docs/guides/running-pipelines/#acl-permissions-for-triggering-pipelineruns. An author filter inside a PR-editable trigger is not a substitute for that external authorization boundary.Validation
cli-its-on-pull-request-t6hlvtested CLI commit257252cb; all three ITS scenarios passed.cli-its-on-pull-request-v6kmvtested CLI commitb6987722. Image building, signing, and attestation succeeded. The ITS PipelineRun then failed withCouldntGetTaskandMANIFEST_UNKNOWNfor the deliberately nonexistent bundle tagec-1943-deliberately-missing-round2. The success scenario failed and the E2E step exited 1; the other two ordered scenarios were skipped.cli-its-on-pull-request-sq7brpassed against CLI commit455530c490a3a4f4a79a54a60dd220730a31f687, with the validquay.io/conforma/tekton-task:konfluxreference restored.6ead7e45: every supplied parameter name/type matches the runner at4bbba993; both runner/test source pins agree; no deliberate-break bundle tag remains. The PR changes only the trigger file.Negative-run logs: https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/rhtap-contract-tenant/pipelinerun/cli-its-on-pull-request-v6kmv/logs/conforma-e2e-tests
Before merge
https://github.com/conforma/e2e-tests.git, and set both revisions to the resulting upstream commit.Remaining rollout work
Complete required-check enforcement without leaving unrelated PRs waiting for a path-filtered check. Keep EC-1943 open until merge protection is active. Separately investigate the artifact collection permission errors seen in the negative run; they did not cause the confirmed bundle-resolution failure.