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

DoH resolver implementation #1

Merged
merged 9 commits into from
Apr 12, 2021
Merged

DoH resolver implementation #1

merged 9 commits into from
Apr 12, 2021

Conversation

vyzo
Copy link
Collaborator

@vyzo vyzo commented Apr 8, 2021

Depends on multiformats/go-multiaddr-dns#26

TBD:

  • tests

resolver.go Outdated Show resolved Hide resolved
resolver.go Outdated Show resolved Hide resolved
@vyzo vyzo requested a review from lidel April 8, 2021 15:03
Copy link
Contributor

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

I'm not so familiar with the DoH specifics, but it looks like you're just using the libraries and testing against actual DoH servers in the wild and it works.

If @lidel is happy then I'm happy 😄

go.mod Outdated Show resolved Hide resolved
@vyzo
Copy link
Collaborator Author

vyzo commented Apr 9, 2021

Updated for go-multiaddr-dns@v0.3.0, as the upstream dependency has been merged.

Awaiting @lidel's review.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM. Some suggestions for improved tests below, but no blockers.

resolver_test.go Outdated Show resolved Hide resolved
"testing"
)

func TestLookupIPAddr(t *testing.T) {
Copy link
Member

@lidel lidel Apr 9, 2021

Choose a reason for hiding this comment

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

Would be good to have an explicit check that confirms this returns both ipv4 AND ipv6 (libp2p.io has both, so good candidate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, let me do that/

vyzo and others added 2 commits April 12, 2021 11:45
@vyzo vyzo merged commit 5933266 into main Apr 12, 2021
@vyzo vyzo deleted the implementation branch April 12, 2021 08:49
@lidel lidel mentioned this pull request Apr 12, 2021
9 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.

None yet

3 participants