Refactor dns cluster api

[Stevenjin8]

TODO:

• Create new DnsConfig proto
• Update deprecation docs
• Update strict dns cluster to consume new dns config
• Update logical dns cluster to consume new dns config
• Test changes

Commit Message: Refactor DNS cluster configuration in its own extension
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #36353 was opened by Stevenjin8.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #36353 was opened by Stevenjin8.

see: more, trace.

[Stevenjin8]

@markdroth @wbpcode Just wanted to check in that I'm going in the right direction

[Stevenjin8]

@markdroth I would also appreciate some guidance on what you expect for testing.

[mattklein123]

Sorry can you provide some context on this?

/wait

[markdroth]

@mattklein123 For context, see #35479 (comment).

@Stevenjin8, I'm not the right person to ask about testing for the Envoy implementation, so I'll let one of the Envoy maintainers do that. My feedback here is just on the xDS API changes.

Please let me know if you have any questions. Thanks!

[Stevenjin8]

/retest

[markdroth]

This looks great from an API perspective!

I'll let one of the Envoy maintainers weigh in for the actual implementation review.

[markdroth]

From an API perspective, this is really hard to understand. What does "logical DNS resolution" mean?

I think we should instead use something like what I suggested earlier:

// If true, all returned addresses are considered to be associated with a single endpoint.
// Otherwise, each address is considered to be a separate endpoint.
bool all_addresses_in_single_endpoint = 9;

[Stevenjin8]

I agree that "logical" is confusing, but I also don't see how "all_addresses_in_single_endpoint" captures the semantics of logical vs strict dns clusters. I could be wrong, but my understanding is as follows:

[logical dns] is optimal for large scale web services that must be accessed via DNS. Such services typically use round robin DNS to return many different IP addresses. Typically a different result is returned for each query. If strict DNS were used in this scenario, Envoy would assume that the cluster’s members were changing during every resolution interval which would lead to draining connection pools, connection cycling, etc. Instead, with logical DNS, connections stay alive until they get cycled.

It seems to me that the crucial difference between is how existing hosts are treated after a dns query, not whether all addresses are in a single endpoint (the docs don't even mention the latter). Some people may even want strict dns resolution when all addresses are in a single endpoint.

[markdroth]

I'm not familiar with the Envoy implementation, but from having implemented LOGICAL_DNS support in gRPC, I think the key difference is actually the fact that all addresses are considered part of a single endpoint.

In a LOGICAL_DNS cluster, Envoy creates a single Host (which is the object it uses internally to represent an endpoint) with all addresses. Since there is only one Host, all requests will always be sent to that Host, regardless of which LB policy is used. Every time the Host is asked for a new connection, it uses whichever address is currently at the front of the list.

In a STRICT_DNS cluster, we create a separate Host for each address. The LB policy is used to pick which Host each request should be sent to, and the chosen Host manages connections to that address.

@mattklein123 can correct me if I'm wrong here, but the above is my understanding.

[Stevenjin8]

@markdroth I think I see what you mean. Logical DNS clusters have a single Host with one address. This address may change based on results of DNS queries.

My question for @mattklein123 (or anyone familiar with the matter) is why the requirement for there to be a single host? Is there a fundamental reason why we can't have a config like

      name: my_cluster
      type: LOGICAL_DNS
      lb_policy: ROUND_ROBIN 
      hosts:
        - socket_address:
            address: eastus.service.com
            port_value: 80
        - socket_address:
            address: westus.service.com
            port_value: 80       

This would create two Hosts under the hood, one for eastus.service.com and one for westus.service.com, and envoy would round robin between them. When any Host is asked for an address, it would return the one "at the front of [its] list".

[markdroth]

In principle, I don't see any reason why we couldn't add a mechanism to do something like that, but it's not something we have today, and we probably wouldn't want to actually do that without a concrete requirement.

But I also think that that feature isn't really relevant to the question of how we indicate the difference between LOGICAL_DNS and STRICT_DNS clusters. If we were going to introduce the ability for a single DNS cluster to look up multiple names, we could do it for both types, but there would still be the same difference in behavior between LOGICAL_DNS and STRICT_DNS: I would expect STRICT_DNS to still create a separate Host for each address, and we could define the LOGICAL_DNS behavior to be either (a) have one Host for each name, each of which has all of the addresses for that name, or (b) have a single Host containing all addresses for both names -- which behavior we chose would depend on the concrete use-case we'd be trying to address.

[markdroth]

From the perspective of the xDS data model and familiarity with the gRPC implementation, I think that's the only difference. However, I'm not familiar with Envoy's implementation, which is why I was asking for confirmation.

If you don't know, is there someone else we can ask who is more familiar with the Envoy implementation? I just want to make sure we're not introducing a misleading API here.

Thanks!

[mattklein123]

I would have to page it all back in and look. I can try to find some time for that.

[markdroth]

Unless anyone can think of any reason why bool all_addresses_in_single_endpoint doesn't work, I'd like to see this changed. I really don't think bool logical makes sense as an API here.

[Stevenjin8]

@markdroth sounds good

[Stevenjin8]

@markdroth I think we've resolved the last outstanding item. With your and @mattklein123 's approval, I'm ready to merge whenever.

[markdroth]

Do these fields still need to be used for other cluster types, like redis? If so, we probably can't mark them as deprecated yet.

[Stevenjin8]

Both redis and dynamic forward proxies define their own refresh rates

envoy/api/envoy/extensions/clusters/redis/v3/redis_cluster.proto

Lines 58 to 85 in 983545f

	message RedisClusterConfig {
	option (udpa.annotations.versioning).previous_message_type =
	"envoy.config.cluster.redis.RedisClusterConfig";
	// Interval between successive topology refresh requests. If not set, this defaults to 5s.
	google.protobuf.Duration cluster_refresh_rate = 1 [(validate.rules).duration = {gt {}}];
	// Timeout for topology refresh request. If not set, this defaults to 3s.
	google.protobuf.Duration cluster_refresh_timeout = 2 [(validate.rules).duration = {gt {}}];
	// The minimum interval that must pass after triggering a topology refresh request before a new
	// request can possibly be triggered again. Any errors received during one of these
	// time intervals are ignored. If not set, this defaults to 5s.
	google.protobuf.Duration redirect_refresh_interval = 3;
	// The number of redirection errors that must be received before
	// triggering a topology refresh request. If not set, this defaults to 5.
	// If this is set to 0, topology refresh after redirect is disabled.
	google.protobuf.UInt32Value redirect_refresh_threshold = 4;
	// The number of failures that must be received before triggering a topology refresh request.
	// If not set, this defaults to 0, which disables the topology refresh due to failure.
	uint32 failure_refresh_threshold = 5;
	// The number of hosts became degraded or unhealthy before triggering a topology refresh request.
	// If not set, this defaults to 0, which disables the topology refresh due to degraded or
	// unhealthy host.
	uint32 host_degraded_refresh_threshold = 6;

envoy/api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

Line 65 in 983545f

google.protobuf.Duration dns_refresh_rate = 3

Couldn't find any other ones.

[markdroth]

This looks really good to me!

[mattklein123]

Thanks this looks great. Please merge main and we can get this in.

/wait

[mattklein123]

Can you write more what the difference here is for users and/or link back to the arch docs where this is discussed?

[Stevenjin8]

Added links to avoid repetition, but I can also be a bit more verbose.

[Stevenjin8]

Forgot to merge main. Should be ready for another review now (even though not much changed)

[Stevenjin8]

/retest

[markdroth]

Just one minor comment improvement needed -- otherwise, this looks great from an API perspective!

[markdroth]

This looks great from an API perspective!

/lgtm api