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

ISSUE-1830: Adding changes for stopping the seceret and malware scans #1316

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

varunsharma0286
Copy link
Contributor

Fixes #
https://github.com/deepfence/enterprise-roadmap/issues/1830
Changes proposed in this pull request:

  • Adding support for stopping the secret scans
  • Adding support for stopping the malware scans

@@ -14,22 +14,37 @@ import (
)

var (
MaxStops = 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

This MaxStops is a bit misleading because it only applies for 1 iteration: Let's imagine we cancel 30 scans, we will do: 5 cancel, next 30 seconds 5 more, next 30 seconds 5 more, without checking if the previous cancel were completed.

Now I think in practice we should be fine because we expect cancels to finish quickly and the request should not use extra resources to complete.. I would rather remove that MaxStops entirely, just to simplify our logic and not mislead us in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we would need to put in a limit to how many we can process in a one go.
Maybe we can raise this to higher number (maybe 50)

Comment on lines +112 to +107
type Entry struct {
Posn int
Data map[string]interface{}
}
Copy link
Collaborator

@noboruma noboruma Jul 13, 2023

Choose a reason for hiding this comment

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

I would rather keep previous code here as this is not solving anything new from what I understand.
The allocation optimization looks fine:

statuses := make([]map[string]interface{}, len(statusBuff))

But the Posn introduction is not solving anything new, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to maintain the ordering of the messages.
Right now we store conflated data in the map, and maps does not guarantee the ordering of the messages.
For instance, if we received the message in below order:
"IN_PROGRESS" -> "CANCELLED" status, we are not guaranteed to process the status in the same order.
Hence we might end up with a record in the neo4j which is in "IN_PROGRESS" status.

@varunsharma0286 varunsharma0286 merged commit c025eca into v2 Jul 14, 2023
@varunsharma0286 varunsharma0286 deleted the ISSUE-1830 branch July 14, 2023 06:41
@varunsharma0286 varunsharma0286 changed the title ISSUE-1830: Adding changes for stopping the seceret and malware scans (Only review, do not merge yet) ISSUE-1830: Adding changes for stopping the seceret and malware scans Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants