From: Remi Gacogne Date: Thu, 22 May 2025 14:34:58 +0000 (+0200) Subject: dnsdist: Fix the behaviour of `TagRule` with an empty string as value X-Git-Tag: dnsdist-2.0.0-alpha2~1^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=08ce46dbc75cc1470cbac4694bde81f1470f00e5;p=thirdparty%2Fpdns.git dnsdist: Fix the behaviour of `TagRule` with an empty string as value Before 2.0.0 passing an empty value to `TagRule` meant that only the presence of the tag with an empty value would be accepted. That behaviour changed when we introduced the YAML configuration format, refactoring the selectors in the process, to instead mean that an empty string meant that all values would be accepted, only the tag presence being checked. This is an unfortunate side-effect of a limitation of the `cxx` Rust <-> C++ interoperability layer that we are using (lack of support for `Option` <-> `std::optional`, namely). This PR fixes restores the exising behaviour by adding an additional boolean to the YAML configuration, and ensuring that passing an empty value to the Lua configuration is not treated as not passing any value. --- diff --git a/pdns/dnsdistdist/dnsdist-lua-rules.cc b/pdns/dnsdistdist/dnsdist-lua-rules.cc index e4367c3fbf..2ba7069652 100644 --- a/pdns/dnsdistdist/dnsdist-lua-rules.cc +++ b/pdns/dnsdistdist/dnsdist-lua-rules.cc @@ -625,6 +625,10 @@ void setupLuaRules(LuaContext& luaCtx) return std::shared_ptr(new QNameSetRule(names)); }); + luaCtx.writeFunction("TagRule", [](std::string tag, boost::optional value) { + return std::shared_ptr(dnsdist::selectors::getTagSelector(tag, boostToStandardOptional(value), value ? false : true)); + }); + #if defined(HAVE_LMDB) || defined(HAVE_CDB) luaCtx.writeFunction("KeyValueStoreLookupRule", [](std::shared_ptr& kvs, std::shared_ptr& lookupKey) { return std::shared_ptr(new KeyValueStoreLookupRule(kvs, lookupKey)); diff --git a/pdns/dnsdistdist/dnsdist-lua-selectors-generated.cc b/pdns/dnsdistdist/dnsdist-lua-selectors-generated.cc index ee31aa13fc..74ae1ba44b 100644 --- a/pdns/dnsdistdist/dnsdist-lua-selectors-generated.cc +++ b/pdns/dnsdistdist/dnsdist-lua-selectors-generated.cc @@ -80,9 +80,6 @@ luaCtx.writeFunction("RegexRule", [](std::string expression) { luaCtx.writeFunction("SNIRule", [](std::string server_name) { return std::shared_ptr(dnsdist::selectors::getSNISelector(server_name)); }); -luaCtx.writeFunction("TagRule", [](std::string tag, boost::optional value) { - return std::shared_ptr(dnsdist::selectors::getTagSelector(tag, boostToStandardOptional(value))); -}); luaCtx.writeFunction("TCPRule", [](bool tcp) { return std::shared_ptr(dnsdist::selectors::getTCPSelector(tcp)); }); diff --git a/pdns/dnsdistdist/dnsdist-rules-factory.hh b/pdns/dnsdistdist/dnsdist-rules-factory.hh index ff2cdcbb6e..c42eb0ed5c 100644 --- a/pdns/dnsdistdist/dnsdist-rules-factory.hh +++ b/pdns/dnsdistdist/dnsdist-rules-factory.hh @@ -1134,8 +1134,8 @@ private: class TagRule : public DNSRule { public: - TagRule(const std::string& tag, boost::optional value) : - d_value(std::move(value)), d_tag(tag) + TagRule(const std::string& tag, boost::optional value, bool emptyAsWildcard) : + d_value(std::move(value)), d_tag(tag), d_emptyAsWildcard(emptyAsWildcard) { } bool matches(const DNSQuestion* dq) const override @@ -1149,7 +1149,7 @@ public: return false; } - if (!d_value || d_value->empty()) { + if (!d_value || (d_emptyAsWildcard && d_value->empty())) { return true; } @@ -1158,12 +1158,13 @@ public: string toString() const override { - return "tag '" + d_tag + "' is set" + (d_value ? (" to '" + *d_value + "'") : ""); + return "tag '" + d_tag + "' is set" + ((d_value && (!d_value->empty() || !d_emptyAsWildcard)) ? (" to '" + *d_value + "'") : ""); } private: boost::optional d_value; std::string d_tag; + bool d_emptyAsWildcard; }; class PoolAvailableRule : public DNSRule diff --git a/pdns/dnsdistdist/dnsdist-rules-generator.py b/pdns/dnsdistdist/dnsdist-rules-generator.py index 7a92c0f983..de32dca838 100644 --- a/pdns/dnsdistdist/dnsdist-rules-generator.py +++ b/pdns/dnsdistdist/dnsdist-rules-generator.py @@ -28,6 +28,7 @@ # special unquoted true value which means to use the default value for the # object type, which needs to exist. # Items can optionally have the following properties: +# - 'skip-lua' means that the corresponding Lua bindings will not be generated, which is useful for objects taking parameters that cannot be directly mapped # - 'skip-cpp' means that the corresponding C++ factory and Lua bindings will not be generated, which is useful for objects taking parameters that cannot be directly mapped # - 'skip-rust' is not used by this script but is used by the dnsdist-settings-generator.py one, where it means that the C++ code to create the Rust-side version of an action or selector will not generated # - 'skip-serde' is not used by this script but is used by the dnsdist-settings-generator.py one, where it means that the Rust structure representing that action or selector in the YAML setting will not be directly created by Serde. It is used for selectors that reference another selector themselves, or actions referencing another action. @@ -192,6 +193,8 @@ def generate_lua_actions_bindings(definitions, response=False): for action in definitions: if 'skip-cpp' in action and action['skip-cpp']: continue + if 'skip-lua' in action and action['skip-lua']: + continue name = get_cpp_object_name(action['name']) output = f'luaCtx.writeFunction("{name}{suffix}", [](' if 'parameters' in action: @@ -250,6 +253,8 @@ def generate_lua_selectors_bindings(definitions): for selector in definitions: if 'skip-cpp' in selector and selector['skip-cpp']: continue + if 'skip-lua' in action and action['skip-lua']: + continue name = get_cpp_object_name(selector['name']) output = f'luaCtx.writeFunction("{name}Rule", [](' if 'parameters' in selector: diff --git a/pdns/dnsdistdist/dnsdist-rust-bridge-selectors-generated.cc b/pdns/dnsdistdist/dnsdist-rust-bridge-selectors-generated.cc index fad3bc9952..1ff8aa5add 100644 --- a/pdns/dnsdistdist/dnsdist-rust-bridge-selectors-generated.cc +++ b/pdns/dnsdistdist/dnsdist-rust-bridge-selectors-generated.cc @@ -146,7 +146,7 @@ std::shared_ptr getSNISelector(const SNISelectorConfiguration& conf } std::shared_ptr getTagSelector(const TagSelectorConfiguration& config) { - auto selector = dnsdist::selectors::getTagSelector(std::string(config.tag), std::string(config.value)); + auto selector = dnsdist::selectors::getTagSelector(std::string(config.tag), std::string(config.value), config.empty_as_wildcard); return newDNSSelector(std::move(selector), config.name); } std::shared_ptr getTCPSelector(const TCPSelectorConfiguration& config) diff --git a/pdns/dnsdistdist/dnsdist-rust-lib/rust/src/lib.rs b/pdns/dnsdistdist/dnsdist-rust-lib/rust/src/lib.rs index fa95875d30..29131313b7 100644 --- a/pdns/dnsdistdist/dnsdist-rust-lib/rust/src/lib.rs +++ b/pdns/dnsdistdist/dnsdist-rust-lib/rust/src/lib.rs @@ -1102,6 +1102,8 @@ mod dnsdistsettings { tag: String, #[serde(default, skip_serializing_if = "crate::is_default")] value: String, + #[serde(rename = "empty-as-wildcard", default = "crate::Bool::::value", skip_serializing_if = "crate::if_true")] + empty_as_wildcard: bool, } #[derive(Deserialize, Serialize, Debug, PartialEq)] diff --git a/pdns/dnsdistdist/dnsdist-selectors-definitions.yml b/pdns/dnsdistdist/dnsdist-selectors-definitions.yml index c2cdee2163..215912fd4c 100644 --- a/pdns/dnsdistdist/dnsdist-selectors-definitions.yml +++ b/pdns/dnsdistdist/dnsdist-selectors-definitions.yml @@ -391,6 +391,7 @@ Set the ``source`` parameter to ``false`` to match against destination address i description: "The exact Server Name Indication value" - name: "Tag" description: "Matches question or answer with a tag named ``tag`` set. If ``value`` is specified, the existing tag value should match too" + skip-lua: true parameters: - name: "tag" type: "String" @@ -399,6 +400,10 @@ Set the ``source`` parameter to ``false`` to match against destination address i type: "String" default: "" description: "If set, the value the tag has to be set to" + - name: "empty-as-wildcard" + type: "bool" + default: "true" + description: "Because of a limitation in our Rust <-> C++ interoperability layer, ``value`` defaults to an empty string, which makes it impossible to express whether an empty ``value`` means that we should match on all values (so as long as the tag has been set) or only if the value is actually empty. This flag fixes that: if ``value`` is empty and this parameter is set to ``false`` the selector will only match if the actual value of the tag is empty, while if it set to ``true`` (default) it will match as long as the tag is set, regardless of the value" - name: "TCP" description: "Matches question received over TCP if ``tcp`` is true, over UDP otherwise" parameters: diff --git a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.cc b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.cc index d9f7e15a13..4c41b72390 100644 --- a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.cc +++ b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.cc @@ -107,9 +107,9 @@ std::shared_ptr getSNISelector(const std::string& server_name) { return std::make_shared(server_name); } -std::shared_ptr getTagSelector(const std::string& tag, std::optional value) +std::shared_ptr getTagSelector(const std::string& tag, std::optional value, std::optional emptyAsWildcard) { - return std::make_shared(tag, value ? *value : ""); + return std::make_shared(tag, value ? *value : "", emptyAsWildcard ? *emptyAsWildcard : true); } std::shared_ptr getTCPSelector(bool tcp) { diff --git a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh index 625358094e..f4dda50ab0 100644 --- a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh +++ b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh @@ -26,6 +26,6 @@ std::shared_ptr getRecordsCountSelector(uint8_t section, uint1 std::shared_ptr getRecordsTypeCountSelector(uint8_t section, uint16_t record_type, uint16_t minimum, uint16_t maximum); std::shared_ptr getRegexSelector(const std::string& expression); std::shared_ptr getSNISelector(const std::string& server_name); -std::shared_ptr getTagSelector(const std::string& tag, std::optional value); +std::shared_ptr getTagSelector(const std::string& tag, std::optional value, std::optional emptyAsWildcard); std::shared_ptr getTCPSelector(bool tcp); std::shared_ptr getTrailingDataSelector();