Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Abrishges opened this issue Jun 24, 2020 · 14 comments

Comments

@Abrishges
Copy link
Contributor

Abrishges commented Jun 24, 2020

Feature Request

What problem are you trying to solve?

Currently, Linkerd proxy provides LINKERD2_PROXY_DNS_CANONICALIZE_TIMEOUT environment variable that allows the default 100ms timeout to be overridden. linkerd/linkerd2-proxy#175
However there is no proxy configuration or annotation to override this environment value during helm or customized installation or even using release pipeline

How should the problem be solved?

we should probably provide linkerd config annotation that can override environment variables like LINKERD2_PROXY_DNS_CANONICALIZE_TIMEOUT

config.linkerd.io/proxy-dns-canonicalize-timeout : "200"

Any alternatives you've considered?

Alternatively we should consider providing CLI flag too --proxy-dns-canonicalize-timeout

How would users interact with this feature?

For example, we can add annotation to our proxy config,

             annotations:
                   config.linkerd.io/proxy-dns-canonicalize-timeout : "200"

or can also be done manually using linkerd CLI
kubectl get deployment ... | linkerd inject --proxy-dns-canonicalize-timeout : "200" | kubectl apply -

@ihcsim
Copy link
Contributor

ihcsim commented Jun 24, 2020

@Abrishges Thanks for the issue. I'm just wondering if you will be interested in working on this; I can point you in the right direction. It will require some knowledge with Go.

@ArthurSens
Copy link
Contributor

I would like to work on this issue!

@ihcsim
Copy link
Contributor

ihcsim commented Jun 24, 2020

@grampelberg WDYT of just adding this as a config annotation as a starter, without the CLI option?

@grampelberg
Copy link
Contributor

SGTM

@grampelberg
Copy link
Contributor

Not saying this is a good idea, but should we just make it possible to override arbitrary proxy variables?

@Pothulapati
Copy link
Contributor

Hey @ArthurSens, This could be similar to #4608 and looking at it might help.

Feel free to reach out here, if you have any questions :)

@ihcsim
Copy link
Contributor

ihcsim commented Jun 25, 2020

@grampelberg That will be great if we want all proxy variables to be overridable. At the moment, not all of them are. And I can see why we don't want variables like LINKERD2_PROXY_DESTINATION_SVC_ADDR, LINKERD2_PROXY_IDENTITY_DIR to be (directly) configurable. fwiw, I'd also feel better about making things more configurable, if we also provide tooling to detect abnormal config.

@ArthurSens
Copy link
Contributor

Hello everyone, I've made some changes that you can see here. A new attribute was added to the ProxyInit struct, but I couldn't find where exactly linkerd uses that struct to configure the proxy-init container. What am I missing?

And just to be sure... am I following the right direction with the changes so far?

@ihcsim
Copy link
Contributor

ihcsim commented Jun 25, 2020

@ArthurSens Yup, that's the right file where we serialize the Go structs that hold the Helm variables. I'm gonna move the rest of the code-level details to the #contributor channel.

@cpretzer
Copy link
Contributor

cpretzer commented Jul 3, 2020

@ArthurSens I ran this in my local environment and it's looking good, so far.

I also read through your conversation with @ihcsim on slack, so I think I'm all caught up. Is there anything I can help with on this? I'd love to see it merged!

@ArthurSens
Copy link
Contributor

Hey @cpretzer @ihcsim, wanted to say that it has been some time since the last time I worked on this and I'm afraid that I won't be able to put as much effort as I'd like to it.

I think it would be better for linkerd if someone else was assigned to this

@cpretzer
Copy link
Contributor

Thanks for the update @ArthurSens, we really appreciate all the effort that you have put in. 🙏 😄

When you have time again, we'd love to work with you on this or other PRs.

@cpretzer
Copy link
Contributor

Closing for now. If anyone is interested in picking up the work, please comment here

@jabdoa2
Copy link

jabdoa2 commented Sep 30, 2020

We need this as well. 100ms seems a bit low under load for most clusters.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants