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: Relay randomization #1584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

junkurihara
Copy link

Hello,

In the documentation, the following was mentioned as caveats.

For each server, a random relay from the set is chosen when the proxy starts, and the same relay will be used until the proxy is restarted. Relay randomization and failover will be implemented in future versions.

https://github.com/DNSCrypt/dnscrypt-proxy/wiki/Anonymized-DNS

I just implemented the relay randomization mentioned above (since I needed :-) ). The option of relay randomization can be enabled by making relay_randomization true in dnscrypt-proxy.toml. If this is enabled, one relay server is randomly chosen from an array of relay candidates at each query issuance.

If you are okay, please review and consider to merge this commit. Thanks a lot for this quite nice software!

@lifenjoiner
Copy link
Member

I'm not an expert on this, but only do 1 thing in 1 commit, please.
Atomic commits: One change, one commit.

@junkurihara
Copy link
Author

junkurihara commented Feb 9, 2021

@lifenjoiner Hey, thanks for your comment. Yes, you're right. I just simply followed commits on the original master branch on my fork, and unintentionally, it was pushed to the remote repo. my commit itself is actually only one for the feature. will revert it to the commit 02461e8.

Copy link
Member

@lifenjoiner lifenjoiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, for example:

.gitignore Outdated Show resolved Hide resolved
dnscrypt-proxy/example-dnscrypt-proxy.toml Outdated Show resolved Hide resolved
@black-ish
Copy link

What's the status on this? It's been quite a while.

@bertusdebruin
Copy link

Interesting feature. Thanks @junkurihara Apparently it's quiet over here. Feasible feature @jedisct1 ?

@jedisct1
Copy link
Member

This is an interesting feature, but it has a couple implications.

What should via = ['*'] do in a route in such a situation?

Also, rtt computation doesn't really work any more, at least with the current code, with multiple routes.

This code also conflicts with ODoH relays, which is why I postponed its review. One thing at a time :)

ODoH may have introduced regressions in the way relays work, so the intent is to release a version with the ODoH set of changes first, and then look at how to add relay randomization on top of that. Order is arbitrary, but one thing at a time :)

@lsm5
Copy link

lsm5 commented Dec 16, 2021

Hello @junkurihara . Just curious about the status of this feature. Would be great to have this.

@junkurihara
Copy link
Author

junkurihara commented Dec 16, 2021

Hello @lsm5 ,

Hello @junkurihara . Just curious about the status of this feature. Would be great to have this.

This PR to dnscrypt-proxy is currently pending. Also I did not implemented this feature based on ODoH yet.

Here's my forked version tracking the master branch of original dnscrypt-proxy and having several additional features like relay randomization. If you like you can use it to use relay randamization.
https://github.com/junkurihara/dnscrypt-proxy-modns

@Daasin
Copy link

Daasin commented Dec 20, 2021

Also I did not implemented this feature based on ODoH yet. @junkurihara

Hopefully you manage to 😊 thank you for the work and can't wait to try it!

@Queuecumber
Copy link

Would be great if this could be prioritized, it's the only PR on the repo right now and it's been well over a year since submission

@Daasin
Copy link

Daasin commented Nov 23, 2022

Is there an update on this by any chance?

@p060477
Copy link

p060477 commented Jan 9, 2023

no news...???... :(

@jedisct1 jedisct1 force-pushed the master branch 3 times, most recently from 750e928 to 9f3ef73 Compare February 7, 2023 10:03
@DNSCrypt DNSCrypt deleted a comment May 14, 2023
@Daasin
Copy link

Daasin commented Jul 2, 2023

Hello @lsm5 ,

Hello @junkurihara . Just curious about the status of this feature. Would be great to have this.

This PR to dnscrypt-proxy is currently pending. Also I did not implemented this feature based on ODoH yet.

Here's my forked version tracking the master branch of original dnscrypt-proxy and having several additional features like relay randomization. If you like you can use it to use relay randamization. https://github.com/junkurihara/dnscrypt-proxy-modns

@junkurihara Were you able to get the ODoH-based support feature implemented in the end? 🤞🏽👀
Or do you plan to while this is pending to be merged upstream/mainline 🙏🏽

@lifenjoiner
Copy link
Member

The current solution is not that bad, and the wiki has been updated to:

For each server, a random relay from the set is chosen when servers are ranked, and then holds for a period. The cycle period is cert_refresh_delay. Per request relay randomization is too hard to measure the route quality, that may break the lb_strategy quality.

That explains that @jedisct1 said about the rtt computation. More: goto above #1584 (comment).

In short, relay randomization is not for per request, but holds since each complete re-ranking.
You can minimize the cert_refresh_delay to 60 minutes, if you concern this very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants