Add destination-get-networks option #4608

zaharidichev · 2020-06-16T08:36:27Z

In #4585 we are observing an issue where a loop is encountered when using nginx ingress. The problem is that the outbound proxy does a dst lookup on the IP address which happens to be the very same address the ingress is listening on.

In order to avoid situations like that this PR introduces a way to modify the set of networks for which the proxy shall do IP based discovery. The change introduces a helm flag .Values.global.proxy.destinationGetNetworks that can be used to modify this value. There are two ways a user can affect the this setting:

setting the destinationGetNetworks field in values during a Helm install, which changes the default on all injected pods
using an annotation config.linkerd.io/proxy-destination-get-networks for injected workloads to override this value

Note that this setting cannot be tweaked through the install or inject command

Fix: #4585

olix0r · 2020-06-16T19:22:20Z

cli/cmd/root.go

@@ -226,6 +227,7 @@ type proxyConfigOptions struct {
 	debugImageVersion             string
 	dockerRegistry                string
 	imagePullPolicy               string
+	destinationGetNetworks        []string


I think for now we should omit this from the CLI flags, leaving it only as an annotation or helm setting.

Okey, we can do that. Would that mean that we do not want it in the config map or this is not related. @alpeb WDYT ?

I think it would be preferable to omit it, but I think it's ultimately an implementation detail.

zaharidichev · 2020-06-17T07:01:15Z

Ok @olix0r I have removed that flag from the CLI but by the looks of it it makes sense to leave that setting in the config map. This allows us to leave the Helm flag as well as override what the default is via an annotation. Moreover if we leave it out of the cli for now, this allows us to easily expose it whenever we want. Let me know if all that works.

Pothulapati

Injection works as expected, and modifications are reflected in the env based on annotation.

But when I mark this annotation of the deployment as empty string, I don't see the env variable being removed/empty for the newly created pod. Is that the expected outcome? In that case, How should I specify that I don't want any destination lookups for ips, as no annotation fallback to the default value. 🤔

zaharidichev · 2020-06-17T14:14:19Z

@Pothulapati Yes I guess this is the byproduct of trating empty strings the same way we treat the NON presence of a value. Go's type system is all sorts of great.....

Pothulapati · 2020-06-17T15:53:54Z

@zaharidichev The empty string case works well for me now. But when I do -oyaml, I get the following

  - env:
    - name: LINKERD2_PROXY_LOG
      value: warn,linkerd=info
    - name: LINKERD2_PROXY_DESTINATION_SVC_ADDR
      value: linkerd-dst.linkerd.svc.cluster.local:8086
    - name: LINKERD2_PROXY_DESTINATION_GET_NETWORKS
    - name: LINKERD2_PROXY_CONTROL_LISTEN_ADDR
      value: 0.0.0.0:4190

How about having a if condition to only set if not empty in the helm yaml templating to make it more cleaner? 🤔 like we do for other env's

zaharidichev · 2020-06-17T16:37:16Z

@Pothulapati I think that makes sense yes

alpeb

Looks and works fine for me, barring @Pothulapati's empty string case 👍

zaharidichev · 2020-06-17T17:42:29Z

@Pothulapati I addressed your feedback :)

adleong

I think this PR is missing a change in _config.tpl which actually sets the value of the get networks into the configmap.

It would also be helpful to have a more detailed PR description which describes the mechanics of how this works. Especially the fact that there is no install or inject flag for this and the value in the configmap can only be set via a helm install. It would also be helpful to describe the ways ways in which a user can interact with this. As far as I can tell there are two:

setting the destinationGetNetworks fields in values.yaml during a Helm install allows users to change the default destinationGetNetworks on all injected pods
setting the annotation on an injected workload will also allow you to change the destinationGetNetworks and overrides the value configured by values.yaml

If I understand correctly there is no change in behavior for the linkerd install or linkerd inject commands (except in the case of --manual injection).

adleong · 2020-06-17T22:14:08Z

cli/cmd/inject.go

@@ -437,6 +437,12 @@ func (options *proxyConfigOptions) overrideConfigs(configs *cfg.All, overrideAnn
 		overrideAnnotations[k8s.ProxyRequireIdentityOnInboundPortsAnnotation] = strings.Join(options.requireIdentityOnInboundPorts, ",")
 	}

