From fded3bc08ca5629e9494f74e1f76338e2de4797c Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 15 Mar 2019 19:20:53 -0700 Subject: [PATCH 1/3] resolve: recurse by default This is what users expect. fixes #5635 fixes #5585 fixes #4181 fixes #4293 fixes #6086 License: MIT Signed-off-by: Steven Allen --- core/commands/dns.go | 2 +- core/commands/name/ipns.go | 2 +- core/commands/resolve.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/commands/dns.go b/core/commands/dns.go index 2c8e31ed48e..534cf556598 100644 --- a/core/commands/dns.go +++ b/core/commands/dns.go @@ -58,7 +58,7 @@ The resolver can recursively resolve: cmdkit.StringArg("domain-name", true, false, "The domain-name name to resolve.").EnableStdin(), }, Options: []cmdkit.Option{ - cmdkit.BoolOption(dnsRecursiveOptionName, "r", "Resolve until the result is not a DNS link."), + cmdkit.BoolOption(dnsRecursiveOptionName, "r", "Resolve until the result is not a DNS link.").WithDefault(true), }, Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error { recursive, _ := req.Options[dnsRecursiveOptionName].(bool) diff --git a/core/commands/name/ipns.go b/core/commands/name/ipns.go index ed713347f43..012d43fb0b7 100644 --- a/core/commands/name/ipns.go +++ b/core/commands/name/ipns.go @@ -73,7 +73,7 @@ Resolve the value of a dnslink: cmdkit.StringArg("name", false, false, "The IPNS name to resolve. Defaults to your node's peerID."), }, Options: []cmdkit.Option{ - cmdkit.BoolOption(recursiveOptionName, "r", "Resolve until the result is not an IPNS name."), + cmdkit.BoolOption(recursiveOptionName, "r", "Resolve until the result is not an IPNS name.").WithDefault(true), cmdkit.BoolOption(nocacheOptionName, "n", "Do not use cached entries."), cmdkit.UintOption(dhtRecordCountOptionName, "dhtrc", "Number of records to request for DHT resolution."), cmdkit.StringOption(dhtTimeoutOptionName, "dhtt", "Max time to collect values during DHT resolution eg \"30s\". Pass 0 for no timeout."), diff --git a/core/commands/resolve.go b/core/commands/resolve.go index 296deadbb6f..bc6f8850ad7 100644 --- a/core/commands/resolve.go +++ b/core/commands/resolve.go @@ -70,7 +70,7 @@ Resolve the value of an IPFS DAG path: cmdkit.StringArg("name", true, false, "The name to resolve.").EnableStdin(), }, Options: []cmdkit.Option{ - cmdkit.BoolOption(resolveRecursiveOptionName, "r", "Resolve until the result is an IPFS name."), + cmdkit.BoolOption(resolveRecursiveOptionName, "r", "Resolve until the result is an IPFS name.").WithDefault(true), cmdkit.IntOption(resolveDhtRecordCountOptionName, "dhtrc", "Number of records to request for DHT resolution."), cmdkit.StringOption(resolveDhtTimeoutOptionName, "dhtt", "Max time to collect values during DHT resolution eg \"30s\". Pass 0 for no timeout."), }, From 1c7a5d1901d94f438d59e1165f650f2a76bde69e Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 15 Mar 2019 19:22:57 -0700 Subject: [PATCH 2/3] resolve: succeed on recursion limit when not recursing Otherwise, non-recursive resolution is pretty much useless. License: MIT Signed-off-by: Steven Allen --- core/commands/dns.go | 5 +---- core/commands/name/ipns.go | 7 ++++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/core/commands/dns.go b/core/commands/dns.go index 534cf556598..f1f309cd848 100644 --- a/core/commands/dns.go +++ b/core/commands/dns.go @@ -71,10 +71,7 @@ The resolver can recursively resolve: } output, err := resolver.Resolve(req.Context, name, ropts...) - if err == namesys.ErrResolveFailed { - return err - } - if err != nil { + if err != nil && (recursive || err != namesys.ErrResolveRecursion) { return err } return cmds.EmitOnce(res, &ncmd.ResolvedPath{Path: output}) diff --git a/core/commands/name/ipns.go b/core/commands/name/ipns.go index 012d43fb0b7..6557ca09e8e 100644 --- a/core/commands/name/ipns.go +++ b/core/commands/name/ipns.go @@ -8,6 +8,7 @@ import ( "time" cmdenv "github.com/ipfs/go-ipfs/core/commands/cmdenv" + namesys "github.com/ipfs/go-ipfs/namesys" cmdkit "github.com/ipfs/go-ipfs-cmdkit" cmds "github.com/ipfs/go-ipfs-cmds" @@ -130,7 +131,7 @@ Resolve the value of a dnslink: if !stream { output, err := api.Name().Resolve(req.Context, name, opts...) - if err != nil { + if err != nil && (recursive || err != namesys.ErrResolveRecursion) { return err } @@ -143,8 +144,8 @@ Resolve the value of a dnslink: } for v := range output { - if v.Err != nil { - return err + if v.Err != nil && (recursive || v.Err != namesys.ErrResolveRecursion) { + return v.Err } if err := res.Emit(&ResolvedPath{path.FromString(v.Path.String())}); err != nil { return err From 417bd243e22c76436d9728acd8f38b8a167ceb0d Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Sat, 16 Mar 2019 12:42:50 -0700 Subject: [PATCH 3/3] test resolve recursion License: MIT Signed-off-by: Steven Allen --- test/sharness/t0160-resolve.sh | 101 +++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 44 deletions(-) diff --git a/test/sharness/t0160-resolve.sh b/test/sharness/t0160-resolve.sh index 77d7ac12c59..87740685644 100755 --- a/test/sharness/t0160-resolve.sh +++ b/test/sharness/t0160-resolve.sh @@ -21,27 +21,26 @@ test_expect_success "resolve: prepare dag" ' dag_hash=$(ipfs dag put <<<"{\"i\": {\"j\": {\"k\": \"asdfasdfasdf\"}}}") ' +test_expect_success "resolve: prepare keys" ' + self_hash=$(ipfs id -f="") && + alt_hash=$(ipfs key gen -t rsa alt) +' + test_resolve_setup_name() { - ref=$1 - - test_expect_success "resolve: prepare name" ' - id_hash=$(ipfs id -f="") && - ipfs name publish --allow-offline "$ref" && - printf "$ref\n" >expected_nameval && - ipfs name resolve >actual_nameval && - test_cmp expected_nameval actual_nameval + local key="$1" + local ref="$2" + + test_expect_success "resolve: prepare $key" ' + ipfs name publish --key="$key" --allow-offline "$ref" ' } test_resolve_setup_name_fail() { - ref=$1 - - test_expect_failure "resolve: prepare name" ' - id_hash=$(ipfs id -f="") && - ipfs name publish --allow-offline "$ref" && - printf "$ref" >expected_nameval && - ipfs name resolve >actual_nameval && - test_cmp expected_nameval actual_nameval + local key="$1" + local ref="$2" + + test_expect_failure "resolve: prepare $key" ' + ipfs name publish --key="$key" --allow-offline "$ref" ' } @@ -51,7 +50,7 @@ test_resolve() { extra=$3 test_expect_success "resolve succeeds: $src" ' - ipfs resolve $extra -r "$src" >actual + ipfs resolve $extra "$src" >actual ' test_expect_success "resolved correctly: $src -> $dst" ' @@ -69,17 +68,31 @@ test_resolve_cmd() { test_resolve "/ipld/$dag_hash/i/j" "/ipld/$dag_hash/i/j" test_resolve "/ipld/$dag_hash/i" "/ipld/$dag_hash/i" - test_resolve_setup_name "/ipfs/$a_hash" - test_resolve "/ipns/$id_hash" "/ipfs/$a_hash" - test_resolve "/ipns/$id_hash/b" "/ipfs/$b_hash" - test_resolve "/ipns/$id_hash/b/c" "/ipfs/$c_hash" + test_resolve_setup_name "self" "/ipfs/$a_hash" + test_resolve "/ipns/$self_hash" "/ipfs/$a_hash" + test_resolve "/ipns/$self_hash/b" "/ipfs/$b_hash" + test_resolve "/ipns/$self_hash/b/c" "/ipfs/$c_hash" - test_resolve_setup_name "/ipfs/$b_hash" - test_resolve "/ipns/$id_hash" "/ipfs/$b_hash" - test_resolve "/ipns/$id_hash/c" "/ipfs/$c_hash" + test_resolve_setup_name "self" "/ipfs/$b_hash" + test_resolve "/ipns/$self_hash" "/ipfs/$b_hash" + test_resolve "/ipns/$self_hash/c" "/ipfs/$c_hash" - test_resolve_setup_name "/ipfs/$c_hash" - test_resolve "/ipns/$id_hash" "/ipfs/$c_hash" + test_resolve_setup_name "self" "/ipfs/$c_hash" + test_resolve "/ipns/$self_hash" "/ipfs/$c_hash" + + # simple recursion succeeds + test_resolve_setup_name "alt" "/ipns/$self_hash" + test_resolve "/ipns/$alt_hash" "/ipfs/$c_hash" + + # partial resolve succeeds + test_resolve "/ipns/$alt_hash" "/ipns/$self_hash" -r=false + + # infinite recursion fails + test_resolve_setup_name "self" "/ipns/$self_hash" + test_expect_success "recursive resolve terminates" ' + test_expect_code 1 ipfs resolve /ipns/$self_hash 2>recursion_error && + grep "recursion limit exceeded" recursion_error + ' } test_resolve_cmd_b32() { @@ -92,17 +105,17 @@ test_resolve_cmd_b32() { # flags needed passed in path does not contain cid to derive base - test_resolve_setup_name "/ipfs/$a_hash_b32" - test_resolve "/ipns/$id_hash" "/ipfs/$a_hash_b32" --cid-base=base32 - test_resolve "/ipns/$id_hash/b" "/ipfs/$b_hash_b32" --cid-base=base32 - test_resolve "/ipns/$id_hash/b/c" "/ipfs/$c_hash_b32" --cid-base=base32 + test_resolve_setup_name "self" "/ipfs/$a_hash_b32" + test_resolve "/ipns/$self_hash" "/ipfs/$a_hash_b32" --cid-base=base32 + test_resolve "/ipns/$self_hash/b" "/ipfs/$b_hash_b32" --cid-base=base32 + test_resolve "/ipns/$self_hash/b/c" "/ipfs/$c_hash_b32" --cid-base=base32 - test_resolve_setup_name "/ipfs/$b_hash_b32" --cid-base=base32 - test_resolve "/ipns/$id_hash" "/ipfs/$b_hash_b32" --cid-base=base32 - test_resolve "/ipns/$id_hash/c" "/ipfs/$c_hash_b32" --cid-base=base32 + test_resolve_setup_name "self" "/ipfs/$b_hash_b32" --cid-base=base32 + test_resolve "/ipns/$self_hash" "/ipfs/$b_hash_b32" --cid-base=base32 + test_resolve "/ipns/$self_hash/c" "/ipfs/$c_hash_b32" --cid-base=base32 - test_resolve_setup_name "/ipfs/$c_hash_b32" - test_resolve "/ipns/$id_hash" "/ipfs/$c_hash_b32" --cid-base=base32 + test_resolve_setup_name "self" "/ipfs/$c_hash_b32" + test_resolve "/ipns/$self_hash" "/ipfs/$c_hash_b32" --cid-base=base32 } @@ -131,17 +144,17 @@ test_resolve_cmd_fail() { test_resolve "/ipld/$dag_hash/i/j" "/ipld/$dag_hash/i/j" test_resolve "/ipld/$dag_hash/i" "/ipld/$dag_hash/i" - test_resolve_setup_name_fail "/ipfs/$a_hash" - test_resolve_fail "/ipns/$id_hash" "/ipfs/$a_hash" - test_resolve_fail "/ipns/$id_hash/b" "/ipfs/$b_hash" - test_resolve_fail "/ipns/$id_hash/b/c" "/ipfs/$c_hash" + test_resolve_setup_name_fail "self" "/ipfs/$a_hash" + test_resolve_fail "/ipns/$self_hash" "/ipfs/$a_hash" + test_resolve_fail "/ipns/$self_hash/b" "/ipfs/$b_hash" + test_resolve_fail "/ipns/$self_hash/b/c" "/ipfs/$c_hash" - test_resolve_setup_name_fail "/ipfs/$b_hash" - test_resolve_fail "/ipns/$id_hash" "/ipfs/$b_hash" - test_resolve_fail "/ipns/$id_hash/c" "/ipfs/$c_hash" + test_resolve_setup_name_fail "self" "/ipfs/$b_hash" + test_resolve_fail "/ipns/$self_hash" "/ipfs/$b_hash" + test_resolve_fail "/ipns/$self_hash/c" "/ipfs/$c_hash" - test_resolve_setup_name_fail "/ipfs/$c_hash" - test_resolve_fail "/ipns/$id_hash" "/ipfs/$c_hash" + test_resolve_setup_name_fail "self" "/ipfs/$c_hash" + test_resolve_fail "/ipns/$self_hash" "/ipfs/$c_hash" } # should work offline