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

Rework mutable namespace resolution to handle recursion #1208

Merged
merged 11 commits into from
May 20, 2015

Conversation

wking
Copy link
Contributor

@wking wking commented May 7, 2015

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:

$ ipfs dns ipfs.tremily.us
/dnslink/tremily.us
$ ipfs dns tremily.us
/dnslink/tremily.us
$ ipfs dns --recursive ipfs.tremily.us
/ipns/QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7

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)?

@jbenet jbenet added the status/in-progress In progress label May 7, 2015
@wking
Copy link
Contributor Author

wking commented May 7, 2015

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.

@cryptix
Copy link
Contributor

cryptix commented May 7, 2015

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 depth int recursion parameter. Would would it mean to recurse with 42? If it's function is to specify to wich protocol level the use wants to resolve i'd like something like this much more:

type Protocol uint
const (
    DNS Protocol = iota
    IPNS
    IPFS
)

type Resolver interface {
    Resolve(ctx context.Context, name string, proto Protocol) (path.Path, error)
}

@wking
Copy link
Contributor Author

wking commented May 7, 2015

On Thu, May 07, 2015 at 04:35:11PM -0700, Henry wrote:

One thing i'm not too sure about is exposing the depth int
recursion parameter. Would would it mean to recurse with 42?

“Please resolve this recursively, but bail out after 42 cycles”. This
lets you do things like the current maxLinks stuff [1,2], although I'd
rather be setting that limit via a config variable (and as this PR
stands, the limit is hardcoded to the infinite 0 in several places).

If it's function is to specify to wich protocol level the use wants
to resolve…

Nope. The per-protocol handlers currently just take the bare object.
For example:

DNSResolver.Resolve(ctx, "ipfs.io", 0)

and the multi-protocol resolver gets the protocol from a prefix:

NewNameSystem(…).Resolve(ctx, "/dnslink/ipfs.io", 0)

@wking
Copy link
Contributor Author

wking commented May 8, 2015

I've rebased this onto #1209 to get my local test suite going again,
and squashed in a few more fixes, changed ‘ipfs name resolve …’ to be
IPNS-only, and shifted the multi-protocol version to ‘ipfs resolve …’.

Besides the encoding issues (base-58 vs. hex vs. proquint) mentioned
in my lead-off comment, I'm also concerned about FUSE mount handling.
Currently /ipns/… is back to IPNS-only handling (e.g. you can't use
/ipns/example.com/). We could pass the first two segments of the
post-mount-point path through to use the new explicit-prefix resolver
(e.g. /ipns/dnslink/example.com/), but that makes the /ipns/ bit seem
strange. Another alternative is to have a separate root directory for
each protocol (/ipfs/…, /ipns/…, /dnslink/…, …), but that seems ugly
too. How about /ipr/ipfs/…, /ipr/ipns/…, /ipr/dnslink/…, where ipr
stands for “InterPlanetary Resolver”?

@jbenet
Copy link
Member

jbenet commented May 8, 2015

@wking this is looking really good! I'll sink into CR in a bit. Do you want it to be merged after or before @cryptix's stuff? Whatever is easiest for you both.

@jbenet
Copy link
Member

jbenet commented May 8, 2015

Let's use /dns/....


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 /ipfs and /ipns links use base58 only. (as you point out, what does it mean to have /ipfs/<hex>?). I think this is --again-- a place where we want to use self-description to signal the encoding. (this is a very tricky change because it would break links up to now, unless we blessed base58 specially)

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 (<word>-)+<word> pattern it's a proquint, if there's a domain it's dns (+ .bit/.onion "tlds"). else it's a hash. This works for the most part.

I do not want to use a double prefix like /ipr because (a) /ipfs and /ipns is already a lot-- these are not just paths, they're URLs. There's a constraint here where the web addresses and the unix pathnames should be the same, so that users can freely enter the exact same string in both (something we cannot do today). and (b), i want to push things towards another idea, fs-root (moving unix filesystems towards mounting lots of protocols).

So the solution i'd prefer in the abstract is to have /dns/... be a top level. I think this definitely would make sense for a dnslink tool. but think it is clunky in the ipfs toolchain because now we're asking for 3 directories at root. there is also the fact that we'd like /ipns/ links to work for users, with dns being a bit of a hack. (so /ipns/ is acting like a resolver of dns names, and ipns pki records). I think in the dns cases, the expression matching works well in practice-- domains and hashes are distinctive enough. proquint is sort of an encoding, so maybe we should be treating it like hex (maybe a self-describing prefix?). -- the gist is that i agree it's impure, but not sure we can expect the world to go full on to "fs root with all the protocol prefixes \o/" land just yet.

@jbenet
Copy link
Member

jbenet commented May 8, 2015

I merged #1189 first, so let's rebase this one on top.

util "github.com/ipfs/go-ipfs/util"
)

var DNSCmd = &cmds.Command{
Copy link
Member

Choose a reason for hiding this comment

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

👍 to this command :)

@wking
Copy link
Contributor Author

wking commented May 8, 2015

On Thu, May 07, 2015 at 10:25:33PM -0700, Juan Batiz-Benet wrote:

I do not want to use a double prefix like /ipr because (a) /ipfs
and /ipns is already a lot-- these are not just paths, they're
URLs.

Well, the URL forms will have some other prefix in front
(e.g. http://gateway.ipfs.io/). I think a local filesystem prefix
like /ipr/ is similar (you can cut and paste between the two once you
know both prefixes), and it allows us to make as many per-protocol
directories as we want without needing lots of root-permission steps.
But this IPR-mount-point-prefix should be configurable anyway (and
might already be), so maybe I should just tweak my local config to
use /ipr/… ;). Then we/I can remove the /ipr/ prefix once the
root-prefixed protocol paths get better traction.

I'll look over the rest of your references and rebase onto the current
master tomorrow.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

this is a protocol problem, not an implementation one.

👍

@jbenet
Copy link
Member

jbenet commented May 8, 2015

Well, the URL forms will have some other prefix in front (e.g. http://gateway.ipfs.io/).

this is a stop-gap:

  • it will shortly be just http://ipfs.io/
  • I hope to get a tld (so /ipfs/... just works)
  • the firefox + chrome extensions already resolve paths directly (i believe? or will soon?). and
  • we can convince browsers vendors to implement support for it.

I'd rather not add another stop-gap (the /ipr prefix) where we don't absolutely have to-- i understand the flexibility niceness, but the problem is people are going to start hard-coding /ipr into things and that's going to hurt a lot down the road. (this is also why i tell people to link to ipfs with /ipfs/... and not with gateway.ipfs.io/ipfs/... unless they absolutely have to (they dont control the rendering). In fact, i'll likely start making links like this from now on: /ipfs/QmSYCpuKPbPQ2iFr2swJj2hvz7wQUXfPBXPiuVsQdL5FEs/cat.jpg

return
}

// TODO: better errors (in the case of not finding the name, we get "failed to find any peer in table")
Copy link
Member

Choose a reason for hiding this comment

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

failed to find any peers in table means that bootstrapping was not successful, not that a lookup necessarily resulted in 'not found'

@whyrusleeping
Copy link
Member

@wking status update here?

@wking
Copy link
Contributor Author

wking commented May 11, 2015

On Sun, May 10, 2015 at 04:08:26PM -0700, Jeromy Johnson wrote:

@wking status update here?

After some IRC discussion with @jbenet, I need to reroll this to:

  1. Leave the current “try all the protocols” handling in place. We're
    putting off the migration towards explicit per-protocol prefixes
    for now.
  2. Maybe not update the IPNS resolver to be DHT-entry specific (it's
    currently multi-protocol)?

I haven't had time to reroll it yet. Maybe tonight? Maybe tomorrow?
I also haven't gotten through a full test run yet, mostly because
something will hang at some point. I haven't looked into this
carefully enough to know if it's always the same test hanging, or if
it's a collection of random unreliable tests.

@wking
Copy link
Contributor Author

wking commented May 13, 2015

On Sun, May 10, 2015 at 08:31:16PM -0700, W. Trevor King wrote:

After some IRC discussion with @jbenet, I need to reroll this to:

  1. Leave the current “try all the protocols” handling in place. We're
    putting off the migration towards explicit per-protocol prefixes
    for now.

Done.

  1. Maybe not update the IPNS resolver to be DHT-entry specific (it's
    currently multi-protocol)?

Not done yet, and the test suite passes locally, so maybe we don't
need to? This branch adds ‘ipfs resolve …’ which handles
multi-protocol resolution, and that should be a simple migration for
users.

I also renamed the multi-protocol type to ‘mpns’ for Multi-Protocol
Name System to avoid previous confusion between IPNS for the whole
mutable space or just the DHT links 1.

We still need:

  • Docs. Where should these live? Is the command-line help sufficent?
    Do I need migration notes about the ‘ipfs name resolve …’ scope
    changes? Output format changes?
  • Possibly more consistent error handling 2.
  • A cap on recursive resolution 3. Maybe hard code a default for
    now (depth == 0 → 32?) and add configurable defaults at the
    ipfs-config and command-line-option levels in follow-up PRs?

@whyrusleeping
Copy link
Member

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 wking changed the title Rework /ipfs/…, /ipns/…, /dnslink/… resolution to separate protocols Rework mutable namespace resolution to handle recursion May 13, 2015
@wking
Copy link
Contributor Author

wking commented May 13, 2015

On Tue, May 12, 2015 at 11:40:17PM -0700, Jeromy Johnson wrote:

50% of that is because i have no idea how to test dns resolution in
a sandbox

I'll make net.LookupTXT pluggable, add a mock version, and use the
mock in new namesys/dns_test.go and namesys/namesys_test.go tests.
Will that address your concerns?

@wking
Copy link
Contributor Author

wking commented May 20, 2015

On Tue, May 19, 2015 at 05:15:39PM -0700, Juan Batiz-Benet wrote:

Actually, we probably should've rebased on new master (fixing travis
things) before this...

Done.

@wking i'd love to be able to do this on your PRs-- could we move to
a workflow where you push a branch to this repo and PR from here?

I don't mind. I'll open new PRs from branches based in this repo.

@jbenet
Copy link
Member

jbenet commented May 20, 2015

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 git push-each thing turned out to be useful :) )

@whyrusleeping
Copy link
Member

"useful"

😛

@wking
Copy link
Contributor Author

wking commented May 20, 2015

On Wed, May 20, 2015 at 06:58:17AM -0700, Juan Batiz-Benet wrote:

Looks like 1d58881 is broken: https://travis-ci.org/ipfs/go-ipfs/builds/63276010

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.

wking added 8 commits May 20, 2015 08:40
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.
@wking
Copy link
Contributor Author

wking commented May 20, 2015

On Wed, May 20, 2015 at 08:36:19AM -0700, W. Trevor King wrote:

Wed, May 20, 2015 at 06:58:17AM -0700, Juan Batiz-Benet:

Looks like 1d58881 is broken:
https://travis-ci.org/ipfs/go-ipfs/builds/63276010

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.

Pushed.

jbenet added a commit that referenced this pull request May 20, 2015
Rework mutable namespace resolution to handle recursion
@jbenet jbenet merged commit 01e1e71 into ipfs:master May 20, 2015
@jbenet jbenet removed the status/in-progress In progress label May 20, 2015
@jbenet
Copy link
Member

jbenet commented May 20, 2015

thanks for bearing with me @wking ! :) 👏


wking referenced this pull request in travisperson/go-ipfs May 21, 2015
Added the original logic to check for a invalid path and a simple test.
wking added a commit to wking/ipfs-examples that referenced this pull request May 27, 2015
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).
@wking wking deleted the dns-resolver branch June 9, 2015 04:03
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
Rework mutable namespace resolution to handle recursion

This commit was moved from ipfs/kubo@01e1e71
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.

5 participants