Skip to content
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

discovery: swap mdns lib for grandcat/zeroconf #285

Closed
wants to merge 8 commits into from

Conversation

rargulati
Copy link
Contributor

@rargulati rargulati commented Feb 27, 2018

Specifically addressing the request here: #257

This commit makes the minimal amount of changes to switch us from
whyrusleeping/mdns to grandcat/zeroconf. Of note, rather than asking the
host.Host which addrs we can listen on, we push this to the
zeroconf library (which runs across available ifaces).

I haven't had a chance to test this on something complicated to see if it retains the desired behavior (to be honest, still new to this codebase, unsure of what all of those would be!), but tests pass locally.

TODO:

  • Revert PORT behavior
  • ensure that grandcat/zerconf dep is in gx; tests pass

info := strings.Join(e.Text, "|")

// addripv4 potential issue
log.Debugf("Handling MDNS entry: %s:%d %s", e.AddrIPv4[0], e.Port, info)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only decision I wasn't certain of.

}

func (m *mdnsService) pollForEntries(ctx context.Context) {

ticker := time.NewTicker(m.interval)
for {
resolver, err := mdns.NewResolver(nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be beast to put resolver in mdnsService struct and reuse it, instead of creating a new one every iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, good catch. Fixed.

func NewMdnsService(ctx context.Context, peerhost host.Host, interval time.Duration, serviceTag string) (Service, error) {

// TODO: dont let mdns use logging...
golog.SetOutput(ioutil.Discard)

var ipaddrs []net.IP
port := 4001
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is old but it is very wrong.

@@ -165,7 +138,7 @@ func (m *mdnsService) handleEntry(e *mdns.ServiceEntry) {
}

maddr, err := manet.FromNetAddr(&net.TCPAddr{
IP: e.AddrV4,
IP: e.AddrIPv4[0],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there is possibility of multiple addresses, I would iterate over them, crate maddr for each and add it to the peerstore.

@rargulati
Copy link
Contributor Author

@Stebalien @Kubuxu I've addressed the feedback in c92f1f6

In regards to the port, what would you recommend here? Skimmed through https://tools.ietf.org/html/rfc6762 and see that 5353 is used for the listener, but not seeing anything for the port that's announcing.

I notice that I'm failing tests due to a gx issue. Any recommendations? I'll need to learn that right quick :)

@Kubuxu
Copy link
Member

Kubuxu commented Feb 27, 2018

@rargulati It was my mistake, I thought it was the port we were announcing to the world but it is mDNS discovery port.

@Stebalien correct if I am wrong, but currently mDNS only publishes peerID in local network, not the full multiaddr, is that right? If it is so, is there a reason not to publish maddr so we can connect to it? Publishing just peerID sees little fruitless as it won't work without DHT capabilities (in case that bootstrap didn't work).

@Stebalien
Copy link
Member

Port is the port of the service. Also, we do announce the IP addresses and we convert port + IP into /ip4/IP/tcp/PORT. However, that's... suboptimal. It doesn't account for any other potential transports.

Note: For now, I'd be fine preserving the existing behavior. The primary concern here is upgrading to a working mDNS library (one that doesn't have memory safety issues).

However, we also have a bunch of bugs/inconsistencies that we need to fix. I've written them up here:libp2p/libp2p#28

@rargulati
Copy link
Contributor Author

rargulati commented Mar 5, 2018

Just to clarify on final things:

  • Revert PORT to 4001 - the fix to this and other issues will be addressed as detailed in Future of mDNS discovery libp2p#28
    - [ ] Get grandcat/zerconf into gx (I'm struggling with this - how do I get a new hash for a new dependency - either of you mind doing a quick how-to in irc/email/here?) @Stebalien handling

Does that sound about right @Stebalien @Kubuxu - I'll get this across the line asap.

@Stebalien
Copy link
Member

Revert PORT to 4001 - the fix to this and other issues will be addressed as detailed in libp2p/libp2p#28

Actually, restore the previous "figure out what port we're using" code. It's not perfect, but it's better than just assuming port 4001.

Get grandcat/zerconf into gx (I'm struggling with this - how do I get a new hash for a new dependency - either of you mind doing a quick how-to in irc/email/here?)

I'll import this into gx tomorrow. We need to write some proper tutorials but it's on our never ending backlog.

@Stebalien
Copy link
Member

I'll import this into gx tomorrow. We need to write some proper tutorials but it's on our never ending backlog.

This will actually require updating some of our dependencies so it may take a week (blocked on a bunch of other stuff).

@deckbsd
Copy link

deckbsd commented Mar 24, 2018

Hi, do you know when this commit will be done ?

@Stebalien
Copy link
Member

Stebalien commented Mar 24, 2018

It been blocked on:

Once those are done, I'll finally be able to bubble a few low-level updates through the entire tree. The problem here is that this zeroconf library requires a newer version of golang.org/x/net than we use in go-ipfs.

rargulati and others added 3 commits July 23, 2018 12:00
This commit makes the minimal amount of changes to switch us from
whyrusleeping/mdns to grandcat/zeroconf. Of note, rather than asking the
host.Host which addrs we're available to listen on, we push this to the
zeroconf library (which runs across available ifaces).
1. If we receive multiple ipv4 addresses, ensure that we convert them
all to multiaddrs and store them in the peerstore.
2. Ensure we don't allocate a new resolver on every tick. Instead reuse
this by storing it in our mdns type.
3. Attempt choosing a port.
@ghost ghost assigned Stebalien Jul 23, 2018
@ghost ghost added the status/in-progress In progress label Jul 23, 2018
Ideally, we'd dynamically announce ports based on the current interfaces.
However, we'd like to merge this ASAP.
@Stebalien Stebalien added need/review Needs a review and removed status/in-progress In progress labels Jul 23, 2018
@Stebalien
Copy link
Member

Still need to fix #376 but I'd like to punt on that.

@Stebalien
Copy link
Member

Stebalien commented Jul 23, 2018

@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Jul 23, 2018
@ghost ghost added the status/in-progress In progress label Jul 23, 2018
@Stebalien Stebalien removed the status/blocked Unable to be worked further until needs are met label Jul 24, 2018
@Stebalien Stebalien removed the status/in-progress In progress label Jul 24, 2018
var out []*net.TCPAddr
for _, addr := range ph.Addrs() {
// Tries to pick the best port.
func getBestPort(addrs []ma.Multiaddr) (int, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed by #376

@Stebalien Stebalien requested a review from bigs July 24, 2018 00:14
Copy link
Contributor

@bigs bigs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just have some confusion regarding some of the handleEntry function. if that's as intended, feel free to merge.

log.Debugf("Handling MDNS entry: %s:%d %s", e.AddrV4, e.Port, e.Info)
mpeer, err := peer.IDB58Decode(e.Info)
// pull out the txt
info := strings.Join(e.Text, "|")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this designed to fail if there are less than or more than one entry?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear, because of the decoding afterwards

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Yeah, that looks wrong. We should just take the first entry and ignore the rest.

@ghost ghost added the status/in-progress In progress label Jul 25, 2018
@Stebalien Stebalien added status/blocked Unable to be worked further until needs are met and removed status/in-progress In progress need/review Needs a review labels Jul 25, 2018
@Stebalien
Copy link
Member

Blocked on grandcat/zeroconf#38. Fixing that was the reason we went down this road in the first place so there's no real reason to merge this till we get that fixed.

@Stebalien
Copy link
Member

Given libp2p/specs#80, we should probably just abort this whole thing and implement our own library that:

  1. Implements that spec.
  2. Doesn't have random race conditions.
  3. Can handle things like network interface changes.

@Stebalien Stebalien closed this May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants