-
Notifications
You must be signed in to change notification settings - Fork 223
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
filter local addresses (for WAN) and localhost addresses (for LAN) #839
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.
Seems duplicated with it looks duplicated but this filtering records sentPrivateRoutingTableFilter
and PublicRoutingTableFilter
.
What do you think of setting the Public IP Addr Filter as the default address filter? go-libp2p-kad-dht/internal/config/config.go Lines 107 to 132 in e94c190
The private DHT could then override this default filter with the IP Loopback filter. |
@guillaumemichel, yes, that makes sense. I'm wondering if we should be more careful which addresses we send out as well. We probably shouldn't even tell other nodes about private addresses, if we're connected to them over the public internet. I suggest to do that in a follow-up PR though. |
2b961bb
to
e94c190
Compare
Many tests are failing when setting the Public IP address filter as a default option (probably because the tests create It is fine for now to leave it on the dual DHT only (no default address filter). This way we won't break anyone that may use a (non-dual) DHT with private addresses. I added basic tests for the |
ef33801
to
4c55004
Compare
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.
@guillaumemichel the tests look good to me.
@guillaumemichel I'm good if you are. |
@Jorropo @guillaumemichel |
Hey, so this is causing my tests in cluster to break because I create a swarm of dual-dht hosts on localhost. It was very hard to arrive here. I don't fully understand the rationale on why Dual DHT will ignore localhost nodes now. Seems a bit arbitrary (localhost becomes second class versus everyone else in the LAN?). Probably not many people hit this, but when they hit it they will have a hard time figuring out why those dht lookups fail. |
The LAN dht ignores localhost in order to not announce Localhost IPs to other nodes on your LAN (which cause libp2p to first attempt all localhost addresses before trying the LAN ones). As a workaround you can add LAN ips to all the Kubo processes on the same machine, which has similar performance as running through localhost (MTU will be worst but QUIC does not use the higher MTU of localhost anyway) I guess you want a triple DHT setup where you have a localhost only DHT (since we would like to avoid advertising localhost addresses on the DHT), |
Understood. The fix is very simple for me (just tests setting Given the explanation, it's better the way it is now. |
It's also an issue coming from test-local-hosts being setup to explicitly need DHT lookups for dials, coming from a time when testing the rest of the ipfs stack was more important. I don't think anyone is going to do it this way nowadays. |
Filters are only set for the Dual DHT. Is that correct? Do they also need to be set for other ways to initialize the DHT?