+	if len(options.destinationGetNetworks) > 0 {


since there is no inject flag for this, this code is unreachable, right?

adleong · 2020-06-17T22:16:14Z

cli/cmd/install.go

@@ -778,6 +779,7 @@ func (options *installOptions) buildValuesWithoutIdentity(configs *pb.All) (*l5d
 	installValues.SMIMetrics.Enabled = options.smiMetricsEnabled

 	installValues.Global.Proxy = &l5dcharts.Proxy{
+		DestinationGetNetworks: strings.Join(options.destinationGetNetworks, ","),


this is basically just here for consistency, right? In practice this will always be the default value.

Yes, correct

Pothulapati

Injection works great to me, including the empty string case.

Will to test the same out with Helm, once the PR is updated! :)

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev · 2020-06-18T08:09:55Z

@adleong I tweaked the PR description

Pothulapati

Works for me, both through CLI and Helm. and the env is skipped when the annotation is set to a empty string.

One small nit, TIOLI: When I set the annotation to 10.0.0.0/8, i.e with a extra comma at the end, the injection fails.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong

Looks good, thanks for updating, @zaharidichev.

Something that I discussed with @olix0r yesterday that I just want to write down here is that I think we want to potentially decouple this change from the problem of preventing loops. In other words, I think this change is good on its own: it's nice to have a mechanism to configure the destination get networks. But I'm not sure it's the right solution to the looping ingress problem.

The problem with removing the node network from the destination get network list is that we'll lose identity and http metrics for calls from the ingress to the default backend. We'll also lose identity and http metrics for all calls to any nodeport services. We'll need to do some followup work to investigate what our options are for preventing loops.

zaharidichev · 2020-06-18T17:00:35Z

@adleong Yes that makes absolute sense.

zaharidichev requested review from olix0r and a team as code owners June 16, 2020 08:36

zaharidichev force-pushed the zd/make-dst-get-nets-configurable branch from bda5f0d to 703f293 Compare June 16, 2020 09:08

zaharidichev mentioned this pull request Jun 16, 2020

nginx-ingress proxy connection timeouts on stable-2.8.0 #4585

Closed

olix0r reviewed Jun 16, 2020

View reviewed changes

zaharidichev requested review from adleong and Pothulapati June 17, 2020 07:28

Pothulapati reviewed Jun 17, 2020

View reviewed changes

zaharidichev requested a review from Pothulapati June 17, 2020 14:18

alpeb approved these changes Jun 17, 2020

View reviewed changes

adleong reviewed Jun 17, 2020

View reviewed changes

Pothulapati reviewed Jun 18, 2020

View reviewed changes

zaharidichev added 8 commits June 18, 2020 10:48

Add destination-get-networks option

2d6a522

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Fix tests

dcb1959

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Remove flag from CLI

72ea77f

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

handle empty string

b6fffdf

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Change approach

615d055

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Guard env var with an empty check

2d01883

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Fix tests

07cb331

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Change _config.tpl and modify tests

f93299a

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/make-dst-get-nets-configurable branch from 20194aa to f93299a Compare June 18, 2020 07:52

zaharidichev requested a review from adleong June 18, 2020 08:09

Pothulapati approved these changes Jun 18, 2020

View reviewed changes

Trim comas from annotation value string

4bc7a9d

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong approved these changes Jun 18, 2020

View reviewed changes

zaharidichev requested a review from olix0r June 18, 2020 17:00

olix0r approved these changes Jun 18, 2020

View reviewed changes

zaharidichev merged commit 7f3d872 into master Jun 18, 2020

olix0r deleted the zd/make-dst-get-nets-configurable branch June 18, 2020 17:25

adleong mentioned this pull request Jun 18, 2020

Investigate avoiding ingress loops when using nodeport service and default backend #4636

Closed

Pothulapati mentioned this pull request Jun 25, 2020

Support annotation to override default timeouts for Linkerd-proxy DNS canonicalization #4660

Closed

ArthurSens mentioned this pull request Jul 4, 2020

Enable overriding timeout for DNS canonization through annotation. #4718

Closed

mmiller1 mentioned this pull request Jul 15, 2020

support overriding inbound and outbound connect timeouts. #4759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add destination-get-networks option #4608

Add destination-get-networks option #4608

zaharidichev commented Jun 16, 2020 •

edited

Loading

olix0r Jun 16, 2020

zaharidichev Jun 16, 2020

olix0r Jun 16, 2020

zaharidichev commented Jun 17, 2020

Pothulapati left a comment •

edited

Loading

zaharidichev commented Jun 17, 2020

Pothulapati commented Jun 17, 2020

zaharidichev commented Jun 17, 2020

alpeb left a comment

zaharidichev commented Jun 17, 2020

adleong left a comment

adleong Jun 17, 2020

adleong Jun 17, 2020

zaharidichev Jun 18, 2020

Pothulapati left a comment

zaharidichev commented Jun 18, 2020

Pothulapati left a comment •

edited

Loading

adleong left a comment

zaharidichev commented Jun 18, 2020

Add destination-get-networks option #4608

Add destination-get-networks option #4608

Conversation

zaharidichev commented Jun 16, 2020 • edited Loading

olix0r Jun 16, 2020

Choose a reason for hiding this comment

zaharidichev Jun 16, 2020

Choose a reason for hiding this comment

olix0r Jun 16, 2020

Choose a reason for hiding this comment

zaharidichev commented Jun 17, 2020

Pothulapati left a comment • edited Loading

Choose a reason for hiding this comment

zaharidichev commented Jun 17, 2020

Pothulapati commented Jun 17, 2020

zaharidichev commented Jun 17, 2020

alpeb left a comment

Choose a reason for hiding this comment

zaharidichev commented Jun 17, 2020

adleong left a comment

Choose a reason for hiding this comment

adleong Jun 17, 2020

Choose a reason for hiding this comment

adleong Jun 17, 2020

Choose a reason for hiding this comment

zaharidichev Jun 18, 2020

Choose a reason for hiding this comment

Pothulapati left a comment

Choose a reason for hiding this comment

zaharidichev commented Jun 18, 2020

Pothulapati left a comment • edited Loading

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

zaharidichev commented Jun 18, 2020

zaharidichev commented Jun 16, 2020 •

edited

Loading

Pothulapati left a comment •

edited

Loading

Pothulapati left a comment •

edited

Loading