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.

Sign up for GitHub

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

Jump to bottom

IfwApiCheckTask uses thread-local variable in coroutine #10144

Open
julianbrost opened this issue Aug 30, 2024 · 1 comment · May be fixed by #10140
Open

IfwApiCheckTask uses thread-local variable in coroutine #10144

julianbrost opened this issue Aug 30, 2024 · 1 comment · May be fixed by #10140
Assignees
@yhabteab
Labels
area/checks Check execution and results bug Something isn't working
Milestone
2.15.0

Comments

@julianbrost
Copy link
Contributor

Random "that can't possibly work" observation while looking for something different:

ReportIfwCheckResult() in ifwapichecktask.cpp uses Checkable::ExecuteCommandProcessFinishedHandler:

icinga2/lib/methods/ifwapichecktask.cpp

Lines 36 to 49 in 4c6b93d

static void ReportIfwCheckResult(
const Checkable::Ptr& checkable, const Value& cmdLine, const CheckResult::Ptr& cr,
const String& output, double start, double end, int exitcode = 3, const Array::Ptr& perfdata = nullptr
)
{
if (Checkable::ExecuteCommandProcessFinishedHandler) {
ProcessResult pr;
pr.PID = -1;
pr.Output = perfdata ? output + " |" + String(perfdata->Join(" ")) : output;
pr.ExecutionStart = start;
pr.ExecutionEnd = end;
pr.ExitStatus = exitcode;
Checkable::ExecuteCommandProcessFinishedHandler(cmdLine, pr);

However, Checkable::ExecuteCommandProcessFinishedHandler is defined as thread_local:

icinga2/lib/icinga/checkable.cpp

Line 34 in 4c6b93d

thread_local std::function<void(const Value& commandLine, const ProcessResult&)> Checkable::ExecuteCommandProcessFinishedHandler;

And ReportIfwCheckResult() is used from within DoIfwNetIo():

icinga2/lib/methods/ifwapichecktask.cpp

Lines 96 to 284 in 4c6b93d

static void DoIfwNetIo(
boost::asio::yield_context yc, const Checkable::Ptr& checkable, const Array::Ptr& cmdLine,
const CheckResult::Ptr& cr, const String& psCommand, const String& psHost, const String& san, const String& psPort,
AsioTlsStream& conn, boost::beast::http::request<boost::beast::http::string_body>& req, double start
)
{
namespace http = boost::beast::http;
boost::beast::flat_buffer buf;
http::response<http::string_body> resp;
try {
Connect(conn.lowest_layer(), psHost, psPort, yc);
} catch (const std::exception& ex) {
ReportIfwCheckResult(
yc, checkable, cmdLine, cr,
"Can't connect to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
start
);
return;
}
auto& sslConn (conn.next_layer());
try {
sslConn.async_handshake(conn.next_layer().client, yc);
} catch (const std::exception& ex) {
ReportIfwCheckResult(
yc, checkable, cmdLine, cr,
"TLS handshake with IfW API on host '" + psHost + "' (SNI: '" + san
+ "') port '" + psPort + "' failed: " + GetUnderstandableError(ex),
start
);
return;
}
if (!sslConn.IsVerifyOK()) {
auto cert (sslConn.GetPeerCertificate());
Value cn;
try {
cn = GetCertificateCN(cert);
} catch (const std::exception&) {
}
ReportIfwCheckResult(
yc, checkable, cmdLine, cr,
"Certificate validation failed for IfW API on host '" + psHost + "' (SNI: '" + san + "'; CN: "
+ (cn.IsString() ? "'" + cn + "'" : "N/A") + ") port '" + psPort + "': " + sslConn.GetVerifyError(),
start
);
return;
}
try {
http::async_write(conn, req, yc);
conn.async_flush(yc);
} catch (const std::exception& ex) {
ReportIfwCheckResult(
yc, checkable, cmdLine, cr,
"Can't send HTTP request to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
start
);
return;
}
try {
http::async_read(conn, buf, resp, yc);
} catch (const std::exception& ex) {
ReportIfwCheckResult(
yc, checkable, cmdLine, cr,
"Can't read HTTP response from IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
start
);
return;
}
double end = Utility::GetTime();
{
boost::system::error_code ec;
sslConn.async_shutdown(yc[ec]);
}
CpuBoundWork cbw (yc);
Value jsonRoot;
try {
jsonRoot = JsonDecode(resp.body());
} catch (const std::exception& ex) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Got bad JSON from IfW API on host '" + psHost + "' port '" + psPort + "': " + ex.what(), start, end
);
return;
}
if (!jsonRoot.IsObjectType<Dictionary>()) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Got JSON, but not an object, from IfW API on host '"
+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot),
start, end
);
return;
}
Value jsonBranch;
if (!Dictionary::Ptr(jsonRoot)->Get(psCommand, &jsonBranch)) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Missing ." + psCommand + " in JSON object from IfW API on host '"
+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot),
start, end
);
return;
}
if (!jsonBranch.IsObjectType<Dictionary>()) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"." + psCommand + " in JSON from IfW API on host '"
+ psHost + "' port '" + psPort + "' is not an object: " + JsonEncode(jsonBranch),
start, end
);
return;
}
Dictionary::Ptr result = jsonBranch;
Value exitcode;
if (!result->Get("exitcode", &exitcode)) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Missing ." + psCommand + ".exitcode in JSON object from IfW API on host '"
+ psHost + "' port '" + psPort + "': " + JsonEncode(result),
start, end
);
return;
}
static const std::set<double> exitcodes {ServiceOK, ServiceWarning, ServiceCritical, ServiceUnknown};
static const auto exitcodeList (Array::FromSet(exitcodes)->Join(", "));
if (!exitcode.IsNumber() || exitcodes.find(exitcode) == exitcodes.end()) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Got bad exitcode " + JsonEncode(exitcode) + " from IfW API on host '" + psHost + "' port '" + psPort
+ "', expected one of: " + exitcodeList,
start, end
);
return;
}
auto perfdataVal (result->Get("perfdata"));
Array::Ptr perfdata;
try {
perfdata = perfdataVal;
} catch (const std::exception&) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Got bad perfdata " + JsonEncode(perfdataVal) + " from IfW API on host '"
+ psHost + "' port '" + psPort + "', expected an array",
start, end
);
return;
}
if (perfdata) {
ObjectLock oLock (perfdata);
for (auto& pv : perfdata) {
if (!pv.IsString()) {
ReportIfwCheckResult(
checkable, cmdLine, cr,
"Got bad perfdata value " + JsonEncode(perfdata) + " from IfW API on host '"
+ psHost + "' port '" + psPort + "', expected an array of strings",
start, end
);
return;
}
}
}
ReportIfwCheckResult(checkable, cmdLine, cr, result->Get("checkresult"), start, end, exitcode, perfdata);
}

