Skip to content

Use and vendor golang.org/x/net/ipv4 and golang.org/x/net/ipv6 #594

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 4, 2017
Merged

Use and vendor golang.org/x/net/ipv4 and golang.org/x/net/ipv6 #594

merged 1 commit into from
Dec 4, 2017

Conversation

jameshartig
Copy link
Collaborator

Removed the internal/socket and used golang.org/x/net/ipv4 and golang.org/x/net/ipv6 adding them with dep to the vendor folder.

Additionally, I removed the udp_linux.go in favor of using ipv4/ipv6 for cross-platform support. We don't quite have Windows support since that will be coming with go 1.10 (and then we can use build flags to exclude udp_windows.go on < 1.10) and is tracked in #571.

I tried to add tests to test as much of the OOB functionality as possible. I will be testing in production with a newer build of CoreDNS to verify everything on Linux.

@jameshartig
Copy link
Collaborator Author

Apparently travis-ci doesn't have any IPv6 support so the test for [::1]:0 will log and not be run. Running that locally for me works but I'm not sure how to get that tested via travis-ci.

@codecov-io
Copy link

codecov-io commented Nov 29, 2017 •
edited
Loading

Codecov Report

Merging #594 into master will decrease coverage by 0.05%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
- Coverage   57.79%   57.73%   -0.06%     
==========================================
  Files          37       36       -1     
  Lines       10062     9976      -86     
==========================================
- Hits         5815     5760      -55     
+ Misses       3187     3168      -19     
+ Partials     1060     1048      -12
Impacted Files Coverage Δ
udp.go 90% <88.88%> (+2.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6da3249...749fc07. Read the comment docs.

@jameshartig
Copy link
Collaborator Author

We don't have a way to test for IPv6 or IPv4 in parseDstFromOOB so I left it defaulting to IPv6 and then falling back to IPv4. If you have an IPv6 kernel and don't specifically listen on IPv4, then the out-of-band data will be in IPv6 format (with :ffff: in front of IPv4) no matter if the packet was actually from IPv4 or IPv6 so I assume that's probably the more common case. We could look at the first few bytes of the data and see if there's a level of 0 or 41 to determine the format but that wouldn't be cross platform. I don't think the overhead of doing both is worth the optimization to only 1 or the other at this point.

@miekg
Copy link
Owner

miekg commented Nov 30, 2017 via email

@miekg
Copy link
Owner

miekg commented Dec 1, 2017

Is everything looking Ok from your side? If so; I will merge this.

@miekg
Copy link
Owner

miekg commented Dec 4, 2017

I'm going ahead with this. CoreDNS 1.0.0 will be announced today and 1.0.1 dev has started, meaning plenty of time for me to see if this works or not.

@miekg miekg merged commit 325e98b into master Dec 4, 2017
@miekg miekg deleted the vendor-x-net branch December 4, 2017 10:10
@jameshartig
Copy link
Collaborator Author

@miekg yes we've been running this in production with CoreDNS since the 30th and it seems to be working fine. Sorry, somehow I missed your comment.

@johnbelamaric
Copy link

@fastest963 if you are running CoreDNS in production would you mind adding yourself to https://github.com/coredns/coredns/blob/master/ADOPTERS.md ? It will help as we move toward incubation status in the CNCF.

@vavrusa vavrusa mentioned this pull request Mar 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jameshartig @codecov-io @miekg @johnbelamaric