Rework mutable namespace resolution to handle recursion #1208
Conversation
|
This is a farily major resolution overhaul, so we probably need to figure out how it relates to the #1150 fix @cryptix is working on. |
|
Hey @wking, thanks for going through this! I also thought similar things when working on #1150. I actually wouldn't mind for this going in first but I don't depend on it and it doesn't look like it would be a hefty merge. One thing i'm not too sure about is exposing the type Protocol uint
const (
DNS Protocol = iota
IPNS
IPFS
)
type Resolver interface {
Resolve(ctx context.Context, name string, proto Protocol) (path.Path, error)
} |
|
On Thu, May 07, 2015 at 04:35:11PM -0700, Henry wrote:
“Please resolve this recursively, but bail out after 42 cycles”. This
Nope. The per-protocol handlers currently just take the bare object. DNSResolver.Resolve(ctx, "ipfs.io", 0) and the multi-protocol resolver gets the protocol from a prefix: NewNameSystem(…).Resolve(ctx, "/dnslink/ipfs.io", 0) |
|
I've rebased this onto #1209 to get my local test suite going again, Besides the encoding issues (base-58 vs. hex vs. proquint) mentioned |
|
Let's use On the encoding, speaking generally for a moment: it's become apparent to me that we need some sort of self-describing encoding. It definitely is a problem that In the IPNS case specifically-- for chosing between proquint and other things, the rules we're using now do expression matching (i.e. if there's a I do not want to use a double prefix like So the solution i'd prefer in the abstract is to have |
|
I merged #1189 first, so let's rebase this one on top. |
| util "github.com/ipfs/go-ipfs/util" | ||
| ) | ||
|
|
||
| var DNSCmd = &cmds.Command{ |
|
On Thu, May 07, 2015 at 10:25:33PM -0700, Juan Batiz-Benet wrote:
Well, the URL forms will have some other prefix in front I'll look over the rest of your references and rebase onto the current |
| @@ -81,7 +81,7 @@ func (i *gatewayHandler) resolveNamePath(ctx context.Context, p string) (string, | |||
| if strings.HasPrefix(p, IpnsPathPrefix) { | |||
| elements := strings.Split(p[len(IpnsPathPrefix):], "/") | |||
| hash := elements[0] | |||
| rp, err := i.node.Namesys.Resolve(ctx, hash) | |||
| rp, err := i.node.Namesys.Resolve(ctx, hash, 0) | |||
There was a problem hiding this comment.
don't think we should resolve names infinitely. we should have a max, something like 16 or 32. (i thought there was one?) otherwise we can hose machines by setting up very long chains and causing nodes to resolve for a very long time). (maybe infinitely, if we can generate + publish names faster than the nodes time out)
There was a problem hiding this comment.
On Thu, May 07, 2015 at 10:54:51PM -0700, Juan Batiz-Benet wrote:
@@ -81,7 +81,7 @@ func (i *gatewayHandler) resolveNamePath(ctx context.Context, p string) (string,
if strings.HasPrefix(p, IpnsPathPrefix) {
elements := strings.Split(p[len(IpnsPathPrefix):], "/")
hash := elements[0]
rp, err := i.node.Namesys.Resolve(ctx, hash)rp, err := i.node.Namesys.Resolve(ctx, hash, 0)… otherwise we can hose machines by setting up very long chains and
causing nodes to resolve for a very long time). (maybe infinitely,
if we can generate + publish names faster than the nodes time out)
Or even just by creating a cycle, which is easy enough with DNS links
now, and will become easy with other protocols once we get support for
IPNS → (IPNS | DNS) and IPFS → (IPNS | DNS). I just don't like
hard-coding a magic recursion limit. I'd suggest the usual “configure
per-node in ~/.ipfs/config and override from $IPFS_RECURSION_LIMIT”,
but I haven't had time to look into the config handling. I'm fine
seeding the config value on ‘ipfs init’ with whatever you think is
reasonable.
There was a problem hiding this comment.
We'll have to provide recommended bounds in the specs. this is a protocol problem, not an implementation one. The resolution limit picked will impact all sorts of implementations out there. If some implementation works with limit k, but others with limit k/2, it will create confusion for users. (dns referral / name recursion had a similar problem)
There was a problem hiding this comment.
this is a protocol problem, not an implementation one.
👍
this is a stop-gap:
I'd rather not add another stop-gap ( |
| return | ||
| } | ||
|
|
||
| // TODO: better errors (in the case of not finding the name, we get "failed to find any peer in table") |
There was a problem hiding this comment.
failed to find any peers in table means that bootstrapping was not successful, not that a lookup necessarily resulted in 'not found'
|
@wking status update here? |
|
On Sun, May 10, 2015 at 04:08:26PM -0700, Jeromy Johnson wrote:
After some IRC discussion with @jbenet, I need to reroll this to:
I haven't had time to reroll it yet. Maybe tonight? Maybe tomorrow? |
|
On Sun, May 10, 2015 at 08:31:16PM -0700, W. Trevor King wrote:
Done.
Not done yet, and the test suite passes locally, so maybe we don't I also renamed the multi-protocol type to ‘mpns’ for Multi-Protocol We still need:
|
|
the tests probably pass because we only have tests for ipns records... 50% of that is because i have no idea how to test dns resolution in a sandbox |
wking
changed the title
|
On Tue, May 12, 2015 at 11:40:17PM -0700, Jeromy Johnson wrote:
I'll make net.LookupTXT pluggable, add a mock version, and use the |
|
On Tue, May 19, 2015 at 05:15:39PM -0700, Juan Batiz-Benet wrote:
Done.
I don't mind. I'll open new PRs from branches based in this repo. |
|
Looks like 1d58881 is broken: https://travis-ci.org/ipfs/go-ipfs/builds/63276010 -- the rest all pass. @wking sorry for even more on this PR. I can try to fix it for you later today, if you don't get to it first. (yeah, this |
|
"useful" 😛 |
|
On Wed, May 20, 2015 at 06:58:17AM -0700, Juan Batiz-Benet wrote:
Likely a rebase error: core/commands/resolve.go:78: too many arguments in call to n.Namesys.Resolve I'll squash a fix an repush shortly. |
This allows direct access to the earlier protocol-specific Resolve implementations. The guts of each protocol-specific resolver are in the internal resolveOnce method, and we've added a new: ResolveN(ctx, name, depth) method to the public interface. There's also: Resolve(ctx, name) which wraps ResolveN using DefaultDepthLimit. The extra API endpoint is intended to reduce the likelyhood of clients accidentally calling the more dangerous ResolveN with a nonsensically high or infinite depth. On IRC on 2015-05-17, Juan said: 15:34 <jbenet> If 90% of uses is the reduced API with no chance to screw it up, that's a huge win. 15:34 <wking> Why would those 90% not just set depth=0 or depth=1, depending on which they need? 15:34 <jbenet> Because people will start writing `r.Resolve(ctx, name, d)` where d is a variable. 15:35 <wking> And then accidentally set that variable to some huge number? 15:35 <jbenet> Grom experience, i've seen this happen _dozens_ of times. people screw trivial things up. 15:35 <wking> Why won't those same people be using ResolveN? 15:36 <jbenet> Because almost every example they see will tell them to use Resolve(), and they will mostly stay away from ResolveN. The per-prodocol versions also resolve recursively within their protocol. For example: DNSResolver.Resolve(ctx, "ipfs.io", 0) will recursively resolve DNS links until the referenced value is no longer a DNS link. I also renamed the multi-protocol ipfs NameSystem (defined in namesys/namesys.go) to 'mpns' (for Multi-Protocol Name System), because I wasn't clear on whether IPNS applied to the whole system or just to to the DHT-based system. The new name is unambiguously multi-protocol, which is good. It would be nice to have a distinct name for the DHT-based link system. Now that resolver output is always prefixed with a namespace and unprefixed mpns resolver input is interpreted as /ipfs/, core/corehttp/ipns_hostname.go can dispense with it's old manual /ipfs/ injection. Now that the Resolver interface handles recursion, we don't need the resolveRecurse helper in core/pathresolver.go. The pathresolver cleanup also called for an adjustment to FromSegments to more easily get slash-prefixed paths. Now that recursive resolution with the namesys/namesys.go composite resolver always gets you to an /ipfs/... path, there's no need for the /ipns/ special case in fuse/ipns/ipns_unix.go. Now that DNS links can be things other than /ipfs/ or DHT-link references (e.g. they could be /ipns/<domain-name> references) I've also loosened the ParsePath logic to only attempt multihash validation on IPFS paths. It checks to ensure that other paths have a known-protocol prefix, but otherwise leaves them alone. I also changed some key-stringification from .Pretty() to .String() following the potential deprecation mentioned in util/key.go.
For explicitly enabling recursive behaviour (it was previously always enabled). That allows folks who are interested in understanding layered indirection to step through the chain one link at a time.
This lets users resolve (recursively or not) DNS links without pulling in the other protocols. That makes an easier, more isolated target for alternative implemenations, since they don't need to understand IPNS, proquint, etc. to handle these resolutions.
And add a generic 'ipfs resolve' to handle cross-protocol name resolution.
Previously we had a confusing situation, with:
* single-arg doc: published name <name> to <value>
* double-arg doc: published name <value> to <name>
* implementation: Published name <name> to <value>
Now we have the uniform:
Published to <name>: <value>
With the following goals:
1. It's clear that we're writing <value> to <name>'s IPNS slot in the
DHT.
2. We preserve the order of arguments from the command-line
invocation:
$ ipfs name publish <name> <value>
Published to <name>: <value>
So we can attach a mock lookup function for testing.
Shifting the generic testResolution helper from the protocol-specific dns_test.go to the generic namesys_test.go.
|
On Wed, May 20, 2015 at 08:36:19AM -0700, W. Trevor King wrote:
Pushed. |
Rework mutable namespace resolution to handle recursion
thanks for bearing with me @wking ! :) 👏
|
Added the original logic to check for a invalid path and a simple test.
Catching up with ipfs/kubo@e4447b3c (core/commands/publish: Fix published message, 2015-05-07, ipfs/kubo#1208, landed 2015-05-20 after v0.3.4).
Rework mutable namespace resolution to handle recursion This commit was moved from ipfs/kubo@01e1e71
There are detailed notes in the individual commits, but the meat is in
the opening "namesys: Add recursive resolution". This is a work in
progress towards the separation sketched out here and endorsed
here. For example, with this PR you can:
However, I'm currently sticking on the multihash encoding. The
/ipns/… and /ipfs/… multihashes are base-58, but there's also the
proquint encoding. However, proquint isn't a protocol, you can have
proquint-encoded multihashes in the IPNS and IPFS protocol spaces.
How do we distinguish that? The heavy approach is:
/ipfs-proquint/…
/ipns-proquint/…
in which case maybe we want to be using:
/ipns-base-58/…
/ipfs-base-58/…
where we currently use /ipns/… and /ipfs/…. Thoughts? More clever
solutions? Should the encoding-choice be part of a layer between
binary multihashes and the IP*S libraries? For example, maybe encoded
multihashes should have a leading character or two representing their
encoding (hex “a:1220…”, base-58 “b:Qm…”, proquint
“c:bikad-fokor-zihoj-bajir”, …).
Also, do we want to use /dnslink/… (to match dnslink=…) or /dns/…
(because “link” is mostly wasted space once we're in the context of
resolvable names)?