Fix: dnslink domain resolving was broken; Add: no caching for those #1272

Luzifer · 2015-05-21T06:45:53Z

jbenet · 2015-05-21T06:47:30Z

Thanks this is great. sorry for the resolve errors.

jbenet · 2015-05-21T06:48:14Z

core/corehttp/ipns_hostname.go

+			if len(host) > 0 && isd.IsDomain(host) {
+				name := "/ipns/" + host
+				if _, err := n.Namesys.Resolve(ctx, name); err == nil {
+					r.URL.Path = path.Join("/ipns/", host) + r.URL.Path


maybe: name + r.URL.Path ?

wking · 2015-05-21T17:02:18Z

And we probably want to some tests with a mock DNS resolver (like namesys/dns_test.go) to make sure this doesn't break again. I think that means we want a public API in namesys to manage the resolver (and probably also the publisher) maps in mpns. That's a bit awkward at the moment, because the ‘resolver’ interface is internal. Maybe namesys needs public backend APIs (currently the mpns type, the resolve helper, and the resolver interface) along side it's current public frontent APIs (the Resolver, Publisher, and NameSys interfaces).

Luzifer · 2015-05-21T17:34:41Z

Just using path.Join(p.String, r.URL.Path) leads to bad results… path.Join removes any trailing slash which then leads to redirect foo…

Regarding two DNS resolves: I don't the the problem with this. The DNS entry is cached by the next DNS resolver in the network. For most people their local router, otherwise providers router, … So this are two inexpensive DNS requests compared to one… Yeah sure, one could implement a DNS cache but for now it would work for me this way.

Luzifer · 2015-05-21T17:40:13Z

About tests: @jbenet and me had an idea for this: https://botbot.me/freenode/ipfs/2015-05-21/?msg=39768745&page=5

wking · 2015-05-21T17:52:55Z

On Thu, May 21, 2015 at 10:34:41AM -0700, Knut Ahlers wrote:

Just using path.Join(p.String, r.URL.Path) leads to bad results…
path.Join removes any trailing slash which then leads to redirect
foo…

Ah, that makes sense. All these little wrinkles to trip us up :p.

wking · 2015-05-21T17:55:59Z

On Thu, May 21, 2015 at 10:40:14AM -0700, Knut Ahlers wrote:

About tests: @jbenet and me had an idea for this:
https://botbot.me/freenode/ipfs/2015-05-21/?msg=39768745&page=5

I just see “we need to test this” there, not how to get a mock DNS for
sharness tests. Personally, I'd be fine with some Go tests for this
functionality, since it's a lot easier to plug in a mock DNS resolver
in Go than it is to set one up for sharness tests.

Luzifer · 2015-05-21T18:00:01Z

We have several tests for the resolver stuff but none of those caught the feature to be fully destroyed. A full-stack-test would test the whole stack and check the "outside" functionality is still intact and not only parts of the codes are okay.

whyrusleeping · 2015-05-21T18:14:20Z

we should have a few domains on ipfs.io, like test.ipfs.io or test1.ipfs.io that have TXT records we can use for testing like this

wking · 2015-05-21T18:25:01Z

On Thu, May 21, 2015 at 11:14:21AM -0700, Jeromy Johnson wrote:

we should have a few domains on ipfs.io, like test.ipfs.io or
test1.ipfs.io that have TXT records we can use for testing like
this

Tests that require a working network connection between the location
running the tests (Travis?) and some external entity (the DNS chain
leading to test.ipfs.io) make me jumpy. For one thing, you could
never change the values in those TXT records without breaking
bisection for commits that assumed the old entries/values. It's easy
enough to mock the DNS entries in Go, so while I like the security of
end-to-end sharness tests, I don't think they're worth the trouble in
this case, and I'd rather have in-Go tests for this functionality.

jbenet · 2015-05-22T09:41:28Z

we should have a few domains on ipfs.io, like test.ipfs.io or test1.ipfs.io that have TXT records we can use for testing like this

Yep, absolutely. if people drop some records here, i'll add them.

Tests that require a working network connection between the location
running the tests (Travis?) and some external entity (the DNS chain
leading to test.ipfs.io) make me jumpy. For one thing, you could
never change the values in those TXT records without breaking
bisection for commits that assumed the old entries/values. It's easy
enough to mock the DNS entries in Go, so while I like the security of
end-to-end sharness tests, I don't think they're worth the trouble in
this case, and I'd rather have in-Go tests for this functionality.

Agreed with this too. I think we should have a test that is usually mocked, but the mocking can be turned off and run live when in the internet (on CI). (good to test for real).

jbenet · 2015-05-22T09:42:07Z

@Luzifer should i merge this, or wait for tests?

Luzifer · 2015-05-22T10:29:23Z

@jbenet for me it would really nice to have this live and working dnslink feature. That way I could continue putting documentation to IPFS… You're the project owner so you have to decide whether to wait or merge…

jbenet · 2015-05-22T10:34:29Z

@Luzifer i'd also want it in-- so that resolve actually works.

hmmm, @Luzifer would you submit a follow-up PR? not sure how long it would take either way.

Luzifer · 2015-05-22T11:01:05Z

Actually I'd prefer someone with more knowledge about the whole project and mocking to write the tests…

whyrusleeping · 2015-05-22T15:46:08Z

@wking do you think you could handle writing a test or two for this?

jbenet · 2015-05-24T16:47:37Z

circlci's detecting races for us :) cc @whyrusleeping

==================
WARNING: DATA RACE
Read by goroutine 77:
  github.com/ipfs/go-ipfs/exchange/bitswap.(*Bitswap).ReceiveMessage()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/bitswap.go:339 +0x395
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*networkClient).ReceiveMessage()
      <autogenerated>:48 +0xde
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*network).deliver()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/testnet/virtual.go:75 +0x16c

