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!

[markdroth]

We don't need a new extension point here. The DNS cluster config should instead be loaded via the existing cluster_type field:

envoy/api/envoy/config/cluster/v3/cluster.proto

Line 833 in 1173629

CustomClusterType cluster_type = 38;

[Stevenjin8]

@markdroth Oh I see, so we are adding relevant DNS fields to the cluster config types of each cluster type. So I would add a api/envoy/extensions/clusters/strict_dns/v3/cluster.proto file with the corresponding fields. and same for logical_dns, etc

[markdroth]

That would work. But it may also be the case that there's enough in common (config-wise and/or implementation-wise) between STRICT_DNS and LOGICAL_DNS that we can combine them into a single DNS cluster type, and the config for that DNS cluster type can have a field that says whether it uses STRICT or LOGICAL behavior.

[Stevenjin8]

@markdroth wait... So would I add an extension point to the typed config. So the configuration would like

  clusters:
  - name: my_cluster
    ...
    cluster_type:
      name: envoy.clusters.strict_dns
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.clusters.strict_dns.v3.ClusterConfig
      dns_config:
         dns_jitter: false
         ... the rest of the dns_* fields

[markdroth]

The new DNS cluster type should not use any of the existing dns_* fields in the Cluster message; those fields should all be copied into the new envoy.extensions.clusters.dns_cluster.v3.DnsCluster message.

The existing dns_* fields in the Cluster message should be used only for backward-compatibility reasons, for existing Cluster resources that use STRICT_DNS or LOGICAL_DNS and configure these existing fields. However, Envoy should internally convert such a config into the corresponding new-style config that uses the new envoy.extensions.clusters.dns_cluster.v3.DnsCluster message.

[Stevenjin8]

In the above example, I just worry that we are forcing both strict and logical dns to share the same typed_cofig

[Stevenjin8]

Oh I messed up the indentation in the first example. Sorry! I meant

  clusters:
  - name: my_cluster
    ...
    cluster_type:
      name: envoy.clusters.strict_dns
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.clusters.strict_dns.v3.ClusterConfig
        dns_config:
           dns_jitter: false
           ... the rest of the dns_* fields

[markdroth]

In the above example, I just worry that we are forcing both strict and logical dns to share the same typed_cofig

Why would that a problem, as long as there's still a way to configure the behavior for each? My suspicion is that if we had separate config messages for STRICT_DNS and LOGICAL_DNS, then both messages would have almost identical sets of fields. If that's the case, then the duplication doesn't really make sense, which is why I suggested having a single message that can be used for both with a knob that controls which behavior is used.

The type present in the type_url field dictates which cluster implementation gets used, so if we have two different implementations for STRICT_DNS and LOGICAL_DNS, then we would need probably-identical config messages, like this:

message StrictDnsCluster {
  // ...a bunch of DNS fields...
}

message LogicalDnsCluster {
  // ...exactly the same set of DNS fields...
}

In contrast, if we have a single cluster implementation for both DNS types with a knob to control the behavior, we can have a single config proto, like this:

message DnsCluster {
  // if true, STRICT_DNS semantics; otherwise, LOGICAL_DNS semantics
  // (could also be an enum or whatever)
  bool strict = N;

  // ...the set of DNS fields...
}

[Stevenjin8]

@markdroth I agree that most fields are Dns-related and would thus be duplicated, but I was thinking something along the lines of

message DnsConfig {
   // ... DNS fields
}

message StrictDnsCluster {
  DnsConfig dns_config = N
}

message LogicalDnsCluster {
  DnsConfig dns_config = N
}

This way we don't have duplicate code and

1. In the future, if we want to add a logical dns-specific field, it won't appear in the strict dns configuration (same for strict dns).
2. It follows the general pattern (AFAIK) of each cluster type having its own config type. We see this pattern for filters too.

Let me know what you think. I'm still new to envoy so if you feel that having a shared config type is the way to go, I'm happy to implement it.

[markdroth]

My inclination is that we're unlikely to have too many fields that are specific to either STRICT_DNS or LOGICAL_DNS, so combining them makes the most sense. But I'd be open to input from @wbpcode or other Envoy maintainers.

[repokitteh-read-only]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #36353 was synchronize by Stevenjin8.

see: more, trace.

[Stevenjin8]

Sorry everybody I messed up my git history

[Stevenjin8]

@markdroth I think I'm ready for another review. I'm hoping CI is passing now.

[Stevenjin8]

/help

[Stevenjin8]

/retest

[Stevenjin8]

/assign @Stevenjin8

[Stevenjin8]

delete this