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

Custom LookupIPAddr function #699

Merged
merged 4 commits into from
Nov 25, 2019
Merged

Conversation

enchantner
Copy link
Contributor

Continuing the work started at https://github.com/valyala/fasthttp/pull/689/files .

Turns out, specifying custom resolver doesn't allow to avoid resolving completely (as Dial() has to return a connection to a DNS server), so in this way it becomes possible to reimplement LookupIPAddr() function to specify resolved name manually or get it from some place other than actual DNS query.

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

Instead of adding another field I would prefer to fix this differently. I think it's better if we change

type TCPDialer struct {
	// ...

	Resolver *net.Resolver
}

into

type Resolver interface {
	LookupAddr(context.Context, string) (names []string, err error)
}

type TCPDialer struct {
	// ...

	Resolver Resolver
}

That way net.Resolver implements this interface, but if you want to use a different LookupAddr function you can use your own struct that implements this interface with your own function.

@enchantner
Copy link
Contributor Author

enchantner commented Nov 25, 2019

Wouldn't it better to use LookupIPAddr instead of LookupAddr? Afaik, LookupAddr only deals with IPv4 addresses, but with LookupIPAddr we're getting both. Anyway, here's a commit.

Copy link
Collaborator

@erikdubbelboer erikdubbelboer 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 sure I understand. LookupAddr does a reverse lookup from IP to name. That's not what we want?

One last change and then I'll merge it. Sorry for being pedantic.

tcpdialer.go Outdated Show resolved Hide resolved
@enchantner
Copy link
Contributor Author

enchantner commented Nov 25, 2019

@erikdubbelboer LookupAddr spits out a list of strings, which are each an IPv4 address in "four numbers with dots" representation. That's totally okay, but IPAddr type is more common and compact, as you can get binary representations for both IPv4 and IPv6 from it, if present. It looks like fasthttp doesn't support IPv6 yet, but in this way we may be able to introduce it without changing this Resolver interface again.

Also, fixed your comment, thanks!

@erikdubbelboer
Copy link
Collaborator

LookupAddr goes from IP to domain. LookupIPAddr goes from domain to IP.

Where in fasthttp do you think we use LookupAddr?

fasthttp does support IPv6 just fine.

@erikdubbelboer erikdubbelboer merged commit bc5b479 into valyala:master Nov 25, 2019
@enchantner
Copy link
Contributor Author

@erikdubbelboer yeah, sorry, I just got everything mixed up looking at the example code you've provided above for the interface. Thanks for merging this!

@erikdubbelboer
Copy link
Collaborator

Ah sorry I didn't realize I put LookupAddr in my example interface. My bad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants