From 9cbef3b20279ad46bb93ceee589a73fb2d2cf360 Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Mon, 13 Jan 2025 16:51:38 -0700 Subject: [PATCH] fix deserialize error: unknown field `relay_from_authz` issue The changes in e4b743cb8f3207ac4918271b450d73514f4be6be to improve memoize performance could cause the error message above because the __newindex method wasn't actually writing the new value to the unshared table, it was returning the existing value! Most of this commit is really plumbing to support calling through the cache layers in the same way that the listener would use, but in the context of the unit tests in the lua file. A nice side effect of this plumbing is that it is now possible to iterate (via pairs) and index fields of the main config objects that are returned by the most common `kumo.make_xxx` functions. --- assets/policy-extras/listener_domains.lua | 91 +++++++++++++++++++---- crates/config/src/lib.rs | 54 +++++++++++++- crates/kumo-api-types/src/egress_path.rs | 6 +- crates/kumod/src/queue.rs | 7 +- crates/kumod/src/smtp_server.rs | 6 +- crates/mod-memoize/src/lib.rs | 35 +++++---- 6 files changed, 164 insertions(+), 35 deletions(-) diff --git a/assets/policy-extras/listener_domains.lua b/assets/policy-extras/listener_domains.lua index 1b7b95fd..37325851 100644 --- a/assets/policy-extras/listener_domains.lua +++ b/assets/policy-extras/listener_domains.lua @@ -119,7 +119,7 @@ local function lookup_impl( -- Add the peer to the relay_from list local peer_ip, _peer_port = utils.split_ip_port(conn_meta:get_meta 'received_from') - table.insert(listener_domain.relay_from, peer_ip) + listener_domain.relay_from = { peer_ip } end if skip_make then @@ -285,6 +285,29 @@ kumo.on('validate_config', function() end) function mod:test() + local cached_load_data = kumo.memoize(load_data, { + name = 'test_listener_domains_data', + ttl = '5 minutes', + capacity = 10, + }) + + local function get_listener_domain( + data, + domain_name, + listener, + conn_meta, + skip_make + ) + local by_listener = cached_load_data { data } + return get_listener_domain_impl( + by_listener, + domain_name, + listener, + conn_meta, + skip_make + ) + end + local open_relay = [=[ ['*'] relay_to = true @@ -297,42 +320,80 @@ relay_from = ['10.0.0.0/24'] [listener."127.0.0.1:25"."*.example.com"] log_oob = false +['elsewhere.com'] +relay_to = false +relay_from_authz = ["john"] + ]=] - local data = parse_toml_data(open_relay) - local skip_make = true + local data = kumo.serde.toml_parse(open_relay) + + -- Fake up something that quacks like the connection metadata object. + -- This is really just a matter of adding a metatable with a get_meta + -- method to it. + local function make_conn_meta(obj) + local methods = {} + function methods:get_meta(key) + return rawget(self, key) + end + local mt = { + __index = methods, + } + setmetatable(obj, mt) + return obj + end utils.assert_eq( - get_listener_domain_impl( + get_listener_domain( data, 'example.com', '127.0.0.1:25', - {}, - skip_make + make_conn_meta {} ), - { relay_to = true } + kumo.make_listener_domain { relay_to = true } ) utils.assert_eq( - get_listener_domain_impl( + get_listener_domain( data, 'woof.example.com', '127.0.0.1:25', - {}, - skip_make + make_conn_meta {} ), - { log_oob = false } + kumo.make_listener_domain { log_oob = false, relay_to = true } ) utils.assert_eq( - get_listener_domain_impl( + get_listener_domain( data, 'somewhere.com', '127.0.0.1:25', - { received_from = '10.0.0.1' }, - skip_make + make_conn_meta { received_from = '10.0.0.1' } ), - { relay_from = { '10.0.0.0/24' }, relay_to = false } + kumo.make_listener_domain { + relay_from = { '10.0.0.0/24' }, + relay_to = false, + } + ) + + utils.assert_eq( + get_listener_domain( + data, + 'elsewhere.com', + '10.0.0.1:25', + make_conn_meta {} + ), + kumo.make_listener_domain { relay_to = false } + ) + + utils.assert_eq( + get_listener_domain( + data, + 'elsewhere.com', + '10.0.0.1:25', + make_conn_meta { authz_id = 'john', received_from = '10.0.0.1:25' } + ), + kumo.make_listener_domain { relay_from = { '10.0.0.1' } } ) local status, err = pcall( diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index ed3fa475..1a061c1d 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -1,7 +1,10 @@ use crate::pool::{pool_get, pool_put}; pub use crate::pool::{set_gc_on_put, set_max_age, set_max_spare, set_max_use}; use anyhow::Context; -use mlua::{FromLua, FromLuaMulti, IntoLuaMulti, Lua, LuaSerdeExt, RegistryKey, Table, Value}; +use mlua::{ + FromLua, FromLuaMulti, IntoLua, IntoLuaMulti, Lua, LuaSerdeExt, MetaMethod, RegistryKey, Table, + UserData, UserDataMethods, Value, +}; use parking_lot::FairMutex as Mutex; use prometheus::{CounterVec, HistogramTimer, HistogramVec}; use serde::Serialize; @@ -465,6 +468,47 @@ pub fn any_err(err: E) -> mlua::Error { mlua::Error::external(format!("{err:#}")) } +/// Provides implementations of __pairs and __index metamethods +/// for a type that is Serialize and UserData. +/// Neither implementation is considered to be ideal, as we must +/// first serialize the value into a json Value which is then either +/// iterated over, or indexed to produce the appropriate result for +/// the metamethod. +pub fn impl_pairs_and_index(methods: &mut M) +where + T: UserData + Serialize, + M: UserDataMethods, +{ + methods.add_meta_method(MetaMethod::Pairs, move |lua, this, _: ()| { + let Ok(serde_json::Value::Object(map)) = serde_json::to_value(this).map_err(any_err) else { + return Err(mlua::Error::external("must serialize to Map")); + }; + + let mut value_iter = map.into_iter(); + + let iter_func = lua.create_function_mut( + move |lua, (_state, _control): (Value, Value)| match value_iter.next() { + Some((key, value)) => { + let key = lua.to_value(&key)?; + let value = lua.to_value(&value)?; + Ok((key, value)) + } + None => Ok((Value::Nil, Value::Nil)), + }, + )?; + + Ok((Value::Function(iter_func), Value::Nil, Value::Nil)) + }); + + methods.add_meta_method(MetaMethod::Index, move |lua, this, field: Value| { + let value = lua.to_value(this)?; + match value { + Value::Table(t) => t.get(field), + _ => Ok(Value::Nil), + } + }); +} + /// This function will try to obtain a native lua representation /// of the provided value. It does this by attempting to iterate /// the pairs of any userdata it finds as either the value itself @@ -476,7 +520,13 @@ pub fn materialize_to_lua_value(lua: &Lua, value: mlua::Value) -> mlua::Result { let mt = ud.metatable()?; - let pairs: mlua::Function = mt.get("__pairs")?; + let Ok(pairs) = mt.get::("__pairs") else { + let value = ud.into_lua(lua)?; + return Err(mlua::Error::external(format!( + "cannot materialize_to_lua_value {value:?} \ + because it has no __pairs metamethod" + ))); + }; let tbl = lua.create_table()?; let (iter_func, state, mut control): (mlua::Function, mlua::Value, mlua::Value) = pairs.call(mlua::Value::UserData(ud.clone()))?; diff --git a/crates/kumo-api-types/src/egress_path.rs b/crates/kumo-api-types/src/egress_path.rs index ebcfe511..367da5e9 100644 --- a/crates/kumo-api-types/src/egress_path.rs +++ b/crates/kumo-api-types/src/egress_path.rs @@ -257,7 +257,11 @@ pub struct EgressPathConfig { } #[cfg(feature = "lua")] -impl LuaUserData for EgressPathConfig {} +impl LuaUserData for EgressPathConfig { + fn add_methods>(methods: &mut M) { + config::impl_pairs_and_index(methods); + } +} impl Default for EgressPathConfig { fn default() -> Self { diff --git a/crates/kumod/src/queue.rs b/crates/kumod/src/queue.rs index 67d3b17b..85d4d7f9 100644 --- a/crates/kumod/src/queue.rs +++ b/crates/kumod/src/queue.rs @@ -29,6 +29,7 @@ use kumo_template::{context, TemplateEngine}; use message::message::{QueueNameComponents, WeakMessage}; use message::Message; use mlua::prelude::*; +use mlua::UserDataMethods; use parking_lot::FairMutex as StdMutex; use prometheus::{Histogram, IntCounter, IntGauge}; use rfc5321::{EnhancedStatusCode, Response}; @@ -446,7 +447,11 @@ pub struct QueueConfig { pub provider_name: Option, } -impl LuaUserData for QueueConfig {} +impl LuaUserData for QueueConfig { + fn add_methods>(methods: &mut M) { + config::impl_pairs_and_index(methods); + } +} impl Default for QueueConfig { fn default() -> Self { diff --git a/crates/kumod/src/smtp_server.rs b/crates/kumod/src/smtp_server.rs index 7edf67b9..b289a4f3 100644 --- a/crates/kumod/src/smtp_server.rs +++ b/crates/kumod/src/smtp_server.rs @@ -108,7 +108,11 @@ pub struct EsmtpDomain { pub ttl: Duration, } -impl LuaUserData for EsmtpDomain {} +impl LuaUserData for EsmtpDomain { + fn add_methods>(methods: &mut M) { + config::impl_pairs_and_index(methods); + } +} fn default_ttl() -> Duration { Duration::from_secs(60) diff --git a/crates/mod-memoize/src/lib.rs b/crates/mod-memoize/src/lib.rs index 831974ab..1806b328 100644 --- a/crates/mod-memoize/src/lib.rs +++ b/crates/mod-memoize/src/lib.rs @@ -175,12 +175,14 @@ impl MemoizedTable { } /// Transform Shared -> Mut - fn unshare(&mut self) { + fn unshare(&mut self) -> &mut HashMap { + if let Self::Shared(t) = self { + *self = Self::Mut(t.iter().map(|(k, v)| (k.clone(), v.clone())).collect()); + } + match self { - Self::Shared(t) => { - *self = Self::Mut(t.iter().map(|(k, v)| (k.clone(), v.clone())).collect()); - } - Self::Mut(_) => {} + Self::Shared(_) => unreachable!(), + Self::Mut(map) => map, } } } @@ -199,16 +201,19 @@ impl UserData for MemoizedTable { }); // NewIndex allows writing fields of the table - methods.add_meta_method_mut(MetaMethod::NewIndex, move |lua, this, key: mlua::Value| { - this.unshare(); - match MapKey::from_lua(key) { - Some(key) => match this.table().get(&key) { - Some(value) => value.as_lua(lua), - None => Ok(mlua::Value::Nil), - }, - None => Ok(mlua::Value::Nil), - } - }); + methods.add_meta_method_mut( + MetaMethod::NewIndex, + move |lua, this, (key, value): (mlua::Value, mlua::Value)| match MapKey::from_lua(key) { + Some(key) => { + let value = CacheValue::from_lua(value, lua)?; + this.unshare().insert(key, value); + Ok(()) + } + None => Err(mlua::Error::external( + "invalid key type while trying to call __newindex and assign a value", + )), + }, + ); // Pairs iterates the keys of the table. // We use add_meta_function rather than add_meta_method here