I realized that we hadn't connected this up to the memory monitor,
so this commit does that.
I'd like to hook this up for every LruCacheWithTtl, but that
requires some more extensive changes.
I'll look that in a follow up commit.
This provides a simpler and easier to use option for dealing with broken
TLS.
The back story here is that sites with broken TLS ideally should result
in a TLS failure and return us to a clear text SMTP session that we
could use for the remainder if we are in opportunistic mode.
In practice, it's not that simple:
* The rustls implementation provides into_fallible() to enable this mode
of operation, but certain handshake errors result in the session being
unusable once the failed handshake has been communicated
* Openssl is perfectly capable of operating this way, but the Rust
ecosystem bindings do not expose the handshake failure in a way that
allows the clear text session to continue
If we wanted to allow the session to communicate with the failed host in
clear text we will need to make a new separate connection for this case.
There is a concern that this will bump a connection counter in a
reputation system implemented on the destination host, but it is
reasonable to wonder if a site that has broken TLS would be sufficiently
sophisticated to employ such a system.
If we did want to allow making a new connection to the same host, it is
a bit awkward to fit into our current mx host connection plan system.
So, with all that in mind, this commit tries a slightly different
approach:
* A new `remember_broken_tls = "3 days"` field in the egress path config
will enable an in-process cache to keep track of the site_names that
advertise STARTTLS and for which we failed to handshake, or failed to
re-EHLO after a handshake.
* That will cause the connection plan to move on to subsequent hosts, if
any. It won't cause us to re-attempt the current host.
* Subsequent connection attempts (which may be immediate if there are
additional hosts in the current session plan) will consult the
broken tls cache for the site, and use that to decide on the
availability of TLS
* If we previously encountered broken TLS, we will effectively treat
the site as not advertising TLS in EHLO, but will generate an
appropriate error message if TLS is Required.
Since we track the breakage in a cache with per-item TTLs, you
can set the duration as appropriate to your situation.
The cache will be cleared when the process is restarted.
There is no sharing of the cache between nodes.
This is a much lower complexity solution than the existing workaround
using TSA automation rules to override the site with Disabled TLS, and
will help to minimize the number of rules for that purpose in the TSA
daemon.
Note that this commit doesn't change any behavior; we can do a followup
commit to replace those TSA rules with this mechanism.
If you have a get_queue_config event handler that raises an error, it
will propagate back to the injector as a transient error, which is the
desired behavior.
However, we didn't ensure that the message was removed from the spool in
that situation, which could lead to duplicate sends of the same logical
message.
This commit makes things more robust in the context of this kind of
local configuration issue:
* QueueManager::insert_or_unwind is now the preferred way to handle
insertion of the message into the queue during reception at the point
of ingress. It will take care to remove the message from the spool
and to adjust the accounting by logging a Bounce record to complement
the Reception that it assumes that its caller has just logged for the
message.
* Both SMTP and HTTP listeners have been updated to use
insert_or_unwind.
* Spool enumeration has been updated to count failed inserts and to
summarize that the failure is serious and that the messages are not
going to go anywhere until the configuration issue is addressed and
the server is restarted. Note that we cannot recover from this while
the server is online because we have to perform a full spool
enumeration in order to re-discover those messages. (We could
potentially build a thing to remember the complete list of messages,
but I'd rather not try to address what is essentially a configuration
deployment validation problem with engineering on this end of things.)
The back story here is that we were investigating a report of
get_egress_path_config sometimes taking a long time (30-60 seconds!) to
complete.
The issue correlates with a large number of TSA entries that
disable the use of TLS for broken sites (>30,000 entries),
and some kind of DNS resolution.
Running the sample of the TSA output through validate-shaping on my
local system can take 30+ seconds to resolve, with a handful of the
entries timing out after 15 seconds (which is 5 seconds timeout on a
query, plus 2 retries of 5 seconds each). In this sort of situation
with a persistently slow resolve on specific domains, the best case
result is therefore 15 seconds, but it could be longer depending on the
ordering of the results and prevailing winds on the upstream dns server.
What's tricky in this situation is that we have to resolve the site
name for each of the entries that have mx_rollup=true enabled in
order to correctly match any given domain when satisfying the
get_egress_path_config event callout. Being lazy with resolution
doesn't help at load time because we're only loaded immediately
prior to invoking this event.
The next logical question to ask is: why do we need to have 30,000+
entries to disable TLS for these broken sites--can't we just retry in
clear text? Well, it's a bit tricky: we're not guaranteed to be able
to continue in clear text after STARTLS fails. If we were to disconnect
and reconnect to the same host immediately after, that's technically
2 connections being made in quick succession to the remote site, and
that might have reputation influencing consequences. I'd like to avoid
adding that sort of reconnection logic without providing a way to
control that behavior, and that is something for a separate change
from this one.
So, looking at the nature of the problematic rule, it doesn't
actually need to be keyed by the site name because there are
not shared limits or constraints that it might make sense to
apply, and in the context of this issue, it's not necessary
to use rollup.
This commit introduces a `SetDomainConfig` action that will
emit a config entry that explicitly sets `mx_rollup=false`
regardless of the value of `mx_rollup` for the defining rule
condition.
When mx_rollup=false, we do not need to resolve the MX
record for the entry when we load the shaping rules,
which eliminates the DNS latency for the aggregate
set of domains with broken TLS.
It would be great if we could automagically fixup the
TSA config database when updating to this version, but
since the rule is an optional part of the suggested
shaping configuration, which may not even be loaded
by a given overall configuration, it's not appropriate
to assume that we should do any fixup.
There's likely a one liner that can be run in sqlite to remove or force
the expiration of the old rules.
We had a user report that launching kumod
post-7fbec8758720dad465d711458f6bf7923b778773 produced a panic about the
spool not being configured.
My supposition is that they were bringing the system up under load
(incoming traffic) and that a new message was being injected before we
had started the spool.
So what we do here is restore the spool_started check, but have it
be satisfied once the startup has been triggered, rather than
on completion of enumeration.
The result is that there is still a small window where we might turn
away a message during startup, but it should be very very small.
This is a bit of a weird one, but the gist of it is that a sequence
of qp encoded words separated by spaces is, per the spec, re-assembled
and the spaces between the encoded words is ignored.
That means that any pair of adjacent encoded words should really
collapsed into a single encoded word that internally includes the
space in order for it to round trip.
To make things a little more complicated, the underlying quoted
printable crate that we're using implements the base qp space rather
than the modified version used by rfc2047 and it will strip trailing
whitespace from the words that are being encoded.
What we do here in this commit is an improvement, but may still
be subject to nuance in the future:
When rebuilding a header value, instead of splitting by space and
emitting a series of encoded words, we will now produce a single
encoded word for the entire header. That word may be split and
wrapped as needed to ensure that the maximum line length is
respected.
These are almost identical to those used by smtp, the main difference
being that we don't include the Received header by default for
http injection, whereas we do for smtp in order to be compliant
with the relevant rfcs. No such rfcs exist for the injection
protocol, so we can pick a default that makes more sense for the
majority of our target audience.
Problem scenario: you have defined an automation rule with the
unanchored regex `detected an unusual rate` that triggers a suspension.
What happens: when the remote host initially responds and that
rule matches, a suspension will be created. When subsequent
messages are checked against that rule, the message will not
be attempted (good), but we will generate and log a transient failure
response of the form: `KumoMTA internal: suspended, rule: detected an
unusual rate`. When that response is presented to TSA, since the regex
is unanchored, the original rule will also match it, and that will
cause the duration to be reset, effectively perpetuating the suspension
until the combination of ingress and the retry window exceeds the
duration specified in the rule (bad).
You can explicity mark up your regex with an anchor to prevent
this sort of matching, but it is a PITA to do that for every
suspension rule.
This commit resolves this issue by recognizing that the default
should be to not match internally generated rules. The newly
introduced `match_internal` boolean defaults to false and controls
whether we should consider these internal messages, and allows
the matching to be turned on when needed. We use this in one
of the default shaping rules that is used to disable the use
of tls for broken hosts.
The original design of the spooling layer didn't require that
the SpoolId have the creation time encoded within it, which
meant that spool enumeration could consider any and all messages
found in the spool and import them into the queue subsystem.
If we allowed reception of new messages and wrote them to the spool
concurrently with the enumeration process, it would be possible
for the enumerator to observe the newly received messages and
import them into the queue subsystem, even though we had already
placed those newly received messages into the queue subsystem.
The result would be that we might send an additional copy of
each of the messages observed in this way.
To defend against that, the system refused to accept new messages
until spool enumeration was complete.
However, for sites with large spools, the enumeration process could
take some time to complete which could present challenges for
deploying updated configurations without an impact to their
service uptime.
This commit tackles that issue:
* Enumeration now filters out any messages that we created at or
after the start of the enumeration process, so it is not possible
for the duplicate scenario to occur.
* We no longer keep global track of whether spool enumeration is
in progress, but will still log that progress to the diagnostic
log.
* The liveness checks no longer check whether spool enumeration
is in progress.
A potential consequence of this change is that the concurrent writes
to the spool may further reduce the speed at which enumeration
operates, but that's a reasonable trade.
A number of our lua event handlers allow registering multiple
implementations to facilitate modular use.
For that to work, we must know ahead of the user lua call running
that any given handle is allowed to register multiple times. This is so
that we can report a meaningful error when incorrectly using a singleton
handler multiple times, and so that we can record a list of handlers
for the multiple case.
Prior to this commit, if we forgot to arrange to register the signature
as part of the context setup the consequence was that the event handler
would get registered as a singleton and when we went to call it, because
the signature is marked as allowing multiple but was not registered
as multiple, we would skate through and do nothing without reporting
an error because we assumed that the signature was registered
consistently.
In hindsight, that's a terrible idea because it results in silently
ignoring the registration issue, and not calling the event handler
at all.
This commit consolidates the multiple/single value resolution into
the same flow, then adds a check to confirm that we have a list
of handlers registered for the allow_multiple case, raising an
error otherwise that will hopefully encourage users to report
this problem to us if it manifests again in the future.
This commit includes fixing two event handlers that we missing
their signature registration.
One of them was broken anyway by being registered with a name
that didn't match the docs.
refs: https://github.com/KumoCorp/kumomta/issues/236
This plumbs the smtp response through to the requeue_message event
handler.
While hooking this up, I noticed tha the registered named of the
event was `message_requeued` instead of `requeue_message`. That
name was from the original implementation of the event, and it
just got overlooked when the rest of the references to its
name were updated.
closes: https://github.com/KumoCorp/kumomta/issues/236
Two issues here:
1. The maintainer would repeatedly try to shrink the ready queue
each time it woke up during a low memory condition. This has
been fixed to trigger whenever we transition to low memory
instead.
2. At some point during refactoring, we dropped the save part
of the message shrinking processing, so we'd only successfully
shrink messages that had previously been saved and stored to
spool. Messages that we deferred spooling, or otherwise modified
post reception, would not be saved and thus not be eligible to
shrink.
Ideally we'd have an integration test for this, but it is a bit awkward
because we'd need to contrive an appropriate ulimit for just this
instance and generate appropriate load to trip over that limit. I don't
fancy the chances that such a test wouldn't be flakey.
My ad-hoc test procedure was:
ulimit -m 2028527
./target/release/kumod --policy simple_policy.lua
then in another window:
./target/release/traffic-gen --target 127.0.0.1:2025 --body-size 100000 --duration 3600 --throttle 15000/s --http
the generator will eventially experience load shedding http responses,
and you can see the shrink procedure triggering in the kumod output.
curl -s 'http://127.0.0.1:8000/metrics' | grep memory
can also be used to check the usage, limit and how many times it trips.
You can also use smtp for this, but the smtp client in traffic gen will
try hard to reconnect without telling you about the shortage, so you
will need to look at the metrics to see it happening.
At the lower level, expose an options struct that allows control
over how various checks and conditions are reported out of the
attempt to load the set of shaping files.
Expose a separate list of errors, distinct from warnings.
Each check can either be ignored, a warning, or an error.
Errors cause validate-shaping and --validate mode to exit
with an error condition, whilst warnings are simply emitted
as informational items.
In the shaping helper, it is possible to configure a separate set of
options for the main live service and validation mode, which allows you
to run a more relaxed configuration by default, but be a bit more strict
in your pre-commit and pre-deploy configuration validation pipeline
refs: https://github.com/KumoCorp/kumomta/issues/287
This commit addresses a couple of related issues around scheduled qeueue
suspensions:
1. There was no check in the ready queue logic to confirm that a
given message was not part of a suspension. Ideally, it wouldn't
land in the ready queue if it is suspended, but if you have a large
ready queue and one of the messages generates a suspension, then
the remainder would get attempted, oblivious to the new suspension.
The resolution here is to add a check for that case, log a transfail
and requeue the message.
2. We only checked whether the scheduled queue was suspended in the
case where a message was being newly inserted into the queuing
system. Importantly, messages being promoted from the scheduled
queue didn't use this code path. This commit fixes this up by
relocating the check to the appropriate location. In addition,
we now will log a transfail for this case and delay the message
according to its retry schedule.
3. Since we're in here changing the retry schedule for suspensions,
take the opportunity to take care of #293 which applies to the
more general logic around all sources being suspended.
The upshot of this is that we're now logging transfails in a number
of suspension cases where we weren't previously, and using the normal
retry schedule for those cases where we weren't previously doing
that either.
refs: https://github.com/KumoCorp/kumomta/issues/290
refs: https://github.com/KumoCorp/kumomta/issues/293
* When using very small file size or duration constraints, we might
attempt to create multiple file segments in the same second. Since
we require exclusive creation access to the log file name, the
subsequent attempts to open the segment would fail with a permission
denied error and cause the associated log record to be dropped.
Add the number of fractional seconds to the log file name to
avoid this.
* When using small durations and low traffic, we wouldn't expire log
files until we had processed 10k records. That's not so bad in
a production setting, but in the test harness it is problematic.
We now check for expiration as part of the file size check so
that we will prune a segment that is at its time limit.
I don't expect either of these conditions to crop up and matter
in a production setting.
I've added a note to the breaking changes section of the changelog
about the addition of the fractional seconds to the log file name.
I don't expect that to impact anyone in practice either, but I
wanted to call it out as a potential difference in case someone
is using a very precise regex/glob to match the file names.
Failure to connect to a proxy server will now include more context
about the proxy server and protocol in the error message, and
will bump a counter.
Failure to directly bind a source address for the outgoing connection
will bump a counter.
refs: https://github.com/KumoCorp/kumomta/issues/286
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.
Dirkjan Ochtman
Wez Furlong
Mike Hillyer
Laurent Marchaud