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

Option to shuffle addresses from DNS #1694

Closed
wants to merge 16 commits into from

Conversation

dreckard
Copy link
Contributor

This patch adds an option CURLOPT_DNS_SHUFFLE_ADDRESSES to explicitly request shuffling of addresses returned for a hostname when there is more than one. This can be useful in situations where the application knows that a round robin approach is appropriate and is willing to accept the consequences of potentially discarding some preference order returned by the system's implementation.

I came across this post from a few years ago which discusses the address ordering issue and mentions shuffling the addresses after they are returned but rejects it as too problematic. I agree that it is not the right approach for general use but I think enough use cases exist for it to be available as an option.

In my case I am working on retrofitting an existing application to use libcurl instead of its own old socket based networking implementation. Its only network task is to connect to a specific hostname which is known to be configured for use with a round robin approach.

I'd be happy to write the docs/tests if this seems merge worthy but I thought I should ask for opinions first.

@mention-bot
Copy link

@dreckard, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @captain-caveman2k to be potential reviewers.

@bagder
Copy link
Member

bagder commented Jul 21, 2017

Thanks for your contribution. I think it is an interesting addition that others might be interested in as well.

Have you thought about how this code should behave on re-use of the cached DNS data? My first cursory look on your code hints that you shuffle the order only once so that repeated uses of the cache will use the same address?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 75.189% when pulling 6ed9930 on dreckard:dns_shuffle_master into 42a4cd4 on curl:master.

@dreckard
Copy link
Contributor Author

dreckard commented Jul 21, 2017

Right about the code not currently reshuffling on reuse of cached names. I did think about that and I didn't see a really decisive argument either way so I went for the simpler option of leaving it out.

The most compelling case I could think of where reshuffling would be preferred is when retrying requests after a failure but the existing fallback logic should usually allow at least a few other addresses to be attempted (timeoutms_per_addr) so I thought the benefit there would be marginal.

@bagder
Copy link
Member

bagder commented Jul 26, 2017

Well, if you ask for a page every second from a round-robin-DNS served site using libcurl with a cached DNS entry, you'll now use the same IP (first) for 60 consecutive requests, then do a new DNS resolve and the following 60 requests again to the (new) first IP. That could be a little surprising or just exactly what you'd expect.

It mostly just needs to be clearly documented that it only shuffles them in the actual resolve moment and not later, even if it gets used several times.

@dreckard
Copy link
Contributor Author

Ok agreed, sounds good. I'll try to draft up the docs additions for it in the next few days.

@dreckard dreckard force-pushed the dns_shuffle_master branch 2 times, most recently from 25dcd34 to 8a8b593 Compare August 3, 2017 12:26
Add new option CURLOPT_DNS_SHUFFLE_ADDRESSES to allow shuffling
address order if multiple A records are returned for a name.
@dreckard
Copy link
Contributor Author

dreckard commented Aug 4, 2017

Added man page changes and a basic unit test.

…e_master

# Conflicts:
#	include/curl/curl.h
#	lib/url.c
@curl curl deleted a comment from coveralls Sep 21, 2017
@curl curl deleted a comment from coveralls Sep 21, 2017
@curl curl deleted a comment from coveralls Sep 21, 2017
@curl curl deleted a comment from coveralls Sep 21, 2017
@curl curl deleted a comment from coveralls Sep 21, 2017
@curl curl deleted a comment from coveralls Sep 21, 2017
@curl curl deleted a comment from coveralls Sep 21, 2017
@bagder bagder added the feature-window A merge of this requires an open feature window label Sep 21, 2017
@bagder
Copy link
Member

bagder commented Sep 21, 2017

I missed merging this PR before the merge window closed, but it will open again on the release date (October 4) and it would be nice if you could fix the merge conflict until then.

@dreckard
Copy link
Contributor Author

Yup will do.

@dreckard
Copy link
Contributor Author

dreckard commented Oct 1, 2017

Fixed.

@jay
Copy link
Member

jay commented Feb 21, 2018

I notice this is labeled next-feature-window and has been open for a while. I don't like the name CURLOPT_DNS_SHUFFLE_ADDRESSES because I think it sounds ambiguous. I think CURLOPT_SHUFFLE_ADDRESSES and --shuffle-addresses would be better. Otherwise if a user is reading a script I think it will sound to them like it's shuffling the dns- server addresses, which can also be specified.

edit:
or CURLOPT_SHUFFLE_HOST_ADDRESSES. longer but easier to figure out

@dreckard
Copy link
Contributor Author

dreckard commented Mar 6, 2018

Fixed the conflicts again. I'm not hugely attached to the name so I can go ahead and change it to CURLOPT_SHUFFLE_HOST_ADDRESSES in the near future.

Several of the resolution related options are currently using the CURLOPT_DNS_ prefix so I was hoping to remain consistent with those but it's a fair point that there's ambiguity introduced.

@bagder
Copy link
Member

bagder commented Mar 16, 2018

ok @dreckard and @jay, let's finally get this feature merged this window!

@dreckard, can you please rebase this, fix the conflicts (again) and squash the commits that should be squashed? I'm sorry for having dragged this out so that you've had to do this multiple times.

@bagder bagder removed the feature-window A merge of this requires an open feature window label Mar 16, 2018
CURLcode curl_easy_setopt(CURL *handle, CURLOPT_DNS_SHUFFLE_ADDRESSES, long onoff);
.fi
.SH DESCRIPTION
When a name is resolved and more than one A record is returned, shuffle all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means "more than one IP address", right? Because A record implies IPv4 address, but surely this also shuffles IPv6 addresses fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does also shuffle IPv6 addresses so good point.

@bagder bagder closed this in d95f3dc Mar 17, 2018
@bagder
Copy link
Member

bagder commented Mar 17, 2018

Thank you. I squashed and rebased your work, and you will also see that I edited it slightly. Amongst others so that torture tests rune fine on it.

@dreckard
Copy link
Contributor Author

Ah great, thanks! I just committed the option name change suggested by @jay before seeing the notifications for these messages, if it's still desired. I think the tool command line option is still needed as well, sorry for missing that earlier.

@dfandrich
Copy link
Contributor

Test 1608 now fails with a memory leak under Polar SSL: https://curl.haxx.se/dev/log.cgi?id=20180318182706-28132#prob75 Was this introduced with this commit or did it just tickle a pre-existing problem?

@lock lock bot locked as resolved and limited conversation to collaborators Jun 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants