fix: failed report auto-resubmits and completes after redeploy#264
fix: failed report auto-resubmits and completes after redeploy#264rhartuv wants to merge 2 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
@rhartuv Thank you for the fix.
This bug exists since the client backend was created...
It looks like a sound solution for the problem..
Have you tested it both on cluster and locally?
There was a problem hiding this comment.
Hi @rhartuv ,
Following our conversation, Two things that i found meaningful before we merging:
-
Add a test for two scenarios - one for failed item , and one for non failed ( queued, active) , you can do it on the POST
/reportsendpoint ( "receive callback endpoint") . -
Another thing, is more semantic - Now that the operation is not really doing anything in case the report has expired/failed, returning 200/202 is incorrect semantically, as the request' processing isn't updating anything, and when it returned to the agent, it logged there as success like it was successful, while it's not.This is not meaningful to user experience, but in centralized logs backend, when trying to trace problems, this looks confusing and inconsistent, so it's better get addressed.
Two possible HTTP return values that are appropriate for that use case are 409 ( Conflict) and 422 ( unprocessable entity).
This reflecting and indicating better what is the state of the processing ( nothing was processed and that there was conflict to update a report that expired/failed before analysis got completed on agent for it).In such case, two possible courses of action on the agent side:
-
Accept it as an error - Which it true, analysis wasn't retrieved on the intended configured target/sink - in such case it will retry like in all http errors non 200/201/202 until reached to maximum re-tries attempts - no change needed on agent
-
Absorb it in the agent with a warning message ( by accepting also 409/422 http statuses as possible expected return codes) that message didn't arrive to its target/sink due to conflict ( report expired/failed before analysis completion) - in such case: no retries on from the agent, and sum up to 1-3 lines of code change in the agent.
-
|
Thank you Zvi for the comment @zvigrinberg 👩🏭
|
Summary
The bug:
When
EXPLOIT_IQ_QUEUE_TIMEOUT=10sandEXPLOIT_IQ_QUEUE_MAX_ACTIVE=5are configured on the exploit-iq-client deployment, a CycloneDX analysis report correctly fails with Failed status and failure reason "timeout after 10 seconds".However, after removing both environment variables from the exploit-iq-client deployment and redeploying the pod, the previously failed report is automatically resubmitted and the analysis completes successfully (e.g. Not vulnerable) without any user action.
For more details see: Jira
The fix:
Prevent late ExploitIQ agent callbacks from overwriting reports that are already in a terminal failure state (expired, failed, etc.).
What changed
ReportRepositoryServiceAdded
hasTerminalError(String id)- returns true when the MongoDB document has an error field (coversexpired,failed, and any other error type).Changed
updateWithOutput()to skip those reports, log a warning, and return only the ids that were actually updated.ReportService.receive()Uses the returned list from
updateWithOutput(), so completion events and queue cleanup only run for reports that were updated.Explicit user retry (POST /reports/{id}/retry) still works:
setAsRetried()clears the error before re-queuing, so a subsequent callback is accepted.