-
Notifications
You must be signed in to change notification settings - Fork 89
Parallelize the scan pr flow #1028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3_er
Are you sure you want to change the base?
Conversation
- Run SCA and JAS scans in parallel for better performance - Use LogCollector for isolated log capture per scan - Logs dumped sequentially after scans complete (no interleaving) - Add SetLogCollector to ScanDetails for passing collector to CLI - Export FrogbotUploadRtRepoPath for result upload
- Add JF_PARALLEL_PR_SCAN env var (default: true, set to 'false' for sequential mode) - Remove verbose/redundant comments - Remove gradle cache files from tracking
…ing, support FailUponAnyScannerError
…t, remove comments
- scanPullRequestBranches: scanning + partial results filtering - auditPullRequestAndReport: output flags + issue conversion
|
frogbot-pr-info.txt - regular log mode frogbot-pr-debug.txt - debug mode |
| var err error | ||
|
|
||
| if isParallelScanEnabled() { | ||
| scanResults, err = auditBranchesInParallel(scanDetails, sourceBranchWd, targetBranchWd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which type of errors can be returned here? isn't it make sense to check here the FailUponAnyScannerError() and continue to the filter function in case its false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk, i kept it consistent with the original logic
| startTime := time.Now() | ||
|
|
||
| // Pre-download Analyzer Manager to avoid parallel download contention | ||
| if err := securityjas.DownloadAnalyzerManagerIfNeeded(0); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure we want to have this analyzer manager logic in frogbot. the AM is a "cli problem", and in general we want to simplify frogbot and not giving it more responsibility. is there a way to avoid it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really
you are triggering cli in parallel
meaning jas-target and jas-source will try to download it if it doesn't exist
and sca-source will also try to install it for ca if CA is needed
worst case you are installing AM 3 times to scan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding @attiasas to the disscussion
what do you think about the following solution: create a new public function in cli-security: PrepareEnvForScan() which will be called by frogbot once and will downloiad AM (and new-sca engine?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have similar thing for our tests in the reposiotry. can you access this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding @attiasas to the disscussion what do you think about the following solution: create a new public function in cli-security: PrepareEnvForScan() which will be called by frogbot once and will downloiad AM (and new-sca engine?)
new sca engine its fine it will be downloaded once anyway by cli , not our responsibility
| diffResults := results.UnifyScaAndJasResults(scaResult.source, jasDiff) | ||
|
|
||
| log.Info("Uploading results to platform...") | ||
| if _, err := output.UploadCommandResults(scanDetails.ServerDetails, utils.FrogbotUploadRtRepoPath, diffResults); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this operation exist in frogbot before ? (uploading the results to RT) or was it cli responsibility before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli
it uploaded after the second scan finished, and it knew it was in a diff context
now scans in parallel and it doesnt know its in diff really (other than sca)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if i understand - it means that for scan-pr -> frogbot uploads results, but for scan-repo - cli uploads the results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
| targetLogCollector := audit.NewLogCollector(log.GetLogger().GetLogLevel()) | ||
| targetScanDetails.SetLogCollector(targetLogCollector) | ||
|
|
||
| targetResults = targetScanDetails.Audit(targetDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are the Audit commands in runScaScans are not running in parallel but in run JasScans they are executed as a go routine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because in sca , for diff:
We calculate the target sca sbom
delta it with source, during source scan
and then what is left we do sca scan and contextual when we scan the source
running the sca on target takes milliseconds becasue sbom calculation is almost immediate with the new engine
| if e != nil { | ||
| err = errors.Join(err, fmt.Errorf("failed to get issues for pull request. Error: %s", e.Error())) | ||
| log.Debug("Scanning source branch code...") | ||
| scanDetails.SetUploadCdxResults(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain to me what is the purpose of this line scanDetails.SetUploadCdxResults(true) ? and in general why did you add SetUploadCdxResults(upload bool) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/jfrog/frogbot/pull/1028/changes#r2693788773
evolved from the issue in this thread
in the past we sent results in scan repo always
and in sca pr only on the second scan, and we knew if we were the second scan because of value of DiffScan and , resultsToComapre see this:
SetUploadCdxResults(!sc.diffScan || sc.ResultsToCompare != nil)
now because scanning logic changed for scan pr,it became a lot more trivial to just send true or false to whether we need to upload results, than to let the function figure it out on its own
scanpullrequest/scanpullrequest.go
Outdated
| scaResult := <-scaChan | ||
| jasResult := <-jasChan | ||
|
|
||
| if scaResult.target != nil && scaResult.target.GetStatusCodes().ScaScanStatusCode != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no possible scenario where for example scaResult.target==nil and also err==nil ? because in this case there is no log at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only case is maybe if sca is turned off , which is basically what im checking here
will validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i updated the condition for this and sca considering all the edge cases (turned off by config, entitlements make jas not run)
| dependencySubmissionFrogbotDetector = "JFrog Frogbot" | ||
| frogbotUrl = "https://github.com/jfrog/frogbot" | ||
| frogbotUploadRtRepoPath = "frogbot" | ||
| FrogbotUploadRtRepoPath = "frogbot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will move it up
| } | ||
| } | ||
|
|
||
| // CloneForBranchScan creates a clone configured for branch scanning with isolated logging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ill proably remove comment say if its clear
Uh oh!
There was an error while loading. Please reload this page.