Skip to content

ci: simplify /build-images to always build PR HEAD - #3076

Merged
rm3l merged 4 commits into
redhat-developer:mainfrom
rm3l:fix/build-images-sha-validation
Jun 25, 2026
Merged

ci: simplify /build-images to always build PR HEAD#3076
rm3l merged 4 commits into
redhat-developer:mainfrom
rm3l:fix/build-images-sha-validation

Conversation

@rm3l

@rm3l rm3l commented Jun 24, 2026

Copy link
Copy Markdown
Member

Description

Follow-up to #2293 (59554ed) — simplifies the /build-images slash command to always build the HEAD of the PR branch.

  • Removes SHA argument parsing — /build-images no longer accepts or requires a commit SHA
  • Uses pr.data.head.sha directly from the GitHub API, which is trusted and always points to the latest PR commit
  • Eliminates the TOCTOU window where a maintainer could (accidentally or intentionally) build from an arbitrary commit in the fork repo

Which issue(s) does this PR fix or relate to

PR acceptance criteria

  • Tests
  • Documentation

How to test changes / Special notes to the reviewer

Can only be tested after merging:

  • Comment /build-images on a PR — should build the PR HEAD
  • Verify the built images match the latest commit on the PR branch

Allow /build-images with no argument (defaults to PR HEAD SHA) and
validate that any provided SHA actually belongs to a commit in the PR,
preventing builds from arbitrary commits in the fork repo.

Assisted-by: Claude <noreply@anthropic.com>
@rm3l

rm3l commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

/build-images

@github-actions

Copy link
Copy Markdown
Contributor

Image build failed

See workflow run for details: https://github.com/redhat-developer/rhdh-operator/actions/runs/28113224441

Triggered by @rm3l

@rm3l

rm3l commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

/build-images 10e3338

@rm3l rm3l changed the title fix: harden /build-images SHA handling and validation fix(ci): harden /build-images SHA handling and validation Jun 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR images built successfully!

Images are available for testing:

  1. Operator: quay.io/rhdh-community/operator:0.11.0-pr-3076-10e3338
  2. Bundle: quay.io/rhdh-community/operator-bundle:0.11.0-pr-3076-10e3338
  3. Catalog: quay.io/rhdh-community/operator-catalog:0.11.0-pr-3076-10e3338

Also available with PR number tag:

  • quay.io/rhdh-community/operator:0.11.0-pr-3076
  • quay.io/rhdh-community/operator-bundle:0.11.0-pr-3076
  • quay.io/rhdh-community/operator-catalog:0.11.0-pr-3076

Triggered by @rm3l

@rm3l

rm3l commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

PR images built successfully!

Images are available for testing:

1. **Operator:** `quay.io/rhdh-community/operator:0.11.0-pr-3076-10e3338`

2. **Bundle:** `quay.io/rhdh-community/operator-bundle:0.11.0-pr-3076-10e3338`

3. **Catalog:** `quay.io/rhdh-community/operator-catalog:0.11.0-pr-3076-10e3338`

Also available with PR number tag:

* `quay.io/rhdh-community/operator:0.11.0-pr-3076`

* `quay.io/rhdh-community/operator-bundle:0.11.0-pr-3076`

* `quay.io/rhdh-community/operator-catalog:0.11.0-pr-3076`

Triggered by @rm3l

This is misleading as 10e3338 does not belong to this PR branch at all. This is the issue this PR aims to solve.

The PR HEAD SHA comes from the GitHub API and is already trusted —
no need to iterate over the commit list when no SHA argument is given.

Assisted-by: Claude <noreply@anthropic.com>
@rm3l
rm3l marked this pull request as ready for review June 24, 2026 16:55
@rm3l
rm3l requested a review from a team as a code owner June 24, 2026 16:55
@rm3l

rm3l commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

/cc @gazarenkov

@openshift-ci
openshift-ci Bot requested a review from gazarenkov June 24, 2026 16:55
@rhdh-qodo-merge

Copy link
Copy Markdown

Looking for bugs?

Check back in a few minutes. Qodo's review agents are on it.

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Harden /build-images SHA parsing and restrict builds to PR commits
🐞 Bug fix ⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

Grey Divider

Description

• Allow /build-images with no SHA to build the PR branch HEAD commit
• Validate any provided SHA is a commit that belongs to the PR (with pagination)
• Update PR template instructions to reflect new /build-images usage
Diagram

