-
Notifications
You must be signed in to change notification settings - Fork 44
Conversation
dns.go
Outdated
DefaultResolver string `json:",omitempty"` | ||
// CustomResolvers is a map of domains to URLs for custom DoH resolution. | ||
CustomResolvers map[string]string `json:",omitempty"` |
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.
Do we need a default resolver? I'd assume that we could have a single "." entry in a Resolvers
map, right?
Also, let's briefly (very briefly) document the format (i.e., URL? Multiaddr?).
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.
note: some documentation in code can happen here, but the user facing docs live in go-ipfs https://github.com/ipfs/go-ipfs/blob/master/docs/config.md
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.
+1 on removing DefaultResolver
and renaming CustomResolvers
to simple Resolvers
Don't skip in JSON
We should avoid json:",omitempty"
here.
It is better for discoverability to have empty DNS.Resolvers
(ok to have {}
) – that is what we already do in Pinning.RemoteServices
.
Values
We could support multiaddrs and URLs, but for DoH URL will be what people want to copy&paste while setting it up, so ok with saying this needs to be https:// URL with DoH endpoint.
Keys
I was thinking about interop with wider DNS ecosystem here and suggest we don't invent any new notation with *
as I initially syggested, but follow the existing DNS convention of using the explicit FQDN syntax in our config files.
To illustrate, a custom DoH-only config for multiple resolvers could look like this::
"DNS": {
"Resolvers": {
"eth.": "https://different-ens.example.net/dns-query",
"crypto.": "https://unstoppablesomething.example.com/dns-query",
"libre.": "https://ns1.iriseden.fr/dns-query",
".": "https://doh-ch.blahdns.com:4443/dns-query"
}
}
Idea here is to closely follow the format of zone files, which should make things easier to understand. (The DNS root is unnamed, expressed as the empty label terminated by the dot.)
We should enforce keys are explicit FQDNs by validating them with dns.IsFqdn
– removes surface for DNS ambiguity.
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.
ok fair enough; we can call the field just 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.
I will also make some minor changes in the madns Resolver to normalize to fqdns.
In principle we could, but it seems more user friendly to have an explicit
default.
…On Mon, Apr 12, 2021, 23:20 Steven Allen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dns.go
<#126 (comment)>:
> + DefaultResolver string `json:",omitempty"`
+ // CustomResolvers is a map of domains to URLs for custom DoH resolution.
+ CustomResolvers map[string]string `json:",omitempty"`
Do we *need* a default resolver? I'd assume that we could have a single
"." entry in a Resolvers map, right?
Also, let's briefly (very briefly) document the format (i.e., URL?
Multiaddr?).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#126 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAI4SXYPWOCWSG4M6QDR33TINIZFANCNFSM42ZBMCIA>
.
|
and sure, i'll add some documentation.
…On Mon, Apr 12, 2021, 23:20 Steven Allen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dns.go
<#126 (comment)>:
> + DefaultResolver string `json:",omitempty"`
+ // CustomResolvers is a map of domains to URLs for custom DoH resolution.
+ CustomResolvers map[string]string `json:",omitempty"`
Do we *need* a default resolver? I'd assume that we could have a single
"." entry in a Resolvers map, right?
Also, let's briefly (very briefly) document the format (i.e., URL?
Multiaddr?).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#126 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAI4SXYPWOCWSG4M6QDR33TINIZFANCNFSM42ZBMCIA>
.
|
Both SGTM. |
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.
Some asks in comment inline.
Rationale: I'd like to avoid inventing custom syntax in keys and concepts, if we can.
DNS ecosystem and tooling already has pre-existing concepts like FQDN and configuration in form of Zone files, we should follow what exists where possible to improve onboarding.
dns.go
Outdated
DefaultResolver string `json:",omitempty"` | ||
// CustomResolvers is a map of domains to URLs for custom DoH resolution. | ||
CustomResolvers map[string]string `json:",omitempty"` |
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.
+1 on removing DefaultResolver
and renaming CustomResolvers
to simple Resolvers
Don't skip in JSON
We should avoid json:",omitempty"
here.
It is better for discoverability to have empty DNS.Resolvers
(ok to have {}
) – that is what we already do in Pinning.RemoteServices
.
Values
We could support multiaddrs and URLs, but for DoH URL will be what people want to copy&paste while setting it up, so ok with saying this needs to be https:// URL with DoH endpoint.
Keys
I was thinking about interop with wider DNS ecosystem here and suggest we don't invent any new notation with *
as I initially syggested, but follow the existing DNS convention of using the explicit FQDN syntax in our config files.
To illustrate, a custom DoH-only config for multiple resolvers could look like this::
"DNS": {
"Resolvers": {
"eth.": "https://different-ens.example.net/dns-query",
"crypto.": "https://unstoppablesomething.example.com/dns-query",
"libre.": "https://ns1.iriseden.fr/dns-query",
".": "https://doh-ch.blahdns.com:4443/dns-query"
}
}
Idea here is to closely follow the format of zone files, which should make things easier to understand. (The DNS root is unnamed, expressed as the empty label terminated by the dot.)
We should enforce keys are explicit FQDNs by validating them with dns.IsFqdn
– removes surface for DNS ambiguity.
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.
LGTM, but please update the comment.
It is a good opportunity to explain the purpose of this field to a drive-by contributor
(devs don't read docs, but they do read comments :))
Co-authored-by: Marcin Rataj <lidel@lidel.org>
No description provided.