dns: extend CURLOPT_RESOLVE syntax for adding non-permanent entries

[pgroke-dt]

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.

[bagder]

Thanks!

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

[danielgustafsson]

On 10 Dec 2020, at 16:36, Daniel Stenberg ***@***.***> wrote: I personally like amended commits and force-pushes (as long as you're the single developer in the branch) so that we can review the final result easier.

Seconded.

[pgroke-dt]

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.

[bagder]

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]

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]

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]

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]

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

[pgroke-dt]

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

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.

[pgroke-dt]

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

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]

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]

Thanks!

[serjepatoff]

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]

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.