ensure docker-proxy sends UDP responses from correct source IP#2577
ensure docker-proxy sends UDP responses from correct source IP#2577tsujamin wants to merge 6 commits intomoby:masterfrom
Conversation
tsujamin
force-pushed
the
1729-docker-proxy-udp-srcaddr-fix
branch
2 times, most recently
from
fdbc3f3 to
6b5d57b
Compare
|
Anything additional changes/justification needed my end to progress this PR? |
|
Any issues? |
| var oob []byte //= nil | ||
|
|
||
| if srcAddr.To4() != nil { | ||
| cm := new(ipv4.ControlMessage) |
There was a problem hiding this comment.
This seems expensive here. Can we re-use control messages?
Even then we'll be allocating for Marshal which is also not great.
There was a problem hiding this comment.
If I'm parsing the go documentation correctly (which I'm likely not 😅 it's a normal language for me) I think cm is both read and written by WriteMsgUDP, so reusing it mightn't be appropriate as the state of the other cm members might have been mutated.
Is there anything else in the change I could modify to get it across the line performance wise? I appreciate the increased cost to processing UDP clients but it seem necessary for UDP correctness.
There was a problem hiding this comment.
cm is only being used to marshal a message nothing shared between uses from what I can tell.
Each call to Marshal() is a new byte slice.
There was a problem hiding this comment.
Sorry I’m on the same page now - working through a fix and addressing regressions now
tsujamin
force-pushed
the
1729-docker-proxy-udp-srcaddr-fix
branch
from
6b5d57b to
1fef916
Compare
Signed-off-by: Benjamin George Roberts <[email protected]>
Signed-off-by: Benjamin George Roberts <[email protected]>
Signed-off-by: Benjamin George Roberts <[email protected]>
Signed-off-by: Benjamin George Roberts <[email protected]>
Signed-off-by: Benjamin George Roberts <[email protected]>
tsujamin
force-pushed
the
1729-docker-proxy-udp-srcaddr-fix
branch
from
1fef916 to
95e7148
Compare
Signed-off-by: Benjamin George Roberts <[email protected]>
tsujamin
force-pushed
the
1729-docker-proxy-udp-srcaddr-fix
branch
from
2995e99 to
0881a04
Compare
|
@cpuguy83 I've removed the marshal and allocation from the reply loop (there's |
|
@cpuguy83 any time to review this? the underlying issue hit us hard in prod today 😢 |
| if proxy.inet6Socket { | ||
| oobBuf = ipv6.NewControlMessage(ipv6.FlagDst | ipv6.FlagInterface) | ||
| } else { | ||
| oobBuf = ipv4.NewControlMessage(ipv4.FlagDst) |
There was a problem hiding this comment.
Seems like we have the same issue here as we did on Write where we allocate a new buffer for every read even though the buffere size should be constant.
| var to *net.IP = nil | ||
|
|
||
| if from.IP.To4() == nil { | ||
| cm := new(ipv6.ControlMessage) |
There was a problem hiding this comment.
Same, can we re-use these objects instead of creating new ones on each read?
|
Also I think we only need to incur this overhead when the IP is |
|
Note we have migrated this codebase over to github.com/moby/moby/libnetwork. |
3 participants
When docker-proxy listens for UDP connections on
0.0.0.0or::it is not garuanteed to respond to a packet from the same IP the packet was sent to. This affects machines running docker-proxy that are multi-homed (multiple default gateways), or have multiple IPs assigned to the same interface. The impact is that packets may not be routed correctly back to the client due to the change of source-ip.Currently the only workaround is to explicitly bind docker-proxy to each local IP address (e.g.
-p 1.1.1.1:1234/1234, -p 1.1.1.2:1234/1234), which is not possible in situations where IPs change (for instance, VPN tunnels coming up and down).This PR uses the
IP_PKTINFOsocket option to retrieve the destination IP of the packet received by the host side, and ensures responses for the given connection are sent from the same IP. This should close #1729I've added a basic unit test which simulate a multi-homed host, specifically:
127.0.0.10.0.0.0on the host side, so it will accept connections to any IP assigned to the host127.0.0.2(simulating a connection to a secondary IP address on a host) is initiatedPrior to this patch, docker-proxy would accept the packet on the 127.0.0.2 address, but respond from the 127.0.0.1 address. After the patch docker-proxy responds from the same IP the initial packet was sent to.
This is my first contribution to libnetwork/moby/docker, and I'm not a Go developer, so I appreciate any feedback or changes that may be required, and am happy to discuss/further advocate for this change.