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

dns: extend CURLOPT_RESOLVE syntax for adding non-permanent entries #6294

Closed

Conversation

pgroke-dt
Copy link
Contributor

@pgroke-dt pgroke-dt commented Dec 9, 2020

Extend the syntax of CURLOPT_RESOLVE strings: allow using a '+' prefix
(similar to the existing '-' prefix for removing entries) to add
DNS cache entries that will time out just like entries that are added
by libcurl itself.

Append " (non-permanent)" to info log message in case a non-permanent
entry is added.

Adjust relevant comments to reflect the new behavior.

Adjust documentation.

Extend unit1607 to test the new functionality.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Thanks!

Also, remember to extend the docs so that users can figure out that this feature exists and how to use it!

tests/unit/unit1607.c Outdated Show resolved Hide resolved
@danielgustafsson
Copy link
Member

danielgustafsson commented Dec 10, 2020 via email

@pgroke-dt pgroke-dt force-pushed the allow-non-permanent-CURLOPT_RESOLVE branch from bfc3107 to 2573aa3 Compare December 10, 2020 18:07
@pgroke-dt
Copy link
Contributor Author

I've updated the PR:

  • use TRUE/FALSE instead of true/false
  • update documentation
  • append " (non-permanent)" to the info log message in case a non-permanent
    entry is added

Please let me know if there's anything else I should/need to do before this can be merged.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

This looks great! One nit: I think the docs/cmdline/resolve.d also needs an update for users of the command line tool to learn about this feature as well!

@pgroke-dt
Copy link
Contributor Author

Thanks!
I didn't know that the feature was also available via command line, thanks for letting me know. Will update the PR.

BTW: Do you know what's up with the tests? Almost all PRs seem to be red currently. Is someone working on fixing this or is this a long-time known flakiness that you/we just live with?

@bagder
Copy link
Member

bagder commented Dec 18, 2020

Do you know what's up with the tests? Almost all PRs seem to be red currently. Is someone working on fixing this or is this a long-time known flakiness that you/we just live with?

Yes and no. I'm always trying to fix flaky tests and make sure they all run green. But some are really complicated and every now and then a new one pops up. This makes it a very rare occasion that all 90-somthing CI jobs actually completes green... 😞

@pgroke-dt
Copy link
Contributor Author

pgroke-dt commented Dec 19, 2020

Thinking about this again, I'm not sure it would make sense to add that to the curl command line documentation.
It's only useful if multiple files are transferred and the transfers take long enough for there to be a realistic chance that the IP specified via --resolve is no longer usable. And of course only if the DNS server can resolve the host in the first place. And in such a case - why use --resolve at all?

Our case is different, because we use libcurl in a tool that's long running but wants to create the initial connection as quickly as possible -- and we already have the resolved IPs at the time we do the first libcurl transfer. So for libcurl it IMO makes a lot of sense. But for the command line tool, I think it may hurt more than help (by creating confusion -- why is this option there/what is it good for?).

Let me know what you think.
Updating resolve.d is trivial, so if you still think we should, then I'll update the PR as promised.

@jay
Copy link
Member

jay commented Dec 19, 2020

Regarding the syntax, would something like ,timeout be easier to understand:
foo.com:443:127.0.0.1,timeout:60

@pgroke-dt
Copy link
Contributor Author

@jay
We discussed the syntax on the mailing list and agreed on the "+" prefix.
I agree that it's not the most intuitive, but there are other factors to consider:

  • How easy is it to integrate into the current source. The "+" prefix wins big time there.
  • How much does it restrict future extensibility. The "+" prefix is good there as well. The option already has a "-" prefix, so adding an additional prefix character doesn't change much. OTOH appending something is a much bigger deal, since the tail of the option string is the address list. And I think that should be kept open for extensions as much as possible. While I think that it's unlikely that a new address format would parse something like "timeout" or "temp" as a valid address, it would still complicate things.

And regarding the 60 in "timeout:60": that would require more modifications because libcurl currently only supports one timeout for all DNS cache entries.

