[WIP] dns: add support for clusters based on SRV DNS record by mikhainin · Pull Request #35160 · envoyproxy/envoy

mikhainin · 2024-07-11T20:11:45Z

One more attempt to address #125 (resolve cluster IPs via SRV-record)

Yet on early stage. Tested with Consul and the collowing configuration:

  clusters:
  - name: local-php
    connect_timeout: 0.25s
    lb_policy: round_robin
    # http2_protocol_options: {}
    cluster_type:
      name: envoy.clusters.dns_srv
      typed_config:
        "@type": "type.googleapis.com/envoy.extensions.clusters.dns_srv.v3.DnsSrvClusterConfig"
        srv_name: _local-php._tcp.service.consul.
    dns_resolvers:
    typed_dns_resolver_config:
      name: envoy.network.dns_resolver.cares
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig
        resolvers:
          - socket_address:
              address: 127.0.0.1
              port_value: 8600

Based on discussion and design doc by @kylebevans in #125 (comment)

Current limitations:

Only one srv_name
Resolution is strictly sequentional: First SRV record, Envoy will try resolving A-record for each target. The whole process will repeat after dns_refresh_rate
weight and priority are ignored (going to support them in the future PRs)
All resolved IP addresses will be addedd to cluster (acting like Strict DNS cluster).
Both initial SRV record and subsequent A/AAAA records resolved via the same DNS resolver.

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

repokitteh-read-only · 2024-07-11T20:11:50Z

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: #35160 was opened by mikhainin.

see: more, trace.

repokitteh-read-only · 2024-07-11T20:11:55Z

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

🐱

Caused by: #35160 was opened by mikhainin.

see: more, trace.

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin · 2024-07-22T19:56:04Z

Testing locally with Consul:
run consul:

consul agent -server -advertise=127.0.0.1 -data-dir=consul-data -ui -bootstrap-expect=1 -dev -log-level=debug

Add service nodes:

consul services register -name=local-php -id=web-1 -address=192.168.1.102 -port=80 -tag=v1 -tag=prod
consul services register -name=local-php -id=web-2 -address=192.168.1.102 -port=81 -tag=v1 -tag=prod

Verify that the service local-php can be resolved via Consul:

dig @127.0.0.1 -p 8600 local-php.service.consul. SRV
# or
dig @127.0.0.1 -p 8600 _local-php._tcp.service.consul. SRV

main doc: https://developer.hashicorp.com/consul/docs/services/discovery/dns-static-lookups

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin · 2024-08-19T19:39:10Z

Hi there, apologies for openning in this state - I acknowledge that there still lots.
I just needed someone to point me at what I'm missing and get feedback whether I'm going in the right direction.

wbpcode · 2024-08-21T12:54:07Z

Thanks for you contribution. But based on our contributing rules (see https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md), any extension, should have a maintainer sponsor to help review and maintain the code.

And I think this should be a DNS extension rather than a new cluster extension?

mikhainin · 2024-08-21T19:45:03Z

should have a maintainer sponsor to help review and maintain the code

@wbpcode, Thank you for your comment. I posted a message in slack some while ago but didn't hear anything back. What would be the right way for this?

And I think this should be a DNS extension rather than a new cluster extension?

Based on previous discussion, it was decided to go with the cluster extension. Basically, I followed the previous proposal.

I will be happy to discuss the new design if someone helps me to find the right place. The initial issue doesn't seem to have activity :/

github-actions · 2024-09-20T20:01:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

mikhainin · 2024-09-26T13:46:31Z

This isn't yet forgotten, going to ping maintainers in the maillist

github-actions · 2024-10-26T16:01:20Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

ggreenway · 2026-04-28T23:56:06Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new cluster type, envoy.clusters.dns_srv, enabling service discovery through DNS SRV records. It extends the DnsResolver interface with a resolveSrv method and provides a concrete implementation for the c-ares resolver. The DnsSrvCluster manages a two-stage resolution process, first retrieving SRV records and then resolving the resulting hostnames into IP addresses. Feedback identifies a critical bug where the cluster fails to remove stale hosts because the current host list is not correctly maintained across updates. Additionally, the reviewer suggests incorporating the SRV record's TTL into the cluster refresh logic and removing the hardcoded restriction that limits SRV support exclusively to the c-ares resolver extension.