Which in turn is started as a coroutine:

icinga2/lib/methods/ifwapichecktask.cpp

Lines 512 to 530 in 4c6b93d

IoEngine::SpawnCoroutine(
*strand,
[strand, checkable, cmdLine, cr, psCommand, psHost, expectedSan, psPort, conn, req, start, checkTimeout](asio::yield_context yc) {
Timeout::Ptr timeout = new Timeout(strand->context(), *strand, boost::posix_time::microseconds(int64_t(checkTimeout * 1e6)),
[&conn, &checkable](boost::asio::yield_context yc) {
Log(LogNotice, "IfwApiCheckTask")
<< "Timeout while checking " << checkable->GetReflectionType()->GetName()
<< " '" << checkable->GetName() << "', cancelling attempt";
boost::system::error_code ec;
conn->lowest_layer().cancel(ec);
}
);
Defer cancelTimeout ([&timeout]() { timeout->Cancel(); });
DoIfwNetIo(yc, checkable, cmdLine, cr, psCommand, psHost, expectedSan, psPort, *conn, *req, start);
}
);

Thus has no guarantee on which thread it's run, so I don't see how accessing that thread-local variable should do something useful.
@julianbrost
Copy link
Contributor Author

PluginCheckTask also processes check results in a callable that may run somewhere else, but it actually took care of this:

icinga2/lib/methods/pluginchecktask.cpp

Lines 48 to 54 in 4c6b93d

if (Checkable::ExecuteCommandProcessFinishedHandler) {
callback = Checkable::ExecuteCommandProcessFinishedHandler;
} else {
callback = [checkable, cr](const Value& commandLine, const ProcessResult& pr) {
ProcessFinishedHandler(checkable, cr, commandLine, pr);
};
}

@yhabteab yhabteab self-assigned this Aug 30, 2024
@yhabteab yhabteab added bug Something isn't working area/checks Check execution and results labels Aug 30, 2024
@yhabteab yhabteab added this to the 2.15.0 milestone Aug 30, 2024
@yhabteab yhabteab linked a pull request Aug 30, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yhabteab yhabteab

Labels
area/checks Check execution and results bug Something isn't working
Projects
None yet
Milestone
2.15.0
Development

Successfully merging a pull request may close this issue.

Don't use thread-local var in coroutine & drop superfluous CpuBoundWork usage Icinga/icinga2
2 participants
@julianbrost @yhabteab