graph TD
  A["Maintainer comment"] --> B["Workflow: parse /build-images"] --> C{{"SHA provided?"}}
  C -->|No| D["Use PR head SHA"] --> G["Set outputs (sha/repo/number)"]
  C -->|Yes| E["List PR commits (paginated)"] --> F{{"SHA in PR commits?"}}
  F -->|Yes| G
  F -->|No| H["Fail security gate"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. GraphQL commit membership check
  • ➕ Potentially fewer round-trips by querying commit OIDs with cursor pagination
  • ➕ Can make the membership check more explicit/structured
  • ➖ More complex queries and harder-to-maintain workflow code
  • ➖ Still requires pagination for large PRs
2. Checkout PR ref and use git to validate SHA
  • ➕ Fast local validation with git branch --contains / merge-base once refs are fetched
  • ➕ No need to build an in-memory SHA list
  • ➖ Requires fetching additional refs from forks, which may reintroduce trust/safety concerns
  • ➖ More moving parts (checkout depth, refs, auth) and longer runtime

Recommendation: The current REST-based approach (default to trusted PR head SHA; otherwise enumerate PR commits with pagination and match full/short SHA) is the most straightforward and security-aligned for a comment-triggered workflow. It avoids relying on fork git fetch behavior while closing the gap that allowed building non-PR commits.

Files changed (2) +37 / -12

Bug fix (1) +32 / -8
pr-container-build.yamlDefault to PR head SHA and validate requested SHA against PR commits +32/-8

Default to PR head SHA and validate requested SHA against PR commits

• Changes the '/build-images' command parsing to accept no SHA and use 'pr.head.sha' as the trusted default. When a SHA is provided, fetches the full (paginated) PR commit list and only proceeds if the requested SHA matches a PR commit (full SHA or prefix), otherwise fails the workflow with a security gate message.

.github/workflows/pr-container-build.yaml

Documentation (1) +5 / -4
PULL_REQUEST_TEMPLATE.mdDocument '/build-images' default-to-HEAD behavior +5/-4

Document '/build-images' default-to-HEAD behavior

• Updates the maintainer instructions to allow '/build-images' with no argument (build PR head) and clarifies that '/build-images <sha>' must reference a commit included in the PR. Adjusts contributor guidance accordingly.

.github/PULL_REQUEST_TEMPLATE.md

@rm3l

rm3l commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/cherry-pick release-1.10
/cherry-pick release-1.9

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: once the present PR merges, I will cherry-pick it on top of release-1.10, release-1.9 in new PRs and assign them to you.

Details

In response to this:

/cherry-pick release-1.10
/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated
rm3l added 2 commits June 25, 2026 12:14
Removes SHA argument parsing and commit validation — /build-images
now always builds the HEAD of the PR branch. The SHA comes directly
from the GitHub API (pr.data.head.sha), which is trusted.

Assisted-by: Claude <noreply@anthropic.com>
@rm3l rm3l changed the title fix(ci): harden /build-images SHA handling and validation fix(ci): simplify /build-images to always build PR HEAD Jun 25, 2026
@rm3l rm3l changed the title fix(ci): simplify /build-images to always build PR HEAD fix(ci): simplify /build-images to always build PR HEAD Jun 25, 2026
@rm3l
rm3l requested a review from gazarenkov June 25, 2026 10:19
@openshift-ci openshift-ci Bot added the lgtm label Jun 25, 2026
@rm3l rm3l changed the title fix(ci): simplify /build-images to always build PR HEAD ci: simplify /build-images to always build PR HEAD Jun 25, 2026
@rm3l
rm3l merged commit fd27f00 into redhat-developer:main Jun 25, 2026
6 checks passed
@rm3l
rm3l deleted the fix/build-images-sha-validation branch June 25, 2026 10:23
@sonarqubecloud

Copy link
Copy Markdown

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: #3076 failed to apply on top of branch "release-1.10":

Applying: fix: harden /build-images SHA handling and validation
Using index info to reconstruct a base tree...
M	.github/workflows/pr-container-build.yaml
Falling back to patching base and 3-way merge...
Auto-merging .github/workflows/pr-container-build.yaml
CONFLICT (content): Merge conflict in .github/workflows/pr-container-build.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 fix: harden /build-images SHA handling and validation

Details

In response to this:

/cherry-pick release-1.10
/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: #3076 failed to apply on top of branch "release-1.9":

Applying: fix: harden /build-images SHA handling and validation
Using index info to reconstruct a base tree...
M	.github/workflows/pr-container-build.yaml
Falling back to patching base and 3-way merge...
Auto-merging .github/workflows/pr-container-build.yaml
CONFLICT (content): Merge conflict in .github/workflows/pr-container-build.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 fix: harden /build-images SHA handling and validation

Details

In response to this:

/cherry-pick release-1.10
/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants