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.

Sign up for GitHub

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

snyk: wrap snyk invocation by timeout(1) by default #127

Closed
wants to merge 1 commit into from
Closed

snyk: wrap snyk invocation by timeout(1) by default #127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion py/plugins/snyk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@

FILTER_CMD = f"csgrep '%s' --mode=json --prepend-path-prefix={SNYK_SCAN_DIR}/ > '%s'"

# default value for the maximum amount of time taken by invocation of Snyk (5 hours)
DEFAULT_SNYK_TIMEOUT=18000
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kdudka Hey Kamil. I don't know if because of the workers, the scans are slower. But when I have been testing this, more than 2 hours means it is going to get stuck. Just in case you want to reduce this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was my observation as well when I tested Snyk initially (although the scans were never hanging when I was experimenting with the Snyk client interactively). But now we are increasing the load on the 3rd party scanning service. Theoretically up to 40 Snyk scans can now run in parallel in OpenScanHub. So I wanted to reserve some extra time to avoid failing OSH tasks just because a scan is temporarily queued on the remote service.

The 5 hours timeout is really a workaround to avoid manual intervention in the edge case where the Snyk client keeps hanging forever. This should not happen and the vendor should fix the client to terminate itself in such cases because the client has more information to decide whether it makes sense to wait longer or not. For example, if the remote scanning process crashes, it would be wasteful to wait even for 2 hours before we terminate the client. However, if the scan is just temporarily queued on the remote service, then waiting for 3 hours might be better for us than failing the OSH task and having to manually investigate what happened.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool then. I have used the same timout for cspodman



class PluginProps:
def __init__(self):
Expand Down Expand Up @@ -67,6 +70,11 @@ def init_parser(self, parser):
"--snyk-refresh", action="store_true",
help="force download of snyk binary executable")

parser.add_argument(
"--snyk-timeout", type=int, default=DEFAULT_SNYK_TIMEOUT,
help="maximum amount of time taken by invocation of Snyk [s]")


def handle_args(self, parser, args, props):
if not self.enabled:
return
Expand Down Expand Up @@ -147,9 +155,15 @@ def scan_hook(results, mock, props):
results.error("failed to copy snyk authentication token", ec=ec)
return ec

# run snyk code
# command to run snyk code
cmd = "%s code test -d %s --sarif-file-output=%s >/dev/null 2>%s" \
% (self.snyk_bin, SNYK_SCAN_DIR, SNYK_OUTPUT, SNYK_LOG)

if args.snyk_timeout:
# wrap snyk invocation by timeout(1)
cmd = f"/usr/bin/timeout {args.snyk_timeout} {cmd}"

# run snyk code
ec = mock.exec_chroot_cmd(cmd)

# remove authentication token from the chroot
Expand Down