gemini-code-assist · 2026-04-29T00:02:11Z

+          for (const auto& dns : response) {
+
+            ENVOY_LOG(debug, "SRV: host: {}, port: {}, weight: {}, prio: {}", dns.srv().target_,
+                      dns.srv().port_, dns.srv().weight_, dns.srv().priority_);
+
+            if (auto address = Envoy::Network::Utility::parseInternetAddressNoThrow(
+                    dns.srv().target_, 0, false);
+                address != nullptr) {
+              // SRV record target is an IP address, not a hostname.
+              ResolveTargetPtr target = std::make_unique<ResolveTarget>(
+                  *active_resolve_list_, dns_resolver_, dns_lookup_family_, dns.srv().target_,
+                  dns.srv().priority_, dns.srv().weight_, dns.srv().port_);
+
+              active_resolve_list_->addResolvedTarget(std::move(target), address);
+            } else {
+              active_resolve_list_->addTarget(std::make_unique<ResolveTarget>(
+                  *active_resolve_list_, dns_resolver_, dns_lookup_family_, dns.srv().target_,
+                  dns.srv().priority_, dns.srv().weight_, dns.srv().port_));
+            }
+          }


The SRV record's TTL should be tracked to influence the cluster's refresh rate. Currently, only the TTLs from the subsequent A/AAAA lookups are used. If the SRV record itself has a shorter TTL, the cluster might use stale targets.

If it is simpler, I think for now you could take the minimum of the TTLs from A/AAAA vs SRV response, and use that

gemini-code-assist · 2026-04-29T00:02:11Z

+  if (cluster.typed_dns_resolver_config().name() != "envoy.network.dns_resolver.cares") {
+    return absl::InvalidArgumentError(
+        fmt::format("Only c-ares supports resolve of SRV records, "
+                    "please use typed_dns_resolver_config.name = "
+                    "'envoy.network.dns_resolver.cares'. Current value: '{}'",
+                    cluster.typed_dns_resolver_config().name()));
+  }


Hardcoding a check for the c-ares resolver name is restrictive. While currently only c-ares supports SRV in this implementation, it would be better to check for a capability or interface support rather than a specific extension name, allowing for future or custom resolvers that might support SRV.

If an out-of-tree resolver supported SRV, this code would disallow it, so it would be good to fix this

What is "out-of-tree resolver"? I thought, we could add other resolvers in the following change sets...

ggreenway

/wait

ggreenway · 2026-04-29T00:26:23Z

+
+  ASSERT_TRUE(response->complete());
+  EXPECT_EQ("200", response->headers().getStatusValue());
+}


Please add an integration test that adds and removes hosts via srv response, as well as via changed A/AAAA response.

ggreenway · 2026-04-29T00:28:38Z

+  EXPECT_CALL(dns_resolver_factory_, createDnsResolver(_, _, _))
+      .WillRepeatedly(testing::Return(dns_resolver));
+
+  EXPECT_CALL(*dns_resolver, resolve(_, _, _))


This isn't really an integration test if it doesn't use a real resolver, with asyncronous resolution. Can you refactor this to use c-ares, and make the test respond with real dns responses?

Hey @ggreenway,
I think this one deserves a bit of discussion (and probably help?).
As far as i understand, this patter is being used in test/extensions/clusters/common/logical_host_integration_test.cc, that's how it appeared here.

There is a FakeUdpDnsServer, which I think is used in benchmark only.

source/extensions/network/dns_resolver/cares/dns_impl.cc already does some sort of network communication, and we could extract it to some common place to reuse between tests, but this looks like a significant change.

How would you suggest to approach?

ggreenway · 2026-04-29T00:43:38Z

+  friend class DnsSrvClusterFactory;
+  friend class DnsSrvClusterTest;


Can you refactor the tests so the friend isn't needed? Maybe add accessors for the data you need in the test?

Hmm, this looks like a common pattern for clusters. But if you think, it worth changing, I can give it a try...

ggreenway · 2026-04-29T00:44:01Z

