The suspension logic changed things so that we'd trip over a DNS
resolution earlier than normal, inside the round robin routine.
That isn't capable of reporting the domain resolution issue and
it would bubble up as NoSources and we'd miscategorize the failure.
This commit addresses that by allowing the round robin to pick a
source and then we can subsequently bubble up and handle the domain
resolution issue as normal.
Previously we'd just use the default error case and retry 1 minute
later, but for sites with broken tls where we can never make a
connection without changing our configuration to skip tls, we don't
want to keep retrying every minute and instead want to follow the
normal queue exponential backoff.
Some sites with broken tls can produce the following transient failure
log:
```
KumoMTA internal: failed to connect to any candidate hosts: EHLO: Not connected, EHLO: Not connected, EHLO: Not connected, EHLO: Not connected
```
which is very thin on actual context.
This commit adds in the useful context!
refs: https://github.com/KumoCorp/kumomta/issues/67
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 .
```
Previously, an authentication issue could manifest as an opaque and
confusing error message like
```
Error: error decoding response body: expected value at line 1 column 1
```
now we show it like:
```
Error: request status 401: Unauthorized. Response body: Missing Authorization header
```
There were a few cases where client operations weren't wrapped
up in a timeout, which is a potential cause of hanging on shutdown:
if we're waiting an unbounded amount of time for a connection to
do something before we can shutdown, then we'll never finish.
The current timeouts struct really encodes the read timeout for
waiting for the result of a command.
What we also need is a duration to wait for sending data. We're
using the smaller of the per-command read timeout or 1 minute for
this.
I've found the right combination to allow seeing the user-specified
log filter in the normal log output, while allowing tokio-console
to start with the appropriate filters when the `--tokio-console`
switch is used to start kumod.
In the future we could consider making this configurable.
For now, this does a reasonable job at getting messages
from the spool and into the queues to unblock receiving
new messages.
The degree to which this helps varies based on the number
of different scheduled and ready queues that arise from
the messages in the spool and the active configuration.
While spool enumeration is ongoing, we'd keep an exclusive lock
on the spool manager.
If we'd subsequently need to remove a message from spool, we'd
attempt to obtain a lock on the spool manager to be able to reference
the spool.
In the best case, the removal logic would be triggered from a separate
task, and would eventually succeed once enumeration completes.
However, if the calling context holds a lock on a schedule queue
then we'd have potential to deadlock, especially in the case where
we are processing an admin bounce request via the HTTP API which
needs to iterate all queues, lock them and then initial the bounces
and associated removal of the messages from the spool. Holding a
scheduled queue lock would prevent forward progress in the spool
enumeration logic which also needs to lock that queue to insert
newly enumerated messages.
In many cases this issue would not be noticed until attempting
to shutdown the server where it would manifest as waiting forever
to shut down.
This commit removes the possibility of deadlock by adjusting
the composition of the SpoolManager. Now, instead of needing
to lock the entire spool manager, the locks are scoped to the map
that manages the named spools; the critical section for those is
now just long enough to obtain a handle.
While examining the flow of this stuff I realized that the admin bounce
processing was overly aggressive and would bounce all the messages in
the ready queues associated with the matching scheduled queue. That's
incorrect as the ready queues are potentially shared with other
scheduled queues can feed into those same ready queues.
I adjusted the logic for those so that we process the admin bounce
entry as part of dispatching the ready queue instead of trying to
bulk fail the ready queue.
Check for shutdown in a couple more places, and adjust the connection
timeout logic to give up on establishing a new connection as soon as we
detect a shutdown.
We were previously only re-doing the EHLO in the success case, but there
are some receiving implementations that seem to roll the TLS failure
error forward, returning an error during MAIL FROM:
```
TLS connection failed: error:1408A0C1:SSL routines:ssl3_get_client_hello:no shared cipher (#4.3.0)
```
we want that error to show up while we're considering the connection, so
that we can fail over to the next candidate in our list, rather than
having it generate a transient failure for the message and stopping
there.
refs: https://github.com/KumoCorp/kumomta/issues/67
Previously, we'd hold out for a different, working, host in the
set of available hosts, but when in OpportunisticInsecure mode
we're willing to send to any of the hosts that is available,
regardless of the state of their TLS. So this commit should
allow us to proceed in clear text on the existing connection
in that case.
refs: https://github.com/KumoCorp/kumomta/issues/67
Commit f3be872791 was only an issue
because we had essentially two copies of the logic to insert
into the ready queue.
This commit refactors the second location to work in terms of the
first.
In doing so, I think this also fixes an issue with queues configured
for non-SMTP delivery, such as lua delivery implementations for
webhooks/AMQP in the delayed->ready case. I think this explains
some weirdness that Tom has seen in the past when playing with
webhook logging.
Relatively serious: if none of the resolved hosts were reachable,
the message that initiated that attempt would get logged as a transfail
but we'd then forget about it until a restart when we'd re-discover
it from the spool.
This is resolved by not taking the message from the dispatcher and
instead leaving it in place so that its centralized Drop processing
will requeue it to the scheduled queue.
This should have been part of df4e571b51.
This case is for migrating from scheduled to ready when delayed
messages are ready, whereas the other one was newly received mail
that was ready immediately.
refs: https://github.com/KumoCorp/kumomta/issues/65
Tom "Edge Case" Mairs found an issue where what I think happened
was that the DNS records changed since we first established our
own internal data structures.
When the new set of records doesn't resolve to any A or MX entries
we'd return the same empty list of addresses that we used to signal
a null mx and incorrectly fail the remainder of that queue.
This commit introduces a separate state for the null mx state so that
we can instead delay the queue.
Wez Furlong
Mike Hillyer
Tom Mairs