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.

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

FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9895

Merged
merged 6 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/base/dictionary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,20 @@ bool Dictionary::Get(const String& key, Value *result) const
return true;
}

/**
* Retrieves a value's address from a dictionary.
*
* @param key The key whose value's address should be retrieved.
* @returns nullptr if the key was not found.
*/
const Value * Dictionary::GetRef(const String& key) const
{
std::shared_lock<std::shared_timed_mutex> lock (m_DataMutex);
auto it (m_Data.find(key));

return it == m_Data.end() ? nullptr : &it->second;
}

/**
* Sets a value in the dictionary.
*
Expand Down
1 change: 1 addition & 0 deletions lib/base/dictionary.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class Dictionary final : public Object

Value Get(const String& key) const;
bool Get(const String& key, Value *result) const;
const Value * GetRef(const String& key) const;
void Set(const String& key, Value value, bool overrideFrozen = false);
bool Contains(const String& key) const;

Expand Down
66 changes: 39 additions & 27 deletions lib/config/applyrule-targeted.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,16 @@ bool ApplyRule::AddTargetedRule(const ApplyRule::Ptr& rule, const String& target
*
* @returns Whether the given assign filter is like above.
*/
bool ApplyRule::GetTargetHosts(Expression* assignFilter, std::vector<const String *>& hosts)
bool ApplyRule::GetTargetHosts(Expression* assignFilter, std::vector<const String *>& hosts, const Dictionary::Ptr& constants)
{
auto lor (dynamic_cast<LogicalOrExpression*>(assignFilter));

if (lor) {
return GetTargetHosts(lor->GetOperand1().get(), hosts)
&& GetTargetHosts(lor->GetOperand2().get(), hosts);
return GetTargetHosts(lor->GetOperand1().get(), hosts, constants)
&& GetTargetHosts(lor->GetOperand2().get(), hosts, constants);
}

auto name (GetComparedName(assignFilter, "host"));
auto name (GetComparedName(assignFilter, "host", constants));

if (name) {
hosts.emplace_back(name);
Expand All @@ -124,16 +124,16 @@ bool ApplyRule::GetTargetHosts(Expression* assignFilter, std::vector<const Strin
*
* @returns Whether the given assign filter is like above.
*/
bool ApplyRule::GetTargetServices(Expression* assignFilter, std::vector<std::pair<const String *, const String *>>& services)
bool ApplyRule::GetTargetServices(Expression* assignFilter, std::vector<std::pair<const String *, const String *>>& services, const Dictionary::Ptr& constants)
{
auto lor (dynamic_cast<LogicalOrExpression*>(assignFilter));

if (lor) {
return GetTargetServices(lor->GetOperand1().get(), services)
&& GetTargetServices(lor->GetOperand2().get(), services);
return GetTargetServices(lor->GetOperand1().get(), services, constants)
&& GetTargetServices(lor->GetOperand2().get(), services, constants);
}

auto service (GetTargetService(assignFilter));
auto service (GetTargetService(assignFilter, constants));

if (service.first) {
services.emplace_back(service);
Expand All @@ -152,7 +152,7 @@ bool ApplyRule::GetTargetServices(Expression* assignFilter, std::vector<std::pai
*
* @returns {host, service} on success and {nullptr, nullptr} on failure.
*/
std::pair<const String *, const String *> ApplyRule::GetTargetService(Expression* assignFilter)
std::pair<const String *, const String *> ApplyRule::GetTargetService(Expression* assignFilter, const Dictionary::Ptr& constants)
{
auto land (dynamic_cast<LogicalAndExpression*>(assignFilter));

Expand All @@ -162,15 +162,15 @@ std::pair<const String *, const String *> ApplyRule::GetTargetService(Expression

auto op1 (land->GetOperand1().get());
auto op2 (land->GetOperand2().get());
auto host (GetComparedName(op1, "host"));
auto host (GetComparedName(op1, "host", constants));

if (!host) {
std::swap(op1, op2);
host = GetComparedName(op1, "host");
host = GetComparedName(op1, "host", constants);
}

if (host) {
auto service (GetComparedName(op2, "service"));
auto service (GetComparedName(op2, "service", constants));

if (service) {
return {host, service};
Expand All @@ -189,7 +189,7 @@ std::pair<const String *, const String *> ApplyRule::GetTargetService(Expression
*
* @returns The object name on success and nullptr on failure.
*/
const String * ApplyRule::GetComparedName(Expression* assignFilter, const char * lcType)
const String * ApplyRule::GetComparedName(Expression* assignFilter, const char * lcType, const Dictionary::Ptr& constants)
{
auto eq (dynamic_cast<EqualExpression*>(assignFilter));

Expand All @@ -200,12 +200,12 @@ const String * ApplyRule::GetComparedName(Expression* assignFilter, const char *
auto op1 (eq->GetOperand1().get());
auto op2 (eq->GetOperand2().get());

if (IsNameIndexer(op1, lcType)) {
return GetLiteralStringValue(op2);
if (IsNameIndexer(op1, lcType, constants)) {
return GetConstString(op2, constants);
}

if (IsNameIndexer(op2, lcType)) {
return GetLiteralStringValue(op1);
if (IsNameIndexer(op2, lcType, constants)) {
return GetConstString(op1, constants);
}

return nullptr;
Expand All @@ -214,7 +214,7 @@ const String * ApplyRule::GetComparedName(Expression* assignFilter, const char *
/**
* @returns Whether the given expression is like $lcType$.name.
*/
bool ApplyRule::IsNameIndexer(Expression* exp, const char * lcType)
bool ApplyRule::IsNameIndexer(Expression* exp, const char * lcType, const Dictionary::Ptr& constants)
{
auto ixr (dynamic_cast<IndexerExpression*>(exp));

Expand All @@ -228,27 +228,39 @@ bool ApplyRule::IsNameIndexer(Expression* exp, const char * lcType)
return false;
}

auto val (GetLiteralStringValue(ixr->GetOperand2().get()));
auto val (GetConstString(ixr->GetOperand2().get(), constants));

return val && *val == "name";
}

/**
* @returns If the given expression is a string literal, the string. nullptr on failure.
* @returns If the given expression is a constant string, its address. nullptr on failure.
*/
const String * ApplyRule::GetLiteralStringValue(Expression* exp)
const String * ApplyRule::GetConstString(Expression* exp, const Dictionary::Ptr& constants)
{
auto cnst (GetConst(exp, constants));

return cnst && cnst->IsString() ? &cnst->Get<String>() : nullptr;
}

/**
* @returns If the given expression is a constant, its address. nullptr on failure.
*/
const Value * ApplyRule::GetConst(Expression* exp, const Dictionary::Ptr& constants)
{
auto lit (dynamic_cast<LiteralExpression*>(exp));

if (!lit) {
return nullptr;
if (lit) {
return &lit->GetValue();
}

auto& val (lit->GetValue());
if (constants) {
auto var (dynamic_cast<VariableExpression*>(exp));

if (!val.IsString()) {
return nullptr;
if (var) {
return constants->GetRef(var->GetVariable());
}
}

return &val.Get<String>();
return nullptr;
}
13 changes: 7 additions & 6 deletions lib/config/applyrule.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ class ApplyRule : public SharedObject
static const std::vector<ApplyRule::Ptr>& GetRules(const Type::Ptr& sourceType, const Type::Ptr& targetType);
static const std::set<ApplyRule::Ptr>& GetTargetedHostRules(const Type::Ptr& sourceType, const String& host);
static const std::set<ApplyRule::Ptr>& GetTargetedServiceRules(const Type::Ptr& sourceType, const String& host, const String& service);
static bool GetTargetHosts(Expression* assignFilter, std::vector<const String *>& hosts, const Dictionary::Ptr& constants = nullptr);
static bool GetTargetServices(Expression* assignFilter, std::vector<std::pair<const String *, const String *>>& services, const Dictionary::Ptr& constants = nullptr);

static void RegisterType(const String& sourceType, const std::vector<String>& targetTypes);
static bool IsValidSourceType(const String& sourceType);
Expand All @@ -108,12 +110,11 @@ class ApplyRule : public SharedObject
static RuleMap m_Rules;

static bool AddTargetedRule(const ApplyRule::Ptr& rule, const String& targetType, PerSourceType& rules);
static bool GetTargetHosts(Expression* assignFilter, std::vector<const String *>& hosts);
static bool GetTargetServices(Expression* assignFilter, std::vector<std::pair<const String *, const String *>>& services);
static std::pair<const String *, const String *> GetTargetService(Expression* assignFilter);
static const String * GetComparedName(Expression* assignFilter, const char * lcType);
static bool IsNameIndexer(Expression* exp, const char * lcType);
static const String * GetLiteralStringValue(Expression* exp);
static std::pair<const String *, const String *> GetTargetService(Expression* assignFilter, const Dictionary::Ptr& constants);
static const String * GetComparedName(Expression* assignFilter, const char * lcType, const Dictionary::Ptr& constants);
static bool IsNameIndexer(Expression* exp, const char * lcType, const Dictionary::Ptr& constants);
static const String * GetConstString(Expression* exp, const Dictionary::Ptr& constants);
static const Value * GetConst(Expression* exp, const Dictionary::Ptr& constants);

ApplyRule(String name, Expression::Ptr expression,
Expression::Ptr filter, String package, String fkvar, String fvvar, Expression::Ptr fterm,
Expand Down
5 changes: 5 additions & 0 deletions lib/config/expression.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,11 @@ class DictExpression final : public DebuggableExpression

void MakeInline();

inline const std::vector<std::unique_ptr<Expression>>& GetExpressions() const noexcept
{
return m_Expressions;
}

protected:
ExpressionResult DoEvaluate(ScriptFrame& frame, DebugHint *dhint) const override;

Expand Down
72 changes: 64 additions & 8 deletions lib/remote/filterutility.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "remote/filterutility.hpp"
#include "remote/httputility.hpp"
#include "config/applyrule.hpp"
#include "config/configcompiler.hpp"
#include "config/expression.hpp"
#include "base/namespace.hpp"
Expand Down Expand Up @@ -271,18 +272,73 @@ std::vector<Value> FilterUtility::GetFilterTargets(const QueryDescription& qd, c
if (query->Contains("filter")) {
String filter = HttpUtility::GetLastParameter(query, "filter");
std::unique_ptr<Expression> ufilter = ConfigCompiler::CompileText("<API query>", filter);

Dictionary::Ptr filter_vars = query->Get("filter_vars");
if (filter_vars) {
ObjectLock olock(filter_vars);
for (const Dictionary::Pair& kv : filter_vars) {
frameNS->Set(kv.first, kv.second);
bool targeted = false;
std::vector<ConfigObject::Ptr> targets;

if (dynamic_cast<ConfigObjectTargetProvider*>(provider.get())) {
auto dict (dynamic_cast<DictExpression*>(ufilter.get()));

if (dict) {
auto& subex (dict->GetExpressions());

if (subex.size() == 1u) {
if (type == "Host") {
Comment on lines +279 to +286
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There's a filter on Hosts/Services

std::vector<const String *> targetNames;

if (ApplyRule::GetTargetHosts(subex.at(0).get(), targetNames, filter_vars)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the goal of this PR is reducing the performance overhead, you should favour subex[0] over subex.at(0).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't give much in addition, at the cost of SEGV -not just exception- if the code breaks in the future.

static const auto typeHost (Type::GetByName("Host"));
static const auto ctypeHost (dynamic_cast<ConfigType*>(typeHost.get()));
targeted = true;

for (auto name : targetNames) {
auto target (ctypeHost->GetObject(*name));

if (target) {
targets.emplace_back(target);
}
}
}
} else if (type == "Service") {
std::vector<std::pair<const String *, const String *>> targetNames;

if (ApplyRule::GetTargetServices(subex.at(0).get(), targetNames, filter_vars)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here!

static const auto typeService (Type::GetByName("Service"));
static const auto ctypeService (dynamic_cast<ConfigType*>(typeService.get()));
targeted = true;

for (auto name : targetNames) {
auto target (ctypeService->GetObject(*name.first + "!" + *name.second));

if (target) {
targets.emplace_back(target);
}
}
}
}
}
}
}

provider->FindTargets(type, [&permissionFrame, &permissionFilter, &frame, &ufilter, &result, variableName](const Object::Ptr& target) {
FilteredAddTarget(permissionFrame, permissionFilter.get(), frame, &*ufilter, result, variableName, target);
});
if (targeted) {
for (auto& target : targets) {
if (FilterUtility::EvaluateFilter(permissionFrame, permissionFilter.get(), target, variableName)) {
result.emplace_back(std::move(target));
}
}
Comment on lines +323 to +328
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. If yes: fetch just them, don't run filter for all

} else {
if (filter_vars) {
ObjectLock olock (filter_vars);

for (auto& kv : filter_vars) {
frameNS->Set(kv.first, kv.second);
}
}

provider->FindTargets(type, [&permissionFrame, &permissionFilter, &frame, &ufilter, &result, variableName](const Object::Ptr& target) {
FilteredAddTarget(permissionFrame, permissionFilter.get(), frame, &*ufilter, result, variableName, target);
});
}
} else {
/* Ensure to pass a nullptr as filter expression.
* GCC 8.1.1 on F28 causes problems, see GH #6533.
Expand Down
33 changes: 33 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ set(base_test_SOURCES
base-type.cpp
base-utility.cpp
base-value.cpp
config-apply.cpp
config-ops.cpp
icinga-checkresult.cpp
icinga-dependencies.cpp
Expand Down Expand Up @@ -123,6 +124,38 @@ add_boost_test(base
base_value/scalar
base_value/convert
base_value/format
config_apply/gettargethosts_literal
config_apply/gettargethosts_const
config_apply/gettargethosts_swapped
config_apply/gettargethosts_two
config_apply/gettargethosts_three
config_apply/gettargethosts_mixed
config_apply/gettargethosts_redundant
config_apply/gettargethosts_badconst
config_apply/gettargethosts_notliteral
config_apply/gettargethosts_wrongop
config_apply/gettargethosts_wrongattr
config_apply/gettargethosts_wrongvar
config_apply/gettargethosts_noindexer
config_apply/gettargetservices_literal
config_apply/gettargetservices_const
config_apply/gettargetservices_swapped_outer
config_apply/gettargetservices_swapped_inner
config_apply/gettargetservices_two
config_apply/gettargetservices_three
config_apply/gettargetservices_mixed
config_apply/gettargetservices_redundant
config_apply/gettargetservices_badconst
config_apply/gettargetservices_notliteral
config_apply/gettargetservices_wrongop_outer
config_apply/gettargetservices_wrongop_host
config_apply/gettargetservices_wrongop_service
config_apply/gettargetservices_wrongattr_host
config_apply/gettargetservices_wrongattr_service
config_apply/gettargetservices_wrongvar_host
config_apply/gettargetservices_wrongvar_service
config_apply/gettargetservices_noindexer_host
config_apply/gettargetservices_noindexer_service
config_ops/simple
config_ops/advanced
icinga_checkresult/host_1attempt
Expand Down
Loading
Loading