ISSUE-1830: Adding Stop scan related changes for Vulnerability, Compliance and Cloud compliance scans #1585

varunsharma0286 · 2023-09-18T18:00:47Z

Fixes #
https://github.com/deepfence/enterprise-roadmap/issues/1830

Changes proposed in this pull request:

Changes to support Stop scan functionality in vulnerability scans.
Changes to support Stop scan functionality in compliance scans.
Changes to support Stop scan functionality in cloud compliance scans

deepfence_bootstrapper/controls/controls.go

deepfence_bootstrapper/router/common.go

deepfence_bootstrapper/router/generate_sbom.go

noboruma · 2023-09-19T00:14:24Z

deepfence_server/controls/agent.go

+		WHERE s.stop_requested=true
 		WITH s LIMIT $max_work
-        SET s.status = '`+utils.SCAN_STATUS_CANCELLING+`', s.updated_at = TIMESTAMP()
+        SET s.status = '`+utils.SCAN_STATUS_CANCELLING+`', 
+		s.updated_at = TIMESTAMP(), s.stop_requested=false


Why are we introducing this flag?
Previous behavior is more aligned with the whole status workflow:

Starting a scan: Starting, In progress

Stopping a scan: Cancelling, Cancelled

No need to fork and have a stop_requested flag now

Stop functionality by relying on the scan status will not work completely.
Reason being, an already running scan will continuously emit "IN PROGRESS" asynchronously
Hence there's a chance we might overwrite the "CANCEL_PENDING" status with IN PROGRESS" even before we check this scan.
This will result in the stop never getting triggered.
Hence we would need this stop_requested flag for deterministic behavior.

That's a good point, but the flag is not the solution: we need to set some priority when setting the status
The same way that if IN PROGRESS come after a COMPLETE we should not "uncomplete" the scan.
Let's make it consistent across the whole scan system

We have some guarantees for the statuses:

IN PROGRESS cannot come in before the "STARTING".

IN PROGRESS cannot come after the "COMPLETED" , "ERROR" or "CANCELLED" state (terminal state).
If these 2 are violated, we have a bug in the code and need to be fixed :)

Also, the IN_PROGRESS is used at the kafka ingester for updating the db. Making any change there is not ideal as it's going to complicate the logic. Also, that's the hot path and might have performance impact.

Reverted the flag change as we will be adding the logic to handle this in the status ingestion.

deepfence_server/handler/scan_reports.go

deepfence_server/model/cloud_node.go

noboruma · 2023-09-19T00:22:21Z

deepfence_server/reporters/scan/scan_result_actions.go

+	query := `MATCH (n:%s) -[:SCANNED]-> ()
+        WHERE n.node_id = $scan_id
+        AND n.status = $in_progress
+        SET n.status=$cancel_pending, n.stop_requested=true`


You can see here how the stop_requested is redundant with the cancel_pending status

Actually this is not going to work :(
Reason being, an already running scan will continuously emit "IN PROGRESS" asynchronously
Hence there's a chance we might overwrite the "CANCEL_PENDING" status with IN PROGRESS" even before we check this scan.
This will result in the stop never getting triggered.
Hence we would need this flag for deterministic behavior.

noboruma · 2023-09-19T00:26:02Z

deepfence_worker/controls/controls.go

+	err = RegisterControl(ctl.StopVulnerabilityScan,
+		GetRegisterControlFunc[ctl.StopVulnerabilityScanRequest](pub,
+			sdkUtils.StopVulnerabilityScanTask))


nit: for consistency, let's put stopVuln scan before secret & malware, to keep them in the same order as the starts

noboruma · 2023-09-19T00:26:19Z

deepfence_worker/controls/controls.go

 	if err != nil {
 		return err
 	}

 	return nil
 }
