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

[PYTHON-2943] : Add socks5 proxy support #2040

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zaif-yuval
Copy link

No description provided.

@zaif-yuval zaif-yuval force-pushed the feature/socks5-proxy-support branch from 5a132e7 to 768bf17 Compare January 1, 2025 12:08
@zaif-yuval zaif-yuval force-pushed the feature/socks5-proxy-support branch from 768bf17 to 4682dac Compare January 1, 2025 12:36
@zaif-yuval zaif-yuval changed the title [PYTHON-2943] : Add SOCKS5 PROXY SUPPORT [PYTHON-2943] : Add socks5 proxy support Jan 1, 2025
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Looks good so far, thank you!

@@ -170,6 +170,15 @@ def _parse_pool_options(
ssl_context, tls_allow_invalid_hostnames = _parse_ssl_options(options)
load_balanced = options.get("loadbalanced")
max_connecting = options.get("maxconnecting", common.MAX_CONNECTING)
if proxy_host := options.get("proxyHost"):
proxy = {
Copy link
Member

Choose a reason for hiding this comment

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

proxy seems like a boolean name, can we please rename to something like proxy_options?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

if Proxy is None:
raise RuntimeError(
"In order to use SOCKS5 proxy, python_socks must be installed. "
"This can be done by re-installing pymongo with `pip install pymongo[socks]`"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be more generic and say pymongo[proxy] in case we switch to a different underlying library in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@blink1073
Copy link
Member

The required tests are specified here.

If you'd like to try running everything locally, I'd suggest something like the following:

git clone https://github.com/mongodb-labs/drivers-evergreen-tools
cd drivers-evergreen-tools
bash .evergreen/setup.sh  #  create a standalone cli for https://github.com/mongodb-labs/drivers-evergreen-tools/blob/master/.evergreen/socks5srv.py
make run-server  # start a server on 27017
./evergreen/socks5srv    # run the cli

Once you have the tests working locally, I can either help walk through the Evergreen Configuration portion, or just make commits to your branch if you prefer.

@zaif-yuval
Copy link
Author

The required tests are specified here.

If you'd like to try running everything locally, I'd suggest something like the following:

git clone https://github.com/mongodb-labs/drivers-evergreen-tools
cd drivers-evergreen-tools
bash .evergreen/setup.sh  #  create a standalone cli for https://github.com/mongodb-labs/drivers-evergreen-tools/blob/master/.evergreen/socks5srv.py
make run-server  # start a server on 27017
./evergreen/socks5srv    # run the cli

Once you have the tests working locally, I can either help walk through the Evergreen Configuration portion, or just make commits to your branch if you prefer.

I'm not sure how to add the tests to work with proxy on your environment
I will try to look into it but if you will be able to assist and push changes into this branch it will be amazing

@zaif-yuval zaif-yuval marked this pull request as ready for review January 6, 2025 20:46
@zaif-yuval zaif-yuval requested a review from blink1073 January 6, 2025 21:12
@blink1073
Copy link
Member

I'm not sure how to add the tests to work with proxy on your environment

I meant for local testing on your machine. If you can get the tests passing locally I can hook up the CI environment part. If you'd rather not, I understand as well.

@zaif-yuval
Copy link
Author

I'm not sure how to add the tests to work with proxy on your environment

I meant for local testing on your machine. If you can get the tests passing locally I can hook up the CI environment part. If you'd rather not, I understand as well.

I would appreciate it, if will be able to hook it into the CI

@blink1073
Copy link
Member

Just to clarify, are you going to try writing the tests and getting them to pass locally?

@zaif-yuval
Copy link
Author

zaif-yuval commented Jan 9, 2025

Just to clarify, are you going to try writing the tests and getting them to pass locally?

I am not getting to and i would love to get your help to implement the tests
I have started to write the cases, I hope you will find it helpful

Let me know if it's working


from __future__ import annotations

import pytest

import pymongo
from pymongo.errors import ConnectionFailure


@pytest.mark.parametrize(
    "conn_str, should_succeed",
    [
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1080&directConnection=true",
            False,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1081&directConnection=true",
            True,
        ),
        ("mongodb://replicaset/?proxyHost=localhost&proxyPort=1080", False),
        ("mongodb://replicaset/?proxyHost=localhost&proxyPort=1081", True),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1080&proxyUsername=nonexistentuser&proxyPassword=badauth&directConnection=true",
            False,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1081&proxyUsername=nonexistentuser&proxyPassword=badauth&directConnection=true",
            True,
        ),
        (
            "mongodb://replicaset/?proxyHost=localhost&proxyPort=1081&proxyUsername=nonexistentuser&proxyPassword=badauth",
            True,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1080&proxyUsername=username&proxyPassword=p4ssw0rd&directConnection=true",
            True,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1081&directConnection=true",
            True,
        ),
        (
            "mongodb://replicaset/?proxyHost=localhost&proxyPort=1080&proxyUsername=username&proxyPassword=p4ssw0rd",
            True,
        ),
        ("mongodb://replicaset/?proxyHost=localhost&proxyPort=1081", True),
    ],
)
def test_socks5_proxy_support(conn_str, should_succeed):
    client = pymongo.MongoClient(conn_str, serverSelectionTimeoutMS=1000)
    try:
        client.admin.command("hello")
        assert should_succeed, f"Connection should have failed: {conn_str}"
    except ConnectionFailure:
        assert not should_succeed, f"Connection should have succeeded: {conn_str}"
    finally:
        client.close()


@pytest.mark.parametrize(
    "conn_str, should_succeed",
    [
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1080&directConnection=true",
            False,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1081&directConnection=true",
            True,
        ),
        ("mongodb://replicaset/?proxyHost=localhost&proxyPort=1080", False),
        ("mongodb://replicaset/?proxyHost=localhost&proxyPort=1081", True),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1080&proxyUsername=nonexistentuser&proxyPassword=badauth&directConnection=true",
            False,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1081&proxyUsername=nonexistentuser&proxyPassword=badauth&directConnection=true",
            True,
        ),
        (
            "mongodb://replicaset/?proxyHost=localhost&proxyPort=1081&proxyUsername=nonexistentuser&proxyPassword=badauth",
            True,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1080&proxyUsername=username&proxyPassword=p4ssw0rd&directConnection=true",
            True,
        ),
        (
            "mongodb://localhost:12345/?proxyHost=localhost&proxyPort=1081&directConnection=true",
            True,
        ),
        (
            "mongodb://replicaset/?proxyHost=localhost&proxyPort=1080&proxyUsername=username&proxyPassword=p4ssw0rd",
            True,
        ),
        ("mongodb://replicaset/?proxyHost=localhost&proxyPort=1081", True),
    ],
)
def test_socks5_proxy_options(conn_str, should_succeed):
    client = pymongo.MongoClient(conn_str, serverSelectionTimeoutMS=1000)
    try:
        client.admin.command("hello")
        assert should_succeed, f"Connection should have failed: {conn_str}"
    except ConnectionFailure:
        assert not should_succeed, f"Connection should have succeeded: {conn_str}"
    finally:
        client.close()


@blink1073
Copy link
Member

Understood, thank you! I won't be able to get to this until at least next week.

@zaif-yuval
Copy link
Author

Understood, thank you! I won't be able to get to this until at least next week.

If you will be able to do it next week it will be amazing :-)

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.

2 participants