Previous write by goroutine 97:
  github.com/ipfs/go-ipfs/exchange/bitswap.(*Bitswap).ReceiveMessage()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/bitswap.go:339 +0x3b4
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*networkClient).ReceiveMessage()
      <autogenerated>:48 +0xde
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*network).deliver()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/testnet/virtual.go:75 +0x16c

Goroutine 77 (running) created at:
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*network).SendMessage()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/testnet/virtual.go:62 +0x265
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*networkClient).SendMessage()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/testnet/virtual.go:89 +0xc9
  github.com/ipfs/go-ipfs/exchange/bitswap.(*Bitswap).send()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/bitswap.go:446 +0x2db
  github.com/ipfs/go-ipfs/exchange/bitswap.(*Bitswap).taskWorker()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/workers.go:74 +0x586
  github.com/ipfs/go-ipfs/exchange/bitswap.func·013()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/workers.go:40 +0x6f
  github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess.func·005()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess/impl-mutex.go:110 +0xa7

Goroutine 97 (running) created at:
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*network).SendMessage()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/testnet/virtual.go:62 +0x265
  github.com/ipfs/go-ipfs/exchange/bitswap/testnet.(*networkClient).SendMessage()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/testnet/virtual.go:89 +0xc9
  github.com/ipfs/go-ipfs/exchange/bitswap.(*Bitswap).send()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/bitswap.go:446 +0x2db
  github.com/ipfs/go-ipfs/exchange/bitswap.(*Bitswap).taskWorker()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/workers.go:74 +0x586
  github.com/ipfs/go-ipfs/exchange/bitswap.func·013()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/exchange/bitswap/workers.go:40 +0x6f
  github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess.func·005()
      /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess/impl-mutex.go:110 +0xa7
==================

Fix: dnslink domain resolving was broken; Add: no caching for those

jbenet · 2015-05-24T16:48:44Z

(let's do a test as a separate PR -- #1289)

Fix: dnslink domain resolving was broken; Add: no caching for those This commit was moved from ipfs/kubo@4e71ce3

Fix: dnslink domain resolving was broken; Add: no caching for those

373260d

fixes #1234 fixes #1267

jbenet added the backlog label May 21, 2015

jbenet reviewed May 21, 2015
View reviewed changes

removed requirement of path package

8df8737

jbenet added a commit that referenced this pull request May 24, 2015

Merge pull request #1272 from Luzifer/fix/dnslink

4e71ce3

Fix: dnslink domain resolving was broken; Add: no caching for those

jbenet merged commit 4e71ce3 into ipfs:master May 24, 2015

jbenet removed the backlog label May 24, 2015

jbenet deleted the fix/dnslink branch May 24, 2015 16:47

jbenet mentioned this pull request May 24, 2015

Test for #1272 resolver problem #1289

Closed

wking mentioned this pull request Jun 12, 2015

changelog updates #1360

Closed

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#1272 from Luzifer/fix/dnslink

87dfaf3

Fix: dnslink domain resolving was broken; Add: no caching for those This commit was moved from ipfs/kubo@4e71ce3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: dnslink domain resolving was broken; Add: no caching for those #1272

Fix: dnslink domain resolving was broken; Add: no caching for those #1272

Luzifer commented May 21, 2015

jbenet commented May 21, 2015

jbenet May 21, 2015

wking commented May 21, 2015 via email

Luzifer commented May 21, 2015

Luzifer commented May 21, 2015

wking commented May 21, 2015

wking commented May 21, 2015

Luzifer commented May 21, 2015

whyrusleeping commented May 21, 2015

wking commented May 21, 2015

jbenet commented May 22, 2015

jbenet commented May 22, 2015

Luzifer commented May 22, 2015

jbenet commented May 22, 2015

Luzifer commented May 22, 2015

whyrusleeping commented May 22, 2015

jbenet commented May 24, 2015

jbenet commented May 24, 2015

Fix: dnslink domain resolving was broken; Add: no caching for those #1272

Fix: dnslink domain resolving was broken; Add: no caching for those #1272

Conversation

Luzifer commented May 21, 2015

jbenet commented May 21, 2015

jbenet May 21, 2015

Choose a reason for hiding this comment

wking commented May 21, 2015 via email

Luzifer commented May 21, 2015

Luzifer commented May 21, 2015

wking commented May 21, 2015

wking commented May 21, 2015

Luzifer commented May 21, 2015

whyrusleeping commented May 21, 2015

wking commented May 21, 2015

jbenet commented May 22, 2015

jbenet commented May 22, 2015

Luzifer commented May 22, 2015

jbenet commented May 22, 2015

Luzifer commented May 22, 2015

whyrusleeping commented May 22, 2015

jbenet commented May 24, 2015

jbenet commented May 24, 2015