Make dns resolve paths under _dnslink.example.com

[Kubuxu]

Thus allowing to CNAME main site entry to gateway and still specify dnslink.

Resolves ipfs/notes#39. Option 4 and 5.

Would be great if merged with #2139, and shouldn't even conflict (depending on merger)

Also if you have any comments to the code, I would love to hear them. If anything could have been done better... I am not proficient in Go..

[jbenet]

Great to see this implemented, thanks!

[jbenet]

i think this leaks goroutines? will want to do a waitgroup sync, and buffer the channel with len(prefixes)

[jbenet]

also, actually no, i think these should not be concurrent and we unfort need them to be sequential for correctness.? and yes, i think we said the _dnslink. prefix had to be first (is this right @lgierth?)

[Kubuxu]

It would be best done concurrently but with _dnslink. starting first. This way on Unix they will be fully concurrent and in case of everything else sequential.

If someone throws different hashes into both places, sorry, but in my opinion it is his problem. Doing it sequentially creates performance trade-off for someone saying conflicting things (which will be really rare).

[ghost]

also, actually no, i think these should not be concurrent and we unfort need them to be sequential for correctness.? and yes, i think we said the _dnslink. prefix had to be first (is this right @lgierth?)

@jbenet yes the _dnslink. record should take precedence, but we can still resolve both concurrently, and as soon as the prefixed record is resolved, we can make a decision.

That way the runtime is roughly max(A, B) and not sum(A, B).

[Kubuxu]

Made it so goroutines start in order. It is still concurrent in case of Unix and in case of other systems will behave as sequential (they use native lib for resolving).

In my opinion if someone places different dnslinks under _dnslink. and . the result should be just undefined.

[Kubuxu]

So the final logic

1. Start both
2. Wait .dnslink resolution.
3. If unsuccessful return result of . resolution.

[ghost]

I think the goroutines don't neccessarily need to start in order. The only thing that needs to happen is that if (= as soon as) _dnslink. resolves successfully, we return early and cancel the other goroutine. We don't need its result if we already know that we'll pick the other result.

• start lookup with _dnslink. prefix
• start lookup without prefix
• when lookup with prefix finishes
  • if success
    • return with that result
    • cancel without-prefix lookup
  • if failure
    • do nothing
• when without-prefix lookup finishes
  • if success and prefix lookup failed
    • return without-prefix result
  • if failure
    • do nothing
  • for additional safety:
    • if prefix lookup succeeded
      • do nothing, we are being cancelled right now anyway

edit: removed a stray line at the end, s/promises/goroutines/

[ghost]

Eventually we might want to put circuit breakers around all DNS lookups, in order to isolate potential DNS breakages that we can do nothing about. But that's a whole different story for another time.

[Kubuxu]

I've made it so it start concurrently, waits for _dnslink. and if it fails uses .. So almost exactly as lgierth said apart from cancelling but there is no much need for it with only two requests which probably start concurrently.

[jbenet]

SGTM!

@lgierth do "unsuccessful" dns requests get cached? or will this always cause a request hit initially?

[jbenet]

👍 great testing adds

[jbenet]

this LGTM and RFM. want to hear from @lgierth first, as if we're introducing lots of waiting and lack of caching would be good to be 100% sure we want this.

[ghost]

@lgierth do "unsuccessful" dns requests get cached? or will this always cause a request hit initially?

@jbenet not within go-land. This PR doesn't make the situation worse though. You could argue that resolving two records instead of one makes it more likely that one of them is a "long tail" request, but meh.

Let's look into decoupling ourselves from failures outside our reach though! Caches and circuit breakers! :)

👍 RFM

[RichardLitt]

Can we merge this? We've got two RFM.

[ghost]

Yep let's get it in, it'll also make my own life a lot easier regarding management of domains :)

[whyrusleeping]

sorry to hang this PR up any longer, but if you could change your channel receives to also check the context, that would be super fantastic:

var subRes *lookupRes
select {
    case subRes = <-subChan:
    case <-ctx.Done():
        return "", ctx.Err()
}

do that for this one and the rootChan one a little lower and i'll merge asap

[Kubuxu]

@whyrusleeping I've addressed the comment.

[whyrusleeping]

LGTM