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

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.Cluster"
        srv_names:
          - 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)

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 · 2024-11-15T04:35:11Z

I'll sponsor this extension. I'll take a closer look at the PR early next week.

mikhainin · 2024-11-17T19:37:27Z

Hi @ggreenway, Thank you for the response.
Just in case, I'm happy to move this extension to contrinb if someone feels it would be a better move.

ggreenway

A good next step is to write more tests. I'd focus first on an integration test.

ggreenway · 2024-11-18T21:47:30Z

api/envoy/extensions/clusters/dns_srv/v3/cluster.proto

+}
+
+message SrvName {
+  string srv_name = 1;


Do you anticipate adding additional fields to this message in the future?

ggreenway · 2024-11-19T17:02:49Z

mobile/envoy_build_config/extensions_build_config.bzl

@@ -4,6 +4,7 @@ EXTENSION_CONFIG_VISIBILITY = ["//visibility:public"]
 EXTENSION_PACKAGE_VISIBILITY = ["//visibility:public"]
 EXTENSIONS = {
    "envoy.clusters.dynamic_forward_proxy":                "//source/extensions/clusters/dynamic_forward_proxy:cluster",
+    "envoy.clusters.dns_srv":                              "//source/extensions/clusters/dns_srv:cluster",


@envoyproxy/envoy-mobile-triage is it correct to add this extension here?

ggreenway · 2024-11-19T17:04:18Z

source/extensions/clusters/dns_srv/dns_srv_cluster.h

+
+  // One SRV-record may return several hostnames
+  // We will need to resolve all of the hostnames to IPs. Potentially,
+  // There can be several hostnames for each hostname.


Do you mean "there can be several IPs for each hostname"?

luckyxkw

Hi glad you initiated this PR! We have been doing a similar customization in X (i.e. add a resolveSrv method in DNS resolver, and a new cluster type that uses the new resolveSrc method) for quite some time.

I am not really an expert in making envoy changes. The code is inherited from my ex-colleague. But I am happy to collaborate if you want.

luckyxkw · 2024-12-02T07:45:41Z

source/extensions/network/dns_resolver/cares/dns_impl.cc

+    // in DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback()).
+    pending_srv_res->owned_ = true;
+    return pending_srv_res.release();
+    return nullptr;


Nit: return nullptr can be removed?

luckyxkw · 2024-12-02T08:35:49Z

source/extensions/network/dns_resolver/cares/dns_impl.cc

+    if (parse_srv_status == ARES_SUCCESS) {
+      for (ares_srv_reply* current_reply = srv_reply; current_reply != NULL;
+           current_reply = current_reply->next) {
+      }


Did you plan to put anything in the loop block?

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, alyssawilk, abeyad, 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>

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 31, 2024

ggreenway self-assigned this Nov 15, 2024

ggreenway requested changes Nov 19, 2024

View reviewed changes

ggreenway added waiting waiting:any and removed waiting labels Nov 20, 2024

luckyxkw reviewed Dec 2, 2024

View reviewed changes

repokitteh-read-only bot removed waiting:any labels Dec 2, 2024

mikhainin added 2 commits December 3, 2024 20:54

Merge branch 'main' into srv-record

a49e36a

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

Merge with main

22c021f

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

ggreenway added the waiting label Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mikhainin commented Jul 11, 2024 •

edited

Loading

repokitteh-read-only bot commented Jul 11, 2024

repokitteh-read-only bot commented Jul 11, 2024

mikhainin commented Jul 22, 2024

mikhainin commented Aug 19, 2024

wbpcode commented Aug 21, 2024

mikhainin commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024

mikhainin commented Sep 26, 2024

github-actions bot commented Oct 26, 2024

ggreenway commented Nov 15, 2024

mikhainin commented Nov 17, 2024

ggreenway left a comment

ggreenway Nov 18, 2024

ggreenway Nov 19, 2024

ggreenway Nov 19, 2024

luckyxkw left a comment

luckyxkw Dec 2, 2024

luckyxkw Dec 2, 2024

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

Are you sure you want to change the base?

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

Conversation

mikhainin commented Jul 11, 2024 • edited Loading

repokitteh-read-only bot commented Jul 11, 2024

repokitteh-read-only bot commented Jul 11, 2024

mikhainin commented Jul 22, 2024

mikhainin commented Aug 19, 2024

wbpcode commented Aug 21, 2024

mikhainin commented Aug 21, 2024 • edited Loading

github-actions bot commented Sep 20, 2024

mikhainin commented Sep 26, 2024

github-actions bot commented Oct 26, 2024

ggreenway commented Nov 15, 2024

mikhainin commented Nov 17, 2024

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway Nov 18, 2024

Choose a reason for hiding this comment

ggreenway Nov 19, 2024

Choose a reason for hiding this comment

ggreenway Nov 19, 2024

Choose a reason for hiding this comment

luckyxkw left a comment

Choose a reason for hiding this comment

luckyxkw Dec 2, 2024

Choose a reason for hiding this comment

luckyxkw Dec 2, 2024

Choose a reason for hiding this comment

mikhainin commented Jul 11, 2024 •

edited

Loading

mikhainin commented Aug 21, 2024 •

edited

Loading