-
Notifications
You must be signed in to change notification settings - Fork 180
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
Support socket address family VSOCK #409
base: main
Are you sure you want to change the base?
Changes from all commits
d165e12
f145ac9
a9e21d2
24d70b0
cc6a369
9273cde
f84f877
ee6f20b
f5f3358
e07b315
c6df686
70c7f0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,4 +149,6 @@ Contributors | |
|
||
- Jonathan Vanasco, 2022-11-15 | ||
|
||
- Ananth Bhaskararaman, 2023-04-05 | ||
|
||
- Simon King, 2024-11-12 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ | |
import socket | ||
import warnings | ||
|
||
from .compat import HAS_IPV6, WIN | ||
from .compat import HAS_IPV6, VSOCK, WIN | ||
from .proxy_headers import PROXY_HEADERS | ||
|
||
truthy = frozenset(("t", "true", "y", "yes", "on", "1")) | ||
|
@@ -81,6 +81,10 @@ def str_iftruthy(s): | |
return str(s) if s else None | ||
|
||
|
||
def int_iftruthy(s): | ||
return int(s) if s else None | ||
|
||
|
||
def as_socket_list(sockets): | ||
"""Checks if the elements in the list are of type socket and | ||
removes them if not.""" | ||
|
@@ -130,6 +134,8 @@ class Adjustments: | |
("asyncore_use_poll", asbool), | ||
("unix_socket", str), | ||
("unix_socket_perms", asoctal), | ||
("vsock_socket_cid", int_iftruthy), | ||
("vsock_socket_port", int_iftruthy), | ||
("sockets", as_socket_list), | ||
("channel_request_lookahead", int), | ||
("server_name", str), | ||
|
@@ -254,6 +260,10 @@ class Adjustments: | |
# Path to a Unix domain socket to use. | ||
unix_socket_perms = 0o600 | ||
|
||
# The CID and port to use for a vsock socket. | ||
vsock_socket_cid = None | ||
vsock_socket_port = None | ||
|
||
# The socket options to set on receiving a connection. It is a list of | ||
# (level, optname, value) tuples. TCP_NODELAY disables the Nagle | ||
# algorithm for writes (Waitress already buffers its writes). | ||
|
@@ -302,12 +312,41 @@ def __init__(self, **kw): | |
if "sockets" in kw and "unix_socket" in kw: | ||
raise ValueError("unix_socket may not be set if sockets is set") | ||
|
||
if "sockets" in kw and ("vsock_socket_cid" in kw or "vsock_socket_port" in kw): | ||
raise ValueError( | ||
"vsock_socket_cid or vsock_socket_port may not be set if sockets is set" | ||
) | ||
|
||
if "unix_socket" in kw and ("host" in kw or "port" in kw): | ||
raise ValueError("unix_socket may not be set if host or port is set") | ||
|
||
if "unix_socket" in kw and "listen" in kw: | ||
raise ValueError("unix_socket may not be set if listen is set") | ||
|
||
if ("vsock_socket_cid" in kw or "vsock_socket_port" in kw) and not VSOCK: | ||
raise ValueError( | ||
"vsock_socket_cid and vsock_socket_port are not supported on this platform" | ||
) | ||
|
||
if ("vsock_socket_cid" in kw or "vsock_socket_port" in kw) and ( | ||
"host" in kw or "port" in kw | ||
): | ||
raise ValueError( | ||
"vsock_socket_cid or vsock_socket_port may not be set if host or port is set" | ||
) | ||
|
||
if ("vsock_socket_cid" in kw or "vsock_socket_port" in kw) and "listen" in kw: | ||
raise ValueError( | ||
"vsock_socket_cid or vsock_socket_port may not be set if listen is set" | ||
) | ||
|
||
if ( | ||
"vsock_socket_cid" in kw or "vsock_socket_port" in kw | ||
) and "unix_socket" in kw: | ||
raise ValueError( | ||
"vsock_socket_cid or vsock_socket_port may not be set if unix_socket is set" | ||
) | ||
|
||
if "send_bytes" in kw: | ||
warnings.warn( | ||
"send_bytes will be removed in a future release", DeprecationWarning | ||
|
@@ -353,10 +392,10 @@ def __init__(self, **kw): | |
# Try turning the port into an integer | ||
port = int(port) | ||
|
||
except Exception: | ||
except Exception as exc: | ||
raise ValueError( | ||
"Windows does not support service names instead of port numbers" | ||
) | ||
) from exc | ||
|
||
try: | ||
if "[" in host and "]" in host: # pragma: nocover | ||
|
@@ -391,20 +430,20 @@ def __init__(self, **kw): | |
wanted_sockets.append((family, socktype, proto, sockaddr)) | ||
hp_pairs.append((sockaddr[0].split("%", 1)[0], sockaddr[1])) | ||
|
||
except Exception: | ||
raise ValueError("Invalid host/port specified.") | ||
except Exception as exc: | ||
raise ValueError("Invalid host/port specified.") from exc | ||
|
||
if self.trusted_proxy_count is not None and self.trusted_proxy is None: | ||
raise ValueError( | ||
"trusted_proxy_count has no meaning without setting " "trusted_proxy" | ||
"trusted_proxy_count has no meaning without setting trusted_proxy" | ||
) | ||
|
||
elif self.trusted_proxy_count is None: | ||
if self.trusted_proxy_count is None: | ||
self.trusted_proxy_count = 1 | ||
|
||
if self.trusted_proxy_headers and self.trusted_proxy is None: | ||
raise ValueError( | ||
"trusted_proxy_headers has no meaning without setting " "trusted_proxy" | ||
"trusted_proxy_headers has no meaning without setting trusted_proxy" | ||
) | ||
|
||
if self.trusted_proxy_headers: | ||
|
@@ -415,9 +454,9 @@ def __init__(self, **kw): | |
unknown_values = self.trusted_proxy_headers - KNOWN_PROXY_HEADERS | ||
if unknown_values: | ||
raise ValueError( | ||
"Received unknown trusted_proxy_headers value (%s) expected one " | ||
"of %s" | ||
% (", ".join(unknown_values), ", ".join(KNOWN_PROXY_HEADERS)) | ||
"Received unknown trusted_proxy_headers value " | ||
f"({', '.join(unknown_values)}) expected one " | ||
f"of {', '.join(KNOWN_PROXY_HEADERS)}" | ||
) | ||
|
||
if ( | ||
|
@@ -486,23 +525,22 @@ def parse_args(cls, argv): | |
|
||
@classmethod | ||
def check_sockets(cls, sockets): | ||
has_unix_socket = False | ||
has_inet_socket = False | ||
has_unsupported_socket = False | ||
supported_families = [socket.AF_INET, socket.AF_INET6] | ||
if hasattr(socket, "AF_UNIX"): | ||
supported_families.append(socket.AF_UNIX) | ||
if hasattr(socket, "AF_VSOCK"): | ||
supported_families.append(socket.AF_VSOCK) | ||
|
||
inet_families = (socket.AF_INET, socket.AF_INET6) | ||
family = None | ||
for sock in sockets: | ||
if ( | ||
sock.family == socket.AF_INET or sock.family == socket.AF_INET6 | ||
) and sock.type == socket.SOCK_STREAM: | ||
has_inet_socket = True | ||
elif ( | ||
hasattr(socket, "AF_UNIX") | ||
and sock.family == socket.AF_UNIX | ||
and sock.type == socket.SOCK_STREAM | ||
): | ||
has_unix_socket = True | ||
else: | ||
has_unsupported_socket = True | ||
if has_unix_socket and has_inet_socket: | ||
raise ValueError("Internet and UNIX sockets may not be mixed.") | ||
if has_unsupported_socket: | ||
raise ValueError("Only Internet or UNIX stream sockets may be used.") | ||
if sock.type != socket.SOCK_STREAM or sock.family not in supported_families: | ||
raise ValueError( | ||
"Only Internet, UNIX, or VSOCK stream sockets may be used." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this restriction exist btw? Is there an issue listening to multiple types of sockets simultaneously? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a fair question - I don't know what the history is here but it doesn't feel like anything would fundamentally break by using multiple different socket types. Things that come to mind are any WSGI environ settings that should be dynamic that aren't right now based on the source connection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each new socket type added later on will increase the number of combined checks needed. I could try removing this restriction and seeing if anything breaks. Removing this restriction would also mean better systemd socket activation support. Systemd can pass multiple different types of sockets to apps quite easily and waitress should support that too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The combinatorial explosion can be prevented by just a better loop and more complicated error message generation but again that’s assuming there’s a technical reason for keeping the checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Technically there is no basis for this check. The only question is if any waitress code assumes so or depends on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of it is that there is a different expectation with being able to receive from a UNIX socket (local, defined by file system permissions) and a TCP/IP socket. There is no remote peer in the case of a UNIX socket that has a port number, certain values can't be automatically provided instead they are guessed at (such as the Host header if none is provided by the remote client over a UNIX socket). Keeping the socket types separate may not be strictly required, it's not something that has been tested or validated and adding the additional testing may not be worth it. If you want to listen on both a UNIX socket and TCP/IP then that can be two different instances of waitress (or a single front-end proxy that does support that). |
||
) | ||
if family is None: | ||
family = sock.family | ||
elif family in inet_families and sock.family in inet_families: | ||
pass | ||
elif family != sock.family: | ||
raise ValueError("All sockets must belong to the same family.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I do not believe that the
from exc
is necessary in these cases you have modified - it is an explicit form of what already happens when raising a new exception from within an except block.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I setup a new linter (not sure which one) and it suggested I try this.