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

Implement address family affinity and prefer IPv4 by default #66

Merged
merged 7 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var (
KeepAlive: 30 * time.Second,
}
defaultNameTTL = 5 * time.Minute
defaultResolver = resolver.NewDNSResolver(net.DefaultResolver, "ip", defaultNameTTL)
defaultResolver = resolver.NewDNSResolver(net.DefaultResolver, resolver.PreferIPv4, defaultNameTTL)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

)

// Client is an HTTP client that supports configurable client-side load
Expand Down
5 changes: 3 additions & 2 deletions doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@
// 1. The client will re-resolve addresses in DNS every 5 minutes.
// The http.DefaultClient does not re-resolve predictably.
//
// 2. The client will route requests in a round-robin fashion to all
// addresses returned by the DNS system (both A and AAAA records).
// 2. The client will route requests in a round-robin fashion to the
// addresses returned by the DNS system, preferring A records if
// present but using AAAA records if no A records are present,
// even with HTTP/2.
//
// This differs from the http.DefaultClient, which will use only a
Expand Down
86 changes: 76 additions & 10 deletions resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,43 @@ import (
"context"
"io"
"net"
"net/netip"
"time"

"github.com/bufbuild/httplb/attribute"
"github.com/bufbuild/httplb/internal"
)

// AddressFamilyPolicy is an option that allows control over the preference
// for which addresses to consider when resolving, based on their address
// family.
type AddressFamilyPolicy int

const (
// PreferIPv4 will result in only IPv4 addresses being used, if any
// IPv4 addresses are present. If no IPv4 addresses are resolved, then
// all addresses will be used.
PreferIPv4 AddressFamilyPolicy = iota

// RequireIPv4 will result in only IPv4 addresses being used. If no IPv4
// addresses are present, no addresses will be resolved.
RequireIPv4

// PreferIPv6 will result in only IPv6 addresses being used, if any
// IPv6 addresses are present. If no IPv6 addresses are resolved, then
// all addresses will be used.
PreferIPv6

// RequireIPv6 will result in only IPv6 addresses being used. If no IPv6
// addresses are present, no addresses will be resolved.
RequireIPv6

// PreferIPv6 will result in only IPv6 addresses being used, if any
// UseBothIPv4AndIPv6 will result in all addresses being used, regardless of
// their address family.
UseBothIPv4AndIPv6
)

// Resolver is an interface for continuous name resolution.
type Resolver interface {
// New creates a continuous resolver task for the given target name. When
Expand Down Expand Up @@ -102,21 +133,19 @@ type Address struct {
Attributes attribute.Values
}

// NewDNSResolver creates a new resolver that resolves DNS names.
// You can specify which kind of network addresses to resolve with the network
// parameter, and the resolver will return only IP addresses of the type
// specified by network. The network must be one of "ip", "ip4" or "ip6".
// Note that because net.Resolver does not expose the record TTL values, this
// resolver uses the fixed TTL provided in the ttl parameter.
// NewDNSResolver creates a new resolver that resolves DNS names. The specified
// address family policy value can be used to require or prefer either IPv4 or
// IPv6 addresses. Note that because net.Resolver does not expose the record
// TTL values, this resolver uses the fixed TTL provided in the ttl parameter.
func NewDNSResolver(
resolver *net.Resolver,
network string,
policy AddressFamilyPolicy,
ttl time.Duration,
) Resolver {
return NewPollingResolver(
&dnsResolveProber{
resolver: resolver,
network: network,
policy: policy,
},
ttl,
)
Expand All @@ -138,7 +167,7 @@ func NewPollingResolver(

type dnsResolveProber struct {
resolver *net.Resolver
network string
policy AddressFamilyPolicy
}

func (r *dnsResolveProber) ResolveOnce(
Expand All @@ -157,10 +186,12 @@ func (r *dnsResolveProber) ResolveOnce(
port = "80"
}
}
addresses, err := r.resolver.LookupNetIP(ctx, r.network, host)
network := networkForAddressFamilyPolicy(r.policy)
addresses, err := r.resolver.LookupNetIP(ctx, network, host)
if err != nil {
return nil, 0, err
}
addresses = applyAddressFamilyPolicy(addresses, r.policy)
result := make([]Address, len(addresses))
for i, address := range addresses {
result[i].HostPort = net.JoinHostPort(address.Unmap().String(), port)
Expand Down Expand Up @@ -248,3 +279,38 @@ func (task *pollingResolverTask) run(ctx context.Context, scheme, hostPort strin
}
}
}

func networkForAddressFamilyPolicy(policy AddressFamilyPolicy) string {
switch policy {
case PreferIPv4, PreferIPv6, UseBothIPv4AndIPv6:
return "ip"
case RequireIPv4:
return "ip4"
case RequireIPv6:
return "ip6"
}
return ""
}

func applyAddressFamilyPolicy(addresses []netip.Addr, policy AddressFamilyPolicy) []netip.Addr {
var check func(netip.Addr) bool
required := policy == RequireIPv4 || policy == RequireIPv6
switch policy {
case PreferIPv4, RequireIPv4:
check = func(address netip.Addr) bool { return address.Is4() || address.Is4In6() }
case PreferIPv6, RequireIPv6:
check = func(address netip.Addr) bool { return address.Is6() && !address.Is4In6() }
case UseBothIPv4AndIPv6:
return addresses
}
matchingAddresses := addresses[:0]
for _, address := range addresses {
if check(address) {
matchingAddresses = append(matchingAddresses, address)
}
}
if required || len(matchingAddresses) > 0 {
addresses = matchingAddresses
}
return addresses
}
Loading
Loading