So IMO adding the option as it's implemented now is a good solution. I think the syntax is OK and I would not want to change the PR again (for obvious reasons :)).

@bagder
Copy link
Member

bagder commented Dec 19, 2020

I'm not sure it would make sense to add that to the curl command line documentation.

I think it should be, because the command line tool passes the data into the same option so it does use he same format, even if I agree that for command line users it might very rarely actually be a useful and wanted feature.

Extend the syntax of CURLOPT_RESOLVE strings: allow using a '+' prefix
(similar to the existing '-' prefix for removing entries) to add
DNS cache entries that will time out just like entries that are added
by libcurl itself.

Append " (non-permanent)" to info log message in case a non-permanent
entry is added.

Adjust relevant comments to reflect the new behavior.

Adjust documentation.

Extend unit1607 to test the new functionality.
@pgroke-dt pgroke-dt force-pushed the allow-non-permanent-CURLOPT_RESOLVE branch from 2573aa3 to 282295a Compare December 22, 2020 21:45
@pgroke-dt
Copy link
Contributor Author

@bagder I've added the change for the command line documentation. I've had a quick look at the code of the command line utility + also the source generated with --libcurl. I'm not 100% certain but it seems to me like this will not be effective when using serial transfers, since the option will be set again for each single transfer in serial mode. Which means it will replace the DNS cache entry every time with a new one which has a fresh timeout.

However after some googling/reading the documentation I'm not even sure this will have an effect in parallel mode. I'd have to add some debug output to the code and run some tests to make sure.

So again, I think we should not add this the the command line documentation. The best case scenario we can achieve with it is to not confuse people by describing an option that they will never need. But I'll leave the decision up to you.

Again, please let me know if I have to do/change anything before this can be merged.

@bagder
Copy link
Member

bagder commented Dec 23, 2020

The + is supported from the command line too - unless you add code to prevent that. And since it is supported from the command line, I think it should be documented.

I totally agree with you that it takes a rather convoluted example to come up with a way for a user to actually want to use this, but I don't think that's reason enough to not document a function that is accessible.

@pgroke
Copy link

pgroke commented Dec 28, 2020

As I said, I think it will harm more by confusing people than help. But if you want to add it, that's fine. As I said I've updated the PR.

Can this be merged in its current form or do you want me to change anything?

@bagder bagder closed this in 8324dc8 Dec 29, 2020
@bagder
Copy link
Member

bagder commented Dec 29, 2020

Thanks!

@serjepatoff
Copy link

Can this option be applied to CURL-multi handle? So that each easy handle added to CURLM use it forcibly?

PS. Sorry for dumb question. Documentation for CURLOPT_RESOLVE option doesn't explicitly allows or forbids this use case.

@pgroke
Copy link

pgroke commented Mar 20, 2021

CURLOPT_RESOLVE is an option which is set to/on an easy object using curl_easy_setopt and stored in the easy.
However it will take effect in the DNS cache object.

The CURLOPT_RESOLVE option will be evaluated only when a transfer is started.

At that time libcurl will determine which DNS cache object to use.
If the easy object is using a share object which shares the DNS cache, the DNS cache from the share object will be used.
Otherwise, if the easy object is used with a multi object, then the DNS cache of the multi object will be used.
Otherwise a DNS cache object that's private to the easy object will be used.

Once the DNS cache object has been determined, the CURLOPT_RESOLVE list will be parsed, and the actions that are encoded in it will be applied to the selected DNS cache object (creating or removing entries).
After this has been done, libcurl will "remember" that the CURLOPT_RESOLVE list has been applied so it's not applied again when another transfer is started using the same easy object. (Setting CURLOPT_RESOLVE again will reset this information so the newly set CURLOPT_RESOLVE list will be used the next time a transfer is started with the respective easy object.)

So... long story short: you can not use CURLOPT_RESOLVE with curl_multi_setopt. But you can use CURLOPT_RESOLVE with curl_easy_setopt to set the option to an easy object and then use that easy object in a multi object to modify the DNS cache of the multi object.

Phew. I hope this description is accurate and actually understandable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants