-
Notifications
You must be signed in to change notification settings - Fork 43
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
Ipv6 #130
Ipv6 #130
Conversation
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.
Thanks for your PR! Nice work!
Overall looks good to me. Only have a few minor comments inline. Yes we would need a version upgrade due to the changes in API (lesson learned: we didn't have to use IPv4 specific name/types even though we didn't support IPv6). I will think a bit more regarding the public API change and make sure it's okay.
Thanks again and this IPv6 support will be very helpful.
I'm not sure why it doesn't work on windows :/ |
I tried it out on Windows 11. It seems like IPv6 diff --git a/src/service_daemon.rs b/src/service_daemon.rs
index 08053e9..23c1e2e 100644
--- a/src/service_daemon.rs
+++ b/src/service_daemon.rs
@@ -642,9 +642,9 @@ fn new_socket_bind(intf: &Interface) -> Result<Socket> {
Ok(sock)
}
IpAddr::V6(ip) => {
- let mut addr =
+ let addr =
SocketAddrV6::new(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0), MDNS_PORT, 0, 0);
- addr.set_scope_id(intf.index.unwrap_or(0));
+ // addr.set_scope_id(intf.index.unwrap_or(0));
let sock = new_socket(addr.into(), true)?;
// Join mDNS group to receive packets. I have to be clear though, I'm no expert on IPv6 and don't really how exactly |
Also I found another test failure on macOS Monterey. It seems depending on what kind of interfaces exist on the system. But anyway, the issue is that in my macOS system, there are two IPv6 interfaces with different names, but the same IPv6 address.
I applied the following diff in the test for it to pass: diff --git a/tests/mdns_test.rs b/tests/mdns_test.rs
index 7350c4a..9d825e1 100644
--- a/tests/mdns_test.rs
+++ b/tests/mdns_test.rs
@@ -2,7 +2,7 @@ use if_addrs::{IfAddr, Interface};
use mdns_sd::{
DaemonEvent, IntoTxtProperties, ServiceDaemon, ServiceEvent, ServiceInfo, UnregisterStatus,
};
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
use std::sync::{Arc, Mutex};
use std::thread::sleep;
@@ -22,7 +22,8 @@ fn integration_success() {
.unwrap();
let instance_name = now.as_micros().to_string(); // Create a unique name.
- let my_ifaddrs: Vec<_> = my_ip_interfaces().iter().map(|intf| intf.ip()).collect();
+ let ifaddrs_set: HashSet<_> = my_ip_interfaces().iter().map(|intf| intf.ip()).collect();
+ let my_ifaddrs: Vec<_> = ifaddrs_set.into_iter().collect();
let my_addrs_count = my_ifaddrs.len();
println!("My IP addr(s): {:?}", &my_ifaddrs); (Note that |
Fix the test when multiple ifaces have the same ip6 addr Co-authored-by: keepsimple1 <keepsimple1@users.noreply.github.com>
Nice catch! For the scope_id, it is extremely confusing, In ipv6 multicast, the kernel needs to know on which interface the packet should be sent because all ip6 interface are required to get a link-local addr (fe80::) and the user might not want to broadcast on all ifaces. Windows documentation seems to agree with that https://learn.microsoft.com/en-us/windows/win32/wininet/ip-version-6-support It is confusing because for multicast addr, part of the address is called Maybe windows only accept a scope_id when the socketaddr is a multicast address, because when it is not a multicast windows already knows on which interface it should send packets? In conclusion I'll applied your patch, the socketaddr iface_id is already set twice right after with
PS: Sorry I wrote this at the same time I did the research, hope its not too messy |
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.
Happy to see all tests passed! I missed one part and have a new comment. The rest looks good! I will be ready to merge.
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.
Looks good, thanks! I did a quick test locally, it requires very little change of code using the API even thought it changed.
One thing I noticed is that some Ipv4Addr
methods are not supported by IpAddr
, for example is_link_local()
, which exists for Ipv6addr
under a different name is_unicast_link_local()
and not stable yet. So a user will not be able to call is_link_local()
on the addrs returns from ServiceInfo.get_addresses()
. I will open a follow-up PR to mitigate this problem.
I missed to ask about this point in your description. How is |
It works if the interface has no IPv6 or no IPv4 configured, the ServiceDaemon won't try to broadcast on an un-configured ip version. |
That's true. I was wondering about the cases when the user wanted to explicitly disable IPv4 or IPv6 even if they are configured on the interface. It would be helpful for one of my use cases. I'll take a look. |
There are two levels of this kind of control:
For browsing, the current API is simple and does not allow much options. But at least a user can filter out the resolved IPv4 or IPv6 addresses based their needs. Any suggestions or comments based on your experience involving both IPv4 and IPv6? |
Actually I've been using mdns-sd on an Ipv6 only host, I agree that the user should be able to choose the interface and ip versions, but the most important thing it to have a good default behavior. |
Add ipv6 and AAAA records support
This will probably need a version bump as the interface is slightly modified
It now takes
IpAddr
andif_addr::Interface
types in most places instead of the V4 oneshost_ipv4
argument renamed toip
AsIpv4Addrs
renamed toAsIpAddrs
The reviewer should look at the diff per commit as it is easier to read
Fix #61