+
+func GetRegisterControlFunc[T ctl.StartVulnerabilityScanRequest | ctl.StartSecretScanRequest |


Nice refactor

noboruma · 2023-09-19T00:30:33Z

deepfence_worker/tasks/sbom/generate_sbom.go

+	scanID := params.ScanId
+	cancelFnObj, found := scanMap.Load(scanID)
+	logMsg := ""
+	if found {
+		cancelFn := cancelFnObj.(context.CancelFunc)
+		cancelFn()
+		logMsg = "Stop GenerateSBOM request submitted"
+	} else {
+		logMsg = "Failed to Stop scan, SBOM may have already generated or errored out"
+	}
+
+	log.Info().Msgf("%s, scan_id: %s", logMsg, scanID)
+	return nil


If there is an error, let's propagate it up instead of logging
We can also wrap it:

err = fmt.Errorf("Failed to stop: %w", err)

This is not a hard error as such.
The stop can fail if the scan was either not started or has already completed.
In either case, the scan is not running.

deepfence_server/reporters/scan/scan_reporters.go

noboruma

The cancelling logic should rely on status only, the same way STARTING is purely based on status

Compliance and Cloud compliance scans

noboruma · 2023-09-19T15:28:35Z

deepfence_server/handler/cloud_node.go

 			scanDetail := model.CloudComplianceScanDetails{
 				ScanId:        scan.ScanId,
 				ScanTypes:     scan.BenchmarkTypes,
 				AccountId:     req.CloudAccount,
 				Benchmarks:    benchmarks,
-				StopRequested: scan.StopRequested,
+				StopRequested: stopRequested,


Suggested change

StopRequested: stopRequested,

StopRequested: scan.Status == utils.SCAN_STATUS_CANCELLING,

noboruma · 2023-09-19T15:28:54Z

deepfence_server/handler/cloud_node.go

 				scanDetail := model.CloudComplianceScanDetails{
 					ScanId:        scan.ScanId,
 					ScanTypes:     scan.BenchmarkTypes,
 					AccountId:     monitoredAccountId,
 					Benchmarks:    benchmarks,
-					StopRequested: scan.StopRequested,
+					StopRequested: stopRequested,


Suggested change

StopRequested: stopRequested,

StopRequested: scan.Status == utils.SCAN_STATUS_CANCELLING,

deepfence_server/handler/cloud_node.go

noboruma

lgtm

varunsharma0286 force-pushed the ISSUE-1830-2 branch from 9b12357 to 17e634c Compare September 18, 2023 20:02

noboruma reviewed Sep 18, 2023

View reviewed changes

deepfence_bootstrapper/controls/controls.go Outdated Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_bootstrapper/controls/controls.go Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_bootstrapper/controls/controls.go Outdated Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_bootstrapper/router/common.go Outdated Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_bootstrapper/router/generate_sbom.go Outdated Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_server/handler/scan_reports.go Outdated Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_server/model/cloud_node.go Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_server/reporters/scan/scan_reporters.go Outdated Show resolved Hide resolved

noboruma requested changes Sep 19, 2023

View reviewed changes

varunsharma0286 force-pushed the ISSUE-1830-2 branch from 7900621 to b0a459c Compare September 19, 2023 09:37

varunsharma0286 and others added 3 commits September 19, 2023 13:20

ISSUE-1830-2: Adding Stop scan related changes for Vulnerability,

9d04216

Compliance and Cloud compliance scans

Handle cancel status in ingestion path

17c668f

ISSUE-1830-2: Changes after testing

b4f27bc

varunsharma0286 force-pushed the ISSUE-1830-2 branch from 88eeb97 to b4f27bc Compare September 19, 2023 13:24

varunsharma0286 added 2 commits September 19, 2023 13:29

ISSUE-1830-2: Removing unused code

5c5b2ad

ISSUE-1830-2: Removing StopRequested

fc59877

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_server/handler/cloud_node.go Show resolved Hide resolved

noboruma reviewed Sep 19, 2023

View reviewed changes

deepfence_server/handler/cloud_node.go Show resolved Hide resolved

noboruma approved these changes Sep 19, 2023

View reviewed changes

varunsharma0286 merged commit 6d151f7 into main Sep 19, 2023

varunsharma0286 deleted the ISSUE-1830-2 branch September 19, 2023 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-1830: Adding Stop scan related changes for Vulnerability, Compliance and Cloud compliance scans #1585

ISSUE-1830: Adding Stop scan related changes for Vulnerability, Compliance and Cloud compliance scans #1585

varunsharma0286 commented Sep 18, 2023

noboruma Sep 19, 2023

varunsharma0286 Sep 19, 2023

noboruma Sep 19, 2023

varunsharma0286 Sep 19, 2023

varunsharma0286 Sep 19, 2023

noboruma Sep 19, 2023

varunsharma0286 Sep 19, 2023

noboruma Sep 19, 2023

noboruma Sep 19, 2023

noboruma Sep 19, 2023

varunsharma0286 Sep 19, 2023

noboruma left a comment

noboruma Sep 19, 2023

noboruma Sep 19, 2023

noboruma left a comment

	StopRequested: stopRequested,
	StopRequested: scan.Status == utils.SCAN_STATUS_CANCELLING,

ISSUE-1830: Adding Stop scan related changes for Vulnerability, Compliance and Cloud compliance scans #1585

ISSUE-1830: Adding Stop scan related changes for Vulnerability, Compliance and Cloud compliance scans #1585

Conversation

varunsharma0286 commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noboruma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noboruma left a comment

Choose a reason for hiding this comment