fix(userspace/falco): fix outputs_http timeout #3523
Conversation
|
Welcome @benierc! It looks like this is your first PR to falcosecurity/falco 🎉 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: benierc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
libcurl timeout prevent to send alert through http keep trying to send the alert Signed-off-by: Clément Bénier <[email protected]>
| @@ -514,6 +514,9 @@ const char config_schema_string[] = LONG_STRING_CONST( | |||
| }, | |||
| "keep_alive": { | |||
| "type": "boolean" | |||
| }, | |||
| "max_consecutive_timeouts:" { | |||
There was a problem hiding this comment.
| "max_consecutive_timeouts:" { | |
| "max_consecutive_timeouts": { |
:P
There was a problem hiding this comment.
This will fix the CI @benierc !
| @@ -103,7 +105,16 @@ bool falco::outputs::output_http::init(const config &oc, | |||
|
|
|||
| void falco::outputs::output_http::output(const message *msg) { | |||
| CURLcode res = curl_easy_setopt(m_curl, CURLOPT_POSTFIELDS, msg->msg.c_str()); | |||
| CHECK_RES(curl_easy_perform(m_curl)); | |||
| uint32_t curl_easy_platform_calls = 0; | |||
There was a problem hiding this comment.
Since m_max_consecutive_timeouts is 8bit no need for uint32_t here.
| uint32_t curl_easy_platform_calls = 0; | |
| uint8_t curl_easy_platform_calls = 0; |
| @@ -36,6 +36,8 @@ bool falco::outputs::output_http::init(const config &oc, | |||
|
|
|||
| m_curl = nullptr; | |||
| m_http_headers = nullptr; | |||
| m_max_consecutive_timeouts = | |||
| static_cast<uint8_t>(std::stoi(m_oc.options["max_consecutive_timeouts"])); | |||
There was a problem hiding this comment.
If we want to be extra careful:
| static_cast<uint8_t>(std::stoi(m_oc.options["max_consecutive_timeouts"])); | |
| static_cast<uint8_t>(std::stoi(m_oc.options["max_consecutive_timeouts"]) & 0xFF); |
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?: