Repository navigation
test: run ITS pipeline e2e checks on pull requests - #3574
dheerajodha wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🟡 Moderate · up to It remains unclear whether an untrusted pull request can reach this test run or whether its pipeline definition can access configured credentials. Confirm the authorization policy and runner isolation before merging. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: running ITS pipeline end-to-end checks on pull requests. |
| Description check | |
The description explains what changed, why it changed, validation results, dependencies, authorization concerns, and remaining work. It does not include a ticket link, but the required context is othe… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
Comment @coderabbitai help to get the list of available commands.
|
Risk Assessment: low (1/5) DetailsTier 1 signals unchanged from prior assessment (single new PaC trigger YAML, no protected paths, no security-sensitive files, no dependency changes, returning contributor, config-only treated as neutral on test ratio); Tier 2 new-file baseline of 2 yields composite 0.62x1.13 + 0.38x2 = 1.46 which rounds to 1; the only delta since prior review is a revision pin bump, so prior score of 1 is preserved. Previous runRisk Assessment: low (1/5) DetailsSingle new PaC trigger YAML (53 lines, small blast radius), no protected paths, no security-sensitive files, no dependency changes, config-only PR treated as neutral on test ratio, returning contributor; new-file Tier 2 baseline of 2 nudges the composite to 1.46, which rounds to 1. Previous run (2)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 run (3)Risk 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 (4)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 (5)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 (6)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. |
ReviewFindingsHigh
Next steps:
Previous runReviewFindingsHigh
Next steps:
Previous run (2)ReviewSince the prior review (SHA FindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewSince the prior review (SHA FindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewSince the prior review (SHA FindingsCritical
High
Medium
Low
Next steps:
Previous run (5)ReviewThe CEL author_association gate added since the prior review ( FindingsCritical
High
Medium
Low
Next steps:
Previous run (6)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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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!
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/dheerajodha/conforma-e2e-tests.git |
There was a problem hiding this comment.
I'm assuming this is WIP, right?
There was a problem hiding this comment.
In a way, but it's still ready for review. I was just waiting for the conforma/e2e-tests#12 to get merged first. And then update the refs here.
|
Looks like it's still WIP, consider moving to draft. |
|
🤖 Review · Commit: |
Run the ITS E2E suite for changes to the enterprise-contract pipeline or its CI trigger. Pin the runner and tests to the merged upstream E2E commit, and resolve the pipeline under test from the pull request source and revision. Refs: EC-1943
8e0348b to
4fc0222
Compare
fullsend-ai-review
Bot
added
risk/low
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
The Red Hat Konflux / cli-its-on-pull-request check failed due to some Kubernetes dependency issue, it's previous runs were successful, I'm looking into it now. |
Opened PR: conforma/e2e-tests#33 |
| - name: deprovision-aws-credentials-secret | ||
| value: mapt-kind-secret | ||
| - name: its-pipeline-repo-url | ||
| value: '{{source_url}}' |
There was a problem hiding this comment.
[high] fail-open
The its-pipeline-repo-url param is set to the PaC template {{source_url}} (line 36) and its-pipeline-revision to {{revision}} (line 38). For a pull_request event these resolve to the PR source repository and commit, which for a fork PR is attacker-controlled. The on-cel-expression (lines 11-14) gates only on event type, target branch, and path changes — .tekton/cli-its-pull-request.yaml or pipelines/enterprise-contract/** — with no author/author-association filter and no restriction to the base repo. The orchestrating pipeline (resolved from a pinned conforma/e2e-tests revision) then fetches the ITS pipeline-under-test from the fork at pipelines/enterprise-contract/0.1/enterprise-contract.yaml (line 40) and runs it under serviceAccountName: konflux-integration-runner (line 53), which has access to the referenced secrets konflux-test-infra (line 30), mapt-kind-secret (lines 32 and 34). The only remaining boundary is Pipelines-as-Code's external authorization (Repository CR / global auth / /ok-to-test). The PR author explicitly states this external config has not been inspected. If that external config is permissive, missing, or regresses to a default that permits fork PRs without explicit maintainer approval, the trigger fail-opens: a fork PR can execute arbitrary pipeline YAML with the integration-runner service account and the above cluster-held credentials. The incremental change since the prior review (revision pin bump from eb59162d... to 18ee6ce5..., both on conforma/e2e-tests main) does not affect this gap; line 36 is unchanged. Severity preserved from the prior review per the severity-anchoring rule for a clear structural match in unchanged code.
Suggested fix: Do not rely solely on out-of-band PaC authorization for a trigger that executes PR-supplied pipeline YAML with secret-bearing credentials. Pick one of: (1) Pin its-pipeline-repo-url to https://github.com/conforma/cli.git (or another trusted repo) and its-pipeline-revision to {{target_branch}} so fork PRs cannot swap in attacker-controlled pipeline YAML; (2) Add a CEL guard that excludes fork PRs, e.g. require body.pull_request.head.repo.full_name == body.pull_request.base.repo.full_name, so this trigger only ever runs same-repo PRs; (3) Document and enforce in-repo (checked-in Repository CR / policy) that this PipelineRun requires explicit maintainer approval (/ok-to-test) before execution for any non-collaborator author, and verify that config is deployed before merging. Also confirm whether the pipeline needs aws-credentials-secret/deprovision-aws-credentials-secret/oci-container-repo-credentials-secret at all for PR validation, and drop any that are not required (least privilege).
After this PR was merged, I updated the SHAs in this PR to track the merged SHA. And now the the check passed successfully. Here's the snippet from the logs which shows that the e2e-tests tested the EC pipeline from this PR's commit: PS: The EC ITS check fails because of a vulnerability, not related to this PR. |
This branch has not been deployed
4 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
Dependency conforma/e2e-tests#12 is merged. Both runner and test-suite sources now use
https://github.com/conforma/e2e-tests.gitpinned to its upstream merge commiteb59162d7c1d069a699f82a50e9f41db852c53d2.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
The PR history is now one signed commit,
4fc0222b, based on upstream maind7152fd9. The final diff adds only the ITS trigger; its contents are unchanged by the rebase. YAML parsing, upstream runner parameter checks, and whitespace validation passed. Fresh CI results are pending. The runs below document earlier positive and negative 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
4fc0222b, rebased onto upstream maind7152fd9).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.