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

Fix localhost handling in DnsLookupIpEndpointSource #121

Merged
merged 2 commits into from
Oct 6, 2018
Merged

Fix localhost handling in DnsLookupIpEndpointSource #121

merged 2 commits into from
Oct 6, 2018

Conversation

martincostello
Copy link
Member

While investigating writing some integration tests to help avoid issues like #110 in the future and assist with the work for 4.0.0, I found a bug in the DNS handling for "localhost".

If you run statsd locally in the default configuration, it listens at 127.0.0.1:8125 using IPv4.

However, on a machine that supports both IPv4 and IPv6, the first address returned by Dns.GetHostAddressesAsync() may be an IPv6 address.

This causes metrics to be sent to ::1 instead of 127.0.0.1, where there is nothing listening, so metrics are silently dropped. This would found by my integration test as the statsd admin interface on 8126 did not report back that the metrics being sent were being received.

This PR fixes this by preferring the first address with an address family of AddressFamily.InterNetwork as that's the intuitive default behaviour for statsd for "pit-of-success".

We could maybe extend things in the future to support preferring IPv6 through configuration, but that's already achievable today by using SimpleIpEndpoint instead.

Add a failing unit test for DnsLookupIpEndpointSource using localhost.
When resolving the hostname for "localhost", prefer IPv4 to IPv6 addresses, as statsd does not listen on IPv6 by default.
@martincostello martincostello added this to the v4.0.0 milestone Oct 6, 2018
@martincostello martincostello requested a review from a team as a code owner October 6, 2018 13:33
@martincostello martincostello merged commit 4d0bd30 into justeattakeaway:master Oct 6, 2018
@martincostello martincostello deleted the Fix-localhost branch October 6, 2018 14:31
This was referenced Oct 6, 2018
@martincostello martincostello modified the milestones: v4.0.0, v3.2.2 Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants