Conversation
Add a DualStack field to ServerConfigs that binds server processes to the IPv6 wildcard address instead of 0.0.0.0, so they also accept IPv4 clients on dual-stack or IPv6-only clusters. Online and offline servers use the bracketed [::] form required by gunicorn and Arrow Flight; ui, lineage, and registry keep their existing behavior or use the bare :: form uvicorn expects. The registry server always binds dual-stack and ignores this setting. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
Mutation testing found survivors in withBindHost's loop bounds: an -h flag as the very last argument (no value to replace) and one as the first argument were both untested edge cases. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6887 +/- ##
=======================================
Coverage 48.50% 48.50%
=======================================
Files 427 427
Lines 53755 53755
Branches 7827 7827
=======================================
Hits 26076 26076
Misses 25813 25813
Partials 1866 1866
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The registry's REST server binds dual-stack by default in the SDK CLI. Render -h 0.0.0.0 only when the shared DualStack field is explicitly set to false, so nil/true keep today's dual-stack default instead of silently going IPv4-only. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
Only the generated_at metadata field diverged from master, which was enough for GitHub to report this PR as unmergeable and apparently skip queuing CI. No scan content changed. Signed-off-by: dbbvitor <vitor.diniz@gympass.com>
Member
|
@dbbvitor I see asymmetry in default behavior, which is confusing:
I think it's better if one field, one meaning, everywhere. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Merge after #6886.
FeastServicesrenders every service container's command with a hardcoded IPv4 host flag (-h 0.0.0.0for online/offline, or the ui/lineage servers' equivalent), with no CRD field to change it. On an IPv6-only or dual-stack cluster, none of these servers bind an address that anything can reach; the Operator half of the same gap #6886 already fixed on the SDK side.This adds a
DualStack *boolfield to [ServerConfigs] https://github.com/dbbvitor/feast/blob/feat/operator-dual-stack/infra/feast-operator/api/v1/featurestore_types.go#L904). When set, a newwithBindHost()helper rewrites the rendered-hargument pair to the IPv6 wildcard address instead of leaving the hardcoded IPv4 literal:"[::]"), both reject a bare"::"as a host argument."::"instead.--hostflag at all and is left untouched; it already always binds dual-stack.withBindHost()is applied both insidegetContainerCommand(), which builds each service Deployment's container args, and insetLineageDeployment(), which builds the lineage container'sCommandslice directly rather than going through the sharedArgspath.Which issue(s) this PR fixes:
Part of #6862 (together with #6886 on the SDK side; don't let this auto-close the issue on merge; #6886 should merge first or alongside, since
dualStackdoesn't fully work for ui/lineage without it)Checks
git commit -s)Testing Strategy
Misc
DualStackis opt-in (nil/false preserves today's0.0.0.0behavior exactly), no change for clusters that don't set it.-h ::for the ui/lineage (uvicorn-based) servers only works, end-to-end because fix: Bind metrics, REST registry, ui, and lineage servers dual-stack #6886 now also fixesui_server.py'sstart_server()andlineage_server.py'sstart_lineage_server(): both previously calleduvicorn.run(host=host, ...)directly, which setsIPV6_V6ONLY=1via asyncio'sloop.create_server()and would have made those two servers IPv6-only under this option, a silent IPv4 regression, not the dual-stack fix this PR promises.