fix(sbom): classify sidecar oversized images as TooLarge - #880
Conversation
Map sidecar FailedPrecondition responses to ErrImageTooLarge so SBOM processing treats oversized images as terminal TooLarge failures instead of retrying them as generic Incomplete errors. Signed-off-by: Manu <kiratcodes99@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesSidecar image-size error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ScannerServer
participant ScannerClient
participant SBOMManager
participant SBOMStorage
ScannerServer->>ScannerClient: Return FailedPrecondition
ScannerClient->>ScannerClient: Map response to ErrImageTooLarge
ScannerClient->>SBOMManager: Return ErrImageTooLarge
SBOMManager->>SBOMStorage: Mark fresh SBOM TooLarge
SBOMManager->>SBOMStorage: Retry content-bearing SBOM
SBOMManager->>SBOMStorage: Mark repeated failure Incomplete
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
| if ok && (st.Code() == codes.Unavailable || st.Code() == codes.Aborted) { | ||
| return nil, fmt.Errorf("%w: %v", ErrScannerCrashed, err) | ||
| } | ||
| if ok && st.Code() == codes.FailedPrecondition { |
There was a problem hiding this comment.
FailedPrecondition is currently the sidecar transport signal for ErrImageTooLarge.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Approved ✅
This is a faithful, minimal implementation of the fix described in #877. The sidecar branch mirrors the in-process branch semantics exactly — including the wipSbomHadContent guard that keeps a content-bearing SBOM away from the TooLarge one-way door — and both new tests are the correct counterparts to the existing in-process ones.
Verification performed locally (at 36de36d)
| Check | Result |
|---|---|
go build ./... |
pass |
go vet ./pkg/sbomscanner/... ./pkg/sbommanager/... |
pass |
go test ./pkg/sbomscanner/... ./pkg/sbommanager/... |
pass |
codes.FailedPrecondition producers repo-wide |
exactly one (server.go:60) — the mapping is unambiguous today |
Correctness review
client.go— the new mapping is placed after theUnavailable/Aborted→ErrScannerCrashedcheck, so a crashed sidecar still wins. Correct ordering.sbom_manager.go— the new branch is checked before the generic fallback and afterErrScannerCrashed, matching the in-process precedence. It correctly does not callSetSBOMScannerReady(false): an oversized image leaves the sidecar healthy.markSBOMStatus(..., TooLarge, nil)with noScannerMemoryLimitAnnotationis right here — that annotation exists to re-open the OOM-crash path when the memory limit changes, and an image-size limit breach is not memory-driven. Same as the in-process path.Test_..._PreservesContentOnSidecarTooLarge— I traced the loop: iterations 1–2 only increment the budget, iteration 3 pinsIncomplete, iterations 4–5 short-circuit on theIncomplete+ same-tool-version case.patchCalls == 1is the correct assertion.- Wire contract is still covered — the integration test now asserts the sentinel, but
server_test.go:TestCreateSBOM_ImageTooLargestill assertscodes.FailedPreconditionat the raw pb layer, so relaxing the integration assertion loses nothing. - Backward compatible under sidecar/node-agent version skew in both directions.
CodeRabbit found nothing actionable, and I found no blockers either. Three non-blocking notes inline — the gofmt one is worth folding in before merge since it's a one-character fix.
| var ( | ||
| ErrScannerCrashed = errors.New("SBOM scanner sidecar crashed during scan") | ||
| ErrScannerNotReady = errors.New("SBOM scanner sidecar not ready") | ||
| ErrImageTooLarge = errors.New("image size exceeds maximum allowed size") |
There was a problem hiding this comment.
Nit (worth fixing before merge): this line isn't gofmt-clean — the new entry breaks the var block's = alignment.
$ gofmt -d pkg/sbomscanner/v1/types.go
- ErrImageTooLarge = errors.New("image size exceeds maximum allowed size")
+ ErrImageTooLarge = errors.New("image size exceeds maximum allowed size")No CI gate catches it (go-basic-tests.yaml runs build + tests, no gofmt/lint step), so this won't turn the PR red — but the next person with format-on-save will produce a spurious diff here. For the record, the gofmt findings in client.go and server.go are import-ordering issues that already exist on main; only this one is new.
Separate, optional: the repo now has two distinct sentinels with byte-identical messages — syftutil.ErrImageTooLarge and this one. errors.Is does not relate them, which is exactly why sbom_manager.go has to check a different sentinel per branch. That's correct as written, but easy to misread. Since this package already imports syftutil (server.go), var ErrImageTooLarge = syftutil.ErrImageTooLarge would collapse them into one identity and make both branches match the same target. Your call — the current split is defensible if you want the transport sentinel to stay independent of the syft layer.
| if ok && (st.Code() == codes.Unavailable || st.Code() == codes.Aborted) { | ||
| return nil, fmt.Errorf("%w: %v", ErrScannerCrashed, err) | ||
| } | ||
| if ok && st.Code() == codes.FailedPrecondition { |
There was a problem hiding this comment.
Non-blocking, but the consequence is severe enough to flag. This is correct today — I grepped the repo and codes.FailedPrecondition is produced in exactly one place (server.go:60), so the mapping is unambiguous.
The concern is what happens when that stops being true. FailedPrecondition is a natural code to reach for on a future precondition failure ("layer paths not mounted", "scanner not initialized", ...). The moment a second one is added, this branch silently converts it to ErrImageTooLarge, and sbom_manager.go writes TooLarge — which, per the comment at sbom_manager.go:381-386, is a storage-layer one-way door where GuaranteedUpdate drops every future write. The SBOM would be permanently frozen and unfixable by any later version. Low probability, unbounded blast radius, and the failure is silent.
Two cheap ways to bound it, pick either:
- Match code and message. You already changed
server.go:60to sendErrImageTooLarge.Error(), but nothing consumes that — the coupling is currently write-only. Both binaries ship from this repo, so message matching is safe here and narrows the check to exactly the intended condition. - Keep the code-only check and add a one-line comment at
server.go:60reservingFailedPreconditionfor image-too-large, pointing at this branch as the consumer. Cheaper, and enough to stop the next person.
Option 2 alone is fine by me.
| } else { | ||
| s.markSBOMStatus(sbomName, helpersv1.TooLarge, nil) | ||
| } | ||
| s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonImageTooLarge, scanErr) |
There was a problem hiding this comment.
Test-coverage gap, non-blocking. #877 asked for a test asserting TooLarge and ReasonImageTooLarge and no retry-budget consumption. The two new tests nail the first and third — but not the reason.
newTestManagerWithScannerErr leaves failureReporter nil, and reportFailure returns immediately on a nil reporter, so this line never executes under test. That matters because the wrong user-facing reason was impact #2 in the issue: operators see "Failed to generate software inventory (SBOM)" instead of "Image exceeds the maximum size limit", which is the whole signal that raising maxImageSize is the remedy. Right now nothing would catch a regression that swaps this back to ReasonSBOMGenerationFailed.
In fairness this is symmetric with existing coverage — the in-process Test_..._MarksFreshImageTooLargeImmediately doesn't assert the reason either, and http_failure_reporter_test.go only tests the reporter in isolation, not which reason the manager hands it. So this is a pre-existing convention, not a regression you introduced.
A fake reporter capturing FailureReason into a slice, wired through newTestManagerWithScannerErr, would close it for both paths in a few lines. Happy to see it land as a follow-up rather than hold this PR.
2 participants
Overview
Fix the sidecar SBOM path to classify oversized images the same way as the in-process path. This change maps gRPC
FailedPreconditiontoErrImageTooLargeand updates SBOM handling to mark fresh SBOMs asTooLargewhile preserving content-bearing SBOMs on the genericIncompletepath.How to Test
Related issues/PRs
Summary by CodeRabbit
New Features
Bug Fixes
Tests