internal/envoy: Set the dns lookup family on externalName type clusters #2894

stevesloka · 2020-09-09T20:35:53Z

Adds a config file option Cluster.DnsLookupFamily which allows users to define what dns lookup family is used for any
externalName type cluster. This ensures that lookups to external resources are resolved correctly.

Fixes #2873

Signed-off-by: Steve Sloka slokas@vmware.com

codecov · 2020-09-09T20:40:12Z

Codecov Report

Merging #2894 into main will increase coverage by 0.02%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##             main    #2894      +/-   ##
==========================================
+ Coverage   74.91%   74.94%   +0.02%     
==========================================
  Files          87       87              
  Lines        5582     5604      +22     
==========================================
+ Hits         4182     4200      +18     
- Misses       1310     1314       +4     
  Partials       90       90

Impacted Files	Coverage Δ
cmd/contour/serve.go	`1.83% <0.00%> (-0.03%)`	⬇️
internal/dag/dag.go	`96.84% <ø> (ø)`
cmd/contour/servecontext.go	`90.24% <100.00%> (+0.86%)`	⬆️
internal/dag/httpproxy_processor.go	`94.24% <100.00%> (+0.01%)`	⬆️
internal/envoy/v2/cluster.go	`100.00% <100.00%> (ø)`

jpeach · 2020-09-09T21:50:39Z

@stevesloka Can you give more detail about why v4-only is the right approach here? What was the underlying problem?

stevesloka · 2020-09-10T14:02:17Z

@jpeach the issue describes the problem in more detail.

The TLDR is: When configuring a route to an externalName service, requests don't get fulfilled properly, but return 503 errors. When Envoy does a DNS lookup to some external resources, a ipv6 address is returned for the external resource and proxying fails with a 503 error. Setting this field ensures an IPv4 is returned so Envoy proxies correctly. I tested this in a GKE cluster as well as my home lab, both showed the same issue and was also resolved with the same fix.

