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

Fix address advertisement bugs #974

Merged
merged 9 commits into from
Jul 7, 2020
Merged

Fix address advertisement bugs #974

merged 9 commits into from
Jul 7, 2020

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jul 2, 2020

This patch fixes several address advertisements bugs that slipped through into go-ipfs 0.6.0.

  • Fallback on advertising all interface addresses if looking up the default route fails (Libp2p fails to determine default route on FreeBSD #973).
  • Don't ever advertise unspecified addresses, even if our NAT lies to us.
  • Resolve addresses before trying to lookup observed addresses.

We really need tests for this code, but that will likely require some mocking.

Copy link
Collaborator

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

This seems correct to me, I just left a few comments/questions mostly about logging errors in user configurations.

p2p/host/basic/basic_host.go Show resolved Hide resolved
Comment on lines 295 to 296
// If we failed to lookup interface addrs.
if len(h.allInterfaceAddrs) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this generally occur? If it's a user configuration or router error is it worth logging it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This can happen due to OS restrictions, or if the there are no network devices. We could probably just detect this case at the top and bail entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this logic to only use fallback addresses if InterfaceMultiaddrs fails. If it just returns no addresses, we now accept that we have no addresses.

p2p/host/basic/basic_host.go Show resolved Hide resolved
p2p/host/basic/basic_host.go Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Show resolved Hide resolved
p2p/host/basic/basic_host_test.go Show resolved Hide resolved
Stebalien and others added 2 commits July 7, 2020 09:20
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
* If we fail to lookup interface addresses, do our best and bail.
* Only fallback on guessing addresses if we fail to lookup any interface
addresses. If the interface address lookup function returns nothing, accept that
we have no IP addresses and move on.
* Log when the NAT returns a bad address.
Copy link
Collaborator

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM. Left a tiny comment typo correction

p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
@Stebalien Stebalien merged commit 9cd6aaa into master Jul 7, 2020
@Stebalien Stebalien deleted the fix/fallback-addrs branch July 7, 2020 23:41
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants