RHOAIENG-58789: trust bots, lgtm, and scope xKS e2e triggers#3767
RHOAIENG-58789: trust bots, lgtm, and scope xKS e2e triggers#3767rinaldodev wants to merge 1 commit into
Conversation
|
/hold |
rinaldodev
force-pushed
the
worktree-RHOAIENG-58789
branch
from
588ce0e to
3ff6a19
Compare
📝 WalkthroughWalkthroughTwo GitHub Actions workflows now derive Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rinaldodev
changed the title
|
/unhold |
| types: [opened, labeled, synchronize, reopened] | ||
| branches: | ||
| - main | ||
| paths: |
There was a problem hiding this comment.
I think was better to keep all paths, since it now only require the label but we ensure to avoid other bugs.
Otherwise, we should also add all changes to all pkg folder (since changes in utilities could change behaviour in reconcilers), config/rhaii which is the manifests deployed to run those tests.
What do you think?
There was a problem hiding this comment.
@davidebianchi Until we can find a way to not need the label for private members, I'd rather not.
It adds value but also fricction, since 80% of org members are private and would require manually adding the label.
WDYT?
If you have a strong opinion I can change it.
There was a problem hiding this comment.
With the addition of run with the lgtm + approved label, we could run it when those are added?
By the way, ok but I'd add at least the config/rhaii folder
rinaldodev
force-pushed
the
worktree-RHOAIENG-58789
branch
from
3ff6a19 to
f48fff1
Compare
rinaldodev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-kind-odh-e2e.yaml:
- Around line 48-51: The run-xks-e2e label check is sticky because
get-merge-commit still uses github.event.pull_request.labels.*.name, so
synchronize updates can keep passing under pull_request_target after the label
was added. Update the gating logic in the workflow to verify the label against
the current head SHA or require a fresh approval/relabel for each push, and make
sure the job no longer proceeds into the checkout and make target steps unless
the current commit is explicitly re-approved. Refer to get-merge-commit and the
HAS_LABEL condition when adjusting the gate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 34085b87-e17f-4d29-a98f-a976b0b4935e
📒 Files selected for processing (2)
.github/workflows/test-cloudmanager-e2e.yaml.github/workflows/test-kind-odh-e2e.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-cloudmanager-e2e.yaml
| AUTHOR_LOGIN: ${{ github.event.pull_request.user.login }} | ||
| EVENT_ACTION: ${{ github.event.action }} | ||
| LABEL_NAME: ${{ github.event.label.name }} | ||
| HAS_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'run-xks-e2e') }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'test-kind-odh-e2e.yaml' .github/workflows --exec sed -n '24,40p;92,101p' {}Repository: opendatahub-io/opendatahub-operator
Length of output: 887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test-kind-odh-e2e.yaml (lines 40-80) =="
sed -n '40,80p' .github/workflows/test-kind-odh-e2e.yaml | cat -n
echo
echo "== get-merge-commit.yaml (entire file, line-numbered) =="
cat -n .github/workflows/get-merge-commit.yamlRepository: opendatahub-io/opendatahub-operator
Length of output: 3797
Critical: make the run-xks-e2e gate non-sticky across pushes. .github/workflows/test-kind-odh-e2e.yaml:12,29-37 uses github.event.pull_request.labels.*.name, so once run-xks-e2e is added, later synchronize runs on new commits still pass under pull_request_target (CWE-829, CWE-284). The job then checks out needs.get-merge-commit.outputs.mergedSha and runs PR-controlled make targets with secrets.CCM_PULL_SECRET in scope, which is the prt-scan secret-exfiltration pattern. Revoke the label on push or require fresh approval for the current head SHA.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-kind-odh-e2e.yaml around lines 48 - 51, The
run-xks-e2e label check is sticky because get-merge-commit still uses
github.event.pull_request.labels.*.name, so synchronize updates can keep passing
under pull_request_target after the label was added. Update the gating logic in
the workflow to verify the label against the current head SHA or require a fresh
approval/relabel for each push, and make sure the job no longer proceeds into
the checkout and make target steps unless the current commit is explicitly
re-approved. Refer to get-merge-commit and the HAS_LABEL condition when
adjusting the gate.
Sources: Path instructions, Linters/SAST tools
| types: [opened, labeled, synchronize, reopened] | ||
| branches: | ||
| - main | ||
| paths: |
There was a problem hiding this comment.
With the addition of run with the lgtm + approved label, we could run it when those are added?
By the way, ok but I'd add at least the config/rhaii folder
author_association misreports opendatahub-io members with private org visibility, so maintainers were having to re-add run-xks-e2e after every commit even for people who are actually trusted. Until a reliable org membership check is in place, accept the label as long as it is present on the PR instead of only on the labeled event. Bot-authored PRs (odh-release-bot, jira-autofix, dependabot) hit the same gap: author_association reports CONTRIBUTOR for them regardless of the app's actual permissions, confirmed on opendatahub-io#3768, opendatahub-io#3760, and opendatahub-io#3756. Trust those exact logins the same way trusted human associations are trusted. lgtm and approved are trusted too, but only at the exact labeled event, not whenever present. Prow strips lgtm from a PR on every new commit by default in this repo (no sticky_lgtm_team or store_tree_hash configured), so a malicious synchronize commit never carries the labeled action either label needs, even if Prow hasn't removed the stale lgtm yet. approved isn't auto-revoked on push, but that doesn't matter here since the check never inspects current label state, only the labeling event itself. Given that author_association gap, running the ODH xKS workflow on every PR relied too much on a trust signal that fails for most of the org. This workflow only exercises the kserve component on a Cloud Manager- provisioned cluster (make e2e-test-xks sets E2E_TEST_COMPONENT=kserve), so scope its trigger to kserve, cloudmanager, and config/rhaii paths (the manifests make deploy-rhaii-local deploys for this test) instead of running on every PR. The cloudmanager workflow already has its own path filter and keeps the label-and-authorization gate as its primary check, since that is where the actual secret risk is.
rinaldodev
force-pushed
the
worktree-RHOAIENG-58789
branch
from
f48fff1 to
6ffa565
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-kind-odh-e2e.yaml:
- Around line 54-64: Remove dependabot[bot] from the TRUSTED_BOTS allowlist in
the test-kind-odh-e2e workflow so Dependabot-authored pull_request_target runs
do not auto-enter the secret-bearing path. Keep the trusted-bot shortcut limited
to the other bot identities, and ensure dependabot stays on the
external-pr/label-based flow or is blocked from the steps that use
secrets.CCM_PULL_SECRET. If needed, adjust the job’s gating logic around the bot
check and the run-xks-e2e maintainer gate to prevent secret access for
Dependabot PRs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 17124c4f-0ff3-4844-a862-6c132f19a4c1
📒 Files selected for processing (2)
.github/workflows/test-cloudmanager-e2e.yaml.github/workflows/test-kind-odh-e2e.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-cloudmanager-e2e.yaml
| TRUSTED_BOTS=("odh-release-bot[bot]" "jira-autofix[bot]" "dependabot[bot]") | ||
| if [[ "$AUTHOR_ASSOCIATION" =~ ^(OWNER|MEMBER|COLLABORATOR)$ ]]; then | ||
| echo "Trusted contributor — running automatically" | ||
| exit 0 | ||
| fi | ||
| if [[ "$EVENT_ACTION" == "labeled" && "$LABEL_NAME" == "run-xks-e2e" ]]; then | ||
| for bot in "${TRUSTED_BOTS[@]}"; do | ||
| if [[ "$AUTHOR_LOGIN" == "$bot" ]]; then | ||
| echo "Trusted bot — running automatically" | ||
| exit 0 | ||
| fi | ||
| done |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'dependabot|CCM_PULL_SECRET|pull_request_target' .github/workflowsRepository: opendatahub-io/opendatahub-operator
Length of output: 1068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline test-kind workflow =="
ast-grep outline .github/workflows/test-kind-odh-e2e.yaml --view expanded || true
echo
echo "== relevant lines from test-kind workflow =="
sed -n '1,140p' .github/workflows/test-kind-odh-e2e.yaml | cat -n
echo
echo "== outline cloudmanager workflow =="
ast-grep outline .github/workflows/test-cloudmanager-e2e.yaml --view expanded || true
echo
echo "== relevant lines from cloudmanager workflow =="
sed -n '1,130p' .github/workflows/test-cloudmanager-e2e.yaml | cat -n
echo
echo "== search for label gates and trusted bot logic in workflows =="
rg -n 'run-xks-e2e|TRUSTED_BOTS|AUTHOR_ASSOCIATION|dependabot\[bot\]|CCM_PULL_SECRET|pull_request_target|mergedSha' .github/workflowsRepository: opendatahub-io/opendatahub-operator
Length of output: 17343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test-kind gate and secret-bearing step =="
sed -n '48,110p' .github/workflows/test-kind-odh-e2e.yaml | cat -n
echo
echo "== test-cloudmanager gate and secret-bearing step =="
sed -n '44,104p' .github/workflows/test-cloudmanager-e2e.yaml | cat -nRepository: opendatahub-io/opendatahub-operator
Length of output: 6277
Remove dependabot[bot] from TRUSTED_BOTS. This pull_request_target job later uses secrets.CCM_PULL_SECRET, so a Dependabot-authored PR can reach the secret-bearing build/e2e path without the run-xks-e2e maintainer gate (CWE-829/CWE-200). Keep Dependabot on the external-pr label path or skip the secret-bearing steps for bot-authored runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-kind-odh-e2e.yaml around lines 54 - 64, Remove
dependabot[bot] from the TRUSTED_BOTS allowlist in the test-kind-odh-e2e
workflow so Dependabot-authored pull_request_target runs do not auto-enter the
secret-bearing path. Keep the trusted-bot shortcut limited to the other bot
identities, and ensure dependabot stays on the external-pr/label-based flow or
is blocked from the steps that use secrets.CCM_PULL_SECRET. If needed, adjust
the job’s gating logic around the bot check and the run-xks-e2e maintainer gate
to prevent secret access for Dependabot PRs.
Source: Path instructions
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidebianchi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Description
RHOAIENG-58789
author_associationwas supposed to auto-trust the right people, but it under-reportsCONTRIBUTORfor cases that are actually trusted:odh-release-bot,jira-autofix, anddependabot(#3768, #3760, #3756).The membership fix still needs its own investigation into org-level read access. As a stopgap,
run-xks-e2eis now accepted whenever present instead of only onlabeled, and those three bot logins are trusted directly since a login can't be spoofed.lgtmis trusted too, but only at the exactlabeledevent: Prow strips it on every new commit here by default, so a malicioussynchronizecommit never carries the actionlgtmneeds, even before Prow removes the stale label.Since that first gap covers most of the org, running the ODH xKS workflow on every PR relied too much on a signal that fails most of the time:
make e2e-test-xkssetsE2E_TEST_COMPONENT=kserve), so its trigger is now scoped to kserve and cloudmanager paths.How Has This Been Tested?
Workflow-only change. Confirmed by reading the updated trigger and authorization logic: a labeled PR keeps running after new commits, a trusted bot login or a fresh
lgtmlabeling runs automatically, an unlabeled PR from a non-trusted author still fails, and a PR touching unrelated paths doesn't trigger the ODH workflow at all.Screenshot or short clip
N/A
Merge criteria
E2E test suite update requirement
E2E update requirement opt-out justification
CI workflow configuration only. No operator source code or tests changed.
Summary by CodeRabbit