Locally listen on IPv4 and IPv6 addresses #775
Conversation
markmandel
added
kind/feature
markmandel
requested review from
XAMPPRocky,
No-ops,
rezvaneh and
vallfors
as code owners
Implemented the `DualStackLocalSocket` for the locally listening socket, and updated the code base and fixed several issues that existed for handling IPv6 addresses in the code base. This included updating the `test_utils` framework to provide coverage in tests for both ipv4 and ipv6 by allowing for either an IPv4 or IPv6 address to be created from the framework. Closes googleforgames#666
markmandel
force-pushed
the
feature/ipv6nv4
branch
from
2c91b4d
to
0bb7796
Compare
|
Huh weird. Testing locally with cargo seems to work just fine, but when running inside the container test suite, it falls over. At least I can replicate locally. |
|
If I run the tests in the container with |
|
Now I'm actually wondering if I can use a socket with I'll take it for a spin and let you know how I go. |
I think I tried that before, I believe that won't work cross-platform. Maybe we can find a compromise that's good enough. |
| let len = value.len(); | ||
| if len > 2 && value.starts_with('[') && value.ends_with(']') { | ||
| return value[1..len - 1].to_string(); | ||
| } |
There was a problem hiding this comment.
Do the brackets not work when passed in? 🤔 Isn't that part of the address?
There was a problem hiding this comment.
If the brackets are still there in the host when it gets to
let host = host.parse::<AddressKind>().unwrap();
Then it parses it as as AddressKind(Name("[::]")) rather than AddressKind(Ip(V6("::"))), which is not what we want.
There was a problem hiding this comment.
Right, but I think a better solution would be to update the FromStr implementation of AddressKind to handle that instead of EndpointAddress, so that it's always correct, rather than EndpointAddress::parse and AddressKind::parse providing different results.
There was a problem hiding this comment.
Aaah gotcha. I will make that change 👍🏻
| let mut rng = thread_rng(); | ||
| // sometimes give ipv6, sometimes ipv4. | ||
| match rng.gen_range(0..2) { | ||
| 0 => socket.local_ipv6_addr().unwrap(), | ||
| 1 => socket.local_ipv4_addr().unwrap(), | ||
| _ => unreachable!(), | ||
| } |
There was a problem hiding this comment.
You can simplify this by using rand::random.
| let mut rng = thread_rng(); | |
| // sometimes give ipv6, sometimes ipv4. | |
| match rng.gen_range(0..2) { | |
| 0 => socket.local_ipv6_addr().unwrap(), | |
| 1 => socket.local_ipv4_addr().unwrap(), | |
| _ => unreachable!(), | |
| } | |
| match rand::random() { | |
| true => socket.local_ipv6_addr().unwrap(), | |
| false => socket.local_ipv4_addr().unwrap(), | |
| } |
That was my conclusion as well from research, but now I don't actually know. I'm trying some small experiments. |
|
Just dropping research so I can also find it later: Seems to indicate that a dual stack socket will work on windows, but the default for a socket is different on Windows (IPV6_V6ONLY is true by default on windows, but false on Linux). So I think we can do this with one ipv6 socket -- but the testing changes through here will still be useful at least. Making updates and will see how we go. |
|
Initial tests with a ipv6 socket with But that helped me narrow down the issue - within a Docker container, it seems that it hates sending traffic to I may just force it to run on |
|
Build Failed 😭 Build Id: e339579e-921b-42a6-b240-718d8994cf27 Status: FAILURE To get permission to view the Cloud Build view, join the quilkin-discuss Google Group. Filter with the Git Commit Development images are retained for at least 30 days. |
|
I've got work on this going on a separate branch (using purely ipv6 sockets). Will likely close this PR and resubmit, but just keeping it open since there's some feedback I want to make sure I capture in here. |
|
New PR incoming using ipv6 sockets, shortly, closing this one to avoid the noise. Also incorporates the feedback in here. |
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Implemented the
DualStackLocalSocketfor the locally listening socket, and updated the code base and fixed several issues that existed for handling IPv6 addresses in the code base.This included updating the
test_utilsframework to provide coverage in tests for both ipv4 and ipv6 by allowing for either an IPv4 or IPv6 address to be created from the framework.Which issue(s) this PR fixes:
Closes #666
Special notes for your reviewer:
Let me know what you think of the
Randomaddress type approach. Felt like the easiest way to get general coverage and force testing of both paths, without having to have some kind of dual-run test harness.