Removes the deref to str as this is now a fallible operation that
can panic.
Process the fanout of this change, which is dealing with more
possibility that things might be binary in more places.
There are some transitional APIs introduced as deprecated so that I'm
nagged to remove them already: I want to get the bulk of the code aware
of the possibility of binary sorted before I tackle adjusting the
message parser for that, so that that particular commit is more
self-contained.
This is a step towards allowing non-UTF-8 data to be tracked
in the SharedString type.
It is not complete; some tests now panic with UTF-8 input where
they would previously implicitly perform a lossy conversion.
That will be resolved in subsequent commit(s).
It was pointed out that we had this policy and that it wasn't part of
the spec.
That behavior was imported along with the crate when we forked it from
the cloudflare implementation.
Let's remove it; folks that need it can use a couple of lines of lua to
recreate it if they need it.
This is similar the prior two commits; this one summarizes the case
where socks5 is in use and the bind(2) fails on the proxy server,
indicating unplumbed addresses on the server.
We cannot do the equivalent with HA proxy as its protocol has
no provision for this sort of feedback.
Similar to how we summarize and inform when all connection errors
are due to broken TLS, recognize and inform when all connection
errors are due to a source_address that cannot be bound due to
the address not being plumbed on the system. This does not
include other reasons that `bind(2)` can fail, just the case
where the address is not present.
Add an integration test to capture the error message for
that situation, for easier future reference.
These utilities would simplistically assume that the input domain
was always a valid DNS name, which is true for the majority of
things, but our internal webhook, TSA and xfer queue names include
http URLs and would choke these utilities.
We now fall back to using the input "domain" if the MX resolution
attempt throws an error; the error is typically a failure to parse
rather than something like NXDOMAIN which is generally returned as an
empty result.
This is a follow on from the previous commit; apparently some sites
will enclose addresses in <> even though this is not covered in the
RFC.
Let's cut the address parsing over to the SMTP parser rather than
the mail header parser, as they have rather different semantics
and both ARF and OOB are SMTP-centric.
We had a report that the enclosing <> were being passed through
to the JsonLogRecord in some log hook flows, which meant that
we'd try to parse them as EnvelopeAddresses and fail.
This dumps out a trace of all tokio tasks. It is quite expensive,
and currently unsettles the tokio runtime such that you need to
repeatedly call this endpoint in order for a subsequent graceful
shutdown to clock through and complete.
In our thundering herd protection we intentionally limit the number of
pending lookups for a given key to just 1. That is controlled via
a semaphore.
We have pre-existing logic that checks to see if the semaphore has been
closed (eg: by a task being cancelled), but despite this, we do
occasionally have reports from users with `timed out after 120s on
semaphore acquire while waiting for cache to populate` for the shaping
data cache.
This typically correlates with eg: long IO waits due to eg: a spike in
transient failures and usually some sort of logging of headers, but not
always.
The not always situation bothers me as we don't currently have a good
explanation of what might be causing the excessive delay.
This commit adds an additional sanity check: when a new lookup is
initiated, if the semaphore is still open (not cancelled), but has been
open for longer than the populate timeout, then we treat this similarly
to the semaphore being closed: we'll create a new semaphore, reset the
expiration and the caller will typically then promote itself to the task
that will satisfy the lookup.
The side effect of this all waiters should become unblocked and awake
either with an error status or with the result of the new lookup, which
should help to clear any persistent/recurring blocking state associated
with this.
This change doesn't provide more insight (I have something in mind for a
follow-on commit for that), but should help make forward progress.
Continuing thread from prior commit, improve the context we
show during shutdown.
Take the opportunity to switch Mutex to parking_lot which is
slightly faster and doesn't have the warty unwrap() API
surface.
We may log things like this during shutdown:
```
logging-3 kumod::logging::hooks: Error while calling should_enqueue_log_record: shutting down
```
but it isn't clear which part of the operation is sensitive to the
shutdown situation, so adding some context may help focus attention.
We had an issue where a typo resulted in a relatively inscrutable
error at runtime:
```lua
local queue_helper = queue_module:setup ('/opt/kumomta/etc/policy/queues.toml')
```
produced this:
```
problem initializing: call validate_config callback: runtime error: /opt/kumomta/share/policy-extras/queue.lua:602: bad argument #1 to 'for iterator' (table expected, got nil)
stack traceback:
[C]: in function 'next'
/opt/kumomta/share/policy-extras/queue.lua:602: in function </opt/kumomta/share/policy-extras/queue.lua:551>
Error: Initialization raised an error: call validate_config callback: runtime error: /opt/kumomta/share/policy-extras/queue.lua:602: bad argument #1 to 'for iterator' (table expected, got nil)
stack traceback:
[C]: in function 'next'
/opt/kumomta/share/policy-extras/queue.lua:602: in function </opt/kumomta/share/policy-extras/queue.lua:551>
```
with the changes in this commit we'll present this issue like this,
during server startup, which points a little more clearly at the setup
call and the file names parameter, and suggests more strongly that it
should be a list of strings (or config objects):
```
runtime error: [string "./simple_policy.lua"]:52: assets/policy-extras/queue.lua:463 QueueHelperSetup: invalid value for field 'file_names'
assets/policy-extras/queue.lua:463 Expected value of type 'list<variant<string,QueueHelperConfig>>' but got type 'string' '/opt/kumomta/etc/policy/queues.toml'
stack traceback:
[C]: in function 'error'
assets/policy-extras/typing.lua:78: in method 'raise'
assets/policy-extras/typing.lua:249: in metamethod 'newindex'
assets/policy-extras/typing.lua:258: in function <assets/policy-extras/typing.lua:253>
(...tail calls...)
assets/policy-extras/queue.lua:463: in function 'policy-extras.queue.setup_with_options'
(...tail calls...)
[string "./simple_policy.lua"]:52: in main chunk
```
this change actually surfaced a minor issue in the ndr.lua file that is
part of an integration test, as well as in my adhoc simple_policy file.
If you have two scheduled queues with the same mx_list and then one of
those domains is updated to use a different mx_list, depending on the
ordering of the updates and subsequent traffic, the changed domain may
continue to have traffic route through the old mx_list.
The reason for this is that protocol configuration for that egress path
comes from the scheduled queue configuration, and there is no explicit
dependency between the two pieces of configuration.
This commit resolves this by adding a sanity check that the protocol
information is consistent with the ready queue instance. If it is no
longer consistent we'll remove the association between the rq name and
the instance, then wind down any associated dispatchers.
It was reported that AWS SES could mark the DKIM signature as failed
when using a specific combination of headers in the header list,
with specific lengths of the other fields.
The reason for this is that we had two passes of wrapping applied
to the header, and they might not agree on the formatting of
the header.
The solution is to remove the second pass and just take a bit more
care to emit the header in a wrapped form in the first instance,
that way there can be no discrepancy or conflict.
closes: https://github.com/KumoCorp/kumomta/pull/483
As part of this, we move template compilation to happen before
we queue up deferred generation; the compilation step should
be plenty fast enough that we can reasonably do that synchronously
and report any compilation errors back to the peer.
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
Wez Furlong
Mike Hillyer
Mike Hillyer
dependabot[bot]
Raphael Sprenger
Aditya Ganti