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

Jump to bottom

fix(terraform): handle hanging Terraform file parsing with process-based timeout #6871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(terraform): handle hanging Terraform file parsing with process-based timeout #6871

wants to merge 4 commits into from

Conversation

MS99-9
Copy link

@MS99-9 MS99-9 commented Nov 24, 2024 •
edited
Loading

User description

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

This PR introduces a ProcessTimeout class to handle cases where Terraform file parsing in Checkov hangs indefinitely, particularly when using the -d flag to scan directories. The ProcessTimeout class uses subprocesses to enforce timeouts for blocking operations, complementing the existing SignalTimeout and ThreadingTimeout mechanisms. Additionally, updates were made to __parse_with_timeout to address race conditions.

Changes include:

  1. Added ProcessTimeout:
  • Introduced a new timeout mechanism that uses subprocesses to execute potentially blocking operations, allowing them to be forcibly terminated if they exceed the specified timeout duration.
  • Serves as a fallback for non-main threads and blocking operations where SignalTimeout (Unix signals) and ThreadingTimeout (used on Windows) are ineffective.
  1. Updated __parse_with_timeout:
  • Integrated ProcessTimeout as an additional timeout mechanism to handle cases where file parsing would hang indefinitely due to blocking operations or syntax errors in Terraform files.
  • The issue occurred because SignalTimeout only works in the main thread and ThreadingTimeout cannot interrupt blocking operations due to Python’s Global Interpreter Lock. As a result, Checkov was unable to terminate the parsing process when using the -d flag.
  • Addressed potential race conditions by ensuring the setup_interrupt process for ProcessTimeout completes before execution proceeds.

Testing

Before the fix, running Checkov with a syntax error in the Terraform file would result in Checkov hanging indefinitely. After the fix, Checkov terminates the process and logs the parsing error properly. Example output:

      _               _              
   ___| |__   ___  ___| | _______   __
  / __| '_ \ / _ \/ __| |/ / _ \ \ / /
 | (__| | | |  __/ (__|   < (_) \ V / 
  \___|_| |_|\___|\___|_|\_\___/ \_/  
                                      
By Prisma Cloud | version: 3.2.313 

terraform scan results:

Passed checks: 0, Failed checks: 0, Skipped checks: 0, Parsing errors: 1

Error parsing file <failed_file_path>

Running Pytests and coverage modules pipenv run python -m coverage run -m pytest tests:

=========================================== test session starts ============================================
platform darwin -- Python 3.10.14, pytest-7.4.4, pluggy-1.5.0
rootdir: /Users/mohamedshalaby/dev/ms99-9/checkov
configfile: pyproject.toml
plugins: asyncio-0.23.8, cov-5.0.0, time-machine-2.15.0, mock-3.14.0, xdist-3.6.1
asyncio: mode=strict
2 workers [4689 items]  
.................................................................................................... [  2%]
.................................................................................................... [  4%]
.................................................................................................... [  6%]
.................................................................................................... [  8%]
.........................................s..........................s............................... [ 10%]
.................................................................................................... [ 12%]
.......................................s...................s......s................................. [ 14%]
.................................................................................................... [ 17%]
.................................................................................................... [ 19%]
.................................................................................................... [ 21%]
.................................................................................................... [ 23%]
.................................................................................................... [ 25%]
.................................................................................................... [ 27%]
.................................................................................................... [ 29%]
.................................................................................................... [ 31%]
.................................................................................................... [ 34%]
.................................................................................................... [ 36%]
.................................................................................................... [ 38%]
.................................................................................................... [ 40%]
.................................................................................................... [ 42%]
.............................................................................................sssssss [ 44%]
.................................................................................................... [ 46%]
.................................................................................................... [ 49%]
..............................................................ss.................................... [ 51%]
.................................................................................................... [ 53%]
.............................................ssss............sss.................................... [ 55%]
.................................................................................................... [ 57%]
.................................................................................................... [ 59%]
...s................................................................................................ [ 61%]
.................................................................................................... [ 63%]
.................................................................................................... [ 66%]
.............................ssss................................................................... [ 68%]
.................................................................................................... [ 70%]
.................................................................................................... [ 72%]
.................................................................................................... [ 74%]
.................................................................................................... [ 76%]
.................................................................................................... [ 78%]
.................................................................................................... [ 81%]
....................................s............................................................... [ 83%]
.................................................................................................... [ 85%]
.................................................................................................... [ 87%]
.................................................................................................... [ 89%]
.................................................................................................... [ 91%]
.................................................................................................... [ 93%]
.................................................................................................... [ 95%]
.................................................................................................... [ 98%]
.........................................................................................            [100%]
======================== 4662 passed, 27 skipped, 11 warnings in 885.55s (0:14:45) =========================

Files created while testing

Fixes #6762

Questions

  1. Should I increment the version in checkov/version.py?
  2. I didn't find any tests for the checkov/common/util/stopit classes, should I create test files for the timeout classes? maybe another PR for this [enhancement]
  3. Can you please check if the introduced changes don't alter the logic for parsing Terraform files, I already built the package locally with the changes and tested multiple cases but my concern if I missed any of the edge cases.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes

Generated description

Below is a concise technical summary of the changes proposed in this PR:

Introduces a new ProcessTimeout class to handle cases where Terraform file parsing hangs indefinitely. Updates the __parse_with_timeout function to use ProcessTimeout as a fallback mechanism for non-main threads and blocking operations. This complements the existing SignalTimeout and ThreadingTimeout mechanisms, addressing issues with hanging parsers when using the -d flag to scan directories.

TopicDetails
ParseTimeout Update Enhances __parse_with_timeout function to use ProcessTimeout as a fallback mechanism
Modified files (1)
  • checkov/terraform/tf_parser.py
Latest Contributors(2)
UserCommitDate
rotemavnifix-terraform-Set-time...October 13, 2024
ChanochShaynerfeat-terraform-Add-pro...May 29, 2024
ProcessTimeout Implements a new ProcessTimeout class using multiprocessing to enforce timeouts for blocking operations
Modified files (2)
  • checkov/common/util/stopit/processstop.py
  • checkov/common/util/stopit/__init__.py
Latest Contributors(2)
UserCommitDate
gruebelchore-remove-unneeded-...February 23, 2023
omryMenfeat-secrets-add-timeo...February 22, 2023
This pull request is reviewed by Baz. Join @MS99-9 and the rest of your team on (Baz).

@MS99-9
fix: add process-based timeout for Terraform file parsing
ff162d7 
Signed-off-by: Mohamed Medhat Mohamed Ibrahim Shalaby <[email protected]>
@MS99-9
Copy link
Author

MS99-9 commented Dec 18, 2024 •
edited
Loading

Is there anything missing in the PR that I need to provide in order for it to get removed? please let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chekov hangs during scanning terraform module with syntax typo
1 participant
@MS99-9