-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use the correct source IP when binding multiple IPs #3067
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3067 +/- ##
==========================================
- Coverage 85.72% 85.29% -0.43%
==========================================
Files 131 131
Lines 9520 9607 +87
==========================================
+ Hits 8161 8194 +33
- Misses 990 1044 +54
Partials 369 369
Continue to review full report at Codecov.
|
efd277f
to
8707051
Compare
Thank you for this PR! Before I do an in-depth review, 3 questions:
|
|
|
Yes but x/net does not support ECN so we will have to use two different ways to deal with OOB. It means we will be parsing OOB twice. |
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.
This looks pretty good to me. I left a few comments on the PR, please have a look.
Test-wise, it would be nice if we could have a unit test, along the lines of the ECN test: https://github.com/lucas-clemente/quic-go/blob/9dcb56b76f21241216e90009f3ed26d82c6c163f/conn_ecn_test.go#L56-L74
Making the unit test platform independent is a good way to test that all the platform-dependent code behaves as expected.
conn_oob.go
Outdated
// int cmsg_level; | ||
// int cmsg_type; | ||
// } | ||
hdrLen := 16 |
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.
hdrLen := 16 | |
const hdrLen = 16 |
conn_oob.go
Outdated
// struct in_addr ipi_addr; /* Header Destination | ||
// address */ | ||
// }; | ||
oob := make([]byte, hdrLen+12) |
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.
What happens to the 4 leftover bytes on FreeBSD?
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.
Good catch
} | ||
|
||
func (c *sconn) Write(p []byte) error { | ||
_, err := c.PacketConn.WriteTo(p, c.remoteAddr) | ||
_, err := c.WritePacket(p, c.remoteAddr, c.info) |
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.
Would it make sense to serialize the packetInfo
once, instead of doing it for every WritePacket
call? This would save a lot of garbage, wouldn't it?
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.
Yes, that's a good point.
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.
We also probably don't need all this machinery in case the server does not listen on ANY.
On freebsd, there is also the limitation that it does not seem to work if ANY v4+v6 is bound, you need to create v4 and v6 listeners separately.
return addr | ||
} | ||
|
||
type spconn struct { |
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'm not sure I understand the logic here. As far as I can see, the spconn
is used by the client, whereas the server would use the sconn
. That means we assume that the client's kernel will choose the right interface to send the packet from. Is my reasoning correct?
Can we achieve this by using the same struct though? WritePacket
also works if the packetInfo
is nil
, doesn't it?
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.
Your reasoning is correct, although, using the same struct for client would require the client code to wrap the net.PacketConn
to make it a connection
in all the client code too.
Looks like the cross-compilation test is failing on some architectures. Can you please check? |
Yes that’s next on my list. I need to learn this testing framework first. |
@rs I'm happy to help if you run into any problem, please let me know. |
@marten-seemann I'm not sure why the keepalive test fails since I rebased. |
Unit tests LGTM. Don't worry about the timeout test, it's been flaky recently. Will investigate it soon. Can you please rebase & squash your PR? |
Done |
When the server is listening on multiple interfaces or interfaces with multiple IPs, the outgoing datagrams are sometime delivered with the wrong source IP address. In order to fix that, each quic connection needs to extract the destination IP (and optionally interface id) of the received datagrams, and set it as source IP (and interface) on the sent datagrams. On most platforms, this can be done using ancillary data with recvmsg() and sendmsg(). Some of the machinery for this is already there for ECN, this change extends it to read the destination IP info and write it to the outgoing packets. Fix quic-go#1736
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.
@rs LGTM now. Thank you for your contribution!
When the server is listening on multiple interfaces or interfaces with multiple IPs, the outgoing datagrams are sometime delivered with the wrong source IP address.
In order to fix that, each quic connection needs to extract the destination IP (and optionally interface id) of the received datagrams, and set it as source IP (and interface) on the sent datagrams.
On most platforms, this can be done using ancillary data with recvmsg() and sendmsg(). Some of the machinery for this is already there for ECN, this change extends it to read the destination IP info and write it to the outgoing packets.
Fix #1736
Note that this change is highly untested at this point.