At some point turning off most features used to make things work in our
environments, but looking at things today, I think it basically makes
it do nothing useful any more.
Turning things off just made the image build a hair faster and the
image a hair lighter.
So let's just turn it back on
In reviewing some live logs, I could see that some timeout errors
were including the debug repr of the SMTP command, which is too verbose
to be useful in the error log line, so let's tidy that up a bit
here to make it easier to understand.
These functions are similar to the existing
invoke_get_egress_path_config and invoke_get_queue_config functions;
they return the configuration information that is defined by your
overall policy in the same way that the system does.
The intent is for more advanced policies that need that info to
decide how to configure other parts of the policy.
Docs are included, and I fleshed out the missing docs for the two
pre-existing functions.
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.
For a sequence like `[BASE64 IP: IPADDR]` we were not previously
able to see the base64 or IP address inside the square brackets.
This commit adds a pre-processing step to strip out parentheticals
surrounded by spaces (or the end of the string) so that our existing
tokenization can take effect.
I spent a few hours today trying a few different matching strategies,
rewriting this crate 3 different ways to try to simplify the logic and
lean more heavily on the regex or fancy_regex crate so that we can avoid
special cases. While they all worked, they all were over an order of
magnitude worse in terms of throughput and performance.
The strategy here is to perform a very cheap SIMD-assisted test to see
if we need to pre-process the string so that we can avoid the overhead
of the additional string allocation and full processing step.
The result is that the performance before and after this change is very
close to neutral when the response contains no brackets, but 33% slower
(and 50% throughput) when brackets are present. That's still 180MiB/s
on my threadripper which is still decent for this application.
While grubbing around in here today, I noticed that a couple of regexeps
had typos: `(:?` when they should have been `(?:`, and I annotated more
sub-expressions to be non-capturing as well.
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.
Saw an MX record that expands to 0.0.0.0 in the wild.
As well as being nonsense, that causes a mail loop, so we should add it
to prohibited_hosts.
I noticed that we also did not document its default value, so the
doc update here shows the before and after for this.
This engine does no templating; the source text is output exactly
as it was entered into the template engine.
Expose template_dialect to the http injection API and add a test
to show that selecting the Static dialect doesn't perform any
template expansion.
refs: https://github.com/KumoCorp/kumomta/issues/446
This is primarily a refactoring of kumo-template to conceptually
support the idea of having multiple dialects, even though it
only defines a single dialect.
It prepares the types/interface for that purpose.
It also pulls in the handlebars crate, but doesn't use anything
from it yet; that'll be in a follow up commit.
refs: https://github.com/KumoCorp/kumomta/issues/446
Continuing in the same vein as 94628f06ab,
ensure that we have a reasonable outcome when a batch of size > 1
encounters 100% ambiguous 452 responses.
Without special consideration we'd end up in an infinite retry loop for
these, as we'd think that we'd hit the recpient limit on the connection
every time.
This commit introduces a small amount of state into the dispatcher so
that we can remember the set of (spoolid, recipient) from the prior
transaction on the session.
When deciding whether something is eligible for immediate retry
we will gate that choice on the current (spoolid, recipient) being
absent from the prior set of ids. In this way, we bound the immediate
retry for a recipient to one per connection.
To make things a bit more robust when trying to decide whether a 452
means "too many recipients" or not, if we're not sure, we now call out
to the bounce classifier which can be configured to emit a
`TooManyRecipients` classification. If the classifier produces either
that or some non-Uncategorized response, then we have a confident
assessment. Otherwise, we're unsure and will factor that into the
retry strategy.
The multi-recipient send logic introduced a regression in `main` that
could cause messages like "452 4.1.1 mailbox full" to be considered
immediately retryable always, because we thought it meant "too many
recipients" in this batch.
This commit fixes up the immediate retry logic to only apply when we
have a batch size > 1, and we try to be a bit more discerning with the
enhanced status code when it is present.
This issue isn't being mentioned in the changelog as a breaking change,
as the breakage is vs. an earlier `dev` build and not a prior stable
release.
in 18e55a6e91 I noticed that we should be
handling 552 as a transient failure, but I neglected to mention this
change in the changelog because it seemed obscure and "couldn't" hurt
anyone.
I overlooked that we have client response rewriting and of course this
change did impact one of our sponsors that rewrites certain transient
452 responses to 552 responses.
So, let's add a note explaining this to see if we can prevent others
from hitting it in the future.
We had a sponsor report an issue where we'd log an error during
processing a log hook. It transpired that the recipient list we were
attempting to log was `["metadata is not loaded"]`, which doesn't parse
as a valid EnvelopeAddress (see prior commit:
52e2f95e9f)
This commit fixes up the recipient_list logic in the logging function to
avoid taking the error message as the recipient list, and moves the
metadata loading to happen just before we try to read the recipient
list.
This should work around the issue, but a consequence is that we might
be holding on to the metadata for that message in RAM for longer than
we should; will need to do some thinking about managing the resident
status in this situation better.
This aggregates the various authentication-results producing auth checks
into a more convenient function.
Part of this change improves some plumbing in the dkim checking (we no
longer raise an error for a missing From, but instead indicate a failed
dkim result), and we now support passing down a resolver name to the
various checking functions, to facilitate testing and other more
advanced use cases.
refs: https://github.com/KumoCorp/kumomta/issues/16
refs: https://github.com/KumoCorp/kumomta/issues/84
This commit adds some plumbing to allow us to collect metadata
about the caches and spit it out into a json file that we can
then use in the docs to show the predefined caches, capacities
and brief comments about their purpose.
We put that info on the set_lruttl_cache_capacity doc page.
Wez Furlong
crxandWez Furlong
dependabot[bot]
Sophia Turner
