CNF-24706: Make scan timeouts configurable via CLI flags - #74
Conversation
Add --scan-timeout-per-target and --connect-timeout flags so users can tune the batch scan timeout formula and testssl.sh connect/openssl timeouts. Introduce ScanTimeouts struct with DefaultScanTimeouts for centralized defaults. Centralize pod exec timeout as a package constant.
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CNF-24706 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sebrandon1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
smith-xyz
left a comment
smith-xyz
left a comment
There was a problem hiding this comment.
overall a good add - just one rec to keep the current defaults that were embedded in the exec
| } | ||
|
|
||
| var DefaultScanTimeouts = ScanTimeouts{ | ||
| PerTargetSeconds: 90, |
There was a problem hiding this comment.
we should ensure that the jobs in prow aren't going to abruptly fail. Might be best to keep per target at 5 as well as a default to match the current behavior.
Just some background on this. Everytime we spin up a testssl pid its a blocking event for go. Ended up adding in the low timeouts because we would discover a good number of ips/ports that would timeout when we attempted tls. We were hitting 4+ hour runs and that wasn't feasible. testssl does a deep dive so it is also costly. It could be more performant but just haven't had a chance to look at it again.
I think for now probably just best to keep the default and allow others to override for their needs.
There was a problem hiding this comment.
Good context on the testssl timeout history, thanks for sharing that.
The defaults here match the existing hardcoded values exactly: PerTargetSeconds: 90 (from the len(targets)*90+120 formula) and ConnectTimeout: 5 (from the --connect-timeout 5 / --openssl-timeout 5 args). So current Prow jobs will behave identically without any flag overrides.
The flags are opt-in only. Users who need longer timeouts for specific environments can pass --connect-timeout 10 etc., but the out-of-the-box behavior is unchanged.
There was a problem hiding this comment.
oh nice! thanks for keeping me honest I misread that "connectTimeoutStr" was used for both the flags down with testssl. Then I think this should be good to go
|
/lgtm |
openshift-ci
Bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sebrandon1, smith-xyz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
The batch scan timeout formula and testssl.sh connect/openssl timeouts were hardcoded, preventing users from tuning for large clusters or slow networks.
--scan-timeout-per-target(default 90s) — controls per-target component of the batch timeout formula--connect-timeout(default 5s) — controls testssl.sh--connect-timeoutand--openssl-timeoutTest plan
make lint— 0 issuesgo test ./...— all pass--helpshows new flags with correct defaults--connect-timeout 0rejected with exit code 2--log-level debug)