libnetwork: respect gw priority per address family#52328
libnetwork: respect gw priority per address family#52328thaJeztah merged 1 commit intomoby:masterfrom
Conversation
|
Hi @Varun5711 - thank you for taking a look at this, from a quick check I think the change is good. I'll paste @thaJeztah's explanation of the DCO sign-off below.
I also use a Mac ... if you want to run the tests, the contributing guide describes how to use a container for development - https://github.com/moby/moby/blob/master/docs/contributing/set-up-dev-env.md - but we can also just see what happens in CI once you've added the "Signed-off-by". Thank you for contributing! It appears your commit message is missing a DCO sign-off, We require all commit messages to have a Signed-off-by line with your name Signed-off-by: YourFirsName YourLastName yourname@example.org Unfortunately, it's not possible to do so through GitHub's web UI, so this needs You can find some instructions in the output of the DCO check (which can be found Steps to do so "roughly" come down to: Set your name and e-mail in git's configuration: git config --global user.name "YourFirstName YourLastName" Clone your fork locally Check out the branch associated with this pull request Sign-off and amend the existing commit(s) git commit --amend --no-edit --signoff Force push your branch to GitHub (using the --force or --force-with-lease flags) to update the pull request. Let me know if you need help or more detailed instructions! |
608a9e1 to
7870ca0
Compare
|
Hi @robmry — thanks for the guidance! I’ve added the DCO sign-off and updated the commit. Please let me know if anything else is needed. |
There was a problem hiding this comment.
Pull request overview
This PR fixes default gateway endpoint selection in daemon/libnetwork so that gateway priority is respected independently for IPv4 and IPv6 when a sandbox is attached to mixed single/dual-stack networks (fixes #51999).
Changes:
- Update
Sandbox.getGatewayEndpoint()to select IPv4 and IPv6 gateway endpoints independently from the already-sorted endpoint list. - Add a regression test covering a higher-priority IPv4-only endpoint alongside a lower-priority dual-stack endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
daemon/libnetwork/default_gateway.go |
Adjusts gateway endpoint selection to honor gw-priority per address family (v4/v6 resolved independently). |
daemon/libnetwork/sandbox_unix_test.go |
Adds a regression test intended to ensure per-family gateway priority selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7870ca0 to
686edb0
Compare
|
Hi @robmry — I’ve pushed a fix for the failing test related to gateway selection per address family. Please let me know if you’d like any changes or once CI is approved. Thanks! |
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! I left some comment (sorry for being nit-picky), but LGTM after those are addressed.
| if ep4 != nil && ep6 != nil { | ||
| return ep4, ep6 | ||
| } | ||
| } | ||
| return ep4, ep6 |
There was a problem hiding this comment.
Really a nit; the really return is a bit confusing, because .. it does the same thing as the return at the end, but I had to look twice to see if there was no bug (because the return at the end would unconditionally return the values, even if one (or both) are nil).
Perhaps a break would be more clear in this case (could even add a comment to be explicit);
if ep4 != nil && ep6 != nil {
// Found both; we're done.
break
}
}
return ep4, ep6|
|
||
| ctrlr, nws := getTestEnv(t, opts...) | ||
|
|
||
| sbx, err := ctrlr.NewSandbox(context.Background(), "sandbox-prio-per-family") |
There was a problem hiding this comment.
We can use t.Context() now (we haven't updated all tests to do so, but for new tests we may as well do so).
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Looks like we're mixing stdlib and gotest.tools asserting within the same test; can you use assert for these as well?
assert.NilError(t, err)|
Oh! For those changes; you can amend the commit (we generally prefer not having separate "fix up" commits) |
Signed-off-by: Varun Hotani <varunhotani@gmail.com>
686edb0 to
e30a6fa
Compare
|
Addressed the requested nits in the existing commit and force-pushed the branch:
|
Fixes #51999
Problem
When a sandbox is attached to both:
Moby may pick the dual-stack endpoint as the IPv4 default gateway even though
GwPrioritysays the IPv4-only endpoint should win.Root cause
Endpoint.Less()already sorts endpoints using:But
getGatewayEndpoint()indaemon/libnetwork/default_gateway.gobypasses that ordering by returning the first dual-stack endpoint for both address families.As a result, a lower-priority dual-stack endpoint can take over IPv4 gateway selection.
Fix
Select IPv4 and IPv6 gateway endpoints independently from the sorted endpoint list:
This makes gateway selection honor priority per address family while preserving current tie behavior.
Tests
Add a regression test covering:
Expected:
Validation
Built with:
GOOS=linux GOARCH=amd64 go test -c ./daemon/libnetworkI could not run the full
daemon/libnetworktest suite locally because it is Linux-only and my current environment is macOS.- if a container has an IPv4-only or an IPv6-only endpoint with higher "gateway priority" than a dual stack endpoint, the single stack endpoint will now be used as the default gateway for its address family.