abedra / libvault

A C++ library for Hashicorp Vault
MIT License
34 stars 25 forks source link

Expose curl setup to user #96

Closed zspasztori closed 2 years ago

zspasztori commented 2 years ago

Exposed a way for the user to be able to set the curl options.

This can be useful for setting different types of timeouts for example.

abedra commented 2 years ago

What timeout other than CURLOPT_TIMEOUT are you looking for? Trying to figure out the use case here a bit more. Are there other settings you are looking to expose? Would any of these be better suited as config options?

zspasztori commented 2 years ago

Right now we would set :

However, it would be nice to have the ability to set others too if the need arises.

abedra commented 2 years ago

Timeout can be setup using https://github.com/abedra/libvault/blob/master/src/domain/ConfigBuilder.cpp#L33. I'd suggest adding to configuration options rather than pushing something that could potentially interfere with other settings required to make the requests work.

zspasztori commented 2 years ago

The timeout in the config is only for the connection. It would be nice to be able to set a timeout for the requests and responses too. I am aware that this has the potential to interfere, but on the other hand, it can only be used by advanced users.

zspasztori commented 2 years ago

We would like to set these additional timeouts for the curl calls : https://curl.se/libcurl/c/CURLOPT_TIMEOUT.html https://curl.se/libcurl/c/CURLOPT_LOW_SPEED_TIME.html https://curl.se/libcurl/c/CURLOPT_LOW_SPEED_LIMIT.html

The responses sent by vault can be bigger in size (3 kB+ in case of PKI engine). We would like to make sure to detect when the server transfer takes too long, and not get stuck.

Do you have any alternative way to do this?

I did not wan to add these 3 config fields to constructors not to bloat them, but that might also be an option.

abedra commented 2 years ago

We would like to set these additional timeouts for the curl calls : https://curl.se/libcurl/c/CURLOPT_TIMEOUT.html https://curl.se/libcurl/c/CURLOPT_LOW_SPEED_TIME.html https://curl.se/libcurl/c/CURLOPT_LOW_SPEED_LIMIT.html

The responses sent by vault can be bigger in size (3 kB+ in case of PKI engine). We would like to make sure to detect when the server transfer takes too long, and not get stuck.

Do you have any alternative way to do this?

I did not wan to add these 3 config fields to constructors not to bloat them, but that might also be an option.

That's actually my preference here. I'm not comfortable with the leaky abstraction being created with a blanket exposure to all curl options by the user. I went ahead and put together https://github.com/abedra/libvault/pull/97 that carves a clear path through configuration with the options listed above and sets defaults that can be modified with the builder.

Please take a look at this PR to see if it meets your needs.

zspasztori commented 2 years ago

thank you, the other PR seems good

abedra commented 2 years ago

This is merged and available in https://github.com/abedra/libvault/releases/tag/0.49.0