fix(queue): guard malformed execution-order annotations - #2952
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2952 +/- ##
==========================================
+ Coverage 80.66% 80.78% +0.11%
==========================================
Files 164 164
Lines 13910 13911 +1
==========================================
+ Hits 11221 11238 +17
+ Misses 1967 1955 -12
+ Partials 722 718 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request hardens queue startup/rebuild logic by preventing the watcher from panicking when it encounters malformed pipelinesascode.tekton.dev/execution-order annotation entries, avoiding a cluster-wide crash-loop scenario.
Changes:
- Guard parsing of
namespace/nameexecution-order entries usingstrings.Cutand skip malformed values instead of indexing blindly. - Add a unit test covering multiple malformed execution-order entries mixed with a valid one to ensure no panic and correct filtering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/queue/queue_manager.go | Adds validation around parsing namespace/name execution-order items to prevent panics on malformed entries. |
| pkg/queue/queue_manager_test.go | Adds a regression test ensuring malformed execution-order entries are skipped without panicking. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Paco Review 🔍The PR fixes a panic risk where malformed "namespace/name" queue keys (e.g., from a user-editable execution-order annotation) were indexed into via strings.Split without validating the split length, which could cause an index-out-of-range panic in the queue manager and reconciler. It introduces a shared SplitPrKey helper that safely parses and validates these keys, applies it at the FilterPipelineRunByState and queuePipelineRun call sites, and refactors finalizer.go's promotion logic to delegate to a new r.startNextPipelineRunInQueue call. Extensive new unit tests cover the malformed-key edge cases for both the queue package and the reconciler's finalize flow. Review difficulty: 3/5 (Moderate) — The change spans queue and reconciler logic with panic-avoidance implications and includes a refactor whose new helper method definition is not visible in the diff, adding uncertainty despite solid test coverage. 1 new inline comment(s) found. Reviewed commit: a9c7fc9 |
pipelines-as-code
Bot
added
paco/review-easy
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
theakshaypant
left a comment
theakshaypant
left a comment
There was a problem hiding this comment.
The original issue states
pkg/reconciler/finalizer.go:80 splits and indexes the same way and
should get the same guard.
Suggest adding the same guard there.
theakshaypant
left a comment
theakshaypant
left a comment
There was a problem hiding this comment.
Also noticed the same call in reconciler.
3400041 to
7c33337
Compare
|
@theakshaypant thanks for catching that, pushed in the latest revision of this PR. (desloped this comment since my agent answered for me while not asking to do it 🙃 ) |
7c33337 to
d88c1b3
Compare
Split the "namespace/name" annotation with strings.Cut instead of strings.Split and skip empty, slash-missing, or extra-slash entries instead of indexing unconditionally. A malformed annotation previously panicked FilterPipelineRunByState, which InitQueues calls before workers start, crash-looping the watcher on restart. Fixes #2945 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com> Assisted-by: Claude
d88c1b3 to
a9c7fc9
Compare
|
/paco review |
pipelines-as-code
Bot
added
paco/review-moderate
| next := r.qm.RemoveAndTakeItemFromQueue(repo, pr) | ||
| if next != "" { | ||
| key := strings.Split(next, "/") | ||
| pr, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(key[0]).Get(ctx, key[1], metav1.GetOptions{}) |
There was a problem hiding this comment.
if this is deleted because of key[0] and key[1] then same is done in r.startNextPipelineRunInQueue as well so should we fix there as well?
📝 Description of the Change
If a PipelineRun ends up with a broken "which run is next" annotation
(empty, missing a slash, or with an extra slash in it), the watcher used
to crash instead of just skipping that broken entry. Because this
annotation gets read again every time the watcher starts up, one bad
entry would put the watcher into a permanent crash-restart-crash loop
for everyone, not just the one repository with the bad entry.
This change makes the watcher skip a broken entry instead of crashing
on it.
🔗 Linked GitHub Issue
Fixes #2945
JIRA: https://redhat.atlassian.net/browse/SRVKP-14113
🧪 Testing Strategy
Added a test that feeds the parser an empty string, a value with no
slash, a value with an empty namespace, a value with an empty name, and
a value with an extra slash, plus one normal valid entry, and checks
none of them panic and the valid one is still parsed correctly.
🤖 AI Assistance
This PR was written with AI assistance (Claude Sonnet 5),
following a post-merge review of PR #2890. The bug, the fix, and the
test were all verified by hand: I reproduced the crash against the
merged code before writing the fix, and confirmed the fix and test pass.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.