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.

Sign up for GitHub

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

Jump to bottom

Add options for record count and timeout for resolving DHT paths #4733

Merged
merged 8 commits into from
Mar 23, 2018
Merged

Add options for record count and timeout for resolving DHT paths #4733

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1abf836
Add options for record count and timeout for resolving DHT paths
dirkmc Feb 23, 2018
5096025
Fix gateway test
dirkmc Feb 26, 2018
e8f79c8
Use variadic options
dirkmc Feb 28, 2018
d47ff79
Document namesys options
dirkmc Feb 28, 2018
350362c
Fix namesys opts package name
dirkmc Feb 28, 2018
5202f76
Syntactic changes
dirkmc Mar 1, 2018
32ef57c
Return ErrNotFound when zero values are returned from DHT namesys res…
dirkmc Mar 22, 2018
494f242
Parse string time (eg 30s) instead of assuming seconds for —-dht-timeout
dirkmc Mar 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/commands/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ The resolver can recursively resolve:
name := req.Arguments()[0]
resolver := namesys.NewDNSResolver()

depth := 1
if recursive {
depth = namesys.DefaultDepthLimit
opts := namesys.DefaultResolveOpts()
if !recursive {
opts.Depth = 1
}
output, err := resolver.ResolveN(req.Context(), name, depth)
output, err := resolver.Resolve(req.Context(), name, opts)
if err == namesys.ErrResolveFailed {
res.SetError(err, cmdkit.ErrNotFound)
return
Expand Down
20 changes: 15 additions & 5 deletions core/commands/ipns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"io"
"strings"
"time"

cmds "github.com/ipfs/go-ipfs/commands"
e "github.com/ipfs/go-ipfs/core/commands/e"
Expand Down Expand Up @@ -57,9 +58,10 @@ Resolve the value of a dnslink:
Options: []cmdkit.Option{
cmdkit.BoolOption("recursive", "r", "Resolve until the result is not an IPNS name."),
cmdkit.BoolOption("nocache", "n", "Do not use cached entries."),
cmdkit.UintOption("dht-record-count", "dhtrc", "Number of records to request for DHT resolution."),
cmdkit.UintOption("dht-timeout", "dhtt", "Timeout in seconds for DHT resolution. Pass 0 for no timeout."),
Copy link
Member

Choose a reason for hiding this comment

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

So, I might be missing something but IPFS commands have a timeout flag (e.g., ipfs --timeout=30s). And we can always set a DHT resolution timeout by setting a timeout on the context. What's the usecase for the additional timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case is that with --dht-timeout the DHT collects as many values as it can within the timeout, and then returns the best value it has found. The global IPFS command --timeout will simply kill the command:

> ./cmd/ipfs/ipfs name resolve --timeout=5s QmSgxPeqLrnM1ZB1pno3tYMQfqLB4cWXuAQNfSWsk77apK
Error: Post http://127.0.0.1:5001/api/v0/name/resolve?arg=QmSgxPeqLrnM1ZB1pno3tYMQfqLB4cWXuAQNfSWsk77apK&encoding=json&stream-channels=true&timeout=5s: context deadline exceeded
> ./cmd/ipfs/ipfs name resolve --dhtt=5 QmSgxPeqLrnM1ZB1pno3tYMQfqLB4cWXuAQNfSWsk77apK
/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be better for the --dht-timeout parameter to take a string though, eg --dht-timeout=5s instead of assuming seconds eg --dht-timeout=5

Copy link
Member

Choose a reason for hiding this comment

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

I see. It would be nice to explain that in the help but I can't think of a succinct way of putting it...

Copy link
Member

Choose a reason for hiding this comment

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

@dirkmc it will be better for it to parse the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a commit to parse the time, and changed the parameter description to
Max time to collect values during DHT resolution eg \"30s\". Pass 0 for no timeout
That's the most succinct way I can think of to say it

},
Run: func(req cmds.Request, res cmds.Response) {

n, err := req.InvocContext().GetNode()
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
Expand Down Expand Up @@ -107,16 +109,24 @@ Resolve the value of a dnslink:
}

recursive, _, _ := req.Option("recursive").Bool()
depth := 1
if recursive {
depth = namesys.DefaultDepthLimit
rc, rcok, _ := req.Option("dht-record-count").Int()
dhtt, dhttok, _ := req.Option("dht-timeout").Int()
opts := namesys.DefaultResolveOpts()
if !recursive {
opts.Depth = 1
}
if rcok {
opts.DhtRecordCount = uint(rc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can get rid of this cast once ipfs/go-ipfs-cmdkit#14 is approved

}
if dhttok {
opts.DhtTimeout = time.Duration(dhtt) * time.Second
}

if !strings.HasPrefix(name, "/ipns/") {
name = "/ipns/" + name
}

output, err := resolver.ResolveN(req.Context(), name, depth)
Copy link
Member

Choose a reason for hiding this comment

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

where did the depth arg go?

Copy link
Member

Choose a reason for hiding this comment

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

oh nvm, i found it

output, err := resolver.Resolve(req.Context(), name, opts)
if err != nil {
res.SetError(err, cmdkit.ErrNormal)
return
Expand Down
15 changes: 14 additions & 1 deletion core/commands/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package commands
import (
"io"
"strings"
"time"

cmds "github.com/ipfs/go-ipfs/commands"
"github.com/ipfs/go-ipfs/core"
Expand Down Expand Up @@ -62,6 +63,8 @@ Resolve the value of an IPFS DAG path:
},
Options: []cmdkit.Option{
cmdkit.BoolOption("recursive", "r", "Resolve until the result is an IPFS name."),
cmdkit.UintOption("dht-record-count", "dhtrc", "Number of records to request for DHT resolution."),
cmdkit.UintOption("dht-timeout", "dhtt", "Timeout in seconds for DHT resolution. Pass 0 for no timeout."),
},
Run: func(req cmds.Request, res cmds.Response) {

Expand All @@ -84,7 +87,17 @@ Resolve the value of an IPFS DAG path:

// the case when ipns is resolved step by step
if strings.HasPrefix(name, "/ipns/") && !recursive {
p, err := n.Namesys.ResolveN(req.Context(), name, 1)
rc, rcok, _ := req.Option("dht-record-count").Int()
dhtt, dhttok, _ := req.Option("dht-timeout").Int()
opts := ns.DefaultResolveOpts()
opts.Depth = 1
if rcok {
opts.DhtRecordCount = uint(rc)
}
if dhttok {
opts.DhtTimeout = time.Duration(dhtt) * time.Second
}
p, err := n.Namesys.Resolve(req.Context(), name, opts)
// ErrResolveRecursion is fine
if err != nil && err != ns.ErrResolveRecursion {
res.SetError(err, cmdkit.ErrNormal)
Expand Down
8 changes: 4 additions & 4 deletions core/coreapi/name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,16 @@ func (api *NameAPI) Resolve(ctx context.Context, name string, opts ...caopts.Nam
resolver = namesys.NewNameSystem(n.Routing, n.Repo.Datastore(), 0)
}

depth := 1
if options.Recursive {
depth = namesys.DefaultDepthLimit
ropts := namesys.DefaultResolveOpts()
if !options.Recursive {
ropts.Depth = 1
}

if !strings.HasPrefix(name, "/ipns/") {
name = "/ipns/" + name
}

output, err := resolver.ResolveN(ctx, name, depth)
output, err := resolver.Resolve(ctx, name, ropts)
if err != nil {
return nil, err
}
Expand Down
6 changes: 1 addition & 5 deletions core/corehttp/gateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,7 @@ var emptyDir = "/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn"

type mockNamesys map[string]path.Path

func (m mockNamesys) Resolve(ctx context.Context, name string) (value path.Path, err error) {
return m.ResolveN(ctx, name, namesys.DefaultDepthLimit)
}

func (m mockNamesys) ResolveN(ctx context.Context, name string, depth int) (value path.Path, err error) {
func (m mockNamesys) Resolve(ctx context.Context, name string, opts *namesys.ResolveOpts) (value path.Path, err error) {
p, ok := m[name]
if !ok {
return "", namesys.ErrResolveFailed
Expand Down
3 changes: 2 additions & 1 deletion core/corehttp/ipns_hostname.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"github.com/ipfs/go-ipfs/core"
namesys "github.com/ipfs/go-ipfs/namesys"

isd "gx/ipfs/QmZmmuAXgX73UQmX1jRKjTGmjzq24Jinqkq8vzkBtno4uX/go-is-domain"
)
Expand All @@ -24,7 +25,7 @@ func IPNSHostnameOption() ServeOption {
host := strings.SplitN(r.Host, ":", 2)[0]
if len(host) > 0 && isd.IsDomain(host) {
name := "/ipns/" + host
if _, err := n.Namesys.Resolve(ctx, name); err == nil {
if _, err := n.Namesys.Resolve(ctx, name, namesys.DefaultResolveOpts()); err == nil {
r.Header["X-Ipns-Original-Path"] = []string{r.URL.Path}
r.URL.Path = name + r.URL.Path
}
Expand Down
2 changes: 1 addition & 1 deletion core/pathresolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func Resolve(ctx context.Context, nsys namesys.NameSystem, r *resolver.Resolver,
return nil, err
}

respath, err := nsys.Resolve(ctx, resolvable.String())
respath, err := nsys.Resolve(ctx, resolvable.String(), namesys.DefaultResolveOpts())
if err != nil {
evt.Append(logging.LoggableMap{"error": err.Error()})
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion fuse/ipns/ipns_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ func (s *Root) Lookup(ctx context.Context, name string) (fs.Node, error) {
}

// other links go through ipns resolution and are symlinked into the ipfs mountpoint
resolved, err := s.Ipfs.Namesys.Resolve(s.Ipfs.Context(), name)
resolved, err := s.Ipfs.Namesys.Resolve(s.Ipfs.Context(), name, namesys.DefaultResolveOpts())
if err != nil {
log.Warningf("ipns: namesys resolve error: %s", err)
return nil, fuse.ENOENT
Expand Down
7 changes: 4 additions & 3 deletions namesys/base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ import (

type resolver interface {
// resolveOnce looks up a name once (without recursion).
resolveOnce(ctx context.Context, name string) (value path.Path, err error)
resolveOnce(ctx context.Context, name string, opts *ResolveOpts) (value path.Path, err error)
}

// resolve is a helper for implementing Resolver.ResolveN using resolveOnce.
func resolve(ctx context.Context, r resolver, name string, depth int, prefixes ...string) (path.Path, error) {
func resolve(ctx context.Context, r resolver, name string, opts *ResolveOpts, prefixes ...string) (path.Path, error) {
depth := opts.Depth
for {
p, err := r.resolveOnce(ctx, name)
p, err := r.resolveOnce(ctx, name, opts)
if err != nil {
return "", err
}
Expand Down
11 changes: 3 additions & 8 deletions namesys/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,8 @@ func newDNSResolver() resolver {
}

// Resolve implements Resolver.
func (r *DNSResolver) Resolve(ctx context.Context, name string) (path.Path, error) {
return r.ResolveN(ctx, name, DefaultDepthLimit)
}

// ResolveN implements Resolver.
func (r *DNSResolver) ResolveN(ctx context.Context, name string, depth int) (path.Path, error) {
return resolve(ctx, r, name, depth, "/ipns/")
func (r *DNSResolver) Resolve(ctx context.Context, name string, opts *ResolveOpts) (path.Path, error) {
return resolve(ctx, r, name, opts, "/ipns/")
}

type lookupRes struct {
Expand All @@ -48,7 +43,7 @@ type lookupRes struct {
// resolveOnce implements resolver.
// TXT records for a given domain name should contain a b58
// encoded multihash.
func (r *DNSResolver) resolveOnce(ctx context.Context, name string) (path.Path, error) {
func (r *DNSResolver) resolveOnce(ctx context.Context, name string, opts *ResolveOpts) (path.Path, error) {
segments := strings.SplitN(name, "/", 2)
domain := segments[0]

Expand Down
13 changes: 2 additions & 11 deletions namesys/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,8 @@ type Resolver interface {
//
// There is a default depth-limit to avoid infinite recursion. Most
// users will be fine with this default limit, but if you need to
// adjust the limit you can use ResolveN.
Resolve(ctx context.Context, name string) (value path.Path, err error)

// ResolveN performs a recursive lookup, returning the dereferenced
// path. The only difference from Resolve is that the depth limit
// is configurable. You can use DefaultDepthLimit, UnlimitedDepth,
// or a depth limit of your own choosing.
//
// Most users should use Resolve, since the default limit works well
// in most real-world situations.
ResolveN(ctx context.Context, name string, depth int) (value path.Path, err error)
// adjust the limit you can specify it as an option.
Resolve(ctx context.Context, name string, opts *ResolveOpts) (value path.Path, err error)
}

// Publisher is an object capable of publishing particular names.
Expand Down
31 changes: 22 additions & 9 deletions namesys/ipns_validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func TestResolverValidation(t *testing.T) {
}

// Resolve entry
resp, err := resolver.resolveOnce(ctx, id.Pretty())
resp, err := resolver.resolveOnce(ctx, id.Pretty(), DefaultResolveOpts())
if err != nil {
t.Fatal(err)
}
Expand All @@ -136,9 +136,9 @@ func TestResolverValidation(t *testing.T) {
}

// Record should fail validation because entry is expired
_, err = resolver.resolveOnce(ctx, id.Pretty())
if err != ErrExpiredRecord {
t.Fatal("ValidateIpnsRecord should have returned ErrExpiredRecord")
_, err = resolver.resolveOnce(ctx, id.Pretty(), DefaultResolveOpts())
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why arent we checking for a specific error anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The resolveOnce() method used to call routing.GetValue() but now we call routing.GetValues() directly.
The mock ValueStore implementation's GetValue() method would get a single record and call VerifyRecord() on it, which would return ErrExpiredRecord/ErrSignature etc. We now call the mock ValueStore implementation's GetValues() method, which gets multiple records, calls VerifyRecord() on each one individually, and adds the record to the list of valid records if there's no error. If there are no valid records it returns routing.ErrNotFound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into whether I can check for routing.ErrNotFound but the go-ipfs-routing mock returns datastore.ErrNotFound instead of routing.ErrNotFound when it doesn't find a key:
https://github.com/ipfs/go-ipfs-routing/blob/master/mock/centralized_client.go#L46

Copy link
Member

Choose a reason for hiding this comment

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

Alright, that seems fine. Thanks for the response!

t.Fatal("ValidateIpnsRecord should have returned error")
}

// Create IPNS record path with a different private key
Expand All @@ -158,8 +158,8 @@ func TestResolverValidation(t *testing.T) {

// Record should fail validation because public key defined by
// ipns path doesn't match record signature
_, err = resolver.resolveOnce(ctx, id2.Pretty())
if err != ErrSignature {
_, err = resolver.resolveOnce(ctx, id2.Pretty(), DefaultResolveOpts())
if err == nil {
t.Fatal("ValidateIpnsRecord should have failed signature verification")
}

Expand All @@ -176,7 +176,7 @@ func TestResolverValidation(t *testing.T) {

// Record should fail validation because public key is not available
// in peer store or on network
_, err = resolver.resolveOnce(ctx, id3.Pretty())
_, err = resolver.resolveOnce(ctx, id3.Pretty(), DefaultResolveOpts())
if err == nil {
t.Fatal("ValidateIpnsRecord should have failed because public key was not found")
}
Expand All @@ -191,7 +191,7 @@ func TestResolverValidation(t *testing.T) {
// public key is available in the peer store by looking it up in
// the DHT, which causes the DHT to fetch it and cache it in the
// peer store
_, err = resolver.resolveOnce(ctx, id3.Pretty())
_, err = resolver.resolveOnce(ctx, id3.Pretty(), DefaultResolveOpts())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -263,7 +263,20 @@ func (m *mockValueStore) GetPublicKey(ctx context.Context, p peer.ID) (ci.PubKey
}

func (m *mockValueStore) GetValues(ctx context.Context, k string, count int) ([]routing.RecvdVal, error) {
return m.r.GetValues(ctx, k, count)
vals, err := m.r.GetValues(ctx, k, count)
if err != nil {
return nil, err
}
valid := make([]routing.RecvdVal, 0, len(vals))
for _, v := range vals {
rec := new(recordpb.Record)
rec.Key = proto.String(k)
rec.Value = v.Val
if err = m.Validator.VerifyRecord(rec); err == nil {
valid = append(valid, v)
}
}
return valid, nil
}

func (m *mockValueStore) PutValue(ctx context.Context, k string, d []byte) error {
Expand Down
19 changes: 7 additions & 12 deletions namesys/namesys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,7 @@ func AddPubsubNameSystem(ctx context.Context, ns NameSystem, host p2phost.Host,
const DefaultResolverCacheTTL = time.Minute

// Resolve implements Resolver.
func (ns *mpns) Resolve(ctx context.Context, name string) (path.Path, error) {
return ns.ResolveN(ctx, name, DefaultDepthLimit)
}

// ResolveN implements Resolver.
func (ns *mpns) ResolveN(ctx context.Context, name string, depth int) (path.Path, error) {
func (ns *mpns) Resolve(ctx context.Context, name string, opts *ResolveOpts) (path.Path, error) {
if strings.HasPrefix(name, "/ipfs/") {
return path.ParsePath(name)
}
Expand All @@ -81,11 +76,11 @@ func (ns *mpns) ResolveN(ctx context.Context, name string, depth int) (path.Path
return path.ParsePath("/ipfs/" + name)
}

return resolve(ctx, ns, name, depth, "/ipns/")
return resolve(ctx, ns, name, opts, "/ipns/")
}

// resolveOnce implements resolver.
func (ns *mpns) resolveOnce(ctx context.Context, name string) (path.Path, error) {
func (ns *mpns) resolveOnce(ctx context.Context, name string, opts *ResolveOpts) (path.Path, error) {
if !strings.HasPrefix(name, "/ipns/") {
name = "/ipns/" + name
}
Expand Down Expand Up @@ -114,15 +109,15 @@ func (ns *mpns) resolveOnce(ctx context.Context, name string) (path.Path, error)
if err == nil {
res, ok := ns.resolvers["pubsub"]
if ok {
p, err := res.resolveOnce(ctx, key)
p, err := res.resolveOnce(ctx, key, opts)
if err == nil {
return makePath(p)
}
}

res, ok = ns.resolvers["dht"]
if ok {
p, err := res.resolveOnce(ctx, key)
p, err := res.resolveOnce(ctx, key, opts)
if err == nil {
return makePath(p)
}
Expand All @@ -134,7 +129,7 @@ func (ns *mpns) resolveOnce(ctx context.Context, name string) (path.Path, error)
if isd.IsDomain(key) {
res, ok := ns.resolvers["dns"]
if ok {
p, err := res.resolveOnce(ctx, key)
p, err := res.resolveOnce(ctx, key, opts)
if err == nil {
return makePath(p)
}
Expand All @@ -145,7 +140,7 @@ func (ns *mpns) resolveOnce(ctx context.Context, name string) (path.Path, error)

res, ok := ns.resolvers["proquint"]
if ok {
p, err := res.resolveOnce(ctx, key)
p, err := res.resolveOnce(ctx, key, opts)
if err == nil {
return makePath(p)
}
Expand Down
8 changes: 5 additions & 3 deletions namesys/namesys_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ type mockResolver struct {
entries map[string]string
}

func testResolution(t *testing.T, resolver Resolver, name string, depth int, expected string, expError error) {
p, err := resolver.ResolveN(context.Background(), name, depth)
func testResolution(t *testing.T, resolver Resolver, name string, depth uint, expected string, expError error) {
opts := DefaultResolveOpts()
opts.Depth = depth
p, err := resolver.Resolve(context.Background(), name, opts)
if err != expError {
t.Fatal(fmt.Errorf(
"Expected %s with a depth of %d to have a '%s' error, but got '%s'",
Expand All @@ -33,7 +35,7 @@ func testResolution(t *testing.T, resolver Resolver, name string, depth int, exp
}
}

func (r *mockResolver) resolveOnce(ctx context.Context, name string) (path.Path, error) {
func (r *mockResolver) resolveOnce(ctx context.Context, name string, opts *ResolveOpts) (path.Path, error) {
return path.ParsePath(r.entries[name])
}

Expand Down
Loading