dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the proxy filter by ntgsx92 · Pull Request #16294 · envoyproxy/envoy

ntgsx92 · 2021-05-03T23:34:10Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:
Adding dns_resolvers option to the dns cache used by the dynamic forward proxy filter. This enables users to switch default dns resolvers from /etc/resolv.conf to custom dns resolvers in the proxy filter. By default, this option will be empty which means dns cache will continue to resolve names from servers listed in /etc/resolv.conf.

Additional Description:
None

Risk Level:
Low

Testing:
Unit test added

Docs Changes:
Yes

Release Notes:
Added

Platform Specific Features:
N/A

[Optional Runtime guard:]
[Optional Fixes #Issue]

#15657
[Optional Deprecated:]
[Optional API Considerations:]

…proxy filter Signed-off-by: Sixiang Gu <sgu@twitter.com>

Signed-off-by: Sixiang Gu <sgu@twitter.com>

repokitteh-read-only · 2021-05-03T23:34:15Z

Hi @ntgsx92, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16294 was opened by ntgsx92.

see: more, trace.

repokitteh-read-only · 2021-05-03T23:34:19Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16294 was opened by ntgsx92.

see: more, trace.

adisuissa

Thanks for working on this.
One small comment about the API.

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

malza321 · 2021-05-04T02:10:06Z

For an explanation of how to fill out the fields, please see the relevant section

in PULL_REQUESTS.md

Commit Message:

Adding dns_resolvers option to the dns cache used by the dynamic forward proxy filter. This enables users to switch default dns resolvers from /etc/resolv.conf to custom dns resolvers in the proxy filter. By default, this option will be empty which means dns cache will continue to resolve names from servers listed in /etc/resolv.conf.

Additional Description:

None

Risk Level:

Low

Testing:

Unit test added

Docs Changes:

Yes

Release Notes:

Added

Platform Specific Features:

N/A

[Optional Runtime guard:]

[Optional Fixes #Issue]

#15657

[Optional Deprecated:]

[Optional API Considerations:]

You are my hero!

Signed-off-by: Sixiang Gu <sgu@twitter.com>

adisuissa

API lgtm.
@htuch can you please take a quick look at the API changes?

htuch · 2021-05-06T00:28:15Z

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

+  // server startup. Apple's API only allows overriding DNS resolvers via system settings.
+  // If this setting is not specified, the value defaults to the default
+  // resolver, which uses /etc/resolv.conf for configuration.
+  repeated config.core.v3.Address dns_resolvers = 9;


How does this relate to #16237? Should this be a repeated DnsResolver and DnsResolver has both address and DnsLookupOptions?

Thanks for letting me know about this change.

Looks like we're going to have resolvers, use_tcp_for_dns_lookups and no_defalt_search_domain as the arguments for DnsResolverImpl after #16237 is merged.
Seems like a good time to encapsulate those fields into a single proto message like DnsResolver.

@suniltheta How do you feel about the above changes?

Thanks for letting me know about this PR. Just recently committed all of the changes for the PR #16237. A new protobuf message DnsResolverOptions in introduced which only include bool fields use_tcp_for_dns_lookups & no_default_search_domain.

Notice the name change from DnsLookupOptions to DnsResolverOptions.

Having said that if we move resolvers inside DnsResolverOptions, then we might have to take care of deprecating dns_resolvers present in config/cluster/v3/cluster.proto & dns_filter/v3alpha/dns_filter.proto.

I lean towards keeping the resolvers (repeated Address type) out of dedicated DnsResolverOptions. Only because if we combine we are looking at a slightly bloated PR and deprecation of a field. But I am also flexible in combining them, if doing so is the best option in the long run.

Yeah, I would separate DnsResolverOptions and reference it from a DnsResolver message, which can deal with addressing and other concerns not related to resolution options.

So is this the format of the protobuf message that is being proposed?

FORMAT A:

└── DnsResolver ├── <repeated Address> dns_resolvers └── <DnsResolverOptions> dns_resolver_options ├── <bool> use_tcp_for_dns_lookups ├── <bool> no_default_search_domain └── /<any options added in future>/

After the merge of #16237 we do
dispatcher().createDnsResolver(resolvers, dns_resolver_options);

Instead we would be doing
dispatcher().createDnsResolver(dns_resolvers);
So when we call the function createDnsResolver we would be only passing the object of type DnsResolver. Here dns_resolvers will contain both resolvers and dns_resolver_options.

Yeah, fair point. Let's go with #16294 (comment) then.

@htuch What's the next step? @suniltheta and I discussed merging this PR as it is and make another PR to add the new DnsResolver message.

If we're planning on immediately restructuring in the next PR that isn't so great. Can we at least introduce the DnsResolver message and place the list of addresses in there?

I am on board with this idea as well. We can place the DnsResolver protobuf message in api/envoy/config/core/v3/base.proto or any other place which makes more sense in this PR. Move repeated Address dns_resolvers inside DnsResolver like mentioned in previous comment. Finally as part of #16237 PR we can move the DnsResolverOptions inside the already merged DnsResolver message.

Signed-off-by: Sixiang Gu <sgu@twitter.com>

suniltheta · 2021-05-14T21:20:44Z

api/envoy/config/core/v3/base.proto

+  // Setting this value causes failure if the
+  // ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime value is true during
+  // server startup. Apple's API only allows overriding DNS resolvers via system settings.
+  repeated Address dns_resolvers = 1;


nit: Should we just name it resolvers instead of dns_resolvers ?

Signed-off-by: Sixiang Gu <sgu@twitter.com>

…proxy-main Signed-off-by: Sixiang Gu <sgu@twitter.com>

Signed-off-by: Sixiang Gu <sgu@twitter.com>

malza321 · 2021-05-15T01:49:17Z

hi!

Can I see how to use custom resolvers in advance?
envoy.yaml will be great help to let me understand!

Thanks!!

htuch · 2021-05-16T18:14:06Z

api/envoy/config/core/v3/base.proto

  string identifier = 1;
 }
+
+// DNS resolver configuration which includes the underlying dns resolver addresses and options


Nit: suggest moving this to its own file resolver.proto in api/envoy/config/v3.

While we are at this, what does others think about renaming DnsResolver to DnsResolverConfig ? So that we don't get confused between this message and a class of the same name DnsResolver. Apologies for not bringing this up earlier.

Sure, although I think that's a good idea just because the naming scheme is clearer; avoiding C++ level type conflicts isn't that strong a motivator, we have namespaces etc to deal with that.

is it also possible to foward ip directly without dns query?

I did curl -x http://envoyip:port -kLv http://targetip:port but envoy keep returning "No healthy upstream"

So, definitely open to (maybe it's clearer) to call this DnsResolverConfig or DnsResolutionConfig.

let's say, we can take that up renaming DnsResolver --> DnsResolverConfig or DnsResolutionConfig in the next PR #16237 ? Since all the CI got passed here, this can go?

htuch

API LGTM modulo nit.

Signed-off-by: Sixiang Gu <sgu@twitter.com>

Signed-off-by: Benjamin Peterson <benjamin@dropbox.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

This is a follow-up to: envoyproxy#14432 (comment) After that PR, it's no longer possible (unless you do a dynamic_cast) to set the remote address from a filter. This is something that we need to do because we have specialized logic for this (XFF doesn't work for us). So this adds an extension point which will allow us to push that logic down to ConnectionManagerUtility::mutateRequestHeaders() where it belongs. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

* docs: comment config extension Signed-off-by: Long Dai <long0dai@foxmail.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

…mit hook (envoyproxy#16505) Signed-off-by: Justin Ely <justincely@gmail.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

repokitteh-read-only · 2021-05-20T17:45:46Z

@envoyproxy/first-pass-reviewers assignee is @junr03

🐱

Caused by: a #16294 (review) was submitted by @adisuissa.

see: more, trace.

ntgsx92 · 2021-05-20T22:39:18Z

/retest

repokitteh-read-only · 2021-05-20T22:39:22Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16294 (comment) was created by @ntgsx92.

see: more, trace.

ntgsx92 · 2021-05-21T00:06:28Z

/retest

repokitteh-read-only · 2021-05-21T00:06:32Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16294 (comment) was created by @ntgsx92.

see: more, trace.

ntgsx92 · 2021-05-21T01:38:02Z

Looks like this test case is broken. Not sure why this PR will cause it to break.

ntgsx92 · 2021-05-24T16:28:19Z

@junr03 Hey, mind taking a look at PR? Thanks! 😄

mattklein123

Thanks LGTM with small comments. Please also merge main.

/wait

api/envoy/config/core/v3/resolver.proto

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

api/envoy/config/core/v3/resolver.proto

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

Signed-off-by: Sixiang Gu <sgu@twitter.com>

…oxy-main

Signed-off-by: Sixiang Gu <sgu@twitter.com>

mattklein123 · 2021-05-27T02:17:23Z

/retest

repokitteh-read-only · 2021-05-27T02:17:27Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16294 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

Thanks!

…proxy filter (envoyproxy#16294) Signed-off-by: Sixiang Gu <sgu@twitter.com>

Sixiang Gu added 3 commits May 3, 2021 16:14

dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the …

c152c7e

…proxy filter Signed-off-by: Sixiang Gu <sgu@twitter.com>

fix test format

9fa85a7

Signed-off-by: Sixiang Gu <sgu@twitter.com>

fix version history doc

3988eb0

Signed-off-by: Sixiang Gu <sgu@twitter.com>

ntgsx92 requested review from alyssawilk and mattklein123 as code owners May 3, 2021 23:34

repokitteh-read-only bot added the api label May 3, 2021

repokitteh-read-only bot assigned adisuissa May 3, 2021

adisuissa reviewed May 4, 2021

View reviewed changes

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto Outdated Show resolved Hide resolved

Sixiang Gu added 2 commits May 4, 2021 10:25

updating description for dns_resolvers option

026ca82

Signed-off-by: Sixiang Gu <sgu@twitter.com>

fixing memory leak issue

6260b09

Signed-off-by: Sixiang Gu <sgu@twitter.com>

adisuissa reviewed May 5, 2021

View reviewed changes

htuch reviewed May 6, 2021

View reviewed changes

suniltheta mentioned this pull request May 8, 2021

dns: disable the use of search namespaces when using the c-ares resolver #16237

Merged

add DnsResolver message

9d95c7b

Signed-off-by: Sixiang Gu <sgu@twitter.com>

suniltheta reviewed May 14, 2021

View reviewed changes

Sixiang Gu added 4 commits May 14, 2021 15:26

update dns_resolvers to resolvers

5501231

Signed-off-by: Sixiang Gu <sgu@twitter.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into envoy…

d3aaa31

…proxy-main Signed-off-by: Sixiang Gu <sgu@twitter.com>

Merge branch 'envoyproxy-main' into main

a673969

Signed-off-by: Sixiang Gu <sgu@twitter.com>

fix release doc

c03697b

Signed-off-by: Sixiang Gu <sgu@twitter.com>

htuch reviewed May 16, 2021

View reviewed changes

Sixiang Gu and others added 5 commits May 18, 2021 14:47

moving DnsResolver to its own proto file

7d41f09

Signed-off-by: Sixiang Gu <sgu@twitter.com>

deps: update protobuf to 3.16.0 (envoyproxy#16390)

0677cf9

Signed-off-by: Benjamin Peterson <benjamin@dropbox.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

docs: comment config extension (envoyproxy#16406)

54af3ba

* docs: comment config extension Signed-off-by: Long Dai <long0dai@foxmail.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

add defensive coding against None (for missing buildifier) in pre-com…

9390e7a

…mit hook (envoyproxy#16505) Signed-off-by: Justin Ely <justincely@gmail.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

ntgsx92 dismissed adisuissa’s stale review via 6a3f7e5 May 20, 2021 17:46

Merge branch 'envoyproxy:main' into main

e0f0df2

alyssawilk assigned mattklein123 May 24, 2021

mattklein123 requested changes May 25, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label May 25, 2021

Sixiang Gu added 2 commits May 26, 2021 14:11

feedback

5eef9b3

Signed-off-by: Sixiang Gu <sgu@twitter.com>

Merge branch 'main' of https://github.com/ntgsx92/envoy into main

80b95ea

Signed-off-by: Sixiang Gu <sgu@twitter.com>

repokitteh-read-only bot added api and removed waiting labels May 26, 2021

Sixiang Gu added 6 commits May 26, 2021 14:27

Merge branch 'main' of git://github.com/envoyproxy/envoy into main

0412e3a

Merge branch 'main' of git://github.com/envoyproxy/envoy into envoypr…

69345fe

…oxy-main

Merge branch 'envoyproxy-main' into main

e83289a

Merge branch 'main' of https://github.com/ntgsx92/envoy into main

004177f

doc fix

aaaea99

Signed-off-by: Sixiang Gu <sgu@twitter.com>

fix test

ecd3229

Signed-off-by: Sixiang Gu <sgu@twitter.com>

mattklein123 approved these changes May 27, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 27, 2021

mattklein123 merged commit e5b4a98 into envoyproxy:main May 27, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the …

adc2a2e

…proxy filter (envoyproxy#16294) Signed-off-by: Sixiang Gu <sgu@twitter.com>

Conversation

ntgsx92 commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 3, 2021

Uh oh!

repokitteh-read-only bot commented May 3, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

malza321 commented May 4, 2021

Uh oh!

adisuissa left a comment

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!

suniltheta May 6, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

suniltheta May 6, 2021 • 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suniltheta May 14, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

malza321 commented May 15, 2021

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented May 20, 2021

Uh oh!

ntgsx92 commented May 20, 2021

Uh oh!

repokitteh-read-only bot commented May 20, 2021

Uh oh!

ntgsx92 commented May 21, 2021

Uh oh!

repokitteh-read-only bot commented May 21, 2021

Uh oh!

ntgsx92 commented May 21, 2021

Uh oh!

ntgsx92 commented May 24, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

ntgsx92 commented May 3, 2021 •

edited

Loading

suniltheta May 6, 2021 •

edited

Loading

suniltheta May 6, 2021 •

edited

Loading

suniltheta May 14, 2021 •

edited

Loading