fix: Bind metrics, REST registry, ui, and lineage servers dual-stack - #6886
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6886 +/- ##
==========================================
+ Coverage 48.06% 48.22% +0.16%
==========================================
Files 427 427
Lines 53591 53665 +74
Branches 7799 7808 +9
==========================================
+ Hits 25758 25881 +123
+ Misses 25980 25924 -56
- Partials 1853 1860 +7
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
a4d478f to
2848771
Compare
ntkathole
left a comment
ntkathole
left a comment
There was a problem hiding this comment.
Thanks for the thorough PR — well-documented problem and solid test coverage (mutation-aware tests are a nice touch). A few comments below.
| sock.bind(address) | ||
| sock.listen(socket.SOMAXCONN) | ||
| sock.setblocking(False) | ||
| return sock |
There was a problem hiding this comment.
Socket leak: if bind() or listen() throws (e.g. port already in use), the socket is never closed. Wrap the post-creation steps:
try:
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
sock.bind(address)
sock.listen(socket.SOMAXCONN)
sock.setblocking(False)
except:
sock.close()
raiseThere was a problem hiding this comment.
Try-except pattern adopted
| @@ -331,21 +331,17 @@ def start_server( | |||
|
|
|||
| _sync_protected_project_tag(self.store) | |||
|
|
|||
There was a problem hiding this comment.
Unlike ui_server and lineage_server (which gate on host == "::"), this unconditionally switches from the old host="0.0.0.0" to dual-stack. On dual-stack hosts this changes the listen address from 0.0.0.0 to [::], which could conflict with firewall rules or existing port binds. Is the unconditional switch intentional, or should this also be opt-in to match the other two servers?
There was a problem hiding this comment.
It was intentional, but it can default to '0.0.0.0' as before, as the follow-up PR can expose an override on the CRD.
There was a problem hiding this comment.
Here is an example of the override pattern on the follow up PR: https://github.com/feast-dev/feast/pull/6887/changes#diff-9a160662c77106b5759918b6ee5293ab38814aca9c0eb035199c5b4397d1cfe4R3644
| sock.setblocking(False) | ||
| return sock | ||
|
|
||
|
|
There was a problem hiding this comment.
setblocking(False) bakes in an asyncio assumption. The function name is generic, but a caller expecting a blocking socket would fail silently. Consider either documenting this in the docstring or adding a blocking=False parameter.
There was a problem hiding this comment.
Added the blocking parameter to the func, defaulting to False, and added it to the docstring.
| """ | ||
| try: | ||
| with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as sock: | ||
| sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) |
There was a problem hiding this comment.
Nit: IPv6 support can't change during a process lifetime — caching with @functools.lru_cache() would make that intent explicit and skip the throwaway socket on repeated calls.
There was a problem hiding this comment.
Added @functools.lru_cache() decorator to the function.
| try: | ||
| with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as sock: | ||
| sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) | ||
| sock.bind(("::", 0)) |
| if dual_stack: | ||
| sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) | ||
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| sock.bind(address) |
The Prometheus metrics server and the REST registry server both hardcode
IPv4-only binds ("" and "0.0.0.0"), so neither is reachable on IPv6-only
or dual-stack clusters, unlike the gRPC registry server which already
binds "[::]" unconditionally. Add an IPv6 probe (feast.utils._ipv6_available)
and bind both servers to the IPv6 wildcard when available, falling back to
IPv4-only on hosts without IPv6 support (e.g. kernels booted with
ipv6.disable=1, where socket(AF_INET6) raises EAFNOSUPPORT).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
Add type hints to _make_metrics_httpd per AGENTS.md, and hoist its stdlib wsgiref/socket imports to module level since there's no import-cycle risk. Also restore a blank line in test_metrics.py that a mis-resolved ruff (stale 0.6.9 from PATH, not the pinned 0.16.0) stripped during an earlier commit's pre-commit hook run. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
uvicorn's host="::" binds IPv6-only: asyncio's loop.create_server() sets IPV6_V6ONLY=1 on any socket it creates itself from a host string, which would silently drop IPv4 clients on every host, not just IPv6-less ones. Bind the socket ourselves (IPV6_V6ONLY=0, falling back to IPv4-only when the host has no IPv6) and hand it to uvicorn via Server.run(sockets=...), the same pattern used for socket-activated Gunicorn workers. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
start_server (ui_server.py) and start_lineage_server (lineage_server.py) both call uvicorn.run(app, host=host, port=port) directly. A plain host="::" there binds IPv6-only -- asyncio's loop.create_server sets IPV6_V6ONLY=1 on any socket it creates itself from a host string -- which silently drops IPv4 clients on every host, not just IPv6-less ones. This is the same bug the metrics and REST registry servers had, fixed separately on fix/sdk-dual-stack-binds. Add feast.utils._make_dual_stack_socket(port), a shared version of _make_rest_socket's pre-bound-socket approach, and route both servers through it when host == "::": build the socket with IPV6_V6ONLY cleared and hand it to uvicorn.Server(config).run(sockets=[sock]) instead of uvicorn.run(host=...). Any other host keeps today's plain uvicorn.run behavior unchanged. This closes the gap the Operator's dualStack option (feat/operator-dual-stack) depends on: without this fix, enabling dualStack for the ui/lineage services would have silently regressed their IPv4 reachability instead of adding IPv6. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
_make_rest_socket and the new _make_dual_stack_socket had identical bodies. Delete _make_rest_socket and have rest_registry_server.py call the shared feast.utils helper instead, so there is one implementation for all three uvicorn-based dual-stack servers. Moves the real (port-0) end-to-end socket reachability test from test_api_rest_registry_server.py to test_utils.py, targeting the shared helper directly -- the ui and lineage code paths now get real IPv4/IPv6 reachability coverage from the same test, not just mocks. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
An unguarded bind()/listen() leaked the socket fd on failure; wrap the setup in try/except so it closes before re-raising. Also cache the IPv6 support probe with lru_cache since it can't change at runtime. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
RestRegistryServer.start_server() gains a host param, defaulting to "::" (dual-stack, unchanged behavior). Any other value falls back to plain uvicorn.run(host=host), letting deployments opt out of dual-stack binding. Threaded through FeatureStore.serve_registry() and a new --host/-h CLI flag on serve_registry. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
setblocking(False) baked in an asyncio assumption invisible from the function name, so a caller expecting a blocking socket would fail silently. Default stays False for the existing asyncio-based callers. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
4b0e7de to
202fde8
Compare
4 participants
What this PR does / why we need it:
Four servers bind IPv4-only or would go IPv6-only under a naive dual-stack fix, unlike the gRPC registry server, which already binds "[::]" unconditionally. On IPv6-only or dual-stack clusters, this makes the affected servers unreachable from one address family or the other (observed as Envoy 503 delayed_connect_error:_Connection_refused for the metrics server).
This adds
_ipv6_available(), a probe that binds a throwaway IPv6 wildcard socket to detect real IPv6 support, plus_make_dual_stack_socket(port)infeast/utils.py, a shared pre-bound-socket builder used by all three uvicorn-based servers below (one implementation, not three copies):_make_metrics_httpd()builds the wsgiref metrics httpd as aWSGIServersubclass bound to"::"withIPV6_V6ONLYcleared when IPv6 is available, or IPv4-only (make_server("", ...), unchanged behavior) otherwise. This one doesn't use_make_dual_stack_socket-- wsgiref's ownWSGIServerneeds the subclass approach instead of a pre-bound socket.RestRegistryServer.start_server()builds the REST registry server's listening socket via
_make_dual_stack_socket, then hands it to uvicorn viaServer(config).run(sockets=[sock])instead ofuvicorn.run(host=...).ui_server.start_server()andlineage_server.start_lineage_server(): when the Operator rendershost="::"(see PR B,feat/operator-dual-stack), route through the same_make_dual_stack_socket+Server(sockets=[sock])pattern; any other host keeps today's plainuvicorn.run(host=host)unchanged.The socket-based indirection is required, not stylistic, for all three uvicorn-based servers (REST registry, ui, lineage): uvicorn's own
host="::"binds IPv6-only, because asyncio'sloop.create_server()setsIPV6_V6ONLY=1on any socket it creates itself from a host string. Passing a pre-bound socket withIPV6_V6ONLY=0avoids that and gets real dual-stack behavior.All four fall back to the existing IPv4-only bind on hosts with no IPv6 (e.g. kernels booted with
ipv6.disable=1, wheresocket(AF_INET6)raisesEAFNOSUPPORT) -- the original issue's "still works on IPv4-only hosts" claim doesn't hold without this fallback.Which issue(s) this PR fixes:
Part of #6862, with a following PR targeting the operator.
Checks
git commit -s)Testing Strategy
Misc
Manually verified end-to-end beyond the unit tests, against a local test repo, for every one of the four affected services, both that dual-stack reachability works with the bind form each actually needs, and that the wrong form for each fails the way PR B's Operator commit assumes it does:
feast serve(gunicorn)-h "[::]"-h "::"feast serve_offline(Arrow Flight)-h "[::]"-h "::"pyarrow.lib.ArrowInvalid: cannot parse URIfeast ui(uvicorn, new path)-h "::"-h "[::]"[Errno 8] nodename nor servname providedfeast serve_lineage(uvicorn, new path)-h "::"