fix(userspace/falco): fix outputs_http timeout #3523
Conversation
|
Welcome @benierc! It looks like this is your first PR to falcosecurity/falco 🎉 |
userspace/falco/outputs_http.cpp
Outdated
| do { | ||
| res = curl_easy_perform(m_curl); | ||
| } while(res == CURLE_OPERATION_TIMEDOUT); |
There was a problem hiding this comment.
The change is fine but we'd better avoid a (possibly) infinite loop here. What about making the number of loops configurable through the Falco config file?
Eg: http_output.max_consecutive_timeouts (https://github.com/falcosecurity/falco/blob/master/falco.yaml#L726); a value like 5/10 by default is enough i think.
There was a problem hiding this comment.
yes good point i will try to add it
|
Thanks for the PR! I left a comment to avoid the unbound loop ;) |
|
Hi @benierc ! Great job! You need to update config json schema too with the new config key: https://github.com/falcosecurity/falco/blob/master/userspace/falco/config_json_schema.h |
benierc
force-pushed
the
master
branch
3 times, most recently
from
0dc7fbf to
ae3dd2f
Compare
userspace/falco/configuration.cpp
Outdated
| @@ -447,6 +447,11 @@ void falco_configuration::load_yaml(const std::string &config_name) { | |||
| keep_alive = m_config.get_scalar<bool>("http_output.keep_alive", false); | |||
| http_output.options["keep_alive"] = keep_alive ? std::string("true") : std::string("false"); | |||
|
|
|||
| uint32_t max_consecutive_timeouts; | |||
| max_consecutive_timeouts = | |||
| m_config.get_scalar<uint32_t>("http_output.max_consecutive_timeouts", 5); | |||
There was a problem hiding this comment.
Shouldn't m_max_consecutive_timeouts be enclosed in a range to avoid having an "almost" infinite loop if someone inputs a high value ?
Maybe also use a type smaller than unit32 to store it.
There was a problem hiding this comment.
ok i see, so an uint16_t or uint8_t should be better @sgaist ?
There was a problem hiding this comment.
unit8's max being 255, I think it should already cover most use cases.
If there's a need in the future to have more than that number of retries, it's always possible to update at this time.
|
The CI failures do not come from this PR! You will need to rebase once #3537 is merged :) |
|
Ok the PR has been merged, you can rebase on top of master! |
libcurl timeout prevent to send alert through http keep trying to send the alert Signed-off-by: Clément Bénier <[email protected]>
Co-authored-by: Samuel Gaist <[email protected]> Signed-off-by: benierc <[email protected]>
Co-authored-by: Federico Di Pierro <[email protected]> Signed-off-by: benierc <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benierc, FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. Git tree hash: 463e1390673a48f8651ac86223978e34c36b258a
|
|
@sgaist PTAL 🙏 |
libcurl timeout prevent to send alert through http keep trying to send the alert
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area engine
What this PR does / why we need it:
this PR ignore http_output libcurl timeout when network is not available,
it allows to send the alert when network is back and to not lost it
Which issue(s) this PR fixes:
Fixes #3522
Special notes for your reviewer:
Does this PR introduce a user-facing change?: