Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: x/net/dns, vendor/golang.org/x/net/dns: new package #16218

Closed
iangudger opened this issue Jun 29, 2016 · 49 comments
Closed

proposal: x/net/dns, vendor/golang.org/x/net/dns: new package #16218

iangudger opened this issue Jun 29, 2016 · 49 comments

Comments

@iangudger
Copy link
Contributor

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.

@iangudger
Copy link
Contributor Author

/cc @mikioh @bradfitz

@bradfitz
Copy link
Contributor

Per discussion earlier, I'm okay with this.

@iangudger
Copy link
Contributor Author

/cc @mdempsky

@mdempsky
Copy link
Member

mdempsky commented Jun 30, 2016

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.

@bradfitz
Copy link
Contributor

Simplifying/redesigning the API would let us eliminate unnecessary heap allocs.

Which API? The proposed x/net/dns one or the existing internal one?

@mdempsky
Copy link
Member

I suppose both, but I'm more interested in reworking the internal one.

@bradfitz
Copy link
Contributor

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.

@mdempsky
Copy link
Member

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).

@mikioh
Copy link
Contributor

mikioh commented Jun 30, 2016

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.

@iangudger
Copy link
Contributor Author

@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.

@mikioh
Copy link
Contributor

mikioh commented Jun 30, 2016

@iangudger,

Thanks. I think it's fine either the new package just focuses on DNS messages and RRs as a first step, or dnsmsg as the package name for clarification.

@dgryski
Copy link
Contributor

dgryski commented Jul 1, 2016

Where does https://github.com/miekg/dns fit in to this?

@miekg @FiloSottile

@miekg
Copy link
Contributor

miekg commented Jul 1, 2016

miekg/dns was forked from the std lib for this reason and was then extended to support the rest of the DNS specification. EDNS0, DNSSEC, etc. etc. Just exposing the parsing bits from std lib covers a fraction of the spec - but I can see it being useful for some people.

As for incrementally walking a message - there is no support for that in the miekg/dns although I'm happy to take patches for that.

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?

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2016

@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.

@miekg
Copy link
Contributor

miekg commented Jul 1, 2016

There is no CLA.

On Fri, Jul 1, 2016 at 11:04 AM, Brad Fitzpatrick notifications@github.com
wrote:

@miekg https://github.com/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.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16218 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAVkW9wEFDqPsoMT2SpDpguViMVTvEniks5qRSyMgaJpZM4JBo1D
.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2016

So, miekg/dns is out of the running.

@dgryski
Copy link
Contributor

dgryski commented Jul 1, 2016

Depends on how hard it is to track down 78 people and get them to sign the CLA.

@miekg
Copy link
Contributor

miekg commented Jul 1, 2016

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.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2016

@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.

@miekg
Copy link
Contributor

miekg commented Jul 1, 2016

Ok.

So to make things clear: I add this CLA, contact my contributors and assuming that all works, miekg/dns will be put in the standard Go library (eventually)?

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2016

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.

@miekg
Copy link
Contributor

miekg commented Jul 1, 2016

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

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2016

You don't need it, but Go does. So if you want to contribute to Go, you need it too.

@miekg
Copy link
Contributor

miekg commented Jul 6, 2016

Short update on this and a question: general response has been positive (I still have to through my email and double check).
One question popped up: if you don't want to sign, can you then re-assign your changes to someone else and if so, how?

@ianlancetaylor
Copy link
Contributor

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.

@quentinmit quentinmit added this to the Proposal milestone Jul 29, 2016
@miekg
Copy link
Contributor

miekg commented Sep 20, 2016

See miekg/dns#404 for an update on the number of people that have (or are willing) to sign the CLA.
Some authors in the 'No reply' category made some significant contributions

@gopherbot
Copy link

Change https://golang.org/cl/97716 mentions this issue: dns/dnsmessage: add (*Builder).AppendStart

gopherbot pushed a commit to golang/net that referenced this issue Mar 1, 2018
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>
@gopherbot
Copy link

Change https://golang.org/cl/98755 mentions this issue: dns/dnsmessage: fix go vet warning about unkeyed composite literals

gopherbot pushed a commit to golang/net that referenced this issue Mar 6, 2018
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>
@gopherbot
Copy link

Change https://golang.org/cl/99623 mentions this issue: dns/dnsmessage: fix handling of non-LDH domain names

@gopherbot
Copy link

Change https://golang.org/cl/99915 mentions this issue: dns/dnsmessage: expose the number of unparsed resources

gopherbot pushed a commit that referenced this issue Mar 15, 2018
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>
@gopherbot
Copy link

Change https://golang.org/cl/101278 mentions this issue: vendor: update golang.org/x/net/dns/dnsmessage from upstream

gopherbot pushed a commit that referenced this issue Mar 18, 2018
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>
gopherbot pushed a commit to golang/net that referenced this issue Mar 30, 2018
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>
@gopherbot
Copy link

Change https://golang.org/cl/107306 mentions this issue: dns/dnsresolver: new package

@gopherbot
Copy link

Change https://golang.org/cl/120697 mentions this issue: dns/dnsmessage: implement fmt.GoStringer.GoString

gopherbot pushed a commit to golang/net that referenced this issue Jul 2, 2018
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>
@gopherbot
Copy link

Change https://golang.org/cl/123835 mentions this issue: dns/dnsmessage: fix bug in length fixup

gopherbot pushed a commit to golang/net that referenced this issue Jul 18, 2018
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>
@gopherbot
Copy link

Change https://golang.org/cl/125735 mentions this issue: net: fix handling of Conns created by Resolver.Dial

gopherbot pushed a commit that referenced this issue Jul 24, 2018
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>
@ianlancetaylor
Copy link
Contributor

@iangudger is there anything else to do on this issue?

@iangudger
Copy link
Contributor Author

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.

@golang golang locked and limited conversation to collaborators Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests