-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(dns): skip DNS lookups for AutoTLS hostnames #11140
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: master
Are you sure you want to change the base?
Conversation
p2p-forge hostnames encode IP addresses directly (e.g., 1-2-3-4.peerID.libp2p.direct -> 1.2.3.4), so DNS queries are wasteful. kubo now parses these IPs in-memory. - applies to both default libp2p.direct and custom AutoTLS.DomainSuffix - TXT queries still delegate to network for ACME DNS-01 compatibility Fixes #11136
regression test for #9199 confirms that DNS.Resolvers config is used when resolving /dnsaddr multiaddrs during swarm connect, not just for DNSLink resolution
core/node/p2pforge_resolver.go
Outdated
| // split subdomain into parts: should be [ip-prefix, peerID] or just [peerID] | ||
| parts := strings.Split(subdomain, ".") | ||
| if len(parts) < 2 { | ||
| // peerID only, no IP component - return empty (NODATA equivalent) | ||
| return nil, nil | ||
| } |
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 can do this, but should we? e.g. under normal circumstances this shouldn't happen and in theory in the future .libp2p.direct could point at a given IP address.
Maybe a fallback here is better? Generally speaking shouldn't we use a fallback everywhere we can't give a definitive answer?
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.
You are right, we don't want to hard-fail here, as that would close the door for future features.
Changed all error paths to fall back to network DNS.
core/node/p2pforge_resolver.go
Outdated
| return nil, fmt.Errorf("hostname %q does not match any p2p-forge suffix", hostname) | ||
| } | ||
|
|
||
| // split subdomain into parts: should be [ip-prefix, peerID] or just [peerID] |
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.
shouldn't we do a "is this a valid peerID" check like happens at libp2p.direct?
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.
Added peer.Decode() validation with fallback on failure
| for _, domain := range forgeDomains { | ||
| opts = append(opts, madns.WithDomainResolver(domain+".", forgeResolver)) | ||
| } |
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.
How is this meant to interact with say the user hard-coding a resolver in the config for *.libp2p.direct?
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.
It overrides it. Documented in DNS.Resolvers section of config.md.
core/node/dns.go
Outdated
|
|
||
| // Build list of p2p-forge domains to resolve locally without network I/O. | ||
| // AutoTLS hostnames encode IP addresses directly (e.g., 1-2-3-4.peerID.libp2p.direct), | ||
| // so DNS lookups are wasteful. We always resolve these in-memory. |
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.
Is there a reason we'd want to be able to turn this feature off? I can't think of a good reason other than debugging right now so I suspect not but wanted to ask in case.
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.
Unlikely, but could be useful for testing/debugging.
Added AutoTLS.SkipDNSLookup config flag (enabled by default).
| // It verifies that DNS.Resolvers config is used when resolving /dnsaddr, | ||
| // /dns, /dns4, /dns6 multiaddrs during peer connections, not just for | ||
| // DNSLink resolution. | ||
| func TestDNSResolversApplyToMultiaddr(t *testing.T) { |
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.
Should we also add a test that even when we override the default resolver, libp2p.direct still uses the custom resolver or just extra noise because we have sufficient tests around the configurability elsewhere?
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.
I think its good idea: added test verifying local resolution with broken DNS.Resolvers
Changes based on PR review comments: - #11140 (comment) use fallback to network DNS instead of returning errors when local parsing fails, ensuring forward compatibility with future DNS records - #11140 (comment) add peerID validation using peer.Decode(), matching libp2p.direct server behavior, with fallback on invalid peerID - #11140 (comment) document interaction with DNS.Resolvers in config.md - #11140 (comment) add AutoTLS.SkipDNSLookup config flag to disable local resolution (useful for debugging or custom DNS override scenarios) - #11140 (comment) add E2E test verifying libp2p.direct resolves locally even when DNS.Resolvers points to a broken server additional improvements: - use madns.BasicResolver interface instead of custom basicResolver - add compile-time interface checks for p2pForgeResolver and madns.Resolver - refactor tests: merge IPv4/IPv6, add helpers, use config.DefaultDomainSuffix - improve changelog to explain public good benefit (reducing DNS load)
p2p-forge hostnames encode IP addresses directly (e.g.,
1-2-3-4.peerID.libp2p.direct->1.2.3.4), so DNS queries are wasteful. kubo now parses these IPs in-memory.cc @aschmahmann as this aims to address #11136
This PR
libp2p.directeTLD and customAutoTLS.DomainSuffixNotes on DNS wiring
Using
DNS.Resolversfor go-libp2p tooiiuc the dependency injection chain is:
core/node/dns.go:13- DNSResolver() creates*madns.Resolverwith p2p-forge supportcore/node/groups.go:357,375-fx.Provide(DNSResolver)makes it available in both Online and Offline modescore/node/groups.go:32- BaseLibP2P includesfx.Provide(libp2p.MultiaddrResolver)core/node/libp2p/dns.go:9-11-MultiaddrResolvertakes*madns.Resolveras a dependency and wires it into go-libp2p:The
fxdependency injection automatically connects these - whenMultiaddrResolverneeds a*madns.Resolver,fxprovides the one fromDNSResolver()which now includes the p2p-forge local resolution.This means all
/dns4/*.libp2p.direct/...and/dns6/*.libp2p.direct/...multiaddrs resolved by go-libp2p's swarm will use the new local resolver, avoiding network I/O. This includes peer addresses discovered via DHT, delegated routing, or any other source.Regression tests
I confirmed this with extra manual test:
The error proves
DNS.ResolversIS being used for/dnsaddrmultiaddr resolution too.Added e2e test to
test/clito ensure we have regression test that detects if this wiring is ever broken due to future go-libp2p update or our own refactors etc.