The Giant v2 rewrite issue

[miekg]

Issue filed to thing about and flesh out ideas about a possible v2 (re?)write. Keep stuff that is good; throw away things that are bad. I'm not a fan of a big bang move; or something that different for the sake of being different.

• One general thing is probably that much more should be pluggeable by means of having an interface. The newer dns transport layer also show the the net/http/RoundTripper interface, instead of the simple Net member in Client.

• Server side is OK(ish?), but can also use a more pluggeable interface. dns.ResponseWriter may need to give guarantees about concurrent use (out-of-order TCP handling) brings this up

• Another discussion point would be if we want to keep wireformat around

• Looks a x/net/dns and see what's happening there.

[tmthrgd]

@miekg Just a quick note, on the subject of out-of-order TCP handling, Hijack will need to be eliminated. (Consider http.Hijacker is only supported for HTTP/1.x, which has no support pipelining in net/http, and isn't supported for HTTP/2).

[miekg]

Hijack is now used for AXFR and other longer running ops on TCP. That prolly needs a bit of a rethink

[tmthrgd]

@miekg Are there examples of how that's being done in the wild? I've been trying to shoe-horn #646 into #653 and the two stumbling blocks are Close and Hijack, I don't think it's really possible to solve #646 without either eliminating those two methods or changing their semantics.

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] The Giant v2 rewrit..." ]

@miekg Are there examples of how that's being done in the wild? I've been trying to shoe-horn #646 into #653 and the two stumbling blocks are `Close` and `Hijack`, I don't think it's really possible to solve #646 without either eliminating those two methods or changing their semantics.

I do this in coredns' secondary code, Hijack to later close - not sure how useful/needed this *actually* is.

[miekg]

• Coming back to hijack; there needs a way to say I've to this incoming Conn, and the caller will later close it when it is done sending all the data (which can be minutes).
• Also it may make sense to start a v2 branch and start playing around a bit
• As mentioned on a PR; the dns.Conn should probably go if we can make dns.Client sufficiently flexible
• All functions should get a context.Context (if dealing with network)
• There should be a bunch of callbacks in the Server for when message fail to unpack and others
• Implement roundtripper and borrow the udp/tcp conn tracking from coredns's forward plugin

[miekg]

Roundtripper can be done in the current code base; but maybe we should open an v2 branch and hack a bit on it?

[miekg]

Even if we have a troundtripper I need to double check in the http package if that would automatically means the server also supports it.

[tmthrgd]

@miekg This library should probably also be ported from a vendor solution to go.mod. That might be worth doing now, I’m not sure how much work that might be to port and maintain. (It may well make building v2 easier).

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] The Giant v2 rewrit..." ]

@miekg This library should probably also be ported from a vendor solution to go.mod. That might be worth doing now, I’m not sure how much work that might be to port and maintain. (It may well make building v2 easier).

Yes, totally. I still need to play with vgo myself. Also considering our 2 major releases supported, we might need to wait until 1.12

[tmthrgd]

@miekg I haven't used vgo/go-modules yet, but I did follow along with the development of it and go1.10.3 (with golang/go#25069) should have minimal backward compatibility support. There should be no problem adding module support and keeping backward compatibility, so I don't think we need to wait to go1.12.

[tmthrgd]

Another: all constants should be type-safe and not uint8s or untyped.

[tmthrgd]

I'd really like to see the handling of domain names changed. Currently string is used, with the presentation form, and the handling of things like escaping is ad-hoc and all over the place (it differs in a several places). I'd like to propose we use a type for domain names, either type Name string or what x/net/dns/dnsmessage uses ("It is used instead of strings to avoid allocations."):

type Name struct {
    Data   [nameLen]byte
    Length uint8
}

Where we (and x/net/dns/dnsmessage) use the presentation format for the domain (including escaping which can cause non-optimal compression^†), I'd like our Name to be in wire format. This eliminates the ambiguity of escaping and moves all the formatting and parsing of domain names into a single place. It would simplify, and speed up, packing and comparing names (NextLabel becomes much simpler and faster).

So our name would be stored as:

"www.example.org." -> "\x03www\x07example\x03org\x00"

That would make packing simply a search for compression points and then a copy.

^†: "a.example.org" and "a.ex\097mple.org" are the same name, but will end up as different entries in the compression map. Escaping is also the cause of several differences (bugs) between the packing code and the compressed Len code.

[miekg]

Yes and no. So.. this has some history. When developing ldns back in the day we opted for wireformat. This meant you need to reimplement the string library to make it work correctly. Which in C is not too bad, because you don't have strings anyway. I choose not to do this for this lib, so "a" + "." + "example.org." will work. Yes this puts enormous effort upon us (maintainers) to make this work properly, but the trade-off is just 'strings' for our users. Was this the correct choice? Maybe? No? Yes? Did this make the API easy, yes. I think the killer usability feature of this library is/has been the use of native types. Would I do it again... Maybe... Fiddling and questioning this is perfectly valid; but you're creating a different beast.

…

-- Miek Gieben

[tmthrgd]

PackDomainName and PackRR should use an opaque internal type for the compression map like type CompressionMap struct { m map[string]int }. If this wasn’t public (it’s not really safe for an external caller to modify it), then we could safely change its type of implemtation.

In particular replacing map[string]int with map[string]uint16 would see a 25% reduction in per-entry memory usage. ((16+2)/(16+8) = 0.75).

I have an idea how this might be possible with the existing implementation. I’ll try that out, but it can’t help other calls.

[miekg]

closing, someone might re-open when actually work is done