From d18eab67e87c8f29dece50480fa6370395bc958e Mon Sep 17 00:00:00 2001 From: Chris Hofstaedtler Date: Mon, 15 Jan 2018 21:11:29 +0100 Subject: [PATCH] dnsdist: reduce resprulactions/cachehitresprulactions code deuplication --- pdns/dnsdist-lua-actions.cc | 70 ++----- pdns/dnsdist-lua-rules.cc | 286 +++++++++------------------ pdns/dnsdist-web.cc | 49 +++-- regression-tests.dnsdist/test_API.py | 20 +- 4 files changed, 144 insertions(+), 281 deletions(-) diff --git a/pdns/dnsdist-lua-actions.cc b/pdns/dnsdist-lua-actions.cc index abc618a8a..9b86a8997 100644 --- a/pdns/dnsdist-lua-actions.cc +++ b/pdns/dnsdist-lua-actions.cc @@ -808,6 +808,19 @@ private: std::string d_value; }; +template +static void addAction(GlobalStateHolder > *someRulActions, luadnsrule_t var, std::shared_ptr action, boost::optional params) { + setLuaSideEffect(); + + boost::uuids::uuid uuid; + parseRuleParams(params, uuid); + + auto rule=makeRule(var); + someRulActions->modify([rule, action, uuid](vector& rulactions){ + rulactions.push_back({rule, action, uuid}); + }); +} + void setupLuaActions() { g_lua.writeFunction("newRuleAction", [](luadnsrule_t dnsrule, std::shared_ptr action, boost::optional params) { @@ -820,76 +833,35 @@ void setupLuaActions() }); g_lua.writeFunction("addAction", [](luadnsrule_t var, boost::variant, std::shared_ptr > era, boost::optional params) { - if (era.type() == typeid(std::shared_ptr)) { + if (era.type() != typeid(std::shared_ptr)) { throw std::runtime_error("addAction() can only be called with query-related actions, not response-related ones. Are you looking for addResponseAction()?"); } - boost::uuids::uuid uuid; - parseRuleParams(params, uuid); - - auto ea = *boost::get>(&era); - setLuaSideEffect(); - auto rule=makeRule(var); - g_rulactions.modify([rule, ea, uuid](decltype(g_rulactions)::value_type& rulactions){ - rulactions.push_back({rule, ea, uuid}); - }); + addAction(&g_rulactions, var, boost::get >(era), params); }); g_lua.writeFunction("addLuaAction", [](luadnsrule_t var, LuaAction::func_t func, boost::optional params) { - setLuaSideEffect(); - - boost::uuids::uuid uuid; - parseRuleParams(params, uuid); - - auto rule=makeRule(var); - g_rulactions.modify([rule, func, uuid](decltype(g_rulactions)::value_type& rulactions){ - rulactions.push_back({rule, std::make_shared(func), uuid}); - }); + addAction(&g_rulactions, var, std::make_shared(func), params); }); g_lua.writeFunction("addLuaResponseAction", [](luadnsrule_t var, LuaResponseAction::func_t func, boost::optional params) { - setLuaSideEffect(); - - boost::uuids::uuid uuid; - parseRuleParams(params, uuid); - - auto rule=makeRule(var); - g_resprulactions.modify([rule, func, uuid](decltype(g_resprulactions)::value_type& rulactions){ - rulactions.push_back({rule, std::make_shared(func), uuid}); - }); + addAction(&g_resprulactions, var, std::make_shared(func), params); }); g_lua.writeFunction("addResponseAction", [](luadnsrule_t var, boost::variant, std::shared_ptr > era, boost::optional params) { - if (era.type() == typeid(std::shared_ptr)) { + if (era.type() != typeid(std::shared_ptr)) { throw std::runtime_error("addResponseAction() can only be called with response-related actions, not query-related ones. Are you looking for addAction()?"); } - auto ea = *boost::get>(&era); - boost::uuids::uuid uuid; - parseRuleParams(params, uuid); - - setLuaSideEffect(); - auto rule=makeRule(var); - g_resprulactions.modify([rule, ea, uuid](decltype(g_resprulactions)::value_type& rulactions){ - rulactions.push_back({rule, ea, uuid}); - }); + addAction(&g_resprulactions, var, boost::get >(era), params); }); g_lua.writeFunction("addCacheHitResponseAction", [](luadnsrule_t var, boost::variant, std::shared_ptr> era, boost::optional params) { - if (era.type() == typeid(std::shared_ptr)) { + if (era.type() != typeid(std::shared_ptr)) { throw std::runtime_error("addCacheHitResponseAction() can only be called with response-related actions, not query-related ones. Are you looking for addAction()?"); } - setLuaSideEffect(); - auto rule=makeRule(var); - - boost::uuids::uuid uuid; - parseRuleParams(params, uuid); - - auto ea = *boost::get>(&era); - g_cachehitresprulactions.modify([rule, ea, uuid](decltype(g_cachehitresprulactions)::value_type& rulactions){ - rulactions.push_back({rule, ea, uuid}); - }); + addAction(&g_cachehitresprulactions, var, boost::get >(era), params); }); g_lua.registerFunction("printStats", [](const DNSAction& ta) { diff --git a/pdns/dnsdist-lua-rules.cc b/pdns/dnsdist-lua-rules.cc index f2b836620..4e2e53c8c 100644 --- a/pdns/dnsdist-lua-rules.cc +++ b/pdns/dnsdist-lua-rules.cc @@ -915,6 +915,87 @@ void parseRuleParams(boost::optional params, boost::uuids::uuid uuid = makeRuleID(uuidStr); } +template +static void showRules(GlobalStateHolder > *someRulActions, boost::optional showUUIDs) { + setLuaNoSideEffect(); + int num=0; + if (showUUIDs.get_value_or(false)) { + boost::format fmt("%-3d %-38s %9d %-56s %s\n"); + g_outputBuffer += (fmt % "#" % "UUID" % "Matches" % "Rule" % "Action").str(); + for(const auto& lim : someRulActions->getCopy()) { + string name = lim.d_rule->toString(); + g_outputBuffer += (fmt % num % boost::uuids::to_string(lim.d_id) % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); + ++num; + } + } + else { + boost::format fmt("%-3d %9d %-56s %s\n"); + g_outputBuffer += (fmt % "#" % "Matches" % "Rule" % "Action").str(); + for(const auto& lim : someRulActions->getCopy()) { + string name = lim.d_rule->toString(); + g_outputBuffer += (fmt % num % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); + ++num; + } + } +} + +template +static void rmRule(GlobalStateHolder > *someRulActions, boost::variant id) { + setLuaSideEffect(); + auto rules = someRulActions->getCopy(); + if (auto str = boost::get(&id)) { + boost::uuids::string_generator gen; + const auto uuid = gen(*str); + if (rules.erase(std::remove_if(rules.begin(), + rules.end(), + [uuid](const T& a) { return a.d_id == uuid; }), + rules.end()) == rules.end()) { + g_outputBuffer = "Error: no rule matched\n"; + return; + } + } + else if (auto pos = boost::get(&id)) { + if (*pos >= rules.size()) { + g_outputBuffer = "Error: attempt to delete non-existing rule\n"; + return; + } + rules.erase(rules.begin()+*pos); + } + someRulActions->setState(rules); +} + +template +static void topRule(GlobalStateHolder > *someRulActions) { + setLuaSideEffect(); + auto rules = someRulActions->getCopy(); + if(rules.empty()) + return; + auto subject = *rules.rbegin(); + rules.erase(std::prev(rules.end())); + rules.insert(rules.begin(), subject); + someRulActions->setState(rules); +} + +template +static void mvRule(GlobalStateHolder > *someRespRulActions, unsigned int from, unsigned int to) { + setLuaSideEffect(); + auto rules = someRespRulActions->getCopy(); + if(from >= rules.size() || to > rules.size()) { + g_outputBuffer = "Error: attempt to move rules from/to invalid index\n"; + return; + } + auto subject = rules[from]; + rules.erase(rules.begin()+from); + if(to == rules.size()) + rules.push_back(subject); + else { + if(from < to) + --to; + rules.insert(rules.begin()+to, subject); + } + someRespRulActions->setState(rules); +} + void setupLuaRules() { g_lua.writeFunction("makeRule", makeRule); @@ -922,209 +1003,47 @@ void setupLuaRules() g_lua.registerFunction::*)()>("toString", [](const std::shared_ptr& rule) { return rule->toString(); }); g_lua.writeFunction("showResponseRules", [](boost::optional showUUIDs) { - setLuaNoSideEffect(); - int num=0; - if (showUUIDs.get_value_or(false)) { - boost::format fmt("%-3d %-38s %9d %-50s %s\n"); - g_outputBuffer += (fmt % "#" % "UUID" % "Matches" % "Rule" % "Action").str(); - for(const auto& lim : g_resprulactions.getCopy()) { - string name = lim.d_rule->toString(); - g_outputBuffer += (fmt % num % boost::uuids::to_string(lim.d_id) % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); - ++num; - } - } - else { - boost::format fmt("%-3d %9d %-50s %s\n"); - g_outputBuffer += (fmt % "#" % "Matches" % "Rule" % "Action").str(); - for(const auto& lim : g_resprulactions.getCopy()) { - string name = lim.d_rule->toString(); - g_outputBuffer += (fmt % num % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); - ++num; - } - } + showRules(&g_resprulactions, showUUIDs); }); g_lua.writeFunction("rmResponseRule", [](boost::variant id) { - setLuaSideEffect(); - auto rules = g_resprulactions.getCopy(); - if (auto str = boost::get(&id)) { - boost::uuids::string_generator gen; - const auto uuid = gen(*str); - if (rules.erase(std::remove_if(rules.begin(), - rules.end(), - [uuid](const DNSDistResponseRuleAction& a) { return a.d_id == uuid; }), - rules.end()) == rules.end()) { - g_outputBuffer = "Error: no rule matched\n"; - } - } - else if (auto pos = boost::get(&id)) { - if (*pos >= rules.size()) { - g_outputBuffer = "Error: attempt to delete non-existing rule\n"; - return; - } - rules.erase(rules.begin()+*pos); - } - g_resprulactions.setState(rules); + rmRule(&g_resprulactions, id); }); g_lua.writeFunction("topResponseRule", []() { - setLuaSideEffect(); - auto rules = g_resprulactions.getCopy(); - if(rules.empty()) - return; - auto subject = *rules.rbegin(); - rules.erase(std::prev(rules.end())); - rules.insert(rules.begin(), subject); - g_resprulactions.setState(rules); + topRule(&g_resprulactions); }); g_lua.writeFunction("mvResponseRule", [](unsigned int from, unsigned int to) { - setLuaSideEffect(); - auto rules = g_resprulactions.getCopy(); - if(from >= rules.size() || to > rules.size()) { - g_outputBuffer = "Error: attempt to move rules from/to invalid index\n"; - return; - } - auto subject = rules[from]; - rules.erase(rules.begin()+from); - if(to == rules.size()) - rules.push_back(subject); - else { - if(from < to) - --to; - rules.insert(rules.begin()+to, subject); - } - g_resprulactions.setState(rules); + mvRule(&g_resprulactions, from, to); }); g_lua.writeFunction("showCacheHitResponseRules", [](boost::optional showUUIDs) { - setLuaNoSideEffect(); - int num=0; - if (showUUIDs.get_value_or(false)) { - boost::format fmt("%-3d %-38s %9d %-50s %s\n"); - g_outputBuffer += (fmt % "#" % "UUID" % "Matches" % "Rule" % "Action").str(); - for(const auto& lim : g_cachehitresprulactions.getCopy()) { - string name = lim.d_rule->toString(); - g_outputBuffer += (fmt % num % boost::uuids::to_string(lim.d_id) % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); - ++num; - } - } - else { - boost::format fmt("%-3d %9d %-50s %s\n"); - g_outputBuffer += (fmt % "#" % "Matches" % "Rule" % "Action").str(); - for(const auto& lim : g_cachehitresprulactions.getCopy()) { - string name = lim.d_rule->toString(); - g_outputBuffer += (fmt % num % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); - ++num; - } - } + showRules(&g_cachehitresprulactions, showUUIDs); }); g_lua.writeFunction("rmCacheHitResponseRule", [](boost::variant id) { - setLuaSideEffect(); - auto rules = g_cachehitresprulactions.getCopy(); - if (auto str = boost::get(&id)) { - boost::uuids::string_generator gen; - const auto uuid = gen(*str); - if (rules.erase(std::remove_if(rules.begin(), - rules.end(), - [uuid](const DNSDistResponseRuleAction& a) { return a.d_id == uuid; }), - rules.end()) == rules.end()) { - g_outputBuffer = "Error: no rule matched\n"; - } - } - else if (auto pos = boost::get(&id)) { - if (*pos >= rules.size()) { - g_outputBuffer = "Error: attempt to delete non-existing rule\n"; - return; - } - rules.erase(rules.begin()+*pos); - } - g_cachehitresprulactions.setState(rules); + rmRule(&g_cachehitresprulactions, id); }); g_lua.writeFunction("topCacheHitResponseRule", []() { - setLuaSideEffect(); - auto rules = g_cachehitresprulactions.getCopy(); - if(rules.empty()) - return; - auto subject = *rules.rbegin(); - rules.erase(std::prev(rules.end())); - rules.insert(rules.begin(), subject); - g_cachehitresprulactions.setState(rules); + topRule(&g_cachehitresprulactions); }); g_lua.writeFunction("mvCacheHitResponseRule", [](unsigned int from, unsigned int to) { - setLuaSideEffect(); - auto rules = g_cachehitresprulactions.getCopy(); - if(from >= rules.size() || to > rules.size()) { - g_outputBuffer = "Error: attempt to move rules from/to invalid index\n"; - return; - } - auto subject = rules[from]; - rules.erase(rules.begin()+from); - if(to == rules.size()) - rules.push_back(subject); - else { - if(from < to) - --to; - rules.insert(rules.begin()+to, subject); - } - g_cachehitresprulactions.setState(rules); + mvRule(&g_cachehitresprulactions, from, to); }); g_lua.writeFunction("rmRule", [](boost::variant id) { - setLuaSideEffect(); - auto rules = g_rulactions.getCopy(); - if (auto str = boost::get(&id)) { - boost::uuids::string_generator gen; - const auto uuid = gen(*str); - if (rules.erase(std::remove_if(rules.begin(), - rules.end(), - [uuid](const DNSDistRuleAction& a) { return a.d_id == uuid; }), - rules.end()) == rules.end()) { - g_outputBuffer = "Error: no rule matched\n"; - } - } - else if (auto pos = boost::get(&id)) { - if (*pos >= rules.size()) { - g_outputBuffer = "Error: attempt to delete non-existing rule\n"; - return; - } - rules.erase(rules.begin()+*pos); - } - g_rulactions.setState(rules); + rmRule(&g_rulactions, id); }); g_lua.writeFunction("topRule", []() { - setLuaSideEffect(); - auto rules = g_rulactions.getCopy(); - if(rules.empty()) - return; - auto subject = *rules.rbegin(); - rules.erase(std::prev(rules.end())); - rules.insert(rules.begin(), subject); - g_rulactions.setState(rules); + topRule(&g_rulactions); }); g_lua.writeFunction("mvRule", [](unsigned int from, unsigned int to) { - setLuaSideEffect(); - auto rules = g_rulactions.getCopy(); - if(from >= rules.size() || to > rules.size()) { - g_outputBuffer = "Error: attempt to move rules from/to invalid index\n"; - return; - } - - auto subject = rules[from]; - rules.erase(rules.begin()+from); - if(to == rules.size()) - rules.push_back(subject); - else { - if(from < to) - --to; - rules.insert(rules.begin()+to, subject); - } - g_rulactions.setState(rules); + mvRule(&g_rulactions, from, to); }); g_lua.writeFunction("clearRules", []() { @@ -1298,26 +1217,7 @@ void setupLuaRules() }); g_lua.writeFunction("showRules", [](boost::optional showUUIDs) { - setLuaNoSideEffect(); - int num=0; - if (showUUIDs.get_value_or(false)) { - boost::format fmt("%-3d %-38s %9d %-56s %s\n"); - g_outputBuffer += (fmt % "#" % "UUID" % "Matches" % "Rule" % "Action").str(); - for(const auto& lim : g_rulactions.getCopy()) { - string name = lim.d_rule->toString(); - g_outputBuffer += (fmt % num % boost::uuids::to_string(lim.d_id) % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); - ++num; - } - } - else { - boost::format fmt("%-3d %9d %-50s %s\n"); - g_outputBuffer += (fmt % "#" % "Matches" % "Rule" % "Action").str(); - for(const auto& lim : g_rulactions.getCopy()) { - string name = lim.d_rule->toString(); - g_outputBuffer += (fmt % num % lim.d_rule->d_matches % name % lim.d_action->toString()).str(); - ++num; - } - } + showRules(&g_rulactions, showUUIDs); }); g_lua.writeFunction("RDRule", []() { diff --git a/pdns/dnsdist-web.cc b/pdns/dnsdist-web.cc index 75b8f3190..6af354d56 100644 --- a/pdns/dnsdist-web.cc +++ b/pdns/dnsdist-web.cc @@ -219,6 +219,26 @@ static void addCustomHeaders(YaHTTP::Response& resp, const boost::optional +static json11::Json::array someResponseRulesToJson(GlobalStateHolder>* someResponseRules) +{ + using namespace json11; + Json::array responseRules; + int num=0; + auto localResponseRules = someResponseRules->getCopy(); + for(const auto& a : localResponseRules) { + Json::object rule{ + {"id", num++}, + {"uuid", boost::uuids::to_string(a.d_id)}, + {"matches", (double)a.d_rule->d_matches}, + {"rule", a.d_rule->toString()}, + {"action", a.d_action->toString()}, + }; + responseRules.push_back(rule); + } + return responseRules; +} + static void connectionThread(int sock, ComboAddress remote, string password, string apiKey, const boost::optional >& customHeaders) { using namespace json11; @@ -457,33 +477,8 @@ static void connectionThread(int sock, ComboAddress remote, string password, str rules.push_back(rule); } - Json::array responseRules; - auto localResponseRules = g_resprulactions.getCopy(); - num=0; - for(const auto& a : localResponseRules) { - Json::object rule{ - {"id", num++}, - {"uuid", boost::uuids::to_string(a.d_id)}, - {"matches", (double)a.d_rule->d_matches}, - {"rule", a.d_rule->toString()}, - {"action", a.d_action->toString()}, - }; - responseRules.push_back(rule); - } - - Json::array cacheHitResponseRules; - num=0; - auto localCacheHitResponseRules = g_cachehitresprulactions.getCopy(); - for(const auto& a : localCacheHitResponseRules) { - Json::object rule{ - {"id", num++}, - {"uuid", boost::uuids::to_string(a.d_id)}, - {"matches", (double)a.d_rule->d_matches}, - {"rule", a.d_rule->toString()}, - {"action", a.d_action->toString()}, - }; - cacheHitResponseRules.push_back(rule); - } + auto responseRules = someResponseRulesToJson(&g_resprulactions); + auto cacheHitResponseRules = someResponseRulesToJson(&g_cachehitresprulactions); string acl; diff --git a/regression-tests.dnsdist/test_API.py b/regression-tests.dnsdist/test_API.py index 3d1e802b3..942dfbed1 100644 --- a/regression-tests.dnsdist/test_API.py +++ b/regression-tests.dnsdist/test_API.py @@ -78,7 +78,8 @@ class TestAPIBasics(DNSDistTest): self.assertEquals(content['daemon_type'], 'dnsdist') - for key in ['version', 'acl', 'local', 'rules', 'response-rules', 'cache-hit-response-rules', 'servers', 'frontends', 'pools']: + rule_groups = ['response-rules', 'cache-hit-response-rules'] + for key in ['version', 'acl', 'local', 'rules', 'servers', 'frontends', 'pools'] + rule_groups: self.assertIn(key, content) for rule in content['rules']: @@ -87,17 +88,12 @@ class TestAPIBasics(DNSDistTest): for key in ['id', 'matches']: self.assertTrue(rule[key] >= 0) - for rule in content['response-rules']: - for key in ['id', 'matches', 'rule', 'action', 'uuid']: - self.assertIn(key, rule) - for key in ['id', 'matches']: - self.assertTrue(rule[key] >= 0) - - for rule in content['cache-hit-response-rules']: - for key in ['id', 'matches', 'rule', 'action']: - self.assertIn(key, rule) - for key in ['id', 'matches']: - self.assertTrue(rule[key] >= 0) + for rule_group in rule_groups: + for rule in content[rule_group]: + for key in ['id', 'matches', 'rule', 'action', 'uuid']: + self.assertIn(key, rule) + for key in ['id', 'matches']: + self.assertTrue(rule[key] >= 0) for server in content['servers']: for key in ['id', 'latency', 'name', 'weight', 'outstanding', 'qpsLimit', -- 2.40.0