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

feat: Add support for NO_PROXY for websocket connections #16538

Merged
merged 4 commits into from
Dec 31, 2024

Conversation

jbw-vtl
Copy link
Contributor

@jbw-vtl jbw-vtl commented Dec 29, 2024

In #16326 the team added support for HTTP_PROXY & HTTPS_PROXY for the websocket connection used by the agent to submit events (such as task runs).
However in some environments these proxies are necessary to route connection for the flow itself running (i.e. to connect to external in an air gapped environment).
Having the events routed through the proxy might mean they cannot reach the server.

This PR adds basic support for the NO_PROXY variable for the websocket connection, allowing to ignore the proxy for the server connection.

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • [ ] If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

This stemmed from the discussion around #15153 and closes #16537

@github-actions github-actions bot added the bug Something isn't working label Dec 29, 2024
Copy link

codspeed-hq bot commented Dec 29, 2024

CodSpeed Performance Report

Merging #16538 will not alter performance

Comparing jbw-vtl:main (195aba2) with main (441f721)

Summary

✅ 3 untouched benchmarks

@jbw-vtl
Copy link
Contributor Author

jbw-vtl commented Dec 29, 2024

Adjusted the used proxy to be http as even a HTTPS_PROXY needs to be http, the tests without mocking were failing with the below

def parse_proxy_url(url: str) -> Tuple[ProxyType, str, int, Optional[str], Optional[str]]:
        parsed = urlparse(url)
    
        scheme = parsed.scheme
        if scheme == 'socks5':
            proxy_type = ProxyType.SOCKS5
        elif scheme == 'socks4':
            proxy_type = ProxyType.SOCKS4
        elif scheme == 'http':
            proxy_type = ProxyType.HTTP
        else:
>           raise ValueError(f'Invalid scheme component: {scheme}')  # pragma: no cover
E           ValueError: Invalid scheme component: https

@@ -84,6 +84,20 @@ def events_out_socket_from_api_url(url: str):
return http_to_ws(url) + "/events/out"


def _host_matches_no_proxy(host: str) -> bool:
"""Checks whether a given host matches NO_PROXY configuration.
NO_PROXY entries with a leading dot are treated as domain suffixes, while all others are treated as exact matches.
Copy link
Contributor

@jakekaplan jakekaplan Dec 30, 2024

Choose a reason for hiding this comment

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

I'm inclined to use from urllib.request import proxy_bypass here to avoid having our own implementation if possible.

My reading of NO_PROXY is that it's either an exact match or substring (as opposed to just a suffix), which would conflict with this as written but would be supported by proxy_bypass.

I'm not super familiar, did you see something that suggested it should work differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was looking for precisely this, also not super familiar here.
This looks indeed perfect, swapped it out and working as expected.
Pushed a change to swap out just now

Copy link
Contributor

@jakekaplan jakekaplan left a comment

Choose a reason for hiding this comment

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

@jbw-vtl will need a review from a core maintainer to merge but this LGTM. Thanks for contributing the fix!

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

thank you @jbw-vtl !

@zzstoatzz zzstoatzz merged commit 7e6f883 into PrefectHQ:main Dec 31, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Websocket http proxies do not support NO_PROXY
3 participants