libp2p / go-libp2p-kad-dht

A Kademlia DHT implementation on go-libp2p
https://github.com/libp2p/specs/tree/master/kad-dht
MIT License
524 stars 224 forks source link

filter local addresses (for WAN) and localhost addresses (for LAN) #839

Closed marten-seemann closed 1 year ago

marten-seemann commented 1 year ago

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?

guillaumemichel commented 1 year ago

What do you think of setting the Public IP Addr Filter as the default address filter?

https://github.com/libp2p/go-libp2p-kad-dht/blob/e94c190c5620adb2d3ab4f6b724010c0890ae255/internal/config/config.go#L107-L132

The private DHT could then override this default filter with the IP Loopback filter.

marten-seemann commented 1 year ago

@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.

guillaumemichel commented 1 year ago

Many tests are failing when setting the Public IP address filter as a default option (probably because the tests create IpfsDHTs using private IP addresses).

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 addrFilter, please have a look and add more checks if some are missing. I am good to merge after that

guillaumemichel commented 1 year ago

Thanks @sukunrt!

@Jorropo are you good to merge?

Jorropo commented 1 year ago

@guillaumemichel I'm good if you are.

sukunrt commented 1 year ago

@Jorropo @guillaumemichel What's required to get this landed in kubo?

hsanjuan commented 1 year ago

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.

Jorropo commented 1 year ago

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). I think what happen in your case is that you have exclusively localhost addresses so all addresses are being filtered out which is a usecase we overlooked.

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), ideally Dual wouldn't exists anymore and you could do that with a custom Kubo config using the composable routing helpers but we never finished the composable refactor.

hsanjuan commented 1 year ago

Understood. The fix is very simple for me (just tests setting dual.LanDHTOption(dht.AddressFilter(nil)),).

Given the explanation, it's better the way it is now.

hsanjuan commented 1 year ago

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.