Chatting with @moderation in an Envoy issue (envoyproxy/envoy#13037 (comment)), he verified that v3 resolved the issue which we haven't yet moved to, or by setting the dns lookup family might also help.

skriss · 2020-09-10T16:50:47Z

Based on https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/cluster.proto#envoy-api-enum-cluster-dnslookupfamily, it sounds to me like this should be working properly (i.e. fallback to v4 resolution if v6 fails), so perhaps an Envoy bug?

I guess my only question re: this change is, are there any scenarios where specifying v4-only would cause issues for users by not supporting v6?

jpeach · 2020-09-10T21:21:02Z

@jpeach the issue describes the problem in more detail.

The TLDR is: When configuring a route to an externalName service, requests don't get fulfilled properly, but return 503 errors. When Envoy does a DNS lookup to some external resources, a ipv6 address is returned for the external resource and proxying fails with a 503 error. Setting this field ensures an IPv4 is returned so Envoy proxies correctly. I tested this in a GKE cluster as well as my home lab, both showed the same issue and was also resolved with the same fix.

Chatting with @moderation in an Envoy issue (envoyproxy/envoy#13037 (comment)), he verified that v3 resolved the issue which we haven't yet moved to, or by setting the dns lookup family might also help.

Thanks for linking the Envoy issue. It's not clear to me from the issue whether the problem was that Envoy couldn't connect to the v6 target for some reason, or whether it hit an internal bug first. Possibly, the bug is that it does't fall back to v4 on a connection failure?

moderation · 2020-09-10T23:28:31Z

To clarify I don't think changing to v3 fixed the issue, it was the addition of dns_lookup_family: V4_ONLY. Not sure hard coding this to V4 makes sense. Although rare I've heard of V6 only environments. Seems like something that should be configurable based on the environment where Contour is running. I think there could be quite a few permutation here too. A DNS that resolves V6 and V4 addresses but the network is V4. A dual stack network but with DNS that only resolves V4 etc.

stevesloka · 2020-09-15T01:40:01Z

I had some more discussion in the upstream Envoy issue and think this isn't 100% an Envoy issue. The Envoy setting dns_lookup_family determines how Envoy will lookup dns entries. The default which Contour uses today is AUTO, meaning Envoy will try to find an ipv6 address first, then fallback to ipv4 addresses.

Since some sites are returning ipv6 addresses, it's then up to the network capabilities of the cluster's network to allow the routing to work. I think this is why we're seeing the errors in the issue referenced.

To fully resolve I think we should add a config setting which will allow users to determine which setting to pass to envoy, be that the ipv4, ipv6, or auto (which should remain the default).

@youngnick @skriss thoughts?

youngnick · 2020-09-15T03:13:54Z

I think that's the right way to do this, thanks @stevesloka.

stevesloka · 2020-09-16T16:08:06Z

Ok this is updated with a new config file option. I named it "cluster", but not sure if that makes sense or not. This new setting defines what is used for the dns_lookup_family on any outgoing request to an externalName type cluster.

skriss

Approach looks reasonable to me -- it seems unlikely that this would need to be toggled on a per-route basis so a global config file setting makes sense.

Need to update configuration.md as well.

cmd/contour/serve.go

internal/envoy/cluster_test.go

cmd/contour/servecontext_test.go

cmd/contour/servecontext.go

skriss · 2020-09-16T18:54:40Z

cmd/contour/servecontext.go

+
+	// ClusterConfig holds various configurable Envoy cluster values that can
+	// be set in the config file.
+	ClusterConfig `yaml:"cluster,omitempty"`


I don't really have a better suggestion for naming here. I guess this could just be a top-level field in the config file too. 🤷‍♂️

Yup that's where I started, but if we ever needed to add another "cluster" type config it's nice to have this section for it.

cmd/contour/serve.go

skriss · 2020-09-17T20:05:36Z

Still need to update site/docs/main/configuration.md too.

stevesloka · 2020-09-18T16:01:03Z

@skriss added configuration.md as well. =)

skriss · 2020-09-18T16:54:00Z

Changes look good; you've got some merge conflicts now.

stevesloka · 2020-09-18T18:16:47Z

site/docs/main/api-reference.html

@@ -1,33 +1,27 @@
 <p>Packages:</p>
 <ul>
 <li>
-<a href="#projectcontour.io%2fv1">projectcontour.io/v1</a>
+<a href="#projectcontour.io%2fv1alpha1">projectcontour.io/v1alpha1</a>


I don't know why this file was changed. Maybe another PR didn't update properly?

Hmm, maybe an unstable sort within the doc generator? Looks like it switched the order of v1 and v1alpha1

When I check out your branch and re-run the generate, it undoes this change..

I think I spotted the issue in the API docs generator, TLDR it's using a map somewhere along the line which potentially throws away the sort

Maybe just remove this file from the PR since you didn't actually change anything in the API?

skriss

LGTM.

It'd be nice to figure out how to resolve the API ref docs sorting issue separately now that we have >1 group, otherwise we're gonna get a lot of noise from this going fwd. We can try getting a patch upstream (looks like there might be an open PR with a fix that we could make some noise on), else we may need to fork the tool.

jpeach · 2020-09-20T00:01:02Z

Are we sure that this makes sense as a global config option? To me, it seems likely that you may need to set this differently for different targets, which argues for an annotation. I could see the utility of setting a default in addition to that though.

youngnick · 2020-09-20T23:37:48Z

I think this is most likely to be an install-wide problem, caused by the shenanigans around AAAA and A records when you've got V4 and V6 around, so that's why I supported the global config first. I think that if we need more configurability, we can add it later, with this as the default.

jpeach · 2020-09-21T22:16:07Z

I think this is most likely to be an install-wide problem, caused by the shenanigans around AAAA and A records when you've got V4 and V6 around, so that's why I supported the global config first. I think that if we need more configurability, we can add it later, with this as the default.

It's an external service, managed by an entirely separate org. That separate org is the one publishing DNS and both the org and the I remember work paths are factors in determining whether IPv6 connectivity works. It's extremely likely that only one out of many external services would exhibit similar problems. At least spin off an issue for the per-service config.

What was the root cause in the problem that triggered this PR? Was it a local install problem or a external service problem?

youngnick · 2020-09-21T23:39:34Z

You're right, it was an external service problem.

stevesloka · 2020-09-21T23:45:33Z

The problem I ran into is the local cluster wouldn't route ipv6. The external service returned an ipv6 address first since that's what we configured envoy to do when using the default of "auto".

That's said, this is a local cluster problem. Nothing to do with the external one. I could see a different issue to add seperate configs for each service type, but not needed today.

youngnick · 2020-09-23T01:05:14Z

So, I've thought about this some more, and I'm still supportive of this config being a global config, for two reasons:

it fixes the ExternalService issue as it's supposed to, when the ExternalService referenced has a V6 record and the Envoy's can't route there.
it can also be used to improve DNS lookup performance in clusters that only do V4 - it's pretty common for a large percentage of DNS queries in a V4-only cluster to be spurious AAAA ones, this is a neat knob to help with reducing that a bit.

Adds a config option for DnsLookupFamily allowing users to define what dns lookup family is used on any cluster that is referenced via an externalName type cluster. This ensures that lookups to external resources are resolved correctly. Fixes projectcontour#2873 Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka added this to the 1.9.0 milestone Sep 9, 2020

stevesloka force-pushed the externalNameDNSLookup branch from 4377594 to 8c95549 Compare September 16, 2020 16:05

stevesloka requested review from skriss and youngnick September 16, 2020 16:07

stevesloka force-pushed the externalNameDNSLookup branch from 8c95549 to c4aa289 Compare September 16, 2020 17:57

stevesloka added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 16, 2020

skriss requested changes Sep 16, 2020

View reviewed changes

stevesloka force-pushed the externalNameDNSLookup branch from c4aa289 to a9b8e70 Compare September 17, 2020 14:47

stevesloka requested a review from skriss September 17, 2020 14:47

stevesloka force-pushed the externalNameDNSLookup branch from a9b8e70 to 6cadac7 Compare September 17, 2020 14:48

skriss reviewed Sep 17, 2020

View reviewed changes

cmd/contour/serve.go Outdated Show resolved Hide resolved

stevesloka force-pushed the externalNameDNSLookup branch from 6cadac7 to e2730d0 Compare September 18, 2020 15:59

stevesloka requested a review from skriss September 18, 2020 16:01

stevesloka force-pushed the externalNameDNSLookup branch from e2730d0 to efa1a86 Compare September 18, 2020 18:13

stevesloka commented Sep 18, 2020

View reviewed changes

skriss approved these changes Sep 18, 2020

View reviewed changes

stevesloka force-pushed the externalNameDNSLookup branch from efa1a86 to 35e524b Compare September 23, 2020 15:31

Merge branch 'main' into externalNameDNSLookup

3fffe49

stevesloka merged commit 12ace66 into projectcontour:main Sep 23, 2020

stevesloka deleted the externalNameDNSLookup branch September 23, 2020 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/envoy: Set the dns lookup family on externalName type clusters #2894

internal/envoy: Set the dns lookup family on externalName type clusters #2894

stevesloka commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading

jpeach commented Sep 9, 2020

stevesloka commented Sep 10, 2020

skriss commented Sep 10, 2020

jpeach commented Sep 10, 2020

moderation commented Sep 10, 2020

stevesloka commented Sep 15, 2020

youngnick commented Sep 15, 2020

stevesloka commented Sep 16, 2020

skriss left a comment

skriss Sep 16, 2020

stevesloka Sep 17, 2020

skriss commented Sep 17, 2020

stevesloka commented Sep 18, 2020

skriss commented Sep 18, 2020

stevesloka Sep 18, 2020

skriss Sep 18, 2020

skriss Sep 18, 2020

skriss Sep 18, 2020

skriss Sep 23, 2020

skriss left a comment

jpeach commented Sep 20, 2020

youngnick commented Sep 20, 2020

jpeach commented Sep 21, 2020

youngnick commented Sep 21, 2020

stevesloka commented Sep 21, 2020

youngnick commented Sep 23, 2020

internal/envoy: Set the dns lookup family on externalName type clusters #2894

internal/envoy: Set the dns lookup family on externalName type clusters #2894

Conversation

stevesloka commented Sep 9, 2020 • edited Loading

codecov bot commented Sep 9, 2020 • edited Loading

Codecov Report

jpeach commented Sep 9, 2020

stevesloka commented Sep 10, 2020

skriss commented Sep 10, 2020

jpeach commented Sep 10, 2020

moderation commented Sep 10, 2020

stevesloka commented Sep 15, 2020

youngnick commented Sep 15, 2020

stevesloka commented Sep 16, 2020

skriss left a comment

Choose a reason for hiding this comment

skriss Sep 16, 2020

Choose a reason for hiding this comment

stevesloka Sep 17, 2020

Choose a reason for hiding this comment

skriss commented Sep 17, 2020

stevesloka commented Sep 18, 2020

skriss commented Sep 18, 2020

stevesloka Sep 18, 2020

Choose a reason for hiding this comment

skriss Sep 18, 2020

Choose a reason for hiding this comment

skriss Sep 18, 2020

Choose a reason for hiding this comment

skriss Sep 18, 2020

Choose a reason for hiding this comment

skriss Sep 23, 2020

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

jpeach commented Sep 20, 2020

youngnick commented Sep 20, 2020

jpeach commented Sep 21, 2020

youngnick commented Sep 21, 2020

stevesloka commented Sep 21, 2020

youngnick commented Sep 23, 2020

stevesloka commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading