Skip to content
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

Enable allow-partial-results for Audit SCA scan and dep tree construction #200

Merged

Conversation

eranturgeman
Copy link
Contributor

@eranturgeman eranturgeman commented Oct 6, 2024

  • The pull request is targeting the dev branch.
  • The code has been validated to compile successfully by running go vet ./....
  • The code has been formatted properly using go fmt ./....
  • All static analysis checks passed.
  • All tests have passed. If this feature is not already covered by the tests, new tests have been added.
  • All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

This PR introduces a new ability that allows partial scan results without failing the entire scan over a single failing part in the scans (for example when we have multiple working directories and the scan on one of them have failed - we will not fail the entire scan, but rather log the error of the failing scan and continue).
This ability should be applied for SCA scans as well as for JAS scans but currently, until the security cli refactor (#96) will be merged it is applied only for SCA scans
Applying this ability to JAS scans will be on a different PR

In addition, a new flag was added to enable using this ability from the cli: --allow-partial-results

This PR contains also 2 bug fixes:

  1. In executeScaScanTask, ScaScansWg marked as Done in defer, which enables another process to close the SecurityParallelRunner.ErrorsQueue. Since the queue closing is performed by another thread, unrelated to the thread performing the scan, the queue sometimes gets closed before the error handling function passed along with executeScaScanTask to AddTaskWithError start executing.
In case we have an error, the addition of this error to the SecurityParallelRunner.ErrorsQueue might occur after the queue is closed, which leads to a panic.
The suggested fix is to close the queue only when there is no error, and if there is- the queue will be closed in the error handling function, after the error is added
  2. In GetScaScannedTechnologies we create a new set and insert the technologies from the current scan results only. I added the ability to pass already existing slice of techutils.Technology to enable preserving technologies from other scans if provided and to include them in the overall list of technologies (this is required for Frogbot, when multiple working directories are being scanned, but get all fixed together after all the scans. In this scenario the only remaining techs before the fix are the technologies of the last performed scan)

Note: Some of the NuGet Audit tests fail. This issue is known and will be fixed soon

@eranturgeman eranturgeman added new feature Automatically generated release notes safe to test Approve running integration tests on a pull request labels Oct 6, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 6, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Oct 8, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 8, 2024
@eranturgeman eranturgeman changed the title Enable partial results for Audit SCA scan and dep tree construction Enable allow-partial-results for Audit SCA scan and dep tree construction Oct 8, 2024
Copy link
Contributor

@attiasas attiasas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, take a look at my comments

cli/docs/flags.go Outdated Show resolved Hide resolved
commands/audit/audit.go Show resolved Hide resolved
commands/audit/audit.go Show resolved Hide resolved
commands/audit/audit_test.go Show resolved Hide resolved
commands/audit/scarunner.go Show resolved Hide resolved
commands/audit/scarunner.go Show resolved Hide resolved
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Oct 10, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 10, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Oct 10, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 10, 2024
…o enable-partial-results-scan-repo-sca

# Conflicts:
#	cli/docs/flags.go
#	cli/scancommands.go
#	commands/audit/scarunner.go
#	utils/auditbasicparams.go
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Oct 10, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 10, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Oct 13, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 13, 2024
Copy link

👍 Frogbot scanned this pull request and did not find any new security issues.


@eranturgeman eranturgeman merged commit 8224664 into jfrog:dev Oct 13, 2024
7 of 9 checks passed
@eranturgeman eranturgeman added ignore for release Automatically generated release notes and removed new feature Automatically generated release notes labels Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore for release Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants