Add optional concurrent per-image verification - #199
Draft
bdehamer wants to merge 3 commits into
Draft
Conversation
When a single admission request contains multiple images (e.g. a multi-container pod), the provider validated them one at a time on the shared request context, so a slow fetch for one image could starve the images processed after it (issue #195). Add an opt-in, bounded worker pool that verifies the request's images in parallel. Only the outer, per-image loop is parallelized; each image still fetches its own attestation bundles serially. The behavior is gated behind a new `-image-concurrency` start-up flag that defaults to 1 (the existing serial behavior). The per-image logic is extracted into a `validateImage` helper, and result ordering plus the verification-error short-circuit are preserved regardless of serial or parallel execution. Refs #195 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds opt-in, bounded concurrent image verification while preserving serial behavior by default.
Changes:
- Adds
WithConcurrencyand ordered concurrent validation. - Adds the
-image-concurrencystartup flag. - Adds concurrency, bounds, parity, and error-path tests.
Show a summary per file
| File | Description |
|---|---|
pkg/provider/provider.go |
Implements concurrent per-image validation. |
pkg/provider/provider_test.go |
Tests concurrency behavior and limits. |
cmd/aaop/aaop.go |
Adds and validates the concurrency flag. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
A panic from the pluggable fetcher/verifier in a wg.Go worker goroutine would crash the entire provider process, since net/http only recovers panics in the request goroutine (the serial path). Recover inside each worker and convert the panic into that image's indexed system error, so opt-in concurrency cannot turn one malformed request or dependency panic into a process-wide outage. Adds a panic-path test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The returned option previously clamped n < 1 inside the closure, mutating its captured variable on every application. Reusing one such option across concurrent New calls would race on that shared state. Normalize n once before creating the closure so applying an Option is read-only and safely reusable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
piceri
approved these changes
Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the first idea from #195: verify the images in a multi-image admission request concurrently instead of one at a time.
Today
Validateloops overr.Request.Keysand callsBundleFromNamefor each image on the shared request context, so a slow fetch for one image can consume most of the shared deadline and starve the images processed after it. This PR adds an opt-in, bounded worker pool for the outer, per-image loop so total wall-clock time approaches the max of the per-image fetches rather than the sum.Per the request, scope is deliberately narrow:
DoBundleFromNameis untouched).Changes
-image-concurrencyflag (cmd/aaop/aaop.go), default1(serial). Values> 1cap the number of images verified simultaneously; values< 1are rejected at start-up.provider.WithConcurrency(n)option (pkg/provider/provider.go).Newis now variadic and defaults to serial;n < 1is clamped to1.validateImagehelper.Validatedispatches to a serial path (unchanged, byte-for-byte behavior including the verification-error short-circuit) or a bounded worker pool (sync.WaitGroup.Go+ a semaphore channel).SystemError(the first error in key order wins, so results are deterministic in either mode).Behavior notes
-image-concurrency=1) is identical to the previous serial implementation.--bundle-timeout/--bundle-max-attemptssemantics or introduce per-image sub-budgets (ideas Added support for authn. #2–Bump golangci/golangci-lint-action from 6.1.1 to 6.5.2 in the minor-patch group #4 in the issue remain open).slogcalls are goroutine-safe, so per-image metrics/logging are unchanged under parallelism.Testing
go test -race ./...passes.golangci-lint runreports 0 issues.WithConcurrencyclamping, and the system-error path under concurrency.Follow-ups / not included
bundle-*tuning flags (also chart-less). Happy to add chart plumbing if desired.Refs #195