+namespace Clusters {
+  class DnsSrvClusterTest;
+  class DnsSrvClusterTest_CreateClusterWithMinimalConfig_Test;
+}
+}
+
+namespace Upstream {
+
+class DnsSrvClusterFactory;
+class DnsSrvClusterTest;


These are declared in different namespaces

Co-authored-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Mikhail Galanin <195510+mikhainin@users.noreply.github.com>

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

ggreenway

Claude code says the following test cases are missing:

  Missing Test Cases

  Resolution lifecycle:
  1. SRV failure at cluster level — resolveSrv returns ResolutionStatus::Failure. Verify update_failure_ incremented,   timer armed, onPreInitComplete called.
  2. Empty SRV response — SRV succeeds with zero records. Cluster should end up with no hosts.
  3. All A/AAAA queries fail — SRV returns 3 targets, all address resolutions fail. some_targets_resolved stays false   — verify host list is NOT updated (existing hosts preserved? or empty?).
  4. Multiple IPs per hostname — One SRV target hostname resolves to 3 A records. Verify all 3 become hosts with the
  same port/priority/weight.

  Timer and re-resolution:
  5. Timer re-resolve — After initial resolution, verify timer fires and triggers a new SRV cycle.
  6. Re-resolve changes hosts — First cycle returns hosts A, B. Second cycle returns B, C. Verify A removed, C added,   B kept.
  7. TTL respect — With respect_dns_ttl: true, verify timer is armed with the minimum TTL from A/AAAA responses.
  8. SRV TTL — Even with the current design (SRV TTL ignored), a test documenting this behavior would be useful.

  Destruction and cancellation:
  9. Cluster destroyed during SRV query — Verify no crash/leak (exercises ~DnsSrvCluster cancel path).
  10. Cluster destroyed during A/AAAA queries — Verify ResolveTarget destructors cancel outstanding queries cleanly.

  Edge cases:
  11. Duplicate addresses — Two different SRV hostnames resolve to the same IP:port. Verify deduplication
  (all_new_hosts logic).
  12. Weight=0 — SRV record with weight 0. Verify host is created and functional.
  13. High SRV priority values — Priority=10, 20. Verify hosts end up at the correct priority levels (or document
  that priorities > 0 are broken).
  14. Synchronous DNS completion — Mock dns_resolver_->resolve() to return nullptr (synchronous). Verify the
  resolution still completes correctly.

  Factory/Config validation:
  15. Non-c-ares resolver rejected — Config with getaddrinfo resolver should fail with an error.
  16. Missing typed_dns_resolver_config — What happens if the cluster config doesn't specify a resolver? The name
  check will reject "".
  17. Proto validation — Empty srv_name rejected by validate.rules.

  c-ares level:
  18. SRV cancellation — Start SRV query, cancel it, verify no callback and clean destruction.
  19. ARES_EDESTRUCTION during SRV — Resolver destroyed while SRV query pending (currently crashes due to null dnsrec   dereference).
  20. Multiple SRV records in DNS response — Verify all records are parsed and returned.
  21. SRV with CNAME indirection — DNS returns CNAME then SRV records.

  The most critical missing tests are #9/#10 (destruction safety), #3 (all failures), and #5/#6 (re-resolution).
  These exercise the paths most likely to have lifetime bugs in production.

/wait

ggreenway · 2026-05-08T16:10:33Z

+void DnsResolverImpl::PendingSrvResolution::onAresSrvCallback(ares_status_t status, size_t timeouts,
+                                                              const ares_dns_record_t* dnsrec) {
+
+  unsigned int aresAnswerQueryId = ares_dns_record_get_id(dnsrec);


I think dnsrec can be null here, and this would crash

Looking at the source, it looks it won't crash here. But it will return zero, which is a valid query_id and will crash on the assertion below (when assertions enabled). Updated implementation, considering dnsrec == nullptr as a non-successful result.

ggreenway · 2026-05-08T16:22:04Z

+          target->weight_, constLocalitySharedPool()->getObject(locality_lb_endpoints.locality()),
+          lb_endpoint.endpoint().health_check_config(), target->priority_,
+          lb_endpoint.health_status());


The PR comments say weight and priority aren't implemented yet, but you're at least half-implementing them here. I think you should set weight and priority to zero, and make a followup PR to implement them fully.

ggreenway · 2026-05-08T16:23:29Z

+    void addResolvedTarget(Network::Address::InstanceConstSharedPtr address);
+
+    ResolveList& parent_;
+    Network::DnsResolverSharedPtr dns_resolver_;


I think this is unused and can be deleted

I think, it is used in DnsSrvCluster::ResolveTarget::startResolve()...

ggreenway · 2026-05-08T16:25:08Z

 dereferencing
 deprioritized
 differentially
+dnsrec


You can put terms like this in backticks in comments so they don't need to be added to the dictionary

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

[WIP] dns: add support for clusters based on SRV DNS record

bfd2d86

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

repokitteh-read-only Bot added the api label Jul 11, 2024

repokitteh-read-only Bot assigned wbpcode Jul 11, 2024

mikhainin added 4 commits July 13, 2024 12:26

Resolve A-records for each SRV hostname

73f9513

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

Cleanup

a8e0b5e

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

Cleanup

a2334a8

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

Report failure status

e60757b

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin mentioned this pull request Jul 22, 2024

SRV service discovery support #125

Open

Polishing

fc7d0f5

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin marked this pull request as ready for review August 19, 2024 19:39

mikhainin requested review from RyanTheOptimist, abeyad, alyssawilk, fredyw, mattklein123 and yanavlasov as code owners August 19, 2024 19:39

wbpcode marked this pull request as draft August 21, 2024 12:49

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 20, 2024

github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 26, 2024

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 26, 2024

Update success counter

31c3498

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin force-pushed the srv-record branch from 8674919 to ed7def7 Compare April 26, 2026 11:00

2 more tests

5c0c951

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

gemini-code-assist Bot reviewed Apr 29, 2026

View reviewed changes

ggreenway requested changes Apr 29, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 29, 2026

Update source/extensions/clusters/dns_srv/dns_srv_cluster.cc

5479bfa

Co-authored-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Mikhail Galanin <195510+mikhainin@users.noreply.github.com>

repokitteh-read-only Bot removed the waiting label Apr 29, 2026

mikhainin and others added 6 commits April 29, 2026 19:46

Update envoy/network/dns.h

d6de8d5

Co-authored-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Mikhail Galanin <195510+mikhainin@users.noreply.github.com>

Update source/extensions/clusters/dns_srv/dns_srv_cluster.h

b9396ac

Co-authored-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Mikhail Galanin <195510+mikhainin@users.noreply.github.com>

style fix

5dd6245

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

review fixes

4b5e520

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

format

8a8d30e

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

Fix test

a3bafcd

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin force-pushed the srv-record branch from 4ee79c2 to a3bafcd Compare May 2, 2026 11:34

Add missing dependencies

333315e

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

mikhainin force-pushed the srv-record branch from 412886e to 333315e Compare May 2, 2026 13:45

ggreenway requested changes May 8, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label May 8, 2026

Fixed test

c56a981

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

repokitteh-read-only Bot removed the waiting label May 15, 2026

mikhainin added 9 commits May 17, 2026 11:16

Change loglevel to warn not to break test in CI

bf49a12

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

fixed tests

94deec4

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

review fixes

ecd803e

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

More tests

78a1ac0

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

More tests: check that "respect_dns_ttl" is considered

56f9b44

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

Removed dead code, add test for channel destruction

2cfc0d4

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

One more test: when DNS returns SRV and non-SRV records

6c1fb5b

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

One more test: failed initial load

3f1d497

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

More tests: check config validation (only c-ares is supported)

f4ab9c1

Signed-off-by: Mikhail Galanin <mikhail.galanin@yahoo.com>

		friend class DnsSrvClusterFactory;
		friend class DnsSrvClusterTest;

Conversation

mikhainin commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jul 11, 2024

Uh oh!

repokitteh-read-only Bot commented Jul 11, 2024

Uh oh!

mikhainin commented Jul 22, 2024

Uh oh!

mikhainin commented Aug 19, 2024

Uh oh!

wbpcode commented Aug 21, 2024

Uh oh!

mikhainin commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 20, 2024

Uh oh!

mikhainin commented Sep 26, 2024

Uh oh!

github-actions Bot commented Oct 26, 2024

Uh oh!

ggreenway commented Apr 28, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhainin May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mikhainin commented Jul 11, 2024 •

edited

Loading

mikhainin commented Aug 21, 2024 •

edited

Loading

mikhainin May 23, 2026 •

edited

Loading