-
Notifications
You must be signed in to change notification settings - Fork 135
refactor(gateway): switch defaults to autoconf.FallbackDNSResolvers #1044
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
base: main
Are you sure you want to change the base?
Conversation
remove implicit DNS-over-HTTPS resolvers that were difficult to discover, override, or disable, improving user agency and configuration transparency - remove defaultResolvers map from gateway.NewDNSResolver - NewDNSResolver(nil) now uses system DNS only (no implicit DoH) - add autoconf.Client.ExpandDNSResolvers() convenience method for applications needing network-specific DNS resolver expansion - update test to use .foo instead of .eth TLD users needing DoH for non-ICANN TLDs should either pass explicit resolvers or use autoconf.ExpandDNSResolvers() to merge network defaults with custom resolvers Closes #771 Closes #772
defaultResolvers
defaultResolvers
defaultResolvers
+ autoconf guidance
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1044 +/- ##
==========================================
- Coverage 60.76% 60.76% -0.01%
==========================================
Files 268 268
Lines 33593 33595 +2
==========================================
- Hits 20414 20413 -1
+ Misses 11508 11507 -1
- Partials 1671 1675 +4
... and 13 files with indirect coverage changes 🚀 New features to boost your workflow:
|
CHANGELOG.md
Outdated
- This change removes implicit defaults that were difficult to discover, override, or disable, improving user agency and configuration transparency | ||
- `NewDNSResolver(nil)` now uses system DNS only (no implicit DoH resolvers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to make this a breaking change for all users? We had centralized / hardcoded endpoints in here beforehand why not just use autoconf by default?
e.g. if ENS is highly visible / expected within the ecosystem and we make it so that by default it doesn't show up we're going to be hoisting a whole bunch of bug reports onto users who'll have to report this to their implementations. As a bonus this type of breakage doesn't even show up in Go tooling so most users will just upgrade (it seems like nobody reads release notes until maybe they see Go yelling at them), think everything is fine, and then rely on users to report bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point – we do not want to triage extra bugs – this is legacy debt we can pay off some other time.
I've restored the old behavior (b3238d9) to not break existing users, but did not add full autoconf, just replaced defaultResolvers
with code that reads defaults from autoconf.FallbackDNSResolvers
. This way we still remove copy of the defaults from boxo/gateway
and have single source of truth for fallback defaults in autoconf
package, but avoid a can of worms described below.
(There are many reasons to not use autoconf here: real autoconf comes with extra network I/O and potential delay in startup/initialization. We would also add a ton of code to ensure user can pass own client, and initialize new implicit default client if user did not pass any. Then, how do we know if user is ok with sending extra HTTP Request every time their app with boxo/gateway
starts? Is user ok with leaking their IP to some http server when they were using boxo/gateway
only in private contexts? Can of worms.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many reasons to not use autoconf here: real autoconf comes with extra network I/O and potential delay in startup/initialization
Agree on some of the can of worms and that we can defer at the library layer if we want, but in most ways it seems like using autoconf still ends up better than the status quo.
real autoconf comes with extra network I/O and potential delay in startup/initialization
Delays and I/O don't have to happen, they can happen on first use of a non-ICANN domain.
Then, how do we know if user is ok with sending extra HTTP Request every time their app with boxo/gateway starts
Mostly fixed by above, unless the user has very short-lived gateways (a real possibility, but other caching capabilities like those that are DoH related are impacted here as well).
Is user ok with leaking their IP to some http server when they were using boxo/gateway only in private contexts?
We already have this if someone uses a .eth or .crypto domain
We would also add a ton of code
Maybe, I'm not sure that's true. We have overrideable defaults like this in a bunch of places (even if they don't rely on I/O since we'd prefer to avoid that if possible).
IIUC the win here from using the default autoconf endpoint is basically that users can rely on the boxo maintainers to give them sane default DNSLink resolvers rather than relying on externally run hard coded ones to be good indefinitely (i.e. the status quo we're keeping here). This could have been done by the maintainers instead running proxies for each default DNSLink resolver, but using autoconf makes the extra operational burden on the maintainers minimal while allowing for extra resiliency with having longer cache time so that even if the default autoconf endpoint goes down for a bit that many users will still be able to resolve their domain names. In theory multiple endpoints for a given eTLD could be configured, but at the moment even ENS in practice only has one public DNSLink resolver 😬.
…defaults - moves hardcoded DNS resolver defaults to autoconf.FallbackDNSResolvers - NewDNSResolver(nil) uses autoconf fallback, preserving behavior - empty map NewDNSResolver({}) explicitly uses system DNS only - removes autoconf.Client.ExpandDNSResolvers method (no use case yet) - addresses privacy concerns by avoiding network calls at gateway init
defaultResolvers
+ autoconf guidance
|
||
var defaultResolvers = map[string]string{ | ||
"eth.": "https://dns.eth.limo/dns-query", | ||
"crypto.": "https://resolver.unstoppable.io/dns-query", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should flag that this is dead / removed now (it's not in the default autoconf resolver or in the fallbacks)
This PR moves hardcoded DNS resolver defaults from
gateway.NewDNSResolver
toautoconf.FallbackDNSResolvers
so we have them in one place, while preserving existing behavior for users.Changes
autoconf.FallbackDNSResolvers
NewDNSResolver(nil)
usesautoconf.FallbackDNSResolvers
(preserves.eth
resolution)NewDNSResolver(map[string]string{})
explicitly uses system DNS onlyautoconf.Client.ExpandDNSResolvers()
method (no use case yet).foo
instead of.eth
TLDMigration
No migration needed. For those wanting custom DNS configuration, existing code (Kubo, Rainbow) continues to work.
References