From d2fb8a91817b8f2b9e27b2a85370e694e92a3915 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 1 Feb 2023 09:30:49 +0100 Subject: [PATCH 1/2] Handle errors when evaluating --define This can be observed when running something like `icinga2 daemon -C -DTypes.Host=42`. Without this commit: [2023-02-01 09:29:00 +0100] critical/Application: Error: Namespace is read-only and must not be modified. Additional information is available in '/var/log/icinga2/crash/report.1675240140.425155' With this commit: [2023-02-01 09:30:37 +0100] critical/icinga-app: cannot set 'Types.Host': Namespace is read-only and must not be modified. --- icinga-app/icinga.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp index be8a10ae4..f3e04dc76 100644 --- a/icinga-app/icinga.cpp +++ b/icinga-app/icinga.cpp @@ -427,8 +427,13 @@ static int Main() std::unique_ptr setExpr{new SetExpression(std::move(expr), OpSetLiteral, MakeLiteral(value))}; setExpr->SetOverrideFrozen(); - ScriptFrame frame(true); - setExpr->Evaluate(frame); + try { + ScriptFrame frame(true); + setExpr->Evaluate(frame); + } catch (const ScriptError& e) { + Log(LogCritical, "icinga-app") << "cannot set '" << key << "': " << e.what(); + return EXIT_FAILURE; + } } } From fd1aa73d252f8b18c385283b1d4626cb00935076 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 1 Feb 2023 09:37:27 +0100 Subject: [PATCH 2/2] Fix config sync after freezing namespaces This was accidentally broken by #9627 because during config sync, a config validation happens that uses `--define System.ZonesStageVarDir=...` which fails on the now frozen namespace. This commit changes this to use `Internal.ZonesStageVarDir` instead. After all, this is used for internal functionality, users should not directly interact with this flag. Additionally, it no longer freezes the `Internal` namespace which actually allows using `Internal.ZonesStageVarDir` in the first place. This also fixes `--define Internal.Debug*` which was also broken by said PR. Freezing of the `Internal` namespace is not necessary for performance reasons as it's not searched implicitly (for example when accessing `globals.x`) and should users actually interact with it, they should know by that name that they are on their own. --- lib/base/scriptframe.cpp | 6 ++---- lib/cli/daemonutility.cpp | 9 ++++++--- lib/remote/apilistener-filesync.cpp | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/base/scriptframe.cpp b/lib/base/scriptframe.cpp index 5d0807d72..7476f13e6 100644 --- a/lib/base/scriptframe.cpp +++ b/lib/base/scriptframe.cpp @@ -11,7 +11,7 @@ using namespace icinga; boost::thread_specific_ptr > ScriptFrame::m_ScriptFrames; -static Namespace::Ptr l_SystemNS, l_StatsNS, l_InternalNS; +static Namespace::Ptr l_SystemNS, l_StatsNS; /* Ensure that this gets called with highest priority * and wins against other static initializers in lib/icinga, etc. @@ -36,14 +36,12 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() { l_StatsNS = new Namespace(true); globalNS->Set("StatsFunctions", l_StatsNS, true); - l_InternalNS = new Namespace(true); - globalNS->Set("Internal", l_InternalNS, true); + globalNS->Set("Internal", new Namespace(true), true); }, InitializePriority::CreateNamespaces); INITIALIZE_ONCE_WITH_PRIORITY([]() { l_SystemNS->Freeze(); l_StatsNS->Freeze(); - l_InternalNS->Freeze(); }, InitializePriority::FreezeNamespaces); ScriptFrame::ScriptFrame(bool allocLocals) diff --git a/lib/cli/daemonutility.cpp b/lib/cli/daemonutility.cpp index c73da694a..ad6526480 100644 --- a/lib/cli/daemonutility.cpp +++ b/lib/cli/daemonutility.cpp @@ -111,6 +111,9 @@ bool DaemonUtility::ValidateConfigFiles(const std::vector& configs, Namespace::Ptr systemNS = ScriptGlobal::Get("System"); VERIFY(systemNS); + Namespace::Ptr internalNS = ScriptGlobal::Get("Internal"); + VERIFY(internalNS); + if (!objectsFile.IsEmpty()) ConfigCompilerContext::GetInstance()->OpenObjectsFile(objectsFile); @@ -134,7 +137,7 @@ bool DaemonUtility::ValidateConfigFiles(const std::vector& configs, success = true; /* Only load zone directory if we're not in staging validation. */ - if (!systemNS->Contains("ZonesStageVarDir")) { + if (!internalNS->Contains("ZonesStageVarDir")) { String zonesEtcDir = Configuration::ZonesDir; if (!zonesEtcDir.IsEmpty() && Utility::PathExists(zonesEtcDir)) { std::set zoneEtcDirs; @@ -177,8 +180,8 @@ bool DaemonUtility::ValidateConfigFiles(const std::vector& configs, String zonesVarDir = Configuration::DataDir + "/api/zones"; /* Cluster config sync stage validation needs this. */ - if (systemNS->Contains("ZonesStageVarDir")) { - zonesVarDir = systemNS->Get("ZonesStageVarDir"); + if (internalNS->Contains("ZonesStageVarDir")) { + zonesVarDir = internalNS->Get("ZonesStageVarDir"); Log(LogNotice, "DaemonUtility") << "Overriding zones var directory with '" << zonesVarDir << "' for cluster config sync staging."; diff --git a/lib/remote/apilistener-filesync.cpp b/lib/remote/apilistener-filesync.cpp index 77af960f4..acf8deb5d 100644 --- a/lib/remote/apilistener-filesync.cpp +++ b/lib/remote/apilistener-filesync.cpp @@ -554,7 +554,7 @@ void ApiListener::HandleConfigUpdate(const MessageOrigin::Ptr& origin, const Dic } /** - * Spawns a new validation process with 'System.ZonesStageVarDir' set to override the config validation zone dirs with + * Spawns a new validation process with 'Internal.ZonesStageVarDir' set to override the config validation zone dirs with * our current stage. Then waits for the validation result and if it was successful, the configuration is copied from * stage to production and a restart is triggered. On validation failure, there is no restart and this is logged. * @@ -584,7 +584,7 @@ void ApiListener::TryActivateZonesStage(const std::vector& relativePaths // Set the ZonesStageDir. This creates our own local chroot without any additional automated zone includes. args->Add("--define"); - args->Add("System.ZonesStageVarDir=" + GetApiZonesStageDir()); + args->Add("Internal.ZonesStageVarDir=" + GetApiZonesStageDir()); Process::Ptr process = new Process(Process::PrepareCommand(args)); process->SetTimeout(Application::GetReloadTimeout());