Fine grained connection setup in Client

[jwriteclub]

@shadrick and I have been running into some problems working on some new features for Tenta DNS. We can plug into Client.Dialer, but we cannot provide a specific Conn to Client. That is, we can replace Dialer, but we cannot replace it with a test.Conn, for instance, because Dialer is not an interface.

Inspired by how the http package handles this, we'd like to propose the addition of a DialerFunc field to Client, which wraps the minimal amount of the dialing process, so that it is possible to substitute any arbitrary net.Conn into Client.

Given how Client.Dial is structured, this is a bit tricky. The easy option (such as http.Transport uses) of only using a function will be very surprising to users. If they set Client.Dialer and Client.DialerFunc, what happens?

While we haven't identified a perfect way to resolve this issue, we think that by creating a defaultDialerFunc, which accepts the Client.Dialer as one of its arguments, anyone who chooses to implement their own connection setup will have to explicitly deal with the Dialer (if they choose to ignore it, they may, but at their peril).

To demonstrate what we're talking about, we've coded up a minimal implementation of our proposal at https://github.com/tenta-browser/dns/commit/adfcced198f7781f547b1aa775baa367965a8dfa

We haven't added tests yet, but we'd like feedback from the community. If this idea seems palatable, we'll get it together into shape for a pull request. If there's other ideas about how to implement this without breaking backwards compatibility, please give us your ideas and we'll give them a whirl.

[jwriteclub]

I also want to note that this is not only motivated by testing concerns, we've also been experimenting with pluggable transports in our client side apps, and this is what has finally prompted us to desire to use arbitrary instances of net.Conn.

[tmthrgd]

Conn doesn’t contain any references back to Client. It seems like this could be accomplished much more simply with func NewConn(net.Conn) *Conn. @jwriteclub Do you have any issue with that?

[tmthrgd]

Actually you don't even need to do that:

c := new(dns.Conn)
c.Conn = ...

@jwriteclub Is there a reason why this isn't working for you?

[jwriteclub]

@tmthrgd

We can certainly create a Conn and call all of the relevant methods by ourselves (as noted in the comment to the now deprecated ExchangeConn func).

However, two points:

1) This means that we cannot create a Client which we pass around. 2) We have to re-implement the logic inside Exchange, everywhere we want to do an exchange.

It seems like will just result in copy pasting the contents of Exchange around a lot.

We've been continuing to go over the code in client.go, and we note that the exchangeDOH function completely ignores the Client.Dialer. There is the options to set a custom http.Client to use. This seems a bit surprising to me, and it strikes me that either the exchangeDOH should use the specified Dialer, or else a note should be added to the documentation that DOH ignores the Dialer.

[tmthrgd]

@jwriteclub So the problem for you is not actually Dial itself, but that Exchange uses Dial and you can't override it's behaviour? That does seem reasonable, although I wish there was a cleaner way. Feel free to open a pull request.

[DNS-over-HTTPS support is experimental, entirely opt-in (w/ Net = "https") and separate from regular dialling. (Note that #651 would have used the existing net.Dialer). It could be better documented, but note it is still experimental].

[joonas-fi]

I want to add my two cents that I found the deprecation of ExchangeConn() really frustrating. I don't see the value in everyone copy-pasting this around:

func dnsRequestResponse(req *dns.Msg, conn net.Conn) (*dns.Msg, error) {
    dnsConn := dns.Conn{
        Conn: conn,
    }

    if err := dnsConn.WriteMsg(req); err != nil {
        return nil, err
    }

    res, err := dnsConn.ReadMsg()
    if err != nil {
        return nil, err
    }

    if res.Id != req.Id {
        return nil, dns.ErrId
    }

    return res, nil
}

My issues with this:

• why should my application hardcode protocol implementation details like ReadMsg() is usable after WriteMsg(req)?
• why should my application care about checking res.Id != req.Id
• why should everyone copy-paste this around? What if the snippet needs to be changed in the future?
• I realize the API is deprecated, but I feel that the "forced deprecation notice log msg to stderr" is pretty hostile. At least take a *log.Logger somewhere to not make me completely surprised about polluting my stderr.

[tamird]

Looks like all the hooks mentioned in this discussion have been removed; it is no longer possible to set Client.Conn.

In my case, I'd like to use https://godoc.org/github.com/google/netstack/tcpip/adapters/gonet#Conn as my connection implementation (in Fuchsia OS). Perhaps some consideration can be given to this use case?