Implement address family affinity and prefer IPv4 by default #66
Conversation
There was a problem hiding this comment.
The Go docs for the package (in doc.go) and for WithResolver (in client.go) refer to the old default behavior and need to be updated, too.
The only reason I didn't approve is because of the question about IPv4 addresses wrapped inside of IPv6. It's not clear to me that what we're doing is correct. (Other comments are non-blocking.)
| @@ -37,7 +37,7 @@ var ( | |||
| KeepAlive: 30 * time.Second, | |||
| } | |||
| defaultNameTTL = 5 * time.Minute | |||
| defaultResolver = resolver.NewDNSResolver(net.DefaultResolver, "ip", defaultNameTTL, resolver.PreferIPv4) | |||
| defaultResolver = resolver.NewDNSResolver(net.DefaultResolver, resolver.PreferIPv4, defaultNameTTL) | |||
There was a problem hiding this comment.
I think we want this default to be controllable via env var. Maybe something like HTTPLB_DEFAULT_ADDRESS_FAMILY_POLICY. We can document the exact values the env var needs to have either in NewClient or maybe it would be better in doc.go where it talks about default behavior.
There was a problem hiding this comment.
Hmmm... To be honest, I'm not sold on this. There are exceptions where it may be useful (a la GODEBUG) but in the general case I see this as a code smell. It essentially adds an implicit "impure" configuration knobs that skip the application's own configuration mechanisms. If an application wants to create a "default" resolver with one setting changed, they have to replicate this behavior too.
If applications want this behavior to be controlled by environment variable, I think they ought to do it themselves.
If you insist I can add it, but is there really a good reason why reading and parsing environment variables is something httplb itself should do?
There was a problem hiding this comment.
skip the application's own configuration mechanisms
It would only be for the default, where the application has not configured anything (which will typically be unintentional, either an inadvertent omission or lack of foresight). If the application does configure it, the env var would not override.
If applications want this behavior to be controlled by environment variable, I think they ought to do it themselves.
I guess my worry is that this would never happen in practice. If they are adding code of this sort at all, then they are thinking about this and likely configuring things intentionally. More likely is that this isn't thought about or configured at all because the defaults "just worked" on day one. And then on day two, when some vendor pushes a DNS change, things break (such as in an IPv6 single-stack setup, where DNS is updated to suddenly start returning an A record). I guess I'm just used to providing knobs for certain scenarios that don't changing code, re-building, and re-deploying, as a dev ops instinct. 🤷
I guess we can leave it out. But this is the only behavior where httplb will observably differ from net/http and could do so in a way that prevents successful client->server communication, so it seemed useful. To be honest, I don't place a lot of value on "purity", especially if it potentially compromises usability or developer experience.
Anyhow, if you really don't like it, leave it out. But we definitely do need to update the docs about the change in default behavior.
There was a problem hiding this comment.
I think I'll hold off on adding this. I can see the operational value of a feature like this, but in practice I think that it is least surprising to users if a library has one and only one way of configuring things, through the API, rather than environment variables. If we do want to make this sort of thing configurable through environment variables, it'd probably be worth considering other things that might be worth exposing as environment variables and/or flags, possibly as a side-effect of importing an optional package, kind of like how some C++ libraries will have packages you can link to to get absl flags. I can see some arguments against this, too (it wouldn't help for people who didn't explicitly think of this, which is arguably the people who have the most to gain from the built-in ability to change a configuration knob at runtime) but ultimately I think that the better way to resolve this potential need is to make our IPv6 fallback work better in the future, so that users don't really have to think about it much more than they would for net.Dial and http.DefaultClient.
I already did do this, and I can't find anywhere else where it's wrong. Am I missing something? Regardless, I did add more documentation. |
Nope. I left that comment just before you pushed the commit with doc updates. Updates look great! Thanks! |
Implements address family policy. When the policy is set to
PreferIPv4orPreferIPv6, any DNS result that has both IPv4 and IPv6 records will be filtered to only the preferred records; in cases where only IPv4 or IPv6 addresses are present, this has no impact. The default is set toPreferIPv4to help lower the likelihood of problems due to the fact thathttplblacks some kind of implementation of a "happy eyeballs" algorithm to perform IPv6 fallback.It winds up being a little tricky to test this, but it's probably worthwhile; right now we can only test the DNS resolver in a fairly limited way (by relying on the fact that resolving an IP address is a no-op.) With this approach, we can test the DNS resolver more-or-less end-to-end.