proposal: x/net/dns, vendor/golang.org/x/net/dns: new package #16218
Comments
|
Per discussion earlier, I'm okay with this. |
|
/cc @mdempsky |
|
I'm also fine with adding/maintaining an x/net/dns package based on the internal DNS code. I'm somewhat hesitant about vendoring it for package net though. Package net's DNS needs are pretty minimal, and the internal API is already overkill for what it needs. Simplifying/redesigning the API would let us eliminate unnecessary heap allocs. |
Which API? The proposed x/net/dns one or the existing internal one? |
|
I suppose both, but I'm more interested in reworking the internal one. |
|
As background, we have a number of forks of various DNS packages (some forked from the Go net internals) inside Google. There's increasing pushback to unify them both for sanity and security, so there's only one correct one to maintain. I think we can make a good one that's also fast. |
|
The main thing I want for package net is an API to incrementally walk over a DNS packet without needing to fully unmarshal it and incur unnecessary heap allocs. The current dnsmsg API could then be implemented on top of that, while package net could use it directly. This is similar to how djbdns's client library is implemented: a low-level abstraction for treating packets as sequences of packed names and raw byte strings (used by djbdns's dnscache server), and a higher-level abstraction that understands the encoding of different RR formats (intended for use by end user applications). |
|
I'm not sure whether this proposal covers #8540 and #11160. My concerns about having a new package related to DNS stuff are simply 1) Will the package be able to support any DNS transport including plain UDP, TCP through TLS/DTLS and 2) Will the package be able to interact with the net, crypto/tls and crypto/x509 packages of standard library seamlessly and without any API breaking changes. If this proposal covers the existing issues, in other words this proposal keeps some blueprint on good DNS package separation for avoiding circular dependency and complexity, I'd like to accept this proposal and want to merge #8540 and #11160 into this. |
|
@mdempsky: I am open to implementing that. @mikioh: I was only proposing DNS packet packing and unpacking for now. The code in question is mostly in net/dnsmsg.go and only depends on the net package for stringifying IP addresses. I think it would probably be worth duplicating that small amount of code to allow this new package to stand on its own. |
|
Thanks. I think it's fine either the new package just focuses on DNS messages and RRs as a first step, or |
|
Where does https://github.com/miekg/dns fit in to this? |
|
As for incrementally walking a message - there is no support for that in the I'm wondering about this feature request in the first place. Is the only reason for it to not depend on the 3rd party package? |
|
@miekg, what's the CLA story for your 78 contributors? I suspect we couldn't vendor it into the standard library even if we wanted to. |
|
There is no CLA. On Fri, Jul 1, 2016 at 11:04 AM, Brad Fitzpatrick notifications@github.com
|
|
So, |
|
Depends on how hard it is to track down 78 people and get them to sign the CLA. |
|
I'm willing to do that. Allthough I might need some pointers for creating a CLA in the first place and how that process works. |
|
@miekg, Go uses Google's CLA: https://cla.developers.google.com/ You wouldn't be creating a new one. First thing you'd need to do is enable the google CLA bot for the miegk/dns repo, enforcing CLAs for all new Pull Requests. Then retroactively contact people. |
|
Ok. So to make things clear: I add this CLA, contact my contributors and assuming that all works, |
|
No promises. But without CLAs, it definitely will not be considered. And by "put in the standard library", what we're actually talking about is being used by the standard library, not exporting any new functionality or symbols to users. |
|
Ok, fair enough. But for this to work I need the Google CLA? I'm asking because it feels a bit strange to slap that CLA on a project that has (in essence) nothing to do with Google |
|
You don't need it, but Go does. So if you want to contribute to Go, you need it too. |
|
Short update on this and a question: general response has been positive (I still have to through my email and double check). |
|
That is a legal question and we are not lawyers. If that is a serious question and the only way we can move forward, I can try to ask Google's lawyers for a legal opinion. I think I can promise that whatever they come up with, if anything, will be more complex and harder to understand than the CLA. I can understand a refusal to sign anything. But if someone is willing to sign something, the CLA is not that hard to understand and is about as minimal as possible for this kind of document. It asserts that the signer has a legal right to distribute the code, and grants Google the same right. |
|
See miekg/dns#404 for an update on the number of people that have (or are willing) to sign the CLA. |
|
Change https://golang.org/cl/97716 mentions this issue: |
The new appending behavior is required for an efficient DNS client. (*Builder).Start was intended to append, but didn't. This was a mistake (all tests and examples assumed that it did). In addition, message compression when appending has been fixed. Updates golang/go#16218 Change-Id: I3f42aa6e653e2990fa90368a2803e588ea15b85a Reviewed-on: https://go-review.googlesource.com/97716 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
Change https://golang.org/cl/98755 mentions this issue: |
Updates golang/go#16218 Change-Id: I078395a8c3967c2ac6a6e86f1528a431346b8ff4 Reviewed-on: https://go-review.googlesource.com/98755 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
Change https://golang.org/cl/99623 mentions this issue: |
|
Change https://golang.org/cl/99915 mentions this issue: |
Vendors golang.org/x/net/dns/dnsmessage from x/net git rev 892bf7b0c6e2f93b51166bf3882e50277fa5afc6 Updates #16218 Updates #21160 Change-Id: Ic4e8f3c3d83c2936354ec14c5be93b0d2b42dd91 Reviewed-on: https://go-review.googlesource.com/37879 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
Change https://golang.org/cl/101278 mentions this issue: |
Updates to x/net git rev 24dd378 for CL 100055 Fixes #10622 Updates #16218 Change-Id: I99e26da7b908b36585a0379d9381030c01819b54 Reviewed-on: https://go-review.googlesource.com/101278 Run-TryBot: Ian Gudger <igudger@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This change introduces OPTResourse type to support DNS messages containing various extension options as defined in RFC 6891. Parsing and building OPT pseudo records requires allocations like TXT records. Also adds DNSSECAllowed, ExtendedRCode and SetEDNS0 methods to ResourceHeader for convenience. Updates golang/go#6464. Updates golang/go#16218. Change-Id: Ib72cea277201e4122c6b5effa320084ff351c886 Reviewed-on: https://go-review.googlesource.com/47170 Reviewed-by: Ian Gudger <igudger@google.com>
|
Change https://golang.org/cl/107306 mentions this issue: |
|
Change https://golang.org/cl/120697 mentions this issue: |
This improves the debugability of the library and eases the transition to this library. For example, test DNS messages defined with another DNS library can be packed and then unpacked with this library. These unpacked messages can have GoString called on them to generate new test messages defined with this library. Updates golang/go#16218 Change-Id: I602586500fd8202892ef04187d3bd8a11039cf27 Reviewed-on: https://go-review.googlesource.com/120697 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
Change https://golang.org/cl/123835 mentions this issue: |
If during packing, the byte slice gets re-allocated between packing the ResourceHeader and ResourceBody, the length will get updated in the old byte slice before re-allocation resulting in a zero length in the final packed message. This change fixes the bug by passing the offset at which the length should be written instead of a slice of the output byte slice from ResourceHeader encoding step. Updates golang/go#16218 Change-Id: Ifd7e2f549b7087ed5b52eaa6ae78970fec4ad988 Reviewed-on: https://go-review.googlesource.com/123835 Run-TryBot: Ian Gudger <igudger@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
Change https://golang.org/cl/125735 mentions this issue: |
The DNS client in net is documented to treat Conns returned by Resolver.Dial which implement PacketConn as UDP and those which don't as TCP regardless of what was requested. golang.org/cl/37879 changed the DNS client to assume that the Conn returned by Resolver.Dial was the requested type which broke compatibility. Fixes #26573 Updates #16218 Change-Id: Idf4f073a4cc3b1db36a3804898df206907f9c43c Reviewed-on: https://go-review.googlesource.com/125735 Run-TryBot: Ian Gudger <igudger@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
@iangudger is there anything else to do on this issue? |
|
I was hoping to include the DNS server in this issue, but I will create a new proposal for that. The primary goal has been achieved. A new package (golang.org/x/net/dns/dnsmessage) was created based on net/dnsmsg.go. This package is now vendored in the standard library and a new DNS client which uses it has replaced the old net DNS client (and dnsmsg.go). This new client is in Go 1.11. Go 1.12 is soon to be released and there are no known unfixed regressions vs the old DNS client. |
We currently have the startings of a nice DNS parsing and packing library in net/dnsmsg.go. This is useful for building custom DNS clients and servers in Go, but is not currently exported. I would like to propose moving this functionality to x/net/dns and vendoring it so that the same version can be used in the net package.
The text was updated successfully, but these errors were encountered: