The goal is to treat peer-initiated-unilateral-disconnects as being
somewhat equivalent to the way that we would idle out the connection if
we had no messages for it, on the supposition that the most likely
cause of a disconnect in between command verbs that we send is that
the peer decided that we were idle too long. That isn't the only
possible reason, but it is the motivating example.
For the idled-out case we should close the current session and
have our message(s) go out on a separate session. We don't want
to try the next host in the connection plan in case we're talking
to some kind of honey pit configuration where only the first
MX in the plan is valid, and even talking to a secondary can
harm your ability to send to the first in the future.
Handling this is a little tricky because we need to take care of
the distinction between getting a unilateral before or after
we've attempted to send a message.
In the before-case we don't want to blindly assume that we should start
a new session because that would mean that a persistent issue on the
first MX would have us spinning our wheels trying new connections only
to the first MX over and over.
This commit adds a couple of integration tests that contrive situations
where we exercise some pertinent cases.
refs: https://github.com/KumoCorp/kumomta/pull/482
This function sets up the accounting log which records authentication
and authorization events to a local log file.
Add some test assertions that we're not seeing any failed authn/authz
in a number of integration tests.
Add explicit check for failed a request to kcli's streaming metrics
parser, which previously would ignore the request status.
When using an explicit mx_list, rather than consuming the DNS-provided
MX records, we were populating the connection plan LIFO with the mx_list
which is specified as a FIFO.
The result was that we would connect to the addresses in the reverse of
the intended order.
Most users only populate a single entry for mx_list, if they use this at
all, so it has flown under the radar. This issue was introduced
together with the mx_list feature in bcd2946c53.
This commit corrects that issue so that the behavior conforms to the
intended and documented ordering, and tweaks the tests to accommodate
it; the test changes required were:
1. disconnect_in_data was actually sensitive to the order, and encoded
the reverse ordering in its expectations. :-/
It now correctly encodes the correct order, which should allow
detection of regression in the future.
2. disconnect_terminate_ok is sensitive to extraneous TransientFailures.
It was implicitly dependent on the ordering of the mx_list in the
sense that it relied on the FIFO list returning as [127.0.0.1, ::1],
which resulted in trying ::1 first (because that list is consumed as
LIFO), failing due to ::1 not actually being bound in the test
environment, and proceeding to test with 127.0.0.1 being the last
working address.
With the ordering fixed that test started to fail because the second
host in the correct connection plan was ::1 and that was not
reachable, triggering a TransientFailure against the expectations
of the test.
The goal of the test was to verify the behavior in the
exhausted-connection-plan case, rather than the
unreachable-final-mx-case, so the resolution here is to add v6
addresses to the skip_hosts config. Will that come back to haunt us?
Time will tell.
This commit tidies up a bit of an API wart that stems from
the initial implementation where we couldn't guarantee that
the message crate would know how to get a spool handle to
implicitly load data when needed.
This has resulted in a couple of issues over time where we try to
operate on the message data and it just hasn't been loaded, leading to
transient errors being raised in the best case, but in a possible worst
case, a silent error condition.
During the rebind_message event, if the message is not loaded,
msg:get_data() could return the placeholder empty data value. If the
rebind_message event is trying to mutate the message then the best case
is that an error is raised, a medium-bad case is that operations like
`msg:append_header` will fail to find the header block and thus fail,
and the worst case is that mutation operations that blinding prepend
data (perhaps msg:prepend_header) could end up assigning a bogus
truncated message. While we don't generally recommend this sort of
action, this is potentially a very gnarly consequence of trying things
out.
Let's just head all of these sorts of issues off by making the accessors
async and have them internally load the data as required.
This commit implements a kumomta-specific message transfer
protocol that is intended to be used to migrate messages
from one kumomta node to another.
The transfer is carried out using an HTTP POST request
to the destination node's http listener.
The request includes the full message metadata and body,
in a compressed form.
An xfer request can be made via `kcli xfer` (and thus also via an HTTP API
endpoint). It works similarly to a rebind operation; you specify the
criteria to be used to match scheduled queues, along with the target
node for the xfer, and kumomta will find matching queues, drain out the
messages, make an adjustment to the metadata to capture current
scheduling information, and then place the messages into an xfer queue.
The xfer queue has hard-coded scheduling queue configuration of its own,
with the base retry interval set to 10 seconds, which should be suitably
aggressive for the intended use case.
You may apply shaping to affect the number of concurrent requests in a
similar way to how TSA shaping is configured.
On the receiving side, the incoming xfer sanity check to prohibit
trying to xfer to itself.
The spool id of the Message is not suitable to be reused verbatim on
another node (spool ids include the local mac address and creation
timestamp information, as well as a random component), so the receiving
side will derive an id that should be suitable for use on that node.
The originating node id and spool id will be preserved in metadata to
aid in tracing.
It is possible for an xfer request to target an existing xfer queue, so
that you can correct/update the target in various circumstances. In that
situation the messages will be "simply" moved from the source queue to
the destination queue.
It is possible to cancel an xfer request via `kcli xfer-cancel` (and
thus also via an HTTP API endpoint). You specify the target queue,
which must be an xfer queue, and it will have its messages drained and
the metadata changes that were applied when the xfer was initiated will
be reversed, allowing the messages to then be reinserted into their
originating queue.
refs: https://github.com/KumoCorp/kumomta/issues/311
This makes a first pass over relaying a batch, with a test
to prove that it worked.
The logic in the smtp client likely needs to be enhanced
to handle hitting recipient-per-transaction limits; tests
for various scenarios need to be added in follow-on commits.
Hooks up the parsed XCLIENT command to the smtp server.
We support just the address changing attributes; we don't
do anything special with the NAME attribute, and I'd like
to think a lot more about authentication before allowing
messing with the LOGIN attribute.
After applying changes to the metadata in the connection,
take care to re-evaluate the listener parameters and
dynamic parameters so that `via` and `peer` blocks can
match the new configuration.
The motivating use case here is to allow using a rebind command to
specify `smarthost:port`. You can achieve that via an invocation
like:
```
kcli rebind --everything --set queue=smarthost:port
```
or:
```
kcli rebind --everything --set routing_domain=smarthost:port
```
refs: https://github.com/KumoCorp/kumomta/issues/352
Some sites are non-conforming in their behavior when they have a policy
that issues a transient failure and then snips the connection without
explicitly issuing a 421.
This can cause "splash damage" where a message is transiently failed
and the connection snipped "disgracefully" and a subsequent message
that is due to go out on that same established connection encounters
a transport error when the RSET part of the pipeline is sent.
This commit provides methods on the ClientError and Response types
to help reason about whether the response might be specifically
about the message being delivered, vs. more of a transport error.
The gist of this check is that timeouts and transport level stuff
where there is no protocol level response are not considered to
be due to the message. Protocol level responses prior to MAIL FROM
are similarly considered to be more like transport errors.
The functional change in this commit is:
* If we get a transport-ish error and we have more candidate hosts
available in the connection plan, then after logging a
TransientFailure for the error we just encountered, we'll push
back the message to be tried again on the next connection that
we open in the current session.
* For timeout errors specifically, this behavior is gated behind
a try_next_host_on_timeout boolean option.
This commit allows setting a per-message `expires` timestamp
via msg:set_scheduling (and thus msg:import_scheduling_header).
The expiration takes precedence over max_age; max_age will be
ignored for messages that have configured and expiration time.
The expiration time is independent of the other scheduling
restrictions.
Add test coverage for source_selection_rate and tidy up how we reflect
being throttled due to source_selection_rate; in the first cut
we made it look like the ready queue was full, but now we're a
bit more deliberate:
* If any of the candidate queues were full, we report that the
ready queue was full so that the message can be retried again
"soon".
* If we hit a source_selection_rate throttle, we use the throttle
delay to adjust the min delay period and if we run out of candidate
sources, we'll have the same sort of return value as we would
for a suspension with a definite duration; the message will
log that transient failure in the same way with the same sort
of retry schedule. I've revised the wording in that case to be
"no sources for SITE pool=`POOL` are eligible for selection at this time"
(the important part being `at this time`) to suggest at the
transient nature of the unavailability.
* If we exhaust all sources for other reasons, we'll treat this
as the NoSources case. In practice, this should mean that the
source has zero weight. We'll now report "no sources available for SITE
pool=`POOL`" in this case instead of the more precise but wordy
"no non-zero-weighted sources available for SITE" that we used
prior to this commit.
This adds a way for the lower level dispatcher impl to communicate
back up to the higher level that the session is done and can be
closed out without treating it as a connection failure.
For SMTP the criteria for this is that, if we ever got as far
as MAIL FROM, then we consider that the connection was established
OK and that means that if we subsequently reach a point where we need to
connect again, then it must be due to an earlier policy issue that was
already recorded as a transient failure.
In that circumstance, if we have not more hosts to connect to, we
will indicate that we terminated "OK".
This works because the upper level checks for this state before
asking the session to connect again.
If there are more addresses that could be tried, we will report a
connection failure for the next message picked up by the session;
this commit only changes the behavior for the case where there
are no more addresses left in the current connection plan.
When used together with an Opportunistic TLS mode, if the handshake
or subsequent EHLO fails, we will re-connect to the current host
and disable TLS.
This is implemented as a recursive solution, which I'm not totally
keen on, but the recursion is limited to a single level so it's
not so bad.
This option should be used with caution, and ideally only
for trusted networks.
The purpose is to absorb the latency of post-DATA processing
and hide it from the trust injector.
It defers processing that would normally happen in smtp_server_message_received
and instead will, at some (ideally) near-future time trigger an
smtp_server_message_deferred_inject event instead.
This will marginally increase your average injection latency but should
clamp your worst case injection latency much lower because the outliers
will not happen inline with the injecting client.
Previously, we added `batch_size` which acts as a maximum batch
size; this was the simplest possible way to achieve batching
with a constrained upper bound with no additional latency.
This commit introduces a minimum batch size and a maximum latency.
The behavior for delivery handlers that configure min_batch_size > 1
is that once we've started a batch, we'll allow for up to
max_batch_latency time to pass to accumulate more messages into
the batch before we just go ahead and send it.
If you wanted to batch say 5,000-10,000 messages per batch or whatever
you get in a 10 second time period you would set:
* min_batch_size = 5000
* batch_size = 10000
* max_batch_latency = "10s"
Note that the default idle_timeout in the product is "5s" so if you want
max_batch_latency to be effective you will also need to increase the
idle_timeout in your shaping configuration/egress path config for that
handler.
The reported behavior was that messages seemd to be retried much faster
than the retry schedule, and at a quick glance it looked like the
nxdomain code path didn't respest the backoff, but from hooking
up this test and making the durations longer, it really doesn't seem
like the issue was that simple:
refs: https://github.com/KumoCorp/kumomta/issues/271
This really is adding batching support to custom lua delivery
protocol handlers, but the main use case for these today is
to implement log hooks.
The way that it works is that you can specify a `batch_size`
as part of setting up the lua protocol handler.
Then, when it is time to send messages, if the batch_size is
the default of 1, the lua delivery logic will invoke the `send` method
on the connection object returned from the constructor. This
is the same as the behavior from before this commit.
However, if the batch_size is greater than 1, then the lua delivery
logic will instead attempt to collect up to batch_size messages
that are immediately available from the ready queue, and then pass
those to a new `send_batch` method.
The send_batch method accepts an array of messages; that array will
always have at least one message, and up to batch_size messages,
depending on the throughput and queue size.
If the send_batch method's return value applies equally to all
messages in the batch, so if it indicates that something failed,
that disposition will apply to all messages.
One of the reasons that I'd avoided implementing batching thus far
was that it makes it awkward to resolve persistent/recurring issues
that are due to a single message in that batch. If the batch is
always retried together then there is a good chance that it will
always fail together.
There's no explicit mitigation for that issue here, but it may
be probablistically mitigated by the jitter that is applied to
messages that transiently fail. If a batch transiently fails,
each message in that batch will be subject to its own random
jitter which should cause an offending message to be retried
with a different subset of messages next time around.
The integration test included here demonstrates the batching
working with an http log hook implementation.
Previously we were using only our timeq module, which is built on top of
hashed hierarchical timer wheels.
Timer wheels have O(1) insertion and removal which are excellent
properties for larger delayed queues.
However, they do not know how to answer the question "when is the
next item due", but only "what is due in the next tick".
The underlying timer wheel implementation assumes a 1ms granularity
which is a little bit at-odds with our pragmatic view of the scheduled
queue, which is "if it's in there, precision timing isn't important, and
it's generally fine to consider once per minute", requiring that we
either aggressively scheduled a maintainer task to wake up every 1ms per
scheduled queue (untenable!) or have it wake up no more than once per
second but ideally closer to once per minute to then tick however
milliseconds are necessary to advance the wheel to the current slot.
For small numbers of scheduled queues with sufficiently large retry
intervals this hasn't bubbled up as an issue so far, but it bothers
me that it isn't as efficient as it could be because we have to wake up
reasonably frequently to keep things ticking over, and that introduces
higher continual CPU utilization. It's small, but I worry about
the aggregate cost spread over very large numbers of scheduled queues.
What I really want here is a a timer wheel that I can tick with
arbitrary granularity and with that in mind I took a look at adapting
the handful of existing implementations and found that we're already
using the cleanest implementation, and it would take some effort that I
didn't really want to spend right now.
I opted for a reasonably simple alternative option, which is to adopt a
skiplist for the queue. This has O(log n) insertion to maintain ordering
with O(1) removal and can answer "when is the next item due". What this
means is that we pay a slightly higher insertion cost one-time in
exchange for being able to put the maintainer for the queue asleep until
we need it, and not have to keep waking up between times, which should
scale better.
What this means in practice is that we now wake up the maintainer either
when the next message is due, or once per minute to re-evaluate the
queue configuration hook, so we're slightly better off, but totally
where I'd like to be.
I've introduced a reap_interval (default 10 minutes) and a
refresh_interval (default 1 minute) as parameters in get_queue_config so
that you can increase that 1m interval for reloading.
What I'd like to do in a follow up commit is introduce a way to define
the refresh policy. For example, it would be neat to say "watch my
policy directory and refresh when it changes", which would make things
the most efficient for many users. For those that are loading their
config from a remote datasource, we'd need to consider some other
mechanism for this; maybe some kind of long-poll or pubsub, but will
obviously still be able to support the current interval based polling.
Now, with all of that said: I didn't want to switch the product
default over and hope for best, so what I did was add a strategy
option to allow this to be adopted on a per-queue basis.
Since I was in here adding some options, I also added an option
that allows explicitly setting the interval used for timerwheel
ticks, so you now have a lot more opportunities for tuning this
stuff.
We've been trying to run down a weird problem with the lapin
client where the system appears to get bogged down around lapin
and the submission rate goes through the floor. There no obvious
signs of a problem elsewhere in the system or in the logging,
so we're trying out an optional alternative client implementation.
This commit adds a "one-shot" publish function:
```
log_hooks:new {
name = 'amqp',
constructor = function(domain, tenant, campaign)
local sender = {}
local host, port = table.unpack(kumo.string.split(AMQP_HOST_PORT, ':'))
function sender:send(msg)
kumo.amqp.basic_publish {
routing_key = 'woot',
payload = msg:get_data(),
connection = {
host = host,
port = tonumber(port),
},
}
return '250 ok'
end
return sender
end,
}
```
behind the scenes we maintain a connection+channel pool for this.
This first pass doesn't support confirms, to the publish is
fire-and-forget; we assume that if there was no error while trying to
acquire the channel or returned immediately by the underlying
basic_publish function, that the server took the message and we're
done.
I was going to use mobc for the connection pooling here, since I just
implemented it for the recent redis client changes, but it triggered a
non-sensical panic around mobc's internal use of the metrics crate.
Upon further inspection, mobc appears to clobber certain metrics when
multiple Pools are in used, which makes it unsuitable for real world
use. That was the cause of the panic; I couldn't figure out the cause of
the panic. I switched to using the deadpool connection pooling crate
which has a similar API and doesn't panic. I will replace the use of
mobc in mod-redis with deadpool in a later commit.
I've introduced a place to stash the "global" tokio runtime that
is configured by `main` in `kumod` and `tsa-daemon`. That allows
us to bounce onto that runtime, rather than spawning an ad-hoc
single threaded runtime.
Since we're still preserving the existing client, this commit
introduces a new and slightly different integration test to
verify that publishing is operational from here.
Docs still need to be written up for this, but I want to put
this through its paces before doing that.
I had kumomta-dev installed, so requiring the helpers succeeded.
It failed in CI because we need to set the path to find the local
assets dir like we do in TSA.
So do that!
The back story is that we've been trying to run down a situation
where the AMQP connections seemd to get busy, running close to 100%,
and that appears to impact some other processing.
This commit dedicates a thread per AMQP connection to manage its
connection state. This is likely not totally necessary, but it
is desirable to move it off of the current tokio context, which
is one of our lua localset threads, in order to reduce that
contention.
I don't really like this as a solution, it's just giving us
an option to play with while we zero-in on the underlying issue.
As part of this, we now have an explicit timeout around the connect
operation.
This commit also adds a publish_with_timeout method on the amqp
connection, which combines the publish and the wait for confirmation
and puts a timeout around that operation.
I'm expecting CI to not like this, as we're using the testcontainers
crate which spawns docker images to set up isolated server instances.
This is not directly compatible with our current CI environment.
So these tests are only enabled when KUMOD_TESTCONTAINERS=1 is
set in the environment.
The test sets up a rabbit instance, declares a queue, sets up simple log
hook in kumo, injects a message and then waits for it to process
through, then checks to receive a reception and delivery record.
To facilitate this, some adjustments needed to be made to the time
calculation in the maintainer because we previously didn't consider it
to be valid to have a retry_interval below 1 minute, but in order for
this integration test to be viable to run as part of the CI it needs to
run in significantly less time than 1 minute.
The approach taken here is to avoid considering 1 minute as the
baseline, but rather take 1/20th of the retry_interval. In the default
configuration, the numbers work out the same as previously, but they
will scale down as the retry_interval is reduced.
Care is taken to avoid a couple of borderline busy wait scenarios where
we might otherwise have woken up at unrealistically small intervals:
timeq can suggest 1ms in a few scenarios, and we just round those up to
the next second to avoid that.
It's worth noting that we do not consider the scheduled queue to be a
realtime, high granularity queue: anything that lands there is
considered to be bulk/batch and will be handled later: it isn't worth
prioritizing with high granularity because messages that land there are
generally not going to be delivered quickly.
When `log_arf` or `log_oob` are set to true with `relay_to=false`, we
now return a 550 error response for messages that are not ARF or OOB
reports. Previously, we would return a 250 response and silently drop
the message in this case, which gave the false impression that it was
accepted for relaying.
Expand integration test to explicitly assert that the right things
are allowed/denied/relayed/parsed.
As best as I can tell, this is a casualty of a last moment
code format/copy-pasta. All of the logic works correctly,
but the Reception log record didn't include the relay
disposition so the log_oob or log_arf flag didn't make it
to the logging layer.
This code isn't currently reachable (defaults to false), but allows
for a runtime selection between rustls and openssl-based tls.
refs: https://github.com/KumoCorp/kumomta/issues/8
Previously, you would do either:
`msg:set_meta('queue', 'smart.host.domain')`
or
`msg:set_meta('queue', '[10.0.0.1]')`
to override the effective domain for a message and cause it to be routed
to somewhere other than the recipient domain.
That was OK for basic smart hosting, but limiting when you wanted to use
multiple candidate hosts.
This commit expands the queue config `protocol` field to support
specifying an explicit list of MX hosts that should be used instead.
The integration tests have been migrated away from the old style to this
new style.
While adding plumbing for this, I uncovered an inconsistency between the
queue name generated for the ready queue and the name used by suspension
handling. The inconsistency was introduced in
0842a0fc8b and related work. This commit
resolves it.
The name is passed through to should_enqueue_log_record as an additonal
parameter to make it possible to reason about whether a given record
should get queued for a specific log hook instance.
This is a breaking change, but it can be easily resolved by adding
the name parameter to the `configure_log_hook` call.
These are two different groups of queues, so there are two different
sets of things to manage them.
kcli now has `suspend(-list|cancel)?` and
`suspend-ready-q(-list|cancel)?` subcommands for establishing a
suspension, listing the suspensions and cancelling a suspension
in the scheduled-q and ready-q namespaces respectively.
The names of the ready queues can be derived from the metrics API:
```console
$ curl -s 'http://localhost:8000/metrics.json' | jq .
...
"ready_count": {
"help": "number of messages in the ready queue",
"type": "gauge",
"value": {
"service": {
"smtp_client:source2->(in1-smtp|in2-smtp).messagingengine.com": 0.0
}
}
},
...
```
From the above, `source2->(in1-smtp|in2-smtp).messagingengine.com` is
the name of the underlying ready queue.
We can and probably should add something to `kcli` to make that slightly
easier to review and manage for the operator.
refs: https://github.com/KumoCorp/kumomta/issues/51
This is relatively basic in that it can catch general lua lints,
but doesn't know about the set of functions available to kumo.
It doesn't appear as though selene is able to be extended to
know about those yet; that is tracked by
https://github.com/Kampfkarren/selene/issues/520
I've fixed the couple of lints in our policy and test files
as part of this commit.
You can install and run it like this:
```console
$ cargo install selene
```
Then:
```console
$ selene .
```
This has our first end-to-end integration test that validates
smtp -> source mta -> sink mta -> maildir
and confirms that the message has the right bits inside it.
The end to end test will get refactored into more easily usable
pieces in follow-up commit(s).
Wez Furlong
kayozaki