Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

fix: avoid dialing/listening on dns addresses #131

Merged
merged 2 commits into from
Apr 3, 2020
Merged

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien requested a review from marten-seemann April 3, 2020 05:37
Copy link
Collaborator

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

There's a QUIC variable define in mafmt: https://godoc.org/github.com/multiformats/go-multiaddr-fmt#pkg-variables.
Should we use that one?

@@ -190,9 +190,12 @@ func (t *transport) Dial(ctx context.Context, raddr ma.Multiaddr, p peer.ID) (tp
}, nil
}

// Don't use mafmt.QUIC as we don't want to dial DNS addresses. Just /ip{4,6}/udp/quic
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@marten-seemann marten-seemann Apr 3, 2020

Choose a reason for hiding this comment

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

🤦‍♂️ Sorry for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's confusing. We may want to just go ahead and revert the change in mafmt (we changed it from IP only to IP+DNS). But it does make sense to allow DNS there, just not here.

Copy link
Collaborator

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

LGTM.

Would you mind adding the unit test from transport_test.go in #126 here?

@Stebalien
Copy link
Member Author

I completely forgot about that PR.

@Stebalien Stebalien merged commit 7a1d94f into master Apr 3, 2020
@Stebalien Stebalien deleted the fix/no-dns branch April 3, 2020 16:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants