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<T>` <-> `std::optional<T>`,
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 e4367c3fbfe7ffca7d51d1cbc718ea8c8171a917..2ba7069652af3ea74d021b8b06222002da40c3dc 100644 (file)
--- 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<DNSRule>(new QNameSetRule(names));
   });
 
+  luaCtx.writeFunction("TagRule", [](std::string tag, boost::optional<std::string> value) {
+    return std::shared_ptr<DNSRule>(dnsdist::selectors::getTagSelector(tag, boostToStandardOptional(value), value ? false : true));
+  });
+
 #if defined(HAVE_LMDB) || defined(HAVE_CDB)
   luaCtx.writeFunction("KeyValueStoreLookupRule", [](std::shared_ptr<KeyValueStore>& kvs, std::shared_ptr<KeyValueLookupKey>& lookupKey) {
     return std::shared_ptr<DNSRule>(new KeyValueStoreLookupRule(kvs, lookupKey));

diff --git a/pdns/dnsdistdist/dnsdist-lua-selectors-generated.cc b/pdns/dnsdistdist/dnsdist-lua-selectors-generated.cc
index ee31aa13fc0bd79844999001997cea0dd6d4d501..74ae1ba44b5218105fe8c3cde0a28ab008209a27 100644 (file)
--- 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<DNSRule>(dnsdist::selectors::getSNISelector(server_name));
 });
-luaCtx.writeFunction("TagRule", [](std::string tag, boost::optional<std::string> value) {
-  return std::shared_ptr<DNSRule>(dnsdist::selectors::getTagSelector(tag, boostToStandardOptional(value)));
-});
 luaCtx.writeFunction("TCPRule", [](bool tcp) {
   return std::shared_ptr<DNSRule>(dnsdist::selectors::getTCPSelector(tcp));
 });

diff --git a/pdns/dnsdistdist/dnsdist-rules-factory.hh b/pdns/dnsdistdist/dnsdist-rules-factory.hh
index ff2cdcbb6e9997c69839094b7e2a4744ff2cccc1..c42eb0ed5c6e3baef5ef694263d19d6b740214b7 100644 (file)
--- 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<std::string> value) :
-    d_value(std::move(value)), d_tag(tag)
+  TagRule(const std::string& tag, boost::optional<std::string> 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<std::string> 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 7a92c0f983647703e5fad3de10d38672ad3c1042..de32dca8384552448f419699b9f7cf66eed00ba1 100644 (file)
--- 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 fad3bc99521716184f0430bdd3426b1a4e07a658..1ff8aa5addb8b9dcadd4a29c522845f1784c331a 100644 (file)
--- 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<DNSSelector> getSNISelector(const SNISelectorConfiguration& conf
 }
 std::shared_ptr<DNSSelector> 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<DNSSelector> 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 fa95875d309adf5378b482abdb0b2afc37381aaf..29131313b707f5129cfd99164cc15f649fdf9ae2 100644 (file)
--- 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::<true>::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 c2cdee2163bf01e1461e69c0f8032e6bb44795b5..215912fd4c28e9defe7fc1d54ee3c53990516587 100644 (file)
--- 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 d9f7e15a1342d7e10e6f8fd2b7bd5467f5ae43a3..4c41b72390bddb1bdedf9dca405739238045402f 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.cc
+++ b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.cc
@@ -107,9 +107,9 @@ std::shared_ptr<SNIRule> getSNISelector(const std::string& server_name)
 {
   return std::make_shared<SNIRule>(server_name);
 }
-std::shared_ptr<TagRule> getTagSelector(const std::string& tag, std::optional<std::string> value)
+std::shared_ptr<TagRule> getTagSelector(const std::string& tag, std::optional<std::string> value, std::optional<bool> emptyAsWildcard)
 {
-  return std::make_shared<TagRule>(tag, value ? *value : "");
+  return std::make_shared<TagRule>(tag, value ? *value : "", emptyAsWildcard ? *emptyAsWildcard : true);
 }
 std::shared_ptr<TCPRule> getTCPSelector(bool tcp)
 {

diff --git a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh
index 625358094ed0b6f1cb0401dc20b4195008b4ddf2..f4dda50ab0790b1859a70f6a01402f161e2a61a6 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh
+++ b/pdns/dnsdistdist/dnsdist-selectors-factory-generated.hh
@@ -26,6 +26,6 @@ std::shared_ptr<RecordsCountRule> getRecordsCountSelector(uint8_t section, uint1
 std::shared_ptr<RecordsTypeCountRule> getRecordsTypeCountSelector(uint8_t section, uint16_t record_type, uint16_t minimum, uint16_t maximum);
 std::shared_ptr<RegexRule> getRegexSelector(const std::string& expression);
 std::shared_ptr<SNIRule> getSNISelector(const std::string& server_name);
-std::shared_ptr<TagRule> getTagSelector(const std::string& tag, std::optional<std::string> value);
+std::shared_ptr<TagRule> getTagSelector(const std::string& tag, std::optional<std::string> value, std::optional<bool> emptyAsWildcard);
 std::shared_ptr<TCPRule> getTCPSelector(bool tcp);
 std::shared_ptr<TrailingDataRule> getTrailingDataSelector();