CASSPYTHON-13: Remove eventlet, gevent and twisted event loops - #969
CASSPYTHON-13: Remove eventlet, gevent and twisted event loops#969mykaul wants to merge 2 commits into
Conversation
patch by Brett Abamonte; reviewed by Bret McGuire (cherry picked from commit 8b39688)
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change removes Eventlet, gevent, and Twisted reactor integrations from runtime selection, benchmarks, tests, and documentation. Thread pools and queues now use standard-library implementations. Cloud configuration and SSL tests now use built-in Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
docs/installation.rst-172-174 (1)
172-174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument
pyasyncoreinstallation for Python 3.12+.
pip install scylla-driverdoes not installpyasyncore. Add a conditional dependency or documentpython -m pip install pyasyncorebeside the driver installation command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/installation.rst` around lines 172 - 174, Update the Python 3.12+ installation guidance in the asyncore paragraph to explicitly instruct users to install pyasyncore separately, using the existing driver installation instructions and a command such as python -m pip install pyasyncore; keep the note scoped to versions where asyncore is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cassandra/datastax/cloud/__init__.py`:
- Around line 97-102: Harden the extraction flow in the secure-bundle handling
code around ZipFile.extractall by resolving every archive member destination and
rejecting any path outside tmp_dir before extraction. Preserve normal extraction
and parse_cloud_config behavior for safe entries, and add a regression test
covering a traversal archive member.
---
Other comments:
In `@docs/installation.rst`:
- Around line 172-174: Update the Python 3.12+ installation guidance in the
asyncore paragraph to explicitly instruct users to install pyasyncore
separately, using the existing driver installation instructions and a command
such as python -m pip install pyasyncore; keep the note scoped to versions where
asyncore is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 380ff746-7fd2-4186-a4b6-7db1ee384a86
📒 Files selected for processing (26)
benchmarks/base.pycassandra/cluster.pycassandra/connection.pycassandra/datastax/cloud/__init__.pycassandra/datastax/insights/reporter.pycassandra/io/eventletreactor.pycassandra/io/geventreactor.pycassandra/io/twistedreactor.pydocs/api/cassandra/io/eventletreactor.rstdocs/api/cassandra/io/geventreactor.rstdocs/api/cassandra/io/twistedreactor.rstdocs/api/index.rstdocs/installation.rstdocs/security.rsttests/__init__.pytests/integration/long/test_ssl.pytests/integration/standard/test_connection.pytests/unit/io/eventlet_utils.pytests/unit/io/gevent_utils.pytests/unit/io/test_asyncioreactor.pytests/unit/io/test_asyncorereactor.pytests/unit/io/test_eventletreactor.pytests/unit/io/test_geventreactor.pytests/unit/io/test_libevreactor.pytests/unit/io/test_twistedreactor.pytests/unit/io/utils.py
💤 Files with no reviewable changes (16)
- docs/api/cassandra/io/eventletreactor.rst
- docs/api/cassandra/io/geventreactor.rst
- tests/unit/io/test_eventletreactor.py
- tests/unit/io/utils.py
- benchmarks/base.py
- cassandra/io/twistedreactor.py
- tests/unit/io/test_geventreactor.py
- docs/security.rst
- tests/unit/io/gevent_utils.py
- cassandra/io/eventletreactor.py
- docs/api/index.rst
- tests/unit/io/test_twistedreactor.py
- tests/unit/io/eventlet_utils.py
- tests/integration/standard/test_connection.py
- cassandra/io/geventreactor.py
- docs/api/cassandra/io/twistedreactor.rst
| with ZipFile(secure_bundle) as zipfile: | ||
| base_dir = tempfile.gettempdir() if use_default_tempdir else os.path.dirname(secure_bundle) | ||
| tmp_dir = tempfile.mkdtemp(dir=base_dir) | ||
| try: | ||
| zipfile.extractall(path=tmp_dir) | ||
| return parse_cloud_config(os.path.join(tmp_dir, 'config.json'), cloud_config, create_pyopenssl_context) | ||
| return parse_cloud_config(os.path.join(tmp_dir, 'config.json'), cloud_config) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Block Zip Slip paths before extraction.
extractall() accepts archive members such as ../../target. A crafted secure-connect bundle can write outside tmp_dir with the driver process permissions. Validate each resolved member path is within tmp_dir before extraction. Add a regression test with a traversal entry.
Proposed fix
- zipfile.extractall(path=tmp_dir)
+ root = os.path.realpath(tmp_dir)
+ for member in zipfile.infolist():
+ target = os.path.realpath(os.path.join(root, member.filename))
+ if os.path.commonpath((root, target)) != root:
+ raise ValueError("The secure connect bundle contains an unsafe path.")
+ zipfile.extract(member, root)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with ZipFile(secure_bundle) as zipfile: | |
| base_dir = tempfile.gettempdir() if use_default_tempdir else os.path.dirname(secure_bundle) | |
| tmp_dir = tempfile.mkdtemp(dir=base_dir) | |
| try: | |
| zipfile.extractall(path=tmp_dir) | |
| return parse_cloud_config(os.path.join(tmp_dir, 'config.json'), cloud_config, create_pyopenssl_context) | |
| return parse_cloud_config(os.path.join(tmp_dir, 'config.json'), cloud_config) | |
| with ZipFile(secure_bundle) as zipfile: | |
| base_dir = tempfile.gettempdir() if use_default_tempdir else os.path.dirname(secure_bundle) | |
| tmp_dir = tempfile.mkdtemp(dir=base_dir) | |
| try: | |
| root = os.path.realpath(tmp_dir) | |
| for member in zipfile.infolist(): | |
| target = os.path.realpath(os.path.join(root, member.filename)) | |
| if os.path.commonpath((root, target)) != root: | |
| raise ValueError("The secure connect bundle contains an unsafe path.") | |
| zipfile.extract(member, root) | |
| return parse_cloud_config(os.path.join(tmp_dir, 'config.json'), cloud_config) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 100-100: Calling extractall() on a zipfile.ZipFile or tarfile archive without validating member paths lets a crafted entry (e.g. "../../etc/passwd") write outside the destination directory (Zip Slip). Validate each member resolves inside the target directory, or pass a safe filter (tarfile: filter="data" / tarfile.data_filter).
Context: zipfile.extractall(path=tmp_dir)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(archive-extractall-path-traversal-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cassandra/datastax/cloud/__init__.py` around lines 97 - 102, Harden the
extraction flow in the secure-bundle handling code around ZipFile.extractall by
resolving every archive member destination and rejecting any path outside
tmp_dir before extraction. Preserve normal extraction and parse_cloud_config
behavior for safe entries, and add a regression test covering a traversal
archive member.
Source: Linters/SAST tools
8b54528 to
c8ee0f0
Compare
…hread_pool_executor This code path in cassandra/cluster.py was not part of the original upstream removal since it doesn't exist upstream; it's fork-specific. With eventletreactor deleted, the import always failed and the method always fell back to a plain ThreadPoolExecutor, so drop the dead branch.
c8ee0f0 to
e97d2dd
Compare
2 participants
Summary
asyncioinconn_fns, retainedis_monkey_patched()helpers used by a fork-specific libev shutdown test).EventletConnectionimport inCluster._create_thread_pool_executor— fork-specific code not present upstream, left dead after the reactor module was deleted.Test plan
python3 -m py_compileon all touched filestests/unitsuite run (no C extensions available in sandbox): 697 passed, 99 skipped (skips due to missing libev/cmurmur3/lz4 extensions, unrelated to this change), 0 failures